[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 v4:
 - Fix whitespace (I forgot to run checkpatch. Thanks for your patience
   as I familiarize myself with the kernel development process)
 - Initialize mipi_dsi_multi_context struct

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 | 197 ++++++------------
 include/drm/drm_mipi_dsi.h                    |  47 +++--
 4 files changed, 98 insertions(+), 202 deletions(-)

v3: https://lore.kernel.org/all/20250717065757.246122-1-me@brighamcampbell.com/
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.50.1

[PATCH v4 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>
---
 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..03199c966ea5 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.50.1

[PATCH v4 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 | 197 ++++++------------
 1 file changed, 60 insertions(+), 137 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
index 5b5082efb282..9df67facdc47 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;
-}
-
-static int jdi_setup_symmetrical_split(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;
 }
 
-static int jdi_write_dcdc_registers(struct jdi_panel *jdi)
+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)
+{
+	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 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 = { .accum_err = 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->mode);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set up symmetrical split: %d\n",
-			err);
-		goto poweroff;
-	}
+	jdi_setup_symmetrical_split(&dsi_ctx, jdi->link1, jdi->link2,
+				    jdi->mode);
 
-	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.50.1

[PATCH v4 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 03199c966ea5..1ab28fd70c8a 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.50.1

[PATCH v4 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.50.1

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

[Brigham Campbell]

Woops. When I copied over the subject from the previous cover letter, I
accidentally overwrote v4 with v3. I'll issue a v5 if that's a problem.

My bad,
Brigham

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

[Diogo Ivo]

On 7/17/25 5:40 PM, 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>

Reviewed-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
Tested-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>

Thanks for the patch! Just the smallest of nits in the review but it's
fine by me if this goes in as is.

> ---
>   drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c | 197 ++++++------------
>   1 file changed, 60 insertions(+), 137 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> index 5b5082efb282..9df67facdc47 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;
> -}
> -
> -static int jdi_setup_symmetrical_split(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;
>   }
>   
> -static int jdi_write_dcdc_registers(struct jdi_panel *jdi)
> +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)
> +{
> +	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 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 = { .accum_err = 0 };

Technically this should be in reverse christmas tree order but it is
just a tiny nitpick.

>   	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->mode);
> -	if (err < 0) {
> -		dev_err(panel->dev, "failed to set up symmetrical split: %d\n",
> -			err);
> -		goto poweroff;
> -	}
> +	jdi_setup_symmetrical_split(&dsi_ctx, jdi->link1, jdi->link2,
> +				    jdi->mode);
>   
> -	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;

Re: [PATCH v4 1/4] drm: Create mipi_dsi_dual macro

[Doug Anderson]

Hi,

On Thu, Jul 17, 2025 at 9:41 AM Brigham Campbell <me@brighamcampbell.com> wrote:
>
> 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>
> ---
>  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..03199c966ea5 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__);          \

nit: shouldn't func be in parenthesis for safety? It's unlikely to
really matter, but just in case it's somehow a calculated value that
would make it safe from an order-of-operations point of view.


> +               (_ctx)->dsi = (_dsi2);                 \
> +               _func((_ctx), ##__VA_ARGS__);          \
> +       } while (0)

Can you explain why you need the extra level of indirection here (in
other words, why do you need to define _mipi_dsi_dual() and then use
it in mipi_dsi_dual())? I don't see it buying anything, but maybe it's
performing some magic trick I'm not aware of?

Reading this with a fresh set of eyes, I also realize that this macro
is probably vulnerable to issues where arguments have side effects
since we have to repeat them. I don't think there's a way around this
and I think the macro is still worthwhile, but something to go into
with open eyes. Possibly worth noting in the macro description? We
could probably at least eliminate the need to reference "_ctx" more
than once by assigning it to a local variable. I think referencing
"_func" and "__VA_ARGS__" more than once is unavoidable...

Maybe this is what you were trying to solve with the extra level of
indirection, but (at least with my knowledge of the C preprocessor), I
don't think it does.


-Doug

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

[Doug Anderson]

Hi,

