[PATCH v2 1/2] led: add oneshot trigger

[PATCH v2 1/2] led: add oneshot trigger

[Fabio Baltieri]

Add oneshot trigger to blink a led with configurable parameters via
sysfs.

Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
Cc: Shuah Khan <shuahkhan@gmail.com>
Cc: Bryan Wu <bryan.wu@canonical.com>
---
Hi all,

that's my v2 of the ledtrig-oneshot trigger, with the correction suggested from
Shuah in the original thread.  These include some unnecessary initialization
and use of the new "activated" flag.

I've kept the name to "oneshot" as I think it suits the role of the trigger as
example/test for the led_blink_set_oneshot function but I have expanded a bit
the Kconfig description to better identify the trigger's usage.

About the namespace question, I think that naming the parameters delay_on and
delay_off is ok as they have the same meaning as timer's ones (they also
uses the same internal fields after all...).

Also, I added a default set of delay_{on,off} to 100ms on activation, so that
the trigger starts with a valid configuration, like ledtrig-timer does.

Anything else that should be changed?

Fabio

 drivers/leds/Kconfig           |  14 +++
 drivers/leds/Makefile          |   1 +
 drivers/leds/ledtrig-oneshot.c | 198 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 213 insertions(+)
 create mode 100644 drivers/leds/ledtrig-oneshot.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 04cb8c8..1f151fb 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -443,6 +443,20 @@ config LEDS_TRIGGER_TIMER
 
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_ONESHOT
+	tristate "LED One-shot Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  This allows LEDs to blink in one-shot pulses with parameters
+	  controlled via sysfs.  It's useful to notify the user on
+	  sporadic events, when there are no clear begin and end trap points,
+	  or on dense events, where this blinks the LED at constant rate if
+	  rearmed continuously.
+
+	  It also shows how to use the led_blink_set_oneshot() function.
+
+	  If unsure, say Y.
+
 config LEDS_TRIGGER_IDE_DISK
 	bool "LED IDE Disk Trigger"
 	depends on IDE_GD_ATA
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index f8958cd..9112d51 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
 
 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
+obj-$(CONFIG_LEDS_TRIGGER_ONESHOT)	+= ledtrig-oneshot.o
 obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)	+= ledtrig-ide-disk.o
 obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
 obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)	+= ledtrig-backlight.o
diff --git a/drivers/leds/ledtrig-oneshot.c b/drivers/leds/ledtrig-oneshot.c
new file mode 100644
index 0000000..1aacdfe
--- /dev/null
+++ b/drivers/leds/ledtrig-oneshot.c
@@ -0,0 +1,198 @@
+/*
+ * One-shot LED Trigger
+ *
+ * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
+ *
+ * Based on ledtrig-timer.c by Richard Purdie <rpurdie@openedhand.com>
+ *
+ * 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/slab.h>
+#include <linux/leds.h>
+
+#define DEFAULT_DELAY 100
+
+struct oneshot_trig_data {
+	unsigned int invert;
+};
+
+static ssize_t led_shot(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
+
+	led_blink_set_oneshot(led_cdev,
+			&led_cdev->blink_delay_on, &led_cdev->blink_delay_off,
+			oneshot_data->invert);
+
+	/* content is ignored */
+	return size;
+}
+static ssize_t led_invert_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
+
+	return sprintf(buf, "%u\n", oneshot_data->invert);
+}
+
+static ssize_t led_invert_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
+	unsigned long state;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &state);
+	if (ret)
+		return ret;
+
+	oneshot_data->invert = !!state;
+
+	return size;
+}
+
+static ssize_t led_delay_on_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%lu\n", led_cdev->blink_delay_on);
+}
+
+static ssize_t led_delay_on_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	unsigned long state;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &state);
+	if (ret)
+		return ret;
+
+	led_cdev->blink_delay_on = state;
+
+	return size;
+}
+static ssize_t led_delay_off_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%lu\n", led_cdev->blink_delay_off);
+}
+
+static ssize_t led_delay_off_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	unsigned long state;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &state);
+	if (ret)
+		return ret;
+
+	led_cdev->blink_delay_off = state;
+
+	return size;
+}
+
+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 DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);
+static DEVICE_ATTR(shot, 0200, NULL, led_shot);
+
+static void oneshot_trig_activate(struct led_classdev *led_cdev)
+{
+	struct oneshot_trig_data *oneshot_data;
+	int rc;
+
+	oneshot_data = kzalloc(sizeof(*oneshot_data), GFP_KERNEL);
+	if (!oneshot_data)
+		return;
+
+	led_cdev->trigger_data = oneshot_data;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_delay_on);
+	if (rc)
+		goto err_out_trig_data;
+	rc = device_create_file(led_cdev->dev, &dev_attr_delay_off);
+	if (rc)
+		goto err_out_delayon;
+	rc = device_create_file(led_cdev->dev, &dev_attr_invert);
+	if (rc)
+		goto err_out_delayoff;
+	rc = device_create_file(led_cdev->dev, &dev_attr_shot);
+	if (rc)
+		goto err_out_invert;
+
+	led_cdev->blink_delay_on = DEFAULT_DELAY;
+	led_cdev->blink_delay_off = DEFAULT_DELAY;
+
+	led_cdev->activated = true;
+
+	return;
+
+err_out_invert:
+	device_remove_file(led_cdev->dev, &dev_attr_invert);
+err_out_delayoff:
+	device_remove_file(led_cdev->dev, &dev_attr_delay_off);
+err_out_delayon:
+	device_remove_file(led_cdev->dev, &dev_attr_delay_on);
+err_out_trig_data:
+	kfree(led_cdev->trigger_data);
+}
+
+static void oneshot_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
+
+	if (led_cdev->activated) {
+		device_remove_file(led_cdev->dev, &dev_attr_delay_on);
+		device_remove_file(led_cdev->dev, &dev_attr_delay_off);
+		device_remove_file(led_cdev->dev, &dev_attr_invert);
+		device_remove_file(led_cdev->dev, &dev_attr_shot);
+		kfree(oneshot_data);
+		led_cdev->activated = false;
+	}
+
+	/* Stop blinking */
+	led_brightness_set(led_cdev, LED_OFF);
+}
+
+static struct led_trigger oneshot_led_trigger = {
+	.name     = "oneshot",
+	.activate = oneshot_trig_activate,
+	.deactivate = oneshot_trig_deactivate,
+};
+
+static int __init oneshot_trig_init(void)
+{
+	return led_trigger_register(&oneshot_led_trigger);
+}
+
+static void __exit oneshot_trig_exit(void)
+{
+	led_trigger_unregister(&oneshot_led_trigger);
+}
+
+module_init(oneshot_trig_init);
+module_exit(oneshot_trig_exit);
+
+MODULE_AUTHOR("Fabio Baltieri <fabio.baltieri@gmail.com>");
+MODULE_DESCRIPTION("One-shot LED trigger");
+MODULE_LICENSE("GPL");
-- 
1.7.10.2.520.g6a4a482.dirty

[PATCH 2/2] leds: fix led_brightness_set when soft-blinking

[Fabio Baltieri]

Change led_brightness_set to ensure software blink timer is stopped
before calling led_stop_software_blink() and use led_set_brightness()
instead of calling led_cdev->brightness_set() directly to ensure
led_cdev->brightness is always consistent with current LED status.

This ensure proper cleaning when changing triggers, as without this fix
a LED may be turned off while leaving it's led_cdev->brightness = 1,
leading to an erratic software-blink behaviour.

The problem was easy to reproduce by changing the trigger from "timer"
to "oneshot".

Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
Cc: Bryan Wu <bryan.wu@canonical.com>
---
 drivers/leds/led-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 579eb78..3e9f934 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -115,7 +115,8 @@ EXPORT_SYMBOL(led_blink_set_oneshot);
 void led_brightness_set(struct led_classdev *led_cdev,
 			enum led_brightness brightness)
 {
+	del_timer_sync(&led_cdev->blink_timer);
 	led_stop_software_blink(led_cdev);
-	led_cdev->brightness_set(led_cdev, brightness);
+	led_set_brightness(led_cdev, brightness);
 }
 EXPORT_SYMBOL(led_brightness_set);
-- 
1.7.10.2.520.g6a4a482.dirty

Re: [PATCH v2 1/2] led: add oneshot trigger

[Shuah Khan]

On Tue, 2012-06-05 at 23:38 +0200, Fabio Baltieri wrote:
> Add oneshot trigger to blink a led with configurable parameters via
> sysfs.

Fabio,

What are the use-cases for this trigger. Please see comments inline:


