[PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[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 <shuahkhan@gmail.com>
Date: Sat, 31 Mar 2012 21:56:07 -0600
Subject: [PATCH] LEDS-One-Shot-Timer-Trigger-implementation

Signed-off-by: Shuah Khan <shuahkhan@gmail.com>
Reviewed-by: NeilBrown <neilb@suse.de>
Cc: Richard Purdie <rpurdie@linux.intel.com>
---
 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

diff --git a/Documentation/leds/leds-one-shot-timer.txt b/Documentation/leds/leds-one-shot-timer.txt
new file mode 100644
index 0000000..a5429dd
--- /dev/null
+++ b/Documentation/leds/leds-one-shot-timer.txt
@@ -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.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 5bff843..ed123ba 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -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));
 }
 
 /**
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index d686004..419b0bc 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -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);
 
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index e77c7f8..b2cda9f 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -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)
 {
diff --git a/drivers/leds/ledtrig-timer.c b/drivers/leds/ledtrig-timer.c
index 328c64c..e323cf2 100644
--- a/drivers/leds/ledtrig-timer.c
+++ b/drivers/leds/ledtrig-timer.c
@@ -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);
-- 
1.7.5.4

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Shuah Khan]

Andrew,

How about this patch?

-- Shuah

On Sun, 2012-04-01 at 13:53 -0600, Shuah Khan 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.
> 
> 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 <shuahkhan@gmail.com>
> Date: Sat, 31 Mar 2012 21:56:07 -0600
> Subject: [PATCH] LEDS-One-Shot-Timer-Trigger-implementation
> 
> Signed-off-by: Shuah Khan <shuahkhan@gmail.com>
> Reviewed-by: NeilBrown <neilb@suse.de>
> Cc: Richard Purdie <rpurdie@linux.intel.com>
> ---
>  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
> 
> diff --git a/Documentation/leds/leds-one-shot-timer.txt b/Documentation/leds/leds-one-shot-timer.txt
> new file mode 100644
> index 0000000..a5429dd
> --- /dev/null
> +++ b/Documentation/leds/leds-one-shot-timer.txt
> @@ -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.
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 5bff843..ed123ba 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -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));
>  }
>  
>  /**
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index d686004..419b0bc 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -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);
>  
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index e77c7f8..b2cda9f 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -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)
>  {
> diff --git a/drivers/leds/ledtrig-timer.c b/drivers/leds/ledtrig-timer.c
> index 328c64c..e323cf2 100644
> --- a/drivers/leds/ledtrig-timer.c
> +++ b/drivers/leds/ledtrig-timer.c
> @@ -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);

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Andrew Morton]

On Sun, 01 Apr 2012 13:53:59 -0600
Shuah Khan <shuahkhan@gmail.com> 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.
> 
> 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 <shuahkhan@gmail.com>
> 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.

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

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

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

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

>
> ...
>
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -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?

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

Is seems so...

> ...
>
> --- a/drivers/leds/ledtrig-timer.c
> +++ b/drivers/leds/ledtrig-timer.c
> @@ -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.

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

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

> +		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()?

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

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

More dittoes here.

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

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

> +}
>  
>
> ...
>

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Shuah Khan]

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

I initiated a discussion on ce-android mainlining mailing list and after
some back and forth, the solution to enhance leds infrastructure to
support one shot timer emerged as the preferred one to address the need
in a generic way. Adding this feature to leds infrastructure allows
ledgpio as well as ledpwm drivers use it. This approach allows us to
extend the existing infrastructure as well as, avoid adding two new
drivers. Even though leds is kind of an odd place for vibrate feature in
that it is counter intuitive, looking at its infrastructure, it made
sense to leverage what it already does and enhance it.

If are interested in reading the discussion, here is the link that
starts the discussion:

http://lists.linuxfoundation.org/pipermail/ce-android-mainline/2012-February/000077.html

Hope this helps understand what I am trying to accomplish. Please let me
know after this explanation, if you think adding this feature is heading
in the right direction to address this need? Other ideas are welcome.

As per the specific comments on the code it self, here is what I will do
while we discuss this feature,

1. Cleanup simple_strtoul() in leds - in addition to the checkpatch
warnings this patch triggered, there is at least one other use of
simple_strtoul() that needs cleanup. I will send a patch to do that,
while we discuss the next steps with this feature.

Please see my responses on this patch in-line:
> 
> > +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".
> 
> > +- 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
> 
> Similarly, it's odd to refer to "adding" things when describing an
> already-existing kernel feature!
> 
> >
> > ...
> >
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -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?
> 
> > +		mod_timer(&led_cdev->blink_timer,
> > +			jiffies + msecs_to_jiffies(delay));
> >  }
> 
> Is seems so...
Let me see, the timer is not set when delay == LED_TIMER_FOREVER. Am I
missing something?

> 
> > ...
> >
> > --- a/drivers/leds/ledtrig-timer.c
> > +++ b/drivers/leds/ledtrig-timer.c
> > @@ -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.

I had some reservations with overloading and using a string, however
using forever would help hide the internal implementation details. That
said I can go either way.

> 
> > @@ -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);
> 
> You ignored the checkpatch warning.  Please don't.  Take the
> opportunity to fix things up when altering existing code!
> 
> > +		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()?

Yes I can do that.
> 
> >  	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.
> 
> > @@ -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;
> > +		}
> >  	}
> 
> More dittoes here.
> 
> >  	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;
> 
> yikes.  What is the led core code doing fiddling around with an
> open-coded (void *)1?

It is being used to save state. Looked strange, however I decided to not
touch it as part of this feature. :)

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

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Dmitry Torokhov]

Hi Shuah,

On Sat, Apr 07, 2012 at 08:13:44AM -0600, Shuah Khan wrote:
> 
> > > +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.

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

Thanks.

-- 
Dmitry

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2414 bytes --]

On Sat, 7 Apr 2012 14:56:41 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com>
wrote:

> Hi Shuah,
> 
> On Sat, Apr 07, 2012 at 08:13:44AM -0600, Shuah Khan wrote:
> > 
> > > > +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.
> 
> So why do not you use memoryless force feedback framework that other
> devices use (see drivers/input/misc/*vibra.c drivers).
> 
> 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?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Dmitry Torokhov]

On Mon, Apr 09, 2012 at 09:42:19AM +1000, NeilBrown wrote:
> On Sat, 7 Apr 2012 14:56:41 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com>
> wrote:
> 
> > Hi Shuah,
> > 
> > On Sat, Apr 07, 2012 at 08:13:44AM -0600, Shuah Khan wrote:
> > > 
> > > > > +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.
> > 
> > So why do not you use memoryless force feedback framework that other
> > devices use (see drivers/input/misc/*vibra.c drivers).
> > 
> > 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.

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

> 
> 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?

No, it is not.

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

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

By specifying replay->length = 120

Thanks.

-- 
Dmitry

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1649 bytes --]

On Sun, 8 Apr 2012 17:06:46 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com>
wrote:

> On Mon, Apr 09, 2012 at 09:42:19AM +1000, NeilBrown wrote:

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

> 
> 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?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Dmitry Torokhov]

On Tue, Apr 10, 2012 at 08:25:06AM +1000, NeilBrown wrote:
> On Sun, 8 Apr 2012 17:06:46 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com>
> wrote:
> 
> > On Mon, Apr 09, 2012 at 09:42:19AM +1000, NeilBrown wrote:
> 
> > > 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.
> > 
> > > 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.
> > 
> > > 
> > > 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:
> 
> > 
> > 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?

I got convinced that 3-axis accelerometers should be included in input
devices, at least until we have a reliable bridge between iio and input.

If you take a look into drivers/input/misc you will find a few of them
there.

Thanks.

-- 
Dmitry

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Shuah Khan]

On Sat, 2012-04-07 at 14:56 -0700, Dmitry Torokhov wrote:
> Hi Shuah,
> 
> On Sat, Apr 07, 2012 at 08:13:44AM -0600, Shuah Khan wrote:
> > 
> > > > +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.
> 
> 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?

-- Shuah

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Dmitry Torokhov]

On Mon, Apr 09, 2012 at 10:55:49AM -0600, Shuah Khan wrote:
> On Sat, 2012-04-07 at 14:56 -0700, Dmitry Torokhov wrote:
> > Hi Shuah,
> > 
> > On Sat, Apr 07, 2012 at 08:13:44AM -0600, Shuah Khan wrote:
> > > 
> > > > > +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.
> > 
> > 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?

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.

-- 
Dmitry

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Shuah Khan]

On Mon, 2012-04-09 at 10:37 -0700, Dmitry Torokhov wrote:
> On Mon, Apr 09, 2012 at 10:55:49AM -0600, Shuah Khan wrote:
> > On Sat, 2012-04-07 at 14:56 -0700, Dmitry Torokhov wrote:
> > > Hi Shuah,
> > > 
> > > On Sat, Apr 07, 2012 at 08:13:44AM -0600, Shuah Khan wrote:
> > > > 
> > > > > > +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.
> > > 
> > > 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?
> 
> 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.

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.

-- Shuah

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Dmitry Torokhov]

On Mon, Apr 09, 2012 at 12:16:05PM -0600, Shuah Khan wrote:
> On Mon, 2012-04-09 at 10:37 -0700, Dmitry Torokhov wrote:
> > On Mon, Apr 09, 2012 at 10:55:49AM -0600, Shuah Khan wrote:
> > > On Sat, 2012-04-07 at 14:56 -0700, Dmitry Torokhov wrote:
> > > > Hi Shuah,
> > > > 
> > > > On Sat, Apr 07, 2012 at 08:13:44AM -0600, Shuah Khan wrote:
> > > > > 
> > > > > > > +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.
> > > > 
> > > > 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?
> > 
> > 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.
> 
> 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.

-- 
Dmitry

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Shuah Khan]

On Mon, 2012-04-09 at 11:45 -0700, Dmitry Torokhov wrote:
> On Mon, Apr 09, 2012 at 12:16:05PM -0600, Shuah Khan wrote:
> > On Mon, 2012-04-09 at 10:37 -0700, Dmitry Torokhov wrote:
> > > On Mon, Apr 09, 2012 at 10:55:49AM -0600, Shuah Khan wrote:
> > > > On Sat, 2012-04-07 at 14:56 -0700, Dmitry Torokhov wrote:
> > > > > Hi Shuah,
> > > > > 
> > > > > On Sat, Apr 07, 2012 at 08:13:44AM -0600, Shuah Khan wrote:
> > > > > > 
> > > > > > > > +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.
> > > > > 
> > > > > 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?
> > > 
> > > 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.
> > 
> > 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.
> 

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?

-- Shuah 

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Dmitry Torokhov]

On Mon, Apr 09, 2012 at 02:20:47PM -0600, Shuah Khan wrote:
> On Mon, 2012-04-09 at 11:45 -0700, Dmitry Torokhov wrote:
> > On Mon, Apr 09, 2012 at 12:16:05PM -0600, Shuah Khan wrote:
> > > On Mon, 2012-04-09 at 10:37 -0700, Dmitry Torokhov wrote:
> > > > On Mon, Apr 09, 2012 at 10:55:49AM -0600, Shuah Khan wrote:
> > > > > On Sat, 2012-04-07 at 14:56 -0700, Dmitry Torokhov wrote:
> > > > > > Hi Shuah,
> > > > > > 
> > > > > > On Sat, Apr 07, 2012 at 08:13:44AM -0600, Shuah Khan wrote:
> > > > > > > 
> > > > > > > > > +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.
> > > > > > 
> > > > > > 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?
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> 
> 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?
> 

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.

-- 
Dmitry

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Shuah Khan]

On Mon, 2012-04-09 at 13:42 -0700, Dmitry Torokhov wrote:
> On Mon, Apr 09, 2012 at 02:20:47PM -0600, Shuah Khan wrote:
> > On Mon, 2012-04-09 at 11:45 -0700, Dmitry Torokhov wrote:
> > > On Mon, Apr 09, 2012 at 12:16:05PM -0600, Shuah Khan wrote:
> > > > On Mon, 2012-04-09 at 10:37 -0700, Dmitry Torokhov wrote:
> > > > > On Mon, Apr 09, 2012 at 10:55:49AM -0600, Shuah Khan wrote:
> > > > > > On Sat, 2012-04-07 at 14:56 -0700, Dmitry Torokhov wrote:
> > > > > > > Hi Shuah,
> > > > > > > 
> > > > > > > On Sat, Apr 07, 2012 at 08:13:44AM -0600, Shuah Khan wrote:
> > > > > > > > 
> > > > > > > > > > +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.
> > > > > > > 
> > > > > > > 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?
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > 
> > 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?
> > 
> 
> 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.
> 

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.

-- Shuah

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Dmitry Torokhov]

On Mon, Apr 09, 2012 at 04:40:29PM -0600, Shuah Khan wrote:
> On Mon, 2012-04-09 at 13:42 -0700, Dmitry Torokhov wrote:
> > On Mon, Apr 09, 2012 at 02:20:47PM -0600, Shuah Khan wrote:
> > > On Mon, 2012-04-09 at 11:45 -0700, Dmitry Torokhov wrote:
> > > > On Mon, Apr 09, 2012 at 12:16:05PM -0600, Shuah Khan wrote:
> > > > > On Mon, 2012-04-09 at 10:37 -0700, Dmitry Torokhov wrote:
> > > > > > On Mon, Apr 09, 2012 at 10:55:49AM -0600, Shuah Khan wrote:
> > > > > > > On Sat, 2012-04-07 at 14:56 -0700, Dmitry Torokhov wrote:
> > > > > > > > Hi Shuah,
> > > > > > > > 
> > > > > > > > On Sat, Apr 07, 2012 at 08:13:44AM -0600, Shuah Khan wrote:
> > > > > > > > > 
> > > > > > > > > > > +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.
> > > > > > > > 
> > > > > > > > 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?
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > 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.
> > > > 
> > > 
> > > 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?
> > > 
> > 
> > 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.
> > 
> 
> 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.

-- 
Dmitry


Input: add support for PWM-based vibrators

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/misc/Kconfig         |   11 ++
 drivers/input/misc/Makefile        |    1 
 drivers/input/misc/pwm-vibrator.c  |  213 ++++++++++++++++++++++++++++++++++++
 include/linux/input/pwm-vibrator.h |   13 ++
 4 files changed, 238 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/misc/pwm-vibrator.c
 create mode 100644 include/linux/input/pwm-vibrator.h


diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index ee077a4..913bdb8 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -451,6 +451,17 @@ config INPUT_PWM_BEEPER
 	  To compile this driver as a module, choose M here: the module will be
 	  called pwm-beeper.
 
+config INPUT_PWM_VIBRATOR
+	tristate "PWM vibrator support"
+	depends on HAVE_PWM
+	help
+	  Say Y here to get support for PWM based vibrator devices.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called pwm-vibrator.
+
 config INPUT_GPIO_ROTARY_ENCODER
 	tristate "Rotary encoders connected to GPIO pins"
 	depends on GPIOLIB && GENERIC_GPIO
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index f55cdf4..e8b7643 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_INPUT_PM8XXX_VIBRATOR)	+= pm8xxx-vibrator.o
 obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY)	+= pmic8xxx-pwrkey.o
 obj-$(CONFIG_INPUT_POWERMATE)		+= powermate.o
 obj-$(CONFIG_INPUT_PWM_BEEPER)		+= pwm-beeper.o
+obj-$(CONFIG_INPUT_PWM_VIBRATOR)	+= pwm-vibrator.o
 obj-$(CONFIG_INPUT_RB532_BUTTON)	+= rb532_button.o
 obj-$(CONFIG_INPUT_GPIO_ROTARY_ENCODER)	+= rotary_encoder.o
 obj-$(CONFIG_INPUT_SGI_BTNS)		+= sgi_btns.o
diff --git a/drivers/input/misc/pwm-vibrator.c b/drivers/input/misc/pwm-vibrator.c
new file mode 100644
index 0000000..c72bac9
--- /dev/null
+++ b/drivers/input/misc/pwm-vibrator.c
@@ -0,0 +1,213 @@
+/*
+ *  PWM vibrator driver
+ *
+ *  Copyright (C) 2012 Dmitry Torokhov <dmitry.torokhov@gmail.com>
+ *
+ *  Based on PWM beeper driver:
+ *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under  the terms of the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the License, or (at your
+ *  option) any later version.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#include <linux/input.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+#include <linux/input/pwm-vibrator.h>
+
+struct pwm_vibrator {
+	struct input_dev *input;
+	struct pwm_device *pwm;
+	unsigned int pwm_period;
+	unsigned int level;
+};
+
+static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
+{
+	unsigned int duty = vibrator->pwm_period * vibrator->level / 100;
+	int error;
+
+	error = pwm_config(vibrator->pwm, duty, vibrator->pwm_period);
+	if (error < 0)
+		return error;
+
+	error = pwm_enable(vibrator->pwm);
+	if (error < 0)
+		return error;
+
+	return 0;
+}
+
+static void pwm_vibrator_stop(struct pwm_vibrator *vibrator)
+{
+	pwm_config(vibrator->pwm, 0, 0);
+	pwm_disable(vibrator->pwm);
+}
+
+static int pwm_vibrator_play_effect(struct input_dev *dev, void *data,
+				    struct ff_effect *effect)
+{
+	struct pwm_vibrator *vibrator = input_get_drvdata(dev);
+	int error;
+
+	vibrator->level = effect->u.rumble.strong_magnitude;
+	if (!vibrator->level)
+		vibrator->level = effect->u.rumble.weak_magnitude;
+
+	if (vibrator->level) {
+		error = pwm_vibrator_start(vibrator);
+		if (error) {
+			dev_err(vibrator->input->dev.parent,
+				"%s: pwm_vibrator_start failed, error: %d\n",
+				__func__, error);
+			return error;
+		}
+	} else {
+		pwm_vibrator_stop(vibrator);
+	}
+
+	return 0;
+}
+
+
+static int __devinit pwm_vibrator_probe(struct platform_device *pdev)
+{
+	const struct pwm_vibrator_platform_data *pdata;
+	struct pwm_vibrator *vibrator;
+	struct input_dev *input;
+	int error;
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		dev_err(&pdev->dev, "Missing platform dat\n");
+		return -EINVAL;
+	}
+
+	vibrator = kzalloc(sizeof(*vibrator), GFP_KERNEL);
+	input = input_allocate_device();
+	if (!vibrator || !input) {
+		error = -ENOMEM;
+		goto err_mem_free;
+	}
+
+	vibrator->input = input;
+	vibrator->pwm_period = pdata->pwm_period;
+
+	vibrator->pwm = pwm_request(pdata->pwm_id, "pwm vibrator");
+	if (IS_ERR(vibrator->pwm)) {
+		error = PTR_ERR(vibrator->pwm);
+		dev_err(&pdev->dev, "Failed to request pwm device: %d\n", error);
+		goto err_mem_free;
+	}
+
+	input->name = "pwm-vibrator";
+	input->id.bustype = BUS_HOST;
+	input->dev.parent = &pdev->dev;
+
+	input_set_drvdata(input, vibrator);
+	input_set_capability(input, EV_FF, FF_RUMBLE);
+
+	error = input_ff_create_memless(input, NULL,
+					pwm_vibrator_play_effect);
+	if (error) {
+		dev_err(&pdev->dev,
+			"Unable to create FF device, error: %d\n",
+			error);
+		goto err_pwm_free;
+	}
+
+	error = input_register_device(input);
+	if (error) {
+		dev_err(&pdev->dev, "Failed to register input device: %d\n", error);
+		goto err_pwm_free;
+	}
+
+	platform_set_drvdata(pdev, vibrator);
+
+	return 0;
+
+err_pwm_free:
+	pwm_free(vibrator->pwm);
+err_mem_free:
+	input_free_device(vibrator->input);
+	kfree(vibrator);
+
+	return error;
+}
+
+static int __devexit pwm_vibrator_remove(struct platform_device *pdev)
+{
+	struct pwm_vibrator *vibrator = platform_get_drvdata(pdev);
+
+	platform_set_drvdata(pdev, NULL);
+
+	input_unregister_device(vibrator->input);
+
+	pwm_disable(vibrator->pwm);
+	pwm_free(vibrator->pwm);
+
+	kfree(vibrator);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int pwm_vibrator_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct pwm_vibrator *vibrator = platform_get_drvdata(pdev);
+	struct input_dev *input = vibrator->input;
+	unsigned long flags;
+
+	spin_lock_irqsave(&input->event_lock, flags);
+	if (vibrator->level)
+		pwm_vibrator_stop(vibrator);
+	spin_unlock_irqrestore(&input->event_lock, flags);
+
+	return 0;
+}
+
+static int pwm_vibrator_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct pwm_vibrator *vibrator = platform_get_drvdata(pdev);
+	struct input_dev *input = vibrator->input;
+	unsigned long flags;
+
+	spin_lock_irqsave(&input->event_lock, flags);
+	if (vibrator->level)
+		pwm_vibrator_start(vibrator);
+	spin_unlock_irqrestore(&input->event_lock, flags);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(pwm_vibrator_pm_ops,
+			 pwm_vibrator_suspend, pwm_vibrator_resume);
+
+static struct platform_driver pwm_vibrator_driver = {
+	.probe	= pwm_vibrator_probe,
+	.remove	= __devexit_p(pwm_vibrator_remove),
+	.driver	= {
+		.name	= "pwm-vibrator",
+		.owner	= THIS_MODULE,
+		.pm	= &pwm_vibrator_pm_ops,
+	},
+};
+module_platform_driver(pwm_vibrator_driver);
+
+MODULE_AUTHOR("Dmitry Torokhov <dmitry.torokhov@gmail.com>");
+MODULE_DESCRIPTION("PWM vibrator driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:pwm-vibrator");
diff --git a/include/linux/input/pwm-vibrator.h b/include/linux/input/pwm-vibrator.h
new file mode 100644
index 0000000..efe1f72
--- /dev/null
+++ b/include/linux/input/pwm-vibrator.h
@@ -0,0 +1,13 @@
+#ifndef __INPUT_PWM_VIBRATOR_H
+#define __INPUT_PWM_VIBRATOR_H
+
+/*
+ * PWM vibrator driver data
+ */
+
+struct pwm_vibrator_platform_data {
+	unsigned int pwm_id;
+	unsigned int pwm_period;
+};
+
+#endif

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Shuah Khan]

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

