Hi,
On Mon, Mar 31, 2025 at 1:28 PM Dmitry Baryshkov
<dmitry.barysh...@oss.qualcomm.com> wrote:
>
> On Mon, Mar 31, 2025 at 08:06:36AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Sun, Mar 30, 2025 at 11:18 PM Tejas Vipin <tejasvipi...@gmail.com> wrote:
> > >
> > > @@ -157,7 +137,6 @@ static int boe_bf060y8m_aj0_prepare(struct drm_panel
> > > *panel)
> > >
> > > ret = boe_bf060y8m_aj0_on(boe);
> > > if (ret < 0) {
> > > - dev_err(dev, "Failed to initialize panel: %d\n", ret);
> > > gpiod_set_value_cansleep(boe->reset_gpio, 1);
> > > return ret;
> >
> > It's not new, but the error handling here looks wrong to me. Instead
> > of just returning after setting the GPIO, this should be turning off
> > the regulators, shouldn't it? That would mean adding a new error label
> > for turning off "BF060Y8M_VREG_VCI" and then jumping to that.
>
> We should not be turning off the regulator in _prepare(), there will be
> an unmatched regulator disable call happening in _unprepare(). Of course
> it can be handled by adding a boolean, etc, but I think keeping them on
> is a saner thing.
Hrmmmm.
The issue is that if we're returning an error from a function the
caller should expect that the function undid anything that it did
partially. It _has_ to work that way, right? Otherwise we've lost the
context of exactly how far we got through the function so we don't
know which things to later undo and which things to later not undo.
...although I think you said that the DRM framework ignores errors
from prepare() and still calls unprepare(). I guess this is in
panel_bridge_atomic_pre_enable() where drm_panel_prepare()'s error
code is ignored? This feels like a bug waiting to happen. Are you
saying that boe_bf060y8m_aj0_unprepare() has to be written such that
it doesn't hit regulator underflows no matter which fail path
boe_bf060y8m_aj0_prepare() hit? That feels wrong.
-Doug
