[PATCH] backlight: led_bl: fix initial power state

[PATCH] backlight: led_bl: fix initial power state

[Mans Rullgard]

The condition for the initial power state based on the default
brightness value is reversed.  Fix it.

Furthermore, use the actual state of the LEDs rather than the default
brightness specified in the devicetree as the latter should not cause
the backlight to be automatically turned on.

If the backlight device is not linked to any display, set the initial
power to on unconditionally.

Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
Signed-off-by: Mans Rullgard <mans@mansr.com>
---
Changes in v3:
- Add comment

Changes in v2:
- Use the reported LED state to set initial power state
- Always power on if no phandle in DT
---
 drivers/video/backlight/led_bl.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index 3259292fda76..c94843c00a30 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -176,6 +176,7 @@ static int led_bl_probe(struct platform_device *pdev)
 {
 	struct backlight_properties props;
 	struct led_bl_data *priv;
+	int init_brightness;
 	int ret, i;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -190,6 +191,8 @@ static int led_bl_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	init_brightness = priv->default_brightness;
+
 	ret = led_bl_parse_levels(&pdev->dev, priv);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to parse DT data\n");
@@ -200,8 +203,11 @@ static int led_bl_probe(struct platform_device *pdev)
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = priv->max_brightness;
 	props.brightness = priv->default_brightness;
-	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
-		      FB_BLANK_UNBLANK;
+
+	/* Set power on if LEDs already on or not linked to a display. */
+	props.power = (init_brightness || !pdev->dev.of_node->phandle) ?
+		FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
+
 	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
 			&pdev->dev, priv, &led_bl_ops, &props);
 	if (IS_ERR(priv->bl_dev)) {
-- 
2.41.0

Re: [PATCH] backlight: led_bl: fix initial power state

[Daniel Thompson]

On Wed, Jul 05, 2023 at 03:24:14PM +0100, Mans Rullgard wrote:
> The condition for the initial power state based on the default
> brightness value is reversed.  Fix it.
>
> Furthermore, use the actual state of the LEDs rather than the default
> brightness specified in the devicetree as the latter should not cause
> the backlight to be automatically turned on.
>
> If the backlight device is not linked to any display, set the initial
> power to on unconditionally.
>
> Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
> Changes in v3:
> - Add comment

This mismatches the subject line ;-) but I can live with that if Lee
and Jingoo can!

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH] backlight: led_bl: fix initial power state

[Måns Rullgård]

Daniel Thompson <daniel.thompson@linaro.org> writes:

> On Wed, Jul 05, 2023 at 03:24:14PM +0100, Mans Rullgard wrote:
>> The condition for the initial power state based on the default
>> brightness value is reversed.  Fix it.
>>
>> Furthermore, use the actual state of the LEDs rather than the default
>> brightness specified in the devicetree as the latter should not cause
>> the backlight to be automatically turned on.
>>
>> If the backlight device is not linked to any display, set the initial
>> power to on unconditionally.
>>
>> Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> ---
>> Changes in v3:
>> - Add comment
>
> This mismatches the subject line ;-) but I can live with that if Lee
> and Jingoo can!

Does it not fix it?  If you think the subject is misleading, feel free
to change it.

-- 
Måns Rullgård

Re: [PATCH] backlight: led_bl: fix initial power state

[Daniel Thompson]

On Wed, Jul 05, 2023 at 03:36:46PM +0100, Måns Rullgård wrote:
> Daniel Thompson <daniel.thompson@linaro.org> writes:
>
> > On Wed, Jul 05, 2023 at 03:24:14PM +0100, Mans Rullgard wrote:
> >> The condition for the initial power state based on the default
> >> brightness value is reversed.  Fix it.
> >>
> >> Furthermore, use the actual state of the LEDs rather than the default
> >> brightness specified in the devicetree as the latter should not cause
> >> the backlight to be automatically turned on.
> >>
> >> If the backlight device is not linked to any display, set the initial
> >> power to on unconditionally.
> >>
> >> Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
> >> ---
> >> Changes in v3:
> >> - Add comment
> >
> > This mismatches the subject line ;-) but I can live with that if Lee
> > and Jingoo can!
>
> Does it not fix it?  If you think the subject is misleading, feel free
> to change it.

The bit that goes into version control is fine!

However without '[PATCH v3]' on the subject line for the initial patch
there is a risk this thread will get overlooked and not queued[1].


Daniel.


[1] Just to be clear, Lee J. typically hoovers up the backlight patches
    and sends the PR. I only queue backlight patches myself as holiday
    cover...

Re: [PATCH] backlight: led_bl: fix initial power state

[Måns Rullgård]

Daniel Thompson <daniel.thompson@linaro.org> writes:

