[PATCH v3 0/4] drm: Fix bug in panel driver, update MIPI support macros

[PATCH v3 0/4] drm: Fix bug in panel driver, update MIPI support macros

[Brigham Campbell]

This series removes the unintuitive mipi_dsi_generic_write_seq() macro
and related mipi_dsi_generic_write_chatty() method from the drm
subsystem. This is in accordance with a TODO item from Douglas Anderson
in the drm subsystem documentation. Tejas Vipin (among others) has
largely spearheaded this effort up until now, converting MIPI panel
drivers one at a time.

The second patch of the series removes the last remaining references to
mipi_dsi_generic_write_seq() in the jdi-lpm102a188a driver and updates
the driver to use the undeprecated _multi variants of MIPI functions. It
fixes a bug in the driver's unprepare function and cleans up duplicated
code using the new mipi_dsi_dual macro introduced in the first patch.

changes to v3:
 - Define new mipi_dsi_dual macro in drm_mipi_dsi.h to reduce code
   duplication.
 - Fix bug in lpm102a188a panel driver's unprepare function which causes
   it to return a nonsensical value.
 - Make lpm102a188a panel driver's unprepare function send "display off"
   and "enter sleep mode" commands to both serial interfaces regardless
   of whether an error occurred when sending the last command.

changes to v2:
 - Remove all usages of deprecated MIPI functions from jdi-lpm102a188a
   driver instead of just mipi_dsi_generic_write_seq().
 - Update TODO item in drm documentation instead of removing it
   entirely.

Brigham Campbell (4):
  drm: Create mipi_dsi_dual macro
  drm/panel: jdi-lpm102a188a: Fix bug and clean up driver
  drm: Remove unused MIPI write seq and chatty functions
  drm: docs: Update task from drm TODO list

 Documentation/gpu/todo.rst                    |  22 +--
 drivers/gpu/drm/drm_mipi_dsi.c                |  34 +---
 drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c | 187 ++++++------------
 include/drm/drm_mipi_dsi.h                    |  46 ++---
 4 files changed, 92 insertions(+), 197 deletions(-)

v2: https://lore.kernel.org/all/20250708073901.90027-1-me@brighamcampbell.com/
v1: https://lore.kernel.org/all/20250707075659.75810-1-me@brighamcampbell.com/

base-commit: 667efb341917bde19f5d7517b65defcdaed67c9e
-- 
2.49.0

[PATCH v3 1/4] drm: Create mipi_dsi_dual macro

[Brigham Campbell]

Create mipi_dsi_dual macro for panels which are driven by two parallel
serial interfaces. This allows for the reduction of code duplication in
drivers for these panels.

Signed-off-by: Brigham Campbell <me@brighamcampbell.com>
---

In case it's not obvious, the indirection between mipi_dsi_dual and
_mipi_dsi_dual is meant to allow for the expansion of _func in the case
that _func is also a macro.

 include/drm/drm_mipi_dsi.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 369b0d8830c3..32da8861f9de 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -431,6 +431,30 @@ void mipi_dsi_dcs_set_tear_off_multi(struct mipi_dsi_multi_context *ctx);
 		mipi_dsi_dcs_write_buffer_multi(ctx, d, ARRAY_SIZE(d)); \
 	} while (0)
 
