[PATCH v2 0/3] drm: docs: Remove deprecated MIPI DSI macro

[PATCH v2 0/3] drm: docs: Remove deprecated MIPI DSI macro

[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 first 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.
Any behavioral modification to the jdi lpm102a188a panel driver by this
series is unintentional.

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 (3):
  drm/panel: jdi-lpm102a188a: Update deprecated MIPI function calls
  Remove unused MIPI write seq and chatty functions
  drm: docs: Update task from drm TODO list

 Documentation/gpu/todo.rst                    |  26 +--
 drivers/gpu/drm/drm_mipi_dsi.c                |  34 +---
 drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c | 150 +++++++-----------
 include/drm/drm_mipi_dsi.h                    |  23 ---
 4 files changed, 71 insertions(+), 162 deletions(-)


Link: https://lore.kernel.org/lkml/20250707075659.75810-1-me@brighamcampbell.com/
base-commit: e33f256dbc293a1a3a31f18d56f659e7a27a491a
-- 
2.49.0

[PATCH v2 1/3] drm/panel: jdi-lpm102a188a: Update deprecated MIPI function calls

[Brigham Campbell]

Update jdi-lpm102a188a panel 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.

This patch's usage of the mipi_dsi_multi_context struct is not
idiomatic. Rightfully, the struct wasn't designed to cater to the needs
of panels with multiple MIPI DSI interfaces. This panel is an oddity
which requires swapping the dsi pointer between MIPI function calls in
order to preserve the exact behavior implemented using the non-multi
variant of the macro.

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

diff --git a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
index 5b5082efb282..5001bea1798f 100644
--- a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
+++ b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
@@ -81,25 +81,20 @@ 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;
+	struct mipi_dsi_multi_context dsi_ctx;
 
-	ret = mipi_dsi_dcs_set_display_off(jdi->link1);
-	if (ret < 0)
-		dev_err(panel->dev, "failed to set display off: %d\n", ret);
-
-	ret = mipi_dsi_dcs_set_display_off(jdi->link2);
-	if (ret < 0)
-		dev_err(panel->dev, "failed to set display off: %d\n", ret);
+	dsi_ctx.dsi = jdi->link1;
+	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
+	dsi_ctx.dsi = jdi->link2;
+	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
 
 	/* 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);
+	dsi_ctx.dsi = jdi->link1;
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
+	dsi_ctx.dsi = jdi->link2;
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
 
 	/* Specified by JDI @ 150ms, subject to change */
 	msleep(150);
@@ -123,72 +118,64 @@ static int jdi_panel_unprepare(struct drm_panel *panel)
 	/* Specified by JDI @ 20ms, subject to change */
 	msleep(20);
 
-	return ret;
+	return dsi_ctx.accum_err;
 }
 
 static int jdi_setup_symmetrical_split(struct mipi_dsi_device *left,
 				       struct mipi_dsi_device *right,
 				       const struct drm_display_mode *mode)
 {
-	int err;
+	struct mipi_dsi_multi_context dsi_ctx;
 
-	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;
-	}
+	dsi_ctx.dsi = left;
+	mipi_dsi_dcs_set_column_address_multi(&dsi_ctx, 0, mode->hdisplay / 2 - 1);
+	dsi_ctx.dsi = right;
+	mipi_dsi_dcs_set_column_address_multi(&dsi_ctx, 0, mode->hdisplay / 2 - 1);
 
-	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;
-	}
+	dsi_ctx.dsi = left;
+	mipi_dsi_dcs_set_page_address_multi(&dsi_ctx, 0, mode->vdisplay - 1);
+	dsi_ctx.dsi = right;
+	mipi_dsi_dcs_set_page_address_multi(&dsi_ctx, 0, mode->vdisplay - 1);
 
