On Tue, Mar 11, 2025 at 11:52 AM Doug Anderson <diand...@chromium.org>
wrote:
> Hi,
>
> On Mon, Mar 10, 2025 at 1:58 PM Anusha Srivatsa <asriv...@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
>
>
