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. The callers that need a blink stop, were updated to explicitly call led_stop_software_blink().

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                |    3 +--
  drivers/leds/trigger/ledtrig-oneshot.c |    1 +
  drivers/leds/trigger/ledtrig-timer.c   |    2 ++
  4 files changed, 9 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 9886dac..3210020 100644

drivers/leds/trigger/ledtrig-oneshot.c +0 -0

  	led_set_brightness(led_cdev, LED_OFF);
  }
 

drivers/leds/trigger/ledtrig-timer.c +1 -0

index 8d09327..3577c23 100644
@@ -17,6 +17,7 @@
  #include <linux/device.h>
  #include <linux/ctype.h>
  #include <linux/leds.h>
+#include "../leds.h"
 
  static ssize_t led_delay_on_show(struct device *dev,
  		struct device_attribute *attr, char *buf)

drivers/leds/trigger/ledtrig-timer.c +0 -0

  	led_set_brightness(led_cdev, LED_OFF);
  }
 

Quoted 4 lines

      if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
          led_cdev->delayed_set_value = brightness;

- schedule_work(&led_cdev->set_brightness_work);

This line is still required. Please refer to the patch d23a22a74.

Quoted 2 lines

      /* Stop blinking */

+ led_stop_software_blink(led_cdev);

This won't be needed.

14.05.2015 11:44, Jacek Anaszewski пишет:

Quoted 6 lines

Quoted 4 lines

       if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
           led_cdev->delayed_set_value = brightness;

- schedule_work(&led_cdev->set_brightness_work);

This line is still required. Please refer to the patch d23a22a74.

But I really want to get rid of this line and
vacate the work-queue, so that in the future the
per-driver work-queues can be replaced with this.

Why do you think it is still needed?
Are there any valid cases where you need to disable the soft-blink from hard-irq context? IMHO the soft-blink is only disabled as a result of user's actions, which is not in a hard-irq context.
Could you please explain the use-case?

Quoted 3 lines

Quoted 2 lines

       /* Stop blinking */

+ led_stop_software_blink(led_cdev);

This won't be needed.

Indeed, but with these changes there should be no requirement for disabling a soft-blink from hard-irq context, which is what I really wanted to have. What am I missing?

Quoted 3 lines

Indeed, but with these changes there should be no requirement for disabling a soft-blink from hard-irq context, which is what I really wanted to have. What am I missing?

Please look at this [1]. Author mentions setting brightness from sound-card irq handler.

[1] http://www.spinics.net/lists/linux-leds/msg00006.html

14.05.2015 13:33, Jacek Anaszewski пишет:

Quoted 7 lines

Quoted 3 lines

Indeed, but with these changes there should be no requirement for disabling a soft-blink from hard-irq context, which is what I really wanted to have. What am I missing?

Please look at this [1]. Author mentions setting brightness from sound-card irq handler.

[1] http://www.spinics.net/lists/linux-leds/msg00006.html

He points to the following (out-of-tree??) code:
---

        if ((jiffies / HZ / 2) & 1)
                led_trigger_blink_oneshot(ledtrig_ide,
                                &ide_blink_delay, &ide_blink_delay, 0);
        if ((jiffies / HZ / 4) & 1)
                led_trigger_event(ledtrig_ide, 100);
        if ((jiffies / HZ / 8) & 1)
                led_trigger_event(ledtrig_ide, 0);

---
I think the problem was that oneshot_trig_deactivate() was not doing led_stop_software_blink(led_cdev), and so he needed a work-queue for switching out from oneshot trigger. My patch fixes exactly that: now oneshot trigger does the proper cleanup itself.
Do you think my patch is not enough to handle this case?

14.05.2015 13:33, Jacek Anaszewski пишет:

Quoted 7 lines

Quoted 3 lines

Indeed, but with these changes there should be no requirement for disabling a soft-blink from hard-irq context, which is what I really wanted to have. What am I missing?

Please look at this [1]. Author mentions setting brightness from sound-card irq handler.

[1] http://www.spinics.net/lists/linux-leds/msg00006.html

He points to the following (out-of-tree??) code:
---

         if ((jiffies / HZ / 2) & 1)
                 led_trigger_blink_oneshot(ledtrig_ide,
                                 &ide_blink_delay, &ide_blink_delay, 0);
         if ((jiffies / HZ / 4) & 1)
                 led_trigger_event(ledtrig_ide, 100);
         if ((jiffies / HZ / 8) & 1)
                 led_trigger_event(ledtrig_ide, 0);

---
I think the problem was that oneshot_trig_deactivate() was not doing led_stop_software_blink(led_cdev), and so he needed a work-queue for switching out from oneshot trigger. My patch fixes exactly that: now oneshot trigger does the proper cleanup itself.
Do you think my patch is not enough to handle this case?