[PATCH 0/2] drm/panel: novatek-nt35560: Fix bug and clean up

[PATCH 0/2] drm/panel: novatek-nt35560: Fix bug and clean up

[Brigham Campbell]

This series does the following:
 - Add mipi_dsi_dcs_read_multi() to drm_mipi_dsi.c for improved error
   handling in drivers which use mipi_dsi_dcs_read() such as the
   novatek-nt35560 driver.
 - Fix a bug in nt35560_set_brightness() which causes the driver to
   incorrectly report that it "failed to disable display backlight".
 - Clean up novatek-nt35560 driver to use "multi" variants of MIPI
   functions which promote cleaner code.

If there's a reason that mipi_dsi_dcs_read_multi doesn't exist, I'm
happy to drop it.

Brigham Campbell (2):
  drm: Create mipi_dsi_dcs_read_multi()
  drm/panel: novatek-nt35560: Fix bug and clean up

 drivers/gpu/drm/drm_mipi_dsi.c                |  37 +++
 drivers/gpu/drm/panel/panel-novatek-nt35560.c | 212 ++++++------------
 include/drm/drm_mipi_dsi.h                    |   2 +
 3 files changed, 105 insertions(+), 146 deletions(-)


base-commit: 1511d3c4d2bb30f784924a877f3cef518bb73077
-- 
2.50.1

[PATCH 1/2] drm: Create mipi_dsi_dcs_read_multi()

[Brigham Campbell]

Create mipi_dsi_dcs_read_multi(), which accepts a mipi_dsi_multi_context
struct for improved error handling and cleaner panel driver code.

Signed-off-by: Brigham Campbell <me@brighamcampbell.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 37 ++++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  2 ++
 2 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index a00d76443128..227b28555174 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -1075,6 +1075,43 @@ ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 }
 EXPORT_SYMBOL(mipi_dsi_dcs_read);
 
