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

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

[Peter Ujfalusi]

Hi,

Changes since v1:
- Handling of the enable GPIO is reworked:
 - Only change direction to output when the pin was input and in this case set
   the GPIO line physical low
 - With this change we can ensure that the enable GPIO is output so we do not
   need to check the direction of it later on.

Cover letter:

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.

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 | 58 +++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 19 deletions(-)

--
2.10.1

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

[Peter Ujfalusi]

Hi,

Changes since v1:
- Handling of the enable GPIO is reworked:
 - Only change direction to output when the pin was input and in this case set
   the GPIO line physical low
 - With this change we can ensure that the enable GPIO is output so we do not
   need to check the direction of it later on.

Cover letter:

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.

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 | 58 +++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 19 deletions(-)

--
2.10.1

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

[Peter Ujfalusi]

Hi,

Changes since v1:
- Handling of the enable GPIO is reworked:
 - Only change direction to output when the pin was input and in this case set
   the GPIO line physical low
 - With this change we can ensure that the enable GPIO is output so we do not
   need to check the direction of it later on.

Cover letter:

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.

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 | 58 +++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 19 deletions(-)

--
2.10.1

[PATCH v2 1/2] backlight: pwm_bl: Move the checks for initial power state to a separate function

[Peter Ujfalusi]

Move the checks to select the initial state for the backlight to a new
function and document the checks we are doing.

At the same time correct the handling of the case when the GPIO is
initially configured as input: we need to change the direction of the GPIO
to output and set the GPIO to low as the pin was not driven when the GPIO
was configured as input.

With the separate function 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 <peter.ujfalusi@ti.com>
---
 drivers/video/backlight/pwm_bl.c | 54 ++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 12614006211e..74083d880144 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -192,6 +192,32 @@ 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 disabled, do not enable the backlight */
