[PATCH] drm/panel: simple: Initialize unprepared_time in probe

[Doug Anderson]

Hi,

On Mon, Jul 31, 2023 at 2:15 PM Marek Vasut <marex at denx.de> wrote:
>
> On 7/31/23 21:50, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Jul 31, 2023 at 11:03 AM Marek Vasut <marex at denx.de> wrote:
> >>
> >> On 7/24/23 15:49, Doug Anderson wrote:
> >>
> >> Hi,
> >>
> >> [...]
> >>
> >>>> Maybe the EPROBE_DEFER actually happens and triggers the failure ?
> >>>
> >>> I could certainly believe that EPROBE_DEFER is involved.
> >>
> >> So no, it is not. It is difficult to set this up and access the signals,
> >> but so I did.
> >>
> >> What happens is this:
> >> panel_simple_probe() calls devm_regulator_get()
> >>     -> If the regulator was ENABLED, then it is now DISABLED
> >
> > As per my previous response, I don't think this is true.
>
> I just measured that with a scope on actual hardware .
>
> reg_fixed_voltage_probe() is the code which turns the regulator OFF,
> specifically in the part gpiod_get_optional(...GPIOD_OUT_LOW);

Great, at least we're on the same page here now.


> > So just as a point of order, do you agree that prior to the commit
> > that you are "Fixing" that we _weren't_ doing the full delay at probe
> > time? That means that if things worked before they were working by
> > some amount of luck, right? The old code used to do a delay after
> > turning _off_ the regulator (at unprepare time).
>
> Yes, that's well possible.

...so assuming we agree that this is _not_ a regression introduced by
e5e30dfcf3db ("drm: panel: simple: Defer unprepare delay till next
prepare to shorten it"), that means that all other preexisting users
of panel-simple were fine with the old behavior where the unprepare
delay was only enforced when the panel driver itself turned things off
and then back on and no extra delay was needed at probe. The one board
we know about that has a problem with this behavior is the one you're
reproducing on, which we think only worked previously by chance.


> > I will also continue to assert that trying to fix the problem via a
> > delay in the probe of the panel code is the wrong place to fix the
> > code. The problem is that you need a board-level constraint on this
> > regulator (off-on-delay-us) preventing it from turning on again within
> > a certain amount of time after it is turned off. This allows the
> > regulator framework, which is what decided to turn this rail off
> > during the regulator probe, to enforce this constraint.
>
> No, the driver must respect the unprepare delay, that is what is
> currently not happening. Trying to somehow work around that by adding
> random changes to DT is not going to fix the fact that panel-simple is
> not respecting its own internal built-in constraints.

Well, except that the panel _isn't_ actually unpreparing the panel.
The constant you're talking about is a delay between unpreparing the
panel and then preparing it again and we're not doing that here. Here,
you are trying to account for an implicit unprepare that happened
outside the context of the panel driver (in the regulator framework).
The regulator framework is the one disabling the regulator on its own
and without the knowledge of the panel driver. The problem should be
addressed at the regulator framework, which might involve the
off-on-delay.

I would even go further and say that, perhaps, when the regulator
framework turns this regulator off at bootup then you might be
violating the power sequencing requirements in the panel anyway. If
the bootloader left the panel on and _also_ left an enable GPIO on
then it's entirely possible that you've got a power leak through the
enable GPIO where you're backpowering the panel's logic when the
regulator framework turns things off. This is something that would be
impossible for the panel driver to solve because the panel driver
hasn't even probed yet.

In any case, at this point it seems unlikely that I'll convince you or
that you'll convince me. I wonder if it's time for someone else on
this thread to provide an opinion.

-Doug