Dmitry,

Thanks - this will be good base to start. 

-- Shuah

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4789 bytes --]

On Fri, 6 Apr 2012 16:53:53 -0700 Andrew Morton <akpm@linux-foundation.org>
wrote:

> On Sun, 01 Apr 2012 13:53:59 -0600
> Shuah Khan <shuahkhan@gmail.com> 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.
> > 
> > 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 <shuahkhan@gmail.com>
> > 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.
> 
> >
> > ...
> >
> > +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?

We use 'scsi' devices to drive SATA disks.
We use 'input' devices to beep the PC 'speaker'.

It is no great deviation from this pattern to use a 'led' device to trigger a
vibrator.

We could rename all of these subsystems, but that probably won't win us any
friends.
It may well make sense to create a 'vibrator' class which shared 99% of the
code of the 'led' class.  This is a bit like the 'backlight' class which also
drives LEDs (sometimes) and sets their brightness.  It is presumably helpful
for user-space to see the difference intention of the LEDS - some for display
backlight and some for signalling - and so also helpful to differentiate
output devices - some emit light, some emit kinetic energy.

It would be really nice to have a system-wide holistic way to address these
classification issues.  But as we tend to work in an incremental-development
approach, it is easiest just to add functionality where it seems to fit.

> > ...
> >
> > --- a/drivers/leds/ledtrig-timer.c
> > +++ b/drivers/leds/ledtrig-timer.c
> > @@ -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.
> 

