Hi,
On Mon, Dec 30, 2024 at 2:10 AM <neil.armstrong@xxxxxxxxxx> wrote:
>
> > static int xpp055c272_unprepare(struct drm_panel *panel)
> > {
> > struct xpp055c272 *ctx = panel_to_xpp055c272(panel);
> > struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> > - int ret;
> > -
> > - ret = mipi_dsi_dcs_set_display_off(dsi);
> > - if (ret < 0)
> > - dev_err(ctx->dev, "failed to set display off: %d\n", ret);
> > -
> > - mipi_dsi_dcs_enter_sleep_mode(dsi);
> > - if (ret < 0) {
> > - dev_err(ctx->dev, "failed to enter sleep mode: %d\n", ret);
> > - return ret;
> > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
> > +
> > + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> > + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> > + if (dsi_ctx.accum_err) {
> > + dev_err(ctx->dev, "failed to enter sleep mode: %d\n",
> > + dsi_ctx.accum_err);
You should delete the above error message, right?
mipi_dsi_dcs_enter_sleep_mode_multi() reports the error for you, I
think.
> > @@ -155,17 +147,19 @@ static int xpp055c272_prepare(struct drm_panel *panel)
> > {
> > struct xpp055c272 *ctx = panel_to_xpp055c272(panel);
> > struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> > - int ret;
> > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
> >
> > dev_dbg(ctx->dev, "Resetting the panel\n");
> > - ret = regulator_enable(ctx->vci);
> > - if (ret < 0) {
> > - dev_err(ctx->dev, "Failed to enable vci supply: %d\n", ret);
> > - return ret;
> > + dsi_ctx.accum_err = regulator_enable(ctx->vci);
> > + if (dsi_ctx.accum_err) {
>
> I would rather keep ret instead of abusing dsi_ctx.accum_err, but it's already like
> that in other converted driver so I won't oppose it...
FWIW, we had this discussion before. I agree with what Tejas did here
and I managed to convince Dmitry Baryshkov in the past. See:
https://lore.kernel.org/all/CAA8EJpr_HYkXnP3XR9LpDhi1xkQfE_CKJzfzGrO5qd_pQYtiOw@xxxxxxxxxxxxxx/
Looking specifically at this driver, using "ret" would have added
complexity when we wanted to do "goto disable_vci" because in some
cases the error code would be in "ret" and sometimes in "accum_err"...
> > @@ -175,30 +169,19 @@ static int xpp055c272_prepare(struct drm_panel *panel)
> > gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> >
> > /* T8: 20ms */
> > - msleep(20);
> > + mipi_dsi_msleep(&dsi_ctx, 20);
Personally, I would have left the above msleep() alone. There can be
no errors at this point in the code, right?
> > - ret = xpp055c272_init_sequence(ctx);
> > - if (ret < 0) {
> > - dev_err(ctx->dev, "Panel init sequence failed: %d\n", ret);
> > - goto disable_iovcc;
> > - }
> > -
> > - ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > - if (ret < 0) {
> > - dev_err(ctx->dev, "Failed to exit sleep mode: %d\n", ret);
> > - goto disable_iovcc;
> > - }
> > + xpp055c272_init_sequence(&dsi_ctx);
> > + dev_dbg(ctx->dev, "Panel init sequence done\n");
Should the above print be only if "accum_err" is 0? That would match
the previous behavior. I guess I would have also left the print as
part of xpp055c272_init_sequence() unless there's a reason for moving
it...