+/**
+ * mipi_dsi_dual - send the same MIPI DSI command to two interfaces
+ *
+ * This macro will send the specified MIPI DSI command twice, once per each of
+ * the two interfaces supplied. This is useful for reducing duplication of code
+ * in panel drivers which use two parallel serial interfaces.
+ *
+ * @_func: MIPI DSI function or macro to pass context and arguments into
+ * @_dsi1: First DSI interface to act as recipient of the MIPI DSI command
+ * @_dsi2: Second DSI interface to act as recipient of the MIPI DSI command
+ * @_ctx: Context for multiple DSI transactions
+ * @...: Arguments to pass to MIPI DSI function or macro
+ */
+#define mipi_dsi_dual(_func, _dsi1, _dsi2, _ctx, ...) 		 \
+	_mipi_dsi_dual(_func, _dsi1, _dsi2, _ctx, ##__VA_ARGS__)
+
+#define _mipi_dsi_dual(_func, _dsi1, _dsi2, _ctx, ...) \
+	do {					       \
+		(_ctx)->dsi = (_dsi1);		       \
+		_func((_ctx), ##__VA_ARGS__);	       \
+		(_ctx)->dsi = (_dsi2);		       \
+		_func((_ctx), ##__VA_ARGS__);	       \
+	} while (0)
+
 /**
  * struct mipi_dsi_driver - DSI driver
  * @driver: device driver model driver
-- 
2.49.0

[PATCH v3 2/4] drm/panel: jdi-lpm102a188a: Fix bug and clean up driver

[Brigham Campbell]

Fix bug in unprepare() which causes the function's return value to be
that of the last mipi "enter sleep mode" command.

Update driver to use the "multi" variant of MIPI functions in order to
facilitate improved error handling and remove the panel's dependency on
deprecated MIPI functions.

Use the new mipi_dsi_dual macro to reduce code duplication.

Signed-off-by: Brigham Campbell <me@brighamcampbell.com>
---
 drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c | 187 ++++++------------
 1 file changed, 55 insertions(+), 132 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
index 5b5082efb282..b72b6518a090 100644
--- a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
+++ b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
@@ -81,25 +81,25 @@ static int jdi_panel_disable(struct drm_panel *panel)
 static int jdi_panel_unprepare(struct drm_panel *panel)
 {
 	struct jdi_panel *jdi = to_panel_jdi(panel);
-	int ret;
 
-	ret = mipi_dsi_dcs_set_display_off(jdi->link1);
-	if (ret < 0)
-		dev_err(panel->dev, "failed to set display off: %d\n", ret);
+	/*
+	 * One context per panel since we'll continue trying to shut down the
+	 * other panel even if one isn't responding.
+	 */
+	struct mipi_dsi_multi_context dsi_ctx1 = { .dsi = jdi->link1 };
+	struct mipi_dsi_multi_context dsi_ctx2 = { .dsi = jdi->link2 };
 
-	ret = mipi_dsi_dcs_set_display_off(jdi->link2);
-	if (ret < 0)
-		dev_err(panel->dev, "failed to set display off: %d\n", ret);
+	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx1);
+	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx2);
 
 	/* Specified by JDI @ 50ms, subject to change */
 	msleep(50);
 
-	ret = mipi_dsi_dcs_enter_sleep_mode(jdi->link1);
-	if (ret < 0)
-		dev_err(panel->dev, "failed to enter sleep mode: %d\n", ret);
-	ret = mipi_dsi_dcs_enter_sleep_mode(jdi->link2);
-	if (ret < 0)
-		dev_err(panel->dev, "failed to enter sleep mode: %d\n", ret);
+	/* Doesn't hurt to try sleep mode even if display off fails */
+	dsi_ctx1.accum_err = 0;
+	dsi_ctx2.accum_err = 0;
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx1);
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx2);
 
 	/* Specified by JDI @ 150ms, subject to change */
 	msleep(150);
@@ -123,72 +123,47 @@ static int jdi_panel_unprepare(struct drm_panel *panel)
 	/* Specified by JDI @ 20ms, subject to change */
 	msleep(20);
 
-	return ret;
+	return 0;
 }
 