> On Wed, Jul 05, 2023 at 03:36:46PM +0100, Måns Rullgård wrote:
>> Daniel Thompson <daniel.thompson@linaro.org> writes:
>>
>> > On Wed, Jul 05, 2023 at 03:24:14PM +0100, Mans Rullgard wrote:
>> >> The condition for the initial power state based on the default
>> >> brightness value is reversed.  Fix it.
>> >>
>> >> Furthermore, use the actual state of the LEDs rather than the default
>> >> brightness specified in the devicetree as the latter should not cause
>> >> the backlight to be automatically turned on.
>> >>
>> >> If the backlight device is not linked to any display, set the initial
>> >> power to on unconditionally.
>> >>
>> >> Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
>> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> >> ---
>> >> Changes in v3:
>> >> - Add comment
>> >
>> > This mismatches the subject line ;-) but I can live with that if Lee
>> > and Jingoo can!
>>
>> Does it not fix it?  If you think the subject is misleading, feel free
>> to change it.
>
> The bit that goes into version control is fine!
>
> However without '[PATCH v3]' on the subject line for the initial patch
> there is a risk this thread will get overlooked and not queued[1].

Oh, I see now I forgot to add the v3 tag.  Sorry about that.

-- 
Måns Rullgård

[PATCH] backlight: led_bl: fix initial power state

[Mans Rullgard]

The condition for the initial power state based on the default
brightness value is reversed.  Fix it.

Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
Signed-off-by: Mans Rullgard <mans@mansr.com>
---
 drivers/video/backlight/led_bl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
index 3259292fda76..28e83618a296 100644
--- a/drivers/video/backlight/led_bl.c
+++ b/drivers/video/backlight/led_bl.c
@@ -200,8 +200,8 @@ static int led_bl_probe(struct platform_device *pdev)
 	props.type = BACKLIGHT_RAW;
 	props.max_brightness = priv->max_brightness;
 	props.brightness = priv->default_brightness;