> 
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> Cc: Shuah Khan <shuahkhan@gmail.com>
> Cc: Bryan Wu <bryan.wu@canonical.com>
> ---
> Hi all,
> 
> that's my v2 of the ledtrig-oneshot trigger, with the correction suggested from
> Shuah in the original thread.  These include some unnecessary initialization
> and use of the new "activated" flag.
> 
> I've kept the name to "oneshot" as I think it suits the role of the trigger as
> example/test for the led_blink_set_oneshot function but I have expanded a bit
> the Kconfig description to better identify the trigger's usage.
> 
> About the namespace question, I think that naming the parameters delay_on and
> delay_off is ok as they have the same meaning as timer's ones (they also
> uses the same internal fields after all...).
> 
> Also, I added a default set of delay_{on,off} to 100ms on activation, so that
> the trigger starts with a valid configuration, like ledtrig-timer does.
> 
> Anything else that should be changed?
> 
> Fabio
> 
>  drivers/leds/Kconfig           |  14 +++
>  drivers/leds/Makefile          |   1 +
>  drivers/leds/ledtrig-oneshot.c | 198 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 213 insertions(+)
>  create mode 100644 drivers/leds/ledtrig-oneshot.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 04cb8c8..1f151fb 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -443,6 +443,20 @@ config LEDS_TRIGGER_TIMER
>  
>  	  If unsure, say Y.
>  
> +config LEDS_TRIGGER_ONESHOT
> +	tristate "LED One-shot Trigger"
> +	depends on LEDS_TRIGGERS
> +	help
> +	  This allows LEDs to blink in one-shot pulses with parameters
> +	  controlled via sysfs.  It's useful to notify the user on
> +	  sporadic events, when there are no clear begin and end trap points,
> +	  or on dense events, where this blinks the LED at constant rate if
> +	  rearmed continuously.
> +
> +	  It also shows how to use the led_blink_set_oneshot() function.
> +
> +	  If unsure, say Y.
> +
>  config LEDS_TRIGGER_IDE_DISK
>  	bool "LED IDE Disk Trigger"
>  	depends on IDE_GD_ATA
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index f8958cd..9112d51 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
>  
>  # LED Triggers
>  obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
> +obj-$(CONFIG_LEDS_TRIGGER_ONESHOT)	+= ledtrig-oneshot.o
>  obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)	+= ledtrig-ide-disk.o
>  obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
>  obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)	+= ledtrig-backlight.o
> diff --git a/drivers/leds/ledtrig-oneshot.c b/drivers/leds/ledtrig-oneshot.c
> new file mode 100644
> index 0000000..1aacdfe
> --- /dev/null
> +++ b/drivers/leds/ledtrig-oneshot.c
> @@ -0,0 +1,198 @@
> +/*
> + * One-shot LED Trigger
> + *
> + * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
> + *
> + * Based on ledtrig-timer.c by Richard Purdie <rpurdie@openedhand.com>
> + *
> + * 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/slab.h>
> +#include <linux/leds.h>
> +
> +#define DEFAULT_DELAY 100
> +
> +struct oneshot_trig_data {
> +	unsigned int invert;
> +};
> +
> +static ssize_t led_shot(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
> +
> +	led_blink_set_oneshot(led_cdev,
> +			&led_cdev->blink_delay_on, &led_cdev->blink_delay_off,
> +			oneshot_data->invert);

Here when shot is set

echo n > shot

What are the values for blink_delay_on and blink_delay_off. Does user
need to set delay_on and delay_off first? If user doesn't set these then
what values are used?

What about invert? Does user need to do echo 1 > invert and then
activate shot?


Thanks,
-- Shuah
> +
> +	/* content is ignored */
> +	return size;
> +}
> +static ssize_t led_invert_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
> +
> +	return sprintf(buf, "%u\n", oneshot_data->invert);
> +}
> +
> +static ssize_t led_invert_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
> +	unsigned long state;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 0, &state);
> +	if (ret)
> +		return ret;
> +
> +	oneshot_data->invert = !!state;
> +
> +	return size;
> +}
> +
> +static ssize_t led_delay_on_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%lu\n", led_cdev->blink_delay_on);
> +}
> +
> +static ssize_t led_delay_on_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	unsigned long state;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 0, &state);
> +	if (ret)
> +		return ret;
> +
> +	led_cdev->blink_delay_on = state;
> +
> +	return size;
> +}
> +static ssize_t led_delay_off_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%lu\n", led_cdev->blink_delay_off);
> +}
> +
> +static ssize_t led_delay_off_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	unsigned long state;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 0, &state);
> +	if (ret)
> +		return ret;
> +
> +	led_cdev->blink_delay_off = state;
> +
> +	return size;
> +}
> +
> +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 DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);
> +static DEVICE_ATTR(shot, 0200, NULL, led_shot);
> +
> +static void oneshot_trig_activate(struct led_classdev *led_cdev)
> +{
> +	struct oneshot_trig_data *oneshot_data;
> +	int rc;
> +
> +	oneshot_data = kzalloc(sizeof(*oneshot_data), GFP_KERNEL);
> +	if (!oneshot_data)
> +		return;
> +
> +	led_cdev->trigger_data = oneshot_data;
> +
> +	rc = device_create_file(led_cdev->dev, &dev_attr_delay_on);
> +	if (rc)
> +		goto err_out_trig_data;
> +	rc = device_create_file(led_cdev->dev, &dev_attr_delay_off);
> +	if (rc)
> +		goto err_out_delayon;
> +	rc = device_create_file(led_cdev->dev, &dev_attr_invert);
> +	if (rc)
> +		goto err_out_delayoff;
> +	rc = device_create_file(led_cdev->dev, &dev_attr_shot);
> +	if (rc)
> +		goto err_out_invert;
> +
> +	led_cdev->blink_delay_on = DEFAULT_DELAY;
> +	led_cdev->blink_delay_off = DEFAULT_DELAY;
> +
> +	led_cdev->activated = true;
> +
> +	return;
> +
> +err_out_invert:
> +	device_remove_file(led_cdev->dev, &dev_attr_invert);
> +err_out_delayoff:
> +	device_remove_file(led_cdev->dev, &dev_attr_delay_off);
> +err_out_delayon:
> +	device_remove_file(led_cdev->dev, &dev_attr_delay_on);
> +err_out_trig_data:
> +	kfree(led_cdev->trigger_data);
> +}
> +
> +static void oneshot_trig_deactivate(struct led_classdev *led_cdev)
> +{
> +	struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
> +
> +	if (led_cdev->activated) {
> +		device_remove_file(led_cdev->dev, &dev_attr_delay_on);
> +		device_remove_file(led_cdev->dev, &dev_attr_delay_off);
> +		device_remove_file(led_cdev->dev, &dev_attr_invert);
> +		device_remove_file(led_cdev->dev, &dev_attr_shot);
> +		kfree(oneshot_data);
> +		led_cdev->activated = false;
> +	}
> +
> +	/* Stop blinking */
> +	led_brightness_set(led_cdev, LED_OFF);
> +}
> +
> +static struct led_trigger oneshot_led_trigger = {
> +	.name     = "oneshot",
> +	.activate = oneshot_trig_activate,
> +	.deactivate = oneshot_trig_deactivate,
> +};
> +
> +static int __init oneshot_trig_init(void)
> +{
> +	return led_trigger_register(&oneshot_led_trigger);
> +}
> +
> +static void __exit oneshot_trig_exit(void)
> +{
> +	led_trigger_unregister(&oneshot_led_trigger);
> +}
> +
> +module_init(oneshot_trig_init);
> +module_exit(oneshot_trig_exit);
> +
> +MODULE_AUTHOR("Fabio Baltieri <fabio.baltieri@gmail.com>");
> +MODULE_DESCRIPTION("One-shot LED trigger");
> +MODULE_LICENSE("GPL");

Re: [PATCH v2 1/2] led: add oneshot trigger

[Bryan Wu]

