On Mon, Mar 31, 2025 at 03:40:27PM -0700, Doug Anderson wrote:
> 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.
Kind of yes. I'd rather make drm_panel functions return void here, as
that matches panel bridge behaviour. The only driver that actually uses
return values of those functions is analogix_dp, see
analogix_dp_prepare_panel(). However most of invocations of that
function can go away. I'll send a patchset.
>
> ...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?
Yes.
> 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.
Let me try to fix that.
--
With best wishes
Dmitry
