On Tue, Apr 01, 2025 at 04:01:03AM +0300, Dmitry Baryshkov wrote:
> 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?
>
Hmm... Most of the drivers ignore the results of the drm_panel_prepare()
/ _unprepare() / _enable() / _disable(), but then the framework handles
error values of the callbacks and skips calling the corresponding
en/dis callback if the previous call has failed. Which means I was
incorrect here.
>
> > 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
--
With best wishes
Dmitry
