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@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.

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@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.

-Doug