Claude review: drm/panel: novatek-nt36672a: Convert to mipi_dsi_*_multi() helpers

Claude review: drm/panel: novatek-nt36672a: Convert to mipi_dsi_*_multi() helpers

[Claude Code Review Bot]

Overall Series Review

Subject: drm/panel: novatek-nt36672a: Convert to mipi_dsi_*_multi() helpers
Author: Chintan Patel <chintanlike@gmail.com>
Patches: 2
Reviewed: 2026-02-24T10:48:42.972211

---

This is a single-patch series converting the panel-novatek-nt36672a driver from the deprecated `mipi_dsi_dcs_write()` API to the newer `mipi_dsi_*_multi()` helpers with `mipi_dsi_multi_context` error accumulation. This is a standard modernization cleanup that many panel drivers have undergone.

The conversion is largely straightforward but contains one clear sequencing bug in `nt36672a_panel_unprepare()`: the enter-sleep-mode command has been moved before the 120ms delay, whereas the original code (and the DCS spec, per the existing comment) placed it after. Doug Anderson already identified this in his on-list reply. There is also a subtler issue where the error accumulation semantics in unprepare cause `set_display_off` to be skipped if the preceding off-commands fail, which differs from the original best-effort-everything behavior.

The prepare() conversion is mostly correct but could be tightened up, as Doug also noted. The `nt36672a_panel_power_off()` return value is now silently discarded.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/panel: novatek-nt36672a: Convert to mipi_dsi_*_multi() helpers

[Claude Code Review Bot]

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

[PATCH v2] drm/panel: novatek-nt36672a: Convert to mipi_dsi_*_multi() helpers

[Chintan Patel]

Convert the driver to use the non-deprecated mipi_dsi_*_multi() helpers and
mipi_dsi_msleep().

Switch DCS command sequences to the multi context API and
accumulate errors via struct mipi_dsi_multi_context. Replace
open-coded error handling with the multi helpers and convert
nt36672a_send_cmds() and power sequencing accordingly.

Signed-off-by: Chintan Patel <chintanlike@gmail.com>
---
Changes in v2:
- Address alignment feedback from Doug.
- Restore original power-down ordering.
- Drop return value from nt36672a_panel_power_off().
- Consolidate error handling around dsi_ctx.accum_err.

 .../gpu/drm/panel/panel-novatek-nt36672a.c    | 98 ++++++-------------
 1 file changed, 30 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
index 29e1f6aea480..3ebdc3048b26 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
@@ -79,70 +79,53 @@ static inline struct nt36672a_panel *to_nt36672a_panel(struct drm_panel *panel)
 	return container_of(panel, struct nt36672a_panel, base);
 }
 
-static int nt36672a_send_cmds(struct drm_panel *panel, const struct nt36672a_panel_cmd *cmds,
-			      int num)
+static void nt36672a_send_cmds(struct mipi_dsi_multi_context *dsi_ctx,
+			       const struct nt36672a_panel_cmd *cmds, int num)
 {
-	struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
 	unsigned int i;
-	int err;
 
 	for (i = 0; i < num; i++) {
 		const struct nt36672a_panel_cmd *cmd = &cmds[i];
 
-		err = mipi_dsi_dcs_write(pinfo->link, cmd->data[0], cmd->data + 1, 1);
-
-		if (err < 0)
-			return err;
+		/* cmd->data[0] is the DCS command, cmd->data[1] is the parameter */
+		mipi_dsi_dcs_write_buffer_multi(dsi_ctx, cmd->data, sizeof(cmd->data));
 	}
-
-	return 0;
 }
 
-static int nt36672a_panel_power_off(struct drm_panel *panel)
+static void nt36672a_panel_power_off(struct drm_panel *panel)
 {
 	struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
-	int ret = 0;
 
 	gpiod_set_value(pinfo->reset_gpio, 1);
 
-	ret = regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies);
-	if (ret)
-		dev_err(panel->dev, "regulator_bulk_disable failed %d\n", ret);
-
-	return ret;
+	if (regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies) < 0)
+		dev_err(panel->dev, "regulator_bulk_disable failed\n");
 }
 
 static int nt36672a_panel_unprepare(struct drm_panel *panel)
 {
 	struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
-	int ret;
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->link };
 
 	/* send off cmds */
