LEDS-One-Shot-Timer-Trigger-implementation

From ebc4c08c138bc8ac1ac8f4eb80f879e8e7f105d3 Mon Sep 17 00:00:00 2001

LED infrastructure lacks support for one shot timer trigger and activation. The current support allows for setting two timers, one for specifying how long a state to be on, and the second for how long the state to be off. For example, delay_on value specifies the time period an LED should stay in on state, followed by a delay_off value that specifies how long the LED should stay in off state. The on and off cycle repeats until the trigger gets deactivated. There is no provision for one time activation to implement features that require an on or off state to be held just once and then stay in the original state forever.

This feature will help implement vibrate functionality which requires one shot activation of vibrate mode without a continuous vibrate on/off cycles.

This patch implements the one-shot-timer trigger support by enhancing the current led-class and ledtrig-timer drivers to support the following use-cases:

use-case 1:
echo one-shot-timer > /sys/class/leds/SOMELED/trigger

echo 2000 > /sys/class/leds/SOMELED/delay_on

echo forever > /sys/class/leds/SOMELED/delay_off

When one-shot-timer is activated in step1, unlike the timer trigger case, one-shot-timer activate routine activates the trigger without starting any timers. The default 1 HZ delay_on and delay_off timers won't be started like in the case of timer trigger activation. Not starting timers ensures that the one shot state isn't stuck if some error occurs before actual timer periods are specified. delay_on and delay_off files get created with 0 values.

When delay_on value is specified in step 2, a timer gets started for delay_on period, and delay_off stays at 0 or whatever its current value is.

When delay_off value is specified in step 3, delay_off_store recognizes the special forever tag and records it and returns without starting any timer.

use-case 2:
echo one-shot-timer > /sys/class/leds/SOMELED/trigger echo 2000 > /sys/class/leds/SOMELED/delay_off echo forever > /sys/class/leds/SOMELED/delay_on

When one-shot-timer is activated in step1, unlike the timer trigger case, one-shot-timer activate routine activates the trigger without starting any timers. The default 1 HZ delay_on and delay_off timers won't be started like in the case of timer trigger activation. Not starting timers ensures that the one shot state isn't stuck if some error occurs before actual timer periods are specified. delay_on and delay_off files get created with 0 values.

When delay_off value is specified in step 2, a timer gets started for delay_off period, and delay_on stays at 0 or whatever its current value is.

When delay_on value is specified in step 3, delay_on_store recognizes the special forever tag and records it and returns without starting any timer.

Quoted 2 lines

From ebc4c08c138bc8ac1ac8f4eb80f879e8e7f105d3 Mon Sep 17 00:00:00 2001

From: Shuah Khan <[email protected]>
Date: Mon, 26 Mar 2012 17:34:42 -0600
Subject: [PATCH] LEDS-One-Shot-Timer-Trigger-implementation

Signed-off-by Shuah Khan <[email protected]>

---
 drivers/leds/led-class.c     |    4 +-
 drivers/leds/led-core.c      |   26 +++++++++--
 drivers/leds/ledtrig-timer.c |  101 +++++++++++++++++++++++++++++------------
 3 files changed, 96 insertions(+), 35 deletions(-)

drivers/leds/led-class.c +3 -1

index 5bff843..be7400a 100644
@@ -107,7 +107,9 @@ static void led_timer_function(unsigned long data)
 
 	led_set_brightness(led_cdev, brightness);
 
-	mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
+	if (delay != ULONG_MAX)
+		mod_timer(&led_cdev->blink_timer,
+			jiffies + msecs_to_jiffies(delay));
 }
 
 /**

drivers/leds/led-core.c +22 -4

index d686004..40685f1 100644
@@ -72,17 +72,35 @@ void led_blink_set(struct led_classdev *led_cdev,
 		   unsigned long *delay_on,
 		   unsigned long *delay_off)
 {
+	unsigned long val_on;
+	unsigned long val_off;
+
 	del_timer_sync(&led_cdev->blink_timer);
 
-	if (led_cdev->blink_set &&
+	if (delay_on && delay_off && led_cdev->blink_set &&
 	    !led_cdev->blink_set(led_cdev, delay_on, delay_off))
 		return;
 
+	/* if delay_on is null, leave it on forever after delay_off period
+	   if delay_off is null, leave it off forever after delay on period */
+	if (!delay_on)
+		val_on = ULONG_MAX;
+	else
+		val_on = *delay_on;
+
+	if (!delay_off)
+		val_off = ULONG_MAX;
+	else
+		val_off = *delay_off;
+
 	/* blink with 1 Hz as default if nothing specified */
-	if (!*delay_on && !*delay_off)
-		*delay_on = *delay_off = 500;
+	if (!val_on && !val_off) {
+		val_on = val_off = 500;
+		*delay_on = 500;
+		*delay_off = 500;
+	}
 
-	led_set_software_blink(led_cdev, *delay_on, *delay_off);
+	led_set_software_blink(led_cdev, val_on, val_off);
 }
 EXPORT_SYMBOL(led_blink_set);
 

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

index 328c64c..bfda51c 100644

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

 	return sprintf(buf, "%lu\n", led_cdev->blink_delay_on);
 }
 

