Re: [PATCH] backlight: pwm_bl: Avoid backlight flicker if backlight control GPIO is input

On 7/21/21 6:12 PM, Daniel Thompson wrote:

On Wed, Jul 21, 2021 at 05:09:57PM +0200, Marek Vasut wrote:

quoted

On 7/21/21 12:49 PM, Daniel Thompson wrote:

quoted

quoted

I'm not sure that's correct, we can simply say that any new uses of the
pwm-backlight should specify the initial GPIO configuration, and for the
legacy ones, use whatever is in the code now.

I'm not 100% against the idea... however if we still have to get the
code to read state from the hardware right for legacy cases that means
we have to do the same work but with fewer people testing it.

We can do something like this:

if (of_property_read_bool(np, "enable-active-high"))
   gpiod_direction_output(pb->enable_gpio, 1);
else if (of_property_read_bool(np, "enable-active-low"))
   gpiod_direction_output(pb->enable_gpio, 0);
else {
   WARN_ON_ONCE("Fix your DT"); // or some such notification
   ... legacy code path ...
}

Note that I picked the same DT prop names as drivers/gpio/gpiolib-of.c
of_gpio_flags_quirks() uses, because we are headed into similar mess here
I'm afraid.

I don't quite understand what you mean here. We are using gpiolib so
for us there is no concept of active-high or active-low. The only
concept for us is whether enable_gpio is asserted or not.

It would look the same -- just substitute in "enable-on-boot" and 
"disable-on-boot" DT property.

What the DT property would be describing is purely whether the
bootloader left the backlight on or off.

Rather, it would simply control what is the default state of the 
backlight enable GPIO (enabled/disabled).

This sails very close to the
edge of what is in-scope for DT (at least it does it we can read
the inherited state directly from the hardware).

The problem with reading it out of hardware is that the hardware might 
be in undefined state and expects Linux to define that state, so that 
does not always work. Hence my initial suggestion to add a DT property 
to define the state up front, instead of using these fragile heuristics.

What it also means decisions about the DT bindings are more about
whether, if the backlight is lit up, the bootloader should also disclose
what it thinks it has established as the PWM duty cycle as well.

Please also consider the case where bootloader configures total minimum 
of the hardware to start Linux as soon as possible, i.e. it puts Linux 
in DRAM and jumps to Linux.

Overall I have fairly grave concerns that this simply moves
fragility into the bootloader rather then reducing it.

Wait a minute, I think we disconnected somewhere. I would rather prefer 
to remove the fragility and bootloader dependency altogether, exactly to 
avoid depending on the state the bootloader left the hardware in.