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@denx.de Cc: Christian Gmeiner christian.gmeiner@gmail.com Cc: Daniel Thompson daniel.thompson@linaro.org Cc: Heiko Stuebner heiko@sntech.de Cc: Philipp Zabel p.zabel@pengutronix.de Cc: Thierry Reding treding@nvidia.com Cc: linux-pwm@vger.kernel.org Cc: linux-fbdev@vger.kernel.org To: dri-devel@lists.freedesktop.org
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.