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

[PATCH] 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().

This removes open-coded error handling and relies on
mipi_dsi_multi_context for error accumulation.

In unprepare(), reset the accumulated error between
set_display_off and enter_sleep_mode to preserve the
existing power-down sequencing semantics.

Signed-off-by: Chintan Patel <chintanlike@gmail.com>
---
 .../gpu/drm/panel/panel-novatek-nt36672a.c    | 83 ++++++-------------
 1 file changed, 26 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
index 29e1f6aea480..2c8d67a69c7e 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
@@ -79,23 +79,17 @@ 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)
@@ -115,34 +109,26 @@ static int nt36672a_panel_power_off(struct drm_panel *panel)
 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;
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
 
 	/* 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);
-
 	/* 0x3C = 60ms delay */
 	msleep(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,51 +156,34 @@ 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);
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->link };
 	int err;
 
 	err = nt36672a_panel_power_on(pinfo);
 	if (err < 0)
-		goto poweroff;
+		return err;
 
 	/* 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;
-	}
-
-	msleep(120);
+	mipi_dsi_msleep(&dsi_ctx, 120);
 
-	return 0;
+	err = dsi_ctx.accum_err;
+	if (err < 0)
+		gpiod_set_value(pinfo->reset_gpio, 0);
 
-poweroff:
-	gpiod_set_value(pinfo->reset_gpio, 0);
 	return err;
 }
 
-- 
2.43.0

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

[Doug Anderson]

Hi,

On Sun, Feb 22, 2026 at 8:35 PM Chintan Patel <chintanlike@gmail.com> wrote:
>
> @@ -79,23 +79,17 @@ 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)

nit: checkpatch --strict yells:

CHECK: Alignment should match open parenthesis
#37: FILE: drivers/gpu/drm/panel/panel-novatek-nt36672a.c:83:
+static void nt36672a_send_cmds(struct mipi_dsi_multi_context *dsi_ctx,
+                               const struct nt36672a_panel_cmd *cmds, int num)


Also: FWIW having a function like nt36672a_send_cmds() is discouraged
these days. One of the reasons for creating the "_multi" functions and
making them efficient was that init functions were preferred. For
instance, see the commit d6ddb6624a7f ("drm/panel: boe-tv101wum-nl6:
Don't use a table for initting panels") or commit 6f6fd690de1a
("drm/panel: innolux-p079zca: Don't use a table for initting panels").

I won't insist that you make the conversion here since I think you're
still providing a valuable cleanup with the patch you've already
written, but in case you want to go above-and-beyond you could try it.
You could even try running bloat-o-meter to see how the size changes.


> @@ -115,34 +109,26 @@ static int nt36672a_panel_power_off(struct drm_panel *panel)
>  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;
> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
>
>         /* 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);

You've changed the order. Enter sleep mode used to be after the 120ms
sleep. Now it's before.


> -
>         /* 0x3C = 60ms delay */
>         msleep(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);

You got rid of the extra print here, which is fine/right from my
perspective (since nt36672a_panel_power_off() already printed). ...but
nt36672a_panel_power_off()'s return code is never checked now. Just
make nt36672a_panel_power_off() return "void".


> @@ -170,51 +156,34 @@ 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);
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->link };
>         int err;
>
>         err = nt36672a_panel_power_on(pinfo);
>         if (err < 0)
> -               goto poweroff;
> +               return err;

I won't insist since it's a matter of opinion, but IMO get rid of the
`err` local variable and consistently use `dsi_ctx.accum_err` to store
the error code in this function. Yeah, it's a bit awkward, but having
the separate `err` variable just makes it too easy to mess up and use
it. As one example of where that happened, see commit 61ce50fd8196
("drm/panel: jdi-lpm102a188a: Fix error code in jdi_panel_prepare()").

Also: instead of returning right away, I think you want to make sure
that the GPIO gets set to 0 like the old code did, don't you? I'd be a
little hesitant changing this unless we can truly prove that the GPIO
set is not needed or unless we're certain that the GPIO set was
incorrect (in which case we should fix it in a separate patch).

...nicely, if you use `dsi_ctx.accum_err` you don't even need a
"goto". You can just let the "_multi" functions all run and be
no-ops...

-Doug

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