-	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;
+	return dsi_ctx.accum_err;
 }
 
 static int jdi_write_dcdc_registers(struct jdi_panel *jdi)
 {
+	struct mipi_dsi_multi_context dsi_ctx;
+
 	/* Clear the manufacturer command access protection */
-	mipi_dsi_generic_write_seq(jdi->link1, MCS_CMD_ACS_PROT,
+	dsi_ctx.dsi = jdi->link1;
+	mipi_dsi_generic_write_seq_multi(&dsi_ctx, MCS_CMD_ACS_PROT,
 				   MCS_CMD_ACS_PROT_OFF);
-	mipi_dsi_generic_write_seq(jdi->link2, MCS_CMD_ACS_PROT,
+	dsi_ctx.dsi = jdi->link2;
+	mipi_dsi_generic_write_seq_multi(&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,
+	dsi_ctx.dsi = jdi->link1;
+	mipi_dsi_generic_write_seq_multi(&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);
+	dsi_ctx.dsi = jdi->link2;
+	mipi_dsi_generic_write_seq_multi(&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);
 
-	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;
+	return dsi_ctx.accum_err;
 }
 
 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
@@ -239,57 +226,32 @@ static int jdi_panel_prepare(struct drm_panel *panel)
 		goto poweroff;
 	}
 
-	err = mipi_dsi_dcs_set_tear_scanline(jdi->link1,
+	dsi_ctx.dsi = jdi->link1;
+	mipi_dsi_dcs_set_tear_scanline_multi(&dsi_ctx,
 					     jdi->mode->vdisplay - 16);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set tear scanline: %d\n", err);
-		goto poweroff;
-	}
-
-	err = mipi_dsi_dcs_set_tear_scanline(jdi->link2,
+	dsi_ctx.dsi = jdi->link2;
+	mipi_dsi_dcs_set_tear_scanline_multi(&dsi_ctx,
 					     jdi->mode->vdisplay - 16);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set tear scanline: %d\n", err);
-		goto poweroff;
-	}
 
-	err = mipi_dsi_dcs_set_tear_on(jdi->link1,
+	dsi_ctx.dsi = jdi->link1;
+	mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx,
 				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set tear on: %d\n", err);
-		goto poweroff;
-	}
-
-	err = mipi_dsi_dcs_set_tear_on(jdi->link2,
+	dsi_ctx.dsi = jdi->link2;
+	mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx,
 				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to set tear on: %d\n", err);
-		goto poweroff;
-	}
 
-	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;
-	}
+	dsi_ctx.dsi = jdi->link1;
+	mipi_dsi_dcs_set_pixel_format_multi(&dsi_ctx, MIPI_DCS_PIXEL_FMT_24BIT);
+	dsi_ctx.dsi = jdi->link2;
+	mipi_dsi_dcs_set_pixel_format_multi(&dsi_ctx, MIPI_DCS_PIXEL_FMT_24BIT);
 
-	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;
-	}
+	dsi_ctx.dsi = jdi->link1;
+	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
+	dsi_ctx.dsi = jdi->link2;
+	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
 
-	err = mipi_dsi_dcs_exit_sleep_mode(jdi->link1);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
+	if (dsi_ctx.accum_err < 0)
 		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) {
@@ -302,17 +264,13 @@ static int jdi_panel_prepare(struct drm_panel *panel)
 	 */
 	msleep(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;
-	}
+	dsi_ctx.dsi = jdi->link1;
+	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
+	dsi_ctx.dsi = jdi->link2;
+	mipi_dsi_dcs_set_display_on_multi(&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

Re: [PATCH v2 1/3] drm/panel: jdi-lpm102a188a: Update deprecated MIPI function calls

[Doug Anderson]

Hi,

On Tue, Jul 8, 2025 at 12:39 AM Brigham Campbell <me@brighamcampbell.com> wrote:
>
> Update jdi-lpm102a188a panel 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.
>
> This patch's usage of the mipi_dsi_multi_context struct is not
> idiomatic. Rightfully, the struct wasn't designed to cater to the needs
> of panels with multiple MIPI DSI interfaces. This panel is an oddity
> which requires swapping the dsi pointer between MIPI function calls in
> order to preserve the exact behavior implemented using the non-multi
> variant of the macro.

Right. We dealt with this before with "novatek-nt36523"...


> Signed-off-by: Brigham Campbell <me@brighamcampbell.com>
> ---
>  drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c | 160 +++++++-----------
>  1 file changed, 59 insertions(+), 101 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> index 5b5082efb282..5001bea1798f 100644
> --- a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> +++ b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> @@ -81,25 +81,20 @@ 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;
> +       struct mipi_dsi_multi_context dsi_ctx;
>
> -       ret = mipi_dsi_dcs_set_display_off(jdi->link1);
> -       if (ret < 0)
> -               dev_err(panel->dev, "failed to set display off: %d\n", ret);
> -
> -       ret = mipi_dsi_dcs_set_display_off(jdi->link2);
> -       if (ret < 0)
> -               dev_err(panel->dev, "failed to set display off: %d\n", ret);
> +       dsi_ctx.dsi = jdi->link1;
> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +       dsi_ctx.dsi = jdi->link2;
> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
>
>         /* Specified by JDI @ 50ms, subject to change */
>         msleep(50);

Needs to be mipi_dsi_msleep() to avoid the sleep in case the above
commands caused an error.


> -       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);
> +       dsi_ctx.dsi = jdi->link1;
> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> +       dsi_ctx.dsi = jdi->link2;
> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);