I don't think it is very risky.  The output will continue to always be a
decimal string unless user-space explicitly chooses to write "forever".
So the risky step is to modify part of userspace to write 'forever' without
modifying other parts to recognise 'forever'.  I doubt anyone would actually
do that but in any case it is not the place of the kernel to care.  The
kernel change is perfectly safe (Well.... it would be if we trapped an
attempt to write 'MAXINT' and converted  it to 'MAXINT-1' - that is trivial
to do and then we would be perfectly safe).

However it might make sense to discard the 'forever' and instead introduce a
'repeat_count' attribute which defaults to 'forever' but could be explicitly
set to '1' (or 2 or 3).
I didn't advocate that at first as it seemed a bigger change than needed (and
incremental development is good), but maybe it is better to take that bigger
step.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Richard Purdie]

On Sun, 2012-04-01 at 13:53 -0600, Shuah Khan 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.
> 
> 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 <shuahkhan@gmail.com>
> Date: Sat, 31 Mar 2012 21:56:07 -0600
> Subject: [PATCH] LEDS-One-Shot-Timer-Trigger-implementation
> 
> Signed-off-by: Shuah Khan <shuahkhan@gmail.com>
> Reviewed-by: NeilBrown <neilb@suse.de>
> Cc: Richard Purdie <rpurdie@linux.intel.com>
> ---
>  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
> 
> diff --git a/Documentation/leds/leds-one-shot-timer.txt b/Documentation/leds/leds-one-shot-timer.txt
> new file mode 100644
> index 0000000..a5429dd
> --- /dev/null
> +++ b/Documentation/leds/leds-one-shot-timer.txt
> @@ -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.

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

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Shuah Khan]

