[v1] leds: pca9532: don't stop blinking for non-zero brightness

[PATCH] leds: pca9532: don't stop blinking for non-zero brightness

Posted by Tobias Deiminger 1 week, 6 days ago

pca9532 unexpectedly stopped blinking when changing brightness to a
non-zero value. To reproduce:

 echo timer > /sys/class/leds/led-1/trigger   # blinks
 echo 255 > /sys/class/leds/led-1/brightness  # blinking stops, light on
 cat /sys/class/leds/led-1/trigger            # still claims [timer]

According to Documentation/leds/leds-class.rst, only brightness = 0
shall be a stop condition:

> You can change the brightness value of a LED independently of the
> timer trigger. However, if you set the brightness value to LED_OFF it
> will also disable the timer trigger.

Therefore add a guard to continue blinking when brightness != LED_OFF,
similar to how pca955x does it since 575f10dc64a2 ("leds: pca955x: Add
HW blink support").

Signed-off-by: Tobias Deiminger <tobias.deiminger@linutronix.de>
---

Notes:
    A more advanced solution was to not simply return early on
    set_brightness, but actually support blinking at arbitrary brightness.
    This would however require conditional fallback to SW blinking, since
    PCA 9532 doesn't support routing PWM 0 (dim) and PWM 1 (blink) in
    series. The bugfix here keeps it simple. Optional SW blinking could be
    tried in a separate patch.

 drivers/leds/leds-pca9532.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index 0344189bb991..3de20e087334 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -184,6 +184,8 @@ static int pca9532_set_brightness(struct led_classdev *led_cdev,
 
 	if (value == LED_OFF)
 		led->state = PCA9532_OFF;
+	else if (led->state == PCA9532_PWM1)
+		return 0; /* non-zero brightness shall not stop HW blinking */
 	else if (value == LED_FULL)
 		led->state = PCA9532_ON;
 	else {

base-commit: b2c87f5e98cd88095dbc6802197526703d5e4e48
-- 
2.47.3

Re: [PATCH] leds: pca9532: don't stop blinking for non-zero brightness

Posted by Lee Jones 3 days, 14 hours ago

On Sat, 21 Mar 2026, Tobias Deiminger wrote:

> pca9532 unexpectedly stopped blinking when changing brightness to a
> non-zero value. To reproduce:
> 
>  echo timer > /sys/class/leds/led-1/trigger   # blinks
>  echo 255 > /sys/class/leds/led-1/brightness  # blinking stops, light on
>  cat /sys/class/leds/led-1/trigger            # still claims [timer]
> 
> According to Documentation/leds/leds-class.rst, only brightness = 0
> shall be a stop condition:
> 
> > You can change the brightness value of a LED independently of the
> > timer trigger. However, if you set the brightness value to LED_OFF it
> > will also disable the timer trigger.
> 
> Therefore add a guard to continue blinking when brightness != LED_OFF,
> similar to how pca955x does it since 575f10dc64a2 ("leds: pca955x: Add
> HW blink support").
> 
> Signed-off-by: Tobias Deiminger <tobias.deiminger@linutronix.de>
> ---
> 
> Notes:
>     A more advanced solution was to not simply return early on
>     set_brightness, but actually support blinking at arbitrary brightness.
>     This would however require conditional fallback to SW blinking, since
>     PCA 9532 doesn't support routing PWM 0 (dim) and PWM 1 (blink) in
>     series. The bugfix here keeps it simple. Optional SW blinking could be
>     tried in a separate patch.
> 
>  drivers/leds/leds-pca9532.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> index 0344189bb991..3de20e087334 100644
> --- a/drivers/leds/leds-pca9532.c
> +++ b/drivers/leds/leds-pca9532.c
> @@ -184,6 +184,8 @@ static int pca9532_set_brightness(struct led_classdev *led_cdev,
>  
>  	if (value == LED_OFF)
>  		led->state = PCA9532_OFF;
> +	else if (led->state == PCA9532_PWM1)
> +		return 0; /* non-zero brightness shall not stop HW blinking */

Comments should start with a capital letter.

Also, as the final 'else' statement uses braces, should we perhaps take the
opportunity to add braces to all branches of this conditional block?

>  	else if (value == LED_FULL)
>  		led->state = PCA9532_ON;
>  	else {
> 
> base-commit: b2c87f5e98cd88095dbc6802197526703d5e4e48
> -- 
> 2.47.3
> 
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH] leds: pca9532: don't stop blinking for non-zero brightness

Posted by Tobias Deiminger 3 days, 3 hours ago

Am Dienstag, 31. März 2026, 11:47:30 Mitteleuropäische Sommerzeit schrieb Lee Jones:
> On Sat, 21 Mar 2026, Tobias Deiminger wrote:
> [...]
> > --- a/drivers/leds/leds-pca9532.c
> > +++ b/drivers/leds/leds-pca9532.c
> > @@ -184,6 +184,8 @@ static int pca9532_set_brightness(struct led_classdev *led_cdev,
> >  
> >  	if (value == LED_OFF)
> >  		led->state = PCA9532_OFF;
> > +	else if (led->state == PCA9532_PWM1)
> > +		return 0; /* non-zero brightness shall not stop HW blinking */
> 
> Comments should start with a capital letter.
> 
> Also, as the final 'else' statement uses braces, should we perhaps take the
> opportunity to add braces to all branches of this conditional block?

Thanks for taking a look, sounds reasonable. Will adjust the comment and K&R-ify the
hunk in v2.

- Tobias

> >  	else if (value == LED_FULL)
> >  		led->state = PCA9532_ON;
> >  	else {
> > 
> > base-commit: b2c87f5e98cd88095dbc6802197526703d5e4e48
> 
>