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 driver for Novatek NT35532 Date: Sun, 22 Mar 2026 05:00:32 +1000 Message-ID: In-Reply-To: <20260318-rimob-new-features-v2-2-c1bf8917449e@protonmail.com> References: <20260318-rimob-new-features-v2-0-c1bf8917449e@protonmail.com> <20260318-rimob-new-features-v2-2-c1bf8917449e@protonmail.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 This is the core panel driver, auto-generated from vendor device tree, with= 779 lines of register init sequences. **Issue (moderate): Sleep/display-on commands sent via `mipi_dsi_generic_wr= ite_seq_multi` instead of DCS helpers** In `nt35532_on()`: ```c mipi_dsi_generic_write_seq_multi(&dsi_ctx, MIPI_DCS_EXIT_SLEEP_MODE, 0x00); mipi_dsi_msleep(&dsi_ctx, 120); mipi_dsi_generic_write_seq_multi(&dsi_ctx, MIPI_DCS_SET_DISPLAY_ON, 0x00); ``` Using `mipi_dsi_generic_write_seq_multi` with DCS command bytes sends these= as **generic write packets** rather than **DCS write packets**. The correc= t approach is to use the dedicated `_multi` DCS helpers, as is already done= in `nt35532_off()`: ```c mipi_dsi_dcs_set_display_off_multi(&dsi_ctx); mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx); ``` The `nt35532_on()` function should similarly use: ```c mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); mipi_dsi_msleep(&dsi_ctx, 120); mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); ``` Additionally, the trailing `0x00` parameter in these calls is suspicious = =E2=80=94 DCS exit_sleep_mode and set_display_on are zero-parameter command= s. The generic_write sends `{0x11, 0x00}` as a 2-byte payload, which is not= standard. Some panels may tolerate it but it's technically incorrect. **Minor: `select DRM_KMS_HELPER` in Kconfig may be unnecessary** ``` select DRM_KMS_HELPER ``` The driver uses `drm_connector_helper_get_modes_fixed()` which comes from `= drm_probe_helper.h`. Check whether this is already pulled in transitively v= ia `DRM_MIPI_DSI`. Many panel drivers don't explicitly select `DRM_KMS_HELP= ER`. **Style: `container_of_const` usage** ```c return container_of_const(panel, struct novatek_nt35532, panel); ``` The function returns a non-const `struct novatek_nt35532 *`, but `container= _of_const` is designed to propagate const-ness. Since all callers need a mu= table pointer, `container_of` would be more conventional here. That said, s= ome newer panel drivers do use `container_of_const` =E2=80=94 this is a min= or style point. **No issues with**: regulator bulk handling, reset sequence, probe/remove l= ifecycle, use of `devm_drm_panel_alloc`, mode definition. The driver is cle= an for an auto-generated panel driver. --- Generated by Claude Code Patch Reviewer