On Tue, 2012-04-10 at 14:24 +0100, Richard Purdie wrote:
> On Sun, 2012-04-01 at 13:53 -0600, Shuah Khan 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.
> > 
> > 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 <shuahkhan@gmail.com>
> > Date: Sat, 31 Mar 2012 21:56:07 -0600
> > Subject: [PATCH] LEDS-One-Shot-Timer-Trigger-implementation
> > 
> > Signed-off-by: Shuah Khan <shuahkhan@gmail.com>
> > Reviewed-by: NeilBrown <neilb@suse.de>
> > Cc: Richard Purdie <rpurdie@linux.intel.com>
> > ---
> >  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
> > 
> > diff --git a/Documentation/leds/leds-one-shot-timer.txt b/Documentation/leds/leds-one-shot-timer.txt
> > new file mode 100644
> > index 0000000..a5429dd
> > --- /dev/null
> > +++ b/Documentation/leds/leds-one-shot-timer.txt
> > @@ -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.
> 
> 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,
};
> 
> 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?

Please see above. I can send that patch for draft review if you would
like to see it. I haven't done a lot of testing on that patch and also I
think I have a few missing pieces. But I am open to either approach.

> 
> 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? 

Thanks,
-- Shuah
> 
> Cheers,
> 
> Richard
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Richard Purdie]

On Tue, 2012-04-10 at 09:31 -0600, Shuah Khan wrote:
> On Tue, 2012-04-10 at 14:24 +0100, Richard Purdie wrote:
> > 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,
> };

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.

> > 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? 

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

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Shuah Khan]

On Wed, 2012-04-11 at 11:05 +0100, Richard Purdie wrote:
> On Tue, 2012-04-10 at 09:31 -0600, Shuah Khan wrote:
> > 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.

This patch adds code to register a new trigger named timer-no-default
and uses delay_on and delay_off to set the timer once. timer is not set
when delay == LED_TIMER_FOREVER. Are you concerned about overloading
blink_delay_on and blink_delay_off values specifically?

> 
> > > 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? 
> 
> I'd first like to understand why this couldn't be a separate trigger,
> then we can understand the alternatives we're comparing.

Yes. No problem doing that.

-- Shuah
> 
> Cheers,
> 
> Richard
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH RESEND] LEDS-One-Shot-Timer-Trigger-implementation

[Shuah Khan]

> 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?

I finally :) understand your question about why I didn't add a new
trigger. I don't see any reason why I should a new trigger should not be
added and it does make it clean without no backwards compatibility
issues. I will get working on that and get back to you.

Thanks,
-- Shuah

[PATCH 1/1] leds: add "kickable" LED trigger

[Jonas Bonn]

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 <jonas@southpole.se>
---

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