+	if (pb->enable_gpio && gpiod_get_value(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;
+
+	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 +226,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,20 +292,14 @@ 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);
-	}
+	/*
+	 * If the GPIO is configured as input, change the direction to output
+	 * and set the GPIO as low since the line was not driven when it was
+	 * configured as input.
+	 */
+	if (pb->enable_gpio &&
+	    gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN)
+		gpiod_direction_output_raw(pb->enable_gpio, 0);
 
 	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
 	if (IS_ERR(pb->power_supply)) {
@@ -288,9 +307,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		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 +363,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.1

[PATCH v2 2/2] backlight: pwm_bl: Check the pwm state for initial backlight power state

[Peter Ujfalusi]

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 <peter.ujfalusi@ti.com>
---
 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 74083d880144..aeedf2d7bbb2 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -215,6 +215,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.1

Re: [PATCH v2 1/2] backlight: pwm_bl: Move the checks for initial power state to a separate function

[Philipp Zabel]

Hi Peter,

Am Donnerstag, den 27.10.2016, 13:01 +0300 schrieb Peter Ujfalusi:
> Move the checks to select the initial state for the backlight to a new
> function and document the checks we are doing.
>
> At the same time correct the handling of the case when the GPIO is
> initially configured as input: we need to change the direction of the GPIO
> to output and set the GPIO to low as the pin was not driven when the GPIO
> was configured as input.

I think this part should be a separate patch. At least it is a
significant enough change to warrant a mention in the subject.

And I'm not entirely sure this is better than before. If there happens
to be a pull-up on the GPIO (for whatever reason), we might introduce
flicker again if the GPIO goes from input (high) to output (low), and
back to output (high) again due to FB_BLANK_UNBLANK.

regards
Philipp

> With the separate function 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 <peter.ujfalusi@ti.com>
> ---
>  drivers/video/backlight/pwm_bl.c | 54 ++++++++++++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 12614006211e..74083d880144 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -192,6 +192,32 @@ 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 disabled, do not enable the backlight */

nitpick: s/if/If/ for consistency.

> +	if (pb->enable_gpio && gpiod_get_value(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;
> +
> +	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 +226,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,20 +292,14 @@ 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);
> -	}
> +	/*
> +	 * If the GPIO is configured as input, change the direction to output
> +	 * and set the GPIO as low since the line was not driven when it was
> +	 * configured as input.
> +	 */
> +	if (pb->enable_gpio &&
> +	    gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN)
> +		gpiod_direction_output_raw(pb->enable_gpio, 0);
>  
>  	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
>  	if (IS_ERR(pb->power_supply)) {
> @@ -288,9 +307,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  		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 +363,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);

Re: [PATCH v2 1/2] backlight: pwm_bl: Move the checks for initial power state to a separate function

[Philipp Zabel]

Hi Peter,

Am Donnerstag, den 27.10.2016, 13:01 +0300 schrieb Peter Ujfalusi:
> Move the checks to select the initial state for the backlight to a new
> function and document the checks we are doing.
>
> At the same time correct the handling of the case when the GPIO is
> initially configured as input: we need to change the direction of the GPIO
> to output and set the GPIO to low as the pin was not driven when the GPIO
> was configured as input.

I think this part should be a separate patch. At least it is a
significant enough change to warrant a mention in the subject.

And I'm not entirely sure this is better than before. If there happens
to be a pull-up on the GPIO (for whatever reason), we might introduce
flicker again if the GPIO goes from input (high) to output (low), and
back to output (high) again due to FB_BLANK_UNBLANK.

regards
Philipp

> With the separate function 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 <peter.ujfalusi@ti.com>
> ---
>  drivers/video/backlight/pwm_bl.c | 54 ++++++++++++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 12614006211e..74083d880144 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -192,6 +192,32 @@ 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 disabled, do not enable the backlight */

nitpick: s/if/If/ for consistency.

> +	if (pb->enable_gpio && gpiod_get_value(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;
> +
> +	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 +226,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,20 +292,14 @@ 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);
> -	}
> +	/*
> +	 * If the GPIO is configured as input, change the direction to output
> +	 * and set the GPIO as low since the line was not driven when it was
> +	 * configured as input.
> +	 */
> +	if (pb->enable_gpio &&
> +	    gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_IN)
> +		gpiod_direction_output_raw(pb->enable_gpio, 0);
>  
>  	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
>  	if (IS_ERR(pb->power_supply)) {
> @@ -288,9 +307,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  		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 +363,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);

Re: [PATCH v2 1/2] backlight: pwm_bl: Move the checks for initial power state to a separate function

[Peter Ujfalusi]

On 10/28/2016 04:27 PM, Philipp Zabel wrote:
> Hi Peter,
> 
> Am Donnerstag, den 27.10.2016, 13:01 +0300 schrieb Peter Ujfalusi:
>> Move the checks to select the initial state for the backlight to a new
>> function and document the checks we are doing.
>>
>> At the same time correct the handling of the case when the GPIO is
>> initially configured as input: we need to change the direction of the GPIO
>> to output and set the GPIO to low as the pin was not driven when the GPIO
>> was configured as input.
> 
> I think this part should be a separate patch. At least it is a
> significant enough change to warrant a mention in the subject.

Yes, I should have separated it it.

> And I'm not entirely sure this is better than before. If there happens
> to be a pull-up on the GPIO (for whatever reason), we might introduce
> flicker again if the GPIO goes from input (high) to output (low), and
> back to output (high) again due to FB_BLANK_UNBLANK.

This is plausible if the pull-up is strong enough. It might be better to
not change this logic at all and instead of gpiod_direction_output_raw()
use the gpiod_direction_output(pb->enable_gpio, 1); to enable the GPIO
when it is set as input.

I'll resend the patches.

-- 
Péter

> regards
> Philipp
> 
>> With the separate function 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 <peter.ujfalusi@ti.com>
>> ---
>>  drivers/video/backlight/pwm_bl.c | 54 ++++++++++++++++++++++++++--------------
>>  1 file changed, 35 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>> index 12614006211e..74083d880144 100644
>> --- a/drivers/video/backlight/pwm_bl.c
>> +++ b/drivers/video/backlight/pwm_bl.c
>> @@ -192,6 +192,32 @@ 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 disabled, do not enable the backlight */
> 
> nitpick: s/if/If/ for consistency.
> 
>> +	if (pb->enable_gpio && gpiod_get_value(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;
>> +
>> +	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 +226,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,20 +292,14 @@ 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);
>> -	}
>> +	/*
>> +	 * If the GPIO is configured as input, change the direction to output
>> +	 * and set the GPIO as low since the line was not driven when it was
>> +	 * configured as input.
>> +	 */
>> +	if (pb->enable_gpio &&
>> +	    gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN)
>> +		gpiod_direction_output_raw(pb->enable_gpio, 0);
>>  
>>  	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
>>  	if (IS_ERR(pb->power_supply)) {
>> @@ -288,9 +307,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>>  		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 +363,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);
> 
> 

Re: [PATCH v2 1/2] backlight: pwm_bl: Move the checks for initial power state to a separate function

[Peter Ujfalusi]

On 10/28/2016 04:27 PM, Philipp Zabel wrote:
> Hi Peter,
> 
> Am Donnerstag, den 27.10.2016, 13:01 +0300 schrieb Peter Ujfalusi:
>> Move the checks to select the initial state for the backlight to a new
>> function and document the checks we are doing.
>>
>> At the same time correct the handling of the case when the GPIO is
>> initially configured as input: we need to change the direction of the GPIO
>> to output and set the GPIO to low as the pin was not driven when the GPIO
>> was configured as input.
> 
> I think this part should be a separate patch. At least it is a
> significant enough change to warrant a mention in the subject.

Yes, I should have separated it it.

> And I'm not entirely sure this is better than before. If there happens
> to be a pull-up on the GPIO (for whatever reason), we might introduce
> flicker again if the GPIO goes from input (high) to output (low), and
> back to output (high) again due to FB_BLANK_UNBLANK.

This is plausible if the pull-up is strong enough. It might be better to
not change this logic at all and instead of gpiod_direction_output_raw()
use the gpiod_direction_output(pb->enable_gpio, 1); to enable the GPIO
when it is set as input.

I'll resend the patches.

-- 
Péter

> regards
> Philipp
> 
>> With the separate function 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 <peter.ujfalusi@ti.com>
>> ---
>>  drivers/video/backlight/pwm_bl.c | 54 ++++++++++++++++++++++++++--------------
>>  1 file changed, 35 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>> index 12614006211e..74083d880144 100644
>> --- a/drivers/video/backlight/pwm_bl.c
>> +++ b/drivers/video/backlight/pwm_bl.c
>> @@ -192,6 +192,32 @@ 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 disabled, do not enable the backlight */
> 
> nitpick: s/if/If/ for consistency.
> 
>> +	if (pb->enable_gpio && gpiod_get_value(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;
>> +
>> +	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 +226,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,20 +292,14 @@ 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);
>> -	}
>> +	/*
>> +	 * If the GPIO is configured as input, change the direction to output
>> +	 * and set the GPIO as low since the line was not driven when it was
>> +	 * configured as input.
>> +	 */
>> +	if (pb->enable_gpio &&
>> +	    gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_IN)
>> +		gpiod_direction_output_raw(pb->enable_gpio, 0);
>>  
>>  	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
>>  	if (IS_ERR(pb->power_supply)) {
>> @@ -288,9 +307,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>>  		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 +363,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);
> 
> 

Re: [PATCH v2 1/2] backlight: pwm_bl: Move the checks for initial power state to a separate function

[Peter Ujfalusi]

On 10/28/2016 04:27 PM, Philipp Zabel wrote:
> Hi Peter,
> 
> Am Donnerstag, den 27.10.2016, 13:01 +0300 schrieb Peter Ujfalusi:
>> Move the checks to select the initial state for the backlight to a new
>> function and document the checks we are doing.
>>
>> At the same time correct the handling of the case when the GPIO is
>> initially configured as input: we need to change the direction of the GPIO
>> to output and set the GPIO to low as the pin was not driven when the GPIO
>> was configured as input.
> 
> I think this part should be a separate patch. At least it is a
> significant enough change to warrant a mention in the subject.

Yes, I should have separated it it.

> And I'm not entirely sure this is better than before. If there happens
> to be a pull-up on the GPIO (for whatever reason), we might introduce
> flicker again if the GPIO goes from input (high) to output (low), and
> back to output (high) again due to FB_BLANK_UNBLANK.

This is plausible if the pull-up is strong enough. It might be better to
not change this logic at all and instead of gpiod_direction_output_raw()
use the gpiod_direction_output(pb->enable_gpio, 1); to enable the GPIO
when it is set as input.

I'll resend the patches.

-- 
Péter

> regards
> Philipp
> 
>> With the separate function 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 <peter.ujfalusi@ti.com>
>> ---
>>  drivers/video/backlight/pwm_bl.c | 54 ++++++++++++++++++++++++++--------------
>>  1 file changed, 35 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>> index 12614006211e..74083d880144 100644
>> --- a/drivers/video/backlight/pwm_bl.c
>> +++ b/drivers/video/backlight/pwm_bl.c
>> @@ -192,6 +192,32 @@ 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 disabled, do not enable the backlight */
> 
> nitpick: s/if/If/ for consistency.
> 
>> +	if (pb->enable_gpio && gpiod_get_value(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;
>> +
>> +	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 +226,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,20 +292,14 @@ 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);
>> -	}
>> +	/*
>> +	 * If the GPIO is configured as input, change the direction to output
>> +	 * and set the GPIO as low since the line was not driven when it was
>> +	 * configured as input.
>> +	 */
>> +	if (pb->enable_gpio &&
>> +	    gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_IN)
>> +		gpiod_direction_output_raw(pb->enable_gpio, 0);
>>  
>>  	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
>>  	if (IS_ERR(pb->power_supply)) {
>> @@ -288,9 +307,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>>  		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 +363,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);
> 
>