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: Thu, 23 Apr 2026 08:16:54 +1000 Message-ID: In-Reply-To: <20260421-ch13726a-v5-2-f7f6f4f30e05@gmail.com> References: <20260421-ch13726a-v5-0-f7f6f4f30e05@gmail.com> <20260421-ch13726a-v5-2-f7f6f4f30e05@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 **Issue 1 (minor): Unnecessary Kconfig selects.** ```c + select DRM_DISPLAY_DP_HELPER + select DRM_DISPLAY_HELPER ``` This is a MIPI-DSI panel driver. It has no DisplayPort functionality. The o= nly other panel drivers that select `DRM_DISPLAY_DP_HELPER` are eDP panels = (Samsung ATNA33XC20 and the eDP panel driver). These selects should be remo= ved. **Issue 2 (minor): `uint8_t` loop variable in `ch13726a_get_modes`.** ```c + for (uint8_t i =3D 0; i < ctx->desc->num_modes; i++) { ``` No other panel driver in the tree uses `uint8_t` for this kind of loop coun= ter. The kernel convention is `unsigned int` or `int`. While functionally f= ine here (there will never be >255 modes), it should be `unsigned int i` fo= r consistency. Also, declaring the variable in the `for` initializer is C99= /C11 style =E2=80=94 while the kernel now permits it, most existing panel d= rivers declare `i` at function scope. Using `unsigned int` declared in the = loop is fine, but `uint8_t` is unusual. **Issue 3 (minor): `thor_bottom_desc` should be `const`.** ```c +static struct ch13726a_desc thor_bottom_desc =3D { ``` This data is never modified. It should be `static const struct ch13726a_des= c thor_bottom_desc`. The `.data` pointer in the of_match_table already cast= s it to `const void *`. Correspondingly, the `desc` member in the panel str= uct should be `const struct ch13726a_desc *desc`. **Issue 4 (very minor): Cast in `of_device_get_match_data`.** ```c + ctx->desc =3D (struct ch13726a_desc *)of_device_get_match_data(dev); ``` If the `desc` field is changed to `const struct ch13726a_desc *` per above,= the cast can become `(const struct ch13726a_desc *)` or be dropped entirel= y since `of_device_get_match_data` returns `const void *`. **Issue 5 (observation): Reset GPIO polarity.** ```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` (deasserted at probe), and the DT= example uses `GPIO_ACTIVE_HIGH`. The reset sequence asserts (1), deasserts= (0), then asserts (1) again =E2=80=94 this means the panel enters reset, e= xits reset, and then enters reset once more as the "end" state of the reset= function, staying in reset. But `ch13726a_prepare` calls `ch13726a_reset()= ` and then proceeds to `ch13726a_on()` which sends DSI commands. If the pan= el is still held in reset when DSI commands are sent, this may not work. Si= nce the driver appears to be tested and working (v5), perhaps the logic val= ue interpretation is inverted from the typical convention =E2=80=94 the aut= hor should confirm that the reset sequence is intentional and correct. Typi= cally reset functions end with the reset line deasserted (panel out of rese= t). **Issue 6 (minor): `ch13726a_disable` clears LPM but `ch13726a_on` sets it.= ** ```c +static int ch13726a_disable(struct drm_panel *panel) +{ + ... + ctx->dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM; ``` In `ch13726a_on`: ```c + ctx->dsi->mode_flags |=3D MIPI_DSI_MODE_LPM; ``` The disable path clears LPM mode, sends display-off and sleep commands with= out LPM. The backlight update callback also clears LPM, sends brightness, t= hen sets it again. The on path sets LPM and sends initialization commands i= n LPM. This is internally consistent, but the disable path never restores L= PM =E2=80=94 so if there's a disable/re-prepare cycle, `ch13726a_on` will s= et it again which is fine. No bug, just worth noting. **Summary of requested changes:** 1. Remove the `DRM_DISPLAY_DP_HELPER` and `DRM_DISPLAY_HELPER` selects from= Kconfig =E2=80=94 they are not needed for a DSI panel driver. 2. Change `uint8_t i` to `unsigned int i` in `ch13726a_get_modes`. 3. Make `thor_bottom_desc` and the `desc` struct member `const`. 4. Please confirm the reset sequence is correct (panel left in reset state = before DSI init commands are sent). --- Generated by Claude Code Patch Reviewer