LEDS-One-Shot-Timer-Trigger-implementation

From 1ebe0fd67580da833f8f06fc3119445e9991100f 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 time activation of vibrate mode without a continuous vibrate on/off cycles.

From 1ebe0fd67580da833f8f06fc3119445e9991100f Mon Sep 17 00:00:00 2001
From: Shuah Khan <[email protected]>
Date: Sat, 31 Mar 2012 21:56:07 -0600
Subject: [PATCH] LEDS-One-Shot-Timer-Trigger-implementation

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

Reviewed-by NeilBrown <[email protected]>

Cc Richard Purdie <[email protected]>

---
 Documentation/leds/leds-one-shot-timer.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-one-shot-timer.txt

Documentation/leds/leds-one-shot-timer.txt new file +79 -0

new file mode 100644
index 0000000..a5429dd
@@ -0,0 +1,79 @@
+
+LED one shot timer feature
+===========================
+
+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
+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);

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 time activation of vibrate mode without a continuous vibrate on/off cycles.

Quoted 1 line

From 1ebe0fd67580da833f8f06fc3119445e9991100f Mon Sep 17 00:00:00 2001

From: Shuah Khan <[email protected]>
Date: Sat, 31 Mar 2012 21:56:07 -0600
Subject: [PATCH] LEDS-One-Shot-Timer-Trigger-implementation

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

@@ -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 +8 -10

@@ -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;
+

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

 	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);

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

+}
 
 
...

Documentation/leds/leds-one-shot-timer.txt line 16

Quoted 3 lines

Documentation/leds/leds-one-shot-timer.txt new file +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

Documentation/leds/leds-one-shot-timer.txt line 19

Quoted 3 lines

Documentation/leds/leds-one-shot-timer.txt new file +2 -0

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

Well, the documentation shouldn't refer to a "patch", as patches are short-term transient things. It is describing a kernel interface. You could call it "this feature" or, better "the one shot trigger feature".

Documentation/leds/leds-one-shot-timer.txt line 29

Quoted 9 lines

+- Add support for forever timer case. forever tag can be written to delay_on

Documentation/leds/leds-one-shot-timer.txt new file +8 -0

+  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

Similarly, it's odd to refer to "adding" things when describing an already-existing kernel feature!

drivers/leds/led-class.c static void led_timer_function(unsigned long data) · line 110

Quoted 11 lines

...

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

@@ -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)

So LED_TIMER_FOREVER really is forever? My LED doesn't turn itself back on after 2^32 jiffies?

drivers/leds/led-class.c static void led_timer_function(unsigned long data) · line 113

Quoted 3 lines

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

+		mod_timer(&led_cdev->blink_timer,
+			jiffies + msecs_to_jiffies(delay));
 }

Is seems so...

Quoted 6 lines

...

drivers/leds/ledtrig-timer.c static ssize_t led_delay_on_show(struct device *dev, · line 31

Quoted 8 lines

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

@@ -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);
 }

This is a somewhat risky kernel interface change. Previously the output was always a decimal string. With this change it is either a decimal string or "forever". Applications which were coded to the old interface will probably misinterpret the "forever" as a zero. Or they will crash and burn.

A safer approach would be to just print the value. So it comes out as 4294967295 or 18446744073709551615. That's pretty nasty, and invites people to write programs which are busted on 32-bit userspace (or on 64-bit).

Maybe it was just a mistake to overload ->blink_delay_on in this fashion. Perhaps the code will be cleaner if we add a couple of new booleans to the led_cdev.

Anyway, please have a think about this.

Quoted 13 lines

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

@@ -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 static ssize_t led_delay_on_store(struct device *dev, · line 45

Quoted 10 lines

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

-		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);

You ignored the checkpatch warning. Please don't. Take the opportunity to fix things up when altering existing code!

drivers/leds/ledtrig-timer.c static ssize_t led_delay_on_store(struct device *dev, · line 57

Quoted 12 lines

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

+		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;
+		}
 	}

Now, *writing* the "forever" is OK - it doesn't break compatibility. But we shouldn't permit bogus input such as "forever42". Could perhaps use sysfs_streq() here. Or strcmp()?

drivers/leds/ledtrig-timer.c static ssize_t led_delay_off_show(struct device *dev, · line 71

Quoted 11 lines

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

 	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);
 }

Dittoes here.

Quoted 23 lines

drivers/leds/ledtrig-timer.c +8 -10

@@ -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;
+

drivers/leds/ledtrig-timer.c static ssize_t led_delay_off_store(struct device *dev, · line 96

Quoted 10 lines

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

+		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;
+		}
 	}

More dittoes here.

Quoted 20 lines

drivers/leds/ledtrig-timer.c +2 -5

 	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);
 

drivers/leds/ledtrig-timer.c static void timer_trig_activate(struct led_classdev *led_cdev) · line 118

Quoted 3 lines

drivers/leds/ledtrig-timer.c +2 -1

-	led_cdev->trigger_data = (void *)1;
+	else
+		led_cdev->trigger_data = (void *)1;

yikes. What is the led core code doing fiddling around with an open-coded (void *)1?

drivers/leds/ledtrig-timer.c static void timer_trig_activate(struct led_classdev *led_cdev) · line 121

Quoted 6 lines

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

+}
 
 
...

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 9 lines

Documentation/leds/leds-one-shot-timer.txt line 16

Quoted 3 lines

Documentation/leds/leds-one-shot-timer.txt new file +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

I owe you filling in the blanks type explanation. Let me describe the use-case I am trying to address first. Vibrater function on phones is implemented using PWM pins on SoC or PMIC. When there is no such hardware present, a software solution is needed. Currently two drivers timed-gpio and timed-output (under staging/android in Linux 3.3) together implement the software vibrate feature. The main functionality it implements is the one time enables of timer to prevent user space crashes leaving the phone in vibrate mode causing the battery to drain. leds as it is implemented currently, is not suitable to address this use-case as it doesn't support one time enables.

Hi Shuah,

Quoted 10 lines · Shuah Khan

Quoted 9 lines

Documentation/leds/leds-one-shot-timer.txt line 16

Quoted 3 lines

Documentation/leds/leds-one-shot-timer.txt new file +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

Quoted 10 lines

I owe you filling in the blanks type explanation. Let me describe the use-case I am trying to address first. Vibrater function on phones is implemented using PWM pins on SoC or PMIC. When there is no such hardware present, a software solution is needed. Currently two drivers timed-gpio and timed-output (under staging/android in Linux 3.3) together implement the software vibrate feature. The main functionality it implements is the one time enables of timer to prevent user space crashes leaving the phone in vibrate mode causing the battery to drain. leds as it is implemented currently, is not suitable to address this use-case as it doesn't support one time enables.