On Wed, Jun 6, 2012 at 8:51 AM, Shuah Khan <shuahkhan@gmail.com> wrote:
> On Tue, 2012-06-05 at 23:38 +0200, Fabio Baltieri wrote:
>> Add oneshot trigger to blink a led with configurable parameters via
>> sysfs.
>
> Fabio,
>
> What are the use-cases for this trigger. Please see comments inline:
>
>
>>
>> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
>> Cc: Shuah Khan <shuahkhan@gmail.com>
>> Cc: Bryan Wu <bryan.wu@canonical.com>
>> ---
>> Hi all,
>>
>> that's my v2 of the ledtrig-oneshot trigger, with the correction suggested from
>> Shuah in the original thread.  These include some unnecessary initialization
>> and use of the new "activated" flag.
>>
>> I've kept the name to "oneshot" as I think it suits the role of the trigger as
>> example/test for the led_blink_set_oneshot function but I have expanded a bit
>> the Kconfig description to better identify the trigger's usage.
>>
>> About the namespace question, I think that naming the parameters delay_on and
>> delay_off is ok as they have the same meaning as timer's ones (they also
>> uses the same internal fields after all...).
>>
>> Also, I added a default set of delay_{on,off} to 100ms on activation, so that
>> the trigger starts with a valid configuration, like ledtrig-timer does.
>>
>> Anything else that should be changed?
>>
>> Fabio
>>
>>  drivers/leds/Kconfig           |  14 +++
>>  drivers/leds/Makefile          |   1 +
>>  drivers/leds/ledtrig-oneshot.c | 198 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 213 insertions(+)
>>  create mode 100644 drivers/leds/ledtrig-oneshot.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 04cb8c8..1f151fb 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -443,6 +443,20 @@ config LEDS_TRIGGER_TIMER
>>
>>         If unsure, say Y.
>>
>> +config LEDS_TRIGGER_ONESHOT
>> +     tristate "LED One-shot Trigger"
>> +     depends on LEDS_TRIGGERS
>> +     help
>> +       This allows LEDs to blink in one-shot pulses with parameters
>> +       controlled via sysfs.  It's useful to notify the user on
>> +       sporadic events, when there are no clear begin and end trap points,
>> +       or on dense events, where this blinks the LED at constant rate if
>> +       rearmed continuously.
>> +
>> +       It also shows how to use the led_blink_set_oneshot() function.
>> +
>> +       If unsure, say Y.
>> +
>>  config LEDS_TRIGGER_IDE_DISK
>>       bool "LED IDE Disk Trigger"
>>       depends on IDE_GD_ATA
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index f8958cd..9112d51 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_DAC124S085)               += leds-dac124s085.o
>>
>>  # LED Triggers
>>  obj-$(CONFIG_LEDS_TRIGGER_TIMER)     += ledtrig-timer.o
>> +obj-$(CONFIG_LEDS_TRIGGER_ONESHOT)   += ledtrig-oneshot.o
>>  obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)  += ledtrig-ide-disk.o
>>  obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT) += ledtrig-heartbeat.o
>>  obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT) += ledtrig-backlight.o
>> diff --git a/drivers/leds/ledtrig-oneshot.c b/drivers/leds/ledtrig-oneshot.c
>> new file mode 100644
>> index 0000000..1aacdfe
>> --- /dev/null
>> +++ b/drivers/leds/ledtrig-oneshot.c
>> @@ -0,0 +1,198 @@
>> +/*
>> + * One-shot LED Trigger
>> + *
>> + * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
>> + *
>> + * Based on ledtrig-timer.c by Richard Purdie <rpurdie@openedhand.com>
>> + *
>> + * 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/slab.h>
>> +#include <linux/leds.h>
>> +
>> +#define DEFAULT_DELAY 100
>> +
>> +struct oneshot_trig_data {
>> +     unsigned int invert;
>> +};
>> +
>> +static ssize_t led_shot(struct device *dev,
>> +             struct device_attribute *attr, const char *buf, size_t size)
>> +{
>> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +     struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
>> +
>> +     led_blink_set_oneshot(led_cdev,
>> +                     &led_cdev->blink_delay_on, &led_cdev->blink_delay_off,
>> +                     oneshot_data->invert);
>
> Here when shot is set
>
> echo n > shot
>
> What are the values for blink_delay_on and blink_delay_off. Does user
> need to set delay_on and delay_off first? If user doesn't set these then
> what values are used?
>
> What about invert? Does user need to do echo 1 > invert and then
> activate shot?
>
>

Exactly, Fabio, what about adding a document for the use case and
sysfs interface?

-Bryan

>> +
>> +     /* content is ignored */
>> +     return size;
>> +}
>> +static ssize_t led_invert_show(struct device *dev,
>> +             struct device_attribute *attr, char *buf)
>> +{
>> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +     struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
>> +
>> +     return sprintf(buf, "%u\n", oneshot_data->invert);
>> +}
>> +
>> +static ssize_t led_invert_store(struct device *dev,
>> +             struct device_attribute *attr, const char *buf, size_t size)
>> +{
>> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +     struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
>> +     unsigned long state;
>> +     int ret;
>> +
>> +     ret = kstrtoul(buf, 0, &state);
>> +     if (ret)
>> +             return ret;
>> +
>> +     oneshot_data->invert = !!state;
>> +
>> +     return size;
>> +}
>> +
>> +static ssize_t led_delay_on_show(struct device *dev,
>> +             struct device_attribute *attr, char *buf)
>> +{
>> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +
>> +     return sprintf(buf, "%lu\n", led_cdev->blink_delay_on);
>> +}
>> +
>> +static ssize_t led_delay_on_store(struct device *dev,
>> +             struct device_attribute *attr, const char *buf, size_t size)
>> +{
>> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +     unsigned long state;
>> +     int ret;
>> +
>> +     ret = kstrtoul(buf, 0, &state);
>> +     if (ret)
>> +             return ret;
>> +
>> +     led_cdev->blink_delay_on = state;
>> +
>> +     return size;
>> +}
>> +static ssize_t led_delay_off_show(struct device *dev,
>> +             struct device_attribute *attr, char *buf)
>> +{
>> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +
>> +     return sprintf(buf, "%lu\n", led_cdev->blink_delay_off);
>> +}
>> +
>> +static ssize_t led_delay_off_store(struct device *dev,
>> +             struct device_attribute *attr, const char *buf, size_t size)
>> +{
>> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +     unsigned long state;
>> +     int ret;
>> +
>> +     ret = kstrtoul(buf, 0, &state);
>> +     if (ret)
>> +             return ret;
>> +
>> +     led_cdev->blink_delay_off = state;
>> +
>> +     return size;
>> +}
>> +
>> +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 DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);
>> +static DEVICE_ATTR(shot, 0200, NULL, led_shot);
>> +
>> +static void oneshot_trig_activate(struct led_classdev *led_cdev)
>> +{
>> +     struct oneshot_trig_data *oneshot_data;
>> +     int rc;
>> +
>> +     oneshot_data = kzalloc(sizeof(*oneshot_data), GFP_KERNEL);
>> +     if (!oneshot_data)
>> +             return;
>> +
>> +     led_cdev->trigger_data = oneshot_data;
>> +
>> +     rc = device_create_file(led_cdev->dev, &dev_attr_delay_on);
>> +     if (rc)
>> +             goto err_out_trig_data;
>> +     rc = device_create_file(led_cdev->dev, &dev_attr_delay_off);
>> +     if (rc)
>> +             goto err_out_delayon;
>> +     rc = device_create_file(led_cdev->dev, &dev_attr_invert);
>> +     if (rc)
>> +             goto err_out_delayoff;
>> +     rc = device_create_file(led_cdev->dev, &dev_attr_shot);
>> +     if (rc)
>> +             goto err_out_invert;
>> +
>> +     led_cdev->blink_delay_on = DEFAULT_DELAY;
>> +     led_cdev->blink_delay_off = DEFAULT_DELAY;
>> +
>> +     led_cdev->activated = true;
>> +
>> +     return;
>> +
>> +err_out_invert:
>> +     device_remove_file(led_cdev->dev, &dev_attr_invert);
>> +err_out_delayoff:
>> +     device_remove_file(led_cdev->dev, &dev_attr_delay_off);
>> +err_out_delayon:
>> +     device_remove_file(led_cdev->dev, &dev_attr_delay_on);
>> +err_out_trig_data:
>> +     kfree(led_cdev->trigger_data);
>> +}
>> +
>> +static void oneshot_trig_deactivate(struct led_classdev *led_cdev)
>> +{
>> +     struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
>> +
>> +     if (led_cdev->activated) {
>> +             device_remove_file(led_cdev->dev, &dev_attr_delay_on);
>> +             device_remove_file(led_cdev->dev, &dev_attr_delay_off);
>> +             device_remove_file(led_cdev->dev, &dev_attr_invert);
>> +             device_remove_file(led_cdev->dev, &dev_attr_shot);
>> +             kfree(oneshot_data);
>> +             led_cdev->activated = false;
>> +     }
>> +
>> +     /* Stop blinking */
>> +     led_brightness_set(led_cdev, LED_OFF);
>> +}
>> +
>> +static struct led_trigger oneshot_led_trigger = {
>> +     .name     = "oneshot",
>> +     .activate = oneshot_trig_activate,
>> +     .deactivate = oneshot_trig_deactivate,
>> +};
>> +
>> +static int __init oneshot_trig_init(void)
>> +{
>> +     return led_trigger_register(&oneshot_led_trigger);
>> +}
>> +
>> +static void __exit oneshot_trig_exit(void)
>> +{
>> +     led_trigger_unregister(&oneshot_led_trigger);
>> +}
>> +
>> +module_init(oneshot_trig_init);
>> +module_exit(oneshot_trig_exit);
>> +
>> +MODULE_AUTHOR("Fabio Baltieri <fabio.baltieri@gmail.com>");
>> +MODULE_DESCRIPTION("One-shot LED trigger");
>> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] leds: fix led_brightness_set when soft-blinking

[Bryan Wu]

On Wed, Jun 6, 2012 at 5:38 AM, Fabio Baltieri <fabio.baltieri@gmail.com> wrote:
> Change led_brightness_set to ensure software blink timer is stopped
> before calling led_stop_software_blink() and use led_set_brightness()
> instead of calling led_cdev->brightness_set() directly to ensure
> led_cdev->brightness is always consistent with current LED status.
>
> This ensure proper cleaning when changing triggers, as without this fix
> a LED may be turned off while leaving it's led_cdev->brightness = 1,
> leading to an erratic software-blink behaviour.
>
> The problem was easy to reproduce by changing the trigger from "timer"
> to "oneshot".
>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> Cc: Bryan Wu <bryan.wu@canonical.com>
> ---
>  drivers/leds/led-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 579eb78..3e9f934 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -115,7 +115,8 @@ EXPORT_SYMBOL(led_blink_set_oneshot);
>  void led_brightness_set(struct led_classdev *led_cdev,
>                        enum led_brightness brightness)
>  {
> +       del_timer_sync(&led_cdev->blink_timer);

This is not necessary, since led_stop_software_blink() will call
del_timer_sync() as you want.

>        led_stop_software_blink(led_cdev);
> -       led_cdev->brightness_set(led_cdev, brightness);
> +       led_set_brightness(led_cdev, brightness);
>  }
>  EXPORT_SYMBOL(led_brightness_set);
> --
> 1.7.10.2.520.g6a4a482.dirty
>

Thanks,
-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Re: [PATCH v2 1/2] led: add oneshot trigger

[Fabio Baltieri]

