[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <uhjjn5fto22s24vy6gaerhnuqepgc3y6im7mq36dbvdya62m5w@wfjp5qgqym4f>
Date: Wed, 26 Jun 2024 14:52:52 +0200
From: Ondřej Jirman <megi@....cz>
To: Guido Günther <agx@...xcpu.org>
Cc: Tejas Vipin <tejasvipin76@...il.com>, kernel@...i.sm,
neil.armstrong@...aro.org, quic_jesszhan@...cinc.com, dianders@...omium.org,
maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com,
daniel@...ll.ch, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/panel: sitronix-st7703: transition to mipi_dsi
wrapped functions
Hi Guido,
On Wed, Jun 26, 2024 at 02:25:16PM GMT, Guido Günther wrote:
> [...]
> > - ret = ctx->desc->init_sequence(ctx);
> > - if (ret < 0) {
> > - dev_err(ctx->dev, "Panel init sequence failed: %d\n", ret);
> > - return ret;
> > - }
> > + ctx->desc->init_sequence(&dsi_ctx);
>
> Why no return early here in the error case (same for the other cases
> below) giving us an indication which step went wrong?
Return early is hidden in the wrapped *_multi() function calls.
> >
> > - ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > - if (ret < 0) {
> > - dev_err(ctx->dev, "Failed to exit sleep mode: %d\n", ret);
> > - return ret;
> > - }
> > + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> >
> > /* It takes the controller 120 msec to wake up after sleep. */
> > - msleep(120);
> > + mipi_dsi_msleep(&dsi_ctx, 120);
> >
> > - ret = mipi_dsi_dcs_set_display_on(dsi);
> > - if (ret)
> > - return ret;
> > + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> >
> > - dev_dbg(ctx->dev, "Panel init sequence done\n");
>
> Would be nice to keep the debug message.
>
> > + if (!dsi_ctx.accum_err)
> > + dev_dbg(ctx->dev, "Panel init sequence done\n");
> >
> > - return 0;
> > + return dsi_ctx.accum_err;
> > }
> >
> > static int st7703_disable(struct drm_panel *panel)
> > {
> > struct st7703 *ctx = panel_to_st7703(panel);
> > struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> > - int ret;
> > + struct mipi_dsi_multi_context dsi_ctx = {.dsi = dsi};
> >
> > - ret = mipi_dsi_dcs_set_display_off(dsi);
> > - if (ret < 0)
> > - dev_err(ctx->dev, "Failed to turn off the display: %d\n", ret);
> > + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
>
> Similar to the above: it'd be nice to keep the information which step
> failed.
That's done by the _multi() wrappers.
kind regards,
o.
Powered by blists - more mailing lists