+/**
+ * mipi_dsi_dcs_read_multi() - mipi_dsi_dcs_read() w/ accum_err
+ * @ctx: Context for multiple DSI transactions
+ * @cmd: DCS command
+ * @data: buffer in which to receive data
+ * @len: size of receive buffer
+ *
+ * Like mipi_dsi_dcs_read() but deals with errors in a way that makes it
+ * convenient to make several calls in a row.
+ */
+void mipi_dsi_dcs_read_multi(struct mipi_dsi_multi_context *ctx, u8 cmd,
+			     void *data, size_t len)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	struct mipi_dsi_msg msg = {
+		.channel = dsi->channel,
+		.type = MIPI_DSI_DCS_READ,
+		.tx_buf = &cmd,
+		.tx_len = 1,
+		.rx_buf = data,
+		.rx_len = len
+	};
+	ssize_t ret;
+
+	if (ctx->accum_err)
+		return;
+
+	ret = mipi_dsi_device_transfer(dsi, &msg);
+	if (ret < 0) {
+		ctx->accum_err = ret;
+		dev_err(dev, "transferring dcs message %xh failed: %d\n", cmd,
+			ctx->accum_err);
+	}
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_read_multi);
+
 /**
  * mipi_dsi_dcs_nop() - send DCS nop packet
  * @dsi: DSI peripheral device
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 369b0d8830c3..d17c00d299c8 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -333,6 +333,8 @@ ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
 			   const void *data, size_t len);
 ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
 			  size_t len);
+void mipi_dsi_dcs_read_multi(struct mipi_dsi_multi_context *ctx, u8 cmd,
+			     void *data, size_t len);
 int mipi_dsi_dcs_nop(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_soft_reset(struct mipi_dsi_device *dsi);
 int mipi_dsi_dcs_get_power_mode(struct mipi_dsi_device *dsi, u8 *mode);
-- 
2.50.1

Re: [PATCH 1/2] drm: Create mipi_dsi_dcs_read_multi()

[Doug Anderson]

Hi,

On Thu, Jul 24, 2025 at 1:23 PM Brigham Campbell <me@brighamcampbell.com> wrote:
>
> +void mipi_dsi_dcs_read_multi(struct mipi_dsi_multi_context *ctx, u8 cmd,
> +                            void *data, size_t len)
> +{
> +       struct mipi_dsi_device *dsi = ctx->dsi;
> +       struct device *dev = &dsi->dev;
> +       struct mipi_dsi_msg msg = {
> +               .channel = dsi->channel,
> +               .type = MIPI_DSI_DCS_READ,
> +               .tx_buf = &cmd,
> +               .tx_len = 1,
> +               .rx_buf = data,
> +               .rx_len = len
> +       };
> +       ssize_t ret;
> +
> +       if (ctx->accum_err)
> +               return;
> +
> +       ret = mipi_dsi_device_transfer(dsi, &msg);
> +       if (ret < 0) {
> +               ctx->accum_err = ret;
> +               dev_err(dev, "transferring dcs message %xh failed: %d\n", cmd,

Format code "%xh" is probably not exactly what you want. If the error
code is 0x10 it will print 10h, which is not very standard. You
probably copied it from the write routine which uses "%*ph". There the
"h" means something. See Documentation/core-api/printk-formats.rst.
Probably you want "%#x".

I'd probably also say "dcs read with cmd" rather than "transferring
dcs message".

Re: [PATCH 1/2] drm: Create mipi_dsi_dcs_read_multi()

[Brigham Campbell]

On Fri Jul 25, 2025 at 3:16 PM MDT, Doug Anderson wrote:
>> +               dev_err(dev, "transferring dcs message %xh failed: %d\n", cmd,
>
> Format code "%xh" is probably not exactly what you want. If the error
> code is 0x10 it will print 10h, which is not very standard. You
> probably copied it from the write routine which uses "%*ph". There the
> "h" means something. See Documentation/core-api/printk-formats.rst.
> Probably you want "%#x".

Ah yes, I had based this change off the "%*ph" format specifier and I
had mistakenly assumed that the 'h' was a literal 'h'. I'll fix that in
v2.

> I'd probably also say "dcs read with cmd" rather than "transferring
> dcs message".

Yes, this sounds more accurate. I'll include this in v2 as well.

Thanks for the review,
Brigham

[PATCH 2/2] drm/panel: novatek-nt35560: Fix bug and clean up

[Brigham Campbell]

Fix bug in nt35560_set_brightness() which causes the function to
erroneously report an error. mipi_dsi_dcs_write() returns either a
negative value when an error occurred or a positive number of bytes
written when no error occurred. The buggy code reports and error under
either condition.

Update driver to use the "multi" variants of MIPI functions which
facilitate improved error handling and cleaner driver code.

Fixes: 7835ed6a9e86 ("drm/panel-sony-acx424akp: Modernize backlight handling")
Signed-off-by: Brigham Campbell <me@brighamcampbell.com>
---

The usage of the u8 array, mipi_buf_out, in nt35560_set_brightness() may
be a little curious. It's useful here because pwm_ratio and pwm_div
aren't constant, therefore we must store them in a buffer at runtime.

Using mipi_dsi_dcs_write_{seq,buffer}_multi() in place of
mipi_dsi_dcs_write() gives the added benefit that kmalloc() isn't used
to write mipi commands.

The jdi-lpm102a188a driver's unprepare() function will ignore errors
reported by mipi_dsi_dcs_{set_display_off,enter_sleep_mode}. This
driver, however, will fail to unprepare the panel if either function
returns an error. The behavior of the jdi-lpm102a188a panel makes more
sense to me, but I strongly prefer not to change the behavior of this
driver without personally having access to hardware to test.

Removed information from a comment which was made obsolete by commit
994ea402c767 ("drm/panel: Rename Sony ACX424 to Novatek NT35560")

 drivers/gpu/drm/panel/panel-novatek-nt35560.c | 212 ++++++------------
 1 file changed, 66 insertions(+), 146 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35560.c b/drivers/gpu/drm/panel/panel-novatek-nt35560.c
index 98f0782c8411..60758ce1401e 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt35560.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35560.c
@@ -148,24 +148,21 @@ static inline struct nt35560 *panel_to_nt35560(struct drm_panel *panel)
 static int nt35560_set_brightness(struct backlight_device *bl)
 {
 	struct nt35560 *nt = bl_get_data(bl);
-	struct mipi_dsi_device *dsi = to_mipi_dsi_device(nt->dev);
-	int period_ns = 1023;
+	struct mipi_dsi_multi_context dsi_ctx = {
+		.dsi = to_mipi_dsi_device(nt->dev)
+	};
 	int duty_ns = bl->props.brightness;
+	static u8 mipi_buf_out[2];
+	int period_ns = 1023;
 	u8 pwm_ratio;
 	u8 pwm_div;
-	u8 par;
-	int ret;
 
 	if (backlight_is_blank(bl)) {
 		/* Disable backlight */