So why do not you use memoryless force feedback framework that other devices use (see drivers/input/misc/*vibra.c drivers).

Thanks.

Quoted 27 lines · On Sat, 7 Apr 2012 14:56:41 -0700 Dmitry Torokhov <[email protected]>…

Hi Shuah,

Quoted 10 lines · Shuah Khan

Quoted 9 lines

Documentation/leds/leds-one-shot-timer.txt line 16

Quoted 3 lines

Documentation/leds/leds-one-shot-timer.txt new file +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

Quoted 10 lines

I owe you filling in the blanks type explanation. Let me describe the use-case I am trying to address first. Vibrater function on phones is implemented using PWM pins on SoC or PMIC. When there is no such hardware present, a software solution is needed. Currently two drivers timed-gpio and timed-output (under staging/android in Linux 3.3) together implement the software vibrate feature. The main functionality it implements is the one time enables of timer to prevent user space crashes leaving the phone in vibrate mode causing the battery to drain. leds as it is implemented currently, is not suitable to address this use-case as it doesn't support one time enables.

So why do not you use memoryless force feedback framework that other devices use (see drivers/input/misc/*vibra.c drivers).

Quoted 2 lines

Thanks.

I don't see that using a "force feedback" "input" device to control a vibrator - which is neither "force feedback" nor "input", makes any more sense than using an "led" device to control something that isn't an LED. So we are even there.

I think driving leds by writing to sysfs files is lot easier (for scripting languages particularly) than the ioctls or binary writes needed for managing input devices.

Of course, if the 'input' framework were used for controlling all LEDs - rather than just the LEDs on keyboard - then it might make sense...

Also, I don't think 'ff' allows for "vibrate for N milliseconds". It appears that one uses the "rumble" effect and have to say "turn it on", then "turn it off". Is that correct?

I found 'struct ff_replay' which has a 'length' which is a duration, but it doesn't seem to be used.

How would you tell the force feedback framework to play the vibrator for 120ms, then stop?

Quoted 3 lines · NeilBrown

Also, I don't think 'ff' allows for "vibrate for N milliseconds". It appears that one uses the "rumble" effect and have to say "turn it on", then "turn it off". Is that correct?

No, it is not.

Quoted 2 lines

I found 'struct ff_replay' which has a 'length' which is a duration, but it doesn't seem to be used.

It does, see drivers/input/ff-memless.c where it us used to schedule when effect starts and how long it should play. Non memoryless devices (such as iforce) are supposed to schedule effects themselves.

Quoted 1 line

Quoted 2 lines

How would you tell the force feedback framework to play the vibrator for 120ms, then stop?

By specifying replay->length = 120

Well, if you consider "input" is really "hid" then FF is really appropriate for iterfacing with a human.

Quoted 16 lines · On Sun, 8 Apr 2012 17:06:46 -0700 Dmitry Torokhov <[email protected]>…

Quoted 3 lines · NeilBrown

Also, I don't think 'ff' allows for "vibrate for N milliseconds". It appears that one uses the "rumble" effect and have to say "turn it on", then "turn it off". Is that correct?

No, it is not.

Quoted 2 lines

I found 'struct ff_replay' which has a 'length' which is a duration, but it doesn't seem to be used.

It does, see drivers/input/ff-memless.c where it us used to schedule when effect starts and how long it should play. Non memoryless devices (such as iforce) are supposed to schedule effects themselves.

Quoted 1 line

Quoted 4 lines

Quoted 2 lines

How would you tell the force feedback framework to play the vibrator for 120ms, then stop?

By specifying replay->length = 120

You seem to make a convincing case. I'll explore this some more and see what it is like in practice.

Clipping from above:

Quoted 4 lines

Well, if you consider "input" is really "hid" then FF is really appropriate for iterfacing with a human.

A slightly related question. My phone has accelerometers in it. I want to use them entirely a human-interface-devices. The device itself can detect inversions and taps and jerks and I want to report just those to user-space, preferably via the input (aka hid :-) subsystem. However my understanding is that accelerometer drivers aren't welcome as input drivers. Is that still true?
There is nothing 'industrial' about these accelerometers so I would like to avoid 'iio'. What are your thoughts about this?

Hi Shuah,

Quoted 10 lines · Shuah Khan

Quoted 9 lines

Documentation/leds/leds-one-shot-timer.txt line 16

Quoted 3 lines

Documentation/leds/leds-one-shot-timer.txt new file +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

Quoted 10 lines

I owe you filling in the blanks type explanation. Let me describe the use-case I am trying to address first. Vibrater function on phones is implemented using PWM pins on SoC or PMIC. When there is no such hardware present, a software solution is needed. Currently two drivers timed-gpio and timed-output (under staging/android in Linux 3.3) together implement the software vibrate feature. The main functionality it implements is the one time enables of timer to prevent user space crashes leaving the phone in vibrate mode causing the battery to drain. leds as it is implemented currently, is not suitable to address this use-case as it doesn't support one time enables.

So why do not you use memoryless force feedback framework that other devices use (see drivers/input/misc/*vibra.c drivers).

Quoted 24 lines · Dmitry Torokhov

Hi Shuah,

Quoted 10 lines · Shuah Khan

Quoted 9 lines

Documentation/leds/leds-one-shot-timer.txt line 16

Quoted 3 lines

Documentation/leds/leds-one-shot-timer.txt new file +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

Quoted 10 lines

I owe you filling in the blanks type explanation. Let me describe the use-case I am trying to address first. Vibrater function on phones is implemented using PWM pins on SoC or PMIC. When there is no such hardware present, a software solution is needed. Currently two drivers timed-gpio and timed-output (under staging/android in Linux 3.3) together implement the software vibrate feature. The main functionality it implements is the one time enables of timer to prevent user space crashes leaving the phone in vibrate mode causing the battery to drain. leds as it is implemented currently, is not suitable to address this use-case as it doesn't support one time enables.

Quoted 3 lines

So why do not you use memoryless force feedback framework that other devices use (see drivers/input/misc/*vibra.c drivers).

Dimitry,

I took a look at these vibra* drivers. The three vibrate drivers are chip-set specific. The use-case I have is a non-chip set approach to address the use-case when vibrate hardware is not present. Are you envisioning a generic approach using ff-memoryless infrastructure?

Quoted 29 lines · Shuah Khan

Quoted 24 lines · Dmitry Torokhov

Hi Shuah,

Quoted 10 lines · Shuah Khan

Quoted 9 lines

Documentation/leds/leds-one-shot-timer.txt line 16

Quoted 3 lines

Documentation/leds/leds-one-shot-timer.txt new file +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

Quoted 10 lines

I owe you filling in the blanks type explanation. Let me describe the use-case I am trying to address first. Vibrater function on phones is implemented using PWM pins on SoC or PMIC. When there is no such hardware present, a software solution is needed. Currently two drivers timed-gpio and timed-output (under staging/android in Linux 3.3) together implement the software vibrate feature. The main functionality it implements is the one time enables of timer to prevent user space crashes leaving the phone in vibrate mode causing the battery to drain. leds as it is implemented currently, is not suitable to address this use-case as it doesn't support one time enables.

Quoted 3 lines

So why do not you use memoryless force feedback framework that other devices use (see drivers/input/misc/*vibra.c drivers).

Quoted 6 lines

Dimitry,

I took a look at these vibra* drivers. The three vibrate drivers are chip-set specific. The use-case I have is a non-chip set approach to address the use-case when vibrate hardware is not present. Are you envisioning a generic approach using ff-memoryless infrastructure?

Shuah,

I guess I am confused now. You need some form of hardware to make your device to vibrate.

What exactly are you trying to do? Are you trying to:

1. activate vibration on devices that can actually do it using LED interface, or

2. use LEDs as an alternative to vibrate on devices that can't physically vibrate?

Thanks.

Quoted 50 lines · Dmitry Torokhov

Quoted 29 lines · Shuah Khan

Quoted 24 lines · Dmitry Torokhov

Hi Shuah,

Quoted 10 lines · Shuah Khan

Quoted 9 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

Quoted 10 lines

I owe you filling in the blanks type explanation. Let me describe the use-case I am trying to address first. Vibrater function on phones is implemented using PWM pins on SoC or PMIC. When there is no such hardware present, a software solution is needed. Currently two drivers timed-gpio and timed-output (under staging/android in Linux 3.3) together implement the software vibrate feature. The main functionality it implements is the one time enables of timer to prevent user space crashes leaving the phone in vibrate mode causing the battery to drain. leds as it is implemented currently, is not suitable to address this use-case as it doesn't support one time enables.

Quoted 3 lines

So why do not you use memoryless force feedback framework that other devices use (see drivers/input/misc/*vibra.c drivers).

Quoted 6 lines

Dimitry,

I took a look at these vibra* drivers. The three vibrate drivers are chip-set specific. The use-case I have is a non-chip set approach to address the use-case when vibrate hardware is not present. Are you envisioning a generic approach using ff-memoryless infrastructure?

Shuah,

I guess I am confused now. You need some form of hardware to make your device to vibrate.

What exactly are you trying to do? Are you trying to:

1. activate vibration on devices that can actually do it using LED interface, or

2. use LEDs as an alternative to vibrate on devices that can't physically vibrate?

Quoted 1 line

Thanks.

What I meant by generic approach is a higher level interface that is not tied too closely to the underlying hardware. Similar to the leds-pwm.c and leds-gpio.c handle gpio and pwm based leds. The vibrate hardware in my sue-case is a gpio based and could pwm based on some phones.

Quoted 51 lines · Shuah Khan

Quoted 50 lines · Dmitry Torokhov

Quoted 29 lines · Shuah Khan

Quoted 24 lines · Dmitry Torokhov

Hi Shuah,

Quoted 10 lines · Shuah Khan

Quoted 9 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

Quoted 10 lines

I owe you filling in the blanks type explanation. Let me describe the use-case I am trying to address first. Vibrater function on phones is implemented using PWM pins on SoC or PMIC. When there is no such hardware present, a software solution is needed. Currently two drivers timed-gpio and timed-output (under staging/android in Linux 3.3) together implement the software vibrate feature. The main functionality it implements is the one time enables of timer to prevent user space crashes leaving the phone in vibrate mode causing the battery to drain. leds as it is implemented currently, is not suitable to address this use-case as it doesn't support one time enables.

Quoted 3 lines

So why do not you use memoryless force feedback framework that other devices use (see drivers/input/misc/*vibra.c drivers).

Quoted 6 lines

Dimitry,

I took a look at these vibra* drivers. The three vibrate drivers are chip-set specific. The use-case I have is a non-chip set approach to address the use-case when vibrate hardware is not present. Are you envisioning a generic approach using ff-memoryless infrastructure?

Shuah,

I guess I am confused now. You need some form of hardware to make your device to vibrate.

What exactly are you trying to do? Are you trying to:

1. activate vibration on devices that can actually do it using LED interface, or

2. use LEDs as an alternative to vibrate on devices that can't physically vibrate?

Quoted 6 lines

Quoted 1 line

Thanks.

What I meant by generic approach is a higher level interface that is not tied too closely to the underlying hardware. Similar to the leds-pwm.c and leds-gpio.c handle gpio and pwm based leds. The vibrate hardware in my sue-case is a gpio based and could pwm based on some phones.

Ok, so you need to add drivers/input/misc/gpio-vibrate.c and pwm-vibrate.c and then use FF to activate them. This way we have all vibrate implementation use one subsystem instead of splitting between input/led/whatever else people could come up with.

Thanks.

Quoted 59 lines · Dmitry Torokhov

Quoted 51 lines · Shuah Khan

Quoted 50 lines · Dmitry Torokhov

Quoted 29 lines · Shuah Khan

Quoted 24 lines · Dmitry Torokhov

Hi Shuah,

Quoted 10 lines · Shuah Khan

Quoted 9 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

Quoted 10 lines

I owe you filling in the blanks type explanation. Let me describe the use-case I am trying to address first. Vibrater function on phones is implemented using PWM pins on SoC or PMIC. When there is no such hardware present, a software solution is needed. Currently two drivers timed-gpio and timed-output (under staging/android in Linux 3.3) together implement the software vibrate feature. The main functionality it implements is the one time enables of timer to prevent user space crashes leaving the phone in vibrate mode causing the battery to drain. leds as it is implemented currently, is not suitable to address this use-case as it doesn't support one time enables.

Quoted 3 lines

So why do not you use memoryless force feedback framework that other devices use (see drivers/input/misc/*vibra.c drivers).

Quoted 6 lines

Dimitry,

I took a look at these vibra* drivers. The three vibrate drivers are chip-set specific. The use-case I have is a non-chip set approach to address the use-case when vibrate hardware is not present. Are you envisioning a generic approach using ff-memoryless infrastructure?

Shuah,

I guess I am confused now. You need some form of hardware to make your device to vibrate.

What exactly are you trying to do? Are you trying to:

1. activate vibration on devices that can actually do it using LED interface, or

2. use LEDs as an alternative to vibrate on devices that can't physically vibrate?

Quoted 6 lines

Quoted 1 line

Thanks.

What I meant by generic approach is a higher level interface that is not tied too closely to the underlying hardware. Similar to the leds-pwm.c and leds-gpio.c handle gpio and pwm based leds. The vibrate hardware in my sue-case is a gpio based and could pwm based on some phones.

Quoted 7 lines

Ok, so you need to add drivers/input/misc/gpio-vibrate.c and pwm-vibrate.c and then use FF to activate them. This way we have all vibrate implementation use one subsystem instead of splitting between input/led/whatever else people could come up with.

Thanks.

Dmitry,

It is unfortunate that we have these two infrastructures evolve that has a lot of overlap. Let me summarize the two alternatives first so we get a feel for the work involved to address this use-case using ff and leds frameworks:

Alternative 1: using leds infrastructure

Add new kernel interface to support one time enables. This will enable existing gpio and pwm drivers to be used to implement vibrate.

Alternative 2: using ff infrastructure

Add new drivers gpio and pwm that use existing one time enable to implement vibrate in a generic way.

Does this sound right? From a quick glance it sounds like we can get to the end goal quicker and in a simpler way with Alternative 1. However, I might be missing longterm view. Any other alternatives we could explore?

Quoted 80 lines · Shuah Khan

Quoted 59 lines · Dmitry Torokhov

Quoted 51 lines · Shuah Khan

Quoted 50 lines · Dmitry Torokhov

Quoted 29 lines · Shuah Khan

Quoted 24 lines · Dmitry Torokhov

Hi Shuah,

Quoted 10 lines · Shuah Khan

Quoted 9 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

Quoted 10 lines

I owe you filling in the blanks type explanation. Let me describe the use-case I am trying to address first. Vibrater function on phones is implemented using PWM pins on SoC or PMIC. When there is no such hardware present, a software solution is needed. Currently two drivers timed-gpio and timed-output (under staging/android in Linux 3.3) together implement the software vibrate feature. The main functionality it implements is the one time enables of timer to prevent user space crashes leaving the phone in vibrate mode causing the battery to drain. leds as it is implemented currently, is not suitable to address this use-case as it doesn't support one time enables.

Quoted 3 lines

So why do not you use memoryless force feedback framework that other devices use (see drivers/input/misc/*vibra.c drivers).

Quoted 6 lines

Dimitry,

I took a look at these vibra* drivers. The three vibrate drivers are chip-set specific. The use-case I have is a non-chip set approach to address the use-case when vibrate hardware is not present. Are you envisioning a generic approach using ff-memoryless infrastructure?

Shuah,

I guess I am confused now. You need some form of hardware to make your device to vibrate.

What exactly are you trying to do? Are you trying to:

1. activate vibration on devices that can actually do it using LED interface, or

2. use LEDs as an alternative to vibrate on devices that can't physically vibrate?

Quoted 6 lines

Quoted 1 line

Thanks.

What I meant by generic approach is a higher level interface that is not tied too closely to the underlying hardware. Similar to the leds-pwm.c and leds-gpio.c handle gpio and pwm based leds. The vibrate hardware in my sue-case is a gpio based and could pwm based on some phones.

Quoted 7 lines

Ok, so you need to add drivers/input/misc/gpio-vibrate.c and pwm-vibrate.c and then use FF to activate them. This way we have all vibrate implementation use one subsystem instead of splitting between input/led/whatever else people could come up with.

Thanks.

Dmitry,

It is unfortunate that we have these two infrastructures evolve that has a lot of overlap. Let me summarize the two alternatives first so we get a feel for the work involved to address this use-case using ff and leds frameworks:

Alternative 1: using leds infrastructure

Add new kernel interface to support one time enables. This will enable existing gpio and pwm drivers to be used to implement vibrate.

Quoted 9 lines

Alternative 2: using ff infrastructure

Add new drivers gpio and pwm that use existing one time enable to implement vibrate in a generic way.

Does this sound right? From a quick glance it sounds like we can get to the end goal quicker and in a simpler way with Alternative 1. However, I might be missing longterm view. Any other alternatives we could explore?

For 1 you are forgetting "persuade current users of mainline vibrator drivers in kernel to adopt their drivers and userpace to LED framework" because we should try to provide single interface for a given function.

Thanks.

Quoted 95 lines · Dmitry Torokhov

Quoted 80 lines · Shuah Khan

Quoted 59 lines · Dmitry Torokhov

Quoted 51 lines · Shuah Khan

Quoted 50 lines · Dmitry Torokhov

Quoted 29 lines · Shuah Khan

Quoted 24 lines · Dmitry Torokhov

Hi Shuah,

Quoted 10 lines · Shuah Khan

Quoted 9 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

Quoted 10 lines

I owe you filling in the blanks type explanation. Let me describe the use-case I am trying to address first. Vibrater function on phones is implemented using PWM pins on SoC or PMIC. When there is no such hardware present, a software solution is needed. Currently two drivers timed-gpio and timed-output (under staging/android in Linux 3.3) together implement the software vibrate feature. The main functionality it implements is the one time enables of timer to prevent user space crashes leaving the phone in vibrate mode causing the battery to drain. leds as it is implemented currently, is not suitable to address this use-case as it doesn't support one time enables.

Quoted 3 lines

So why do not you use memoryless force feedback framework that other devices use (see drivers/input/misc/*vibra.c drivers).

Quoted 6 lines

Dimitry,

I took a look at these vibra* drivers. The three vibrate drivers are chip-set specific. The use-case I have is a non-chip set approach to address the use-case when vibrate hardware is not present. Are you envisioning a generic approach using ff-memoryless infrastructure?

Shuah,

I guess I am confused now. You need some form of hardware to make your device to vibrate.

What exactly are you trying to do? Are you trying to:

1. activate vibration on devices that can actually do it using LED interface, or

2. use LEDs as an alternative to vibrate on devices that can't physically vibrate?

Quoted 6 lines

Quoted 1 line

Thanks.

What I meant by generic approach is a higher level interface that is not tied too closely to the underlying hardware. Similar to the leds-pwm.c and leds-gpio.c handle gpio and pwm based leds. The vibrate hardware in my sue-case is a gpio based and could pwm based on some phones.

Quoted 7 lines

Ok, so you need to add drivers/input/misc/gpio-vibrate.c and pwm-vibrate.c and then use FF to activate them. This way we have all vibrate implementation use one subsystem instead of splitting between input/led/whatever else people could come up with.

Thanks.

Dmitry,

It is unfortunate that we have these two infrastructures evolve that has a lot of overlap. Let me summarize the two alternatives first so we get a feel for the work involved to address this use-case using ff and leds frameworks:

Alternative 1: using leds infrastructure

Add new kernel interface to support one time enables. This will enable existing gpio and pwm drivers to be used to implement vibrate.

Quoted 9 lines

Alternative 2: using ff infrastructure

Add new drivers gpio and pwm that use existing one time enable to implement vibrate in a generic way.

Does this sound right? From a quick glance it sounds like we can get to the end goal quicker and in a simpler way with Alternative 1. However, I might be missing longterm view. Any other alternatives we could explore?

For 1 you are forgetting "persuade current users of mainline vibrator drivers in kernel to adopt their drivers and userpace to LED framework" because we should try to provide single interface for a given function.

Quoted 2 lines

Thanks.

Dmitry,

I can't argue against the user-space angle. Let me give more thought to Alternative 2 and see if it indeed turns out to be more work. Thanks for a good discussion.

See if the following patch will make the decision a bit easier ;) It might need some love as I do not actually have the hardware.

Quoted 10 lines · On Sun, 01 Apr 2012 13:53:59 -0600 Shuah Khan <[email protected]> wrote:

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.

Quoted 7 lines

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

Quoted 1 line

From 1ebe0fd67580da833f8f06fc3119445e9991100f Mon Sep 17 00:00:00 2001

From: Shuah Khan <[email protected]>
Date: Sat, 31 Mar 2012 21:56:07 -0600
Subject: [PATCH] LEDS-One-Shot-Timer-Trigger-implementation

Should be "leds: one shot timer trigger implementation", please.

Quoted 3 lines

...

Documentation/leds/leds-one-shot-timer.txt line 16

Quoted 2 lines

Documentation/leds/leds-one-shot-timer.txt new file +2 -0

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

They make vibrating LED? ;)

What's going on here? You're proposing to repurpose the LEDs code to drive vibration devices? Or some devices couple a LED with a vibration device?

drivers/leds/ledtrig-timer.c static ssize_t led_delay_on_show(struct device *dev, · line 31

Quoted 13 lines

...

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

@@ -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);
 }

This is a somewhat risky kernel interface change. Previously the output was always a decimal string. With this change it is either a decimal string or "forever". Applications which were coded to the old interface will probably misinterpret the "forever" as a zero. Or they will crash and burn.

A safer approach would be to just print the value. So it comes out as 4294967295 or 18446744073709551615. That's pretty nasty, and invites people to write programs which are busted on 32-bit userspace (or on 64-bit).

Maybe it was just a mistake to overload ->blink_delay_on in this fashion. Perhaps the code will be cleaner if we add a couple of new booleans to the led_cdev.

Anyway, please have a think about this.

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 time activation of vibrate mode without a continuous vibrate on/off cycles.

From 1ebe0fd67580da833f8f06fc3119445e9991100f Mon Sep 17 00:00:00 2001
From: Shuah Khan <[email protected]>
Date: Sat, 31 Mar 2012 21:56:07 -0600
Subject: [PATCH] LEDS-One-Shot-Timer-Trigger-implementation

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

Reviewed-by NeilBrown <[email protected]>

Cc Richard Purdie <[email protected]>

---
 Documentation/leds/leds-one-shot-timer.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-one-shot-timer.txt

Documentation/leds/leds-one-shot-timer.txt new file +69 -0

new file mode 100644
index 0000000..a5429dd
@@ -0,0 +1,79 @@
+
+LED one shot timer feature
+===========================
+
+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
+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

Quoted 111 lines · Shuah Khan

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 time activation of vibrate mode without a continuous vibrate on/off cycles.

From 1ebe0fd67580da833f8f06fc3119445e9991100f Mon Sep 17 00:00:00 2001
From: Shuah Khan <[email protected]>
Date: Sat, 31 Mar 2012 21:56:07 -0600
Subject: [PATCH] LEDS-One-Shot-Timer-Trigger-implementation

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

Reviewed-by NeilBrown <[email protected]>

Cc Richard Purdie <[email protected]>

---
 Documentation/leds/leds-one-shot-timer.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-one-shot-timer.txt

Documentation/leds/leds-one-shot-timer.txt new file +75 -0

new file mode 100644
index 0000000..a5429dd
@@ -0,0 +1,79 @@
+
+LED one shot timer feature
+===========================
+
+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
+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.

Documentation/leds/leds-one-shot-timer.txt line 79

Quoted 4 lines

Documentation/leds/leds-one-shot-timer.txt new file +4 -0

+
+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.

Having looked at the code and read through the thread and Andrew's patch review, I'm left wondering why you didn't add a new trigger for this functionality?

The reason I ask that there do seem to be a number of questions about backwards compatibility and this also seems to complicate the standard timer trigger in non-obvious ways. Having a new trigger for this functionality would allow for a much clearer namespace and no backwards compatibility issues. It also means additional functionality can be added later in a contained place. I'm wondering if there is a downside to a separate trigger I'm missing?

Dimity raises some valid questions about the force-feedback framework in the input system too. We need to make a decision about where phone vibration framework belongs and then stick to that. You can argue this to either subsystem, neither "led" or "input" is a obvious description of phone vibration at a first glance!

Cheers,

Richard

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 3 lines · Richard Purdie

Having looked at the code and read through the thread and Andrew's patch review, I'm left wondering why you didn't add a new trigger for this functionality?

By new trigger do you mean, adding another interface to struct led_trigger. My first patch to solve this use-case indeed did that. I still happen to have a copy of that patch. It would require more changes to the infrastructure than this approach, however it is more explicit and clear.

static struct led_trigger gpio_led_trigger = {
.name = "gpio",
+ .activate_once = NULL,
.activate = gpio_trig_activate,
.deactivate = gpio_trig_deactivate,
};

Quoted 5 lines

Dimity raises some valid questions about the force-feedback framework in the input system too. We need to make a decision about where phone vibration framework belongs and then stick to that. You can argue this to either subsystem, neither "led" or "input" is a obvious description of phone vibration at a first glance!

force-feedback framework is another alternative. Making a decision is great, what are the next steps to get closer to making a call?

Quoted 1 line · Shuah Khan

On Tue, 2012-04-10 at 14:24 +0100, Richard Purdie wrote:

No, I did not mean adding another interface. Why can't we have a trigger which just triggers once and then stops? It would be similar to the timer trigger but with a different name and way of operating.

Quoted 7 lines

Quoted 6 lines

Dimity raises some valid questions about the force-feedback framework in the input system too. We need to make a decision about where phone vibration framework belongs and then stick to that. You can argue this to either subsystem, neither "led" or "input" is a obvious description of phone vibration at a first glance!

Quoted 2 lines

force-feedback framework is another alternative. Making a decision is great, what are the next steps to get closer to making a call?

I'd first like to understand why this couldn't be a separate trigger, then we can understand the alternatives we're comparing.

Cheers,

Richard

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Having looked at the code and read through the thread and Andrew's patch review, I'm left wondering why you didn't add a new trigger for this functionality?

The reason I ask that there do seem to be a number of questions about backwards compatibility and this also seems to complicate the standard timer trigger in non-obvious ways. Having a new trigger for this functionality would allow for a much clearer namespace and no backwards compatibility issues. It also means additional functionality can be added later in a contained place. I'm wondering if there is a downside to a separate trigger I'm missing?

This LED trigger allows userspace to "kick" the LED so that it illuminates for a short period of time.  That period is currently hard-coded to 200 ms, but that can be easily fixed by adding a sysfs attribute for the illumination time.

The original motivation for this trigger was to provide a way for userspace to provide an activity indicator for data sent/received on a serial bus, along the lines of a network activity indicator on a NIC.

Signed-off-by Jonas Bonn <[email protected]>

---

Hi,

I just stumbled across this mail thread today.  I've got this trigger that we've been using in another project that seems to fit the bill here.  It should just be a matter of adding a sysfs attribute to set the "illumination" duration to get what you need for the vibrator interface.

The interface is simple enough.  You set the LED (vibrator) trigger to "kickable" and then you get a file "kick" in the sysfs directory for the led.  Any time you write to that file, the illumination timer is reset so that the LED shines for the next 200ms.  If the LED is not kicked within 200 ms, the LED is extinguished.

Feel free to modify this to fit your needs.

Caveat:  I haven't even compile tested this on a recent kernel... we've been using this on a 2.6.32 kernel.  As far as I know, though, the LED interfaces haven't changed recently, so this is probably usable as it.

Best regards,
Jonas

 drivers/leds/ledtrig-kickable.c |  111 +++++++++++++++++++++++++++++++++++++++

 1 files changed, 111 insertions(+), 0 deletions(-)

 create mode 100644 drivers/leds/ledtrig-kickable.c

drivers/leds/ledtrig-kickable.c new file +111 -1

new file mode 100644
index 0000000..50699b1
@@ -0,0 +1,111 @@
+/*
+ * LED Kernel "Kickable" Trigger
+ *
+ * Copyright 2012 Jonas Bonn <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This is a simple trigger that provides a file 'kick' for the LED's
+ * userspace interface.  Writing anything to the 'kick' file causes the
+ * LED to illuminate for 200 ms.  Everytime the LED is 'kicked', its
+ * timer is reset to a full 200 ms.
+ *
+ * This can be used as an activity indicator for userspace processes.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/ctype.h>
+#include <linux/leds.h>
+#include "leds.h"
+
+struct kickable_trig_data {
+       struct timer_list timer;
+};
+
+static void led_kickable_function(unsigned long data)
+{
+       struct led_classdev *led_cdev = (struct led_classdev *) data;
+
+       led_set_brightness(led_cdev, LED_OFF);
+}
+
+static ssize_t kick_store(struct device *dev,
+               struct device_attribute *attr, const char *buf, size_t size)
+{
+       struct led_classdev *led_cdev = dev_get_drvdata(dev);
+       struct kickable_trig_data *kdata = led_cdev->trigger_data;
+       unsigned long delay = 0;
+
+       delay = msecs_to_jiffies(200);
+
+       mod_timer(&kdata->timer, jiffies + delay);
+
+       led_set_brightness(led_cdev, LED_FULL);
+
+       return size;
+}
+
+static DEVICE_ATTR(kick, 0200, NULL, kick_store);
+
+static void kickable_trig_activate(struct led_classdev *led_cdev)
+{
+       struct kickable_trig_data *kickable_data;
+       int rc;
+
+       kickable_data = kzalloc(sizeof(*kickable_data), GFP_KERNEL);
+       if (!kickable_data)
+               return;
+
+       led_cdev->trigger_data = kickable_data;
+       setup_timer(&kickable_data->timer,
+                   led_kickable_function, (unsigned long) led_cdev);
+
+
+       rc = device_create_file(led_cdev->dev, &dev_attr_kick);
+       if (rc)
+               return;
+
+       led_set_brightness(led_cdev, LED_OFF);
+}
+
+static void kickable_trig_deactivate(struct led_classdev *led_cdev)
+{
+       struct kickable_trig_data *kdata = led_cdev->trigger_data;
+
+       if (kdata) {
+               del_timer_sync(&kdata->timer);
+               kfree(kdata);
+               device_remove_file(led_cdev->dev, &dev_attr_kick);
+       }
+
+       led_set_brightness(led_cdev, LED_OFF);
+}
+
+static struct led_trigger kickable_led_trigger = {
+       .name     = "kickable",
+       .activate = kickable_trig_activate,
+       .deactivate = kickable_trig_deactivate,
+};
+
+static int __init kickable_trig_init(void)
+{
+       return led_trigger_register(&kickable_led_trigger);
+}
+
+static void __exit kickable_trig_exit(void)
+{
+       led_trigger_unregister(&kickable_led_trigger);
+}
+
+module_init(kickable_trig_init);
+module_exit(kickable_trig_exit);
+
+MODULE_AUTHOR("Jonas Bonn <[email protected]>");
+MODULE_DESCRIPTION("Kickable LED trigger");
+MODULE_LICENSE("GPL");
--

1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Hmm... I think I messed up the --in-reply-to parameter to git send-email. This is in reply to the "LEDS-One-Shot-Timer-Trigger-implementation" thread.

Sorry about that.

/Jonas

From 3ded425401e5c72dfa161fb4533443bebe287398 Mon Sep 17 00:00:00 2001

Quoted 164 lines

This LED trigger allows userspace to "kick" the LED so that it illuminates for a short period of time. That period is currently hard-coded to 200 ms, but that can be easily fixed by adding a sysfs attribute for the illumination time.

The original motivation for this trigger was to provide a way for userspace to provide an activity indicator for data sent/received on a serial bus, along the lines of a network activity indicator on a NIC.

Signed-off-by Jonas Bonn <[email protected]>

---

Hi,

I just stumbled across this mail thread today. I've got this trigger that we've been using in another project that seems to fit the bill here. It should just be a matter of adding a sysfs attribute to set the "illumination" duration to get what you need for the vibrator interface.

The interface is simple enough. You set the LED (vibrator) trigger to "kickable" and then you get a file "kick" in the sysfs directory for the led. Any time you write to that file, the illumination timer is reset so that the LED shines for the next 200ms. If the LED is not kicked within 200 ms, the LED is extinguished.

Feel free to modify this to fit your needs.

Caveat: I haven't even compile tested this on a recent kernel... we've been using this on a 2.6.32 kernel. As far as I know, though, the LED interfaces haven't changed recently, so this is probably usable as it.

Best regards,
Jonas

 drivers/leds/ledtrig-kickable.c |  111 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 111 insertions(+), 0 deletions(-)
 create mode 100644 drivers/leds/ledtrig-kickable.c

drivers/leds/ledtrig-kickable.c new file +111 -1

new file mode 100644
index 0000000..50699b1
@@ -0,0 +1,111 @@
+/*
+ * LED Kernel "Kickable" Trigger
+ *
+ * Copyright 2012 Jonas Bonn <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This is a simple trigger that provides a file 'kick' for the LED's
+ * userspace interface.  Writing anything to the 'kick' file causes the
+ * LED to illuminate for 200 ms.  Everytime the LED is 'kicked', its
+ * timer is reset to a full 200 ms.
+ *
+ * This can be used as an activity indicator for userspace processes.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/ctype.h>
+#include <linux/leds.h>
+#include "leds.h"
+
+struct kickable_trig_data {
+       struct timer_list timer;
+};
+
+static void led_kickable_function(unsigned long data)
+{
+       struct led_classdev *led_cdev = (struct led_classdev *) data;
+
+       led_set_brightness(led_cdev, LED_OFF);
+}
+
+static ssize_t kick_store(struct device *dev,
+               struct device_attribute *attr, const char *buf, size_t size)
+{
+       struct led_classdev *led_cdev = dev_get_drvdata(dev);
+       struct kickable_trig_data *kdata = led_cdev->trigger_data;
+       unsigned long delay = 0;
+
+       delay = msecs_to_jiffies(200);
+
+       mod_timer(&kdata->timer, jiffies + delay);
+
+       led_set_brightness(led_cdev, LED_FULL);
+
+       return size;
+}
+
+static DEVICE_ATTR(kick, 0200, NULL, kick_store);
+
+static void kickable_trig_activate(struct led_classdev *led_cdev)
+{
+       struct kickable_trig_data *kickable_data;
+       int rc;
+
+       kickable_data = kzalloc(sizeof(*kickable_data), GFP_KERNEL);
+       if (!kickable_data)
+               return;
+
+       led_cdev->trigger_data = kickable_data;
+       setup_timer(&kickable_data->timer,
+                   led_kickable_function, (unsigned long) led_cdev);
+
+
+       rc = device_create_file(led_cdev->dev, &dev_attr_kick);
+       if (rc)
+               return;
+
+       led_set_brightness(led_cdev, LED_OFF);
+}
+
+static void kickable_trig_deactivate(struct led_classdev *led_cdev)
+{
+       struct kickable_trig_data *kdata = led_cdev->trigger_data;
+
+       if (kdata) {
+               del_timer_sync(&kdata->timer);
+               kfree(kdata);
+               device_remove_file(led_cdev->dev, &dev_attr_kick);
+       }
+
+       led_set_brightness(led_cdev, LED_OFF);
+}
+
+static struct led_trigger kickable_led_trigger = {
+       .name     = "kickable",
+       .activate = kickable_trig_activate,
+       .deactivate = kickable_trig_deactivate,
+};
+
+static int __init kickable_trig_init(void)
+{
+       return led_trigger_register(&kickable_led_trigger);
+}
+
+static void __exit kickable_trig_exit(void)
+{
+       led_trigger_unregister(&kickable_led_trigger);
+}
+
+module_init(kickable_trig_init);
+module_exit(kickable_trig_exit);
+
+MODULE_AUTHOR("Jonas Bonn <[email protected]>");
+MODULE_DESCRIPTION("Kickable LED trigger");
+MODULE_LICENSE("GPL");
--

1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 7 lines · Jonas Bonn

Hmm... I think I messed up the --in-reply-to parameter to git send-email. This is in reply to the "LEDS-One-Shot-Timer-Trigger-implementation" thread.

Sorry about that.

/Jonas

Jonas,

Cool. I got a patch ready yesterday which is very close to yours, except it also keeps the state. Thanks for sharing the patch with me. Here is what I have. Please take a look and see. I haven't gone through testing yet though.

Thanks,
-- Shuah

From 3ded425401e5c72dfa161fb4533443bebe287398 Mon Sep 17 00:00:00 2001
From: Shuah Khan <[email protected]>
Date: Mon, 16 Apr 2012 09:23:03 -0600
Subject: [PATCH] leds: add ledtrig-vibrate

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

---
 drivers/leds/ledtrig-vibrate.c |  133 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 133 insertions(+), 0 deletions(-)
 create mode 100644 drivers/leds/ledtrig-vibrate.c

drivers/leds/ledtrig-vibrate.c new file +133 -0

new file mode 100644
index 0000000..9354003
@@ -0,0 +1,133 @@
+/*
+ * LED Kernel Vibrate Trigger
+ *
+ * Copyright (C) 2012 Shuah Khan <[email protected]>
+ *
+ * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
+ * ledtrig-heartbeat.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/ctype.h>
+#include <linux/leds.h>
+#include "leds.h"
+
+static struct vibrate_trig_data {
+	unsigned long vbrate_time;
+	bool vibrate_on;
+	struct timer_list timer;
+};
+
+static void vibrate_timer_function(unsigned long data)
+{
+	struct led_classdev *led_cdev = (struct led_classdev *) data;
+	struct vibrate_trig_data *vibrate_data = led_cdev->trigger_data;
+
+	if (vibrate_data->vibrate_on) {
+		del_timer(vibrate_date->timer);
+		vibrate_data->vibrate_on = false;
+		vibrate_data->vibrate_time = 0;
+	}
+}
+
+static ssize_t led_vibrate_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct vibrate_trig_data *vibrate_data = led_cdev->trigger_data;
+
+	return sprintf(buf, "%lu\n", vibrate_data->vibrate_time);
+}
+
+static ssize_t led_vibrate_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct vibrate_trig_data *vibrate_data = led_cdev->trigger_data;
+	unsigned long state;
+	ssize_t ret = -EINVAL;
+
+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	/* should there be a default max. cap on the time? */
+	if (!vibrate_data->vibrate_on && !state)
+		vibrate_data->vibrate_time = state;
+		if (!led_cdev->brightness_set)
+			led_cdev->brightness_set(led_cdev, LED_ON);
+		vibrate_data->vibrate_on = true;
+		mod_timer(&vibrate_data->timer, jiffies + state);
+	}
+	/* if vibrate_on is true - then ignore this new request? */
+
+	return size;
+}
+
+static DEVICE_ATTR(vibrate, 0644, led_vibrate_show, led_vibrate_store);
+
+static void vibrate_trig_activate(struct led_classdev *led_cdev)
+{
+	int rc;
+	struct vibrate_trig_data *vibrate_data;
+
+	vibrate_data = kzalloc(sizeof(struct vibrate_trig_data), GFP_KERNEL);
+	if (!vibrate_data) {
+		dev_err(led->dev, "unable to allocate vibrate trigger\n");
+		return;
+	}
+	led_cdev->trigger_data = vibrate_data;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_vibrate);
+	if (rc) {
+		dev_err(led->dev, "unable to register vibrate trigger\n");
+		led_cdev->trigger_data = NULL;
+		kfree(vibrate_data);
+	}
+	setup_timer(&vibrate_data->timer, vibrate_timer_function,
+		    (unsigned long) led_cdev);
+	/* vibrate_timer_function(vibrate_data); */
+}
+
+static void vibrate_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct vibrate_trig_data *vibrate_data = led_cdev->trigger_data;
+
+	if (vibrate_data) {
+		device_remove_file(led_cdev->dev, &dev_attr_vibrate);
+		del_timer_sync(&vibrate_data->timer);
+		led_cdev->trigger_data = NULL;
+		kfree(vibrate_data);
+	}
+}
+
+static struct led_trigger vibrate_trigger = {
+	.name     = "vibrate",
+	.activate = vibrate_trig_activate,
+	.deactivate = vibrate_trig_deactivate,
+};
+
+static int __init vibrate_trig_init(void)
+{
+	return led_trigger_register(&vibrate_led_trigger);
+}
+
+static void __exit vibrate_trig_exit(void)
+{
+	led_trigger_unregister(&vibrate_led_trigger);
+}
+
+module_init(vibrate_trig_init);
+module_exit(vibrate_trig_exit);
+
+MODULE_AUTHOR("Shuah Khan <[email protected]>");
+MODULE_DESCRIPTION("vibrate LED trigger");
+MODULE_LICENSE("GPL");