-	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
-		      FB_BLANK_UNBLANK;
+	props.power = (priv->default_brightness > 0) ? FB_BLANK_UNBLANK :
+		      FB_BLANK_POWERDOWN;
 	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
 			&pdev->dev, priv, &led_bl_ops, &props);
 	if (IS_ERR(priv->bl_dev)) {
-- 
2.41.0

Re: [PATCH] backlight: led_bl: fix initial power state

[Daniel Thompson]

On Tue, Jul 04, 2023 at 03:07:50PM +0100, Mans Rullgard wrote:
> The condition for the initial power state based on the default
> brightness value is reversed.  Fix it.
>
> Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>  drivers/video/backlight/led_bl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> index 3259292fda76..28e83618a296 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -200,8 +200,8 @@ static int led_bl_probe(struct platform_device *pdev)
>  	props.type = BACKLIGHT_RAW;
>  	props.max_brightness = priv->max_brightness;
>  	props.brightness = priv->default_brightness;
> -	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
> -		      FB_BLANK_UNBLANK;
> +	props.power = (priv->default_brightness > 0) ? FB_BLANK_UNBLANK :
> +		      FB_BLANK_POWERDOWN;

The logic was wrong before but I think will still be wrong after the
change too (e.g. the bogus logic is probably avoiding backlight flicker
in some use cases).

The logic here needs to be similar to what pwm_bl.c implements in
pwm_backlight_initial_power_state(). Whilst it might be better
to implement this in led_bl_get_leds() let me show what I mean
in code that fits in the current line:

	/*
	 * Activate the backlight if the LEDs are already lit *or*
	 * there is no phandle link (meaning the backlight power
	 * state cannot be synced with the display state).
	 */
	props.power = (active_at_boot || !dev->node->phandle) ?
			FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;

Note that active_at_boot is not the same as (priv->default_brightness > 0)
since the value read by led_bl_get_leds() can be clobbered when we
parse the properties.


Daniel.

Re: [PATCH] backlight: led_bl: fix initial power state

[Måns Rullgård]

Daniel Thompson <daniel.thompson@linaro.org> writes:

> On Tue, Jul 04, 2023 at 03:07:50PM +0100, Mans Rullgard wrote:
>> The condition for the initial power state based on the default
>> brightness value is reversed.  Fix it.
>>
>> Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> ---
>>  drivers/video/backlight/led_bl.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
>> index 3259292fda76..28e83618a296 100644
>> --- a/drivers/video/backlight/led_bl.c
>> +++ b/drivers/video/backlight/led_bl.c
>> @@ -200,8 +200,8 @@ static int led_bl_probe(struct platform_device *pdev)
>>  	props.type = BACKLIGHT_RAW;
>>  	props.max_brightness = priv->max_brightness;
>>  	props.brightness = priv->default_brightness;
>> -	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
>> -		      FB_BLANK_UNBLANK;
>> +	props.power = (priv->default_brightness > 0) ? FB_BLANK_UNBLANK :
>> +		      FB_BLANK_POWERDOWN;
>
> The logic was wrong before but I think will still be wrong after the
> change too (e.g. the bogus logic is probably avoiding backlight flicker
> in some use cases).
>
> The logic here needs to be similar to what pwm_bl.c implements in
> pwm_backlight_initial_power_state(). Whilst it might be better
> to implement this in led_bl_get_leds() let me show what I mean
> in code that fits in the current line:
>
> 	/*
> 	 * Activate the backlight if the LEDs are already lit *or*
> 	 * there is no phandle link (meaning the backlight power
> 	 * state cannot be synced with the display state).
> 	 */
> 	props.power = (active_at_boot || !dev->node->phandle) ?
> 			FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
>
> Note that active_at_boot is not the same as (priv->default_brightness > 0)
> since the value read by led_bl_get_leds() can be clobbered when we
> parse the properties.

Am I understanding correctly that the code should be using the
default_brightness value as set by led_bl_get_leds() to determine the
initial power state, not whatever default value the devicetree provides?

-- 
Måns Rullgård

Re: [PATCH] backlight: led_bl: fix initial power state

[Daniel Thompson]

On Tue, Jul 04, 2023 at 04:31:51PM +0100, Måns Rullgård wrote:
> Daniel Thompson <daniel.thompson@linaro.org> writes:
>
> > On Tue, Jul 04, 2023 at 03:07:50PM +0100, Mans Rullgard wrote:
> >> The condition for the initial power state based on the default
> >> brightness value is reversed.  Fix it.
> >>
> >> Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
> >> ---
> >>  drivers/video/backlight/led_bl.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> >> index 3259292fda76..28e83618a296 100644
> >> --- a/drivers/video/backlight/led_bl.c
> >> +++ b/drivers/video/backlight/led_bl.c
> >> @@ -200,8 +200,8 @@ static int led_bl_probe(struct platform_device *pdev)
> >>  	props.type = BACKLIGHT_RAW;
> >>  	props.max_brightness = priv->max_brightness;
> >>  	props.brightness = priv->default_brightness;
> >> -	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
> >> -		      FB_BLANK_UNBLANK;
> >> +	props.power = (priv->default_brightness > 0) ? FB_BLANK_UNBLANK :
> >> +		      FB_BLANK_POWERDOWN;
> >
> > The logic was wrong before but I think will still be wrong after the
> > change too (e.g. the bogus logic is probably avoiding backlight flicker
> > in some use cases).
> >
> > The logic here needs to be similar to what pwm_bl.c implements in
> > pwm_backlight_initial_power_state(). Whilst it might be better
> > to implement this in led_bl_get_leds() let me show what I mean
> > in code that fits in the current line:
> >
> > 	/*
> > 	 * Activate the backlight if the LEDs are already lit *or*
> > 	 * there is no phandle link (meaning the backlight power
> > 	 * state cannot be synced with the display state).
> > 	 */
> > 	props.power = (active_at_boot || !dev->node->phandle) ?
> > 			FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
> >
> > Note that active_at_boot is not the same as (priv->default_brightness > 0)
> > since the value read by led_bl_get_leds() can be clobbered when we
> > parse the properties.
>
> Am I understanding correctly that the code should be using the
> default_brightness value as set by led_bl_get_leds() to determine the
> initial power state, not whatever default value the devicetree provides?

Yes.

The devicetree allows us to specify a default brightness but the DT
value cannot not be used to decide if the backlight has already been
lit up.


Daniel.

Re: [PATCH] backlight: led_bl: fix initial power state

[Sam Ravnborg]

Hi Daniel,

> > @@ -200,8 +200,8 @@ static int led_bl_probe(struct platform_device *pdev)
> >  	props.type = BACKLIGHT_RAW;
> >  	props.max_brightness = priv->max_brightness;
> >  	props.brightness = priv->default_brightness;
> > -	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
> > -		      FB_BLANK_UNBLANK;
> > +	props.power = (priv->default_brightness > 0) ? FB_BLANK_UNBLANK :
> > +		      FB_BLANK_POWERDOWN;
> 
> The logic was wrong before but I think will still be wrong after the
> change too (e.g. the bogus logic is probably avoiding backlight flicker
> in some use cases).
> 
> The logic here needs to be similar to what pwm_bl.c implements in
> pwm_backlight_initial_power_state(). Whilst it might be better
> to implement this in led_bl_get_leds() let me show what I mean
> in code that fits in the current line:
> 
> 	/*
> 	 * Activate the backlight if the LEDs are already lit *or*
> 	 * there is no phandle link (meaning the backlight power
> 	 * state cannot be synced with the display state).
> 	 */
> 	props.power = (active_at_boot || !dev->node->phandle) ?
> 			FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
> 
The following code does the same using helpers:

	if (active_at_boot || !dev->node->phandle))
		backlight_enable(bd);
	else
		backlight_disable(bd);

