[PATCH] drm/panel: ilitek-ili9805: Use _multi variants

[PATCH] drm/panel: ilitek-ili9805: Use _multi variants

[Michail Tatas]

Convert functions mipi_dsi_* to mipi_dsi_*_multi as per the
gpu/todo file.
The multi variant of these functions include improved error 
handling.

Signed-off-by: Michail Tatas <michail.tatas@gmail.com>
---
 drivers/gpu/drm/panel/panel-ilitek-ili9805.c | 51 +++++---------------
 1 file changed, 13 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9805.c b/drivers/gpu/drm/panel/panel-ilitek-ili9805.c
index e6c483851f1f..7e9587afebbe 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9805.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9805.c
@@ -159,36 +159,23 @@ static int ili9805_power_off(struct ili9805 *ctx)
 
 static int ili9805_activate(struct ili9805 *ctx)
 {
-	struct mipi_dsi_device *dsi = ctx->dsi;
-	struct device *dev = &dsi->dev;
-	int i, ret;
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
+	int i;
 
 	for (i = 0; i < ctx->desc->init_length; i++) {
 		const struct ili9805_instr *instr = &ctx->desc->init[i];
 
-		ret = mipi_dsi_dcs_write_buffer(ctx->dsi, instr->data, instr->len);
-		if (ret < 0)
-			return ret;
+		mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, instr->data, instr->len);
 
 		if (instr->delay > 0)
-			msleep(instr->delay);
-	}
-
-	ret = mipi_dsi_dcs_exit_sleep_mode(ctx->dsi);
-	if (ret) {
-		dev_err(dev, "Failed to exit sleep mode (%d)\n", ret);
-		return ret;
+			mipi_dsi_msleep(&dsi_ctx, instr->delay);
 	}
 
-	usleep_range(5000, 6000);
+	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
+	mipi_dsi_usleep_range(&dsi_ctx, 5000, 6000);
+	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
 
-	ret = mipi_dsi_dcs_set_display_on(ctx->dsi);
-	if (ret) {
-		dev_err(dev, "Failed to set display ON (%d)\n", ret);
-		return ret;
-	}
-
-	return 0;
+	return dsi_ctx.accum_err;
 }
 
 static int ili9805_prepare(struct drm_panel *panel)
@@ -211,25 +198,13 @@ static int ili9805_prepare(struct drm_panel *panel)
 
 static int ili9805_deactivate(struct ili9805 *ctx)
 {
-	struct mipi_dsi_device *dsi = ctx->dsi;
-	struct device *dev = &dsi->dev;
-	int ret;
-
-	ret = mipi_dsi_dcs_set_display_off(ctx->dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to set display OFF (%d)\n", ret);
-		return ret;
-	}
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
 
-	usleep_range(5000, 10000);
-
-	ret = mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to enter sleep mode (%d)\n", ret);
-		return ret;
-	}
+	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
+	mipi_dsi_usleep_range(&dsi_ctx, 5000, 10000);
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
 
-	return 0;
+	return dsi_ctx.accum_err;
 }
 
 static int ili9805_unprepare(struct drm_panel *panel)
-- 
2.43.0

Re: [PATCH] drm/panel: ilitek-ili9805: Use _multi variants

[Doug Anderson]

Hi,

On Thu, Jul 16, 2026 at 3:09 PM Michail Tatas <michail.tatas@gmail.com> wrote:
>
> @@ -159,36 +159,23 @@ static int ili9805_power_off(struct ili9805 *ctx)
>
>  static int ili9805_activate(struct ili9805 *ctx)
>  {
> -       struct mipi_dsi_device *dsi = ctx->dsi;
> -       struct device *dev = &dsi->dev;
> -       int i, ret;
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +       int i;
>
>         for (i = 0; i < ctx->desc->init_length; i++) {
>                 const struct ili9805_instr *instr = &ctx->desc->init[i];
>
> -               ret = mipi_dsi_dcs_write_buffer(ctx->dsi, instr->data, instr->len);
> -               if (ret < 0)
> -                       return ret;
> +               mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, instr->data, instr->len);
>
>                 if (instr->delay > 0)
> -                       msleep(instr->delay);
> -       }