Hi Shuah,

Nice work! If I may comment on this briefly:

i) The semantics of this patch make me cringe. It's bad enough that this is a "LED" trigger; having a "vibrating LED" is even worse. Really, this is a GPIO trigger (which is reasonably logical); secondly, this trigger is about maintaining a transient state given some criteria.

ii) Why force the trigger to a fixed duration? The point, if I understand correctly, is to return to the "rest state" after a given time in case the triggering entity dies; but if the triggering entity (userspace application) is still alive, why not let it prolong the "active state" by triggering again. This is along the lines of the 'kickable' trigger patch I posted earlier.

iii) I don't see the point of maintaining the state. The state is maintained by the fact that you have running timer; if the timer is not running, the state is the "rest state".

I'd suggest:

i) Calling this ledtrig-transient
ii) Having a property 'duration' that determines how long the GPIO/LED/signal is 'active'
iii) Having a property 'activate' (or 'tickle'!) that just sets the 'active' state if it isn't already active; writing to this properly while active just resets the timer to 0 so that another full 'duration' can pass before the LED deactivates.
iv) Maybe having a property 'active-state' to decide whether the LED is on or off when active; this isn't strictly necessary as the LED class already has the active-low property to invert the meaning of the ON state.

This would allow this trigger to be used for a bunch of different use cases:

i) Control of vibrator by userspace app.
ii) Use of LED by userspace app as activity indicator. iii) Use of LED by userspace app as a kind of watchdog indicator -- as long as the app is alive, it can keep the LED illuminated, if it dies the LED will be extinguished automatically.
iv) Use by any userspace app that needs a transient GPIO output.

I hope that makes sense. It mostly comes down to semantics... the implementation is mostly fine, with the exception of maintaining the illumination state that I don't see the need for.

/Jonas

From 80a15b18f6758d2890dfa6b266f2ea4e30338e3c Mon Sep 17 00:00:00 2001

This patch adds a new transient trigger for one shot timer support and is to be used for the following example use cases:

- Control of vibrate (phones, tablets etc.) hardware by userspace app.
- Use of LED by userspace app as activity indicator.
- Use of LED by userspace app as a kind of watchdog indicator -- as long as the app is alive, it can keep the LED illuminated, if it dies the LED will be extinguished automatically.
- Use by any userspace app that needs a transient GPIO output

Transient trigger exports two attributes:
transient_enabled - one shot timer enable mechanism.
1 when enabled, 0 when disabled. enabled state indicates a timer with a value of transient_time running. disabled state indicates no active timer running.
transient_time - one shot timer value. When transient_enabled
is set, transient_time value is used to start a timer that runs once.
When timer expires transient_enabled goes back to disabled state, transient_time is left at the set value to be used when transient is enabled at a future time. This will allow user app to set the time once and enable it to run it once for the specified value as needed.

