From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/panel: Add support for Tianma TA066VVHM03 panel Date: Wed, 11 Mar 2026 13:40:04 +1000 Message-ID: In-Reply-To: <20260310-tianma-ta066vvhm03-v3-2-fc2938971d79@pm.me> References: <20260310-tianma-ta066vvhm03-v3-0-fc2938971d79@pm.me> <20260310-tianma-ta066vvhm03-v3-2-fc2938971d79@pm.me> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Issue 1 (bug): Missing cleanup in `prepare()` error paths for PPS/compres= sion failures** When `mipi_dsi_picture_parameter_set()` or `mipi_dsi_compression_mode()` fa= ils, the function returns without undoing the work done by `tianma_ta066vvh= m03_on()` (which sends DSI commands to the panel), without deasserting the = reset/enable GPIOs, and without disabling regulators. Compare with the erro= r path after `tianma_ta066vvhm03_on()` fails just above, which correctly cl= eans all of these up. ```c ret =3D mipi_dsi_picture_parameter_set(ctx->dsi, &pps); if (ret < 0) { dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); return ret; /* <-- leaks regulators, GPIOs, panel state */ } ret =3D mipi_dsi_compression_mode(ctx->dsi, true); if (ret < 0) { dev_err(dev, "failed to enable compression mode: %d\n", ret); return ret; /* <-- same leak */ } ``` Additionally, the `_multi` variants (`mipi_dsi_picture_parameter_set_multi`= and `mipi_dsi_compression_mode_multi`) are available in the current tree. = The `prepare()` function could use a multi context throughout to simplify e= rror handling =E2=80=94 or at minimum, the error paths need to call the sam= e cleanup as the `_on()` failure path (reset gpio, enable gpio, regulator d= isable). **Issue 2 (minor): Inconsistent `dev` variable usage in `prepare()` error m= essages** The PPS error path uses `panel->dev` while all other error paths use the lo= cal `dev` variable: ```c dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); ``` vs: ```c dev_err(dev, "failed to enable compression mode: %d\n", ret); ``` Both refer to the same device, but using `panel->dev` here is inconsistent = and unexpected. Should use `dev` for consistency. **Issue 3 (dependency): `dsc_slice_per_pkt` field does not exist upstream** ```c dsi->dsc_slice_per_pkt =3D 2; ``` This field doesn't exist in `struct mipi_dsi_device` in the current tree. T= he cover letter correctly documents this dependency on a pending patch. Thi= s will prevent the driver from compiling until that dependency is merged. T= his is fine as long as the maintainer is aware and merges them in the right= order. **Issue 4 (minor nit): `WARN_ON` for a compile-time-knowable condition** ```c WARN_ON(1080 % ctx->dsc.slice_width); ``` Since `slice_width` is set to 540 just above, this is always false (1080 % = 540 =3D=3D 0). This is a runtime check for what is effectively a compile-ti= me constant. It's harmless and used by other panel drivers (presumably a pa= ttern from the generator), so no action strictly needed. **Observation (style): The driver closely follows the `panel-raydium-rm692e= 5.c` pattern** The overall structure, probe sequence, backlight handling, DSC setup, and r= emove function match the established patterns. The use of `devm_drm_panel_a= lloc`, `devm_regulator_bulk_get_const`, and `drm_connector_helper_get_modes= _fixed` are all current best practices. Good. **Summary for Patch 2:** The main issue to fix is the missing cleanup in th= e `prepare()` error paths after PPS/compression mode failures. Consider usi= ng the `_multi` variants for PPS and compression mode to simplify the contr= ol flow and ensure consistent error handling. Also fix the `panel->dev` vs = `dev` inconsistency. --- Generated by Claude Code Patch Reviewer