-static int jdi_setup_symmetrical_split(struct mipi_dsi_device *left,
+static void jdi_setup_symmetrical_split(struct mipi_dsi_multi_context *dsi_ctx,
+				       struct mipi_dsi_device *left,
 				       struct mipi_dsi_device *right,
 				       const struct drm_display_mode *mode)
 {
-	int err;
-
-	err = mipi_dsi_dcs_set_column_address(left, 0, mode->hdisplay / 2 - 1);
-	if (err < 0) {
-		dev_err(&left->dev, "failed to set column address: %d\n", err);
-		return err;
-	}
-
-	err = mipi_dsi_dcs_set_column_address(right, 0, mode->hdisplay / 2 - 1);
-	if (err < 0) {
-		dev_err(&right->dev, "failed to set column address: %d\n", err);
-		return err;
-	}
-
-	err = mipi_dsi_dcs_set_page_address(left, 0, mode->vdisplay - 1);
-	if (err < 0) {
-		dev_err(&left->dev, "failed to set page address: %d\n", err);
-		return err;
-	}
-
-	err = mipi_dsi_dcs_set_page_address(right, 0, mode->vdisplay - 1);
-	if (err < 0) {
-		dev_err(&right->dev, "failed to set page address: %d\n", err);
-		return err;
-	}
-
-	return 0;
+	mipi_dsi_dual(mipi_dsi_dcs_set_column_address_multi,
+			left, right, dsi_ctx,
+			0, mode->hdisplay / 2 - 1);
+	mipi_dsi_dual(mipi_dsi_dcs_set_page_address_multi,
+			left, right, dsi_ctx,
+			0, mode->vdisplay - 1);
 }
 
-static int jdi_write_dcdc_registers(struct jdi_panel *jdi)
+static void jdi_write_dcdc_registers(struct mipi_dsi_multi_context *dsi_ctx,
+				    struct jdi_panel *jdi)
 {
 	/* Clear the manufacturer command access protection */
-	mipi_dsi_generic_write_seq(jdi->link1, MCS_CMD_ACS_PROT,
-				   MCS_CMD_ACS_PROT_OFF);
-	mipi_dsi_generic_write_seq(jdi->link2, MCS_CMD_ACS_PROT,
-				   MCS_CMD_ACS_PROT_OFF);
+	mipi_dsi_dual(mipi_dsi_generic_write_seq_multi,
+			jdi->link1, jdi->link2, dsi_ctx,
+			MCS_CMD_ACS_PROT, MCS_CMD_ACS_PROT_OFF);
 	/*
-	 * Change the VGH/VGL divide rations to move the noise generated by the
+	 * Change the VGH/VGL divide ratios to move the noise generated by the
 	 * TCONN. This should hopefully avoid interaction with the backlight
 	 * controller.
 	 */
-	mipi_dsi_generic_write_seq(jdi->link1, MCS_PWR_CTRL_FUNC,
-				   MCS_PWR_CTRL_PARAM1_VGH_330_DIV |
-				   MCS_PWR_CTRL_PARAM1_DEFAULT,
-				   MCS_PWR_CTRL_PARAM2_VGL_410_DIV |
-				   MCS_PWR_CTRL_PARAM2_DEFAULT);
-
-	mipi_dsi_generic_write_seq(jdi->link2, MCS_PWR_CTRL_FUNC,
-				   MCS_PWR_CTRL_PARAM1_VGH_330_DIV |
-				   MCS_PWR_CTRL_PARAM1_DEFAULT,
-				   MCS_PWR_CTRL_PARAM2_VGL_410_DIV |
-				   MCS_PWR_CTRL_PARAM2_DEFAULT);
-
-	return 0;
+	mipi_dsi_dual(mipi_dsi_generic_write_seq_multi,
+			jdi->link1, jdi->link2, dsi_ctx,
+			MCS_PWR_CTRL_FUNC,
+			MCS_PWR_CTRL_PARAM1_VGH_330_DIV |
+			MCS_PWR_CTRL_PARAM1_DEFAULT,
+			MCS_PWR_CTRL_PARAM2_VGL_410_DIV |
+			MCS_PWR_CTRL_PARAM2_DEFAULT);
 }
 
 static int jdi_panel_prepare(struct drm_panel *panel)
 {
 	struct jdi_panel *jdi = to_panel_jdi(panel);
+	struct mipi_dsi_multi_context dsi_ctx;
 	int err;
 
 	/* Disable backlight to avoid showing random pixels
@@ -231,88 +206,36 @@ static int jdi_panel_prepare(struct drm_panel *panel)
 	 * put in place to communicate the configuration back to the DSI host
 	 * controller.
 	 */
-	err = jdi_setup_symmetrical_split(jdi->link1, jdi->link2,
+	jdi_setup_symmetrical_split(&dsi_ctx, jdi->link1, jdi->link2,
 					  jdi->mode);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set up symmetrical split: %d\n",
-			err);
-		goto poweroff;
-	}
 
-	err = mipi_dsi_dcs_set_tear_scanline(jdi->link1,
-					     jdi->mode->vdisplay - 16);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set tear scanline: %d\n", err);
-		goto poweroff;
-	}
+	mipi_dsi_dual(mipi_dsi_dcs_set_tear_scanline_multi,
+			jdi->link1, jdi->link2, &dsi_ctx,
+			jdi->mode->vdisplay - 16);
 
