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 panel driver for ChipWealth CH13726A based panels Date: Wed, 25 Mar 2026 07:40:21 +1000 Message-ID: In-Reply-To: <20260323-ch13726a-v3-2-e28b6f97fe80@gmail.com> References: <20260323-ch13726a-v3-0-e28b6f97fe80@gmail.com> <20260323-ch13726a-v3-2-e28b6f97fe80@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review Several issues: 1. **Unnecessary and incorrect Kconfig selects.** The Kconfig entry selects= `DRM_DISPLAY_DP_HELPER` and `DRM_DISPLAY_HELPER`: ``` + select DRM_DISPLAY_DP_HELPER + select DRM_DISPLAY_HELPER ``` This is a MIPI-DSI panel, not a DisplayPort panel. The driver doesn't use a= ny DP helpers. These selects should be removed entirely =E2=80=94 no replac= ement is needed since `drm_mipi_dsi.h` and `drm_panel.h` don't depend on th= em. 2. **`prepared` flag is deprecated.** The driver manually tracks `prepared`= state: ```c + bool prepared; ``` ```c + if (ctx->prepared) + return 0; ``` The DRM panel framework has handled prepare/unprepare idempotency since com= mit work in recent kernels. Only 2 legacy drivers in the tree still use thi= s pattern. Remove the `prepared` field and the guard checks in `ch13726a_pr= epare()` and `ch13726a_unprepare()`. 3. **`of_device_get_match_data()` cast drops const.** The probe function ca= sts away const: ```c + ctx->desc =3D (struct ch13726a_desc *)of_device_get_match_data(dev); ``` The `desc` field in the struct should be `const struct ch13726a_desc *desc`= and the cast removed. Similarly, `thor_bottom_desc` should be `static cons= t`: ```c +static struct ch13726a_desc thor_bottom_desc =3D { ``` Should be `static const struct ch13726a_desc thor_bottom_desc`. 4. **`uint8_t` is not kernel style.** The kernel uses `u8` not `uint8_t`: ```c + for (uint8_t i =3D 0; i < ctx->desc->num_modes; i++) { ``` Should use `unsigned int i` (since `num_modes` is `unsigned int` and the va= lue is small, there's no reason to use `u8` here anyway). 5. **Reset GPIO polarity may be inverted.** The reset sequence does high-lo= w-high: ```c + gpiod_set_value_cansleep(ctx->reset_gpio, 1); + usleep_range(10000, 11000); + gpiod_set_value_cansleep(ctx->reset_gpio, 0); + usleep_range(10000, 11000); + gpiod_set_value_cansleep(ctx->reset_gpio, 1); ``` The GPIO is obtained with `GPIOD_OUT_LOW` (meaning initially deasserted). T= ypically a reset is assert-deassert (1-0-1 asserts then deasserts), but the= dt-binding example uses `GPIO_ACTIVE_HIGH`, which means `gpiod_set_value(1= )` drives the pin high. This is plausible for an active-high reset, but wor= th confirming the hardware =E2=80=94 most AMOLED panels use active-low rese= t where the sequence would be 0-1-0 (assert low, release high). If the GPIO= is active-high reset, then the unprepare path sets reset to 0 (deasserted/= released), which means the panel leaves reset deasserted during power-down = =E2=80=94 that's backwards. Usually you want to assert reset *before* cutti= ng power. 6. **`ch13726a_disable()` clears `MIPI_DSI_MODE_LPM` but `ch13726a_on()` se= ts it.** In `ch13726a_disable()`: ```c + ctx->dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM; ``` This means the display-off and sleep-enter commands are sent in HS mode, bu= t the init sequence sends commands in LP mode. This is fine if intentional,= but the backlight handler also toggles LPM: ```c + dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM; + ret =3D mipi_dsi_dcs_set_display_brightness(dsi, brightness); + ... + dsi->mode_flags |=3D MIPI_DSI_MODE_LPM; ``` Toggling `mode_flags` like this is racy if backlight updates can happen con= currently with enable/disable. Consider setting the mode flags once in prob= e and not toggling them per-operation. 7. **Missing `#include ` guard.** The driver includes `` but could use `` instead for `of_device_id`,= matching newer driver conventions (see `panel-boe-td4320.c`). Minor style = point. 8. **`ch13726a_remove()` error check on detach is unnecessary boilerplate.*= * `mipi_dsi_detach()` return value logging in remove is acceptable but some= maintainers prefer to just ignore it since there's nothing useful to do wi= th the error in the remove path. --- Generated by Claude Code Patch Reviewer