On Wed, Jun 06, 2012 at 10:56:44AM +0800, Bryan Wu wrote:
> On Wed, Jun 6, 2012 at 8:51 AM, Shuah Khan <shuahkhan@gmail.com> wrote:
> > On Tue, 2012-06-05 at 23:38 +0200, Fabio Baltieri wrote:
> >> Add oneshot trigger to blink a led with configurable parameters via
> >> sysfs.
> >
> > Fabio,
> >
> > What are the use-cases for this trigger. Please see comments inline:
> >
> >
> >>
> >> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> >> Cc: Shuah Khan <shuahkhan@gmail.com>
> >> Cc: Bryan Wu <bryan.wu@canonical.com>
> >> ---
> >> Hi all,
> >>
> >> that's my v2 of the ledtrig-oneshot trigger, with the correction suggested from
> >> Shuah in the original thread.  These include some unnecessary initialization
> >> and use of the new "activated" flag.
> >>
> >> I've kept the name to "oneshot" as I think it suits the role of the trigger as
> >> example/test for the led_blink_set_oneshot function but I have expanded a bit
> >> the Kconfig description to better identify the trigger's usage.
> >>
> >> About the namespace question, I think that naming the parameters delay_on and
> >> delay_off is ok as they have the same meaning as timer's ones (they also
> >> uses the same internal fields after all...).
> >>
> >> Also, I added a default set of delay_{on,off} to 100ms on activation, so that
> >> the trigger starts with a valid configuration, like ledtrig-timer does.
> >>
> >> Anything else that should be changed?
> >>
> >> Fabio
> >>
> >>  drivers/leds/Kconfig           |  14 +++
> >>  drivers/leds/Makefile          |   1 +
> >>  drivers/leds/ledtrig-oneshot.c | 198 +++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 213 insertions(+)
> >>  create mode 100644 drivers/leds/ledtrig-oneshot.c
> >>
> >> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> >> index 04cb8c8..1f151fb 100644
> >> --- a/drivers/leds/Kconfig
> >> +++ b/drivers/leds/Kconfig
> >> @@ -443,6 +443,20 @@ config LEDS_TRIGGER_TIMER
> >>
> >>         If unsure, say Y.
> >>
> >> +config LEDS_TRIGGER_ONESHOT
> >> +     tristate "LED One-shot Trigger"
> >> +     depends on LEDS_TRIGGERS
> >> +     help
> >> +       This allows LEDs to blink in one-shot pulses with parameters
> >> +       controlled via sysfs.  It's useful to notify the user on
> >> +       sporadic events, when there are no clear begin and end trap points,
> >> +       or on dense events, where this blinks the LED at constant rate if
> >> +       rearmed continuously.
> >> +
> >> +       It also shows how to use the led_blink_set_oneshot() function.
> >> +
> >> +       If unsure, say Y.
> >> +
> >>  config LEDS_TRIGGER_IDE_DISK
> >>       bool "LED IDE Disk Trigger"
> >>       depends on IDE_GD_ATA
> >> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> >> index f8958cd..9112d51 100644
> >> --- a/drivers/leds/Makefile
> >> +++ b/drivers/leds/Makefile
> >> @@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_DAC124S085)               += leds-dac124s085.o
> >>
> >>  # LED Triggers
> >>  obj-$(CONFIG_LEDS_TRIGGER_TIMER)     += ledtrig-timer.o
> >> +obj-$(CONFIG_LEDS_TRIGGER_ONESHOT)   += ledtrig-oneshot.o
> >>  obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)  += ledtrig-ide-disk.o
> >>  obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT) += ledtrig-heartbeat.o
> >>  obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT) += ledtrig-backlight.o
> >> diff --git a/drivers/leds/ledtrig-oneshot.c b/drivers/leds/ledtrig-oneshot.c
> >> new file mode 100644
> >> index 0000000..1aacdfe
> >> --- /dev/null
> >> +++ b/drivers/leds/ledtrig-oneshot.c
> >> @@ -0,0 +1,198 @@
> >> +/*
> >> + * One-shot LED Trigger
> >> + *
> >> + * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
> >> + *
> >> + * Based on ledtrig-timer.c by Richard Purdie <rpurdie@openedhand.com>
> >> + *
> >> + * 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/slab.h>
> >> +#include <linux/leds.h>
> >> +
> >> +#define DEFAULT_DELAY 100
> >> +
> >> +struct oneshot_trig_data {
> >> +     unsigned int invert;
> >> +};
> >> +
> >> +static ssize_t led_shot(struct device *dev,
> >> +             struct device_attribute *attr, const char *buf, size_t size)
> >> +{
> >> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >> +     struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
> >> +
> >> +     led_blink_set_oneshot(led_cdev,
> >> +                     &led_cdev->blink_delay_on, &led_cdev->blink_delay_off,
> >> +                     oneshot_data->invert);
> >
> > Here when shot is set
> >
> > echo n > shot
> >
> > What are the values for blink_delay_on and blink_delay_off. Does user
> > need to set delay_on and delay_off first? If user doesn't set these then
> > what values are used?
> >
> > What about invert? Does user need to do echo 1 > invert and then
> > activate shot?
> >
> >
> 
> Exactly, Fabio, what about adding a document for the use case and
> sysfs interface?

Ok, I'll post a v3 with an additional document for the trigger then.

Fabio

> 
> -Bryan
> 
> >> +
> >> +     /* content is ignored */
> >> +     return size;
> >> +}
> >> +static ssize_t led_invert_show(struct device *dev,
> >> +             struct device_attribute *attr, char *buf)
> >> +{
> >> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >> +     struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
> >> +
> >> +     return sprintf(buf, "%u\n", oneshot_data->invert);
> >> +}
> >> +
> >> +static ssize_t led_invert_store(struct device *dev,
> >> +             struct device_attribute *attr, const char *buf, size_t size)
> >> +{
> >> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >> +     struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
> >> +     unsigned long state;
> >> +     int ret;
> >> +
> >> +     ret = kstrtoul(buf, 0, &state);
> >> +     if (ret)
> >> +             return ret;
> >> +
> >> +     oneshot_data->invert = !!state;
> >> +
> >> +     return size;
> >> +}
> >> +
> >> +static ssize_t led_delay_on_show(struct device *dev,
> >> +             struct device_attribute *attr, char *buf)
> >> +{
> >> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >> +
> >> +     return sprintf(buf, "%lu\n", led_cdev->blink_delay_on);
> >> +}
> >> +
> >> +static ssize_t led_delay_on_store(struct device *dev,
> >> +             struct device_attribute *attr, const char *buf, size_t size)
> >> +{
> >> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >> +     unsigned long state;
> >> +     int ret;
> >> +
> >> +     ret = kstrtoul(buf, 0, &state);
> >> +     if (ret)
> >> +             return ret;
> >> +
> >> +     led_cdev->blink_delay_on = state;
> >> +
> >> +     return size;
> >> +}
> >> +static ssize_t led_delay_off_show(struct device *dev,
> >> +             struct device_attribute *attr, char *buf)
> >> +{
> >> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >> +
> >> +     return sprintf(buf, "%lu\n", led_cdev->blink_delay_off);
> >> +}
> >> +
> >> +static ssize_t led_delay_off_store(struct device *dev,
> >> +             struct device_attribute *attr, const char *buf, size_t size)
> >> +{
> >> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >> +     unsigned long state;
> >> +     int ret;
> >> +
> >> +     ret = kstrtoul(buf, 0, &state);
> >> +     if (ret)
> >> +             return ret;
> >> +
> >> +     led_cdev->blink_delay_off = state;
> >> +
> >> +     return size;
> >> +}
> >> +
> >> +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 DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);
> >> +static DEVICE_ATTR(shot, 0200, NULL, led_shot);
> >> +
> >> +static void oneshot_trig_activate(struct led_classdev *led_cdev)
> >> +{
> >> +     struct oneshot_trig_data *oneshot_data;
> >> +     int rc;
> >> +
> >> +     oneshot_data = kzalloc(sizeof(*oneshot_data), GFP_KERNEL);
> >> +     if (!oneshot_data)
> >> +             return;
> >> +
> >> +     led_cdev->trigger_data = oneshot_data;
> >> +
> >> +     rc = device_create_file(led_cdev->dev, &dev_attr_delay_on);
> >> +     if (rc)
> >> +             goto err_out_trig_data;
> >> +     rc = device_create_file(led_cdev->dev, &dev_attr_delay_off);
> >> +     if (rc)
> >> +             goto err_out_delayon;
> >> +     rc = device_create_file(led_cdev->dev, &dev_attr_invert);
> >> +     if (rc)
> >> +             goto err_out_delayoff;
> >> +     rc = device_create_file(led_cdev->dev, &dev_attr_shot);
> >> +     if (rc)
> >> +             goto err_out_invert;
> >> +
> >> +     led_cdev->blink_delay_on = DEFAULT_DELAY;
> >> +     led_cdev->blink_delay_off = DEFAULT_DELAY;
> >> +
> >> +     led_cdev->activated = true;
> >> +
> >> +     return;
> >> +
> >> +err_out_invert:
> >> +     device_remove_file(led_cdev->dev, &dev_attr_invert);
> >> +err_out_delayoff:
> >> +     device_remove_file(led_cdev->dev, &dev_attr_delay_off);
> >> +err_out_delayon:
> >> +     device_remove_file(led_cdev->dev, &dev_attr_delay_on);
> >> +err_out_trig_data:
> >> +     kfree(led_cdev->trigger_data);
> >> +}
> >> +
> >> +static void oneshot_trig_deactivate(struct led_classdev *led_cdev)
> >> +{
> >> +     struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
> >> +
> >> +     if (led_cdev->activated) {
> >> +             device_remove_file(led_cdev->dev, &dev_attr_delay_on);
> >> +             device_remove_file(led_cdev->dev, &dev_attr_delay_off);
> >> +             device_remove_file(led_cdev->dev, &dev_attr_invert);
> >> +             device_remove_file(led_cdev->dev, &dev_attr_shot);
> >> +             kfree(oneshot_data);
> >> +             led_cdev->activated = false;
> >> +     }
> >> +
> >> +     /* Stop blinking */
> >> +     led_brightness_set(led_cdev, LED_OFF);
> >> +}
> >> +
> >> +static struct led_trigger oneshot_led_trigger = {
> >> +     .name     = "oneshot",
> >> +     .activate = oneshot_trig_activate,
> >> +     .deactivate = oneshot_trig_deactivate,
> >> +};
> >> +
> >> +static int __init oneshot_trig_init(void)
> >> +{
> >> +     return led_trigger_register(&oneshot_led_trigger);
> >> +}
> >> +
> >> +static void __exit oneshot_trig_exit(void)
> >> +{
> >> +     led_trigger_unregister(&oneshot_led_trigger);
> >> +}
> >> +
> >> +module_init(oneshot_trig_init);
> >> +module_exit(oneshot_trig_exit);
> >> +
> >> +MODULE_AUTHOR("Fabio Baltieri <fabio.baltieri@gmail.com>");
> >> +MODULE_DESCRIPTION("One-shot LED trigger");
> >> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] leds: fix led_brightness_set when soft-blinking

[Fabio Baltieri]

On Wed, Jun 06, 2012 at 11:58:10AM +0800, Bryan Wu wrote:
> On Wed, Jun 6, 2012 at 5:38 AM, Fabio Baltieri <fabio.baltieri@gmail.com> wrote:
> > Change led_brightness_set to ensure software blink timer is stopped
> > before calling led_stop_software_blink() and use led_set_brightness()
> > instead of calling led_cdev->brightness_set() directly to ensure
> > led_cdev->brightness is always consistent with current LED status.
> >
> > This ensure proper cleaning when changing triggers, as without this fix
> > a LED may be turned off while leaving it's led_cdev->brightness = 1,
> > leading to an erratic software-blink behaviour.
> >
> > The problem was easy to reproduce by changing the trigger from "timer"
> > to "oneshot".
> >
> > Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> > Cc: Bryan Wu <bryan.wu@canonical.com>
> > ---
> >  drivers/leds/led-core.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> > index 579eb78..3e9f934 100644
> > --- a/drivers/leds/led-core.c
> > +++ b/drivers/leds/led-core.c
> > @@ -115,7 +115,8 @@ EXPORT_SYMBOL(led_blink_set_oneshot);
> >  void led_brightness_set(struct led_classdev *led_cdev,
> >                        enum led_brightness brightness)
> >  {
> > +       del_timer_sync(&led_cdev->blink_timer);
> 
> This is not necessary, since led_stop_software_blink() will call
> del_timer_sync() as you want.

Actually I removed it in the oneshot patch because it was only needed in
the non-oneshot variant of led_blink_set, but I may put it back where it
was and move led_stop_software_blink earlier into led_blink_set... do
you think it's better?

> 
> >        led_stop_software_blink(led_cdev);
> > -       led_cdev->brightness_set(led_cdev, brightness);
> > +       led_set_brightness(led_cdev, brightness);
> >  }
> >  EXPORT_SYMBOL(led_brightness_set);
> > --
> > 1.7.10.2.520.g6a4a482.dirty
> >
> 
> Thanks,
> -- 
> Bryan Wu <bryan.wu@canonical.com>
> Kernel Developer    +86.186-168-78255 Mobile
> Canonical Ltd.      www.canonical.com
> Ubuntu - Linux for human beings | www.ubuntu.com

[PATCH v2] leds: fix led_brightness_set when soft-blinking

[Fabio Baltieri]

Put del_timer_sync() back into led_stop_software_blink() and move the
call earlier into led_blink_set() to ensure software blink timer is
stopped when changing trigger.  Also use led_set_brightness() instead of
calling led_cdev->brightness_set() directly to ensure
led_cdev->brightness is always consistent with current LED status.

This ensure proper cleaning when changing triggers, as without this fix
a LED may be turned off while leaving it's led_cdev->brightness = 1,
leading to an erratic software-blink behaviour.

The problem was easy to reproduce by changing the trigger from "timer"
to "oneshot".

Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
Cc: Bryan Wu <bryan.wu@canonical.com>
---

...maybe that version makes more sense.

