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: novatek-nt36672a: Convert to mipi_dsi_*_multi() helpers Date: Tue, 24 Feb 2026 10:48:43 +1000 Message-ID: In-Reply-To: <20260223043441.5295-1-chintanlike@gmail.com> References: <20260223043441.5295-1-chintanlike@gmail.com> <20260223043441.5295-1-chintanlike@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **nt36672a_send_cmds conversion** > +static void nt36672a_send_cmds(struct mipi_dsi_multi_context *dsi_ctx, > + const struct nt36672a_panel_cmd *cmds, int num) The function signature conversion from returning `int` to `void` and taking `mipi_dsi_multi_context` is correct. The replacement of `mipi_dsi_dcs_write(pinfo->link, cmd->data[0], cmd->data + 1, 1)` with `mipi_dsi_dcs_write_buffer_multi(dsi_ctx, cmd->data, sizeof(cmd->data))` is semantically equivalent -- `mipi_dsi_dcs_write_buffer` treats `data[0]` as the DCS command byte, same as the explicit split in the old call. Minor alignment nit already noted by Doug: the `const` on the second line doesn't align with the open parenthesis. **unprepare() sequencing bug** > + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx); > + /* Reset error to continue power-down even if display off failed */ > + dsi_ctx.accum_err = 0; > + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx); > + > + /* 120ms delay required here as per DCS spec */ > + msleep(120); The original sequence was: set_display_off, 120ms delay, enter_sleep_mode, 60ms delay. The new code sends enter_sleep_mode immediately after set_display_off (with only the accum_err reset between them), and both msleep calls come afterward. This changes the power-down timing in a way that contradicts the comment about the DCS-spec-required 120ms delay. The 120ms gap was between set_display_off and enter_sleep_mode; now both commands are sent back-to-back before either delay. Additionally, the `accum_err = 0` reset is placed after `set_display_off_multi` but before `enter_sleep_mode_multi`. This means if `nt36672a_send_cmds` (the off-commands) sets an error, `set_display_off_multi` will be a no-op because it checks `accum_err` at entry. In the original code, `set_display_off` was always attempted regardless of whether the off-commands failed. To preserve the original best-effort behavior, the `accum_err` should be reset before `set_display_off_multi` as well, or each call should reset independently. **unprepare() return value / power_off** > + nt36672a_panel_power_off(panel); > + > + return 0; The return value of `nt36672a_panel_power_off()` is now discarded, and unprepare unconditionally returns 0. This is a reasonable change for a shutdown path, but as Doug noted, `nt36672a_panel_power_off()` should then be converted to return `void` to make it clear the error is intentionally ignored. **prepare() error handling** > + err = nt36672a_panel_power_on(pinfo); > + if (err < 0) > + return err; The old code went to `goto poweroff` which called `gpiod_set_value(pinfo->reset_gpio, 0)`. The new code returns directly. This is harmless in practice: `nt36672a_panel_power_on()` can only fail at `regulator_bulk_enable()`, before the GPIO is ever toggled, so the GPIO is still in its initial low state at that point. > + err = dsi_ctx.accum_err; > + if (err < 0) > + gpiod_set_value(pinfo->reset_gpio, 0); > + > + return err; This preserves the error-path GPIO reset for DSI command failures, matching the original behavior. However, neither the old nor new code disables the regulators on a DSI-command failure in prepare(), leaving them enabled. That's a pre-existing issue, not introduced by this patch. As Doug suggested, using `dsi_ctx.accum_err` to store the `nt36672a_panel_power_on()` result (and letting the `_multi` functions no-op on error) would simplify this function and eliminate the separate `err` variable, reducing the risk of the kind of bug fixed in commit 61ce50fd8196. --- Generated by Claude Code Patch Reviewer