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: Sun, 12 Apr 2026 13:10:38 +1000 Message-ID: In-Reply-To: <20260408-ch13726a-v4-2-9bb1a9b8f329@gmail.com> References: <20260408-ch13726a-v4-0-9bb1a9b8f329@gmail.com> <20260408-ch13726a-v4-2-9bb1a9b8f329@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 need attention: **1. Unnecessary `select DRM_DISPLAY_DP_HELPER` and `select DRM_DISPLAY_HEL= PER` in Kconfig (important)** ```c + select DRM_DISPLAY_DP_HELPER + select DRM_DISPLAY_HELPER ``` This is a pure MIPI-DSI panel driver =E2=80=94 it has no DisplayPort or DSC= functionality. These selects are unnecessary and pull in code the driver d= oesn't use. Other pure MIPI-DSI panel drivers (e.g., `panel-boe-th101mb31ig= 002-28a`, `panel-himax-hx8394`) do not select these. Remove both lines. **2. Redundant `prepared` flag tracking (important)** ```c +struct ch13726a_panel { + ... + bool prepared; +}; ``` And in `ch13726a_prepare()`: ```c + if (ctx->prepared) + return 0; + ... + ctx->prepared =3D true; ``` And in `ch13726a_unprepare()`: ```c + if (!ctx->prepared) + return 0; + ... + ctx->prepared =3D false; ``` The DRM panel core already tracks `panel->prepared` and guards against doub= le-prepare/unprepare in `drm_panel_prepare()`/`drm_panel_unprepare()` (see = `drm_panel.c` lines 122-134 and 177-199). This driver-level tracking is red= undant and should be removed. No current in-tree panel driver uses a privat= e `prepared` flag anymore. **3. `thor_bottom_desc` should be `const` (minor)** ```c +static struct ch13726a_desc thor_bottom_desc =3D { ``` This should be `static const struct ch13726a_desc`. It's immutable data. Th= e struct member should also be `const`: ```c - struct ch13726a_desc *desc; + const struct ch13726a_desc *desc; ``` This would also eliminate the need for the cast in probe: ```c + ctx->desc =3D (struct ch13726a_desc *)of_device_get_match_data(dev); ``` With `const struct ch13726a_desc *desc`, this becomes simply: ```c ctx->desc =3D of_device_get_match_data(dev); ``` **4. `uint8_t` loop variable (minor, style)** ```c + for (uint8_t i =3D 0; i < ctx->desc->num_modes; i++) { ``` Kernel code uses `u8` rather than `uint8_t`, but for a loop index iterating= over a small count, a plain `unsigned int` or `int` is more idiomatic. Dec= laring the variable inside `for()` is a C99/C11 feature =E2=80=94 while it = works with modern kernel builds, many panel drivers still declare loop vari= ables at function scope. Use `int i;` declared at the top for consistency w= ith the prevailing kernel style. **5. Reset GPIO polarity in error path (worth verifying)** In `ch13726a_prepare()`: ```c + gpiod_set_value_cansleep(ctx->reset_gpio, 0); ``` The GPIO is obtained with `GPIOD_OUT_LOW` and the reset sequence asserts hi= gh (1), deasserts to low (0), then asserts high (1) again. In the error pat= h after `ch13726a_on()` fails, the driver sets GPIO to 0 (deasserted/inacti= ve), which keeps the panel out of reset. This is inconsistent =E2=80=94 typ= ically on error you want to put the panel *into* reset (assert reset) befor= e disabling regulators. If active-high means "reset active", the error path= should set the GPIO to 1. However, this depends on the actual hardware sem= antics. Since the binding says `GPIO_ACTIVE_HIGH` and `reset-gpios` is the = name, `gpiod_set_value(1)` means "assert reset" which would be the safer st= ate on error. Worth confirming with the hardware. Similarly in `ch13726a_unprepare()`: ```c + gpiod_set_value_cansleep(ctx->reset_gpio, 0); ``` This deasserts reset before killing power, which could leave the panel in a= n undefined state. Most panel drivers assert reset (set to 1) before cuttin= g regulators to ensure clean shutdown. Compare with how `ch13726a_reset()` = works =E2=80=94 it uses `1` as the active/working state. The unprepare shou= ld logically assert reset (1) then cut power, not the other way around. **6. `mipi_dsi_detach` error checking in `remove()` is unnecessary noise (v= ery minor)** ```c + ret =3D mipi_dsi_detach(dsi); + if (ret < 0) + dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret); ``` `mipi_dsi_detach()` never fails in practice (the host ops->detach can retur= n an error, but no upstream host driver does). Newer panel drivers tend to = not check this return. Not a blocker, but the error message adds no value. **7. Missing `msleep`/delay between display-off and sleep-enter in `ch13726= a_disable()` (minor)** ```c + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx); + mipi_dsi_msleep(&dsi_ctx, 50); + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx); ``` There is a 50ms delay between display-off and sleep-enter, which is good. B= ut there is no delay *after* entering sleep mode. Many MIPI panel datasheet= s require a delay (commonly 120ms) after entering sleep mode before cutting= power in `unprepare()`. The `ch13726a_on()` path has a 28ms `msleep` at th= e end of `prepare()`, but the disable->unprepare transition has no such gua= rd. If the panel requires a post-sleep delay per its datasheet, this could = cause issues. **8. Mode flags manipulation in backlight callback (worth noting)** ```c +static int ch13726a_bl_update_status(struct backlight_device *bl) +{ + ... + 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; ``` And in `ch13726a_on()`: ```c + ctx->dsi->mode_flags |=3D MIPI_DSI_MODE_LPM; ``` And `ch13726a_disable()`: ```c + ctx->dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM; ``` The LPM flag toggling is not protected by any locking. If backlight updates= can race with disable/prepare calls, the mode_flags could be corrupted. Th= is is a pre-existing pattern in many panel drivers, so not unique to this o= ne, but worth noting. **Summary of requested changes for v5:** 1. Remove `DRM_DISPLAY_DP_HELPER` and `DRM_DISPLAY_HELPER` from Kconfig 2. Remove the private `prepared` bool and the associated checks 3. Make `thor_bottom_desc` and the `desc` pointer `const`, drop the cast 4. Use `int` or `unsigned int` for the loop variable, not `uint8_t` 5. Verify GPIO reset polarity in error/unprepare paths (assert reset before= cutting power) --- Generated by Claude Code Patch Reviewer