linux-kernel - Re: [PATCH] drm/panel: xinpeng-xpp055c272: transition to mipi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=WEEbC+LB9TM8VHDgO41=XPdgJR6eSYHo=KytDbPTROqw@mail.gmail.com>
Date: Mon, 6 Jan 2025 16:07:31 -0800
From: Doug Anderson <dianders@...omium.org>
To: neil.armstrong@...aro.org
Cc: Tejas Vipin <tejasvipin76@...il.com>, maarten.lankhorst@...ux.intel.com, 
	mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch, 
	quic_jesszhan@...cinc.com, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/panel: xinpeng-xpp055c272: transition to mipi_dsi
 wrapped functions

Hi,

On Mon, Dec 30, 2024 at 2:10 AM <neil.armstrong@...aro.org> 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@mail.gmail.com/

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...