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@xxxxxxxxx> 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@xxxxxxxxxxxxxx
> 
> 
> > @@ -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