On 7/31/23 21:50, Doug Anderson wrote:
Hi,
On Mon, Jul 31, 2023 at 11:03 AM Marek Vasut <marex@xxxxxxx> 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);
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).
Yes, that's well possible.
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.
