Hi, On Thu, May 9, 2024 at 3:37 PM Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote: > > Remove conditional code and always use mipi_dsi_dcs_*multi() wrappers to > simplify driver's init/exit code. This also includes passing context to > the init_sequence() function instead of passing the DSI device. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > --- > drivers/gpu/drm/panel/panel-novatek-nt36672e.c | 587 ++++++++++++------------- > 1 file changed, 281 insertions(+), 306 deletions(-) One note is that we might not be able to land ${SUBJECT} patch since the patch it's based on [1] doesn't have any Reviewed-by tags. Just sayin'. ;-) [1] https://lore.kernel.org/r/20240508135148.v4.6.I3c08a7d02c467d2bc88da14e513ea4c8649fce45@changeid > @@ -381,61 +377,40 @@ static int nt36672e_power_off(struct nt36672e_panel *ctx) > return ret; > } > > -static int nt36672e_on(struct nt36672e_panel *ctx) > +static int nt36672e_on(struct nt36672e_panel *panel) Small nit that I think of the variable "panel" as referring to a "struct drm_panel". I'd personally rather this be named something else, like "nt36672e". > { > - struct mipi_dsi_device *dsi = ctx->dsi; > - const struct panel_desc *desc = ctx->desc; > - int ret = 0; > + struct mipi_dsi_multi_context ctx = { .dsi = panel->dsi }; > + struct mipi_dsi_device *dsi = panel->dsi; > + const struct panel_desc *desc = panel->desc; > > dsi->mode_flags |= MIPI_DSI_MODE_LPM; > > - if (desc->init_sequence) { > - ret = desc->init_sequence(dsi); > - if (ret < 0) { > - dev_err(&dsi->dev, "panel init sequence failed: %d\n", ret); > - return ret; > - } > - } > + if (desc->init_sequence) > + desc->init_sequence(&ctx); > > - ret = mipi_dsi_dcs_exit_sleep_mode(dsi); > - if (ret < 0) { > - dev_err(&dsi->dev, "Failed to exit sleep mode: %d\n", ret); > - return ret; > - } > - msleep(120); > + mipi_dsi_dcs_exit_sleep_mode_multi(&ctx); > + mipi_dsi_msleep(&ctx, 120); > > - ret = mipi_dsi_dcs_set_display_on(dsi); > - if (ret < 0) { > - dev_err(&dsi->dev, "Failed to set display on: %d\n", ret); > - return ret; > - } > - msleep(100); > + mipi_dsi_dcs_set_display_on_multi(&ctx); > > - return 0; > + mipi_dsi_msleep(&ctx, 100); > + > + return ctx.accum_err; > } > > -static int nt36672e_off(struct nt36672e_panel *ctx) > +static int nt36672e_off(struct nt36672e_panel *panel) > { > - struct mipi_dsi_device *dsi = ctx->dsi; > - int ret = 0; > + struct mipi_dsi_multi_context ctx = { .dsi = panel->dsi }; > > - dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; > + panel->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; > > - ret = mipi_dsi_dcs_set_display_off(dsi); > - if (ret < 0) { > - dev_err(&dsi->dev, "Failed to set display off: %d\n", ret); > - return ret; > - } > - msleep(20); > + mipi_dsi_dcs_set_display_off_multi(&ctx); > + mipi_dsi_msleep(&ctx, 20); > > - ret = mipi_dsi_dcs_enter_sleep_mode(dsi); > - if (ret < 0) { > - dev_err(&dsi->dev, "Failed to enter sleep mode: %d\n", ret); > - return ret; > - } > - msleep(60); > + mipi_dsi_dcs_enter_sleep_mode_multi(&ctx); > + mipi_dsi_msleep(&ctx, 60); > > - return 0; > + return ctx.accum_err; > } nit: similar to other patches in the series, the callers of nt36672e_on() and nt36672e_off() should be able to get rid of their error prints. In any case: Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>