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

[Daniel Thompson <daniel.thompson@linaro.org>]

On Tue, Jul 20, 2021 at 10:28:32PM +0200, Marek Vasut wrote:
> 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.

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.


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

It is only safe to do this once we know what the initial value should be
and I'm not sure that value can comes exclusively from reading the pin.


> > 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 didn't say that the pin was floating. I said that the pin was in a HiZ
state meaning it could still be subject to pull up/down.

However there are cases, such as when the regulator is off, where I
think it is entirely legitimate for the enable pin to be floating. The
current driver does the wrong thing here if the pin is set as input
since if the regulator is off the initial enable_gpio value should be 0.


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

I followed that idea and came to a similar conclusion w.r.t. to the
first time flag.

I think a reasonably elegant approach can be reached by making
pwm_backlight_initial_power_state() responsible for ensuring enable_gpio
matches the observed hardware state (taking into account both the pin
state and the regulator). I think this will fix both your flicker
concerns whilst permitting the legitimate cases for a floating pin.

Something like:

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index e48fded3e414..8d8959a70e44 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -409,6 +409,33 @@ static bool pwm_backlight_is_linear(struct platform_pwm_backlight_data *data)
 static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
 {
 	struct device_node *node = pb->dev->of_node;
+	bool active = true;
+
+	/*
+	 * If the enable GPIO is present, observable (either as input
+	 * or output) and off then the backlight is not currently active.
+	 * */
+	if (pb->enable_gpio && gpiod_get_value_cansleep(pb->enable_gpio) == 0)
+		active = false;
+
+	if (!regulator_is_enabled(pb->power_supply))
+		active = false;
+
+	if (!pwm_is_enabled(pb->pwm))
+		active = false;
+
+	/*
+	 * Synchronize the enable_gpio with the observed state of the
+	 * hardware.
+	 */
+	if (pb->enable_gpio)
+		gpiod_direction_output(pb->enable_gpio, active);
+
+	/*
+	 * Do not change pb->enabled here! pb->enabled essentially
+	 * tells us if we own one of the regulator's use counts and
+	 * right now we do not.
+	 */
 
 	/* Not booted with device tree or no phandle link to the node */
 	if (!node || !node->phandle)
@@ -420,20 +447,7 @@ static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
 	 * assume that another driver will enable the backlight at the
 	 * appropriate time. Therefore, if it is disabled, keep it so.
 	 */
-
-	/* if the enable GPIO is disabled, do not enable the backlight */
-	if (pb->enable_gpio && gpiod_get_value_cansleep(pb->enable_gpio) == 0)
-		return FB_BLANK_POWERDOWN;
-
-	/* The regulator is disabled, do not enable the backlight */
-	if (!regulator_is_enabled(pb->power_supply))
-		return FB_BLANK_POWERDOWN;
-
-	/* The PWM is disabled, keep it like this */
-	if (!pwm_is_enabled(pb->pwm))
-		return FB_BLANK_POWERDOWN;
-
-	return FB_BLANK_UNBLANK;
+	return active ? FB_BLANK_UNBLANK: FB_BLANK_POWERDOWN;
 }
 
 static int pwm_backlight_probe(struct platform_device *pdev)
@@ -486,18 +500,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
-	/*
-	 * If the GPIO is not known to be already configured as output, that
-	 * is, if gpiod_get_direction returns either 1 or -EINVAL, change the
-	 * direction to output and set the GPIO as active.
-	 * Do not force the GPIO to active when it was already output as it
-	 * could cause backlight flickering or we would enable the backlight too
-	 * early. Leave the decision of the initial backlight state for later.
-	 */
-	if (pb->enable_gpio &&
-	    gpiod_get_direction(pb->enable_gpio) != 0)
-		gpiod_direction_output(pb->enable_gpio, 1);
-
 	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
 	if (IS_ERR(pb->power_supply)) {
 		ret = PTR_ERR(pb->power_supply);