[PATCH 0/2] backlight: pwm_bl: Fix the initial power state selection

Hi,

3698d7e7d221 backlight: pwm_bl: Avoid backlight flicker when probed from DT

added support for avoiding backlight flickering, which in essence was designed
to not enable the baclkight when the driver loads, but let the user of the
backlight to enable it later on.

There are boards (like am437x-gp-evm) where we do not have valid GPIO to enable
the backlight (TPS61081DRC's EN pin is connected to V3_3D) and the regulator
is always on (VBAT in case of the gp-evm). In this board the logic to check the
GPIO state and the regulator is failing and the backlight will be enabled as
soon as the pwm_bl driver is loaded.

By extending the check to look at the PWM state this issue can be avoided and
the backlight will be enabled only when it's user is asking it to be enabled.

Note:
I don't know the history of 3698d7e7d221, but I would have opted to add a DT
bool property (default-off) indicating that the given backlight device must stay
off when the driver is probing, other driver will enable it later.
This way all pwm driver could use the same property (gpio-backlight would need
to be converted) and the backward compatibility would have been retained.

Regards,
Peter
---
Peter Ujfalusi (2):
  backlight: pwm_bl: Move the checks for initial power state to a
    separate function
  backlight: pwm_bl: Check the pwm state for initial backlight power
    state

 drivers/video/backlight/pwm_bl.c | 59 ++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 20 deletions(-)

--
2.10.0

If the pwm is not enabled the backlight initially should not be enabled
either if we have booted with DT and there is a phandle pointing to the
backlight node.

The patch extends the checks to decide if we should keep the backlight off
initially.

Signed-off-by: Peter Ujfalusi <redacted>
---
 drivers/video/backlight/pwm_bl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 9bc4715bf116..e4326bbf807f 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c

@@ -223,6 +223,10 @@ static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
 	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;
 }
 

-- 
2.10.0

On Fri, 30 Sep 2016, Peter Ujfalusi wrote:

If the pwm is not enabled the backlight initially should not be enabled
either if we have booted with DT and there is a phandle pointing to the
backlight node.

The patch extends the checks to decide if we should keep the backlight off
initially.

Signed-off-by: Peter Ujfalusi <redacted>
---
 drivers/video/backlight/pwm_bl.c | 4 ++++
 1 file changed, 4 insertions(+)

Makes sense to me.

I'm going to apply this until someone screams at me.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Move the check for gpio and regulator initial state to a new function and
document better what we are checking and why.

It is going to be easier to fix or improve the initial power state
configuration later and it is easier to read the code.

Signed-off-by: Peter Ujfalusi <redacted>
---
 drivers/video/backlight/pwm_bl.c | 55 +++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index b2b366bb0f97..9bc4715bf116 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c

@@ -192,6 +192,40 @@ static int pwm_backlight_parse_dt(struct device *dev,
 }
 #endif
 
+static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
+{
+	struct device_node *node = pb->dev->of_node;
+
+	/* Not booted with device tree or no phandle link to the node */
+	if (!node || !node->phandle)
+		return FB_BLANK_UNBLANK;
+
+	/*
+	 * If the driver is probed from the device tree and there is a
+	 * phandle link pointing to the backlight node, it is safe to
+	 * 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 set as output and it is disabled, do not enable
+	 * the backlight
+	 */
+	if (pb->enable_gpio) {
+		if (gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT &&
+		    gpiod_get_value(pb->enable_gpio) = 0)
+			return FB_BLANK_POWERDOWN;
+
+		gpiod_direction_output(pb->enable_gpio, 1);
+	}
+
+	/* The regulator is disabled, do not enable the backlight */
+	if (!regulator_is_enabled(pb->power_supply))
+		return FB_BLANK_POWERDOWN;
+
+	return FB_BLANK_UNBLANK;
+}
+
 static int pwm_backlight_probe(struct platform_device *pdev)
 {
 	struct platform_pwm_backlight_data *data = dev_get_platdata(&pdev->dev);

@@ -200,7 +234,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	struct backlight_device *bl;
 	struct device_node *node = pdev->dev.of_node;
 	struct pwm_bl_data *pb;
-	int initial_blank = FB_BLANK_UNBLANK;
 	struct pwm_args pargs;
 	int ret;
 

@@ -267,30 +300,12 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		pb->enable_gpio = gpio_to_desc(data->enable_gpio);
 	}
 
-	if (pb->enable_gpio) {
-		/*
-		 * If the driver is probed from the device tree and there is a
-		 * phandle link pointing to the backlight node, it is safe to
-		 * assume that another driver will enable the backlight at the
-		 * appropriate time. Therefore, if it is disabled, keep it so.
-		 */
-		if (node && node->phandle &&
-		    gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT &&
-		    gpiod_get_value(pb->enable_gpio) = 0)
-			initial_blank = FB_BLANK_POWERDOWN;
-		else
-			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);
 		goto err_alloc;
 	}
 