drivers/leds/ledtrig-transient.c new file +2 -0

+	  If unsure, say Y.
+
 
...

drivers/leds/ledtrig-transient.c new file +7 -0

+	}
+}
+
 
...
 
+static ssize_t led_transient_enabled_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);

drivers/leds/ledtrig-transient.c new file +10 -0

+	/* start timer with transient_time value */
+	if (state == 1 && transient_data->transient_time != 0) {
+		led_cdev->brightness_set(led_cdev, LED_FULL);
+		mod_timer(&transient_data->timer,
+			  jiffies + transient_data->transient_time);
+	}
+
+	return size;
+}
+

drivers/leds/ledtrig-transient.c new file +10 -0

+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	transient_data->transient_time = state;
+
+	return size;
+}
+
 
...
 
+static void transient_trig_deactivate(struct led_classdev *led_cdev)

drivers/leds/ledtrig-transient.c new file +5 -0

+	}
+	printk(KERN_DEBUG "Deativated led transient trigger %s\n",
+		led_cdev->name);
+}
+
 
...

Are there no comments from anyone on this?

Quoted 4 lines

...

drivers/leds/Kconfig config LEDS_TRIGGER_DEFAULT_ON · line 486

Quoted 6 lines

config LEDS_TRIGGER_TRANSIENT