On Thu, Jul 17, 2025 at 9:41 AM Brigham Campbell <me@brighamcampbell.com> wrote:
>
>  static int jdi_panel_prepare(struct drm_panel *panel)
>  {
>         struct jdi_panel *jdi = to_panel_jdi(panel);
> +       struct mipi_dsi_multi_context dsi_ctx = { .accum_err = 0 };

nit: can just be this:

struct mipi_dsi_multi_context dsi_ctx = {};

This looks so nice and clean now! :-) I'd bet that the compiled size
of the code is also quite a bit smaller as well...

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v4 1/4] drm: Create mipi_dsi_dual macro

[Brigham Campbell]

On Fri Jul 18, 2025 at 10:10 AM MDT, Doug Anderson wrote:
>> +#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__);          \
>
> nit: shouldn't func be in parenthesis for safety? It's unlikely to
> really matter, but just in case it's somehow a calculated value that
> would make it safe from an order-of-operations point of view.

My assumption is that wrapping _func in parenthesis would cause a
compilation error in the case of _func being a macro (more on that
later...). I'll test that later today.

>> +               (_ctx)->dsi = (_dsi2);                 \
>> +               _func((_ctx), ##__VA_ARGS__);          \
>> +       } while (0)
>
> Can you explain why you need the extra level of indirection here (in
> other words, why do you need to define _mipi_dsi_dual() and then use
> it in mipi_dsi_dual())? I don't see it buying anything, but maybe it's
> performing some magic trick I'm not aware of?

I mentioned this in v3 after the changelog and prompty forgot to include
that information in v4: The extra indirection between mipi_dsi_dual()
and _mipi_dsi_dual() is to allow for the expansion of _func in the case
that _func is also a macro (as is the case with
mipi_dsi_generic_write_seq_multi, i believe). Compilation fails after
removing the indirection.

There may very well be a better solution to this problem. I'd appreciate
your thoughts.

> Reading this with a fresh set of eyes, I also realize that this macro
> is probably vulnerable to issues where arguments have side effects
> since we have to repeat them. I don't think there's a way around this
> and I think the macro is still worthwhile, but something to go into
> with open eyes. Possibly worth noting in the macro description? We
> could probably at least eliminate the need to reference "_ctx" more
> than once by assigning it to a local variable. I think referencing
> "_func" and "__VA_ARGS__" more than once is unavoidable...

I'm using _func, _ctx, and __VA_ARGS__ more than once in this macro and
I don't expect the indirection to fix the potential issue of unintended
side effects. I believe we can use GNU extensions to eliminate side
effects to _ctx, but especially since _func can be a macro, I don't
think there's much to be done about it. Not sure about __VA_ARGS__.

I fear my inexperience is made sorely manifest here.

Happy Friday,
Brigham

Re: [PATCH v4 1/4] drm: Create mipi_dsi_dual macro

[Doug Anderson]

Hi,

On Fri, Jul 18, 2025 at 10:17 AM Brigham Campbell
<me@brighamcampbell.com> wrote:
>
> On Fri Jul 18, 2025 at 10:10 AM MDT, Doug Anderson wrote:
> >> +#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__);          \
> >
> > nit: shouldn't func be in parenthesis for safety? It's unlikely to
> > really matter, but just in case it's somehow a calculated value that
> > would make it safe from an order-of-operations point of view.
>
> My assumption is that wrapping _func in parenthesis would cause a
> compilation error in the case of _func being a macro (more on that
> later...). I'll test that later today.

Huh, OK. If that's the case then no need to do it.


> >> +               (_ctx)->dsi = (_dsi2);                 \
> >> +               _func((_ctx), ##__VA_ARGS__);          \
> >> +       } while (0)
> >
> > Can you explain why you need the extra level of indirection here (in
> > other words, why do you need to define _mipi_dsi_dual() and then use
> > it in mipi_dsi_dual())? I don't see it buying anything, but maybe it's
> > performing some magic trick I'm not aware of?
>
> I mentioned this in v3 after the changelog and prompty forgot to include
> that information in v4: The extra indirection between mipi_dsi_dual()
> and _mipi_dsi_dual() is to allow for the expansion of _func in the case
> that _func is also a macro (as is the case with
> mipi_dsi_generic_write_seq_multi, i believe). Compilation fails after
> removing the indirection.
>
> There may very well be a better solution to this problem. I'd appreciate
> your thoughts.