diff --git a/drivers/leds/ledtrig-kickable.c b/drivers/leds/ledtrig-kickable.c
new file mode 100644
index 0000000..50699b1
--- /dev/null
+++ b/drivers/leds/ledtrig-kickable.c
@@ -0,0 +1,111 @@
+/*
+ * LED Kernel "Kickable" Trigger
+ *
+ * Copyright 2012 Jonas Bonn <jonas@southpole.se>
+ *
+ * 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 <jonas@southpole.se>");
+MODULE_DESCRIPTION("Kickable LED trigger");
+MODULE_LICENSE("GPL");
-- 
1.7.0.4

Re: [PATCH 1/1] leds: add "kickable" LED trigger

[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

On 16 April 2012 00:34, Jonas Bonn <jonas@southpole.se> wrote:
>
> 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 <jonas@southpole.se>
> ---
>
> 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
>
> diff --git a/drivers/leds/ledtrig-kickable.c b/drivers/leds/ledtrig-kickable.c
> new file mode 100644
> index 0000000..50699b1
> --- /dev/null
> +++ b/drivers/leds/ledtrig-kickable.c
> @@ -0,0 +1,111 @@
> +/*
> + * LED Kernel "Kickable" Trigger
> + *
> + * Copyright 2012 Jonas Bonn <jonas@southpole.se>
> + *
> + * 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 <jonas@southpole.se>");
> +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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Jonas Bonn
Stockholm, Sweden

Re: [PATCH 1/1] leds: add "kickable" LED trigger

[Shuah Khan]

On Mon, 2012-04-16 at 00:37 +0200, Jonas Bonn wrote:
> 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 <shuahkhan@gmail.com>
Date: Mon, 16 Apr 2012 09:23:03 -0600
Subject: [PATCH] leds: add ledtrig-vibrate


Signed-off-by: Shuah Khan <shuahkhan@gmail.com>
---
 drivers/leds/ledtrig-vibrate.c |  133 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 133 insertions(+), 0 deletions(-)
 create mode 100644 drivers/leds/ledtrig-vibrate.c

diff --git a/drivers/leds/ledtrig-vibrate.c b/drivers/leds/ledtrig-vibrate.c
new file mode 100644
index 0000000..9354003
--- /dev/null
+++ b/drivers/leds/ledtrig-vibrate.c
@@ -0,0 +1,133 @@
+/*
+ * LED Kernel Vibrate Trigger
+ *
+ * Copyright (C) 2012 Shuah Khan <shuahkhan@gmail.com>
+ *
+ * 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 <shuahkhan@gmail.com>");
+MODULE_DESCRIPTION("vibrate LED trigger");
+MODULE_LICENSE("GPL");
-- 
1.7.5.4



> 
> On 16 April 2012 00:34, Jonas Bonn <jonas@southpole.se> wrote:
> >
> > 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 <jonas@southpole.se>
> > ---
> >
> > 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
> >
> > diff --git a/drivers/leds/ledtrig-kickable.c b/drivers/leds/ledtrig-kickable.c
> > new file mode 100644
> > index 0000000..50699b1
> > --- /dev/null
> > +++ b/drivers/leds/ledtrig-kickable.c
> > @@ -0,0 +1,111 @@
> > +/*
> > + * LED Kernel "Kickable" Trigger
> > + *
> > + * Copyright 2012 Jonas Bonn <jonas@southpole.se>
> > + *
> > + * 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 <jonas@southpole.se>");
> > +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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> 
> 

Re: [PATCH 1/1] leds: add "kickable" LED trigger

[Jonas Bonn]

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

On Mon, 2012-04-16 at 09:28 -0600, Shuah Khan wrote:
> On Mon, 2012-04-16 at 00:37 +0200, Jonas Bonn wrote:
> > 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 <shuahkhan@gmail.com>
> Date: Mon, 16 Apr 2012 09:23:03 -0600
> Subject: [PATCH] leds: add ledtrig-vibrate
> 
> 
> Signed-off-by: Shuah Khan <shuahkhan@gmail.com>
> ---
>  drivers/leds/ledtrig-vibrate.c |  133 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 133 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/leds/ledtrig-vibrate.c
> 
> diff --git a/drivers/leds/ledtrig-vibrate.c b/drivers/leds/ledtrig-vibrate.c
> new file mode 100644
> index 0000000..9354003
> --- /dev/null
> +++ b/drivers/leds/ledtrig-vibrate.c
> @@ -0,0 +1,133 @@
> +/*
> + * LED Kernel Vibrate Trigger
> + *
> + * Copyright (C) 2012 Shuah Khan <shuahkhan@gmail.com>
> + *
> + * 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 <shuahkhan@gmail.com>");
> +MODULE_DESCRIPTION("vibrate LED trigger");
> +MODULE_LICENSE("GPL");

Re: [PATCH 1/1] leds: add "kickable" LED trigger

[Shuah Khan]

On Tue, 2012-04-17 at 00:33 +0200, Jonas Bonn wrote:
> 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

Jonas,

Thanks for the feedback. I better change the name before too long :)
Thanks for summarizing the use-cases. I like your duration and activate
idea. I am testing the patch now and will incorporate your ideas. I
would like to include you in the thread when I send it out if you don't
mind. Thanks again,

-- Shuah

[PATCH ] leds: add new transient trigger for one shot timer support

[Shuah Khan]

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 <shuahkhan@gmail.com>
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 <shuahkhan@gmail.com>
---
 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

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 589ba02..c6d8006 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -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
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index fa0f428..48d5af7 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -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
diff --git a/drivers/leds/ledtrig-transient.c b/drivers/leds/ledtrig-transient.c
new file mode 100644
index 0000000..e829124
--- /dev/null
+++ b/drivers/leds/ledtrig-transient.c
@@ -0,0 +1,246 @@
+/*
+ * LED Kernel Transient Trigger
+ *
+ * Copyright (C) 2012 Shuah Khan <shuahkhan@gmail.com>
+ *
+ * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
+ * ledtrig-heartbeat.c
+ * Design and use-case input from Jonas Bonn <jonas@southpole.se>
+ *
+ * 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>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/timer.h>
+#include <linux/leds.h>
+#include "leds.h"
+
+struct transient_trig_data {
+	int transient_enabled;
+	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);
+	if (ret)
+		return ret;
+
+	transient_data->transient_time = state;
+
+	return size;
+}
+
+static DEVICE_ATTR(transient_enabled, 0644, led_transient_enabled_show,
+		   led_transient_enabled_store);
+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 <shuahkhan@gmail.com>");
+MODULE_DESCRIPTION("Transient LED trigger");
+MODULE_LICENSE("GPL");
-- 
1.7.5.4

Re: [PATCH ] leds: add new transient trigger for one shot timer support

[Andrew Morton]

On Thu, 19 Apr 2012 22:04:34 -0600
Shuah Khan <shuahkhan@gmail.com> wrote:

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

Are there no comments from anyone on this?

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

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

> +	  If unsure, say Y.
> +
>
> ...
>
> +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.

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

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


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


> +		return size;
> +	}
> +
> +	transient_data->transient_enabled = (int) state;

The typecast is unneeded.

> +	/* 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_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.

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

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

Re: [PATCH ] leds: add new transient trigger for one shot timer support

[Shuah Khan]

> 
> Are there no comments from anyone on this?
Thanks for the comments.
> 
> >
> > ...
> >
> > 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.
> 
> Make it "This allows one time enabling of a transient state on GPIO/PWM
> based hardware."
Will do.
> 
> > +	  If unsure, say Y.
> > +
> >
> > ...
> >
> > +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.
Good point. Will fix.
> 
> > +	}
> > +}
> > +
> >
> > ...
> >
> > +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;
> 
> Bug - we'll return 0 here.  Use "return -EINVAL" and remove the above
> initialisation of `ret'.

Good point. Will fix.
> 
> 
> > +	/* 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.

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

I am a bit concerned about it as well. Does adding a mutex to
trigger_data a good way to go to protect transient_enabled? I will give
that a try.

> The typecast is unneeded.
ok
> 
> > +	/* 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_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.
Yup
> 
> > +	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)
> > +{
> > +	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.
Yes that is correct - will fix it.
> 
> > +	}
> > +	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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH ] leds: add new transient trigger for one shot timer support

[Jonas Bonn]

Hi Shuah,

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.

A couple more comments inline below.

/Jonas

On Thu, 2012-04-19 at 22:04 -0600, Shuah Khan wrote:
> 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.

You can already read the state of the LED... it's in the 'brightness'
property.  You don't need to be able to read it here, too.

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

'duration' might be a better name.  What are the units?  I like to see
the units explicity in the property name so that I don't need to think:
'duration_msec' might be appropriate.

>        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 <shuahkhan@gmail.com>
> 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 <shuahkhan@gmail.com>
> ---
>  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
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 589ba02..c6d8006 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -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
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index fa0f428..48d5af7 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -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
> diff --git a/drivers/leds/ledtrig-transient.c b/drivers/leds/ledtrig-transient.c
> new file mode 100644
> index 0000000..e829124
> --- /dev/null
> +++ b/drivers/leds/ledtrig-transient.c
> @@ -0,0 +1,246 @@
> +/*
> + * LED Kernel Transient Trigger
> + *
> + * Copyright (C) 2012 Shuah Khan <shuahkhan@gmail.com>
> + *
> + * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
> + * ledtrig-heartbeat.c
> + * Design and use-case input from Jonas Bonn <jonas@southpole.se>
> + *
> + * 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>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/timer.h>
> +#include <linux/leds.h>
> +#include "leds.h"
> +
> +struct transient_trig_data {
> +	int transient_enabled;

As mentioned earlier, I don't think you need to track this.

> +	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);
> +	if (ret)
> +		return ret;
> +
> +	transient_data->transient_time = state;
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR(transient_enabled, 0644, led_transient_enabled_show,
> +		   led_transient_enabled_store);

I would make this a verb, 'enable', and make it 0200.  Even 'enable'
isn't a great choice; something along the lines of 'activate' seems more
appropriate. 

> +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 <shuahkhan@gmail.com>");
> +MODULE_DESCRIPTION("Transient LED trigger");
> +MODULE_LICENSE("GPL");

Re: [PATCH ] leds: add new transient trigger for one shot timer support

[Shuah Khan]

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

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

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

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.

Thanks,
-- Shuah

Re: [PATCH ] leds: add new transient trigger for one shot timer support

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2248 bytes --]

On Sun, 22 Apr 2012 17:51:27 -0600 Shuah Khan <shuahkhan@gmail.com> wrote:

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

I'm not sure I agree.

I think there are two possible correct choices:

 1/ follow the established pattern:  use "delay_off" and assume milliseconds.
    Consistency is good.

 2/ Do the "right" thing which is to use seconds.  "seconds" are the SI 
    unit for time and we should always use them (says Linus - I could
    probably hunt up a reference if you were interested).
    so "0.2" is 200ms.  In that case don't use "delay_off" - probably
    "duration" would be good.
    This doesn't mean you need to use floating point.  See
    "safe_delay_show" and "safe_delay_store" in drivers/md/md.c

But that is just my opinion.


> 
> > 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 agree that "delay" and "activate" should be separate interfaces.

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.


So
  echo 100 > brightness

set the current brightness, sets the future brightness, starts the timer.
Or maybe if we have to hook into led_set_brightness anyway, then we could
cause "echo 100 > brightness" *not* to set the current brightness but just
to record a value for later usage...

But these are just minor things.  I think you are close to something useful.

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH ] leds: add new transient trigger for one shot timer support

[Jonas Bonn]

On Mon, 2012-04-23 at 11:56 +1000, NeilBrown wrote:
> 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!

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

Re: [PATCH ] leds: add new transient trigger for one shot timer support

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1680 bytes --]

On Mon, 23 Apr 2012 07:29:59 +0200 Jonas Bonn <jonas@southpole.se> wrote:

> 
> On Mon, 2012-04-23 at 11:56 +1000, NeilBrown wrote:
> > 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:
> 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).

Thanks,
NeilBrown

> 
> Worst case, the brightness doesn't get set and you get a 'bright' LED
> until it expires... but at least it does expire!
> 
> > 
> > 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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH ] leds: add new transient trigger for one shot timer support

[Shuah Khan]

On Mon, 2012-04-23 at 15:45 +1000, NeilBrown wrote:
> On Mon, 23 Apr 2012 07:29:59 +0200 Jonas Bonn <jonas@southpole.se> wrote:
> 
> > 
> > On Mon, 2012-04-23 at 11:56 +1000, NeilBrown wrote:
> > > 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:
> > 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).
> 
Neil/Jonas,

Thanks for a good discussion and ideas. Are you okay with supporting the
following use-case as a first cut implementation and see how that works
and then add brightness adjustment later?

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

I am taking a look at "safe_delay_show" and "safe_delay_store" in
drivers/md/md.c. I think I can easily change what I have now to do what
these routines do. i.e going with seconds as units based on Neil's
suggestion.

I did make the change to name the properties duration and activate,
dropping the transient pre-fix on both properties.

Thanks,
-- Shuah

[PATCH v2] leds: add new transient trigger for one shot timer activation

[Shuah Khan]

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 188e7791c91eb17b1a857dfa7e64a0b8c66247e7 Mon Sep 17 00:00:00 2001
From: Shuah Khan <shuahkhan@gmail.com>
Date: Wed, 25 Apr 2012 11:27:20 -0600
Subject: [PATCH] leds: add new transient trigger for one shot timer
 activation


Signed-off-by: Shuah Khan <shuahkhan@gmail.com>
---
 drivers/leds/Kconfig             |    8 ++
 drivers/leds/Makefile            |    1 +
 drivers/leds/ledtrig-transient.c |  252 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 261 insertions(+), 0 deletions(-)
 create mode 100644 drivers/leds/ledtrig-transient.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 5f12659..638219b 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -488,4 +488,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 activation of a transient state on
+	  GPIO/PWM based hadrware.
+	  If unsure, say Y.
+
 endif # NEW_LEDS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 9475bbb..202b187 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -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
diff --git a/drivers/leds/ledtrig-transient.c b/drivers/leds/ledtrig-transient.c
new file mode 100644
index 0000000..3a846d7
--- /dev/null
+++ b/drivers/leds/ledtrig-transient.c
@@ -0,0 +1,252 @@
+/*
+ * LED Kernel Transient Trigger
+ *
+ * Copyright (C) 2012 Shuah Khan <shuahkhan@gmail.com>
+ *
+ * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
+ * ledtrig-heartbeat.c
+ * Design and use-case input from Jonas Bonn <jonas@southpole.se> and
+ * Neil Brown <neilb@suse.de>
+ *
+ * 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 exports two properties, activate and duration.
+* duration allows setting timer value in msecs.
+* activate allows activating and deactivating the timer specified by
+* duration as needed.
+*	activate - one shot timer activate mechanism.
+*		   1 when activated, 0 when deactivated.
+*
+*		   activate state indicates a timer
+*		   with a value of specified duration running.
+*		   deactivated state indicates no active timer is running.
+*	duration - one shot timer value. When activate is set, duration value
+*		   is used to start a timer that runs once.
+*	When timer expires activate goes back to deactivated state,
+*	duration is left at the set value to be used when activate
+*	is set at a future time. This will allow user app to set the
+*	time once and activate it to run it once for the specified value as
+*	needed.
+*
+*	echo 1 > activate -	starts timer = duration when duration is not 0.
+*	echo 0 > activate -	cancels currently running timer.
+*	echo n > duration -	stores timer value to be used upon next
+*				activate. Currently active timer if
+*				any, continues to run for the specified time.
+*	echo 0 > duration -	stores timer value to be used upon next
+*				activate. 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 duration is set when
+*	   activate is in activated state. For example:
+*	   echo n > duration will extend/shorten timer, however
+*	   this could lead race conditions with timer expiry.
+*	2. The same could be done using a third attribute to control
+*	   extending and shortening the timer.
+*
+*	Example use-case 1:
+*	echo transient > trigger
+*	echo n > duration
+*	repeat the following step as needed:
+*	echo 1 > activate - start timer = duration to run once
+*	echo 1 > activate - start timer = duration to run once
+*	echo none > trigger
+*
+*	Example use-case 2:
+*	echo transient > trigger
+*	echo n > duration
+*	repeat the following step as needed:
+*	echo 1 > activate - start timer = duration to run once
+*	echo 1 > activate - start timer = duration to run once
+*	echo none > trigger
+*
+* This trigger is intended to be used for for the following example use cases:
+* - Control of vibrate (phones, tablets etc.) hardware by user space app.
+* - Use of LED by user space app as activity indicator.
+* - Use of LED by user space 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 user space app that needs a transient GPIO output.
+*/
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/timer.h>
+#include <linux/leds.h>
+#include "leds.h"
+
+struct transient_trig_data {
+	int activate;
+	unsigned long duration;
+	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;
+
+	transient_data->activate = 0;
+	led_cdev->brightness_set(led_cdev, LED_OFF);
+}
+
+static ssize_t transient_activate_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->activate);
+}
+
+static ssize_t transient_activate_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;
+
+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	if (state != 1 && state != 0)
+		return -EINVAL;
+
+	/* cancel the running timer */
+	if (state == 0 && transient_data->activate == 1) {
+		del_timer(&transient_data->timer);
+		transient_data->activate = state;
+		led_cdev->brightness_set(led_cdev, LED_OFF);
+		return size;
+	}
+
+	/* start timer if there is no active timer */
+	if (state == 1 && transient_data->activate == 0 &&
+	    transient_data->duration != 0) {
+		transient_data->activate = state;
+		led_cdev->brightness_set(led_cdev, LED_FULL);
+		mod_timer(&transient_data->timer,
+			  jiffies + transient_data->duration);
+	}
+
+	/* state == 0 && transient_data->activate == 0
+		timer is not active - just return */
+	/* state == 1 && transient_data->activate == 1
+		timer is already active - just return */
+
+	return size;
+}
+
+static ssize_t transient_duration_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->duration);
+}
+
+static ssize_t transient_duration_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;
+
+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	transient_data->duration = state;
+
+	return size;
+}
+
+static DEVICE_ATTR(activate, 0644, transient_activate_show,
+		   transient_activate_store);
+static DEVICE_ATTR(duration, 0644, transient_duration_show,
+		   transient_duration_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_activate);
+	if (rc)
+		goto err_out;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_duration);
+	if (rc)
+		goto err_out_time;
+
+	setup_timer(&tdata->timer, transient_timer_function,
+		    (unsigned long) led_cdev);
+	led_cdev->activated = true;
+
+	return;
+
+err_out_time:
+	device_remove_file(led_cdev->dev, &dev_attr_activate);
+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) {
+		del_timer_sync(&transient_data->timer);
+		device_remove_file(led_cdev->dev, &dev_attr_activate);
+		device_remove_file(led_cdev->dev, &dev_attr_duration);
+		led_cdev->trigger_data = NULL;
+		led_cdev->activated = false;
+		kfree(transient_data);
+	}
+}
+
+static struct led_trigger transient_trigger = {
+	.name     = "transient",
+	.activate = transient_trig_activate,
+	.deactivate = transient_trig_deactivate,
+};
+
+static int __init transient_trig_init(void)
+{
+	return led_trigger_register(&transient_trigger);
+}
+
+static void __exit transient_trig_exit(void)
+{
+	led_trigger_unregister(&transient_trigger);
+}
+
+module_init(transient_trig_init);
+module_exit(transient_trig_exit);
+
+MODULE_AUTHOR("Shuah Khan <shuahkhan@gmail.com>");
+MODULE_DESCRIPTION("Transient LED trigger");
+MODULE_LICENSE("GPL");
-- 
1.7.5.4