-	err = mipi_dsi_dcs_set_tear_scanline(jdi->link2,
-					     jdi->mode->vdisplay - 16);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set tear scanline: %d\n", err);
-		goto poweroff;
-	}
+	mipi_dsi_dual(mipi_dsi_dcs_set_tear_on_multi,
+			jdi->link1, jdi->link2, &dsi_ctx,
+			MIPI_DSI_DCS_TEAR_MODE_VBLANK);
 
-	err = mipi_dsi_dcs_set_tear_on(jdi->link1,
-				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set tear on: %d\n", err);
-		goto poweroff;
-	}
+	mipi_dsi_dual(mipi_dsi_dcs_set_pixel_format_multi,
+			jdi->link1, jdi->link2, &dsi_ctx,
+			MIPI_DCS_PIXEL_FMT_24BIT);
 
-	err = mipi_dsi_dcs_set_tear_on(jdi->link2,
-				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set tear on: %d\n", err);
-		goto poweroff;
-	}
+	mipi_dsi_dual(mipi_dsi_dcs_exit_sleep_mode_multi,
+			jdi->link1, jdi->link2, &dsi_ctx);
 
-	err = mipi_dsi_dcs_set_pixel_format(jdi->link1, MIPI_DCS_PIXEL_FMT_24BIT);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set pixel format: %d\n", err);
-		goto poweroff;
-	}
-
-	err = mipi_dsi_dcs_set_pixel_format(jdi->link2, MIPI_DCS_PIXEL_FMT_24BIT);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set pixel format: %d\n", err);
-		goto poweroff;
-	}
-
-	err = mipi_dsi_dcs_exit_sleep_mode(jdi->link1);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
-		goto poweroff;
-	}
-
-	err = mipi_dsi_dcs_exit_sleep_mode(jdi->link2);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
-		goto poweroff;
-	}
-
-	err = jdi_write_dcdc_registers(jdi);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to write dcdc registers: %d\n", err);
-		goto poweroff;
-	}
+	jdi_write_dcdc_registers(&dsi_ctx, jdi);
 	/*
-	 * We need to wait 150ms between mipi_dsi_dcs_exit_sleep_mode() and
-	 * mipi_dsi_dcs_set_display_on().
+	 * We need to wait 150ms between mipi_dsi_dcs_exit_sleep_mode_multi()
+	 * and mipi_dsi_dcs_set_display_on_multi().
 	 */
-	msleep(150);
+	mipi_dsi_msleep(&dsi_ctx, 150);
 
-	err = mipi_dsi_dcs_set_display_on(jdi->link1);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set display on: %d\n", err);
-		goto poweroff;
-	}
+	mipi_dsi_dual(mipi_dsi_dcs_set_display_on_multi,
+			jdi->link1, jdi->link2, &dsi_ctx);
 
-	err = mipi_dsi_dcs_set_display_on(jdi->link2);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set display on: %d\n", err);
+	if (dsi_ctx.accum_err < 0)
 		goto poweroff;
-	}
 
 	jdi->link1->mode_flags &= ~MIPI_DSI_MODE_LPM;
 	jdi->link2->mode_flags &= ~MIPI_DSI_MODE_LPM;
-- 
2.49.0

[PATCH v3 3/4] drm: Remove unused MIPI write seq and chatty functions

[Brigham Campbell]