-	ret = nt36672a_send_cmds(panel, pinfo->desc->off_cmds,
-				 pinfo->desc->num_off_cmds);
+	nt36672a_send_cmds(&dsi_ctx, pinfo->desc->off_cmds,
+			   pinfo->desc->num_off_cmds);
 
-	if (ret < 0)
-		dev_err(panel->dev, "failed to send DCS off cmds: %d\n", ret);
-
-	ret = mipi_dsi_dcs_set_display_off(pinfo->link);
-	if (ret < 0)
-		dev_err(panel->dev, "set_display_off cmd failed ret = %d\n", ret);
+	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;
 
 	/* 120ms delay required here as per DCS spec */
 	msleep(120);
 
-	ret = mipi_dsi_dcs_enter_sleep_mode(pinfo->link);
-	if (ret < 0)
-		dev_err(panel->dev, "enter_sleep cmd failed ret = %d\n", ret);
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
 
 	/* 0x3C = 60ms delay */
-	msleep(60);
+	mipi_dsi_msleep(&dsi_ctx, 60);
 
-	ret = nt36672a_panel_power_off(panel);
-	if (ret < 0)
-		dev_err(panel->dev, "power_off failed ret = %d\n", ret);
+	nt36672a_panel_power_off(panel);
 
-	return ret;
+	return 0;
 }
 
 static int nt36672a_panel_power_on(struct nt36672a_panel *pinfo)
@@ -170,52 +153,31 @@ static int nt36672a_panel_power_on(struct nt36672a_panel *pinfo)
 static int nt36672a_panel_prepare(struct drm_panel *panel)
 {
 	struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
-	int err;
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->link };
 
-	err = nt36672a_panel_power_on(pinfo);
-	if (err < 0)
-		goto poweroff;
+	dsi_ctx.accum_err = nt36672a_panel_power_on(pinfo);
 
 	/* send first part of init cmds */
-	err = nt36672a_send_cmds(panel, pinfo->desc->on_cmds_1,
-				 pinfo->desc->num_on_cmds_1);
+	nt36672a_send_cmds(&dsi_ctx, pinfo->desc->on_cmds_1,
+			   pinfo->desc->num_on_cmds_1);
 
-	if (err < 0) {
-		dev_err(panel->dev, "failed to send DCS Init 1st Code: %d\n", err);
-		goto poweroff;
-	}
-
-	err = mipi_dsi_dcs_exit_sleep_mode(pinfo->link);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
-		goto poweroff;
-	}
+	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
 
 	/* 0x46 = 70 ms delay */
-	msleep(70);
+	mipi_dsi_msleep(&dsi_ctx, 70);
 
-	err = mipi_dsi_dcs_set_display_on(pinfo->link);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to Set Display ON: %d\n", err);
-		goto poweroff;
-	}
+	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
 
 	/* Send rest of the init cmds */
-	err = nt36672a_send_cmds(panel, pinfo->desc->on_cmds_2,
-				 pinfo->desc->num_on_cmds_2);
+	nt36672a_send_cmds(&dsi_ctx, pinfo->desc->on_cmds_2,
+			   pinfo->desc->num_on_cmds_2);
 
-	if (err < 0) {
-		dev_err(panel->dev, "failed to send DCS Init 2nd Code: %d\n", err);
-		goto poweroff;
-	}
+	mipi_dsi_msleep(&dsi_ctx, 120);
 
-	msleep(120);
+	if (dsi_ctx.accum_err < 0)
+		gpiod_set_value(pinfo->reset_gpio, 0);
 