drivers/leds/Kconfig +5 -0

+	tristate "LED Transient Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  This allows one time enable of a transient state on GPIO/PWM based
+	  hadrware.

Make it "This allows one time enabling of a transient state on GPIO/PWM based hardware."

Quoted 6 lines

drivers/leds/Kconfig +2 -0

+	  If unsure, say Y.
+
 
...

drivers/leds/ledtrig-transient.c line 92

Quoted 9 lines

drivers/leds/ledtrig-transient.c new file +9 -0

+static void transient_timer_function(unsigned long data)
+{
+	struct led_classdev *led_cdev = (struct led_classdev *) data;
+	struct transient_trig_data *transient_data = led_cdev->trigger_data;
+
+	if (transient_data->transient_enabled) {
+		transient_data->transient_enabled = 0;
+		led_cdev->brightness_set(led_cdev, LED_OFF);
+		del_timer(&transient_data->timer);

Deleting the timer from within its handler is ... odd. Also it is a bit racy against a concurrent add_timer() on a different CPU.

Quoted 16 lines

drivers/leds/ledtrig-transient.c new file +12 -0

+	}
+}
+
 
...
 
+static ssize_t led_transient_enabled_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct transient_trig_data *transient_data = led_cdev->trigger_data;
+	unsigned long state;
+	ssize_t ret = -EINVAL;
+
+	ret = kstrtoul(buf, 10, &state);

drivers/leds/ledtrig-transient.c line 118

Quoted 5 lines

drivers/leds/ledtrig-transient.c new file +5 -0

+	if (ret)
+		return ret;
+
+	if (state != 1 && state != 0)
+		return ret;

Bug - we'll return 0 here. Use "return -EINVAL" and remove the above initialisation of `ret'.

drivers/leds/ledtrig-transient.c line 122

Quoted 5 lines

drivers/leds/ledtrig-transient.c new file +3 -0