-		par = 0x00;
-		ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
-					 &par, 1);
-		if (ret) {
-			dev_err(nt->dev, "failed to disable display backlight (%d)\n", ret);
-			return ret;
-		}
-		return 0;
+		mipi_dsi_dcs_write_seq_multi(&dsi_ctx,
+					     MIPI_DCS_WRITE_CONTROL_DISPLAY,
+					     0x00);
+		return dsi_ctx.accum_err;
 	}
 
 	/* Calculate the PWM duty cycle in n/256's */
@@ -176,62 +173,28 @@ static int nt35560_set_brightness(struct backlight_device *bl)
 
 	/* Set up PWM dutycycle ONE byte (differs from the standard) */
 	dev_dbg(nt->dev, "calculated duty cycle %02x\n", pwm_ratio);
-	ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
-				 &pwm_ratio, 1);
-	if (ret < 0) {
-		dev_err(nt->dev, "failed to set display PWM ratio (%d)\n", ret);
-		return ret;
-	}
 
-	/*
-	 * Sequence to write PWMDIV:
-	 *	address		data
-	 *	0xF3		0xAA   CMD2 Unlock
-	 *	0x00		0x01   Enter CMD2 page 0
-	 *	0X7D		0x01   No reload MTP of CMD2 P1
-	 *	0x22		PWMDIV
-	 *	0x7F		0xAA   CMD2 page 1 lock
-	 */
-	par = 0xaa;
-	ret = mipi_dsi_dcs_write(dsi, 0xf3, &par, 1);
-	if (ret < 0) {
-		dev_err(nt->dev, "failed to unlock CMD 2 (%d)\n", ret);
-		return ret;
-	}
-	par = 0x01;
-	ret = mipi_dsi_dcs_write(dsi, 0x00, &par, 1);
-	if (ret < 0) {
-		dev_err(nt->dev, "failed to enter page 1 (%d)\n", ret);
-		return ret;
-	}
-	par = 0x01;
-	ret = mipi_dsi_dcs_write(dsi, 0x7d, &par, 1);
-	if (ret < 0) {
-		dev_err(nt->dev, "failed to disable MTP reload (%d)\n", ret);
-		return ret;
-	}
-	ret = mipi_dsi_dcs_write(dsi, 0x22, &pwm_div, 1);
-	if (ret < 0) {
-		dev_err(nt->dev, "failed to set PWM divisor (%d)\n", ret);
-		return ret;
-	}
-	par = 0xaa;
-	ret = mipi_dsi_dcs_write(dsi, 0x7f, &par, 1);
-	if (ret < 0) {
-		dev_err(nt->dev, "failed to lock CMD 2 (%d)\n", ret);
-		return ret;
-	}
+	mipi_buf_out[0] = MIPI_DCS_SET_DISPLAY_BRIGHTNESS;
+	mipi_buf_out[1] = pwm_ratio;
+	mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, mipi_buf_out,
+					ARRAY_SIZE(mipi_buf_out));
+
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf3, 0xaa);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x00, 0x01);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x7d, 0x01);
+
+	mipi_buf_out[0] = 0x22;
+	mipi_buf_out[1] = pwm_div;
+	mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, mipi_buf_out,
+					ARRAY_SIZE(mipi_buf_out));
+
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x7f, 0xaa);
 
 	/* Enable backlight */