Remove the deprecated mipi_dsi_generic_write_seq() and
mipi_dsi_generic_write_chatty() functions now that they are no longer
used.

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Brigham Campbell <me@brighamcampbell.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 34 +++-------------------------------
 include/drm/drm_mipi_dsi.h     | 23 -----------------------
 2 files changed, 3 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index a00d76443128..3b8ff24980b4 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -772,41 +772,13 @@ ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,
 EXPORT_SYMBOL(mipi_dsi_generic_write);
 
 /**
- * mipi_dsi_generic_write_chatty() - mipi_dsi_generic_write() w/ an error log
- * @dsi: DSI peripheral device
- * @payload: buffer containing the payload
- * @size: size of payload buffer
- *
- * Like mipi_dsi_generic_write() but includes a dev_err()
- * call for you and returns 0 upon success, not the number of bytes sent.
- *
- * Return: 0 on success or a negative error code on failure.
- */
-int mipi_dsi_generic_write_chatty(struct mipi_dsi_device *dsi,
-				  const void *payload, size_t size)
-{
-	struct device *dev = &dsi->dev;
-	ssize_t ret;
-
-	ret = mipi_dsi_generic_write(dsi, payload, size);
-	if (ret < 0) {
-		dev_err(dev, "sending generic data %*ph failed: %zd\n",
-			(int)size, payload, ret);
-		return ret;
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL(mipi_dsi_generic_write_chatty);
-
-/**
- * mipi_dsi_generic_write_multi() - mipi_dsi_generic_write_chatty() w/ accum_err
+ * mipi_dsi_generic_write_multi() - mipi_dsi_generic_write() w/ accum_err
  * @ctx: Context for multiple DSI transactions
  * @payload: buffer containing the payload
  * @size: size of payload buffer
  *
- * Like mipi_dsi_generic_write_chatty() but deals with errors in a way that
- * makes it convenient to make several calls in a row.
+ * A wrapper around mipi_dsi_generic_write() that deals with errors in a way
+ * that makes it convenient to make several calls in a row.
  */
 void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
 				  const void *payload, size_t size)
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 32da8861f9de..526308e24f6c 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -285,8 +285,6 @@ void mipi_dsi_picture_parameter_set_multi(struct mipi_dsi_multi_context *ctx,
 
 ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,
 			       size_t size);
-int mipi_dsi_generic_write_chatty(struct mipi_dsi_device *dsi,
-				  const void *payload, size_t size);
 void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
 				  const void *payload, size_t size);
 ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
@@ -379,27 +377,6 @@ void mipi_dsi_dcs_set_tear_scanline_multi(struct mipi_dsi_multi_context *ctx,
 					  u16 scanline);
 void mipi_dsi_dcs_set_tear_off_multi(struct mipi_dsi_multi_context *ctx);
 
