[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