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

[Marek Vasut <marex@denx.de>]

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

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