+	/* cancel the running timer */
+	if (state == 0) {
+		transient_timer_function((unsigned long) led_cdev);

And this is perhaps why transient_timer_function() does del_timer().

I suggest it would be cleaner and simpler to do

	transient_data->transient_enabled = 0;
	del_timer(...);

right here.

This is all rather racy in its handling of ->transient_enabled (at least), but afacit the races are harmless.

The typecast is unneeded.

Quoted 14 lines

drivers/leds/ledtrig-transient.c new file +10 -0

+	/* start timer with transient_time value */
+	if (state == 1 && transient_data->transient_time != 0) {
+		led_cdev->brightness_set(led_cdev, LED_FULL);
+		mod_timer(&transient_data->timer,
+			  jiffies + transient_data->transient_time);
+	}
+
+	return size;
+}
+
 
...

drivers/leds/ledtrig-transient.c line 153

Quoted 7 lines

drivers/leds/ledtrig-transient.c new file +7 -0

+static ssize_t led_transient_time_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct transient_trig_data *transient_data = led_cdev->trigger_data;
+	unsigned long state;
+	ssize_t ret = -EINVAL;

Unneeded initialisation.

Quoted 13 lines

drivers/leds/ledtrig-transient.c new file +9 -0

+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	transient_data->transient_time = state;
+
+	return size;
+}
+
 
...

drivers/leds/ledtrig-transient.c line 217

Quoted 11 lines

drivers/leds/ledtrig-transient.c new file +11 -0

+static void transient_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct transient_trig_data *transient_data = led_cdev->trigger_data;
+
+	if (led_cdev->activated) {
+		device_remove_file(led_cdev->dev, &dev_attr_transient_enabled);
+		device_remove_file(led_cdev->dev, &dev_attr_transient_time);
+		del_timer_sync(&transient_data->timer);
+		led_cdev->trigger_data = NULL;
+		led_cdev->activated = false;
+		kfree(transient_data);

OK. But it might be nicer to kill off the timer before doing anything else.

drivers/leds/ledtrig-transient.c

Quoted 9 lines

drivers/leds/ledtrig-transient.c new file +5 -0

+	}
+	printk(KERN_DEBUG "Deativated led transient trigger %s\n",
+		led_cdev->name);
+}
+
 
...

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

This patch adds a new transient trigger for one shot timer support and is to be used for the following example use cases:

- Control of vibrate (phones, tablets etc.) hardware by userspace app.
- Use of LED by userspace app as activity indicator.
- Use of LED by userspace app as a kind of watchdog indicator -- as long as the app is alive, it can keep the LED illuminated, if it dies the LED will be extinguished automatically.
- Use by any userspace app that needs a transient GPIO output

Transient trigger exports two attributes:
transient_enabled - one shot timer enable mechanism.
1 when enabled, 0 when disabled. enabled state indicates a timer with a value of transient_time running. disabled state indicates no active timer running.

transient_time - one shot timer value. When transient_enabled
is set, transient_time value is used to start a timer that runs once.

When timer expires transient_enabled goes back to disabled state, transient_time is left at the set value to be used when transient is enabled at a future time. This will allow user app to set the time once and enable it to run it once for the specified value as needed.

From 80a15b18f6758d2890dfa6b266f2ea4e30338e3c Mon Sep 17 00:00:00 2001
From: Shuah Khan <[email protected]>
Date: Thu, 19 Apr 2012 21:05:36 -0600
Subject: [PATCH 2/2] leds: add new transient trigger for one shot timer
support

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

---
 drivers/leds/Kconfig             |    8 ++
 drivers/leds/Makefile            |    1 +
 drivers/leds/ledtrig-transient.c |  246 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 255 insertions(+), 0 deletions(-)
 create mode 100644 drivers/leds/ledtrig-transient.c

drivers/leds/Kconfig +8 -0

index 589ba02..c6d8006 100644
@@ -478,4 +478,12 @@ config LEDS_TRIGGER_DEFAULT_ON
 comment "iptables trigger is under Netfilter config (LED target)"
 	depends on LEDS_TRIGGERS
 
+config LEDS_TRIGGER_TRANSIENT
+	tristate "LED Transient Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  This allows one time enable of a transient state on GPIO/PWM based
+	  hadrware.
+	  If unsure, say Y.
+
 endif # NEW_LEDS

drivers/leds/Makefile +1 -0

index fa0f428..48d5af7 100644
@@ -57,3 +57,4 @@ obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
 obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)	+= ledtrig-backlight.o
 obj-$(CONFIG_LEDS_TRIGGER_GPIO)		+= ledtrig-gpio.o
 obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
+obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o

drivers/leds/ledtrig-transient.c new file +69 -0

new file mode 100644
index 0000000..e829124
@@ -0,0 +1,246 @@
+/*
+ * LED Kernel Transient Trigger
+ *
+ * Copyright (C) 2012 Shuah Khan <[email protected]>
+ *
+ * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
+ * ledtrig-heartbeat.c
+ * Design and use-case input from Jonas Bonn <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+/*
+* This trigger is intended to be used for the following example use cases:
+
+* - Control of vibrate (phones, tablets etc.) hardware by userspace app.
+* - Use of LED by userspace app as activity indicator.
+* - Use of LED by userspace app as a kind of watchdog indicator -- as
+*	long as the app is alive, it can keep the LED illuminated, if it dies
+*	the LED will be extinguished automatically.
+* - Use by any userspace app that needs a transient GPIO output.
+*
+* Transient trigger exports two attributes:
+*	transient_enabled -	one shot timer enable mechanism.
+*				1 when enabled, 0 when disabled.
+*				enabled state indicates a timer
+*				with a value of transient_time running.
+*				disabled state indicates no active timer
+*				running.
+*	transient_time -	one shot timer value. When transient_enabled
+*				is set, transient_time value is used to start
+*				a timer that runs once.
+*	When timer expires transient_enabled goes back to disabled state,
+*	transient_time is left at the set value to be used when transient
+*	is enabled at a future time. This will allow user app to set the
+*	time once and enable it to run it once for the specified value as
+*	needed.
+*
+*	echo 1 > transient_enabled - starts timer = transient_time when
+*				     transient_time is not 0.
+*	echo 0 > transient_enabled - cancels currently running timer.
+*	echo n > transient_time	   - stores timer value to be used upon next
+*				     enable. Currently active timer if any,
+*				     continues to run for the specified time.
+*	echo 0 > transient_time	   - stores timer value to be used upon next
+*				     enable. Currently active timer if any,
+*				     continues to run for the specified time.
+*
+*	Future enhancements:
+*	1. extending and shortening the timer could be supported either by
+*	   simply modifying the running timer transient_timer is set when
+*	   transient_enabled is in enabled state. For example:
+*	   echo n > transient_time will extend/shorten timer, however this
+*	   could lead race condtions with timer expiry.
+*	2. The same could be done using a third attribute to control
+*	   extending and shortening the timer.
+*
+*	Example use-cases:
+*	echo transient > trigger
+*	echo n > transient_time
+*	repeat the following step as needed:
+*	echo 1 > transient_enabled - start timer = transient_time to run once
+*	echo 1 > transient_enabled - start timer = transient_time to run once
+*	echo none > trigger
+*/
+
+#include <linux/module.h>

drivers/leds/ledtrig-transient.c new file +76 -0

+	unsigned long transient_time;
+	struct timer_list timer;
+};
+
+static void transient_timer_function(unsigned long data)
+{
+	struct led_classdev *led_cdev = (struct led_classdev *) data;
+	struct transient_trig_data *transient_data = led_cdev->trigger_data;
+
+	if (transient_data->transient_enabled) {
+		transient_data->transient_enabled = 0;
+		led_cdev->brightness_set(led_cdev, LED_OFF);
+		del_timer(&transient_data->timer);
+	}
+}
+
+static ssize_t led_transient_enabled_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct transient_trig_data *transient_data = led_cdev->trigger_data;
+
+	return sprintf(buf, "%d\n", transient_data->transient_enabled);
+}
+
+static ssize_t led_transient_enabled_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct transient_trig_data *transient_data = led_cdev->trigger_data;
+	unsigned long state;
+	ssize_t ret = -EINVAL;
+
+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	if (state != 1 && state != 0)
+		return ret;
+
+	/* cancel the running timer */
+	if (state == 0) {
+		transient_timer_function((unsigned long) led_cdev);
+		return size;
+	}
+
+	transient_data->transient_enabled = (int) state;
+
+	/* start timer with transient_time value */
+	if (state == 1 && transient_data->transient_time != 0) {
+		led_cdev->brightness_set(led_cdev, LED_FULL);
+		mod_timer(&transient_data->timer,
+			  jiffies + transient_data->transient_time);
+	}
+
+	return size;
+}
+
+static ssize_t led_transient_time_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct transient_trig_data *transient_data = led_cdev->trigger_data;
+
+	return sprintf(buf, "%lu\n", transient_data->transient_time);
+}
+
+static ssize_t led_transient_time_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct transient_trig_data *transient_data = led_cdev->trigger_data;
+	unsigned long state;
+	ssize_t ret = -EINVAL;
+
+	ret = kstrtoul(buf, 10, &state);