@@ -32,17 +35,25 @@ static ssize_t led_delay_on_store(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	int ret = -EINVAL;
-	char *after;
-	unsigned long state = simple_strtoul(buf, &after, 10);
-	size_t count = after - buf;
-
-	if (isspace(*after))
-		count++;
 
-	if (count == size) {

drivers/leds/ledtrig-timer.c +51 -19

+		size_t count = after - buf;
+
+		if (isspace(*after))
+			count++;
+
+		if (count == size) {
+			led_blink_set(led_cdev, &state,
+				&led_cdev->blink_delay_off);
+			led_cdev->blink_delay_on = state;
+			ret = count;
+		}
 	}
 
 	return ret;

@@ -53,6 +64,9 @@ static ssize_t led_delay_off_show(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 
+	if (led_cdev->blink_delay_off == ULONG_MAX)
+		return sprintf(buf, "%s\n", "forever");
+
 	return sprintf(buf, "%lu\n", led_cdev->blink_delay_off);
 }
 

@@ -61,17 +75,24 @@ static ssize_t led_delay_off_store(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	int ret = -EINVAL;
-	char *after;
-	unsigned long state = simple_strtoul(buf, &after, 10);
-	size_t count = after - buf;
 
-	if (isspace(*after))
-		count++;
-
-	if (count == size) {
-		led_blink_set(led_cdev, &led_cdev->blink_delay_on, &state);
-		led_cdev->blink_delay_off = state;
-		ret = count;
+	if (strncmp(buf, "forever", 7) == 0) {
+		led_blink_set(led_cdev, &led_cdev->blink_delay_on, NULL);
+		led_cdev->blink_delay_off = ULONG_MAX;
+	} else {
+		char *after;
+		unsigned long state = simple_strtoul(buf, &after, 10);
+		size_t count = after - buf;
+
+		if (isspace(*after))
+			count++;
+
+		if (count == size) {
+			led_blink_set(led_cdev, &led_cdev->blink_delay_on,
+				&state);
+			led_cdev->blink_delay_off = state;
+			ret = count;
+		}
 	}
 
 	return ret;

@@ -80,7 +101,7 @@ static ssize_t led_delay_off_store(struct device *dev,
 static DEVICE_ATTR(delay_on, 0644, led_delay_on_show, led_delay_on_store);
 static DEVICE_ATTR(delay_off, 0644, led_delay_off_show, led_delay_off_store);
 
-static void timer_trig_activate(struct led_classdev *led_cdev)
+static void timer_trig_activate_common(struct led_classdev *led_cdev)
 {
 	int rc;
 

@@ -91,17 +112,24 @@ static void timer_trig_activate(struct led_classdev *led_cdev)
 		return;
 	rc = device_create_file(led_cdev->dev, &dev_attr_delay_off);
 	if (rc)
-		goto err_out_delayon;
-
-	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
-		      &led_cdev->blink_delay_off);
+		device_remove_file(led_cdev->dev, &dev_attr_delay_on);
 
-	led_cdev->trigger_data = (void *)1;
+	else
+		led_cdev->trigger_data = (void *)1;
+}
 
-	return;
+static void timer_trig_activate_one_shot(struct led_classdev *led_cdev)
+{
+	timer_trig_activate_common(led_cdev);
+}
 
-err_out_delayon:
-	device_remove_file(led_cdev->dev, &dev_attr_delay_on);
+static void timer_trig_activate(struct led_classdev *led_cdev)
+{
+	timer_trig_activate_common(led_cdev);
+	if (led_cdev->trigger_data) {
+		led_blink_set(led_cdev, &led_cdev->blink_delay_on,
+		      &led_cdev->blink_delay_off);
+	}
 }
 
 static void timer_trig_deactivate(struct led_classdev *led_cdev)

@@ -121,14 +149,27 @@ static struct led_trigger timer_led_trigger = {
 	.deactivate = timer_trig_deactivate,
 };
 
+static struct led_trigger one_shot_timer_led_trigger = {
+	.name     = "one-shot-timer",
+	.activate = timer_trig_activate_one_shot,
+	.deactivate = timer_trig_deactivate,

In general I approve of this - thanks for your efforts.

However there are some details that need attention.

Quoted 8 lines

This patch implements the one-shot-timer trigger support by enhancing the current led-class and ledtrig-timer drivers to support the following use-cases:

use-case 1:
echo one-shot-timer > /sys/class/leds/SOMELED/trigger

I don't like the name "one-shot-timer". This name describes how you expect the functionality to be used, but not what the actual difference between "timer" and "one-shot-timer" is.
That difference is simply that one-shot-timer does not start an initial default blink, while "timer" does.
So a name like "timer-no-default" would be a more accurate description and so should be preferred.

Quoted 3 lines

echo 2000 > /sys/class/leds/SOMELED/delay_on

This command will call led_blink_set with an on time of '2000' and an off time of '0'. This will cause led_set_software_blink to turn the LED on and set no off timer. This is just as bad as starting a default blink - there is no guarantee that the LED (or vibrator) will ever be turned off.

You need to set 'delay_off' to 'forever' first.

Quoted 2 lines

I think you should also mention here that:

- The 'forever' value is stored as 'ULONG_MAX'. Possibly a

       #define FOREVER ULONG_MAX

in the code would be appropriate. This mapping is only used internally, not in the interface.

- The led_blink_set function - which takes two pointers to times - has been extended so that a NULL instead of a pointer means "forever".

Quoted 1 line

+

I think

                return sprintf(buf, "forever\n");

if the more common usage.

Also in led_delay_off_show() below.

kstrtoul is currently preferred. It ensures uniform error codes.

Review

Quoted 2 lines

(quoted diff) +1 -0

+	

Do you need to led_trigger_unregister(&timer_led_trigger) if the registration of one_shot_timer_led_trigger fails?

Neil,

Quoted 4 lines

In general I approve of this - thanks for your efforts.

However there are some details that need attention.

Thanks.

Quoted 9 lines

Quoted 8 lines

This patch implements the one-shot-timer trigger support by enhancing the current led-class and ledtrig-timer drivers to support the following use-cases:

use-case 1:
echo one-shot-timer > /sys/class/leds/SOMELED/trigger

Quoted 7 lines

I don't like the name "one-shot-timer". This name describes how you expect the functionality to be used, but not what the actual difference between "timer" and "one-shot-timer" is.
That difference is simply that one-shot-timer does not start an initial default blink, while "timer" does.
So a name like "timer-no-default" would be a more accurate description and so should be preferred.

Yes, timer-no-default describes the enhancement I am making, better than one-shot-timer does. I will generate patch v2 to address your concerns and will change the name to timer-no-default and update the relevant code and documentation that goes with the patch to reflect the change.

Quoted 4 lines

kstrtoul is currently preferred. It ensures uniform error codes.

Yes. I noticed the checkpatch.pl warnings. This change is better made as a separate patch. Do you mind if I deferred it and volunteer to fix it in a separate patch shortly. :)

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +1 -0

+	

Do you need to led_trigger_unregister(&timer_led_trigger) if the registration of one_shot_timer_led_trigger fails?

Yes, considered that while I was changing this routine, and thought might be better to leave the registered timer alone when the second registration fails. I can go either way.

A separate patch is certainly a good idea.
I would do the clean-up patches first, and then have the big change at the end of the series. That way checkpatch won't complain about anything in your change.
But you should do what you are most comfortable with.

Quoted 8 lines

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +1 -0

+	

Do you need to led_trigger_unregister(&timer_led_trigger) if the registration of one_shot_timer_led_trigger fails?

Quoted 3 lines

Yes, considered that while I was changing this routine, and thought might be better to leave the registered timer alone when the second registration fails. I can go either way.

The important point is that if timer_trig_init fails, then timer_trig_exit will never be called.
So when timer_trig_init fails, it must ensure that it has un-done any bits that it successfully did.

From f7e8a55fb9ffa10daad1bbb80f2e9e0d81ad359f Mon Sep 17 00:00:00 2001

Neil,

I also added documentation file in this second version. Thanks again for brainstorming ideas and reviews. Leaving the subject line unchanged to avoid confusion, and the documentation and code are updated to change one-shot-timer to timer-no-default.

Quoted 6 lines

A separate patch is certainly a good idea.
I would do the clean-up patches first, and then have the big change at the end of the series. That way checkpatch won't complain about anything in your change.
But you should do what you are most comfortable with.

Decided to do the simple_strtoul() cleanup patch later - after this one goes through.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +1 -0

+	

Do you need to led_trigger_unregister(&timer_led_trigger) if the registration of one_shot_timer_led_trigger fails?

Quoted 8 lines

Quoted 3 lines

Yes, considered that while I was changing this routine, and thought might be better to leave the registered timer alone when the second registration fails. I can go either way.

The important point is that if timer_trig_init fails, then timer_trig_exit will never be called.
So when timer_trig_init fails, it must ensure that it has un-done any bits that it successfully did.

Fixed.

-- Shuah

LED infrastructure lacks support for timer-no-default trigger and activation. The current support allows for setting two timers, one for specifying how long a state to be on, and the second for how long the state to be off. For example, delay_on value specifies the time period an LED should stay in on state, followed by a delay_off value that specifies how long the LED should stay in off state. The on and off cycle repeats until the trigger gets deactivated. There is no provision for one time activation to implement features that require an on or off state to be held just once and then stay in the original state forever.

This feature will help implement vibrate functionality which requires one time activation of vibrate mode without a continuous vibrate on/off cycles.

This patch implements the timer-no-default trigger support by enhancing the current led-class, led-core, and ledtrig-timer drivers to

1. Add support for forever timer case. forever tag can be written to delay_on or delay_off files. Internally forever is mapped to ULONG_MAX with no timer associated with it.

2. The led_blink_set() which takes two pointers to times one each for delay_on and delay_off has been extended so that a NULL instead of a pointer means "forever".

3. Add a new timer-no-default trigger to ledtrig-timer

The above enhancements support the following use-cases:

use-case 1:
echo timer-no-default > /sys/class/leds/SOMELED/trigger echo forever > /sys/class/leds/SOMELED/delay_off echo 2000 > /sys/class/leds/SOMELED/delay_on

When timer-no-default is activated in step1, unlike the timer trigger case, timer-no-default activate routine activates the trigger without starting any timers. The default 1 HZ delay_on and delay_off timers won't be started like in the case of timer trigger activation. Not starting timers ensures that the one time state isn't stuck if some error occurs before actual timer periods are specified. delay_on and delay_off files get created with 0 values. Please note that it is important to set delay_off to forever prior to setting delay_on value. If the order is reversed, the LED will be turned on, with no timer set to turn it off.

When delay_off value is specified in step 2, delay_off_store recognizes the special forever tag and records it and returns without starting any timer. Internally forever maps to ULONG_MAX. The led_blink_set() which takes two pointers to times one each for delay_on and delay_off has been extended so that a NULL instead of a pointer means "forever".

When delay_on value is specified in step 3, a timer gets started for delay_on period, and delay_off stays at ULONG_MAX with no timer associated with it.

use-case 2:
echo timer-no-default > /sys/class/leds/SOMELED/trigger echo forever > /sys/class/leds/SOMELED/delay_on echo 2000 > /sys/class/leds/SOMELED/delay_off

When timer-no-default is activated in step1, unlike the timer trigger case, timer-no-default activate routine activates the trigger without starting any timers. The default 1 HZ delay_on and delay_off timers won't be started like in the case of timer trigger activation. Not starting timers ensures that the one time state isn't stuck if some error occurs before actual timer periods are specified. delay_on and delay_off files get created with 0 values. Please note that it is important to set delay_on to forever prior to setting delay_off value. If the order is reversed, the LED will be turned off, with no timer set to turn it back on.

When delay_on value is specified in step 2, delay_on_store recognizes the special forever tag and records it and returns without starting any timer. Internally forever maps to ULONG_MAX.

When delay_off value is specified in step 3, a timer gets started for delay_off period, and delay_on stays at ULONG_MAX with no timer associated with it.

From f7e8a55fb9ffa10daad1bbb80f2e9e0d81ad359f Mon Sep 17 00:00:00 2001
From: Shuah Khan <[email protected]>
Date: Wed, 28 Mar 2012 15:40:33 -0600
Subject: [PATCH] LEDS-timer-no-default-implementation

Signed-off-by Shuah Khan <[email protected]>

---
 .../leds/leds-timer_no_default_trigger.txt         |   79 +++++++++++++++
 drivers/leds/led-class.c                           |    4 +-
 drivers/leds/led-core.c                            |   26 ++++-
 drivers/leds/leds.h                                |    2 +
 drivers/leds/ledtrig-timer.c                       |  104 ++++++++++++++------
 5 files changed, 180 insertions(+), 35 deletions(-)
 create mode 100644 Documentation/leds/leds-timer_no_default_trigger.txt

Documentation/leds/leds-timer_no_default_trigger.txt new file +79 -0

new file mode 100644
index 0000000..ff3fdad
@@ -0,0 +1,79 @@
+
+LED timer no default trigger feature (one shot timer)
+=====================================================
+
+LED infrastructure lacks support for timer-no-default trigger and activation.
+The current support allows for setting two timers, one for specifying how
+long a state to be on, and the second for how long the state to be off. For
+example, delay_on value specifies the time period an LED should stay in on
+state, followed by a delay_off value that specifies how long the LED should
+stay in off state. The on and off cycle repeats until the trigger gets
+deactivated. There is no provision for one time activation to implement
+features that require an on or off state to be held just once and then stay
+in the original state forever.
+
+This feature will help implement vibrate functionality which requires one
+time activation of vibrate mode without a continuous vibrate on/off cycles.
+
+This patch implements the timer-no-default trigger support by enhancing the
+current led-class, led-core, and ledtrig-timer drivers to:
+
+- Add support for forever timer case. forever tag can be written to delay_on
+  or delay_off files. Internally forever is mapped to ULONG_MAX with no timer
+  associated with it.
+
+- The led_blink_set() which takes two pointers to times one each for delay_on
+  and delay_off has been extended so that a NULL instead of a pointer means
+  "forever".
+
+- Add a new timer-no-default trigger to ledtrig-timer
+
+The above enhancements support the following use-cases:
+
+use-case 1:
+echo timer-no-default > /sys/class/leds/SOMELED/trigger
+echo forever > /sys/class/leds/SOMELED/delay_off
+echo 2000 > /sys/class/leds/SOMELED/delay_on
+
+When timer-no-default is activated in step1, unlike the timer trigger case,
+timer-no-default activate routine activates the trigger without starting
+any timers. The default 1 HZ delay_on and delay_off timers won't be started
+like in the case of timer trigger activation. Not starting timers ensures
+that the one time state isn't stuck if some error occurs before actual timer
+periods are specified. delay_on and delay_off files get created with 0
+values. Please note that it is important to set delay_off to forever prior
+to setting delay_on value. If the order is reversed, the LED will be turned
+on, with no timer set to turn it off.
+
+When delay_off value is specified in step 2, delay_off_store recognizes the
+special forever tag and records it and returns without starting any timer.
+Internally forever maps to ULONG_MAX. The led_blink_set() which takes
+two pointers to times one each for delay_on and delay_off has been extended
+so that a NULL instead of a pointer means "forever".
+
+When delay_on value is specified in step 3, a timer gets started for
+delay_on period, and delay_off stays at ULONG_MAX with no timer associated
+with it.
+
+use-case 2:
+echo timer-no-default > /sys/class/leds/SOMELED/trigger
+echo forever > /sys/class/leds/SOMELED/delay_on
+echo 2000 > /sys/class/leds/SOMELED/delay_off
+
+When timer-no-default is activated in step1, unlike the timer trigger case,
+timer-no-default activate routine activates the trigger without starting
+any timers. The default 1 HZ delay_on and delay_off timers won't be started
+like in the case of timer trigger activation. Not starting timers ensures
+that the one time state isn't stuck if some error occurs before actual timer
+periods are specified. delay_on and delay_off files get created with 0
+values. Please note that it is important to set delay_on to forever prior
+to setting delay_off value. If the order is reversed, the LED will be turned
+off, with no timer set to turn it back on.
+
+When delay_on value is specified in step 2, delay_on_store recognizes the
+special forever tag and records it and returns without starting any timer.
+Internally forever maps to ULONG_MAX.
+
+When delay_off value is specified in step 3, a timer gets started for
+delay_off period, and delay_on stays at ULONG_MAX with no timer associated
+with it.

drivers/leds/led-class.c +3 -1

index 5bff843..ed123ba 100644
@@ -107,7 +107,9 @@ static void led_timer_function(unsigned long data)
 
 	led_set_brightness(led_cdev, brightness);
 
-	mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
+	if (delay != LED_TIMER_FOREVER)
+		mod_timer(&led_cdev->blink_timer,
+			jiffies + msecs_to_jiffies(delay));
 }
 
 /**

drivers/leds/led-core.c +22 -4

index d686004..419b0bc 100644
@@ -72,17 +72,35 @@ void led_blink_set(struct led_classdev *led_cdev,
 		   unsigned long *delay_on,
 		   unsigned long *delay_off)
 {
+	unsigned long val_on;
+	unsigned long val_off;
+
 	del_timer_sync(&led_cdev->blink_timer);
 
-	if (led_cdev->blink_set &&
+	if (delay_on && delay_off && led_cdev->blink_set &&
 	    !led_cdev->blink_set(led_cdev, delay_on, delay_off))
 		return;
 
+	/* if delay_on is null, leave it on forever after delay_off period
+	   if delay_off is null, leave it off forever after delay on period */
+	if (!delay_on)
+		val_on = LED_TIMER_FOREVER;
+	else
+		val_on = *delay_on;
+
+	if (!delay_off)
+		val_off = LED_TIMER_FOREVER;
+	else
+		val_off = *delay_off;
+
 	/* blink with 1 Hz as default if nothing specified */
-	if (!*delay_on && !*delay_off)
-		*delay_on = *delay_off = 500;
+	if (!val_on && !val_off) {
+		val_on = val_off = 500;
+		*delay_on = 500;
+		*delay_off = 500;
+	}
 
-	led_set_software_blink(led_cdev, *delay_on, *delay_off);
+	led_set_software_blink(led_cdev, val_on, val_off);
 }
 EXPORT_SYMBOL(led_blink_set);
 

drivers/leds/leds.h +2 -0

index e77c7f8..b2cda9f 100644
@@ -17,6 +17,8 @@
 #include <linux/rwsem.h>
 #include <linux/leds.h>
 
+#define LED_TIMER_FOREVER ULONG_MAX
+
 static inline void led_set_brightness(struct led_classdev *led_cdev,
 					enum led_brightness value)
 {

drivers/leds/ledtrig-timer.c +74 -30

index 328c64c..e323cf2 100644
@@ -24,6 +24,9 @@ static ssize_t led_delay_on_show(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 
+	if (led_cdev->blink_delay_on == LED_TIMER_FOREVER)
+		return sprintf(buf, "forever\n");
+
 	return sprintf(buf, "%lu\n", led_cdev->blink_delay_on);
 }
 

@@ -32,17 +35,25 @@ static ssize_t led_delay_on_store(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	int ret = -EINVAL;
-	char *after;
-	unsigned long state = simple_strtoul(buf, &after, 10);
-	size_t count = after - buf;
-
-	if (isspace(*after))
-		count++;
 
-	if (count == size) {
-		led_blink_set(led_cdev, &state, &led_cdev->blink_delay_off);
-		led_cdev->blink_delay_on = state;
-		ret = count;
+	if (strncmp(buf, "forever", 7) == 0) {
+		led_blink_set(led_cdev, NULL, &led_cdev->blink_delay_off);
+		led_cdev->blink_delay_on = LED_TIMER_FOREVER;
+		ret = size;
+	} else {
+		char *after;
+		unsigned long state = simple_strtoul(buf, &after, 10);
+		size_t count = after - buf;
+
+		if (isspace(*after))
+			count++;
+
+		if (count == size) {
+			led_blink_set(led_cdev, &state,
+				&led_cdev->blink_delay_off);
+			led_cdev->blink_delay_on = state;
+			ret = count;
+		}
 	}
 
 	return ret;

@@ -53,6 +64,9 @@ static ssize_t led_delay_off_show(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 
+	if (led_cdev->blink_delay_off == LED_TIMER_FOREVER)
+		return sprintf(buf, "forever\n");
+
 	return sprintf(buf, "%lu\n", led_cdev->blink_delay_off);
 }
 

@@ -61,17 +75,24 @@ static ssize_t led_delay_off_store(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	int ret = -EINVAL;
-	char *after;
-	unsigned long state = simple_strtoul(buf, &after, 10);
-	size_t count = after - buf;
 
-	if (isspace(*after))
-		count++;
-
-	if (count == size) {
-		led_blink_set(led_cdev, &led_cdev->blink_delay_on, &state);
-		led_cdev->blink_delay_off = state;
-		ret = count;
+	if (strncmp(buf, "forever", 7) == 0) {
+		led_blink_set(led_cdev, &led_cdev->blink_delay_on, NULL);
+		led_cdev->blink_delay_off = LED_TIMER_FOREVER;
+	} else {
+		char *after;
+		unsigned long state = simple_strtoul(buf, &after, 10);
+		size_t count = after - buf;
+
+		if (isspace(*after))
+			count++;
+
+		if (count == size) {
+			led_blink_set(led_cdev, &led_cdev->blink_delay_on,
+				&state);
+			led_cdev->blink_delay_off = state;
+			ret = count;
+		}
 	}
 
 	return ret;

@@ -80,7 +101,7 @@ static ssize_t led_delay_off_store(struct device *dev,
 static DEVICE_ATTR(delay_on, 0644, led_delay_on_show, led_delay_on_store);
 static DEVICE_ATTR(delay_off, 0644, led_delay_off_show, led_delay_off_store);
 
-static void timer_trig_activate(struct led_classdev *led_cdev)
+static void timer_trig_activate_common(struct led_classdev *led_cdev)
 {
 	int rc;
 

@@ -91,17 +112,24 @@ static void timer_trig_activate(struct led_classdev *led_cdev)
 		return;
 	rc = device_create_file(led_cdev->dev, &dev_attr_delay_off);
 	if (rc)
-		goto err_out_delayon;
-
-	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
-		      &led_cdev->blink_delay_off);
+		device_remove_file(led_cdev->dev, &dev_attr_delay_on);
 
-	led_cdev->trigger_data = (void *)1;
+	else
+		led_cdev->trigger_data = (void *)1;
+}
 
-	return;
+static void timer_trig_activate_timer_no_default(struct led_classdev *led_cdev)
+{
+	timer_trig_activate_common(led_cdev);
+}
 
-err_out_delayon:
-	device_remove_file(led_cdev->dev, &dev_attr_delay_on);
+static void timer_trig_activate(struct led_classdev *led_cdev)
+{
+	timer_trig_activate_common(led_cdev);
+	if (led_cdev->trigger_data) {
+		led_blink_set(led_cdev, &led_cdev->blink_delay_on,
+		      &led_cdev->blink_delay_off);
+	}
 }
 
 static void timer_trig_deactivate(struct led_classdev *led_cdev)

@@ -121,14 +149,30 @@ static struct led_trigger timer_led_trigger = {
 	.deactivate = timer_trig_deactivate,
 };
 
+static struct led_trigger timer_no_default_led_trigger = {
+	.name     = "timer-no-default",
+	.activate = timer_trig_activate_timer_no_default,
+	.deactivate = timer_trig_deactivate,
+};
+
 static int __init timer_trig_init(void)
 {
-	return led_trigger_register(&timer_led_trigger);
+	int rc = 0;
+
+	rc = led_trigger_register(&timer_led_trigger);
+	if (!rc) {
+		rc = led_trigger_register(&timer_no_default_led_trigger);
+		if (rc)
+			led_trigger_unregister(&timer_led_trigger);
+	}
+
+	return rc;
 }
 
 static void __exit timer_trig_exit(void)
 {
 	led_trigger_unregister(&timer_led_trigger);
+	led_trigger_unregister(&timer_no_default_led_trigger);
 }
 
 module_init(timer_trig_init);

Neil, Richard,

How about this patch? Does this good now? Anything else that needs attention?

Thanks,
-- Shuah

Quoted 454 lines · Shuah Khan

Neil,

I also added documentation file in this second version. Thanks again for brainstorming ideas and reviews. Leaving the subject line unchanged to avoid confusion, and the documentation and code are updated to change one-shot-timer to timer-no-default.

Quoted 6 lines

A separate patch is certainly a good idea.
I would do the clean-up patches first, and then have the big change at the end of the series. That way checkpatch won't complain about anything in your change.
But you should do what you are most comfortable with.

Decided to do the simple_strtoul() cleanup patch later - after this one goes through.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +1 -0

+	

Do you need to led_trigger_unregister(&timer_led_trigger) if the registration of one_shot_timer_led_trigger fails?

Quoted 8 lines

Quoted 3 lines

Yes, considered that while I was changing this routine, and thought might be better to leave the registered timer alone when the second registration fails. I can go either way.

The important point is that if timer_trig_init fails, then timer_trig_exit will never be called.
So when timer_trig_init fails, it must ensure that it has un-done any bits that it successfully did.

Fixed.

-- Shuah

LED infrastructure lacks support for timer-no-default trigger and activation. The current support allows for setting two timers, one for specifying how long a state to be on, and the second for how long the state to be off. For example, delay_on value specifies the time period an LED should stay in on state, followed by a delay_off value that specifies how long the LED should stay in off state. The on and off cycle repeats until the trigger gets deactivated. There is no provision for one time activation to implement features that require an on or off state to be held just once and then stay in the original state forever.

This feature will help implement vibrate functionality which requires one time activation of vibrate mode without a continuous vibrate on/off cycles.

This patch implements the timer-no-default trigger support by enhancing the current led-class, led-core, and ledtrig-timer drivers to

1. Add support for forever timer case. forever tag can be written to delay_on or delay_off files. Internally forever is mapped to ULONG_MAX with no timer associated with it.

2. The led_blink_set() which takes two pointers to times one each for delay_on and delay_off has been extended so that a NULL instead of a pointer means "forever".

3. Add a new timer-no-default trigger to ledtrig-timer

The above enhancements support the following use-cases:

use-case 1:
echo timer-no-default > /sys/class/leds/SOMELED/trigger echo forever > /sys/class/leds/SOMELED/delay_off echo 2000 > /sys/class/leds/SOMELED/delay_on

When timer-no-default is activated in step1, unlike the timer trigger case, timer-no-default activate routine activates the trigger without starting any timers. The default 1 HZ delay_on and delay_off timers won't be started like in the case of timer trigger activation. Not starting timers ensures that the one time state isn't stuck if some error occurs before actual timer periods are specified. delay_on and delay_off files get created with 0 values. Please note that it is important to set delay_off to forever prior to setting delay_on value. If the order is reversed, the LED will be turned on, with no timer set to turn it off.

When delay_off value is specified in step 2, delay_off_store recognizes the special forever tag and records it and returns without starting any timer. Internally forever maps to ULONG_MAX. The led_blink_set() which takes two pointers to times one each for delay_on and delay_off has been extended so that a NULL instead of a pointer means "forever".

When delay_on value is specified in step 3, a timer gets started for delay_on period, and delay_off stays at ULONG_MAX with no timer associated with it.

use-case 2:
echo timer-no-default > /sys/class/leds/SOMELED/trigger echo forever > /sys/class/leds/SOMELED/delay_on echo 2000 > /sys/class/leds/SOMELED/delay_off

When timer-no-default is activated in step1, unlike the timer trigger case, timer-no-default activate routine activates the trigger without starting any timers. The default 1 HZ delay_on and delay_off timers won't be started like in the case of timer trigger activation. Not starting timers ensures that the one time state isn't stuck if some error occurs before actual timer periods are specified. delay_on and delay_off files get created with 0 values. Please note that it is important to set delay_on to forever prior to setting delay_off value. If the order is reversed, the LED will be turned off, with no timer set to turn it back on.

When delay_on value is specified in step 2, delay_on_store recognizes the special forever tag and records it and returns without starting any timer. Internally forever maps to ULONG_MAX.

When delay_off value is specified in step 3, a timer gets started for delay_off period, and delay_on stays at ULONG_MAX with no timer associated with it.

From f7e8a55fb9ffa10daad1bbb80f2e9e0d81ad359f Mon Sep 17 00:00:00 2001
From: Shuah Khan <[email protected]>
Date: Wed, 28 Mar 2012 15:40:33 -0600
Subject: [PATCH] LEDS-timer-no-default-implementation

Signed-off-by Shuah Khan <[email protected]>

---
 .../leds/leds-timer_no_default_trigger.txt         |   79 +++++++++++++++
 drivers/leds/led-class.c                           |    4 +-
 drivers/leds/led-core.c                            |   26 ++++-
 drivers/leds/leds.h                                |    2 +
 drivers/leds/ledtrig-timer.c                       |  104 ++++++++++++++------
 5 files changed, 180 insertions(+), 35 deletions(-)
 create mode 100644 Documentation/leds/leds-timer_no_default_trigger.txt

Documentation/leds/leds-timer_no_default_trigger.txt new file +79 -0

new file mode 100644
index 0000000..ff3fdad
@@ -0,0 +1,79 @@
+
+LED timer no default trigger feature (one shot timer)
+=====================================================
+
+LED infrastructure lacks support for timer-no-default trigger and activation.
+The current support allows for setting two timers, one for specifying how
+long a state to be on, and the second for how long the state to be off. For
+example, delay_on value specifies the time period an LED should stay in on
+state, followed by a delay_off value that specifies how long the LED should
+stay in off state. The on and off cycle repeats until the trigger gets
+deactivated. There is no provision for one time activation to implement
+features that require an on or off state to be held just once and then stay
+in the original state forever.
+
+This feature will help implement vibrate functionality which requires one
+time activation of vibrate mode without a continuous vibrate on/off cycles.
+
+This patch implements the timer-no-default trigger support by enhancing the
+current led-class, led-core, and ledtrig-timer drivers to:
+
+- Add support for forever timer case. forever tag can be written to delay_on
+  or delay_off files. Internally forever is mapped to ULONG_MAX with no timer
+  associated with it.
+
+- The led_blink_set() which takes two pointers to times one each for delay_on
+  and delay_off has been extended so that a NULL instead of a pointer means
+  "forever".
+
+- Add a new timer-no-default trigger to ledtrig-timer
+
+The above enhancements support the following use-cases:
+
+use-case 1:
+echo timer-no-default > /sys/class/leds/SOMELED/trigger
+echo forever > /sys/class/leds/SOMELED/delay_off
+echo 2000 > /sys/class/leds/SOMELED/delay_on
+
+When timer-no-default is activated in step1, unlike the timer trigger case,
+timer-no-default activate routine activates the trigger without starting
+any timers. The default 1 HZ delay_on and delay_off timers won't be started
+like in the case of timer trigger activation. Not starting timers ensures
+that the one time state isn't stuck if some error occurs before actual timer
+periods are specified. delay_on and delay_off files get created with 0
+values. Please note that it is important to set delay_off to forever prior
+to setting delay_on value. If the order is reversed, the LED will be turned
+on, with no timer set to turn it off.
+
+When delay_off value is specified in step 2, delay_off_store recognizes the
+special forever tag and records it and returns without starting any timer.
+Internally forever maps to ULONG_MAX. The led_blink_set() which takes
+two pointers to times one each for delay_on and delay_off has been extended
+so that a NULL instead of a pointer means "forever".
+
+When delay_on value is specified in step 3, a timer gets started for
+delay_on period, and delay_off stays at ULONG_MAX with no timer associated
+with it.
+
+use-case 2:
+echo timer-no-default > /sys/class/leds/SOMELED/trigger
+echo forever > /sys/class/leds/SOMELED/delay_on
+echo 2000 > /sys/class/leds/SOMELED/delay_off
+
+When timer-no-default is activated in step1, unlike the timer trigger case,
+timer-no-default activate routine activates the trigger without starting
+any timers. The default 1 HZ delay_on and delay_off timers won't be started
+like in the case of timer trigger activation. Not starting timers ensures
+that the one time state isn't stuck if some error occurs before actual timer
+periods are specified. delay_on and delay_off files get created with 0
+values. Please note that it is important to set delay_on to forever prior
+to setting delay_off value. If the order is reversed, the LED will be turned
+off, with no timer set to turn it back on.
+
+When delay_on value is specified in step 2, delay_on_store recognizes the
+special forever tag and records it and returns without starting any timer.
+Internally forever maps to ULONG_MAX.
+
+When delay_off value is specified in step 3, a timer gets started for
+delay_off period, and delay_on stays at ULONG_MAX with no timer associated
+with it.

drivers/leds/led-class.c +3 -1

index 5bff843..ed123ba 100644
@@ -107,7 +107,9 @@ static void led_timer_function(unsigned long data)
 
 	led_set_brightness(led_cdev, brightness);
 
-	mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
+	if (delay != LED_TIMER_FOREVER)
+		mod_timer(&led_cdev->blink_timer,
+			jiffies + msecs_to_jiffies(delay));
 }
 
 /**

drivers/leds/led-core.c +22 -4

index d686004..419b0bc 100644
@@ -72,17 +72,35 @@ void led_blink_set(struct led_classdev *led_cdev,
 		   unsigned long *delay_on,
 		   unsigned long *delay_off)
 {
+	unsigned long val_on;
+	unsigned long val_off;
+
 	del_timer_sync(&led_cdev->blink_timer);
 
-	if (led_cdev->blink_set &&
+	if (delay_on && delay_off && led_cdev->blink_set &&
 	    !led_cdev->blink_set(led_cdev, delay_on, delay_off))
 		return;
 
+	/* if delay_on is null, leave it on forever after delay_off period
+	   if delay_off is null, leave it off forever after delay on period */
+	if (!delay_on)
+		val_on = LED_TIMER_FOREVER;
+	else
+		val_on = *delay_on;
+
+	if (!delay_off)
+		val_off = LED_TIMER_FOREVER;
+	else
+		val_off = *delay_off;
+
 	/* blink with 1 Hz as default if nothing specified */
-	if (!*delay_on && !*delay_off)
-		*delay_on = *delay_off = 500;
+	if (!val_on && !val_off) {
+		val_on = val_off = 500;
+		*delay_on = 500;
+		*delay_off = 500;
+	}
 
-	led_set_software_blink(led_cdev, *delay_on, *delay_off);
+	led_set_software_blink(led_cdev, val_on, val_off);
 }
 EXPORT_SYMBOL(led_blink_set);
 

drivers/leds/leds.h +2 -0

index e77c7f8..b2cda9f 100644
@@ -17,6 +17,8 @@
 #include <linux/rwsem.h>
 #include <linux/leds.h>
 
+#define LED_TIMER_FOREVER ULONG_MAX
+
 static inline void led_set_brightness(struct led_classdev *led_cdev,
 					enum led_brightness value)
 {

drivers/leds/ledtrig-timer.c +74 -30

index 328c64c..e323cf2 100644
@@ -24,6 +24,9 @@ static ssize_t led_delay_on_show(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 
+	if (led_cdev->blink_delay_on == LED_TIMER_FOREVER)
+		return sprintf(buf, "forever\n");
+
 	return sprintf(buf, "%lu\n", led_cdev->blink_delay_on);
 }
 

@@ -32,17 +35,25 @@ static ssize_t led_delay_on_store(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	int ret = -EINVAL;
-	char *after;
-	unsigned long state = simple_strtoul(buf, &after, 10);
-	size_t count = after - buf;
-
-	if (isspace(*after))
-		count++;
 
-	if (count == size) {
-		led_blink_set(led_cdev, &state, &led_cdev->blink_delay_off);
-		led_cdev->blink_delay_on = state;
-		ret = count;
+	if (strncmp(buf, "forever", 7) == 0) {
+		led_blink_set(led_cdev, NULL, &led_cdev->blink_delay_off);
+		led_cdev->blink_delay_on = LED_TIMER_FOREVER;
+		ret = size;
+	} else {
+		char *after;
+		unsigned long state = simple_strtoul(buf, &after, 10);
+		size_t count = after - buf;
+
+		if (isspace(*after))
+			count++;
+
+		if (count == size) {
+			led_blink_set(led_cdev, &state,
+				&led_cdev->blink_delay_off);
+			led_cdev->blink_delay_on = state;
+			ret = count;
+		}
 	}
 
 	return ret;

@@ -53,6 +64,9 @@ static ssize_t led_delay_off_show(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 
+	if (led_cdev->blink_delay_off == LED_TIMER_FOREVER)
+		return sprintf(buf, "forever\n");
+
 	return sprintf(buf, "%lu\n", led_cdev->blink_delay_off);
 }
 

@@ -61,17 +75,24 @@ static ssize_t led_delay_off_store(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	int ret = -EINVAL;
-	char *after;
-	unsigned long state = simple_strtoul(buf, &after, 10);
-	size_t count = after - buf;
 
-	if (isspace(*after))
-		count++;
-
-	if (count == size) {
-		led_blink_set(led_cdev, &led_cdev->blink_delay_on, &state);
-		led_cdev->blink_delay_off = state;
-		ret = count;
+	if (strncmp(buf, "forever", 7) == 0) {
+		led_blink_set(led_cdev, &led_cdev->blink_delay_on, NULL);
+		led_cdev->blink_delay_off = LED_TIMER_FOREVER;
+	} else {
+		char *after;
+		unsigned long state = simple_strtoul(buf, &after, 10);
+		size_t count = after - buf;
+
+		if (isspace(*after))
+			count++;
+
+		if (count == size) {
+			led_blink_set(led_cdev, &led_cdev->blink_delay_on,
+				&state);
+			led_cdev->blink_delay_off = state;
+			ret = count;
+		}
 	}
 
 	return ret;

@@ -80,7 +101,7 @@ static ssize_t led_delay_off_store(struct device *dev,
 static DEVICE_ATTR(delay_on, 0644, led_delay_on_show, led_delay_on_store);
 static DEVICE_ATTR(delay_off, 0644, led_delay_off_show, led_delay_off_store);
 
-static void timer_trig_activate(struct led_classdev *led_cdev)
+static void timer_trig_activate_common(struct led_classdev *led_cdev)
 {
 	int rc;
 

@@ -91,17 +112,24 @@ static void timer_trig_activate(struct led_classdev *led_cdev)
 		return;
 	rc = device_create_file(led_cdev->dev, &dev_attr_delay_off);
 	if (rc)
-		goto err_out_delayon;
-
-	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
-		      &led_cdev->blink_delay_off);
+		device_remove_file(led_cdev->dev, &dev_attr_delay_on);
 
-	led_cdev->trigger_data = (void *)1;
+	else
+		led_cdev->trigger_data = (void *)1;
+}
 
-	return;
+static void timer_trig_activate_timer_no_default(struct led_classdev *led_cdev)
+{
+	timer_trig_activate_common(led_cdev);
+}
 
-err_out_delayon:
-	device_remove_file(led_cdev->dev, &dev_attr_delay_on);
+static void timer_trig_activate(struct led_classdev *led_cdev)
+{
+	timer_trig_activate_common(led_cdev);
+	if (led_cdev->trigger_data) {
+		led_blink_set(led_cdev, &led_cdev->blink_delay_on,
+		      &led_cdev->blink_delay_off);
+	}
 }
 
 static void timer_trig_deactivate(struct led_classdev *led_cdev)

@@ -121,14 +149,30 @@ static struct led_trigger timer_led_trigger = {
 	.deactivate = timer_trig_deactivate,
 };
 
+static struct led_trigger timer_no_default_led_trigger = {
+	.name     = "timer-no-default",
+	.activate = timer_trig_activate_timer_no_default,
+	.deactivate = timer_trig_deactivate,
+};
+
 static int __init timer_trig_init(void)
 {
-	return led_trigger_register(&timer_led_trigger);
+	int rc = 0;
+
+	rc = led_trigger_register(&timer_led_trigger);
+	if (!rc) {
+		rc = led_trigger_register(&timer_no_default_led_trigger);
+		if (rc)
+			led_trigger_unregister(&timer_led_trigger);
+	}
+
+	return rc;
 }
 
 static void __exit timer_trig_exit(void)
 {
 	led_trigger_unregister(&timer_led_trigger);
+	led_trigger_unregister(&timer_no_default_led_trigger);
 }
 
 module_init(timer_trig_init);