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

[Marek Vasut <marex@denx.de>]

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.