Hi, On Mon, Jul 27, 2026 at 6:45 PM Akash Sukhavasi <[email protected]> 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 <[email protected]> > --- > 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/[email protected] > --- > 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/[email protected] 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/[email protected] -Doug