-	if (node && node->phandle && !regulator_is_enabled(pb->power_supply))
-		initial_blank = FB_BLANK_POWERDOWN;
-
 	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
 	if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER && !node) {
 		dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");

@@ -347,7 +362,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	}
 
 	bl->props.brightness = data->dft_brightness;
-	bl->props.power = initial_blank;
+	bl->props.power = pwm_backlight_initial_power_state(pb);
 	backlight_update_status(bl);
 
 	platform_set_drvdata(pdev, bl);

-- 
2.10.0

On Fri, 30 Sep 2016, Peter Ujfalusi wrote:

This changes the semantics of the current implementation.

In the code you're removing, if the enable_gpio is present and no DT
is found, we set the GPIO direction to output.  In your implementation,
if no DT is found you don't do anything regardless.

Is that intentional?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On 10/26/16 15:12, Lee Jones wrote:

On Fri, 30 Sep 2016, Peter Ujfalusi wrote:

quoted

Move the check for gpio and regulator initial state to a new function and
document better what we are checking and why.

It is going to be easier to fix or improve the initial power state
configuration later and it is easier to read the code.

Signed-off-by: Peter Ujfalusi <redacted>
---
 drivers/video/backlight/pwm_bl.c | 55 +++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index b2b366bb0f97..9bc4715bf116 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c

@@ -192,6 +192,40 @@ static int pwm_backlight_parse_dt(struct device *dev,
 }
 #endif
 
+static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
+{
+	struct device_node *node = pb->dev->of_node;
+
+	/* Not booted with device tree or no phandle link to the node */
+	if (!node || !node->phandle)
+		return FB_BLANK_UNBLANK;

This changes the semantics of the current implementation.

In the code you're removing, if the enable_gpio is present and no DT
is found, we set the GPIO direction to output.  In your implementation,
if no DT is found you don't do anything regardless.

Is that intentional?

Argh, no, it is not intentional.

It should have been like this:

	/* Not booted with device tree or no phandle link to the node */
	if (!node || !node->phandle) {
		/* Set enable GPIO if it is valid */
		if (pb->enable_gpio)
			gpiod_direction_output(pb->enable_gpio, 1);

		return FB_BLANK_UNBLANK;
	}

On the other hand we are not doing the same for the regulator in case of non
DT boot or when the backlight has no users. So we do enable the GPIO, but the
regulator might be left disabled...

I'll resend the series with this change.

quoted

+	/*
+	 * If the driver is probed from the device tree and there is a
+	 * phandle link pointing to the backlight node, it is safe to
+	 * 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 set as output and it is disabled, do not enable
+	 * the backlight
+	 */
+	if (pb->enable_gpio) {
+		if (gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT &&
+		    gpiod_get_value(pb->enable_gpio) = 0)
+			return FB_BLANK_POWERDOWN;
+
+		gpiod_direction_output(pb->enable_gpio, 1);
+	}
+
+	/* The regulator is disabled, do not enable the backlight */
+	if (!regulator_is_enabled(pb->power_supply))
+		return FB_BLANK_POWERDOWN;
+
+	return FB_BLANK_UNBLANK;
+}
+
 static int pwm_backlight_probe(struct platform_device *pdev)
 {
 	struct platform_pwm_backlight_data *data = dev_get_platdata(&pdev->dev);

@@ -200,7 +234,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	struct backlight_device *bl;
 	struct device_node *node = pdev->dev.of_node;
 	struct pwm_bl_data *pb;
-	int initial_blank = FB_BLANK_UNBLANK;
 	struct pwm_args pargs;
 	int ret;
 

@@ -267,30 +300,12 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		pb->enable_gpio = gpio_to_desc(data->enable_gpio);
 	}
 
-	if (pb->enable_gpio) {
-		/*
-		 * If the driver is probed from the device tree and there is a
-		 * phandle link pointing to the backlight node, it is safe to
-		 * assume that another driver will enable the backlight at the
-		 * appropriate time. Therefore, if it is disabled, keep it so.
-		 */
-		if (node && node->phandle &&
-		    gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT &&
-		    gpiod_get_value(pb->enable_gpio) = 0)
-			initial_blank = FB_BLANK_POWERDOWN;
-		else
-			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);
 		goto err_alloc;
 	}
 
-	if (node && node->phandle && !regulator_is_enabled(pb->power_supply))
-		initial_blank = FB_BLANK_POWERDOWN;
-
 	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
 	if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER && !node) {
 		dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");

@@ -347,7 +362,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	}
 
 	bl->props.brightness = data->dft_brightness;
