[PATCH v2] drm/panel/synaptics-r63353: Use _multi variants

[Anusha Srivatsa]

On Tue, Mar 11, 2025 at 11:52 AM Doug Anderson <dianders at chromium.org>
wrote:

> Hi,
>
> On Mon, Mar 10, 2025 at 1:58 PM Anusha Srivatsa <asrivats at redhat.com>
> wrote:
> >
> > @@ -70,6 +70,7 @@ static int r63353_panel_power_on(struct r63353_panel
> *rpanel)
> >  {
> >         struct mipi_dsi_device *dsi = rpanel->dsi;
> >         struct device *dev = &dsi->dev;
> > +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
> >         int ret;
> >
> >         ret = regulator_enable(rpanel->avdd);
> > @@ -78,7 +79,7 @@ static int r63353_panel_power_on(struct r63353_panel
> *rpanel)
> >                 return ret;
> >         }
> >
> > -       usleep_range(15000, 25000);
> > +       mipi_dsi_usleep_range(&dsi_ctx, 15000, 25000);
>
> No. None of the conversions in this function are correct.
> mipi_dsi_usleep_range() is only for use when you're in the middle of a
> bunch of other "multi" calls and want the sleep to be conditional upon
> there being no error. Here there is no chance of an error because no
> _multi() are used. Go back to the normal usleep_range().
>
>
OK. Then the approach to prefer mipi_dsi_usleep_range() over the previously
used usleep_range() everywhere is out the window. Sounds good. Is replacing
msleep() with mipi_dsi_msleep() preferable?

> @@ -106,53 +107,46 @@ static int r63353_panel_power_off(struct
> r63353_panel *rpanel)
> >  static int r63353_panel_activate(struct r63353_panel *rpanel)
> >  {
> >         struct mipi_dsi_device *dsi = rpanel->dsi;
> > -       struct device *dev = &dsi->dev;
> > -       int i, ret;
> > +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
> > +       int i;
> >
> > -       ret = mipi_dsi_dcs_soft_reset(dsi);
> > -       if (ret < 0) {
> > -               dev_err(dev, "Failed to do Software Reset (%d)\n", ret);
> > +       mipi_dsi_dcs_soft_reset_multi(&dsi_ctx);
> > +       if (dsi_ctx.accum_err)
> >                 goto fail;
> > -       }
>
> This isn't how the _multi() functions are intended to be used. The
> whole idea is _not_ to have scattered "if" statements everywhere and
> to just deal with errors at the appropriate places. You just trust
> that the _multi() functions are no-ops if an error is set and so it
> doesn't hurt to keep calling them. In this case you'd just have a pile
> of _multi() functions with no "if" checks and then at the very end of
> the function you check for the error. If the error is set then you set
> the reset GPIO and return the error.
>
>
Perfect. Thanks.

Anusha

> -Doug
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20250312/21b95b58/attachment.htm>