The code needs to execute after backlight_device_register() so maybe not
so great an idea?!?

	Sam

Re: [PATCH] backlight: led_bl: fix initial power state

[Daniel Thompson]

On Tue, Jul 04, 2023 at 07:07:31PM +0200, Sam Ravnborg wrote:
> Hi Daniel,
>
> > > @@ -200,8 +200,8 @@ static int led_bl_probe(struct platform_device *pdev)
> > >  	props.type = BACKLIGHT_RAW;
> > >  	props.max_brightness = priv->max_brightness;
> > >  	props.brightness = priv->default_brightness;
> > > -	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
> > > -		      FB_BLANK_UNBLANK;
> > > +	props.power = (priv->default_brightness > 0) ? FB_BLANK_UNBLANK :
> > > +		      FB_BLANK_POWERDOWN;
> >
> > The logic was wrong before but I think will still be wrong after the
> > change too (e.g. the bogus logic is probably avoiding backlight flicker
> > in some use cases).
> >
> > The logic here needs to be similar to what pwm_bl.c implements in
> > pwm_backlight_initial_power_state(). Whilst it might be better
> > to implement this in led_bl_get_leds() let me show what I mean
> > in code that fits in the current line:
> >
> > 	/*
> > 	 * Activate the backlight if the LEDs are already lit *or*
> > 	 * there is no phandle link (meaning the backlight power
> > 	 * state cannot be synced with the display state).
> > 	 */
> > 	props.power = (active_at_boot || !dev->node->phandle) ?
> > 			FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
> >
> The following code does the same using helpers:
>
> 	if (active_at_boot || !dev->node->phandle))
> 		backlight_enable(bd);
> 	else
> 		backlight_disable(bd);
>
> The code needs to execute after backlight_device_register() so maybe not
> so great an idea?!?

It would introduce a need for a bunch of new locks since userspace
could get in between device creation and the call to the helpers.

Additionally, it is even correct for a driver to forcefully set
fb_blank to powerdown during the probe? All current drivers set
fb_blank to unblank during the probe.


Daniel.

Re: [PATCH] backlight: led_bl: fix initial power state

[Sam Ravnborg]

Hi Daniel,

On Wed, Jul 05, 2023 at 03:07:31PM +0100, Daniel Thompson wrote:
> On Tue, Jul 04, 2023 at 07:07:31PM +0200, Sam Ravnborg wrote:
> > Hi Daniel,
> >
> > > > @@ -200,8 +200,8 @@ static int led_bl_probe(struct platform_device *pdev)
> > > >  	props.type = BACKLIGHT_RAW;
> > > >  	props.max_brightness = priv->max_brightness;
> > > >  	props.brightness = priv->default_brightness;
> > > > -	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
> > > > -		      FB_BLANK_UNBLANK;
> > > > +	props.power = (priv->default_brightness > 0) ? FB_BLANK_UNBLANK :
> > > > +		      FB_BLANK_POWERDOWN;
> > >
> > > The logic was wrong before but I think will still be wrong after the
> > > change too (e.g. the bogus logic is probably avoiding backlight flicker
> > > in some use cases).
> > >
> > > The logic here needs to be similar to what pwm_bl.c implements in
> > > pwm_backlight_initial_power_state(). Whilst it might be better
> > > to implement this in led_bl_get_leds() let me show what I mean
> > > in code that fits in the current line:
> > >
> > > 	/*
> > > 	 * Activate the backlight if the LEDs are already lit *or*
> > > 	 * there is no phandle link (meaning the backlight power
> > > 	 * state cannot be synced with the display state).
> > > 	 */
> > > 	props.power = (active_at_boot || !dev->node->phandle) ?
> > > 			FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
> > >
> > The following code does the same using helpers:
> >
> > 	if (active_at_boot || !dev->node->phandle))
> > 		backlight_enable(bd);
> > 	else
> > 		backlight_disable(bd);
> >
> > The code needs to execute after backlight_device_register() so maybe not
> > so great an idea?!?
> 
> It would introduce a need for a bunch of new locks since userspace
> could get in between device creation and the call to the helpers.
I thought we were safe while in the probe function, but I have been
fooled by the driver model before.

> 
> Additionally, it is even correct for a driver to forcefully set
> fb_blank to powerdown during the probe? All current drivers set
> fb_blank to unblank during the probe.
fb_blank is more or less unused. I thought that Lee applied the patch set
to eliminate most users, but I see that this is not the case.
I need to resend one day.
Some (at least one) drivers update .power after registering the device, so if this
is racy then these drivers could use some care.

Anyway, looking at how many drivers access backlight_properties direct is
is futile to try to avoid it. So the approach you suggest seems the best
way to do it.

	Sam