Re: [PATCH v2] leds: add new transient trigger for one shot timer activation

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

On Wed, 25 Apr 2012 11:42:38 -0600 Shuah Khan <shuahkhan@gmail.com> 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).

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v2] leds: add new transient trigger for one shot timer activation

[Shuah Khan]

On Thu, 2012-04-26 at 16:02 +1000, NeilBrown wrote:
> On Wed, 25 Apr 2012 11:42:38 -0600 Shuah Khan <shuahkhan@gmail.com> 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).

My bad. Yes you are right. strict_strtoul_scaled() does do scaling. This
routine is currently defined in drivers/md/md.c. The right to do would
be to move this routine to where kstrtoul() is for general consumption.
Something I might take on as a separate task. :)

-- Shuah

Re: [PATCH v2] leds: add new transient trigger for one shot timer activation

[Andrew Morton]

On Wed, 25 Apr 2012 11:42:38 -0600
Shuah Khan <shuahkhan@gmail.com> wrote:

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

First rule of changelogs: think about how your words will appear to
others in a year's time.  Text which refers to an earlier version of
the patch is uninteresting.  Text which refers to some review
discussion is uninteresting.  There is a convention that this short-term
information is presented after the separating "^---" in the changelog,
after the signoffs, cc's, etc.

When I stripped out all the short-term fluff all I was left with was
part of the first paragraph, and it's a poor changelog.  It doesn't
describe what a "transient trigger" is, it doesn't define "one shot
timer activation".  Quite importantly, it doesn't describe the user
interface to this feature, which is the most important part of the
patch, because it is the part we can never change.