Wow, crazy. I think the C preprocessor is one step away from magic.
While I know there are rules for it, I often find the way that it
behaves to be counter-intuitive. I can't say I've followed exactly how
your solution is working, but if it works and is needed then it's OK
w/ me. It might be worth promoting the note to be in the commit
message itself (or even a code comment?) so future people trying to
understand the code will have some chance of stumbling across it...

You might hate this, but one possible other solution would be to make
a custom `mipi_dsi_dual_dcs_write_seq_multi` (lifting it out of the
novatek driver) and then say that the "_func" parameter can't be a
macro. If you did it correctly, it would be a pretty big space savings
too. Unlike how we did it in the novatek driver, I think a proper way
to do it that would save the most space would be:

#define mipi_dsi_dual_dcs_write_seq_multi(ctx, dsi0, dsi1, cmd, seq...) \
  do { \
  static const u8 d[] = { cmd, seq }; \
  mipi_dsi_dual_dcs_write_buffer_multi(ctx, dsi0, dsi1, \
                             d, ARRAY_SIZE(d)); \
  } while (0)

...and then mipi_dsi_dual_dcs_write_buffer_multi() would be
implemented in drm_mipi_dsi.c.

With the above implementation, you only have one "static const" buffer
(maybe the compiler is smart enough to combine w/ the novatek code,
but  it might not be) and also only have a single function call taking
up space in the panel driver. You'd only have the "custom" dual
implementation for the "write_seq" stuff since that appears to be the
most common. All the other DSI calls could use the normal
mipi_dsi_dual() macro...

I was thinking of suggesting that as an optional followup to your
series anyway (for the space savings), but it could also solve some of
the preprocessor woes. :-P

I'm certainly not dead-set on this, so if you want to just keep
something like your current solution that's OK w/ me too.


> > Reading this with a fresh set of eyes, I also realize that this macro
> > is probably vulnerable to issues where arguments have side effects
> > since we have to repeat them. I don't think there's a way around this
> > and I think the macro is still worthwhile, but something to go into
> > with open eyes. Possibly worth noting in the macro description? We
> > could probably at least eliminate the need to reference "_ctx" more
> > than once by assigning it to a local variable. I think referencing
> > "_func" and "__VA_ARGS__" more than once is unavoidable...
>
> I'm using _func, _ctx, and __VA_ARGS__ more than once in this macro and
> I don't expect the indirection to fix the potential issue of unintended
> side effects. I believe we can use GNU extensions to eliminate side
> effects to _ctx,

I wasn't thinking of any GNU extensions. Just using a scope-defined
local variable...