-	par = 0x24;
-	ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
-				 &par, 1);
-	if (ret < 0) {
-		dev_err(nt->dev, "failed to enable display backlight (%d)\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MIPI_DCS_WRITE_CONTROL_DISPLAY,
+				     0x24);
 
-	return 0;
+	return dsi_ctx.accum_err;
 }
 
 static const struct backlight_ops nt35560_bl_ops = {
@@ -244,32 +207,23 @@ static const struct backlight_properties nt35560_bl_props = {
 	.max_brightness = 1023,
 };
 
-static int nt35560_read_id(struct nt35560 *nt)
+static void nt35560_read_id(struct mipi_dsi_multi_context *dsi_ctx)
 {
-	struct mipi_dsi_device *dsi = to_mipi_dsi_device(nt->dev);
+	struct device dev = dsi_ctx->dsi->dev;
 	u8 vendor, version, panel;
 	u16 val;
-	int ret;
 
-	ret = mipi_dsi_dcs_read(dsi, NT35560_DCS_READ_ID1, &vendor, 1);
-	if (ret < 0) {
-		dev_err(nt->dev, "could not vendor ID byte\n");
-		return ret;
-	}
-	ret = mipi_dsi_dcs_read(dsi, NT35560_DCS_READ_ID2, &version, 1);
-	if (ret < 0) {
-		dev_err(nt->dev, "could not read device version byte\n");
-		return ret;
-	}
-	ret = mipi_dsi_dcs_read(dsi, NT35560_DCS_READ_ID3, &panel, 1);
-	if (ret < 0) {
-		dev_err(nt->dev, "could not read panel ID byte\n");
-		return ret;
-	}
+	mipi_dsi_dcs_read_multi(dsi_ctx, NT35560_DCS_READ_ID1, &vendor, 1);
+	mipi_dsi_dcs_read_multi(dsi_ctx, NT35560_DCS_READ_ID2, &version, 1);
+	mipi_dsi_dcs_read_multi(dsi_ctx, NT35560_DCS_READ_ID3, &panel, 1);
+
+	if (dsi_ctx->accum_err < 0)
+		return;
 
 	if (vendor == 0x00) {
-		dev_err(nt->dev, "device vendor ID is zero\n");
-		return -ENODEV;
+		dev_err(&dev, "device vendor ID is zero\n");
+		dsi_ctx->accum_err = -ENODEV;
+		return;
 	}
 
 	val = (vendor << 8) | panel;
@@ -278,16 +232,18 @@ static int nt35560_read_id(struct nt35560 *nt)
 	case DISPLAY_SONY_ACX424AKP_ID2:
 	case DISPLAY_SONY_ACX424AKP_ID3:
 	case DISPLAY_SONY_ACX424AKP_ID4:
-		dev_info(nt->dev, "MTP vendor: %02x, version: %02x, panel: %02x\n",
+		dev_info(&dev,
+			 "MTP vendor: %02x, version: %02x, panel: %02x\n",
 			 vendor, version, panel);
 		break;
 	default:
-		dev_info(nt->dev, "unknown vendor: %02x, version: %02x, panel: %02x\n",
+		dev_info(&dev,
+			 "unknown vendor: %02x, version: %02x, panel: %02x\n",
 			 vendor, version, panel);
 		break;
 	}
 
-	return 0;
+	return;
 }
 
 static int nt35560_power_on(struct nt35560 *nt)
@@ -322,92 +278,56 @@ static void nt35560_power_off(struct nt35560 *nt)
 static int nt35560_prepare(struct drm_panel *panel)
 {
 	struct nt35560 *nt = panel_to_nt35560(panel);
-	struct mipi_dsi_device *dsi = to_mipi_dsi_device(nt->dev);
-	const u8 mddi = 3;
+	struct mipi_dsi_multi_context dsi_ctx = {
+		.dsi = to_mipi_dsi_device(nt->dev)
+	};
 	int ret;
 
 	ret = nt35560_power_on(nt);
 	if (ret)
 		return ret;
 
-	ret = nt35560_read_id(nt);
-	if (ret) {
-		dev_err(nt->dev, "failed to read panel ID (%d)\n", ret);
-		goto err_power_off;
-	}
+	nt35560_read_id(&dsi_ctx);
 
-	/* Enabe tearing mode: send TE (tearing effect) at VBLANK */
-	ret = mipi_dsi_dcs_set_tear_on(dsi,
+	/* Enable tearing mode: send TE (tearing effect) at VBLANK */
+	mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx,
 				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
-	if (ret) {
-		dev_err(nt->dev, "failed to enable vblank TE (%d)\n", ret);
-		goto err_power_off;
-	}
 
 	/*
 	 * Set MDDI
 	 *
 	 * This presumably deactivates the Qualcomm MDDI interface and
 	 * selects DSI, similar code is found in other drivers such as the
-	 * Sharp LS043T1LE01 which makes us suspect that this panel may be
-	 * using a Novatek NT35565 or similar display driver chip that shares
-	 * this command. Due to the lack of documentation we cannot know for
-	 * sure.
+	 * Sharp LS043T1LE01
 	 */
-	ret = mipi_dsi_dcs_write(dsi, NT35560_DCS_SET_MDDI,
-				 &mddi, sizeof(mddi));
-	if (ret < 0) {
-		dev_err(nt->dev, "failed to set MDDI (%d)\n", ret);
-		goto err_power_off;
-	}
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, NT35560_DCS_SET_MDDI, 3);
 
-	/* Exit sleep mode */
-	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
-	if (ret) {
-		dev_err(nt->dev, "failed to exit sleep mode (%d)\n", ret);
-		goto err_power_off;
-	}
-	msleep(140);
+	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
+	mipi_dsi_msleep(&dsi_ctx, 140);
 
-	ret = mipi_dsi_dcs_set_display_on(dsi);
-	if (ret) {
-		dev_err(nt->dev, "failed to turn display on (%d)\n", ret);
-		goto err_power_off;
-	}
+	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
 	if (nt->video_mode) {
-		/* In video mode turn peripheral on */
-		ret = mipi_dsi_turn_on_peripheral(dsi);
-		if (ret) {
-			dev_err(nt->dev, "failed to turn on peripheral\n");
-			goto err_power_off;
-		}
+		mipi_dsi_turn_on_peripheral_multi(&dsi_ctx);
 	}
 
-	return 0;
-
-err_power_off:
-	nt35560_power_off(nt);
-	return ret;
+	if (dsi_ctx.accum_err < 0)
+		nt35560_power_off(nt);
+	return dsi_ctx.accum_err;
 }
 
 static int nt35560_unprepare(struct drm_panel *panel)
 {
 	struct nt35560 *nt = panel_to_nt35560(panel);
-	struct mipi_dsi_device *dsi = to_mipi_dsi_device(nt->dev);
-	int ret;
+	struct mipi_dsi_multi_context dsi_ctx = {
+		.dsi = to_mipi_dsi_device(nt->dev)
+	};
 
-	ret = mipi_dsi_dcs_set_display_off(dsi);
-	if (ret) {
-		dev_err(nt->dev, "failed to turn display off (%d)\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
+
+	if (dsi_ctx.accum_err < 0)
+		return dsi_ctx.accum_err;
 
-	/* Enter sleep mode */
-	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
-	if (ret) {
-		dev_err(nt->dev, "failed to enter sleep mode (%d)\n", ret);
-		return ret;
-	}
 	msleep(85);
 
 	nt35560_power_off(nt);
-- 
2.50.1

Re: [PATCH 2/2] drm/panel: novatek-nt35560: Fix bug and clean up

[Doug Anderson]

Hi,

On Thu, Jul 24, 2025 at 1:23 PM Brigham Campbell <me@brighamcampbell.com> wrote:
>
> Fix bug in nt35560_set_brightness() which causes the function to
> erroneously report an error. mipi_dsi_dcs_write() returns either a
> negative value when an error occurred or a positive number of bytes
> written when no error occurred. The buggy code reports and error under
> either condition.

My personal preference would be to code up the fix itself (without the
multi transition) as patch #1. That will make everyone's lives easier
with stable backports. You'll touch the same code twice in your
series, but it will keep it cleaner...


> Update driver to use the "multi" variants of MIPI functions which
> facilitate improved error handling and cleaner driver code.
>
> Fixes: 7835ed6a9e86 ("drm/panel-sony-acx424akp: Modernize backlight handling")
> Signed-off-by: Brigham Campbell <me@brighamcampbell.com>
> ---
>
> The usage of the u8 array, mipi_buf_out, in nt35560_set_brightness() may
> be a little curious. It's useful here because pwm_ratio and pwm_div
> aren't constant, therefore we must store them in a buffer at runtime.
>
> Using mipi_dsi_dcs_write_{seq,buffer}_multi() in place of
> mipi_dsi_dcs_write() gives the added benefit that kmalloc() isn't used
> to write mipi commands.

Ah, this makes sense. We've seen this before, but I keep forgetting
about it. Thanks for mentioning it. I wonder if it makes sense to have
variants of mipi_dsi_generic_write_seq_multi() and
mipi_dsi_dcs_write_seq_multi() that take non-const data. The only
difference would be that the array they declare on the stack would be
a "const" array instead of a "static const" array...


> The jdi-lpm102a188a driver's unprepare() function will ignore errors
> reported by mipi_dsi_dcs_{set_display_off,enter_sleep_mode}. This
> driver, however, will fail to unprepare the panel if either function
> returns an error. The behavior of the jdi-lpm102a188a panel makes more
> sense to me, but I strongly prefer not to change the behavior of this
> driver without personally having access to hardware to test.

Makes sense to me.


> @@ -176,62 +173,28 @@ static int nt35560_set_brightness(struct backlight_device *bl)
>
>         /* Set up PWM dutycycle ONE byte (differs from the standard) */
>         dev_dbg(nt->dev, "calculated duty cycle %02x\n", pwm_ratio);
> -       ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
> -                                &pwm_ratio, 1);
> -       if (ret < 0) {
> -               dev_err(nt->dev, "failed to set display PWM ratio (%d)\n", ret);
> -               return ret;
> -       }
>
> -       /*
> -        * Sequence to write PWMDIV:
> -        *      address         data
> -        *      0xF3            0xAA   CMD2 Unlock
> -        *      0x00            0x01   Enter CMD2 page 0
> -        *      0X7D            0x01   No reload MTP of CMD2 P1
> -        *      0x22            PWMDIV
> -        *      0x7F            0xAA   CMD2 page 1 lock
> -        */

The above comment was useful. Can you keep it?


> @@ -278,16 +232,18 @@ static int nt35560_read_id(struct nt35560 *nt)
>         case DISPLAY_SONY_ACX424AKP_ID2:
>         case DISPLAY_SONY_ACX424AKP_ID3:
>         case DISPLAY_SONY_ACX424AKP_ID4:
> -               dev_info(nt->dev, "MTP vendor: %02x, version: %02x, panel: %02x\n",
> +               dev_info(&dev,
> +                        "MTP vendor: %02x, version: %02x, panel: %02x\n",
>                          vendor, version, panel);
>                 break;
>         default:
> -               dev_info(nt->dev, "unknown vendor: %02x, version: %02x, panel: %02x\n",
> +               dev_info(&dev,
> +                        "unknown vendor: %02x, version: %02x, panel: %02x\n",
>                          vendor, version, panel);
>                 break;
>         }
>
> -       return 0;
> +       return;
>  }

nit: no need for explicit return here, right?


> @@ -322,92 +278,56 @@ static void nt35560_power_off(struct nt35560 *nt)
>  static int nt35560_prepare(struct drm_panel *panel)
>  {
>         struct nt35560 *nt = panel_to_nt35560(panel);
> -       struct mipi_dsi_device *dsi = to_mipi_dsi_device(nt->dev);
> -       const u8 mddi = 3;
> +       struct mipi_dsi_multi_context dsi_ctx = {
> +               .dsi = to_mipi_dsi_device(nt->dev)
> +       };
>         int ret;
>
>         ret = nt35560_power_on(nt);
>         if (ret)
>                 return ret;
>
> -       ret = nt35560_read_id(nt);
> -       if (ret) {
> -               dev_err(nt->dev, "failed to read panel ID (%d)\n", ret);
> -               goto err_power_off;
> -       }
> +       nt35560_read_id(&dsi_ctx);
>
> -       /* Enabe tearing mode: send TE (tearing effect) at VBLANK */
> -       ret = mipi_dsi_dcs_set_tear_on(dsi,
> +       /* Enable tearing mode: send TE (tearing effect) at VBLANK */
> +       mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx,
>                                        MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> -       if (ret) {
> -               dev_err(nt->dev, "failed to enable vblank TE (%d)\n", ret);
> -               goto err_power_off;
> -       }
>
>         /*
>          * Set MDDI
>          *
>          * This presumably deactivates the Qualcomm MDDI interface and
>          * selects DSI, similar code is found in other drivers such as the
> -        * Sharp LS043T1LE01 which makes us suspect that this panel may be
> -        * using a Novatek NT35565 or similar display driver chip that shares
> -        * this command. Due to the lack of documentation we cannot know for
> -        * sure.
> +        * Sharp LS043T1LE01

Ah, this is the obsolete comment that you removed and talked about
"after the cut". You could probably include that info in the commit
message itself since someone looking at the commit later would
otherwise not know why this info was removed.

Also, nit: you should end your sentence with a period. :-)


Overall this looks like a nicely done cleanup. Thanks! ...just a few
small nits...


-Doug

Re: [PATCH 2/2] drm/panel: novatek-nt35560: Fix bug and clean up

[Brigham Campbell]

On Fri Jul 25, 2025 at 3:17 PM MDT, Doug Anderson wrote:
> Hi,
>
> On Thu, Jul 24, 2025 at 1:23 PM Brigham Campbell <me@brighamcampbell.com> wrote:
>>
>> Fix bug in nt35560_set_brightness() which causes the function to
>> erroneously report an error. mipi_dsi_dcs_write() returns either a
>> negative value when an error occurred or a positive number of bytes
>> written when no error occurred. The buggy code reports and error under
>> either condition.
>
> My personal preference would be to code up the fix itself (without the
> multi transition) as patch #1. That will make everyone's lives easier
> with stable backports. You'll touch the same code twice in your
> series, but it will keep it cleaner...

Oh, this is good to know. It makes sense to me that a lazer-focused bug
fix would be less likely to conflict with other changes in stable
branches and would be easier to resolve in the case of conflict. I'll
just fix the bug in patch 1/3 of v2.

>> The usage of the u8 array, mipi_buf_out, in nt35560_set_brightness() may
>> be a little curious. It's useful here because pwm_ratio and pwm_div
>> aren't constant, therefore we must store them in a buffer at runtime.
>>
>> Using mipi_dsi_dcs_write_{seq,buffer}_multi() in place of
>> mipi_dsi_dcs_write() gives the added benefit that kmalloc() isn't used
>> to write mipi commands.
>
> Ah, this makes sense. We've seen this before, but I keep forgetting
> about it. Thanks for mentioning it. I wonder if it makes sense to have
> variants of mipi_dsi_generic_write_seq_multi() and
> mipi_dsi_dcs_write_seq_multi() that take non-const data. The only
> difference would be that the array they declare on the stack would be
> a "const" array instead of a "static const" array...

Ok, I've thought about this one for a while. The problem with my patch
as it is now is that it uses a u8 array, mipi_buf_out, to construct MIPI
messages and send them out. My patch reuses mipi_buf_out because it
happens to be the right size for both messages which need to be
constructed at runtime. Not a super clean solution, perhaps.

The Novatek NT35950 has a better solution. See the following function
from drivers/gpu/drm/panel/panel-novatek-nt35950.c:107:

static void nt35950_set_cmd2_page(struct mipi_dsi_multi_context *dsi_ctx,
				  struct nt35950 *nt, u8 page)
{
	const u8 mauc_cmd2_page[] = { MCS_CMD_MAUCCTR, 0x55, 0xaa, 0x52,
				      0x08, page };

	mipi_dsi_dcs_write_buffer_multi(dsi_ctx, mauc_cmd2_page,
					ARRAY_SIZE(mauc_cmd2_page));
	if (!dsi_ctx->accum_err)
		nt->last_page = page;
}

The driver has a couple different functions like this and they're all
for the express purpose of writing out a single MIPI buffer which is
constructed at runtime.

Arguably, a more readable solution would involve the definition of a new
non-static macro like you suggest. The macro's `do {} while 0;` block
would achieve effectively the exact same effect as the functions in the
NT35950 driver, causing the buffer to be popped off the stack as soon as
the code inside the macro completed.

We could call it mipi_dsi_dcs_write_var_seq_multi(), perhaps. Or
mipi_dsi_dcs_write_sequence_of_bytes_determined_at_runtime_multi()? ...
(Help! I genuinely don't know what I would call it...)

Please let me know if you'd prefer that in v2 I adopt the approach that
the NT35950 driver uses or that I instead introduce a new macro for
non-static data.

I'll address the rest of your comments in v2.

Thanks again for another thorough review,
Brigham

Re: [PATCH 2/2] drm/panel: novatek-nt35560: Fix bug and clean up

[Doug Anderson]

Hi,

On Sun, Jul 27, 2025 at 11:04 PM Brigham Campbell
<me@brighamcampbell.com> wrote:
>
> >> Using mipi_dsi_dcs_write_{seq,buffer}_multi() in place of
> >> mipi_dsi_dcs_write() gives the added benefit that kmalloc() isn't used
> >> to write mipi commands.
> >
> > Ah, this makes sense. We've seen this before, but I keep forgetting
> > about it. Thanks for mentioning it. I wonder if it makes sense to have
> > variants of mipi_dsi_generic_write_seq_multi() and
> > mipi_dsi_dcs_write_seq_multi() that take non-const data. The only
> > difference would be that the array they declare on the stack would be
> > a "const" array instead of a "static const" array...
>
> Ok, I've thought about this one for a while. The problem with my patch
> as it is now is that it uses a u8 array, mipi_buf_out, to construct MIPI
> messages and send them out. My patch reuses mipi_buf_out because it
> happens to be the right size for both messages which need to be
> constructed at runtime. Not a super clean solution, perhaps.
>
> The Novatek NT35950 has a better solution. See the following function
> from drivers/gpu/drm/panel/panel-novatek-nt35950.c:107:
>
> static void nt35950_set_cmd2_page(struct mipi_dsi_multi_context *dsi_ctx,
>                                   struct nt35950 *nt, u8 page)
> {
>         const u8 mauc_cmd2_page[] = { MCS_CMD_MAUCCTR, 0x55, 0xaa, 0x52,
>                                       0x08, page };
>
>         mipi_dsi_dcs_write_buffer_multi(dsi_ctx, mauc_cmd2_page,
>                                         ARRAY_SIZE(mauc_cmd2_page));
>         if (!dsi_ctx->accum_err)
>                 nt->last_page = page;
> }
>
> The driver has a couple different functions like this and they're all
> for the express purpose of writing out a single MIPI buffer which is
> constructed at runtime.
>
> Arguably, a more readable solution would involve the definition of a new
> non-static macro like you suggest. The macro's `do {} while 0;` block
> would achieve effectively the exact same effect as the functions in the
> NT35950 driver, causing the buffer to be popped off the stack as soon as
> the code inside the macro completed.
>
> We could call it mipi_dsi_dcs_write_var_seq_multi(), perhaps. Or
> mipi_dsi_dcs_write_sequence_of_bytes_determined_at_runtime_multi()? ...
> (Help! I genuinely don't know what I would call it...)
>
> Please let me know if you'd prefer that in v2 I adopt the approach that
> the NT35950 driver uses or that I instead introduce a new macro for
> non-static data.

The absolute best would be if one function could somehow magically do
the right thing. It's something that would be better if the caller
didn't need to think about it. It might be possible to construct some
ugly snarl of macro using __builtin_constant_p() (maybe requiring
__VA_OPT__?), but I don't see anything super clean for it
unfortunately.

IMO adding something like mipi_dsi_dcs_write_var_seq_multi() would be
the best. The name is a little unwieldy, but I can't think of better
options and I suspect this will come in handy for over drivers as
well...

-Doug

Re: [PATCH 0/2] drm/panel: novatek-nt35560: Fix bug and clean up

[Doug Anderson]

Hi,

On Thu, Jul 24, 2025 at 1:23 PM Brigham Campbell <me@brighamcampbell.com> wrote:
>
> This series does the following:
>  - Add mipi_dsi_dcs_read_multi() to drm_mipi_dsi.c for improved error
>    handling in drivers which use mipi_dsi_dcs_read() such as the
>    novatek-nt35560 driver.
>  - Fix a bug in nt35560_set_brightness() which causes the driver to
>    incorrectly report that it "failed to disable display backlight".
>  - Clean up novatek-nt35560 driver to use "multi" variants of MIPI
>    functions which promote cleaner code.
>
> If there's a reason that mipi_dsi_dcs_read_multi doesn't exist, I'm
> happy to drop it.

It's a _little_ weird since one would normally assume that the caller
would want to immediately act on whatever was read and to do that
they'd need to look at the error code right away.

...but then again, there are places where a driver might want to read
multiple things and then check for errors once in the end (like the
driver you're changing here does). So I'm not against it. I'm not sure
I'd expect it to be universally used, though...