So, please fully describe the feature and fully describe the interface
which it exposes to users.  This would include a description of dynamic
behaviour such as cancelation, what happens if a LED driver module is
removed when the trigger is in its pending and active states, what
happens if the trigger is re-acticated when it is pending, what happens
if the trigger is re-activated when the LED is active, etc.

This stuff matters!  We want to hear you describe it in your own words,
so we can understand (and hopefully agree with) your design intent and
then check the implementation agaisnt that intent.

Can I use this feature to have my LED temporarily blink *off*?

Another thing which is missing here is a decent description of why the
kernel needs this feature.  I can implement a one-shot LED trigger in
userspace, using complex things like "sleep".  Where is the
justification for adding kernel code to do this?

[PATCH v3] leds: add new transient trigger for one shot timer activation

[Shuah Khan]

The leds timer trigger does not currently have an interface to activate
a one shot timer. 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. The 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.

Without one shot timer interface, user space can still use timer trigger to
set a timer to hold a state, however when user space application crashes or
goes away without deactivating the timer, the hardware will be left in that
state permanently.

As a specific example of this use-case, let's look at vibrate feature on
phones. Vibrate function on phones is implemented using PWM pins on SoC or
PMIC. There is a need to activate one shot timer to control the vibrate
feature, to prevent user space crashes leaving the phone in vibrate mode
permanently causing the battery to drain.

This trigger exports three properties, activate, state, and duration When
transient trigger is activated these properties are set to default values.

- duration allows setting timer value in msecs. The initial value is 0.
- activate allows activating and deactivating the timer specified by
  duration as needed. The initial and default value is 0.  This will allow
  duration to be set after trigger activation.
- state allows user to specify a transient state to be held for the specified
  duration.

>From a8249a6738020823277ef02d7787e8078c17844b Mon Sep 17 00:00:00 2001
From: Shuah Khan <shuahkhan@gmail.com>
Date: Mon, 30 Apr 2012 14:18:16 -0600
Subject: [PATCH v3] leds: add new transient trigger for one shot timer
 activation


Signed-off-by: Shuah Khan <shuahkhan@gmail.com>
---
 Documentation/leds/ledtrig-transient.txt |  152 +++++++++++++++++++
 drivers/leds/Kconfig                     |    8 +
 drivers/leds/Makefile                    |    1 +
 drivers/leds/ledtrig-transient.c         |  237 ++++++++++++++++++++++++++++++
 4 files changed, 398 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/leds/ledtrig-transient.txt
 create mode 100644 drivers/leds/ledtrig-transient.c

diff --git a/Documentation/leds/ledtrig-transient.txt b/Documentation/leds/ledtrig-transient.txt
new file mode 100644
index 0000000..3bd38b4
--- /dev/null
+++ b/Documentation/leds/ledtrig-transient.txt
@@ -0,0 +1,152 @@
+LED Transient Trigger
+=====================
+
+The leds timer trigger does not currently have an interface to activate
+a one shot timer. 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. The 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.
+
+Without one shot timer interface, user space can still use timer trigger to
+set a timer to hold a state, however when user space application crashes or
+goes away without deactivating the timer, the hardware will be left in that
+state permanently.
+
+As a specific example of this use-case, let's look at vibrate feature on
+phones. Vibrate function on phones is implemented using PWM pins on SoC or
+PMIC. There is a need to activate one shot timer to control the vibrate
+feature, to prevent user space crashes leaving the phone in vibrate mode
+permanently causing the battery to drain.
+
+Transient trigger addresses the need for one shot timer activation. The
+transient trigger can be enabled and disabled just like the other leds
+triggers.
+
+When an led class device driver registers itself, it can specify all leds
+triggers it supports and a default trigger. During registration, activation
+routine for the default trigger gets called. During registration of an led
+class device, the LED state does not change.
+
+When the driver unregisters, deactivation routine for the currently active
+trigger will be called, and LED state is changed to LED_OFF.
+
+Driver suspend changes the LED state to LED_OFF and resume doesn't change
+the state. Please note that there is no explicit interaction between the
+suspend and resume actions and the currently enabled trigger. LED state
+changes are suspended while the driver is in suspend state. Any timers
+that are active at the time driver gets suspended, continue to run, without
+being able to actually change the LED state. Once driver is resumed, triggers
+start functioning again.
+
+LED state changes are controlled using brightness which is a common led
+class device property. When brightness is set to 0 from user space via
+echo 0 > brightness, it will result in deactivating the current trigger.
+
+Transient trigger uses standard register and unregister interfaces. During
+trigger registration, for each led class device that specifies this trigger
+as its default trigger, trigger activation routine will get called. During
+registration, the LED state does not change, unless there is another trigger
+active, in which case LED state changes to LED_OFF.
+
+During trigger unregistration, LED state gets changed to LED_OFF.
+
+Transient trigger activation routine doesn't change the LED state. It
+creates its properties and does its initialization. Transient trigger
+deactivation routine, will cancel any timer that is active before it cleans
+up and removes the properties it created. It will restore the LED state to
+non-transient state. When driver gets suspended, irrespective of the transient
+state, the LED state changes to LED_OFF.
+
+Transient trigger can be enabled and disabled from user space on led class
+devices, that support this trigger as shown below:
+
+echo transient > trigger
+echo none > trigger
+
+NOTE: Add a new property trigger state to control the state.
+
+This trigger exports three properties, activate, state, and duration. When
+transient trigger is activated these properties are set to default values.
+
+- duration allows setting timer value in msecs. The initial value is 0.
+- activate allows activating and deactivating the timer specified by
+  duration as needed. The initial and default value is 0.  This will allow
+  duration to be set after trigger activation.
+- state allows user to specify a transient state to be held for the specified
+  duration.
+
+	activate - one shot timer activate mechanism.
+		1 when activated, 0 when deactivated.
+		default value is zero when transient trigger is enabled,
+		to allow duration to be set.
+
+		activate state indicates a timer with a value of specified
+		duration running.
+		deactivated state indicates that there is no active timer
+		running.
+
+	duration - one shot timer value. When activate is set, duration value
+		is used to start a timer that runs once. This value doesn't
+		get changed by the trigger unless user does a set via
+		echo new_value > duration
+
+	state - transient state to be held. It has two values 0 or 1. 0 maps
+		to LED_OFF and 1 maps to LED_FULL. The specified state is
+		held for the duration of the one shot timer and then the
+		state gets changed to the non-transient state which is the
+		inverse of transient state.
+		If state = LED_FULL, when the timer runs out the state will
+		go back to LED_OFF.
+		If state = LED_OFF, when the timer runs out the state will
+		go back to LED_FULL.
+		Please note that current LED state is not checked prior to
+		changing the state to the specified state.
+		Driver could map these values to inverted depending on the
+		default states it defines for the LED in its brightness_set()
+		interface which is called from the led brightness_set()
+		interfaces to control the LED state.
+
+When timer expires activate goes back to deactivated state, duration is left
+at the set value to be used when activate is set at a future time. This will
+allow user app to set the time once and activate it to run it once for the
+specified value as needed. When timer expires, state is restored to the
+non-transient state which is the inverse of the transient state.
+
+	echo 1 > activate - starts timer = duration when duration is not 0.
+	echo 0 > activate - cancels currently running timer.
+	echo n > duration - stores timer value to be used upon next
+                            activate. Currently active timer if
+                            any, continues to run for the specified time.
+	echo 0 > duration - stores timer value to be used upon next
+                            activate. Currently active timer if any,
+                            continues to run for the specified time.
+	echo 1 > state    - stores desired transient state LED_FULL to be
+			    held for the specified duration.
+	echo 0 > state    - stores desired transient state LED_OFF to be
+			    held for the specified duration.
+
+What is not supported:
+======================
+- Timer activation is one shot and extending and/or shortening the timer
+  is not supported.
+
+Example use-case 1:
+	echo transient > trigger
+	echo n > duration
+	echo 1 > state
+repeat the following step as needed:
+	echo 1 > activate - start timer = duration to run once
+	echo 1 > activate - start timer = duration to run once
+	echo none > trigger
+
+This trigger is intended to be used for for the following example use cases:
+ - Control of vibrate (phones, tablets etc.) hardware by user space app.
+ - Use of LED by user space app as activity indicator.
+ - Use of LED by user space 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 user space app that needs a transient GPIO output.
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 5f12659..638219b 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -488,4 +488,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 activation of a transient state on
+	  GPIO/PWM based hadrware.
+	  If unsure, say Y.
+
 endif # NEW_LEDS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 9475bbb..202b187 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -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