Fabio

 drivers/leds/led-core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 579eb78..2477109 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -27,6 +27,7 @@ EXPORT_SYMBOL_GPL(leds_list);
 static void led_stop_software_blink(struct led_classdev *led_cdev)
 {
 	/* deactivate previous settings */
+	del_timer_sync(&led_cdev->blink_timer);
 	led_cdev->blink_delay_on = 0;
 	led_cdev->blink_delay_off = 0;
 }
@@ -43,8 +44,6 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
 	if (!led_cdev->blink_brightness)
 		led_cdev->blink_brightness = led_cdev->max_brightness;
 
-	led_stop_software_blink(led_cdev);
-
 	led_cdev->blink_delay_on = delay_on;
 	led_cdev->blink_delay_off = delay_off;
 
@@ -82,7 +81,7 @@ void led_blink_set(struct led_classdev *led_cdev,
 		   unsigned long *delay_on,
 		   unsigned long *delay_off)
 {
-	del_timer_sync(&led_cdev->blink_timer);
+	led_stop_software_blink(led_cdev);
 
 	led_cdev->flags &= ~LED_BLINK_ONESHOT;
 	led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
@@ -116,6 +115,6 @@ void led_brightness_set(struct led_classdev *led_cdev,
 			enum led_brightness brightness)
 {
 	led_stop_software_blink(led_cdev);
-	led_cdev->brightness_set(led_cdev, brightness);
+	led_set_brightness(led_cdev, brightness);
 }
 EXPORT_SYMBOL(led_brightness_set);
-- 
1.7.11.rc1.9.gf623ca1.dirty

Re: [PATCH v2] leds: fix led_brightness_set when soft-blinking

[Bryan Wu]

On Wed, Jun 6, 2012 at 3:19 PM, Fabio Baltieri <fabio.baltieri@gmail.com> wrote:
> Put del_timer_sync() back into led_stop_software_blink() and move the
> call earlier into led_blink_set() to ensure software blink timer is
> stopped when changing trigger.  Also use led_set_brightness() instead of
> calling led_cdev->brightness_set() directly to ensure
> led_cdev->brightness is always consistent with current LED status.
>
> This ensure proper cleaning when changing triggers, as without this fix
> a LED may be turned off while leaving it's led_cdev->brightness = 1,
> leading to an erratic software-blink behaviour.
>
> The problem was easy to reproduce by changing the trigger from "timer"
> to "oneshot".
>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> Cc: Bryan Wu <bryan.wu@canonical.com>

Looks fine and actually a patch fixed similar issue before in my
fixes-for-3.5 branch.
http://git.kernel.org/?p=linux/kernel/git/cooloney/linux-leds.git;a=commitdiff;h=b2f819714f40a6593c1ed20b69573d5fef71f392
I plan to send out these fixes soon.

I have to rebase all patches in for-next on top of fixes, then I met
some conflicts. After fixing conflicts, I rebuilt the for-next tree
which contains 3 patches from you. Please grab it and test, I will try
that on my hardware.
http://git.kernel.org/?p=linux/kernel/git/cooloney/linux-leds.git;a=shortlog;h=refs/heads/for-next

Thanks,
-Bryan

> ---
>
> ...maybe that version makes more sense.
>
> Fabio
>
>  drivers/leds/led-core.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 579eb78..2477109 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -27,6 +27,7 @@ EXPORT_SYMBOL_GPL(leds_list);
>  static void led_stop_software_blink(struct led_classdev *led_cdev)
>  {
>        /* deactivate previous settings */
> +       del_timer_sync(&led_cdev->blink_timer);
>        led_cdev->blink_delay_on = 0;
>        led_cdev->blink_delay_off = 0;
>  }
> @@ -43,8 +44,6 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>        if (!led_cdev->blink_brightness)
>                led_cdev->blink_brightness = led_cdev->max_brightness;
>
> -       led_stop_software_blink(led_cdev);
> -
>        led_cdev->blink_delay_on = delay_on;
>        led_cdev->blink_delay_off = delay_off;
>
> @@ -82,7 +81,7 @@ void led_blink_set(struct led_classdev *led_cdev,
>                   unsigned long *delay_on,
>                   unsigned long *delay_off)
>  {
> -       del_timer_sync(&led_cdev->blink_timer);
> +       led_stop_software_blink(led_cdev);
>
>        led_cdev->flags &= ~LED_BLINK_ONESHOT;
>        led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> @@ -116,6 +115,6 @@ void led_brightness_set(struct led_classdev *led_cdev,
>                        enum led_brightness brightness)
>  {
>        led_stop_software_blink(led_cdev);
> -       led_cdev->brightness_set(led_cdev, brightness);
> +       led_set_brightness(led_cdev, brightness);
>  }
>  EXPORT_SYMBOL(led_brightness_set);
> --
> 1.7.11.rc1.9.gf623ca1.dirty
>



-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Re: [PATCH v2] leds: fix led_brightness_set when soft-blinking

[Fabio Baltieri]

Hi Bryan,

On Wed, Jun 06, 2012 at 04:19:05PM +0800, Bryan Wu wrote:
> On Wed, Jun 6, 2012 at 3:19 PM, Fabio Baltieri <fabio.baltieri@gmail.com> wrote:
> > Put del_timer_sync() back into led_stop_software_blink() and move the
> > call earlier into led_blink_set() to ensure software blink timer is
> > stopped when changing trigger.  Also use led_set_brightness() instead of
> > calling led_cdev->brightness_set() directly to ensure
> > led_cdev->brightness is always consistent with current LED status.
> >
> > This ensure proper cleaning when changing triggers, as without this fix
> > a LED may be turned off while leaving it's led_cdev->brightness = 1,
> > leading to an erratic software-blink behaviour.
> >
> > The problem was easy to reproduce by changing the trigger from "timer"
> > to "oneshot".
> >
> > Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> > Cc: Bryan Wu <bryan.wu@canonical.com>
> 
> Looks fine and actually a patch fixed similar issue before in my
> fixes-for-3.5 branch.
> http://git.kernel.org/?p=linux/kernel/git/cooloney/linux-leds.git;a=commitdiff;h=b2f819714f40a6593c1ed20b69573d5fef71f392
> I plan to send out these fixes soon.
> 
> I have to rebase all patches in for-next on top of fixes, then I met
> some conflicts. After fixing conflicts, I rebuilt the for-next tree
> which contains 3 patches from you. Please grab it and test, I will try
> that on my hardware.
> http://git.kernel.org/?p=linux/kernel/git/cooloney/linux-leds.git;a=shortlog;h=refs/heads/for-next

I see the rebase but it looks like Rafal's patch was lost and that
condition in led_set_software_blink() re-appeared as the line removal
vanished from my patch but Rafal's one was not applied.

Can you check the rebase? I think that putting 

http://git.kernel.org/?p=linux/kernel/git/cooloney/linux-leds.git;a=commitdiff;h=b2f819714f40a6593c1ed20b69573d5fef71f392

before my "addoneshot blink functions" should fix the code.

Fabio

> 
> Thanks,
> -Bryan
> 
> > ---
> >
> > ...maybe that version makes more sense.
> >
> > Fabio
> >
> >  drivers/leds/led-core.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> > index 579eb78..2477109 100644
> > --- a/drivers/leds/led-core.c
> > +++ b/drivers/leds/led-core.c
> > @@ -27,6 +27,7 @@ EXPORT_SYMBOL_GPL(leds_list);
> >  static void led_stop_software_blink(struct led_classdev *led_cdev)
> >  {
> >        /* deactivate previous settings */
> > +       del_timer_sync(&led_cdev->blink_timer);
> >        led_cdev->blink_delay_on = 0;
> >        led_cdev->blink_delay_off = 0;
> >  }
> > @@ -43,8 +44,6 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
> >        if (!led_cdev->blink_brightness)
> >                led_cdev->blink_brightness = led_cdev->max_brightness;
> >
> > -       led_stop_software_blink(led_cdev);
> > -
> >        led_cdev->blink_delay_on = delay_on;
> >        led_cdev->blink_delay_off = delay_off;
> >
> > @@ -82,7 +81,7 @@ void led_blink_set(struct led_classdev *led_cdev,
> >                   unsigned long *delay_on,
> >                   unsigned long *delay_off)
> >  {
> > -       del_timer_sync(&led_cdev->blink_timer);
> > +       led_stop_software_blink(led_cdev);
> >
> >        led_cdev->flags &= ~LED_BLINK_ONESHOT;
> >        led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> > @@ -116,6 +115,6 @@ void led_brightness_set(struct led_classdev *led_cdev,
> >                        enum led_brightness brightness)
> >  {
> >        led_stop_software_blink(led_cdev);
> > -       led_cdev->brightness_set(led_cdev, brightness);
> > +       led_set_brightness(led_cdev, brightness);
> >  }
> >  EXPORT_SYMBOL(led_brightness_set);
> > --
> > 1.7.11.rc1.9.gf623ca1.dirty
> >
> 
> 
> 
> -- 
> Bryan Wu <bryan.wu@canonical.com>
> Kernel Developer    +86.186-168-78255 Mobile
> Canonical Ltd.      www.canonical.com
> Ubuntu - Linux for human beings | www.ubuntu.com

Re: [PATCH v2] leds: fix led_brightness_set when soft-blinking

[Bryan Wu]

On Wed, Jun 6, 2012 at 7:10 PM, Fabio Baltieri <fabio.baltieri@gmail.com> wrote:
> Hi Bryan,
>
> On Wed, Jun 06, 2012 at 04:19:05PM +0800, Bryan Wu wrote:
>> On Wed, Jun 6, 2012 at 3:19 PM, Fabio Baltieri <fabio.baltieri@gmail.com> wrote:
>> > Put del_timer_sync() back into led_stop_software_blink() and move the
>> > call earlier into led_blink_set() to ensure software blink timer is
>> > stopped when changing trigger.  Also use led_set_brightness() instead of
>> > calling led_cdev->brightness_set() directly to ensure
>> > led_cdev->brightness is always consistent with current LED status.
>> >
>> > This ensure proper cleaning when changing triggers, as without this fix
>> > a LED may be turned off while leaving it's led_cdev->brightness = 1,
>> > leading to an erratic software-blink behaviour.
>> >
>> > The problem was easy to reproduce by changing the trigger from "timer"
>> > to "oneshot".
>> >
>> > Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
>> > Cc: Bryan Wu <bryan.wu@canonical.com>
>>
>> Looks fine and actually a patch fixed similar issue before in my
>> fixes-for-3.5 branch.
>> http://git.kernel.org/?p=linux/kernel/git/cooloney/linux-leds.git;a=commitdiff;h=b2f819714f40a6593c1ed20b69573d5fef71f392
>> I plan to send out these fixes soon.
>>
>> I have to rebase all patches in for-next on top of fixes, then I met
>> some conflicts. After fixing conflicts, I rebuilt the for-next tree
>> which contains 3 patches from you. Please grab it and test, I will try
>> that on my hardware.
>> http://git.kernel.org/?p=linux/kernel/git/cooloney/linux-leds.git;a=shortlog;h=refs/heads/for-next
>
> I see the rebase but it looks like Rafal's patch was lost and that
> condition in led_set_software_blink() re-appeared as the line removal
> vanished from my patch but Rafal's one was not applied.
>
> Can you check the rebase? I think that putting
>
> http://git.kernel.org/?p=linux/kernel/git/cooloney/linux-leds.git;a=commitdiff;h=b2f819714f40a6593c1ed20b69573d5fef71f392
>
> before my "addoneshot blink functions" should fix the code.
>

Yeah, that's right. Rafal's patch was in my fixes-for-3.5 branch, I
will send out by this week for Linus. So I have to rebase all the
for-next patches on top of it. Although I got conflicts, I will fix
that. I prepared a new branch merged for-next and fixes-for-3.5
together name devel, please help to test.

http://git.kernel.org/?p=linux/kernel/git/cooloney/linux-leds.git;a=shortlog;h=refs/heads/devel

-Bryan

>> > ---
>> >
>> > ...maybe that version makes more sense.
>> >
>> > Fabio
>> >
>> >  drivers/leds/led-core.c | 7 +++----
>> >  1 file changed, 3 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> > index 579eb78..2477109 100644
>> > --- a/drivers/leds/led-core.c
>> > +++ b/drivers/leds/led-core.c
>> > @@ -27,6 +27,7 @@ EXPORT_SYMBOL_GPL(leds_list);
>> >  static void led_stop_software_blink(struct led_classdev *led_cdev)
>> >  {
>> >        /* deactivate previous settings */
>> > +       del_timer_sync(&led_cdev->blink_timer);
>> >        led_cdev->blink_delay_on = 0;
>> >        led_cdev->blink_delay_off = 0;
>> >  }
>> > @@ -43,8 +44,6 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>> >        if (!led_cdev->blink_brightness)
>> >                led_cdev->blink_brightness = led_cdev->max_brightness;
>> >
>> > -       led_stop_software_blink(led_cdev);
>> > -
>> >        led_cdev->blink_delay_on = delay_on;
>> >        led_cdev->blink_delay_off = delay_off;
>> >
>> > @@ -82,7 +81,7 @@ void led_blink_set(struct led_classdev *led_cdev,
>> >                   unsigned long *delay_on,
>> >                   unsigned long *delay_off)
>> >  {
>> > -       del_timer_sync(&led_cdev->blink_timer);
>> > +       led_stop_software_blink(led_cdev);
>> >
>> >        led_cdev->flags &= ~LED_BLINK_ONESHOT;
>> >        led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
>> > @@ -116,6 +115,6 @@ void led_brightness_set(struct led_classdev *led_cdev,
>> >                        enum led_brightness brightness)
>> >  {
>> >        led_stop_software_blink(led_cdev);
>> > -       led_cdev->brightness_set(led_cdev, brightness);
>> > +       led_set_brightness(led_cdev, brightness);
>> >  }
>> >  EXPORT_SYMBOL(led_brightness_set);
>> > --
>> > 1.7.11.rc1.9.gf623ca1.dirty
>> >
>>
>>
>>
>> --
>> Bryan Wu <bryan.wu@canonical.com>
>> Kernel Developer    +86.186-168-78255 Mobile
>> Canonical Ltd.      www.canonical.com
>> Ubuntu - Linux for human beings | www.ubuntu.com



-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Re: [PATCH v2] leds: fix led_brightness_set when soft-blinking

[Fabio Baltieri]

Hi Bryan,

On Wed, Jun 06, 2012 at 10:10:23PM +0800, Bryan Wu wrote:
> Yeah, that's right. Rafal's patch was in my fixes-for-3.5 branch, I
> will send out by this week for Linus. So I have to rebase all the
> for-next patches on top of it. Although I got conflicts, I will fix
> that. I prepared a new branch merged for-next and fixes-for-3.5
> together name devel, please help to test.
> 
> http://git.kernel.org/?p=linux/kernel/git/cooloney/linux-leds.git;a=shortlog;h=refs/heads/devel

I realize now that my last patch broke ledtrig-timer updates as that
works by passing delay_{on,off} values by pointer, and moving
led_stop_software_blink() earlier destroy the other value.

That's a bit of a pitfall, as the old code was working because values
were copied when entering led_set_software_blink.

I see three options here:
- reverting back my leds: "fix led_brightness_set when soft-blinking"
  (9b05cd0) to the first version I posted, wich should work as before.
- modify ledtrig-timer to use two internal variables to store delay_on
  and delay_off instead of the led_cdev ones.
- moving the two led_cdev->blink_delay_xx = 0 only into
  led_brightness_set, as that's the only place when they are needed.

What you think about it?

I think the third option is the best, as it removes the pitfall and
simplify the code a bit.  I'll post that as a v3 "fix led_brightness_set
when soft-blinking", would you consider replacing the last patch you
applied in the new branch with the v3?

Fabio

[PATCH v3] leds: fix led_brightness_set when soft-blinking

[Fabio Baltieri]

Move led_stop_software_blink() code into led_brightness_set() to ensure
software blink timer is stopped and cleared when changing trigger.

Also use led_set_brightness() instead of calling
led_cdev->brightness_set() directly to keep led_cdev->brightness
consistent with current LED status.

This ensure proper cleaning when changing triggers, as without this fix
a LED may be turned off while leaving it's led_cdev->brightness = 1,
leading to an erratic software-blink behaviour.

The problem was easy to reproduce by changing the trigger from "timer"
to "oneshot".

Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
Cc: Bryan Wu <bryan.wu@canonical.com>
---
 drivers/leds/led-core.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index a6f4d91..31f1f78 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -24,13 +24,6 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
 LIST_HEAD(leds_list);
 EXPORT_SYMBOL_GPL(leds_list);
 
-static void led_stop_software_blink(struct led_classdev *led_cdev)
-{
-	/* deactivate previous settings */
-	led_cdev->blink_delay_on = 0;
-	led_cdev->blink_delay_off = 0;
-}
-
 static void led_set_software_blink(struct led_classdev *led_cdev,
 				   unsigned long delay_on,
 				   unsigned long delay_off)
@@ -113,7 +106,11 @@ EXPORT_SYMBOL(led_blink_set_oneshot);
 void led_brightness_set(struct led_classdev *led_cdev,
 			enum led_brightness brightness)
 {
-	led_stop_software_blink(led_cdev);
-	led_cdev->brightness_set(led_cdev, brightness);
+	/* stop and clear soft-blink timer */
+	del_timer_sync(&led_cdev->blink_timer);
+	led_cdev->blink_delay_on = 0;
+	led_cdev->blink_delay_off = 0;
+
+	led_set_brightness(led_cdev, brightness);
 }
 EXPORT_SYMBOL(led_brightness_set);
-- 
1.7.11.rc1.9.gf623ca1.dirty

Re: [PATCH v2] leds: fix led_brightness_set when soft-blinking

[Shuah Khan]

> I realize now that my last patch broke ledtrig-timer updates as that
> works by passing delay_{on,off} values by pointer, and moving
> led_stop_software_blink() earlier destroy the other value.

Fabio,

This type of interaction is what I am concerned about, and came up
during the transient trigger discussion as well. In the case of
transient trigger it was an easy decision to use separate namespace, but
that doesn't make sense in this case.

> 
> That's a bit of a pitfall, as the old code was working because values
> were copied when entering led_set_software_blink.
> 
> I see three options here:
> - reverting back my leds: "fix led_brightness_set when soft-blinking"
>   (9b05cd0) to the first version I posted, wich should work as before.
> - modify ledtrig-timer to use two internal variables to store delay_on
>   and delay_off instead of the led_cdev ones.

Don't this this is a viable option without restructuring the leds code.
These two variables are common to led_cdev and used by other drivers as
well. ledtrig-timer will still have to store it as these are used in the
led_timer_function() in led-class.c

> - moving the two led_cdev->blink_delay_xx = 0 only into
>   led_brightness_set, as that's the only place when they are needed.

Sounds reasonable. I would like to pull your patches in and do some
testing, but very likely I won't be able to get to it until this
weekend. This is where use-cases will help as well for us go through
various transitions and see if it all comes together.

-- Shuah

[PATCH v3] led: add oneshot trigger

[Fabio Baltieri]

Add oneshot trigger to blink a led with configurale parameters via
sysfs.

Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
Cc: Shuah Khan <shuahkhan@gmail.com>
Cc: Bryan Wu <bryan.wu@canonical.com>
---

Here is the v3, with added documentation file with short description, sysfs
description, use case example (the one I developed ths for, actually) and
default values.

Also, while trying the use case, I added instantaneous setting of led state when
setting the "invert" attribute.

Let me know what you think of this!

Fabio

 Documentation/leds/ledtrig-oneshot.txt |  59 ++++++++++
 drivers/leds/Kconfig                   |  14 +++
 drivers/leds/Makefile                  |   1 +
 drivers/leds/ledtrig-oneshot.c         | 204 +++++++++++++++++++++++++++++++++
 4 files changed, 278 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-oneshot.txt
 create mode 100644 drivers/leds/ledtrig-oneshot.c

diff --git a/Documentation/leds/ledtrig-oneshot.txt b/Documentation/leds/ledtrig-oneshot.txt
new file mode 100644
index 0000000..07cd1fa
--- /dev/null
+++ b/Documentation/leds/ledtrig-oneshot.txt
@@ -0,0 +1,59 @@
+One-shot LED Trigger
+====================
+
+This is a LED trigger useful for signaling the user of an event where there are
+no clear trap points to put standard led-on and led-off settings.  Using this
+trigger, the application needs only to signal the trigger when an event has
+happened, than the trigger turns the LED on and than keeps it off for a
+specified amount of time.
+
+This trigger is meant to be usable both for sporadic and dense events.  In the
+first case, the trigger produces a clear single controlled blink for each
+event, while in the latter it keeps blinking at constant rate, as to signal
+that the events are arriving continuously.
+
+A one-shot LED only stays in a constant state when there are no events.  An
+additional "invert" property specifies if the LED has to stay off (normal) or
+on (inverted) when not rearmed.
+
+The trigger can be activated from user space on led class devices as shown
+below:
+
+  echo oneshot > trigger
+
+This adds the following sysfs attributes to the LED:
+
+  delay_on - specifies for how many milliseconds the LED has to stay at
+             LED_FULL brightness after it has been armed.
+             Default to 100 ms.
+
+  delay_off - specifies for how many milliseconds the LED has to stay at
+              LED_OFF brightness after it has been armed.
+              Default to 100 ms.
+
+  invert - reverse the blink logic.  If set to 0 (default) blink on for delay_on
+           ms, then blink off for delay_off ms, leaving the LED normally off.  If
+           set to 1, blink off for delay_off ms, then blink on for delay_on ms,
+           leaving the LED normally on.
+           Setting this value also immediately change the LED state.
+
+  shot - write any non-empty string to signal an events, this starts a blink
+         sequence if not already running.
+
+Example use-case: network devices, initialization:
+
+  echo oneshot > trigger # set trigger for this led
+  echo 33 > delay_on     # blink at 1 / (33 + 33) Hz on continuous traffic
+  echo 33 > delay_off
+
+interface goes up:
+
+  echo 1 > invert # set led as normally-on, turn the led on
+
+packet received/transmitted:
+
+  echo 1 > shot # led starts blinking, ignored if already blinking
+
+interface goes down
+
+  echo 0 > invert # set led as normally-off, turn the led off
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 04cb8c8..1f151fb 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -443,6 +443,20 @@ config LEDS_TRIGGER_TIMER
 
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_ONESHOT
+	tristate "LED One-shot Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  This allows LEDs to blink in one-shot pulses with parameters
+	  controlled via sysfs.  It's useful to notify the user on
+	  sporadic events, when there are no clear begin and end trap points,
+	  or on dense events, where this blinks the LED at constant rate if
+	  rearmed continuously.
+
+	  It also shows how to use the led_blink_set_oneshot() function.
+
+	  If unsure, say Y.
+
 config LEDS_TRIGGER_IDE_DISK
 	bool "LED IDE Disk Trigger"
 	depends on IDE_GD_ATA
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index f8958cd..9112d51 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
 
 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
+obj-$(CONFIG_LEDS_TRIGGER_ONESHOT)	+= ledtrig-oneshot.o
 obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)	+= ledtrig-ide-disk.o
 obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
 obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)	+= ledtrig-backlight.o
diff --git a/drivers/leds/ledtrig-oneshot.c b/drivers/leds/ledtrig-oneshot.c
new file mode 100644
index 0000000..5c9edf7
--- /dev/null
+++ b/drivers/leds/ledtrig-oneshot.c
@@ -0,0 +1,204 @@
+/*
+ * One-shot LED Trigger
+ *
+ * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
+ *
+ * Based on ledtrig-timer.c by Richard Purdie <rpurdie@openedhand.com>
+ *
+ * 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/slab.h>
+#include <linux/leds.h>
+#include "leds.h"
+
+#define DEFAULT_DELAY 100
+
+struct oneshot_trig_data {
+	unsigned int invert;
+};
+
+static ssize_t led_shot(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
+
+	led_blink_set_oneshot(led_cdev,
+			&led_cdev->blink_delay_on, &led_cdev->blink_delay_off,
+			oneshot_data->invert);
+
+	/* content is ignored */
+	return size;
+}
+static ssize_t led_invert_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
+
+	return sprintf(buf, "%u\n", oneshot_data->invert);
+}
+
+static ssize_t led_invert_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
+	unsigned long state;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &state);
+	if (ret)
+		return ret;
+
+	oneshot_data->invert = !!state;
+
+	if (oneshot_data->invert)
+		led_set_brightness(led_cdev, LED_FULL);
+	else
+		led_set_brightness(led_cdev, LED_OFF);
+
+	return size;
+}
+
+static ssize_t led_delay_on_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%lu\n", led_cdev->blink_delay_on);
+}
+
+static ssize_t led_delay_on_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	unsigned long state;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &state);
+	if (ret)
+		return ret;
+
+	led_cdev->blink_delay_on = state;
+
+	return size;
+}
+static ssize_t led_delay_off_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%lu\n", led_cdev->blink_delay_off);
+}
+
+static ssize_t led_delay_off_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	unsigned long state;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &state);
+	if (ret)
+		return ret;
+
+	led_cdev->blink_delay_off = state;
+
+	return size;
+}
+
+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 DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);
+static DEVICE_ATTR(shot, 0200, NULL, led_shot);
+
+static void oneshot_trig_activate(struct led_classdev *led_cdev)
+{
+	struct oneshot_trig_data *oneshot_data;
+	int rc;
+
+	oneshot_data = kzalloc(sizeof(*oneshot_data), GFP_KERNEL);
+	if (!oneshot_data)
+		return;
+
+	led_cdev->trigger_data = oneshot_data;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_delay_on);
+	if (rc)
+		goto err_out_trig_data;
+	rc = device_create_file(led_cdev->dev, &dev_attr_delay_off);
+	if (rc)
+		goto err_out_delayon;
+	rc = device_create_file(led_cdev->dev, &dev_attr_invert);
+	if (rc)
+		goto err_out_delayoff;
+	rc = device_create_file(led_cdev->dev, &dev_attr_shot);
+	if (rc)
+		goto err_out_invert;
+
+	led_cdev->blink_delay_on = DEFAULT_DELAY;
+	led_cdev->blink_delay_off = DEFAULT_DELAY;
+
+	led_cdev->activated = true;
+
+	return;
+
+err_out_invert:
+	device_remove_file(led_cdev->dev, &dev_attr_invert);
+err_out_delayoff:
+	device_remove_file(led_cdev->dev, &dev_attr_delay_off);
+err_out_delayon:
+	device_remove_file(led_cdev->dev, &dev_attr_delay_on);
+err_out_trig_data:
+	kfree(led_cdev->trigger_data);
+}
+
+static void oneshot_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
+
+	if (led_cdev->activated) {
+		device_remove_file(led_cdev->dev, &dev_attr_delay_on);
+		device_remove_file(led_cdev->dev, &dev_attr_delay_off);
+		device_remove_file(led_cdev->dev, &dev_attr_invert);
+		device_remove_file(led_cdev->dev, &dev_attr_shot);
+		kfree(oneshot_data);
+		led_cdev->activated = false;
+	}
+
+	/* Stop blinking */
+	led_brightness_set(led_cdev, LED_OFF);
+}
+
+static struct led_trigger oneshot_led_trigger = {
+	.name     = "oneshot",
+	.activate = oneshot_trig_activate,
+	.deactivate = oneshot_trig_deactivate,
+};
+
+static int __init oneshot_trig_init(void)
+{
+	return led_trigger_register(&oneshot_led_trigger);
+}
+
+static void __exit oneshot_trig_exit(void)
+{
+	led_trigger_unregister(&oneshot_led_trigger);
+}
+
+module_init(oneshot_trig_init);
+module_exit(oneshot_trig_exit);
+
+MODULE_AUTHOR("Fabio Baltieri <fabio.baltieri@gmail.com>");
+MODULE_DESCRIPTION("One-shot LED trigger");
+MODULE_LICENSE("GPL");
-- 
1.7.11.rc1.9.gf623ca1.dirty