-	bl->props.power = initial_blank;
+	bl->props.power = pwm_backlight_initial_power_state(pb);
 	backlight_update_status(bl);
 
 	platform_set_drvdata(pdev, bl);

-- 
Péter

On 10/26/16 16:02, Peter Ujfalusi wrote:

On 10/26/16 15:12, Lee Jones wrote:

quoted

On Fri, 30 Sep 2016, Peter Ujfalusi wrote:

quoted

Move the check for gpio and regulator initial state to a new function and
document better what we are checking and why.

It is going to be easier to fix or improve the initial power state
configuration later and it is easier to read the code.

Signed-off-by: Peter Ujfalusi <redacted>
---
 drivers/video/backlight/pwm_bl.c | 55 +++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index b2b366bb0f97..9bc4715bf116 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c

@@ -192,6 +192,40 @@ static int pwm_backlight_parse_dt(struct device *dev,
 }
 #endif
 
+static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
+{
+	struct device_node *node = pb->dev->of_node;
+
+	/* Not booted with device tree or no phandle link to the node */
+	if (!node || !node->phandle)
+		return FB_BLANK_UNBLANK;

This changes the semantics of the current implementation.

In the code you're removing, if the enable_gpio is present and no DT
is found, we set the GPIO direction to output.  In your implementation,
if no DT is found you don't do anything regardless.

Is that intentional?

Argh, no, it is not intentional.

It should have been like this:

	/* Not booted with device tree or no phandle link to the node */
	if (!node || !node->phandle) {
		/* Set enable GPIO if it is valid */
		if (pb->enable_gpio)
			gpiod_direction_output(pb->enable_gpio, 1);

		return FB_BLANK_UNBLANK;
	}

On the other hand we are not doing the same for the regulator in case of non
DT boot or when the backlight has no users. So we do enable the GPIO, but the
regulator might be left disabled...

I'll resend the series with this change.

This is not really correct either.

If we boot in legacy mode the data->enable_gpio number is used to get the GPIO
and we configure the initial state as GPIOF_OUT_INIT_HIGH. So legacy boot will
have the line high in any case.

I think the only case we should handle differently is when the GPIO is
configured as input (I don't think it happens, but just to be sure). In that
case we need to change the direction to output and set the GPIO as disabled.
If it was input, it was not driving the line.

In the pwm_backlight_initial_power_state() function we will not need to check
for GPIO direction as it is guaranteed that it is output, we can only check
for the value of the GPIO.

I will resubmit with this change.

quoted

quoted

+	/*
+	 * If the driver is probed from the device tree and there is a
+	 * phandle link pointing to the backlight node, it is safe to
+	 * 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 set as output and it is disabled, do not enable
+	 * the backlight
+	 */
+	if (pb->enable_gpio) {
+		if (gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT &&
+		    gpiod_get_value(pb->enable_gpio) = 0)
+			return FB_BLANK_POWERDOWN;
+
+		gpiod_direction_output(pb->enable_gpio, 1);
+	}
+
+	/* The regulator is disabled, do not enable the backlight */
+	if (!regulator_is_enabled(pb->power_supply))
+		return FB_BLANK_POWERDOWN;
+
+	return FB_BLANK_UNBLANK;
+}
+
 static int pwm_backlight_probe(struct platform_device *pdev)
 {
 	struct platform_pwm_backlight_data *data = dev_get_platdata(&pdev->dev);

@@ -200,7 +234,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	struct backlight_device *bl;
 	struct device_node *node = pdev->dev.of_node;
 	struct pwm_bl_data *pb;
-	int initial_blank = FB_BLANK_UNBLANK;
 	struct pwm_args pargs;
 	int ret;
 

@@ -267,30 +300,12 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		pb->enable_gpio = gpio_to_desc(data->enable_gpio);
 	}
 
-	if (pb->enable_gpio) {
-		/*
-		 * If the driver is probed from the device tree and there is a
-		 * phandle link pointing to the backlight node, it is safe to
-		 * assume that another driver will enable the backlight at the
-		 * appropriate time. Therefore, if it is disabled, keep it so.
-		 */
-		if (node && node->phandle &&
-		    gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT &&
-		    gpiod_get_value(pb->enable_gpio) = 0)
-			initial_blank = FB_BLANK_POWERDOWN;
-		else
-			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);
 		goto err_alloc;
 	}
 
-	if (node && node->phandle && !regulator_is_enabled(pb->power_supply))
-		initial_blank = FB_BLANK_POWERDOWN;
-
 	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
 	if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER && !node) {
 		dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");

@@ -347,7 +362,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	}
 
 	bl->props.brightness = data->dft_brightness;
-	bl->props.power = initial_blank;
+	bl->props.power = pwm_backlight_initial_power_state(pb);
 	backlight_update_status(bl);
 
 	platform_set_drvdata(pdev, bl);

-- 
Péter