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

[Doug Anderson]

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. This was the
part where I referred to `Documentation/power/regulator/consumer.rst`
which said:

NOTE:
  The supply may already be enabled before regulator_enabled() is called.
  This may happen if the consumer shares the regulator or the regulator has been
  previously enabled by bootloader or kernel board initialization code.


My belief is that what's actually happening is that when the regulator
_probes_ that the regulator turns off. In Linux GPIO regulators cannot
read the state of the regulator at bootup. That means that when the
regulator itself probes that Linux has to decide on the default state
of the regulator itself. If the device tree has "regulator-boot-on"
then Linux will turn the regulator on (even without any clients). In
this case the regulator will stay on until some client enables and
then disables the regulator, or until the regulator boot timeout
happens and all unused regulators are powered off. If the device tree
doesn't have "regulator-boot-on" then Linux will turn the regulator
off.


>    -> For regulator-fixed, this means the regulator GPIO goes HIGH->LOW
>
> panel_simple_prepare() triggers panel_simple_resume()
>    -> If this occurs too soon after devm_regulator_get() turned the
>       regulator OFF and thus regulator GPIO low, then unprepare time is
>       not respected => FAIL
>
> Since there is no way to find out in which state the regulator was when
> devm_regulator_get() was called, we have to wait the full unprepare time
> before re-enabling that regulator in panel_simple_resume().

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).

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.


-Doug