Re: [PATCH v2] leds: fix led_brightness_set when soft-blinking

[Bryan Wu]

On Thu, Jun 7, 2012 at 4:30 AM, Shuah Khan <shuahkhan@gmail.com> wrote:
>
>> I realize now that my last patch broke ledtrig-timer updates as that
>> works by passing delay_{on,off} values by pointer, and moving
>> led_stop_software_blink() earlier destroy the other value.
>
> Fabio,
>
> This type of interaction is what I am concerned about, and came up
> during the transient trigger discussion as well. In the case of
> transient trigger it was an easy decision to use separate namespace, but
> that doesn't make sense in this case.
>
>>
>> That's a bit of a pitfall, as the old code was working because values
>> were copied when entering led_set_software_blink.
>>
>> I see three options here:
>> - reverting back my leds: "fix led_brightness_set when soft-blinking"
>>   (9b05cd0) to the first version I posted, wich should work as before.
>> - modify ledtrig-timer to use two internal variables to store delay_on
>>   and delay_off instead of the led_cdev ones.
>
> Don't this this is a viable option without restructuring the leds code.
> These two variables are common to led_cdev and used by other drivers as
> well. ledtrig-timer will still have to store it as these are used in the
> led_timer_function() in led-class.c
>
>> - moving the two led_cdev->blink_delay_xx = 0 only into
>>   led_brightness_set, as that's the only place when they are needed.
>
> Sounds reasonable. I would like to pull your patches in and do some
> testing, but very likely I won't be able to get to it until this
> weekend. This is where use-cases will help as well for us go through
> various transitions and see if it all comes together.
>

