[PATCH RESEND v2] led: multicolor: Fix intensity setting while SW blinking

[PATCH RESEND v2] led: multicolor: Fix intensity setting while SW blinking

[Sven Schwermer]

From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

When writing to the multi_intensity file, don't unconditionally call
led_set_brightness. By only doing this if blinking is inactive we
prevent blinking from stopping if the blinking is in its off phase while
the file is written.

Instead, if blinking is active, the changed intensity values are applied
upon the next blink. This is consistent with changing the brightness on
monochrome LEDs with active blinking.

Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Tested-by: Sven Schuchmann <schuchmann@schleissheimer.de>
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
---

Notes:
    V1->V2: Change title, add tags

 drivers/leds/led-class-multicolor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
index e317408583df..5b1479b5d32c 100644
--- a/drivers/leds/led-class-multicolor.c
+++ b/drivers/leds/led-class-multicolor.c
@@ -59,7 +59,8 @@ static ssize_t multi_intensity_store(struct device *dev,
 	for (i = 0; i < mcled_cdev->num_colors; i++)
 		mcled_cdev->subled_info[i].intensity = intensity_value[i];
 
-	led_set_brightness(led_cdev, led_cdev->brightness);
+	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
+		led_set_brightness(led_cdev, led_cdev->brightness);
 	ret = size;
 err_out:
 	mutex_unlock(&led_cdev->led_access);

base-commit: 210e04ff768142b96452030c4c2627512b30ad95
-- 
2.36.1

AW: [PATCH RESEND v2] led: multicolor: Fix intensity setting while SW blinking

[Sven Schuchmann]

Hi,
can someone have a look at this patch.
I think it did not get accepted.
Any complains about this?

Regards,

   Sven

> -----Ursprüngliche Nachricht-----
> Von: Sven Schwermer <sven@svenschwermer.de>
> Gesendet: Montag, 27. Juni 2022 15:31
> An: linux-leds@vger.kernel.org
> Cc: Sven Schwermer <sven.schwermer@disruptive-technologies.com>;
> jacek.anaszewski@gmail.com; Sven Schuchmann <schuchmann@schleissheimer.de>; pavel@ucw.cz
> Betreff: [PATCH RESEND v2] led: multicolor: Fix intensity setting while SW blinking
> 
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> When writing to the multi_intensity file, don't unconditionally call
> led_set_brightness. By only doing this if blinking is inactive we
> prevent blinking from stopping if the blinking is in its off phase while
> the file is written.
> 
> Instead, if blinking is active, the changed intensity values are applied
> upon the next blink. This is consistent with changing the brightness on
> monochrome LEDs with active blinking.
> 
> Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Tested-by: Sven Schuchmann <schuchmann@schleissheimer.de>
> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> ---
> 
> Notes:
>     V1->V2: Change title, add tags
> 
>  drivers/leds/led-class-multicolor.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
> index e317408583df..5b1479b5d32c 100644
> --- a/drivers/leds/led-class-multicolor.c
> +++ b/drivers/leds/led-class-multicolor.c
> @@ -59,7 +59,8 @@ static ssize_t multi_intensity_store(struct device *dev,
>  	for (i = 0; i < mcled_cdev->num_colors; i++)
>  		mcled_cdev->subled_info[i].intensity = intensity_value[i];
> 
> -	led_set_brightness(led_cdev, led_cdev->brightness);
> +	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
> +		led_set_brightness(led_cdev, led_cdev->brightness);
>  	ret = size;
>  err_out:
>  	mutex_unlock(&led_cdev->led_access);
> 
> base-commit: 210e04ff768142b96452030c4c2627512b30ad95
> --
> 2.36.1

Re: [PATCH RESEND v2] led: multicolor: Fix intensity setting while SW blinking

[Jacek Anaszewski]

Hi Lee,

I've recently recalled this patch that was ready to apply.
Would you mind to take a look?

On 6/27/22 15:31, Sven Schwermer wrote:
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> When writing to the multi_intensity file, don't unconditionally call
> led_set_brightness. By only doing this if blinking is inactive we
> prevent blinking from stopping if the blinking is in its off phase while
> the file is written.
> 
> Instead, if blinking is active, the changed intensity values are applied
> upon the next blink. This is consistent with changing the brightness on
> monochrome LEDs with active blinking.
> 
> Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Tested-by: Sven Schuchmann <schuchmann@schleissheimer.de>
> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> ---
> 
> Notes:
>      V1->V2: Change title, add tags
> 
>   drivers/leds/led-class-multicolor.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
> index e317408583df..5b1479b5d32c 100644
> --- a/drivers/leds/led-class-multicolor.c
> +++ b/drivers/leds/led-class-multicolor.c
> @@ -59,7 +59,8 @@ static ssize_t multi_intensity_store(struct device *dev,
>   	for (i = 0; i < mcled_cdev->num_colors; i++)
>   		mcled_cdev->subled_info[i].intensity = intensity_value[i];
>   
> -	led_set_brightness(led_cdev, led_cdev->brightness);
> +	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
> +		led_set_brightness(led_cdev, led_cdev->brightness);
>   	ret = size;
>   err_out:
>   	mutex_unlock(&led_cdev->led_access);
> 
> base-commit: 210e04ff768142b96452030c4c2627512b30ad95

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH RESEND v2] led: multicolor: Fix intensity setting while SW blinking

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

On Mon 2022-06-27 15:31:10, Sven Schwermer wrote:
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> When writing to the multi_intensity file, don't unconditionally call
> led_set_brightness. By only doing this if blinking is inactive we
> prevent blinking from stopping if the blinking is in its off phase while
> the file is written.
> 
> Instead, if blinking is active, the changed intensity values are applied
> upon the next blink. This is consistent with changing the brightness on
> monochrome LEDs with active blinking.
> 
> Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Tested-by: Sven Schuchmann <schuchmann@schleissheimer.de>
> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Signed-off-by: Sven Schwermer
<sven.schwermer@disruptive-technologies.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

BR,
									Pavel
									
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH RESEND v2] led: multicolor: Fix intensity setting while SW blinking

[Tobias Deiminger]

Hi Lee and Pavel,

this is still an issue. Sven's patch still applies and fixes the bug.
Would you mind having another look?

Minimal reproducer:

 echo timer > trigger
 echo 255 > brightness
 echo 255 255 255 > multi_intensity  # stops blinking with 50% probability

I encountered this independently and found the thread in hindsight. See review
comments below.

Am Mo, 27. Jun 22 15:31 schrieb Sven Schwermer <sven@svenschwermer.de>:
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> When writing to the multi_intensity file, don't unconditionally call
> led_set_brightness. By only doing this if blinking is inactive we
> prevent blinking from stopping if the blinking is in its off phase while
> the file is written.
> 
> Instead, if blinking is active, the changed intensity values are applied
> upon the next blink. This is consistent with changing the brightness on
> monochrome LEDs with active blinking.
> 
> Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Tested-by: Sven Schuchmann <schuchmann@schleissheimer.de>
> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> ---
> 
> Notes:
>     V1->V2: Change title, add tags
> 
>  drivers/leds/led-class-multicolor.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
> index e317408583df..5b1479b5d32c 100644
> --- a/drivers/leds/led-class-multicolor.c
> +++ b/drivers/leds/led-class-multicolor.c
> @@ -59,7 +59,8 @@ static ssize_t multi_intensity_store(struct device *dev,
>  	for (i = 0; i < mcled_cdev->num_colors; i++)
>  		mcled_cdev->subled_info[i].intensity = intensity_value[i];
>  
> -	led_set_brightness(led_cdev, led_cdev->brightness);
> +	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
> +		led_set_brightness(led_cdev, led_cdev->brightness);

Had my own debugging session and ended up with the very same conclusion. Seems
solid and consistent.

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

Btw, my initial attempt to fix it was to have two led_get_brightness variants.
The existing variant gets the timer-aware momentary brightness, and a new one
would get the timer-agnostic on-value. The latter variant could then be used to
call led_set_brightness without the risk of resetting blinking. Worked, but got
overly complicated.

Best regards
Tobias

>  	ret = size;
>  err_out:
>  	mutex_unlock(&led_cdev->led_access);
> 
> base-commit: 210e04ff768142b96452030c4c2627512b30ad95
> -- 
> 2.36.1
> 

Re: [PATCH RESEND v2] led: multicolor: Fix intensity setting while SW blinking

[Lee Jones]

On Wed, 02 Apr 2025, Tobias Deiminger wrote:

> Hi Lee and Pavel,
> 
> this is still an issue. Sven's patch still applies and fixes the bug.
> Would you mind having another look?

I don't have Sven's patch in my inbox.  Please resubmit it.

> Minimal reproducer:
> 
>  echo timer > trigger
>  echo 255 > brightness
>  echo 255 255 255 > multi_intensity  # stops blinking with 50% probability
> 
> I encountered this independently and found the thread in hindsight. See review
> comments below.
> 
> Am Mo, 27. Jun 22 15:31 schrieb Sven Schwermer <sven@svenschwermer.de>:
> > From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> > 
> > When writing to the multi_intensity file, don't unconditionally call
> > led_set_brightness. By only doing this if blinking is inactive we
> > prevent blinking from stopping if the blinking is in its off phase while
> > the file is written.
> > 
> > Instead, if blinking is active, the changed intensity values are applied
> > upon the next blink. This is consistent with changing the brightness on
> > monochrome LEDs with active blinking.
> > 
> > Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > Tested-by: Sven Schuchmann <schuchmann@schleissheimer.de>
> > Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> > ---
> > 
> > Notes:
> >     V1->V2: Change title, add tags
> > 
> >  drivers/leds/led-class-multicolor.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
> > index e317408583df..5b1479b5d32c 100644
> > --- a/drivers/leds/led-class-multicolor.c
> > +++ b/drivers/leds/led-class-multicolor.c
> > @@ -59,7 +59,8 @@ static ssize_t multi_intensity_store(struct device *dev,
> >  	for (i = 0; i < mcled_cdev->num_colors; i++)
> >  		mcled_cdev->subled_info[i].intensity = intensity_value[i];
> >  
> > -	led_set_brightness(led_cdev, led_cdev->brightness);
> > +	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
> > +		led_set_brightness(led_cdev, led_cdev->brightness);
> 
> Had my own debugging session and ended up with the very same conclusion. Seems
> solid and consistent.
> 
> Reviewed-by: Tobias Deiminger <tobias.deiminger@linutronix.de>
> 
> Btw, my initial attempt to fix it was to have two led_get_brightness variants.
> The existing variant gets the timer-aware momentary brightness, and a new one
> would get the timer-agnostic on-value. The latter variant could then be used to
> call led_set_brightness without the risk of resetting blinking. Worked, but got
> overly complicated.
> 
> Best regards
> Tobias
> 
> >  	ret = size;
> >  err_out:
> >  	mutex_unlock(&led_cdev->led_access);
> > 
> > base-commit: 210e04ff768142b96452030c4c2627512b30ad95
> > -- 
> > 2.36.1
> > 

-- 
Lee Jones [李琼斯]

[PATCH v3] led: multicolor: Fix intensity setting while SW blinking

[Sven Schwermer]

From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

When writing to the multi_intensity file, don't unconditionally call
led_set_brightness. By only doing this if blinking is inactive we
prevent blinking from stopping if the blinking is in its off phase while
the file is written.

Instead, if blinking is active, the changed intensity values are applied
upon the next blink. This is consistent with changing the brightness on
monochrome LEDs with active blinking.

Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Tested-by: Sven Schuchmann <schuchmann@schleissheimer.de>
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---

Notes:
    V1->V2: Change title, add tags
    V2->V3: Add Pavel's ACK

 drivers/leds/led-class-multicolor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
index e317408583df9..5b1479b5d32ca 100644
--- a/drivers/leds/led-class-multicolor.c
+++ b/drivers/leds/led-class-multicolor.c
@@ -59,7 +59,8 @@ static ssize_t multi_intensity_store(struct device *dev,
 	for (i = 0; i < mcled_cdev->num_colors; i++)
 		mcled_cdev->subled_info[i].intensity = intensity_value[i];
 
-	led_set_brightness(led_cdev, led_cdev->brightness);
+	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
+		led_set_brightness(led_cdev, led_cdev->brightness);
 	ret = size;
 err_out:
 	mutex_unlock(&led_cdev->led_access);
-- 
2.49.0

Re: (subset) [PATCH v3] led: multicolor: Fix intensity setting while SW blinking

[Lee Jones]

On Fri, 04 Apr 2025 20:40:36 +0200, Sven Schwermer wrote:
> When writing to the multi_intensity file, don't unconditionally call
> led_set_brightness. By only doing this if blinking is inactive we
> prevent blinking from stopping if the blinking is in its off phase while
> the file is written.
> 
> Instead, if blinking is active, the changed intensity values are applied
> upon the next blink. This is consistent with changing the brightness on
> monochrome LEDs with active blinking.
> 
> [...]

Applied, thanks!

[1/1] led: multicolor: Fix intensity setting while SW blinking
      commit: 0b923d52bd15008d5a0e7b0f82e06db10a868d98

--
Lee Jones [李琼斯]

[PATCH RESEND v2] led: multicolor: Fix intensity setting while SW blinking

[Sven Schwermer]

From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

When writing to the multi_intensity file, don't unconditionally call
led_set_brightness. By only doing this if blinking is inactive we
prevent blinking from stopping if the blinking is in its off phase while
the file is written.

Instead, if blinking is active, the changed intensity values are applied
upon the next blink. This is consistent with changing the brightness on
monochrome LEDs with active blinking.

Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Tested-by: Sven Schuchmann <schuchmann@schleissheimer.de>
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
---

Notes:
    V1->V2: Change title, add tags

 drivers/leds/led-class-multicolor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
index e317408583df..5b1479b5d32c 100644
--- a/drivers/leds/led-class-multicolor.c
+++ b/drivers/leds/led-class-multicolor.c
@@ -59,7 +59,8 @@ static ssize_t multi_intensity_store(struct device *dev,
 	for (i = 0; i < mcled_cdev->num_colors; i++)
 		mcled_cdev->subled_info[i].intensity = intensity_value[i];
 
-	led_set_brightness(led_cdev, led_cdev->brightness);
+	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
+		led_set_brightness(led_cdev, led_cdev->brightness);
 	ret = size;
 err_out:
 	mutex_unlock(&led_cdev->led_access);

base-commit: 210e04ff768142b96452030c4c2627512b30ad95
-- 
2.36.1