diff --git a/drivers/leds/ledtrig-transient.c b/drivers/leds/ledtrig-transient.c
new file mode 100644
index 0000000..83179f4
--- /dev/null
+++ b/drivers/leds/ledtrig-transient.c
@@ -0,0 +1,237 @@
+/*
+ * LED Kernel Transient Trigger
+ *
+ * Copyright (C) 2012 Shuah Khan <shuahkhan@gmail.com>
+ *
+ * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
+ * ledtrig-heartbeat.c
+ * Design and use-case input from Jonas Bonn <jonas@southpole.se> and
+ * Neil Brown <neilb@suse.de>
+ *
+ * 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.
+ *
+ */
+/*
+ * Transient trigger allows one shot timer activation. Please refer to
+ * Documentation/leds/ledtrig-transient.txt for details
+*/
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/timer.h>
+#include <linux/leds.h>
+#include "leds.h"
+
+struct transient_trig_data {
+	int activate;
+	int state;
+	int restore_state;
+	unsigned long duration;
+	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;
+
+	transient_data->activate = 0;
+	led_set_brightness(led_cdev, transient_data->restore_state);
+}
+
+static ssize_t transient_activate_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->activate);
+}
+
+static ssize_t transient_activate_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;
+
+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	if (state != 1 && state != 0)
+		return -EINVAL;
+
+	/* cancel the running timer */
+	if (state == 0 && transient_data->activate == 1) {
+		del_timer(&transient_data->timer);
+		transient_data->activate = state;
+		led_set_brightness(led_cdev, transient_data->restore_state);
+		return size;
+	}
+
+	/* start timer if there is no active timer */
+	if (state == 1 && transient_data->activate == 0 &&
+	    transient_data->duration != 0) {
+		transient_data->activate = state;
+		led_set_brightness(led_cdev, transient_data->state);
+		transient_data->restore_state =
+		    (transient_data->state == LED_FULL) ? LED_OFF : LED_FULL;
+		mod_timer(&transient_data->timer,
+			  jiffies + transient_data->duration);
+	}
+
+	/* state == 0 && transient_data->activate == 0
+		timer is not active - just return */
+	/* state == 1 && transient_data->activate == 1
+		timer is already active - just return */
+
+	return size;
+}
+
+static ssize_t transient_duration_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->duration);
+}
+
+static ssize_t transient_duration_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;
+
+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	transient_data->duration = state;
+	return size;
+}
+
+static ssize_t transient_state_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;
+	int state;
+
+	state = (transient_data->state == LED_FULL) ? 1 : 0;
+	return sprintf(buf, "%d\n", state);
+}
+
+static ssize_t transient_state_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;
+
+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	if (state != 1 && state != 0)
+		return -EINVAL;
+
+	transient_data->state = (state == 1) ? LED_FULL : LED_OFF;
+	return size;
+}
+
+static DEVICE_ATTR(activate, 0644, transient_activate_show,
+		   transient_activate_store);
+static DEVICE_ATTR(duration, 0644, transient_duration_show,
+		   transient_duration_store);
+static DEVICE_ATTR(state, 0644, transient_state_show, transient_state_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_activate);
+	if (rc)
+		goto err_out;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_duration);
+	if (rc)
+		goto err_out_duration;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_state);
+	if (rc)
+		goto err_out_state;
+
+	setup_timer(&tdata->timer, transient_timer_function,
+		    (unsigned long) led_cdev);
+	led_cdev->activated = true;
+
+	return;
+
+err_out_state:
+	device_remove_file(led_cdev->dev, &dev_attr_duration);
+err_out_duration:
+	device_remove_file(led_cdev->dev, &dev_attr_activate);
+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) {
+		del_timer_sync(&transient_data->timer);
+		led_set_brightness(led_cdev, transient_data->restore_state);
+		device_remove_file(led_cdev->dev, &dev_attr_activate);
+		device_remove_file(led_cdev->dev, &dev_attr_duration);
+		device_remove_file(led_cdev->dev, &dev_attr_state);
+		led_cdev->trigger_data = NULL;
+		led_cdev->activated = false;
+		kfree(transient_data);
+	}
+}
+
+static struct led_trigger transient_trigger = {
+	.name     = "transient",
+	.activate = transient_trig_activate,
+	.deactivate = transient_trig_deactivate,
+};
+
+static int __init transient_trig_init(void)
+{
+	return led_trigger_register(&transient_trigger);
+}
+
+static void __exit transient_trig_exit(void)
+{
+	led_trigger_unregister(&transient_trigger);
+}
+
+module_init(transient_trig_init);
+module_exit(transient_trig_exit);
+
+MODULE_AUTHOR("Shuah Khan <shuahkhan@gmail.com>");
+MODULE_DESCRIPTION("Transient LED trigger");
+MODULE_LICENSE("GPL");
-- 
1.7.5.4

Re: [PATCH ] leds: add new transient trigger for one shot timer support

[Jonas Bonn]

On Sun, 2012-04-22 at 17:51 -0600, Shuah Khan wrote:
> 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 :)

'duration' is shorter than than 'transient_time' :)

I don't think you need to prefix the property with the trigger
namespace... no other trigger does this.  I'm still inclined to say that
'duration' is clearer than 'time'... the length of the property name
can't seriously be an issue here.  Neil also commented that it should be
'delay_on' which has the advantage of being consistent with other
existing triggers.

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

You misunderstood me... it's the not the 'duration' state that I was
asking about, it's the 'illuminated' state.  In your patch you had 3
fields in your trigger data struct... I'd argue you only need two: the
duration and the timer itself; you don't need a field to keep track of
the state of the LED.

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

OK, this can be accomplished by writing 0 to the 'brightness' value...
you don't need to be able to do this via the 'activate' property.  The
trigger will clean itself up automatically when you do this, too.

> > 
> > 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.
> 
> That is correct. brightness could be used, however having a distinct
> name-space might be better for future changes instead of overloading
> brightness.

This is a simple trigger that does one thing and does it well; what
future changes do you have in mind?  Will I need to download a PDF
manual and dedicate a weekend to understanding the intricate interplay
of all its properties in the future?

The 'brightness' property will always be there; it doesn't go away just
because you install a trigger for the LED.  It's best to use the
interface that already exists instead of adding a second one that
essentially does the same thing...

> 
> Hope this helps explain my reasoning to keep transient state.
> 

Yeah, for sure... I'd say you're almost there so hang in there!

/Jonas