Hi Fabio, thanks for updating this. I've added use the v3 replace the
old one in my for-next and devel branches

Shuah, Thanks for reviewing, if you wanna try this, just grab patches
from devel branch.

-Bryan



-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Re: [PATCH v3] led: add oneshot trigger

[Bryan Wu]

On Thu, Jun 7, 2012 at 6:11 AM, Fabio Baltieri <fabio.baltieri@gmail.com> wrote:
> Add oneshot trigger to blink a led with configurale parameters via
> sysfs.
>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> Cc: Shuah Khan <shuahkhan@gmail.com>
> Cc: Bryan Wu <bryan.wu@canonical.com>
> ---
>
> Here is the v3, with added documentation file with short description, sysfs
> description, use case example (the one I developed ths for, actually) and
> default values.
>
> Also, while trying the use case, I added instantaneous setting of led state when
> setting the "invert" attribute.
>
> Let me know what you think of this!
>

I'm fine this patch now, and added to my for-next and devel branches.

-Bryan


> Fabio
>
>  Documentation/leds/ledtrig-oneshot.txt |  59 ++++++++++
>  drivers/leds/Kconfig                   |  14 +++
>  drivers/leds/Makefile                  |   1 +
>  drivers/leds/ledtrig-oneshot.c         | 204 +++++++++++++++++++++++++++++++++
>  4 files changed, 278 insertions(+)
>  create mode 100644 Documentation/leds/ledtrig-oneshot.txt
>  create mode 100644 drivers/leds/ledtrig-oneshot.c
>
> diff --git a/Documentation/leds/ledtrig-oneshot.txt b/Documentation/leds/ledtrig-oneshot.txt
> new file mode 100644
> index 0000000..07cd1fa
> --- /dev/null
> +++ b/Documentation/leds/ledtrig-oneshot.txt
> @@ -0,0 +1,59 @@
> +One-shot LED Trigger
> +====================
> +
> +This is a LED trigger useful for signaling the user of an event where there are
> +no clear trap points to put standard led-on and led-off settings.  Using this
> +trigger, the application needs only to signal the trigger when an event has
> +happened, than the trigger turns the LED on and than keeps it off for a
> +specified amount of time.
> +
> +This trigger is meant to be usable both for sporadic and dense events.  In the
> +first case, the trigger produces a clear single controlled blink for each
> +event, while in the latter it keeps blinking at constant rate, as to signal
> +that the events are arriving continuously.
> +
> +A one-shot LED only stays in a constant state when there are no events.  An
> +additional "invert" property specifies if the LED has to stay off (normal) or
> +on (inverted) when not rearmed.
> +
> +The trigger can be activated from user space on led class devices as shown
> +below:
> +
> +  echo oneshot > trigger
> +
> +This adds the following sysfs attributes to the LED:
> +
> +  delay_on - specifies for how many milliseconds the LED has to stay at
> +             LED_FULL brightness after it has been armed.
> +             Default to 100 ms.
> +
> +  delay_off - specifies for how many milliseconds the LED has to stay at
> +              LED_OFF brightness after it has been armed.
> +              Default to 100 ms.
> +
> +  invert - reverse the blink logic.  If set to 0 (default) blink on for delay_on
> +           ms, then blink off for delay_off ms, leaving the LED normally off.  If
> +           set to 1, blink off for delay_off ms, then blink on for delay_on ms,
> +           leaving the LED normally on.
> +           Setting this value also immediately change the LED state.
> +
> +  shot - write any non-empty string to signal an events, this starts a blink
> +         sequence if not already running.
> +
> +Example use-case: network devices, initialization:
> +
> +  echo oneshot > trigger # set trigger for this led
> +  echo 33 > delay_on     # blink at 1 / (33 + 33) Hz on continuous traffic
> +  echo 33 > delay_off
> +
> +interface goes up:
> +
> +  echo 1 > invert # set led as normally-on, turn the led on
> +
> +packet received/transmitted:
> +
> +  echo 1 > shot # led starts blinking, ignored if already blinking
> +
> +interface goes down
> +
> +  echo 0 > invert # set led as normally-off, turn the led off
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 04cb8c8..1f151fb 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -443,6 +443,20 @@ config LEDS_TRIGGER_TIMER
>
>          If unsure, say Y.
>
> +config LEDS_TRIGGER_ONESHOT
> +       tristate "LED One-shot Trigger"
> +       depends on LEDS_TRIGGERS
> +       help
> +         This allows LEDs to blink in one-shot pulses with parameters
> +         controlled via sysfs.  It's useful to notify the user on
> +         sporadic events, when there are no clear begin and end trap points,
> +         or on dense events, where this blinks the LED at constant rate if
> +         rearmed continuously.
> +
> +         It also shows how to use the led_blink_set_oneshot() function.
> +
> +         If unsure, say Y.
> +
>  config LEDS_TRIGGER_IDE_DISK
>        bool "LED IDE Disk Trigger"
>        depends on IDE_GD_ATA
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index f8958cd..9112d51 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_DAC124S085)         += leds-dac124s085.o
>
>  # LED Triggers
>  obj-$(CONFIG_LEDS_TRIGGER_TIMER)       += ledtrig-timer.o
> +obj-$(CONFIG_LEDS_TRIGGER_ONESHOT)     += ledtrig-oneshot.o
>  obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)    += ledtrig-ide-disk.o
>  obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)   += ledtrig-heartbeat.o
>  obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)   += ledtrig-backlight.o
> diff --git a/drivers/leds/ledtrig-oneshot.c b/drivers/leds/ledtrig-oneshot.c
> new file mode 100644
> index 0000000..5c9edf7
> --- /dev/null
> +++ b/drivers/leds/ledtrig-oneshot.c
> @@ -0,0 +1,204 @@
> +/*
> + * One-shot LED Trigger
> + *
> + * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
> + *
> + * Based on ledtrig-timer.c by Richard Purdie <rpurdie@openedhand.com>
> + *
> + * 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/slab.h>
> +#include <linux/leds.h>
> +#include "leds.h"
> +
> +#define DEFAULT_DELAY 100
> +
> +struct oneshot_trig_data {
> +       unsigned int invert;
> +};
> +
> +static ssize_t led_shot(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t size)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +       struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
> +
> +       led_blink_set_oneshot(led_cdev,
> +                       &led_cdev->blink_delay_on, &led_cdev->blink_delay_off,
> +                       oneshot_data->invert);
> +
> +       /* content is ignored */
> +       return size;
> +}
> +static ssize_t led_invert_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +       struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
> +
> +       return sprintf(buf, "%u\n", oneshot_data->invert);
> +}
> +
> +static ssize_t led_invert_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t size)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +       struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
> +       unsigned long state;
> +       int ret;
> +
> +       ret = kstrtoul(buf, 0, &state);
> +       if (ret)
> +               return ret;
> +
> +       oneshot_data->invert = !!state;
> +
> +       if (oneshot_data->invert)
> +               led_set_brightness(led_cdev, LED_FULL);
> +       else
> +               led_set_brightness(led_cdev, LED_OFF);
> +
> +       return size;
> +}
> +
> +static ssize_t led_delay_on_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +       return sprintf(buf, "%lu\n", led_cdev->blink_delay_on);
> +}
> +
> +static ssize_t led_delay_on_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t size)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +       unsigned long state;
> +       int ret;
> +
> +       ret = kstrtoul(buf, 0, &state);
> +       if (ret)
> +               return ret;
> +
> +       led_cdev->blink_delay_on = state;
> +
> +       return size;
> +}
> +static ssize_t led_delay_off_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +       return sprintf(buf, "%lu\n", led_cdev->blink_delay_off);
> +}
> +
> +static ssize_t led_delay_off_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t size)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +       unsigned long state;
> +       int ret;
> +
> +       ret = kstrtoul(buf, 0, &state);
> +       if (ret)
> +               return ret;
> +
> +       led_cdev->blink_delay_off = state;
> +
> +       return size;
> +}
> +
> +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 DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);
> +static DEVICE_ATTR(shot, 0200, NULL, led_shot);
> +
> +static void oneshot_trig_activate(struct led_classdev *led_cdev)
> +{
> +       struct oneshot_trig_data *oneshot_data;
> +       int rc;
> +
> +       oneshot_data = kzalloc(sizeof(*oneshot_data), GFP_KERNEL);
> +       if (!oneshot_data)
> +               return;
> +
> +       led_cdev->trigger_data = oneshot_data;
> +
> +       rc = device_create_file(led_cdev->dev, &dev_attr_delay_on);
> +       if (rc)
> +               goto err_out_trig_data;
> +       rc = device_create_file(led_cdev->dev, &dev_attr_delay_off);
> +       if (rc)
> +               goto err_out_delayon;
> +       rc = device_create_file(led_cdev->dev, &dev_attr_invert);
> +       if (rc)
> +               goto err_out_delayoff;
> +       rc = device_create_file(led_cdev->dev, &dev_attr_shot);
> +       if (rc)
> +               goto err_out_invert;
> +
> +       led_cdev->blink_delay_on = DEFAULT_DELAY;
> +       led_cdev->blink_delay_off = DEFAULT_DELAY;
> +
> +       led_cdev->activated = true;
> +
> +       return;
> +
> +err_out_invert:
> +       device_remove_file(led_cdev->dev, &dev_attr_invert);
> +err_out_delayoff:
> +       device_remove_file(led_cdev->dev, &dev_attr_delay_off);
> +err_out_delayon:
> +       device_remove_file(led_cdev->dev, &dev_attr_delay_on);
> +err_out_trig_data:
> +       kfree(led_cdev->trigger_data);
> +}
> +
> +static void oneshot_trig_deactivate(struct led_classdev *led_cdev)
> +{
> +       struct oneshot_trig_data *oneshot_data = led_cdev->trigger_data;
> +
> +       if (led_cdev->activated) {
> +               device_remove_file(led_cdev->dev, &dev_attr_delay_on);
> +               device_remove_file(led_cdev->dev, &dev_attr_delay_off);
> +               device_remove_file(led_cdev->dev, &dev_attr_invert);
> +               device_remove_file(led_cdev->dev, &dev_attr_shot);
> +               kfree(oneshot_data);
> +               led_cdev->activated = false;
> +       }
> +
> +       /* Stop blinking */
> +       led_brightness_set(led_cdev, LED_OFF);
> +}
> +
> +static struct led_trigger oneshot_led_trigger = {
> +       .name     = "oneshot",
> +       .activate = oneshot_trig_activate,
> +       .deactivate = oneshot_trig_deactivate,
> +};
> +
> +static int __init oneshot_trig_init(void)
> +{
> +       return led_trigger_register(&oneshot_led_trigger);
> +}
> +
> +static void __exit oneshot_trig_exit(void)
> +{
> +       led_trigger_unregister(&oneshot_led_trigger);
> +}
> +
> +module_init(oneshot_trig_init);
> +module_exit(oneshot_trig_exit);
> +
> +MODULE_AUTHOR("Fabio Baltieri <fabio.baltieri@gmail.com>");
> +MODULE_DESCRIPTION("One-shot LED trigger");
> +MODULE_LICENSE("GPL");
> --
> 1.7.11.rc1.9.gf623ca1.dirty
>



