Re: [PATCH v2] drm/panel: tdo-tl070wsh30: Use mipi_dsi_*_multi() functions

[Doug Anderson]

Hi,

On Mon, Jul 27, 2026 at 6:45 PM Akash Sukhavasi
<akash.sukhavasi@xxxxxxxxx> wrote:
>
> The mipi_dsi_dcs_*() functions used by this driver are deprecated in
> favor of their _multi() counterparts, as noted in
> Documentation/gpu/todo.rst. The _multi() variants record the first
> error in a context structure and skip every later call once an error
> is set, so the return value no longer has to be checked after each
> command. They also log their own failures, which makes the per-call
> dev_err() calls redundant.
>
> Convert prepare() and unprepare(), using mipi_dsi_msleep() and
> mipi_dsi_usleep_range() for the delays between DSI commands. The
> delays in the GPIO reset sequence stay as plain msleep() and
> usleep_range(), since they run before any DSI transaction.
>
> unprepare() now disables the regulator unconditionally and returns 0.
> Previously a failure of enter_sleep_mode() returned early, leaving the
> regulator enabled. drm_panel_unprepare() skips panel->prepared = false
> when the callback returns an error, and drm_panel_prepare() returns
> early when prepared is already set, so that path left the panel
> powered and unable to be brought back up. Both functions return void,
> so the error was never propagated to a caller in any case.
>
> Signed-off-by: Akash Sukhavasi <akash.sukhavasi@xxxxxxxxx>
> ---
> Compile tested only, no hardware available. checkpatch and a W=1 build
> are clean.
>
> Changes in v2:
> - unprepare() disables the regulator unconditionally and returns 0,
>   per Sashiko's review on v1. Returning an error left panel->prepared
>   set, so the panel could not be prepared again.
> - Link to v1: https://lore.kernel.org/r/20260725-mipi-dsi-tl070wsh30-multi-v1-1-69160b83982e@xxxxxxxxx
> ---
>  drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c | 40 ++++++++++------------------
>  1 file changed, 14 insertions(+), 26 deletions(-)

Just as a point of note, something about your original email to me and
your original patch triggered gmail's SPAM filters, so I actually
didn't see anything from you until your first reply to Sashiko [1],
then I've been a bit backlogged. Sorry about that!

[1] https://lore.kernel.org/r/amU_GppAwt3XXPX-@xxxxxxxxx


Also: since your patch contains some minor bugfixes, it's probably
good not to hide under a code cleanup. Maybe a better subject line:

drm/panel: tdo-tl070wsh30: Use mipi_dsi_*_multi(); fix minor bugs


> diff --git a/drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c b/drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c
> index 13cfe252a838..cd846e5ab2e7 100644
> --- a/drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c
> +++ b/drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c
> @@ -35,6 +35,7 @@ struct tdo_tl070wsh30_panel *to_tdo_tl070wsh30_panel(struct drm_panel *panel)
>  static int tdo_tl070wsh30_panel_prepare(struct drm_panel *panel)
>  {
>         struct tdo_tl070wsh30_panel *tdo_tl070wsh30 = to_tdo_tl070wsh30_panel(panel);
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = tdo_tl070wsh30->link };
>         int err;
>
>         err = regulator_enable(tdo_tl070wsh30->supply);
> @@ -51,45 +52,32 @@ static int tdo_tl070wsh30_panel_prepare(struct drm_panel *panel)
>
>         msleep(200);
>
> -       err = mipi_dsi_dcs_exit_sleep_mode(tdo_tl070wsh30->link);
> -       if (err < 0) {
> -               dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
> -               regulator_disable(tdo_tl070wsh30->supply);
> -               return err;
> -       }
> +       mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
>
> -       msleep(200);
> +       mipi_dsi_msleep(&dsi_ctx, 200);
>
> -       err = mipi_dsi_dcs_set_display_on(tdo_tl070wsh30->link);
> -       if (err < 0) {
> -               dev_err(panel->dev, "failed to set display on: %d\n", err);
> -               regulator_disable(tdo_tl070wsh30->supply);
> -               return err;
> -       }
> +       mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
>
> -       msleep(20);
> +       mipi_dsi_msleep(&dsi_ctx, 20);
>
> -       return 0;
> +       if (dsi_ctx.accum_err)
> +               regulator_disable(tdo_tl070wsh30->supply);
> +
> +       return dsi_ctx.accum_err;

Style nit: in general, there are way too many blank lines. You don't
need a blank line between every statement.

>  }
>
>  static int tdo_tl070wsh30_panel_unprepare(struct drm_panel *panel)
>  {
>         struct tdo_tl070wsh30_panel *tdo_tl070wsh30 = to_tdo_tl070wsh30_panel(panel);
> -       int err;
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = tdo_tl070wsh30->link };
>
> -       err = mipi_dsi_dcs_set_display_off(tdo_tl070wsh30->link);
> -       if (err < 0)
> -               dev_err(panel->dev, "failed to set display off: %d\n", err);
> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
>
> -       usleep_range(10000, 11000);
> +       mipi_dsi_usleep_range(&dsi_ctx, 10000, 11000);
>
> -       err = mipi_dsi_dcs_enter_sleep_mode(tdo_tl070wsh30->link);
> -       if (err < 0) {
> -               dev_err(panel->dev, "failed to enter sleep mode: %d\n", err);
> -               return err;
> -       }
> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
>
> -       usleep_range(10000, 11000);
> +       mipi_dsi_usleep_range(&dsi_ctx, 10000, 11000);
>
>         regulator_disable(tdo_tl070wsh30->supply);

I support fixing `unprepare` to always return 0, and I'm glad you
mentioned that fix in the commit message.

...and I think it should be fine to disable the regulator even if the
MIPI commands fail. ...at least, I hope so.

However, I think the Sashiko bot had a point [2] when it noted that
the old code used to still sleep for ~10ms and try to enter sleep mode
even if the "display off" failed. That behavior is probably worth
preserving.

[2] http://lore.kernel.org/r/20260728015051.97FA71F000E9@xxxxxxxxxxxxxxx

-Doug