-	return 0;
-
-poweroff:
-	gpiod_set_value(pinfo->reset_gpio, 0);
-	return err;
+	return dsi_ctx.accum_err;
 }
 
 static int nt36672a_panel_get_modes(struct drm_panel *panel,
-- 
2.43.0

Claude review: drm/panel: novatek-nt36672a: Convert to mipi_dsi_*_multi() helpers

[Claude Code Review Bot]

Overall Series Review

Subject: drm/panel: novatek-nt36672a: Convert to mipi_dsi_*_multi() helpers
Author: Chintan Patel <chintanlike@gmail.com>
Patches: 1
Reviewed: 2026-03-04T07:58:46.804279

---

This is a single-patch conversion of the `panel-novatek-nt36672a` driver from the deprecated `mipi_dsi_dcs_*()` API to the `mipi_dsi_*_multi()` helpers with `struct mipi_dsi_multi_context`. The conversion is largely correct and well-structured for a v2 submission. However, there is one behavioral change in the unprepare error path that silently drops a DSI command when an earlier step fails, which changes the original "always try everything during power-down" behavior.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/panel: novatek-nt36672a: Convert to mipi_dsi_*_multi() helpers

[Claude Code Review Bot]

Patch Review

**nt36672a_send_cmds conversion** - The switch from `mipi_dsi_dcs_write(pinfo->link, cmd->data[0], cmd->data + 1, 1)` to `mipi_dsi_dcs_write_buffer_multi(dsi_ctx, cmd->data, sizeof(cmd->data))` is semantically equivalent. Good.

**Behavioral change in unprepare error path** - This is the main concern. In the new code at lines 111-116:

```c
nt36672a_send_cmds(&dsi_ctx, pinfo->desc->off_cmds,
		   pinfo->desc->num_off_cmds);

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;
```

If `nt36672a_send_cmds()` fails (setting `accum_err`), then `mipi_dsi_dcs_set_display_off_multi()` will be a no-op because all `_multi` helpers bail out early when `accum_err != 0`. The `accum_err = 0` reset comes *after* `set_display_off_multi`, so it doesn't help. In the original code, `set_display_off` was always attempted independently regardless of whether off_cmds succeeded. For a power-down path, you generally want to try everything. The reset should be moved *before* `set_display_off_multi`:

```c
nt36672a_send_cmds(&dsi_ctx, pinfo->desc->off_cmds,
		   pinfo->desc->num_off_cmds);

/* Reset error to continue power-down even if off cmds failed */
dsi_ctx.accum_err = 0;

mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
```

Or alternatively, reset `accum_err` both before `set_display_off_multi` and after it, to also handle its failure.

**120ms sleep uses plain `msleep()` instead of `mipi_dsi_msleep()`** (line 119) - This is actually correct behavior here, since the 120ms delay must always occur unconditionally during power-down regardless of prior errors. Good.

**nt36672a_panel_prepare error path** - The pattern of seeding `accum_err` from `nt36672a_panel_power_on()` at line 158 is a good use of the multi context:

```c
dsi_ctx.accum_err = nt36672a_panel_power_on(pinfo);
```

The error cleanup at lines 177-178 preserves the original behavior:

```c
if (dsi_ctx.accum_err < 0)
    gpiod_set_value(pinfo->reset_gpio, 0);
```

Note: the original `poweroff:` label also set the GPIO to 0, which appears to be a pre-existing issue (this de-asserts reset rather than re-asserting it, so it's effectively a no-op after a successful `power_on`). Not introduced by this patch.

**Lost error context** - The original code had specific `dev_err` messages like `"failed to send DCS off cmds: %d"` and `"set_display_off cmd failed ret = %d"`. These are all removed. While the low-level `_multi` helpers do log transport-level errors, the higher-level context (which operation was being attempted) is lost. This is typical for multi conversions and generally accepted, but worth noting.

**nt36672a_panel_power_off** - Dropping the error code from the log message (line 101-102):

```c
if (regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies) < 0)
    dev_err(panel->dev, "regulator_bulk_disable failed\n");
```

Minor nit: the original included `%d` with the error code, which can be useful for debugging. Consider preserving it:
```c
int ret = regulator_bulk_disable(...);
if (ret)
    dev_err(panel->dev, "regulator_bulk_disable failed %d\n", ret);
```

**Summary**: The conversion is mostly correct. The one issue that should be fixed before merging is the `accum_err` reset placement in `nt36672a_panel_unprepare()`, which causes `set_display_off` to be skipped when `send_cmds` fails. Move the reset before `set_display_off_multi()` to preserve the original always-try-everything power-down behavior.

---
Generated by Claude Code Patch Reviewer