-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Re: [PATCH v2] leds: fix led_brightness_set when soft-blinking

[Shuah Khan]

On Thu, 2012-06-07 at 20:58 +0800, Bryan Wu wrote:

> 
> Hi Fabio, thanks for updating this. I've added use the v3 replace the
> old one in my for-next and devel branches
> 
> Shuah, Thanks for reviewing, if you wanna try this, just grab patches
> from devel branch.
> 

Bryan,

Did some testing on your devel branch. Did back to back timer and
oneshot trigger activations and didn't see any problems. Looks good to
me.

-- Shuah

Re: [PATCH v2] leds: fix led_brightness_set when soft-blinking

[Bryan Wu]

On Mon, Jun 11, 2012 at 11:06 AM, Shuah Khan <shuahkhan@gmail.com> wrote:
> On Thu, 2012-06-07 at 20:58 +0800, Bryan Wu wrote:
>
>>
>> Hi Fabio, thanks for updating this. I've added use the v3 replace the
>> old one in my for-next and devel branches
>>
>> Shuah, Thanks for reviewing, if you wanna try this, just grab patches
>> from devel branch.
>>
>
> Bryan,
>
> Did some testing on your devel branch. Did back to back timer and
> oneshot trigger activations and didn't see any problems. Looks good to
> me.
>
Thanks a lot, Shuah,

-Bryan


-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com