From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/panel: Add support for Tianma TA066VVHM03 panel
Date: Mon, 09 Mar 2026 07:59:03 +1000
Message-ID: <review-patch2-20260308-tianma-ta066vvhm03-v1-2-869fac443b20@pm.me>
In-Reply-To: <20260308-tianma-ta066vvhm03-v1-2-869fac443b20@pm.me>
References: <20260308-tianma-ta066vvhm03-v1-0-869fac443b20@pm.me> <20260308-tianma-ta066vvhm03-v1-2-869fac443b20@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

**1. Build dependency on unmerged `slice_per_pkt` field (blocking):**

The probe function sets:
```c
dsi->dsc_slice_per_pkt =3D 2;
```

This field (`dsc_slice_per_pkt`) does not exist in `struct mipi_dsi_device`=
 in the current kernel tree. The cover letter notes this depends on a patch=
 currently under review. This patch cannot be applied or built until that d=
ependency is merged. The series should either explicitly note this with a `=
Depends-on:` tag or be resubmitted once the dependency lands.

**2. Missing cleanup in `prepare` error paths (bug):**

In `tianma_ta066vvhm03_prepare`, the error paths after `mipi_dsi_picture_pa=
rameter_set()` and `mipi_dsi_compression_mode()` just return the error with=
out disabling GPIOs or regulators:

```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, enable_gpio, reset_gpio */
}

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 */
}
```

Compare to the earlier error path after `tianma_ta066vvhm03_on()` which cor=
rectly cleans up. These later error paths need the same treatment: assert r=
eset, deassert enable, and disable regulators. Consider using a common `got=
o` label.

Also note the inconsistent `dev` pointer usage: the PPS error message uses =
`panel->dev` while all other messages use the local `dev` variable. Use `de=
v` consistently.

**3. LPM mode toggling in `_off` (questionable):**

```c
static int tianma_ta066vvhm03_off(struct tianma_ta066vvhm03 *ctx)
{
    ...
    ctx->dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM;

    mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
    mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
```

This clears LPM, meaning display-off and enter-sleep commands are sent in H=
S mode. Most panels send shutdown commands in LP mode. This may be correct =
for this specific panel (vendor DT generated), but it's worth double-checki=
ng since it's unusual.

Similarly, the backlight callback toggles LPM off for brightness writes the=
n back on:
```c
dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM;
ret =3D mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
...
dsi->mode_flags |=3D MIPI_DSI_MODE_LPM;
```

This is a common pattern in generated drivers but is inherently racy if mul=
tiple threads could call into the DSI driver simultaneously.

**4. Clock/refresh rate:**

The pixel clock calculation gives:
```c
.clock =3D (1080 + 24 + 4 + 10) * (2340 + 12 + 1 + 4) * 160 / 1000,
```

This yields ~421 MHz and ~160 Hz refresh rate. For a gaming phone (ROG Phon=
e 3) this seems plausible, but should be verified against hardware document=
ation. The 160 divisor is the target refresh rate =E2=80=94 is 160 Hz confi=
rmed for this panel?

**5. MAINTAINERS entry is fine.** Kconfig/Makefile additions are properly s=
orted alphabetically.

**6. Minor:** The `_reset` function uses `GPIOD_OUT_HIGH` at probe (assert =
reset), then in the reset sequence does 0=E2=86=921=E2=86=920. With the act=
ive-low polarity from the DT (`GPIO_ACTIVE_LOW`), this means physical low=
=E2=86=92high=E2=86=92low. This should be verified against the panel datash=
eet reset timing requirements.

---
Generated by Claude Code Patch Reviewer