-/**
- * mipi_dsi_generic_write_seq - transmit data using a generic write packet
- *
- * This macro will print errors for you and will RETURN FROM THE CALLING
- * FUNCTION (yes this is non-intuitive) upon error.
- *
- * Because of the non-intuitive return behavior, THIS MACRO IS DEPRECATED.
- * Please replace calls of it with mipi_dsi_generic_write_seq_multi().
- *
- * @dsi: DSI peripheral device
- * @seq: buffer containing the payload
- */
-#define mipi_dsi_generic_write_seq(dsi, seq...)                                \
-	do {                                                                   \
-		static const u8 d[] = { seq };                                 \
-		int ret;                                                       \
-		ret = mipi_dsi_generic_write_chatty(dsi, d, ARRAY_SIZE(d));    \
-		if (ret < 0)                                                   \
-			return ret;                                            \
-	} while (0)
-
 /**
  * mipi_dsi_generic_write_seq_multi - transmit data using a generic write packet
  *
-- 
2.49.0

[PATCH v3 4/4] drm: docs: Update task from drm TODO list

[Brigham Campbell]

Update TODO item from drm documentation to contain more applicable
information regarding the removal of deprecated MIPI DSI functions and
no longer reference functions which have already been removed from the
kernel.

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Brigham Campbell <me@brighamcampbell.com>
---
 Documentation/gpu/todo.rst | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index be8637da3fe9..92db80793bba 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -497,19 +497,19 @@ Contact: Douglas Anderson <dianders@chromium.org>
 
 Level: Intermediate
 
-Transition away from using mipi_dsi_*_write_seq()
--------------------------------------------------
+Transition away from using deprecated MIPI DSI functions
+--------------------------------------------------------
 
-The macros mipi_dsi_generic_write_seq() and mipi_dsi_dcs_write_seq() are
-non-intuitive because, if there are errors, they return out of the *caller's*
-function. We should move all callers to use mipi_dsi_generic_write_seq_multi()
-and mipi_dsi_dcs_write_seq_multi() macros instead.
+There are many functions defined in ``drm_mipi_dsi.c`` which have been
+deprecated. Each deprecated function was deprecated in favor of its `multi`
+variant (e.g. `mipi_dsi_generic_write()` and `mipi_dsi_generic_write_multi()`).
+The `multi` variant of a function includes improved error handling and logic
+which makes it more convenient to make several calls in a row, as most MIPI
+drivers do.
 
-Once all callers are transitioned, the macros and the functions that they call,
-mipi_dsi_generic_write_chatty() and mipi_dsi_dcs_write_buffer_chatty(), can
-probably be removed. Alternatively, if people feel like the _multi() variants
-are overkill for some use cases, we could keep the mipi_dsi_*_write_seq()
-variants but change them not to return out of the caller.
+Drivers should be updated to use undeprecated functions. Once all usages of the
+deprecated MIPI DSI functions have been removed, their definitions may be
+removed from ``drm_mipi_dsi.c``.
 
 Contact: Douglas Anderson <dianders@chromium.org>
 
-- 
2.49.0

Re: [PATCH v3 2/4] drm/panel: jdi-lpm102a188a: Fix bug and clean up driver

[Diogo Ivo]

Hi Brigham, thanks for the patch! The code looks a lot cleaner.
Some review follows.

On 7/17/25 7:57 AM, Brigham Campbell wrote:
> Fix bug in unprepare() which causes the function's return value to be
> that of the last mipi "enter sleep mode" command.
> 
> Update driver to use the "multi" variant of MIPI functions in order to
> facilitate improved error handling and remove the panel's dependency on
> deprecated MIPI functions.
> 
> Use the new mipi_dsi_dual macro to reduce code duplication.
> 
> Signed-off-by: Brigham Campbell <me@brighamcampbell.com>
> ---
>   drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c | 187 ++++++------------
>   1 file changed, 55 insertions(+), 132 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> index 5b5082efb282..b72b6518a090 100644
> --- a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> +++ b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> @@ -81,25 +81,25 @@ static int jdi_panel_disable(struct drm_panel *panel)
>   static int jdi_panel_unprepare(struct drm_panel *panel)
>   {
>   	struct jdi_panel *jdi = to_panel_jdi(panel);
> -	int ret;
>   
> -	ret = mipi_dsi_dcs_set_display_off(jdi->link1);
> -	if (ret < 0)
> -		dev_err(panel->dev, "failed to set display off: %d\n", ret);
> +	/*
> +	 * One context per panel since we'll continue trying to shut down the
> +	 * other panel even if one isn't responding.
> +	 */
> +	struct mipi_dsi_multi_context dsi_ctx1 = { .dsi = jdi->link1 };
> +	struct mipi_dsi_multi_context dsi_ctx2 = { .dsi = jdi->link2 };
>   
> -	ret = mipi_dsi_dcs_set_display_off(jdi->link2);
> -	if (ret < 0)
> -		dev_err(panel->dev, "failed to set display off: %d\n", ret);
> +	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx1);
> +	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx2);
>   
>   	/* Specified by JDI @ 50ms, subject to change */
>   	msleep(50);
>   
> -	ret = mipi_dsi_dcs_enter_sleep_mode(jdi->link1);
> -	if (ret < 0)
> -		dev_err(panel->dev, "failed to enter sleep mode: %d\n", ret);
> -	ret = mipi_dsi_dcs_enter_sleep_mode(jdi->link2);
> -	if (ret < 0)
> -		dev_err(panel->dev, "failed to enter sleep mode: %d\n", ret);
> +	/* Doesn't hurt to try sleep mode even if display off fails */
> +	dsi_ctx1.accum_err = 0;
> +	dsi_ctx2.accum_err = 0;
> +	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx1);
> +	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx2);
>   
>   	/* Specified by JDI @ 150ms, subject to change */
>   	msleep(150);
> @@ -123,72 +123,47 @@ static int jdi_panel_unprepare(struct drm_panel *panel)
>   	/* Specified by JDI @ 20ms, subject to change */
>   	msleep(20);
>   
> -	return ret;
> +	return 0;
>   }
>   
> -static int jdi_setup_symmetrical_split(struct mipi_dsi_device *left,
> +static void jdi_setup_symmetrical_split(struct mipi_dsi_multi_context *dsi_ctx,
> +				       struct mipi_dsi_device *left,
>   				       struct mipi_dsi_device *right,
>   				       const struct drm_display_mode *mode)

Nit: align the indentation of the function arguments, here and in the 
following function.

>   {
> -	int err;
> -
> -	err = mipi_dsi_dcs_set_column_address(left, 0, mode->hdisplay / 2 - 1);
> -	if (err < 0) {
> -		dev_err(&left->dev, "failed to set column address: %d\n", err);
> -		return err;
> -	}
> -
> -	err = mipi_dsi_dcs_set_column_address(right, 0, mode->hdisplay / 2 - 1);
> -	if (err < 0) {
> -		dev_err(&right->dev, "failed to set column address: %d\n", err);
> -		return err;
> -	}
> -
> -	err = mipi_dsi_dcs_set_page_address(left, 0, mode->vdisplay - 1);
> -	if (err < 0) {
> -		dev_err(&left->dev, "failed to set page address: %d\n", err);
> -		return err;
> -	}
> -
> -	err = mipi_dsi_dcs_set_page_address(right, 0, mode->vdisplay - 1);
> -	if (err < 0) {
> -		dev_err(&right->dev, "failed to set page address: %d\n", err);
> -		return err;
> -	}
> -
> -	return 0;
> +	mipi_dsi_dual(mipi_dsi_dcs_set_column_address_multi,
> +			left, right, dsi_ctx,
> +			0, mode->hdisplay / 2 - 1);
> +	mipi_dsi_dual(mipi_dsi_dcs_set_page_address_multi,
> +			left, right, dsi_ctx,
> +			0, mode->vdisplay - 1);

Nit: align the indentation of the function arguments, here and in the 
following function calls.

>   }
>   
> -static int jdi_write_dcdc_registers(struct jdi_panel *jdi)
> +static void jdi_write_dcdc_registers(struct mipi_dsi_multi_context *dsi_ctx,
> +				    struct jdi_panel *jdi)
>   {
>   	/* Clear the manufacturer command access protection */
> -	mipi_dsi_generic_write_seq(jdi->link1, MCS_CMD_ACS_PROT,
> -				   MCS_CMD_ACS_PROT_OFF);
> -	mipi_dsi_generic_write_seq(jdi->link2, MCS_CMD_ACS_PROT,
> -				   MCS_CMD_ACS_PROT_OFF);
> +	mipi_dsi_dual(mipi_dsi_generic_write_seq_multi,
> +			jdi->link1, jdi->link2, dsi_ctx,
> +			MCS_CMD_ACS_PROT, MCS_CMD_ACS_PROT_OFF);
>   	/*
> -	 * Change the VGH/VGL divide rations to move the noise generated by the
> +	 * Change the VGH/VGL divide ratios to move the noise generated by the
>   	 * TCONN. This should hopefully avoid interaction with the backlight
>   	 * controller.
>   	 */
> -	mipi_dsi_generic_write_seq(jdi->link1, MCS_PWR_CTRL_FUNC,
> -				   MCS_PWR_CTRL_PARAM1_VGH_330_DIV |
> -				   MCS_PWR_CTRL_PARAM1_DEFAULT,
> -				   MCS_PWR_CTRL_PARAM2_VGL_410_DIV |
> -				   MCS_PWR_CTRL_PARAM2_DEFAULT);
> -
> -	mipi_dsi_generic_write_seq(jdi->link2, MCS_PWR_CTRL_FUNC,
> -				   MCS_PWR_CTRL_PARAM1_VGH_330_DIV |
> -				   MCS_PWR_CTRL_PARAM1_DEFAULT,
> -				   MCS_PWR_CTRL_PARAM2_VGL_410_DIV |
> -				   MCS_PWR_CTRL_PARAM2_DEFAULT);
> -
> -	return 0;
> +	mipi_dsi_dual(mipi_dsi_generic_write_seq_multi,
> +			jdi->link1, jdi->link2, dsi_ctx,
> +			MCS_PWR_CTRL_FUNC,
> +			MCS_PWR_CTRL_PARAM1_VGH_330_DIV |
> +			MCS_PWR_CTRL_PARAM1_DEFAULT,
> +			MCS_PWR_CTRL_PARAM2_VGL_410_DIV |
> +			MCS_PWR_CTRL_PARAM2_DEFAULT);
>   }
>   
>   static int jdi_panel_prepare(struct drm_panel *panel)
>   {
>   	struct jdi_panel *jdi = to_panel_jdi(panel);
> +	struct mipi_dsi_multi_context dsi_ctx;

Here we need dsi_ctx = { .accum_err = 0 }; since by not initializing any
field of dsi_ctx we can't rely on accum_error being init'ed to 0.

>   	int err;
>   
>   	/* Disable backlight to avoid showing random pixels
> @@ -231,88 +206,36 @@ static int jdi_panel_prepare(struct drm_panel *panel)
>   	 * put in place to communicate the configuration back to the DSI host
>   	 * controller.
>   	 */
> -	err = jdi_setup_symmetrical_split(jdi->link1, jdi->link2,
> +	jdi_setup_symmetrical_split(&dsi_ctx, jdi->link1, jdi->link2,
>   					  jdi->mode);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to set up symmetrical split: %d\n",
> -			err);
> -		goto poweroff;
> -	}
>   
> -	err = mipi_dsi_dcs_set_tear_scanline(jdi->link1,
> -					     jdi->mode->vdisplay - 16);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to set tear scanline: %d\n", err);
> -		goto poweroff;
> -	}
> +	mipi_dsi_dual(mipi_dsi_dcs_set_tear_scanline_multi,
> +			jdi->link1, jdi->link2, &dsi_ctx,
> +			jdi->mode->vdisplay - 16);
>   
> -	err = mipi_dsi_dcs_set_tear_scanline(jdi->link2,
> -					     jdi->mode->vdisplay - 16);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to set tear scanline: %d\n", err);
> -		goto poweroff;
> -	}
> +	mipi_dsi_dual(mipi_dsi_dcs_set_tear_on_multi,
> +			jdi->link1, jdi->link2, &dsi_ctx,
> +			MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>   
> -	err = mipi_dsi_dcs_set_tear_on(jdi->link1,
> -				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to set tear on: %d\n", err);
> -		goto poweroff;
> -	}
> +	mipi_dsi_dual(mipi_dsi_dcs_set_pixel_format_multi,
> +			jdi->link1, jdi->link2, &dsi_ctx,
> +			MIPI_DCS_PIXEL_FMT_24BIT);
>   
> -	err = mipi_dsi_dcs_set_tear_on(jdi->link2,
> -				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to set tear on: %d\n", err);
> -		goto poweroff;
> -	}
> +	mipi_dsi_dual(mipi_dsi_dcs_exit_sleep_mode_multi,
> +			jdi->link1, jdi->link2, &dsi_ctx);
>   
> -	err = mipi_dsi_dcs_set_pixel_format(jdi->link1, MIPI_DCS_PIXEL_FMT_24BIT);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to set pixel format: %d\n", err);
> -		goto poweroff;
> -	}
> -
> -	err = mipi_dsi_dcs_set_pixel_format(jdi->link2, MIPI_DCS_PIXEL_FMT_24BIT);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to set pixel format: %d\n", err);
> -		goto poweroff;
> -	}
> -
> -	err = mipi_dsi_dcs_exit_sleep_mode(jdi->link1);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
> -		goto poweroff;
> -	}
> -
> -	err = mipi_dsi_dcs_exit_sleep_mode(jdi->link2);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
> -		goto poweroff;
> -	}
> -
> -	err = jdi_write_dcdc_registers(jdi);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to write dcdc registers: %d\n", err);
> -		goto poweroff;
> -	}
> +	jdi_write_dcdc_registers(&dsi_ctx, jdi);
>   	/*
> -	 * We need to wait 150ms between mipi_dsi_dcs_exit_sleep_mode() and
> -	 * mipi_dsi_dcs_set_display_on().
> +	 * We need to wait 150ms between mipi_dsi_dcs_exit_sleep_mode_multi()
> +	 * and mipi_dsi_dcs_set_display_on_multi().
>   	 */
> -	msleep(150);
> +	mipi_dsi_msleep(&dsi_ctx, 150);
>   
> -	err = mipi_dsi_dcs_set_display_on(jdi->link1);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to set display on: %d\n", err);
> -		goto poweroff;
> -	}
> +	mipi_dsi_dual(mipi_dsi_dcs_set_display_on_multi,
> +			jdi->link1, jdi->link2, &dsi_ctx);
>   
> -	err = mipi_dsi_dcs_set_display_on(jdi->link2);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to set display on: %d\n", err);
> +	if (dsi_ctx.accum_err < 0)
>   		goto poweroff;
> -	}
>   
>   	jdi->link1->mode_flags &= ~MIPI_DSI_MODE_LPM;
>   	jdi->link2->mode_flags &= ~MIPI_DSI_MODE_LPM;