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

[Lee Jones <lee@kernel.org>]

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 [李琼斯]