Hi,
On Tue, Jun 18, 2024 at 5:25 AM Tejas Vipin <tejasvipi...@gmail.com> wrote:
>
> >> rm692e5_reset(ctx);
> >>
> >> - ret = rm692e5_on(ctx);
> >> - if (ret < 0) {
> >> - dev_err(dev, "Failed to initialize panel: %d\n", ret);
> >> + dsi_ctx.accum_err = rm692e5_on(ctx);
> >> + if (dsi_ctx.accum_err) {
> >> + dev_err(dev, "Failed to initialize panel: %d\n",
> >> dsi_ctx.accum_err);
> >
> > I'd probably change rm692e5_on() to take the "dsi_ctx" as a parameter
> > and then you don't need to declare a new one there.
> >
> > ...also, you don't need to add an error message since rm692e5_on()
> > will have already printed one (since the "multi" style functions
> > always print error messages for you).
>
> I'm guessing that the change about regulator_bulk_enable and
> rm692e5 should also be applied to all the other panels where
> similar behavior occurs?
Yeah, I'd say so.
> >> gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> >> regulator_bulk_disable(ARRAY_SIZE(ctx->supplies),
> >> ctx->supplies);
> >> - return ret;
> >> + return dsi_ctx.accum_err;
> >
> > Not new for your patch, but it seems odd that we don't do this error
> > handling (re-assert reset and disable the regulator) for errors later
> > in the function. Shouldn't it do that? It feels like the error
> > handling should be in an "err" label and we should end up doing that
> > any time we return an error code... What do you think?
>
> Personally I don't think this is necessary because imo labels
> only get useful when there's a couple of them and/or they're
> jumped to multiple times. I don't think either would happen in
> this particular function. But I guess if you have some convention
> in mind, then it could be done?
I think mostly my suggestion was just that we should also do the
gpiod_set_value_cansleep() and regulator_bulk_disable() at the end of
rm692e5_prepare() if `dsi_ctx.accum_err` is non-zero. Then you've got
two places doing the same thing: here and at the end of the function.
...oh, but everything below here is already a no-op if the error is
set. ...so I guess looking at it closer, my suggestion wouldn't be a
"goto" but would instead be to just move the gpio/regulator call to
the end. What do you think?
-Doug