drivers/leds/ledtrig-transient.c new file +81 -0

+static DEVICE_ATTR(transient_time, 0644, led_transient_time_show,
+		   led_transient_time_store);
+
+static void transient_trig_activate(struct led_classdev *led_cdev)
+{
+	int rc;
+	struct transient_trig_data *tdata;
+
+	tdata = kzalloc(sizeof(struct transient_trig_data), GFP_KERNEL);
+	if (!tdata) {
+		dev_err(led_cdev->dev,
+			"unable to allocate transient trigger\n");
+		return;
+	}
+	led_cdev->trigger_data = tdata;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_transient_enabled);
+	if (rc)
+		goto err_out;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_transient_time);
+	if (rc)
+		goto err_out_time;
+
+	setup_timer(&tdata->timer, transient_timer_function,
+		    (unsigned long) led_cdev);
+	led_cdev->activated = true;
+
+	printk(KERN_DEBUG "Ativated led transient trigger %s\n",
+		led_cdev->name);
+
+	return;
+
+err_out_time:
+	device_remove_file(led_cdev->dev, &dev_attr_transient_enabled);
+err_out:
+	dev_err(led_cdev->dev, "unable to register transient trigger\n");
+	led_cdev->trigger_data = NULL;
+	kfree(tdata);
+}
+
+static void transient_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct transient_trig_data *transient_data = led_cdev->trigger_data;
+
+	if (led_cdev->activated) {
+		device_remove_file(led_cdev->dev, &dev_attr_transient_enabled);
+		device_remove_file(led_cdev->dev, &dev_attr_transient_time);
+		del_timer_sync(&transient_data->timer);
+		led_cdev->trigger_data = NULL;
+		led_cdev->activated = false;
+		kfree(transient_data);
+	}
+	printk(KERN_DEBUG "Deativated led transient trigger %s\n",
+		led_cdev->name);
+}
+
+static struct led_trigger transient_trigger = {
+	.name     = "transient",
+	.activate = transient_trig_activate,
+	.deactivate = transient_trig_deactivate,
+};
+
+static int __init transient_trig_init(void)
+{
+	printk(KERN_DEBUG "Registered led transient trigger\n");
+	return led_trigger_register(&transient_trigger);
+}
+
+static void __exit transient_trig_exit(void)
+{
+	led_trigger_unregister(&transient_trigger);
+	printk(KERN_DEBUG "Unregistered led transient trigger\n");
+}
+
+module_init(transient_trig_init);
+module_exit(transient_trig_exit);
+
+MODULE_AUTHOR("Shuah Khan <[email protected]>");
+MODULE_DESCRIPTION("Transient LED trigger");
+MODULE_LICENSE("GPL");

A couple of questions:

i) Why is it important for you to maintain the LED state?

ii) Why do you need to be able to cancel the timer?

As I understand this trigger (and your use-case), this is about being able to turn on a LED (signal) and forget about it, knowing that it will be extinguished in due time without any intervention. So I would leave it at that; don't even present an option to intervene.

That said, there is already a way to intervene: I can always write 0 to 'brightness' to extinguish the led immediately.

Hi Jonas,

activate does sound better and having units in the property name is a good idea. Will make the changes including the 0200 change for activate. Will change the transient_time to transient_time_msec (no problem using duration, just that time is shorter :)

Quoted 4 lines

A couple of questions:

i) Why is it important for you to maintain the LED state?

One use-case that would be easier for user to comprehend would be to have a constant timer that runs periodically. time value doesn't change and by simply writing 1 or 0, timer with a value specified by the time file can be started. The following use-case is a good example for what I am talking about:

I wonder if we should allow control of the brightness during the "on" time as well.
You could set the brightness after enabling the timer, but awkward pauses or races could then leave the "led" permanently on.

Possibly we could hook into led_set_brightness() and restart the timer whenever the brightness was set - and remember the setting.

Quoted 4 lines · NeilBrown

I wonder if we should allow control of the brightness during the "on" time as well.
You could set the brightness after enabling the timer, but awkward pauses or races could then leave the "led" permanently on.

echo transient > trigger
echo 200 > duration
echo 1 > activate

echo 100 > brightness

Worst case, the brightness doesn't get set and you get a 'bright' LED until it expires... but at least it does expire!

Quoted 3 lines

Possibly we could hook into led_set_brightness() and restart the timer whenever the brightness was set - and remember the setting.

An easy way to do this might be to check the brightness setting at the time the timer expires and save this value. Next time the LED is activated, we use this brightness setting instead of FULL_ON. It would be nice to keep this trigger self-contained and not have to hook into led_set_brightness

You make interesting point about using the brightness property as the actual timer trigger, though. That's pretty elegant, but it does require hooking into led_set_brightness.

/Jonas

Quoted 10 lines · On Mon, 23 Apr 2012 07:29:59 +0200 Jonas Bonn <[email protected]> wrote:

Quoted 4 lines · NeilBrown

I wonder if we should allow control of the brightness during the "on" time as well.
You could set the brightness after enabling the timer, but awkward pauses or races could then leave the "led" permanently on.

echo transient > trigger
echo 200 > duration
echo 1 > activate

context switch - lots of IO - time passes, 200ms or more, led gets turned of and then back to:

Quoted 1 line

echo 100 > brightness

led gets turned on and there is nothing to turn it off.

Not a likely case I agree, but not impossible.

The problem is that just setting the brightness will turn the led on independent of the start of the trigger (unless you set the brightness to 0 - that disables the trigger).

From 188e7791c91eb17b1a857dfa7e64a0b8c66247e7 Mon Sep 17 00:00:00 2001

There was a suggestion to change units to seconds similar to "safe_delay_show" and "safe_delay_store" in drivers/md/md.c. safe_delay_show shows time in seconds, however safe_delay_store still takes the value in msecs. This sounded like an asymmetric interface.

Quoted 4 lines · On Wed, 25 Apr 2012 11:42:38 -0600 Shuah Khan <[email protected]> wrote:

There was a suggestion to change units to seconds similar to "safe_delay_show" and "safe_delay_store" in drivers/md/md.c. safe_delay_show shows time in seconds, however safe_delay_store still takes the value in msecs. This sounded like an asymmetric interface.

I don't think you read the code properly. "strict_strtoul_scaled" takes a number like "0.20" and scales it to an integer...
(though your decision not to take that approach may still be justified).

Second version of the patch to add new transient trigger to support one shot timer activation. This trigger has two properties, activate and duration. duration allows setting timer value in msecs. activate allows activating and deactivating the timer specified by duration as needed.

There was a suggestion to change units to seconds similar to "safe_delay_show" and "safe_delay_store" in drivers/md/md.c. safe_delay_show shows time in seconds, however safe_delay_store still takes the value in msecs. This sounded like an asymmetric interface.

I decided to leave duration units as msecs to be consistent with the rest of the triggers in the leds sub-system for now.

I also found couple of bugs in activate store causing it to cancel the timer when it shouldn't and extending the timer when a second activate comes in while timer is running.

Hope I addressed all of the review comments and didn't miss anything. Thanks again for a good discussion and ideas.

From a8249a6738020823277ef02d7787e8078c17844b Mon Sep 17 00:00:00 2001

Hi Jonas,

activate does sound better and having units in the property name is a good idea. Will make the changes including the 0200 change for activate. Will change the transient_time to transient_time_msec (no problem using duration, just that time is shorter :)

Quoted 4 lines

A couple of questions:

i) Why is it important for you to maintain the LED state?

One use-case that would be easier for user to comprehend would be to have a constant timer that runs periodically. time value doesn't change and by simply writing 1 or 0, timer with a value specified by the time file can be started. The following use-case is a good example for what I am talking about:

echo transient > trigger
echo n > transient_time
repeat the following step as needed:
echo 1 > transient_enabled - start timer = transient_time to run once echo 1 > transient_enabled - start timer = transient_time to run once echo none > trigger

This could be accomplished by simply using just the time property, however, each time, the same timer value needs to be echoed. Not a big deal, however time value either needs to stay where it is even when a timer is not running, or clear it 0. I don't like clearing the time value. Here is why. The use-case is very similar to a stop and start timer function on a watch for example. I typically set a time value and then activate and deactivate as needed without needing to reset the time value.

Quoted 2 lines

ii) Why do you need to be able to cancel the timer?

Here is an example where canceling timer is necessary. Phone is in silent mode and a call comes in. When call comes in phone will start vibrating (maybe vibrate mode set for 2 seconds), when caller answers, vibrate mode needs to deactivated right away by stopping the vibrate mode and canceling the timer.

Quoted 6 lines

As I understand this trigger (and your use-case), this is about being able to turn on a LED (signal) and forget about it, knowing that it will be extinguished in due time without any intervention. So I would leave it at that; don't even present an option to intervene.

Quoted 2 lines

That said, there is already a way to intervene: I can always write 0 to 'brightness' to extinguish the led immediately.

That is correct. brightness could be used, however having a distinct name-space might be better for future changes instead of overloading brightness.

Hope this helps explain my reasoning to keep transient state.