leds: fix brightness changing when software blinking is active

PATCH Applied Stas Sergeev 4 participants 10 messages May 14, 2015 – Jun 24, 2015 LED

Share & tools

Stas Sergeev

author

2015-05-14 15:24 UTC #0 lore raw

PATCH v2 1/1 leds: fix brightness changing when software blinking is active

The following sequence:
echo timer >/sys/class/leds/<name>/trigger
echo 1 >/sys/class/leds/<name>/brightness
should change the ON brightness for blinking.
The function led_set_brightness() was mistakenly initiating the delayed blink stop procedure, which resulted in no blinking with the timer trigger still active.

This patch fixes the problem by changing led_set_brightness() to not initiate the delayed blink stop when brightness is not 0.

CC Bryan Wu <[email protected]>

CC Richard Purdie <[email protected]>

CC Jacek Anaszewski <[email protected]>

CC Kyungmin Park <[email protected]>

CC [email protected]

Signed-off-by Stas Sergeev <[email protected]>

---
 drivers/leds/led-class.c |    5 +++++
 drivers/leds/led-core.c  |    5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

drivers/leds/led-class.c +5 -0

index 795ec99..65c2c80 100644
@@ -121,6 +121,11 @@ static void led_timer_function(unsigned long data)
 	brightness = led_get_brightness(led_cdev);
 	if (!brightness) {
 		/* Time to switch the LED on. */
+		if (led_cdev->delayed_set_value) {
+			led_cdev->blink_brightness =
+					led_cdev->delayed_set_value;
+			led_cdev->delayed_set_value = 0;
+		}
 		brightness = led_cdev->blink_brightness;
 		delay = led_cdev->blink_delay_on;
 	} else {

drivers/leds/led-core.c +3 -2

index 356e851..a90dd26 100644
@@ -119,10 +119,11 @@ void led_set_brightness(struct led_classdev *led_cdev,
 {
 	int ret = 0;
 
-	/* delay brightness setting if need to stop soft-blink timer */
+	/* delay brightness if soft-blink is active */
 	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
 		led_cdev->delayed_set_value = brightness;
-		schedule_work(&led_cdev->set_brightness_work);
+		if (brightness == LED_OFF)
+			schedule_work(&led_cdev->set_brightness_work);
 		return;
 	}

Signature · 1 line

1.7.9.5

Jacek Anaszewski

Acked-by on v2 1/1

2015-05-15 08:12 UTC #1 lore raw

Hi Stas,

Quoted 49 lines · Stas Sergeev

The following sequence:
echo timer >/sys/class/leds/<name>/trigger
echo 1 >/sys/class/leds/<name>/brightness
should change the ON brightness for blinking.
The function led_set_brightness() was mistakenly initiating the delayed blink stop procedure, which resulted in no blinking with the timer trigger still active.
This patch fixes the problem by changing led_set_brightness() to not initiate the delayed blink stop when brightness is not 0.
CC Bryan Wu <[email protected]>
CC Richard Purdie <[email protected]>
CC Jacek Anaszewski <[email protected]>
CC Kyungmin Park <[email protected]>
CC [email protected]
CC [email protected]
Signed-off-by Stas Sergeev <[email protected]>
---
  drivers/leds/led-class.c |    5 +++++
  drivers/leds/led-core.c  |    5 +++--
  2 files changed, 8 insertions(+), 2 deletions(-)
drivers/leds/led-class.c +5 -0
index 795ec99..65c2c80 100644
@@ -121,6 +121,11 @@ static void led_timer_function(unsigned long data)
  	brightness = led_get_brightness(led_cdev);
  	if (!brightness) {
  		/* Time to switch the LED on. */
+		if (led_cdev->delayed_set_value) {
+			led_cdev->blink_brightness =
+					led_cdev->delayed_set_value;
+			led_cdev->delayed_set_value = 0;
+		}
  		brightness = led_cdev->blink_brightness;
  		delay = led_cdev->blink_delay_on;
  	} else {
drivers/leds/led-core.c +0 -0
index 356e851..a90dd26 100644
@@ -119,10 +119,11 @@ void led_set_brightness(struct led_classdev *led_cdev,
  {
  	int ret = 0;
 

drivers/leds/led-core.c void led_set_brightness(struct led_classdev *led_cdev, · line 129

Quoted 10 lines

drivers/leds/led-core.c +3 -2

-	/* delay brightness setting if need to stop soft-blink timer */
+	/* delay brightness if soft-blink is active */
  	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
  		led_cdev->delayed_set_value = brightness;
-		schedule_work(&led_cdev->set_brightness_work);
+		if (brightness == LED_OFF)
+			schedule_work(&led_cdev->set_brightness_work);
  		return;
  	}

Acked-by Jacek Anaszewski <[email protected]>

Signature · 2 lines

Best Regards,
Jacek Anaszewski

Jacek Anaszewski

2015-05-15 14:09 UTC #2 lore raw

Quoted 14 lines · Jacek Anaszewski

Hi Stas,
Quoted 11 lines · Stas Sergeev
The following sequence:
echo timer >/sys/class/leds/<name>/trigger
echo 1 >/sys/class/leds/<name>/brightness
should change the ON brightness for blinking.
The function led_set_brightness() was mistakenly initiating the delayed blink stop procedure, which resulted in no blinking with the timer trigger still active.
This patch fixes the problem by changing led_set_brightness() to not initiate the delayed blink stop when brightness is not 0.

This commit message is not valid for this version of the patch. Current patch just allows to change the LED brightness while timer trigger is active. Please fix this description.

Quoted 52 lines

Quoted 39 lines

CC Bryan Wu <[email protected]>

CC Richard Purdie <[email protected]>

CC Jacek Anaszewski <[email protected]>

CC Kyungmin Park <[email protected]>

CC [email protected]

Signed-off-by Stas Sergeev <[email protected]>

---
  drivers/leds/led-class.c |    5 +++++
  drivers/leds/led-core.c  |    5 +++--
  2 files changed, 8 insertions(+), 2 deletions(-)

drivers/leds/led-class.c +5 -0

index 795ec99..65c2c80 100644
@@ -121,6 +121,11 @@ static void led_timer_function(unsigned long data)
      brightness = led_get_brightness(led_cdev);
      if (!brightness) {
          /* Time to switch the LED on. */
+        if (led_cdev->delayed_set_value) {
+            led_cdev->blink_brightness =
+                    led_cdev->delayed_set_value;
+            led_cdev->delayed_set_value = 0;
+        }
          brightness = led_cdev->blink_brightness;
          delay = led_cdev->blink_delay_on;
      } else {

drivers/leds/led-core.c +0 -0

index 356e851..a90dd26 100644
@@ -119,10 +119,11 @@ void led_set_brightness(struct led_classdev
 *led_cdev,
  {
      int ret = 0;

drivers/leds/led-core.c void led_set_brightness(struct led_classdev · line 129

Quoted 10 lines

drivers/leds/led-core.c +3 -2

 
-    /* delay brightness setting if need to stop soft-blink timer */
+    /* delay brightness if soft-blink is active */
      if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
          led_cdev->delayed_set_value = brightness;
-        schedule_work(&led_cdev->set_brightness_work);
+        if (brightness == LED_OFF)
+            schedule_work(&led_cdev->set_brightness_work);
          return;
      }

Acked-by Jacek Anaszewski <[email protected]>

Signature · 2 lines

Best Regards,
Jacek Anaszewski

Stas Sergeev

author

2015-05-15 14:11 UTC #3 lore raw

15.05.2015 17:09, Jacek Anaszewski пишет:

Quoted 13 lines · Jacek Anaszewski

Quoted 12 lines
Hi Stas,
Quoted 9 lines · Stas Sergeev
The following sequence:
echo timer >/sys/class/leds/<name>/trigger
echo 1 >/sys/class/leds/<name>/brightness
should change the ON brightness for blinking.
The function led_set_brightness() was mistakenly initiating the delayed blink stop procedure, which resulted in no blinking with the timer trigger still active.

Quoted 4 lines

Quoted 2 lines
Quoted 2 lines
This patch fixes the problem by changing led_set_brightness() to not initiate the delayed blink stop when brightness is not 0.
This commit message is not valid for this version of the patch.

Why do you think so?
---
- schedule_work(&led_cdev->set_brightness_work);
+ if (brightness == LED_OFF)
+ schedule_work(&led_cdev->set_brightness_work);
---

LED_OFF is a 0 define.

Jacek Anaszewski

2015-05-15 14:41 UTC #4 lore raw

Quoted 25 lines · Stas Sergeev

15.05.2015 17:09, Jacek Anaszewski пишет:
Quoted 13 lines · Jacek Anaszewski
Quoted 12 lines
Hi Stas,
Quoted 9 lines · Stas Sergeev
The following sequence:
echo timer >/sys/class/leds/<name>/trigger
echo 1 >/sys/class/leds/<name>/brightness
should change the ON brightness for blinking.
The function led_set_brightness() was mistakenly initiating the delayed blink stop procedure, which resulted in no blinking with the timer trigger still active.
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
This patch fixes the problem by changing led_set_brightness() to not initiate the delayed blink stop when brightness is not 0.
This commit message is not valid for this version of the patch.
Why do you think so?
---
drivers/leds/led-core.c +2 -2
-		schedule_work(&led_cdev->set_brightness_work);
+		if (brightness == LED_OFF)
+			schedule_work(&led_cdev->set_brightness_work);
---

Quoted 2 lines

LED_OFF is a 0 define.

You're right, I was just looking at the issue from different perspective.

Signature · 2 lines

Best Regards,
Jacek Anaszewski

Bryan Wu

Applied

2015-05-20 00:01 UTC #5 lore raw

Quoted 32 lines · Jacek Anaszewski

Quoted 31 lines · Stas Sergeev
15.05.2015 17:09, Jacek Anaszewski пишет:
Quoted 16 lines · Jacek Anaszewski
Quoted 14 lines
Hi Stas,
Quoted 10 lines · Stas Sergeev
The following sequence:
echo timer >/sys/class/leds/<name>/trigger
echo 1 >/sys/class/leds/<name>/brightness
should change the ON brightness for blinking.
The function led_set_brightness() was mistakenly initiating the delayed blink stop procedure, which resulted in no blinking with the timer trigger still active.
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
This patch fixes the problem by changing led_set_brightness() to not initiate the delayed blink stop when brightness is not 0.
This commit message is not valid for this version of the patch.
Why do you think so?
---
drivers/leds/led-core.c +2 -2
-               schedule_work(&led_cdev->set_brightness_work);
+               if (brightness == LED_OFF)
+                       schedule_work(&led_cdev->set_brightness_work);
---

Quoted 6 lines

Quoted 2 lines
LED_OFF is a 0 define.
You're right, I was just looking at the issue from different perspective.

Applied into my tree. Thanks,
-Bryan

Pavel Machek

2015-06-23 16:51 UTC #6 lore raw

In reply to Stas Sergeev

Quoted 11 lines · Stas Sergeev

The following sequence:
echo timer >/sys/class/leds/<name>/trigger
echo 1 >/sys/class/leds/<name>/brightness
should change the ON brightness for blinking.
The function led_set_brightness() was mistakenly initiating the delayed blink stop procedure, which resulted in no blinking with the timer trigger still active.
This patch fixes the problem by changing led_set_brightness() to not initiate the delayed blink stop when brightness is not 0.

Could we get this part of API documented? It is quite non-obvious... 0 clears the trigger, other values do not, I thought it is a bug when I saw it...

Thanks,
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Stas Sergeev

author

2015-06-23 16:59 UTC #7 lore raw

02.05.2015 17:59, Pavel Machek пишет:

Quoted 10 lines · Stas Sergeev

Quoted 9 lines
The following sequence:
echo timer >/sys/class/leds/<name>/trigger
echo 1 >/sys/class/leds/<name>/brightness
should change the ON brightness for blinking.
The function led_set_brightness() was mistakenly initiating the delayed blink stop procedure, which resulted in no blinking with the timer trigger still active.

Quoted 5 lines

Quoted 2 lines
This patch fixes the problem by changing led_set_brightness() to not initiate the delayed blink stop when brightness is not 0.
Could we get this part of API documented? It is quite non-obvious... 0 clears the trigger, other values do not, I thought it is a bug when I saw it...

I guess the thinking was that if ON brightness differs from OFF brightness, you should not clear the trigger. But if you put ON brightness similar to OFF brightness, then you can as well stop blinking at all.
Not that I am amused by that kind of logic though.

Pavel Machek

2015-06-24 08:44 UTC #8 lore raw

Quoted 14 lines · Stas Sergeev

02.05.2015 17:59, Pavel Machek пишет:
Quoted 13 lines · Stas Sergeev
Quoted 11 lines
The following sequence:
echo timer >/sys/class/leds/<name>/trigger
echo 1 >/sys/class/leds/<name>/brightness
should change the ON brightness for blinking.
The function led_set_brightness() was mistakenly initiating the delayed blink stop procedure, which resulted in no blinking with the timer trigger still active.
This patch fixes the problem by changing led_set_brightness() to not initiate the delayed blink stop when brightness is not 0.

Quoted 6 lines

Quoted 2 lines
Could we get this part of API documented? It is quite non-obvious... 0 clears the trigger, other values do not, I thought it is a bug when I saw it...
I guess the thinking was that if ON brightness differs from OFF brightness, you should not clear the trigger. But if you put ON brightness similar to OFF brightness, then you can as well stop blinking at all.

And none of this is documented. So what about this?

Documentation/ABI/testing/sysfs-class-led +12 -3

index 3646ec8..b734d7f 100644
@@ -4,16 +4,25 @@ KernelVersion:	2.6.17
 Contact:	Richard Purdie <[email protected]>
 Description:
 		Set the brightness of the LED. Most LEDs don't
-		have hardware brightness support so will just be turned on for
+		have hardware brightness support, so will just be turned on for
 		non-zero brightness settings. The value is between 0 and
 		/sys/class/leds/<led>/max_brightness.
 
+		Writing 0 to this file clears active trigger.
+
+		Writing non-zero to this file while trigger is active changes the
+		top brightness trigger is going to use.
+		
+
 What:		/sys/class/leds/<led>/max_brightness
 Date:		March 2006
 KernelVersion:	2.6.17
 Contact:	Richard Purdie <[email protected]>
 Description:
-		Maximum brightness level for this led, default is 255 (LED_FULL).
+		Maximum brightness level for this LED, default is 255 (LED_FULL).
+
+		If the LED does not support different brightness levels, this
+		should be 1.
 
 What:		/sys/class/leds/<led>/trigger
 Date:		March 2006

@@ -21,7 +30,7 @@ KernelVersion:	2.6.17
 Contact:	Richard Purdie <[email protected]>
 Description:
 		Set the trigger for this LED. A trigger is a kernel based source
-		of led events.
+		of LED events.
 		You can change triggers in a similar manner to the way an IO
 		scheduler is chosen. Trigger specific parameters can appear in
 		/sys/class/leds/<led> once a given trigger is selected.

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Jacek Anaszewski

2015-06-24 09:53 UTC #9 lore raw

Quoted 15 lines · Pavel Machek

Quoted 14 lines · Stas Sergeev
02.05.2015 17:59, Pavel Machek пишет:
Quoted 13 lines · Stas Sergeev
Quoted 11 lines
The following sequence:
echo timer >/sys/class/leds/<name>/trigger
echo 1 >/sys/class/leds/<name>/brightness
should change the ON brightness for blinking.
The function led_set_brightness() was mistakenly initiating the delayed blink stop procedure, which resulted in no blinking with the timer trigger still active.
This patch fixes the problem by changing led_set_brightness() to not initiate the delayed blink stop when brightness is not 0.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Could we get this part of API documented? It is quite non-obvious... 0 clears the trigger, other values do not, I thought it is a bug when I saw it...
I guess the thinking was that if ON brightness differs from OFF brightness, you should not clear the trigger. But if you put ON brightness similar to OFF brightness, then you can as well stop blinking at all.
And none of this is documented. So what about this?

It is documented in Documentation/leds/leds-class.txt:

"However, if you set the brightness value to LED_OFF it will also disable the timer trigger".

Nonetheless, obviously it should be also covered in the ABI documentation.

Quoted 8 lines

Documentation/ABI/testing/sysfs-class-led +0 -0

index 3646ec8..b734d7f 100644
@@ -4,16 +4,25 @@ KernelVersion:	2.6.17
  Contact:	Richard Purdie <[email protected]>
  Description:

Documentation/ABI/testing/sysfs-class-led KernelVersion: 2.6.17 · line 14

Quoted 10 lines

Documentation/ABI/testing/sysfs-class-led +5 -1

  		Set the brightness of the LED. Most LEDs don't
-		have hardware brightness support so will just be turned on for
+		have hardware brightness support, so will just be turned on for
  		non-zero brightness settings. The value is between 0 and
  		/sys/class/leds/<led>/max_brightness.
 
+		Writing 0 to this file clears active trigger.
+
+		Writing non-zero to this file while trigger is active changes the
+		top brightness trigger is going to use.

This currently will not be true e.g. for heartbeat trigger which uses max_brightness.

Quoted 25 lines

Documentation/ABI/testing/sysfs-class-led +7 -2

+		
+
  What:		/sys/class/leds/<led>/max_brightness
  Date:		March 2006
  KernelVersion:	2.6.17
  Contact:	Richard Purdie <[email protected]>
  Description:
-		Maximum brightness level for this led, default is 255 (LED_FULL).
+		Maximum brightness level for this LED, default is 255 (LED_FULL).
+
+		If the LED does not support different brightness levels, this
+		should be 1.
 
  What:		/sys/class/leds/<led>/trigger
  Date:		March 2006

@@ -21,7 +30,7 @@ KernelVersion:	2.6.17
  Contact:	Richard Purdie <[email protected]>
  Description:
  		Set the trigger for this LED. A trigger is a kernel based source
-		of led events.
+		of LED events.
  		You can change triggers in a similar manner to the way an IO
  		scheduler is chosen. Trigger specific parameters can appear in
  		/sys/class/leds/<led> once a given trigger is selected.

Signature · 2 lines

Best Regards,
Jacek Anaszewski