#define _mipi_dsi_dual(_func, _dsi1, _dsi2, _ctx, ...) \
  do { \
    struct mipi_dsi_multi_context *__ctx = (_ctx); \
    __ctx->dsi = (_dsi1); \
    ...

> but especially since _func can be a macro, I don't
> think there's much to be done about it. Not sure about __VA_ARGS__.
>
> I fear my inexperience is made sorely manifest here.

I think it's a rare person who fully understands the dirty corners of
the C preprocessor and I wouldn't count myself as one of them. I can
sometimes make it do what I want, but I think we're up against my
limits too...

-Doug

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

[Diogo Ivo]

On 7/18/25 5:11 PM, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jul 17, 2025 at 9:41 AM Brigham Campbell <me@brighamcampbell.com> wrote:
>>
>>   static int jdi_panel_prepare(struct drm_panel *panel)
>>   {
>>          struct jdi_panel *jdi = to_panel_jdi(panel);
>> +       struct mipi_dsi_multi_context dsi_ctx = { .accum_err = 0 };
> 
> nit: can just be this:
> 
> struct mipi_dsi_multi_context dsi_ctx = {};

I am not an expert here but I was under the impression that this is only
valid with C23 while the kernel is written in C11. Is there something I
am missing?

Diogo

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

[Brigham Campbell]

On Sat Jul 19, 2025 at 11:10 AM MDT, Diogo Ivo wrote:
>> nit: can just be this:
>> 
>> struct mipi_dsi_multi_context dsi_ctx = {};
>
> I am not an expert here but I was under the impression that this is only
> valid with C23 while the kernel is written in C11. Is there something I
> am missing?
>
> Diogo

You're right, C23 was the first standard to bless the usage of the empty
initializer, ` = {};`, but if I'm right, it's been a GNU extension long
before C11. At risk of being pedantic, I'll draw attention to line 580
of the kernel's root Makefile:

KBUILD_CFLAGS += -std=gnu11

The kernel is technically written in the GNU variant of C11, extensions
and all. In fact, the first patch of this series uses optional variadic
macro arguments, which aren't a part of any official C standard as far
as I'm aware.

In any case, a simple grep for some forms of the empty initializer shows
usages all over the drm subsystem.

That said, I don't know if GNU extensions are formally documented or
where one would look for that information. Importantly, I am by far the
junior as far as kernel coding is concerned. I yield to your experience
and I'm happy to change this initialization in v6 if that's best.

Cheers,
Brigham

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

[Diogo Ivo]

On 7/20/25 8:50 AM, Brigham Campbell wrote:
> On Sat Jul 19, 2025 at 11:10 AM MDT, Diogo Ivo wrote:
>>> nit: can just be this:
>>>
>>> struct mipi_dsi_multi_context dsi_ctx = {};
>>
>> I am not an expert here but I was under the impression that this is only
>> valid with C23 while the kernel is written in C11. Is there something I
>> am missing?
>>
>> Diogo
> 
> You're right, C23 was the first standard to bless the usage of the empty
> initializer, ` = {};`, but if I'm right, it's been a GNU extension long
> before C11. At risk of being pedantic, I'll draw attention to line 580
> of the kernel's root Makefile:
> 
> KBUILD_CFLAGS += -std=gnu11
> 
> The kernel is technically written in the GNU variant of C11, extensions
> and all. In fact, the first patch of this series uses optional variadic
> macro arguments, which aren't a part of any official C standard as far
> as I'm aware.
> 
> In any case, a simple grep for some forms of the empty initializer shows
> usages all over the drm subsystem.
> 
> That said, I don't know if GNU extensions are formally documented or
> where one would look for that information. Importantly, I am by far the
> junior as far as kernel coding is concerned. I yield to your experience
> and I'm happy to change this initialization in v6 if that's best.

I found the documentation here [1], and it does state regarding designated
initializers that "Omitted fields are implicitly initialized the same as for
objects that have static storage duration." so I take it that no v6 is 
needed :)

Diogo

[1]: 
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html#Designated-Inits

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

[Doug Anderson]

Hi,

On Sun, Jul 20, 2025 at 4:19 AM Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt> wrote:
>
> On 7/20/25 8:50 AM, Brigham Campbell wrote:
> > On Sat Jul 19, 2025 at 11:10 AM MDT, Diogo Ivo wrote:
> >>> nit: can just be this:
> >>>
> >>> struct mipi_dsi_multi_context dsi_ctx = {};
> >>
> >> I am not an expert here but I was under the impression that this is only
> >> valid with C23 while the kernel is written in C11. Is there something I
> >> am missing?
> >>
> >> Diogo
> >
> > You're right, C23 was the first standard to bless the usage of the empty
> > initializer, ` = {};`, but if I'm right, it's been a GNU extension long
> > before C11. At risk of being pedantic, I'll draw attention to line 580
> > of the kernel's root Makefile:
> >
> > KBUILD_CFLAGS += -std=gnu11
> >
> > The kernel is technically written in the GNU variant of C11, extensions
> > and all. In fact, the first patch of this series uses optional variadic
> > macro arguments, which aren't a part of any official C standard as far
> > as I'm aware.
> >
> > In any case, a simple grep for some forms of the empty initializer shows
> > usages all over the drm subsystem.
> >
> > That said, I don't know if GNU extensions are formally documented or
> > where one would look for that information. Importantly, I am by far the
> > junior as far as kernel coding is concerned. I yield to your experience
> > and I'm happy to change this initialization in v6 if that's best.
>
> I found the documentation here [1], and it does state regarding designated
> initializers that "Omitted fields are implicitly initialized the same as for
> objects that have static storage duration." so I take it that no v6 is
> needed :)
>
> Diogo
>
> [1]:
> https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html#Designated-Inits

Right. I think the key here is (as Brigham said) `git grep '= {}'`
shows countless hits in the kernel. :-) ...so we're not introducing
any new compiler requirement here with your patch. ;-)

-Doug