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 Novatek/Tianma NT37700F panel Date: Tue, 10 Mar 2026 11:42:23 +1000 Message-ID: In-Reply-To: <20260310002606.16413-4-mailingradian@gmail.com> References: <20260310002606.16413-1-mailingradian@gmail.com> <20260310002606.16413-4-mailingradian@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 **Status: Mostly good, minor observations.** The driver is well-structured and uses modern APIs (`mipi_dsi_multi_context= `, `drm_connector_helper_get_modes_fixed`). A few observations: 1. **Includes**: `` is included but not directly needed =E2=80= =94 the OF match table works through `` and the `MODULE_DEV= ICE_TABLE` macro. This is very minor and cosmetic. 2. **`prepare` error path missing regulator disable**: In `nt37700f_tianma_= prepare()`: ```c ret =3D nt37700f_tianma_on(ctx); if (ret < 0) { dev_err(dev, "Failed to initialize panel: %d\n", ret); gpiod_set_value_cansleep(ctx->reset_gpio, 1); return ret; } ``` When `nt37700f_tianma_on()` fails, the reset GPIO is asserted but the re= gulator is **not** disabled. This will leave the `power` supply enabled aft= er a failed prepare. The error path should call `regulator_disable(ctx->sup= ply)` as well to match what `unprepare` does. 3. **Backlight mode flag toggling**: The backlight `update_status` and `get= _brightness` callbacks toggle `MIPI_DSI_MODE_LPM`: ```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 not thread-safe =E2=80=94 if backlight updates could race with o= ther DSI operations, the mode flags could be corrupted. This is a pattern s= een in generated drivers, and in practice it may be fine for this single-pa= nel setup, but it's worth noting. 4. **`MIPI_DSI_MODE_VIDEO_BURST` without `MIPI_DSI_MODE_VIDEO`**: The mode = flags are: ```c dsi->mode_flags =3D MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_CLOCK_NON_CONTINUOUS | MIPI_DSI_MODE_LPM; ``` `MODE_VIDEO_BURST` implies video mode, but `MIPI_DSI_MODE_VIDEO` itself = is not set. This means the panel operates in **command mode** (since `MODE_= VIDEO` is not set), and `MODE_VIDEO_BURST` has no effect. The MODULE_DESCRI= PTION says "cmd mode" which is consistent, but having `MODE_VIDEO_BURST` se= t when not in video mode is misleading/unnecessary. Consider removing `MIPI= _DSI_MODE_VIDEO_BURST` if this is indeed a command-mode panel. --- Generated by Claude Code Patch Reviewer