What you've done is an improvement, but it's not all the way there.
Specifically, we'd really want to get rid of the whole "struct
ili9805_instr" type and instead each panel should have an init
function. The "struct ili9805_desc" should have a pointer to the init
function instead of a pointer to the init data.

For details, you can see a preivous email about this [1], which then
has further links if you want to dig into details. You can see that
Chintan eventually implemented this in commit a89d9a327d06
("drm/panel: novatek-nt36672a: Inline panel init sequences").

If landing this patch without cleaning up the init sequences is really
important to you, I could be convinced. However, the init sequences
aren't all that big and it would be nice if you could clean it up all
at once. It could be one patch or two.

[1] http://lore.kernel.org/r/CAD=FV=UCyfjiqcpYCM5ePz-auX4g=i_+i78ivvvyA8R1XtAKzg@mail.gmail.com


> @@ -211,25 +198,13 @@ static int ili9805_prepare(struct drm_panel *panel)
>
>  static int ili9805_deactivate(struct ili9805 *ctx)
>  {
> -       struct mipi_dsi_device *dsi = ctx->dsi;
> -       struct device *dev = &dsi->dev;
> -       int ret;
> -
> -       ret = mipi_dsi_dcs_set_display_off(ctx->dsi);
> -       if (ret < 0) {
> -               dev_err(dev, "Failed to set display OFF (%d)\n", ret);
> -               return ret;
> -       }
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
>
> -       usleep_range(5000, 10000);
> -
> -       ret = mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
> -       if (ret < 0) {
> -               dev_err(dev, "Failed to enter sleep mode (%d)\n", ret);
> -               return ret;
> -       }
> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +       mipi_dsi_usleep_range(&dsi_ctx, 5000, 10000);
> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
>
> -       return 0;
> +       return dsi_ctx.accum_err;

Nobody looks at the return code of this function. Since you're
touching it anyway, can you change ili9805_deactivate() to return
"void"? I wouldn't object if you made ili9805_power_off() return
"void" in the same patch too, even though it's a bit unrelated to the
rest of the patch.

-Doug

Re: [PATCH] drm/panel: ilitek-ili9805: Use _multi variants

[Michail Tatas]

On Thu, Jul 16, 2026 at 04:31:53PM -0700, Doug Anderson wrote:

Hello Doug, thanks a lot for the review!

> Hi,
> 
> On Thu, Jul 16, 2026 at 3:09 PM Michail Tatas <michail.tatas@gmail.com> wrote:
> >
> > @@ -159,36 +159,23 @@ static int ili9805_power_off(struct ili9805 *ctx)
> >
> >  static int ili9805_activate(struct ili9805 *ctx)
> >  {
> > -       struct mipi_dsi_device *dsi = ctx->dsi;
> > -       struct device *dev = &dsi->dev;
> > -       int i, ret;
> > +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> > +       int i;
> >
> >         for (i = 0; i < ctx->desc->init_length; i++) {
> >                 const struct ili9805_instr *instr = &ctx->desc->init[i];
> >
> > -               ret = mipi_dsi_dcs_write_buffer(ctx->dsi, instr->data, instr->len);
> > -               if (ret < 0)
> > -                       return ret;
> > +               mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, instr->data, instr->len);
> >
> >                 if (instr->delay > 0)
> > -                       msleep(instr->delay);
> > -       }
> 
> What you've done is an improvement, but it's not all the way there.
> Specifically, we'd really want to get rid of the whole "struct
> ili9805_instr" type and instead each panel should have an init
> function. The "struct ili9805_desc" should have a pointer to the init
> function instead of a pointer to the init data.

I am more than happy to do these changes as well. My only concern is
that I do not have hardware to test the panel. Are compiling and static
analysis sufficient tests for these changes ?

> For details, you can see a preivous email about this [1], which then
> has further links if you want to dig into details. You can see that
> Chintan eventually implemented this in commit a89d9a327d06
> ("drm/panel: novatek-nt36672a: Inline panel init sequences").
> 
> If landing this patch without cleaning up the init sequences is really
> important to you, I could be convinced. However, the init sequences
> aren't all that big and it would be nice if you could clean it up all
> at once. It could be one patch or two.
> 

It is not all that important, I can clean up the init sequences and
send it as part of the v2

> [1] http://lore.kernel.org/r/CAD=FV=UCyfjiqcpYCM5ePz-auX4g=i_+i78ivvvyA8R1XtAKzg@mail.gmail.com
> 
> 
> > @@ -211,25 +198,13 @@ static int ili9805_prepare(struct drm_panel *panel)
> >
> >  static int ili9805_deactivate(struct ili9805 *ctx)
> >  {
> > -       struct mipi_dsi_device *dsi = ctx->dsi;
> > -       struct device *dev = &dsi->dev;
> > -       int ret;
> > -
> > -       ret = mipi_dsi_dcs_set_display_off(ctx->dsi);
> > -       if (ret < 0) {
> > -               dev_err(dev, "Failed to set display OFF (%d)\n", ret);
> > -               return ret;
> > -       }
> > +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> >
> > -       usleep_range(5000, 10000);
> > -
> > -       ret = mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
> > -       if (ret < 0) {
> > -               dev_err(dev, "Failed to enter sleep mode (%d)\n", ret);
> > -               return ret;
> > -       }
> > +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> > +       mipi_dsi_usleep_range(&dsi_ctx, 5000, 10000);
> > +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> >
> > -       return 0;
> > +       return dsi_ctx.accum_err;
> 
> Nobody looks at the return code of this function. Since you're
> touching it anyway, can you change ili9805_deactivate() to return
> "void"? I wouldn't object if you made ili9805_power_off() return
> "void" in the same patch too, even though it's a bit unrelated to the
> rest of the patch.
> 

Good point regarding the return types, I will change them to
return void.

I will send a v2 with the requested changes.

Regards,
Michail

Re: [PATCH] drm/panel: ilitek-ili9805: Use _multi variants

[Doug Anderson]

Hi,

On Fri, Jul 17, 2026 at 3:12 AM Michail Tatas <michail.tatas@gmail.com> wrote:
>
> On Thu, Jul 16, 2026 at 04:31:53PM -0700, Doug Anderson wrote:
>
> Hello Doug, thanks a lot for the review!
>
> > Hi,
> >
> > On Thu, Jul 16, 2026 at 3:09 PM Michail Tatas <michail.tatas@gmail.com> wrote:
> > >
> > > @@ -159,36 +159,23 @@ static int ili9805_power_off(struct ili9805 *ctx)
> > >
> > >  static int ili9805_activate(struct ili9805 *ctx)
> > >  {
> > > -       struct mipi_dsi_device *dsi = ctx->dsi;
> > > -       struct device *dev = &dsi->dev;
> > > -       int i, ret;
> > > +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> > > +       int i;
> > >
> > >         for (i = 0; i < ctx->desc->init_length; i++) {
> > >                 const struct ili9805_instr *instr = &ctx->desc->init[i];
> > >
> > > -               ret = mipi_dsi_dcs_write_buffer(ctx->dsi, instr->data, instr->len);
> > > -               if (ret < 0)
> > > -                       return ret;
> > > +               mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, instr->data, instr->len);
> > >
> > >                 if (instr->delay > 0)
> > > -                       msleep(instr->delay);
> > > -       }
> >
> > What you've done is an improvement, but it's not all the way there.
> > Specifically, we'd really want to get rid of the whole "struct
> > ili9805_instr" type and instead each panel should have an init
> > function. The "struct ili9805_desc" should have a pointer to the init
> > function instead of a pointer to the init data.
>
> I am more than happy to do these changes as well. My only concern is
> that I do not have hardware to test the panel. Are compiling and static
> analysis sufficient tests for these changes ?

That is inevitable with these cleanups. We're doing our best with
static analysis and code review. If someone comes along later and
finds that something broke, we'll fix it.

-Doug