On 7/19/21 1:22 PM, Daniel Thompson wrote:
On Sun, Jul 18, 2021 at 11:14:15PM +0200, Marek Vasut wrote:
If the backlight enable GPIO is configured as input, the driver currently
unconditionally forces the GPIO to output-enable. This can cause backlight
flicker on boot e.g. in case the GPIO should not be enabled before the PWM
is configured and is correctly pulled low by external resistor.
Fix this by extending the current check to differentiate between backlight
GPIO enable set as input and set as direction unknown. In case of input,
read the GPIO value to determine the pull resistor placement, set the GPIO
as output, and drive that exact value it was pulled to. In case of unknown
direction, retain previous behavior, that is set the GPIO as output-enable.
Fixes: 3698d7e7d221 ("backlight: pwm_bl: Avoid backlight flicker when probed from DT")
Signed-off-by: Marek Vasut <marex@xxxxxxx>
Cc: Christian Gmeiner <christian.gmeiner@xxxxxxxxx>
Cc: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
Cc: Heiko Stuebner <heiko@xxxxxxxxx>
Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
Cc: Thierry Reding <treding@xxxxxxxxxx>
Cc: linux-pwm@xxxxxxxxxxxxxxx
Cc: linux-fbdev@xxxxxxxxxxxxxxx
To: dri-devel@xxxxxxxxxxxxxxxxxxxxx
---
NOTE: I think this whole auto-detection scheme should just be replaced by a
DT prop, because it is very fragile.
I have some sympathy for this view... although I think the boat has
already set sail.
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.
However, on the basis of making things less fragile, I think the
underlying problem here is the assumption that it is safe to modify
enable_gpio before the driver has imposed state upon the PWM (this
assumption has always been made and, in addition to systems where the BL
has a phandle will also risks flicker problems on systems where
power_pwm_on_delay is not zero).
It is safe to modify the GPIO into defined state, but that defined state
is not always out/enabled, that defined state depends on the hardware.
This patch does not change the assumption that we can configure the
GPIO before we modify the PWM state. This means it won't fix the problem
for cases there the pin is HiZ by default but whose GPIOD_ASIS state is
neither input nor output.
That is correct, for pin that is floating, we lost. But then I would
argue that if your backlight-enable GPIO is floating, the hardware is
buggy, I would expect some pull resistor to keep the backlight off on
power on on that GPIO.
I wonder if it might be better to move the code to configure the
direction of enable_gpio out of the probe function and into
pwm_backlight_power_on():
No, I tried that already.
The first thing that is called on boot is pwm_backlight_power_off() to
set the backlight to 0 (and thus set pwm to 0), but since pb->enabled is
false, that is where the function exits with configuring PWM and without
configuring the GPIO state.
I also experimented with some "first time only" flag in those functions,
but that looked ugly and complicated the runtime code.
if (pb->enable_gpio) {
if (gpiod_get_direction(pb->enable_gpio) != 0))
gpiod_direction_output(pb->enable_gpio, 1);
else
gpiod_set_value_can_sleep(pb->enable_gpio, 1);
}
By the time we reach this function the driver explicitly applies state
to the GPIO then we know what the value must be.
See above, I don't think that's the best option.