I think you need this here:

if (dsi_ctx.accum_err)
  return dsi_ctx.accum_err;

...otherwise the code will do the sleeps, GPIO calls, and regulator
calls even in the case of an error. You don't want that. Then at the
end of the function you'd just have "return 0;"


>  static int jdi_setup_symmetrical_split(struct mipi_dsi_device *left,
>                                        struct mipi_dsi_device *right,
>                                        const struct drm_display_mode *mode)
>  {
> -       int err;
> +       struct mipi_dsi_multi_context dsi_ctx;

I think you should actually pass in the "dsi_ctx" to this function
since the caller already has one. Then you can change it to a "void"
function...


>  static int jdi_write_dcdc_registers(struct jdi_panel *jdi)
>  {

I think you want to pass the context into this function too and make
it "void". Then the caller doesn't need to check for the error before
calling it...

Then the "msleep(150)" after calling this function can change to a
`mipi_dsi_msleep()` and you don't need to check the error until right
before the LPM flag is cleared...


> +       struct mipi_dsi_multi_context dsi_ctx;
> +
>         /* Clear the manufacturer command access protection */
> -       mipi_dsi_generic_write_seq(jdi->link1, MCS_CMD_ACS_PROT,
> +       dsi_ctx.dsi = jdi->link1;
> +       mipi_dsi_generic_write_seq_multi(&dsi_ctx, MCS_CMD_ACS_PROT,
>                                    MCS_CMD_ACS_PROT_OFF);
> -       mipi_dsi_generic_write_seq(jdi->link2, MCS_CMD_ACS_PROT,
> +       dsi_ctx.dsi = jdi->link2;
> +       mipi_dsi_generic_write_seq_multi(&dsi_ctx, MCS_CMD_ACS_PROT,
>                                    MCS_CMD_ACS_PROT_OFF);

All the duplication is annoying. In "novatek-nt36523" I guess we only
needed to send _some_ of the commands to both panels and so we ended
up with a macro in just that C file just for
mipi_dsi_dual_dcs_write_seq_multi(). ...but here you seem to be
needing it for lots of functions.

Maybe the solution is to add something like this to "drm_mipi_dsi.h":

#define mipi_dsi_dual(_func, _dsi1, _dsi2, _ctx, _args...) \
  do { \
    (_ctx)->dsi = (_dsi1); \
    (_func)((_ctx), _args); \
    (_ctx)->dsi = (_dsi2); \
    (_func)((_ctx), _args); \
  } while (0)

Then you could have statements like:

mipi_dsi_dual(mipi_dsi_generic_write_seq_multi, jdi->link1,
jdi->link2, &dsi_ctx, ...);

I _think_ that will work? I at least prototyped it up with some simple
test code and it seemed to work... What do others think of that?

-Doug

Re: [PATCH v2 1/3] drm/panel: jdi-lpm102a188a: Update deprecated MIPI function calls

[Brigham Campbell]

On Mon Jul 14, 2025 at 3:46 PM MDT, Doug Anderson wrote:
> Hi,
>
> On Tue, Jul 8, 2025 at 12:39 AM Brigham Campbell <me@brighamcampbell.com> wrote:
>>
>> Update jdi-lpm102a188a panel 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.
>>
>> This patch's usage of the mipi_dsi_multi_context struct is not
>> idiomatic. Rightfully, the struct wasn't designed to cater to the needs
>> of panels with multiple MIPI DSI interfaces. This panel is an oddity
>> which requires swapping the dsi pointer between MIPI function calls in
>> order to preserve the exact behavior implemented using the non-multi
>> variant of the macro.
>
> Right. We dealt with this before with "novatek-nt36523"...
>
>
>> Signed-off-by: Brigham Campbell <me@brighamcampbell.com>
>> ---
>>  drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c | 160 +++++++-----------
>>  1 file changed, 59 insertions(+), 101 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
>> index 5b5082efb282..5001bea1798f 100644
>> --- a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
>> +++ b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
>> @@ -81,25 +81,20 @@ 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;
>> +       struct mipi_dsi_multi_context dsi_ctx;
>>
>> -       ret = mipi_dsi_dcs_set_display_off(jdi->link1);
>> -       if (ret < 0)
>> -               dev_err(panel->dev, "failed to set display off: %d\n", ret);
>> -
>> -       ret = mipi_dsi_dcs_set_display_off(jdi->link2);
>> -       if (ret < 0)
>> -               dev_err(panel->dev, "failed to set display off: %d\n", ret);
>> +       dsi_ctx.dsi = jdi->link1;
>> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
>> +       dsi_ctx.dsi = jdi->link2;
>> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
>>
>>         /* Specified by JDI @ 50ms, subject to change */
>>         msleep(50);
>
> Needs to be mipi_dsi_msleep() to avoid the sleep in case the above
> commands caused an error.
>
>
>> -       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);
>> +       dsi_ctx.dsi = jdi->link1;
>> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
>> +       dsi_ctx.dsi = jdi->link2;
>> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
>
> I think you need this here:
>
> if (dsi_ctx.accum_err)
>   return dsi_ctx.accum_err;
>
> ...otherwise the code will do the sleeps, GPIO calls, and regulator
> calls even in the case of an error. You don't want that. Then at the
> end of the function you'd just have "return 0;"
>
>

Regarding these two suggestions, I prepared this patch with the intent
to change the drivers actual behavior as little as possible. It looks
like the original driver will happily msleep and try to continue
initialization even after an error has occurred. Of course, using the
_multi variants of mipi dbi functions kind of implies that we want to
stop communicating with a display after an error has occurred. And
because all _multi functions are effectively no-ops after an error has
occurred, it doesn't make sense to perform the other half of the
initialization sequence while anything involving mipi is dutifully
skipped.

I'll make these changes in the next patch revision.

>>  static int jdi_setup_symmetrical_split(struct mipi_dsi_device *left,
>>                                        struct mipi_dsi_device *right,
>>                                        const struct drm_display_mode *mode)
>>  {
>> -       int err;
>> +       struct mipi_dsi_multi_context dsi_ctx;
>
> I think you should actually pass in the "dsi_ctx" to this function
> since the caller already has one. Then you can change it to a "void"
> function...
>
>
>>  static int jdi_write_dcdc_registers(struct jdi_panel *jdi)
>>  {
>
> I think you want to pass the context into this function too and make
> it "void". Then the caller doesn't need to check for the error before
> calling it...
>
> Then the "msleep(150)" after calling this function can change to a
> `mipi_dsi_msleep()` and you don't need to check the error until right
> before the LPM flag is cleared...
>
>

About the suggestion, "you don't need to check the error until right
before the LPM flag is cleared...", if I change
jdi_setup_symmetrical_split to accept a mipi_dsi_multi_context pointer,
the driver will output "failed to set up symmetrical split" even if the
error was encountered well before setting up the symmetrical split
(meaning the driver doesn't even try to set up symmetrical split at all).

The appropriate solution will be to either maintain the error checks in
the driver, or remove the print statements. For the next revision, I'll
simply go ahead and remove the error print statements because:
  - the mipi _multi functions should handle error printing well enough
    to facilitate tracking down the particular mipi sequence which
    caused an error during probe/unprepare.
  - less logic in this driver makes it easier to maintain

>> +       struct mipi_dsi_multi_context dsi_ctx;
>> +
>>         /* Clear the manufacturer command access protection */
>> -       mipi_dsi_generic_write_seq(jdi->link1, MCS_CMD_ACS_PROT,
>> +       dsi_ctx.dsi = jdi->link1;
>> +       mipi_dsi_generic_write_seq_multi(&dsi_ctx, MCS_CMD_ACS_PROT,
>>                                    MCS_CMD_ACS_PROT_OFF);
>> -       mipi_dsi_generic_write_seq(jdi->link2, MCS_CMD_ACS_PROT,
>> +       dsi_ctx.dsi = jdi->link2;
>> +       mipi_dsi_generic_write_seq_multi(&dsi_ctx, MCS_CMD_ACS_PROT,
>>                                    MCS_CMD_ACS_PROT_OFF);
>
> All the duplication is annoying. In "novatek-nt36523" I guess we only
> needed to send _some_ of the commands to both panels and so we ended
> up with a macro in just that C file just for
> mipi_dsi_dual_dcs_write_seq_multi(). ...but here you seem to be
> needing it for lots of functions.
>
> Maybe the solution is to add something like this to "drm_mipi_dsi.h":
>
> #define mipi_dsi_dual(_func, _dsi1, _dsi2, _ctx, _args...) \
>   do { \
>     (_ctx)->dsi = (_dsi1); \
>     (_func)((_ctx), _args); \
>     (_ctx)->dsi = (_dsi2); \
>     (_func)((_ctx), _args); \
>   } while (0)
>
> Then you could have statements like:
>
> mipi_dsi_dual(mipi_dsi_generic_write_seq_multi, jdi->link1,
> jdi->link2, &dsi_ctx, ...);
>
> I _think_ that will work? I at least prototyped it up with some simple
> test code and it seemed to work... What do others think of that?

In my opinion, this change is absolutely worth making, but the creation
of a new macro like mipi_dsi_dual in drm_mipi_dsi.h is beyond the scope
of this patch series because it has implications for panels like
novatek-nt36523 and others. It looks like you've already effectively
completed the work of implementing the macro, but I'm happy to address
the adoption of the macro in lpm102a188a and other drivers as a part of
a later patch series. Besides, there is no more duplication in this
driver after applying my patch than there was before.

Of course, maybe that's just me being pedantic. I'm happy to include
mipi_dsi_dual in this series if you insist.


Thanks for the thorough review,
Brigham

Re: [PATCH v2 1/3] drm/panel: jdi-lpm102a188a: Update deprecated MIPI function calls

[Doug Anderson]

Hi,

On Tue, Jul 15, 2025 at 10:32 PM Brigham Campbell
<me@brighamcampbell.com> wrote:
>
> On Mon Jul 14, 2025 at 3:46 PM MDT, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Jul 8, 2025 at 12:39 AM Brigham Campbell <me@brighamcampbell.com> wrote:
> >>
> >> Update jdi-lpm102a188a panel 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.
> >>
> >> This patch's usage of the mipi_dsi_multi_context struct is not
> >> idiomatic. Rightfully, the struct wasn't designed to cater to the needs
> >> of panels with multiple MIPI DSI interfaces. This panel is an oddity
> >> which requires swapping the dsi pointer between MIPI function calls in
> >> order to preserve the exact behavior implemented using the non-multi
> >> variant of the macro.
> >
> > Right. We dealt with this before with "novatek-nt36523"...
> >
> >
> >> Signed-off-by: Brigham Campbell <me@brighamcampbell.com>
> >> ---
> >>  drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c | 160 +++++++-----------
> >>  1 file changed, 59 insertions(+), 101 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> >> index 5b5082efb282..5001bea1798f 100644
> >> --- a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> >> +++ b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> >> @@ -81,25 +81,20 @@ 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;
> >> +       struct mipi_dsi_multi_context dsi_ctx;
> >>
> >> -       ret = mipi_dsi_dcs_set_display_off(jdi->link1);
> >> -       if (ret < 0)
> >> -               dev_err(panel->dev, "failed to set display off: %d\n", ret);
> >> -
> >> -       ret = mipi_dsi_dcs_set_display_off(jdi->link2);
> >> -       if (ret < 0)
> >> -               dev_err(panel->dev, "failed to set display off: %d\n", ret);
> >> +       dsi_ctx.dsi = jdi->link1;
> >> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> >> +       dsi_ctx.dsi = jdi->link2;
> >> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> >>
> >>         /* Specified by JDI @ 50ms, subject to change */
> >>         msleep(50);
> >
> > Needs to be mipi_dsi_msleep() to avoid the sleep in case the above
> > commands caused an error.
> >
> >
> >> -       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);
> >> +       dsi_ctx.dsi = jdi->link1;
> >> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> >> +       dsi_ctx.dsi = jdi->link2;
> >> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> >
> > I think you need this here:
> >
> > if (dsi_ctx.accum_err)
> >   return dsi_ctx.accum_err;
> >
> > ...otherwise the code will do the sleeps, GPIO calls, and regulator
> > calls even in the case of an error. You don't want that. Then at the
> > end of the function you'd just have "return 0;"
> >
> >
>
> Regarding these two suggestions, I prepared this patch with the intent
> to change the drivers actual behavior as little as possible. It looks
> like the original driver will happily msleep and try to continue
> initialization even after an error has occurred. Of course, using the
> _multi variants of mipi dbi functions kind of implies that we want to
> stop communicating with a display after an error has occurred. And
> because all _multi functions are effectively no-ops after an error has
> occurred, it doesn't make sense to perform the other half of the
> initialization sequence while anything involving mipi is dutifully
> skipped.
>
> I'll make these changes in the next patch revision.

Hmmmm. You're right that the old driver was indeed plowing forward
when it got errors. It checks them and does a printout, but then it
throws away the error code and continues executing the rest of the
functions.

Oh! ...but looking at this with fresh eyes, perhaps what the old code
was doing was more correct specifically because this is _unprepare_
and we want to power off even if there were errors. To be explicit:

1. Just because MIPI commands are failing on one link doesn't mean
that they are failing on both links.

2. Even though trying to send "enter sleep mode" after a failed
"display off" will probably fail, it doesn't seem like it would hurt
to try it.

3. You definitely still want all the GPIO / regulator calls even if
the MIP commands failed.

4. Given all the above, I guess one could argue that the sleeps should
all be executed in this case even if the MIPI commands fail.

Maybe something like this would make sense?

/*
* 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 };

mipi_dsi_dcs_set_display_off_multi(&dsi_ctx1);
mipi_dsi_dcs_set_display_off_multi(&dsi_ctx2);

msleep(50);

/* Doesn't hurt to try sleep mode even if display off fails */
dsi_ctx1.accum_err = dsi_ctx2.accum_err = 0;
mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx1);
mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx2);

msleep(150);

...
...

/* We powered off; return no error even if MIPI cmds failed */
return 0;

What do you think? This is still a small functionality change since
the old code would have returned an error if the second
mipi_dsi_dcs_enter_sleep_mode() returned an error code, but that feels
like it was a bug. That should probably be mentioned in the commit
message.


> >>  static int jdi_setup_symmetrical_split(struct mipi_dsi_device *left,
> >>                                        struct mipi_dsi_device *right,
> >>                                        const struct drm_display_mode *mode)
> >>  {
> >> -       int err;
> >> +       struct mipi_dsi_multi_context dsi_ctx;
> >
> > I think you should actually pass in the "dsi_ctx" to this function
> > since the caller already has one. Then you can change it to a "void"
> > function...
> >
> >
> >>  static int jdi_write_dcdc_registers(struct jdi_panel *jdi)
> >>  {
> >
> > I think you want to pass the context into this function too and make
> > it "void". Then the caller doesn't need to check for the error before
> > calling it...
> >
> > Then the "msleep(150)" after calling this function can change to a
> > `mipi_dsi_msleep()` and you don't need to check the error until right
> > before the LPM flag is cleared...
> >
> >
>
> About the suggestion, "you don't need to check the error until right
> before the LPM flag is cleared...", if I change
> jdi_setup_symmetrical_split to accept a mipi_dsi_multi_context pointer,
> the driver will output "failed to set up symmetrical split" even if the
> error was encountered well before setting up the symmetrical split
> (meaning the driver doesn't even try to set up symmetrical split at all).
>
> The appropriate solution will be to either maintain the error checks in
> the driver, or remove the print statements. For the next revision, I'll
> simply go ahead and remove the error print statements because:
>   - the mipi _multi functions should handle error printing well enough
>     to facilitate tracking down the particular mipi sequence which
>     caused an error during probe/unprepare.
>   - less logic in this driver makes it easier to maintain

Right. You'd remove the "failed to set up symmetrical split" error
since all sources of the error would already be reported.


> >> +       struct mipi_dsi_multi_context dsi_ctx;
> >> +
> >>         /* Clear the manufacturer command access protection */
> >> -       mipi_dsi_generic_write_seq(jdi->link1, MCS_CMD_ACS_PROT,
> >> +       dsi_ctx.dsi = jdi->link1;
> >> +       mipi_dsi_generic_write_seq_multi(&dsi_ctx, MCS_CMD_ACS_PROT,
> >>                                    MCS_CMD_ACS_PROT_OFF);
> >> -       mipi_dsi_generic_write_seq(jdi->link2, MCS_CMD_ACS_PROT,
> >> +       dsi_ctx.dsi = jdi->link2;
> >> +       mipi_dsi_generic_write_seq_multi(&dsi_ctx, MCS_CMD_ACS_PROT,
> >>                                    MCS_CMD_ACS_PROT_OFF);
> >
> > All the duplication is annoying. In "novatek-nt36523" I guess we only
> > needed to send _some_ of the commands to both panels and so we ended
> > up with a macro in just that C file just for
> > mipi_dsi_dual_dcs_write_seq_multi(). ...but here you seem to be
> > needing it for lots of functions.
> >
> > Maybe the solution is to add something like this to "drm_mipi_dsi.h":
> >
> > #define mipi_dsi_dual(_func, _dsi1, _dsi2, _ctx, _args...) \
> >   do { \
> >     (_ctx)->dsi = (_dsi1); \
> >     (_func)((_ctx), _args); \
> >     (_ctx)->dsi = (_dsi2); \
> >     (_func)((_ctx), _args); \
> >   } while (0)
> >
> > Then you could have statements like:
> >
> > mipi_dsi_dual(mipi_dsi_generic_write_seq_multi, jdi->link1,
> > jdi->link2, &dsi_ctx, ...);
> >
> > I _think_ that will work? I at least prototyped it up with some simple
> > test code and it seemed to work... What do others think of that?
>
> In my opinion, this change is absolutely worth making, but the creation
> of a new macro like mipi_dsi_dual in drm_mipi_dsi.h is beyond the scope
> of this patch series because it has implications for panels like
> novatek-nt36523 and others. It looks like you've already effectively
> completed the work of implementing the macro, but I'm happy to address
> the adoption of the macro in lpm102a188a and other drivers as a part of
> a later patch series. Besides, there is no more duplication in this
> driver after applying my patch than there was before.
>
> Of course, maybe that's just me being pedantic. I'm happy to include
> mipi_dsi_dual in this series if you insist.

I would prefer if you had a patch #1 introducing mipi_dsi_dual() and
then used it in patch #2. IMO there's no need for you to go back and
re-cleanup "novatek-nt36523". I think the panel you're dealing with is
the second one to have run into this issue during the "multi"
conversion and it's better to get the cleaner solution landed before
others copy.

That being said, I won't insist. You're right that the duplication
isn't any worse with your patch.

-Doug

[PATCH v2 2/3] 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.

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 369b0d8830c3..f9cc106c8eb6 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

Re: [PATCH v2 2/3] drm: Remove unused MIPI write seq and chatty functions

[Doug Anderson]

Hi,

On Tue, Jul 8, 2025 at 12:39 AM Brigham Campbell <me@brighamcampbell.com> wrote:
>
> Remove the deprecated mipi_dsi_generic_write_seq() and
> mipi_dsi_generic_write_chatty() functions now that they are no longer
> used.
>
> 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(-)

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

[PATCH v2 3/3] 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.

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 v2 3/3] drm: docs: Update task from drm TODO list

[Doug Anderson]

Hi,

On Tue, Jul 8, 2025 at 12:39 AM Brigham Campbell <me@brighamcampbell.com> wrote:
>
> 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.
>
> Signed-off-by: Brigham Campbell <me@brighamcampbell.com>
> ---
>  Documentation/gpu/todo.rst | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)

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