[PATCH] Add LED pattern trigger

[PATCH] Add LED pattern trigger

[Joe Xue]

The LED pattern trigger allows LEDs blink in user defined pattern.

	new file:   Documentation/leds/ledtrig-pattern.txt
	modified:   drivers/leds/trigger/Kconfig
	modified:   drivers/leds/trigger/Makefile
	new file:   drivers/leds/trigger/ledtrig-pattern.c

Suggested-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Joe Xue <lgxue@hotmail.com>
---
 Documentation/leds/ledtrig-pattern.txt |  60 ++++++++++
 drivers/leds/trigger/Kconfig           |   9 ++
 drivers/leds/trigger/Makefile          |   1 +
 drivers/leds/trigger/ledtrig-pattern.c | 207 +++++++++++++++++++++++++++++++++
 4 files changed, 277 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-pattern.txt
 create mode 100644 drivers/leds/trigger/ledtrig-pattern.c

diff --git a/Documentation/leds/ledtrig-pattern.txt b/Documentation/leds/ledtrig-pattern.txt
new file mode 100644
index 0000000..4b546c3
--- /dev/null
+++ b/Documentation/leds/ledtrig-pattern.txt
@@ -0,0 +1,60 @@
+LED Pattern Trigger
+===================
+
+0. Introduction
+
+LED Pattern trigger is designed to let LEDs indicate the patterns defined by
+users. This is very useful for those scenarios where only one non-color led
+needs to indicate different states.
+
+1. How to use
+
+Pattern trigger can be enabled and disabled from user space on led class
+devices that support this trigger as shown below:
+
+     echo pattern > trigger
+
+When the pattern trigger is activated, it will get the current brightness as
+its brightness if the led is on. Otherwise, it will use the maximum brightness.
+
+If the led supports different values rather than ON/OFF, the brightness can be
+set in advance before enabling the trigger.
+
+     echo 128 > brightness
+     echo pattern > trigger
+
+Two properties are exported. They are delay_unit and pattern.
+
+     delay_unit - a delay time unit, default value is 125 ms
+     pattern    - blink pattern, includes three legal characters
+                  '#', ' '(space), and '/'
+                  '#' let LED on and last delay_unit long time
+                  ' ' let LED off and last delay_unit long time
+                  '/' stop pattern
+                      pattern will be repeated without it
+
+3. Examples
+
+     Example 1
+
+     echo pattern > trigger
+     echo "# ## /"
+
+     The behaviour is like below:
+
+     on(125ms)off(125ms)on(250ms)off
+     This is Morse code 'A'
+
+     Example 2
+
+     echo pattern > trigger
+     echo 200 > delay_unit
+     echo "# #    "
+
+     The behaviour is like below:
+
+     on(200ms)off(200ms)on(200ms)off(800ms)
+     on(200ms)off(200ms)on(200ms)off(800ms)
+     ...(Repeat)
+
+     This is 2 times burst blinking.
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 49794b4..23d0967 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -108,4 +108,13 @@ config LEDS_TRIGGER_CAMERA
 	  This enables direct flash/torch on/off by the driver, kernel space.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_PATTERN
+	tristate "LED Pattern Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  This allows LEDs to blink in a user defined pattern controlled via
+          sysfs. It's useful to notify different states by using one led.
+	  For more details read Documentation/leds/leds-pattern.txt.
+	  If unsure, say Y.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 1abf48d..a739429 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_LEDS_TRIGGER_CPU)		+= ledtrig-cpu.o
 obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
 obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
+obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
new file mode 100644
index 0000000..80fc238
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-pattern.c
@@ -0,0 +1,207 @@
+/*
+ * LED Kernel Morse Code Trigger
+ *
+ * Copyright (C) 2013 Joe Xue <lgxue@hotmail.com>
+ *
+ * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
+ * ledtrig-heartbeat.c and Shuah Khan's ledtrig-transient.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/slab.h>
+#include <linux/timer.h>
+#include <linux/leds.h>
+#include "../leds.h"
+
+#define MAX_PATTEN_LEN	255
+
+struct pattern_trig_data {
+	unsigned long delay_unit;
+	unsigned long pattern_len;
+	unsigned long count;
+	int brightness_on;
+	char pattern[MAX_PATTEN_LEN + 1];
+	struct timer_list timer;
+};
+
+static void pattern_timer_function(unsigned long data)
+{
+	struct led_classdev *led_cdev = (struct led_classdev *) data;
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	if (pattern_data->pattern[pattern_data->count] == '#') {
+		__led_set_brightness(led_cdev, pattern_data->brightness_on);
+		mod_timer(&pattern_data->timer,
+			jiffies + msecs_to_jiffies(pattern_data->delay_unit));
+	} else if (pattern_data->pattern[pattern_data->count] == ' ') {
+		__led_set_brightness(led_cdev, LED_OFF);
+		mod_timer(&pattern_data->timer,
+			jiffies + msecs_to_jiffies(pattern_data->delay_unit));
+	/* stop blinking */
+	} else if (pattern_data->pattern[pattern_data->count] == '/') {
+		return;
+	}
+
+	pattern_data->count++;
+	if (pattern_data->count == pattern_data->pattern_len)
+		pattern_data->count = 0;
+}
+
+static ssize_t pattern_delay_unit_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	return sprintf(buf, "%lu\n", pattern_data->delay_unit);
+}
+
+static ssize_t pattern_delay_unit_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+	unsigned long state;
+	ssize_t ret = -EINVAL;
+
+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	pattern_data->delay_unit = state;
+
+	return size;
+}
+
+static ssize_t pattern_pattern_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	return sprintf(buf, "%s\n", pattern_data->pattern);
+}
+
+static ssize_t pattern_pattern_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+	int i;
+	ssize_t ret = -EINVAL;
+
+	int len = (size > MAX_PATTEN_LEN) ? MAX_PATTEN_LEN : (size - 1);
+
+	/* legality check */
+	for (i = 0; i < len; i++) {
+		if (buf[i] != ' ' && buf[i] != '#' && buf[i] != '/')
+			return ret;
+	}
+
+	del_timer_sync(&pattern_data->timer);
+
+	memcpy(pattern_data->pattern, buf, len);
+	pattern_data->pattern[len] = '\0';
+	pattern_data->pattern_len = len;
+	pattern_data->count = 0;
+
+	mod_timer(&pattern_data->timer, jiffies + 1);
+
+	return size;
+}
+
+static DEVICE_ATTR(pattern, 0644, pattern_pattern_show, pattern_pattern_store);
+static DEVICE_ATTR(delay_unit, 0644,
+		pattern_delay_unit_show, pattern_delay_unit_store);
+
+static void pattern_trig_activate(struct led_classdev *led_cdev)
+{
+	int rc;
+	struct pattern_trig_data *tdata;
+
+	tdata = kzalloc(sizeof(struct pattern_trig_data), GFP_KERNEL);
+	if (!tdata) {
+		dev_err(led_cdev->dev,
+			"unable to allocate pattern trigger\n");
+		return;
+	}
+
+	led_cdev->trigger_data = tdata;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_pattern);
+	if (rc)
+		goto err_out;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_delay_unit);
+	if (rc)
+		goto err_out_delay_unit;
+
+	memset(tdata->pattern, 0, MAX_PATTEN_LEN + 1);
+	/* default delay_unit 125ms */
+	tdata->delay_unit = 125;
+
+	setup_timer(&tdata->timer, pattern_timer_function,
+		    (unsigned long) led_cdev);
+
+	tdata->brightness_on = led_get_brightness(led_cdev);
+	if (tdata->brightness_on == LED_OFF)
+		tdata->brightness_on = led_cdev->max_brightness;
+
+	__led_set_brightness(led_cdev, LED_OFF);
+	led_cdev->activated = true;
+
+	return;
+
+err_out_delay_unit:
+	device_remove_file(led_cdev->dev, &dev_attr_pattern);
+err_out:
+	dev_err(led_cdev->dev, "unable to register pattern trigger\n");
+	led_cdev->trigger_data = NULL;
+	kfree(tdata);
+}
+
+static void pattern_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	if (led_cdev->activated) {
+		del_timer_sync(&pattern_data->timer);
+		device_remove_file(led_cdev->dev, &dev_attr_pattern);
+		device_remove_file(led_cdev->dev, &dev_attr_delay_unit);
+		led_cdev->trigger_data = NULL;
+		led_cdev->activated = false;
+		kfree(pattern_data);
+	}
+	__led_set_brightness(led_cdev, LED_OFF);
+}
+
+static struct led_trigger pattern_trigger = {
+	.name     = "pattern",
+	.activate = pattern_trig_activate,
+	.deactivate = pattern_trig_deactivate,
+};
+
+static int __init pattern_trig_init(void)
+{
+	return led_trigger_register(&pattern_trigger);
+}
+
+static void __exit pattern_trig_exit(void)
+{
+	led_trigger_unregister(&pattern_trigger);
+}
+
+module_init(pattern_trig_init);
+module_exit(pattern_trig_exit);
+
+MODULE_AUTHOR("Joe Xue <lgxue@hotmail.com");
+MODULE_DESCRIPTION("Patten LED trigger");
+MODULE_LICENSE("GPL");
-- 
1.8.1.2

Re: [PATCH] Add LED pattern trigger

[One Thousand Gnomes]

> + * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
> + * ledtrig-heartbeat.c and Shuah Khan's ledtrig-transient.c

I stil think this belongs in user space except for platforms with hardware
acceleration for it.

> +#define MAX_PATTEN_LEN	255

Arbitary limits that are not needed if it was in userspace, and not it
seems a sensible one - why not use 256 ?

> +static ssize_t pattern_delay_unit_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> +	unsigned long state;
> +	ssize_t ret = -EINVAL;
> +
> +	ret = kstrtoul(buf, 10, &state);
> +	if (ret)
> +		return ret;
> +
> +	pattern_data->delay_unit = state;

What happens if this is zero ?

> +static ssize_t pattern_pattern_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> +	int i;
> +	ssize_t ret = -EINVAL;
> +
> +	int len = (size > MAX_PATTEN_LEN) ? MAX_PATTEN_LEN : (size - 1);
> +
> +	/* legality check */
> +	for (i = 0; i < len; i++) {
> +		if (buf[i] != ' ' && buf[i] != '#' && buf[i] != '/')
> +			return ret;
> +	}
> +
> +	del_timer_sync(&pattern_data->timer);
> +
> +	memcpy(pattern_data->pattern, buf, len);
> +	pattern_data->pattern[len] = '\0';
> +	pattern_data->pattern_len = len;
> +	pattern_data->count = 0;
> +
> +	mod_timer(&pattern_data->timer, jiffies + 1);

What if the pattern isn't currently active ?

> +	return size;

You only consumed len bytes so you should return len here.

> +}
> +
> +static DEVICE_ATTR(pattern, 0644, pattern_pattern_show, pattern_pattern_store);
> +static DEVICE_ATTR(delay_unit, 0644,
> +		pattern_delay_unit_show, pattern_delay_unit_store);

Why are these world readable. If patterns tell you an action is due they
provide information that other processes shouldn't have access to.

> +	memset(tdata->pattern, 0, MAX_PATTEN_LEN + 1);

Why +1, you don't need a zero terminator you know the length

Why allocate a fixed 256 byte blob when you can make the data the end of
the struct (ie pattern[0] in the declaration) and not waste memory.

> +static void pattern_trig_deactivate(struct led_classdev *led_cdev)
> +{
> +	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> +
> +	if (led_cdev->activated) {
> +		del_timer_sync(&pattern_data->timer);
> +		device_remove_file(led_cdev->dev, &dev_attr_pattern);
> +		device_remove_file(led_cdev->dev, &dev_attr_delay_unit);

This doesn't as far as I can see do what you think. If I have the file
currently open then device_remove_file will not remove my existing access
to it, but you just released the pattern data so I now write to free
memory.

> +		led_cdev->trigger_data = NULL;
> +		led_cdev->activated = false;
> +		kfree(pattern_data);
> +	}
> +	__led_set_brightness(led_cdev, LED_OFF);
> +}
> +
> +static struct led_trigger pattern_trigger = {

const ?

Re: [PATCH] Add LED pattern trigger

[Pavel Machek]

Hi!

> The LED pattern trigger allows LEDs blink in user defined pattern.
> 
> 	new file:   Documentation/leds/ledtrig-pattern.txt
> 	modified:   drivers/leds/trigger/Kconfig
> 	modified:   drivers/leds/trigger/Makefile
> 	new file:   drivers/leds/trigger/ledtrig-pattern.c
> 
> Suggested-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Joe Xue <lgxue@hotmail.com>

> +     echo pattern > trigger
> +     echo "# ## /"
> +
> +     The behaviour is like below:
> +
> +     on(125ms)off(125ms)on(250ms)off


> +static void pattern_timer_function(unsigned long data)
> +{
> +	struct led_classdev *led_cdev = (struct led_classdev *) data;
> +	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> +
> +	if (pattern_data->pattern[pattern_data->count] == '#') {
> +		__led_set_brightness(led_cdev, pattern_data->brightness_on);
> +		mod_timer(&pattern_data->timer,
> +			jiffies + msecs_to_jiffies(pattern_data->delay_unit));
> +	} else if (pattern_data->pattern[pattern_data->count] == ' ') {
> +		__led_set_brightness(led_cdev, LED_OFF);
> +		mod_timer(&pattern_data->timer,
> +			jiffies + msecs_to_jiffies(pattern_data->delay_unit));
> +	/* stop blinking */
> +	} else if (pattern_data->pattern[pattern_data->count] == '/') {
> +		return;
> +	}

What about something like this?

Not shcheduling timer when nothing changed should save a bit of power/cpu...

  	if (pattern_data->pattern[pattern_data->count] == '/') {
		return;
	}

	this = pattern_data->pattern[pattern_data->count]
	if (this == '#')
		new_brigtness = pattern_data->brightness_on;
	if (this == ' ')
		new_brigtness = LED_OFF;
	repeat = 1;
	while (pattern_data->pattern[pattern_data->count + repeat] == this)
	      repeat++;

	mod_timer(&pattern_data->timer,
		jiffies + msecs_to_jiffies(pattern_data->delay_unit * repeat));


Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [PATCH] Add LED pattern trigger

[Joe Xue]

>> +#define MAX_PATTEN_LEN 255
>
> Arbitary limits that are not needed if it was in userspace, and not it
> seems a sensible one - why not use 256 ?

The maximum memory is 256, we keep one for '\0'

>> +static ssize_t pattern_delay_unit_store(struct device *dev,
>> + struct device_attribute *attr, const char *buf, size_t size)
>> +{
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
>> + unsigned long state;
>> + ssize_t ret = -EINVAL;
>> +
>> + ret = kstrtoul(buf, 10, &state);
>> + if (ret)
>> + return ret;
>> +
>> + pattern_data->delay_unit = state;
>
> What happens if this is zero ?

Yes, we should not accept 0 here. Will fix it.

>> +static ssize_t pattern_pattern_store(struct device *dev,
>> + struct device_attribute *attr, const char *buf, size_t size)
>> +{
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
>> + int i;
>> + ssize_t ret = -EINVAL;
>> +
>> + int len = (size> MAX_PATTEN_LEN) ? MAX_PATTEN_LEN : (size - 1);
>> +
>> + /* legality check */
>> + for (i = 0; i < len; i++) {
>> + if (buf[i] != ' ' && buf[i] != '#' && buf[i] != '/')
>> + return ret;
>> + }
>> +
>> + del_timer_sync(&pattern_data->timer);
>> +
>> + memcpy(pattern_data->pattern, buf, len);
>> + pattern_data->pattern[len] = '\0';
>> + pattern_data->pattern_len = len;
>> + pattern_data->count = 0;
>> +
>> + mod_timer(&pattern_data->timer, jiffies + 1);
>
> What if the pattern isn't currently active ?

Doesn't matter as per my test.

>> + return size;
>
> You only consumed len bytes so you should return len here.
>
>> +}
>> +
>> +static DEVICE_ATTR(pattern, 0644, pattern_pattern_show, pattern_pattern_store);
>> +static DEVICE_ATTR(delay_unit, 0644,
>> + pattern_delay_unit_show, pattern_delay_unit_store);
>
> Why are these world readable. If patterns tell you an action is due they
> provide information that other processes shouldn't have access to.
>
>> + memset(tdata->pattern, 0, MAX_PATTEN_LEN + 1);
>
> Why +1, you don't need a zero terminator you know the length
>
> Why allocate a fixed 256 byte blob when you can make the data the end of
> the struct (ie pattern[0] in the declaration) and not waste memory.

This just easy for patten_show.

>> +static void pattern_trig_deactivate(struct led_classdev *led_cdev)
>> +{
>> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
>> +
>> + if (led_cdev->activated) {
>> + del_timer_sync(&pattern_data->timer);
>> + device_remove_file(led_cdev->dev, &dev_attr_pattern);
>> + device_remove_file(led_cdev->dev, &dev_attr_delay_unit);
>
> This doesn't as far as I can see do what you think. If I have the file
> currently open then device_remove_file will not remove my existing access
> to it, but you just released the pattern data so I now write to free
> memory.

I believe kernel will handle this

>> + led_cdev->trigger_data = NULL;
>> + led_cdev->activated = false;
>> + kfree(pattern_data);
>> + }
>> + __led_set_brightness(led_cdev, LED_OFF);
>> +}
>> +
>> +static struct led_trigger pattern_trigger = {
>
> const ?

? 		 	   		  

Re: [PATCH] Add LED pattern trigger

[One Thousand Gnomes]

On Mon, 30 Dec 2013 18:24:51 -0500
Joe Xue <lgxue@hotmail.com> wrote:

> >> +#define MAX_PATTEN_LEN 255
> >
> > Arbitary limits that are not needed if it was in userspace, and not it
> > seems a sensible one - why not use 256 ?
> 
> The maximum memory is 256, we keep one for '\0'

Why - you have pattern_len already ?

> >> + del_timer_sync(&pattern_data->timer);
> >> +
> >> + memcpy(pattern_data->pattern, buf, len);
> >> + pattern_data->pattern[len] = '\0';
> >> + pattern_data->pattern_len = len;
> >> + pattern_data->count = 0;
> >> +
> >> + mod_timer(&pattern_data->timer, jiffies + 1);
> >
> > What if the pattern isn't currently active ?
> 
> Doesn't matter as per my test.

What test - you've not explained why it doesn't blow up ?

> > Why +1, you don't need a zero terminator you know the length
> >
> > Why allocate a fixed 256 byte blob when you can make the data the end of
> > the struct (ie pattern[0] in the declaration) and not waste memory.
> 
> This just easy for patten_show.

Even so the fixed 256 is broken - you can just allocate the proper size
for the pattern when you set it up.
 
> >> +static void pattern_trig_deactivate(struct led_classdev *led_cdev)
> >> +{
> >> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> >> +
> >> + if (led_cdev->activated) {
> >> + del_timer_sync(&pattern_data->timer);
> >> + device_remove_file(led_cdev->dev, &dev_attr_pattern);
> >> + device_remove_file(led_cdev->dev, &dev_attr_delay_unit);
> >
> > This doesn't as far as I can see do what you think. If I have the file
> > currently open then device_remove_file will not remove my existing access
> > to it, but you just released the pattern data so I now write to free
> > memory.
> 
> I believe kernel will handle this

I looked through the code paths and I see nothing to protect this at all ?

> >> + led_cdev->trigger_data = NULL;
> >> + led_cdev->activated = false;
> >> + kfree(pattern_data);
> >> + }
> >> + __led_set_brightness(led_cdev, LED_OFF);
> >> +}
> >> +
> >> +static struct led_trigger pattern_trigger = {
> >
> > const ?
> 

You don't want function pointers in objects that are not const, at least
whenever possible. On processors that have suitable protection modes that
helps prevent attacks based upon patching kernel function pointers.


Alan

RE: [PATCH] Add LED pattern trigger

[Joe Xue]

> What about something like this?
>
> Not shcheduling timer when nothing changed should save a bit of power/cpu...
>
> if (pattern_data->pattern[pattern_data->count] == '/') {
> return;
> }
>
> this = pattern_data->pattern[pattern_data->count]
> if (this == '#')
> new_brigtness = pattern_data->brightness_on;
> if (this == ' ')
> new_brigtness = LED_OFF;
> repeat = 1;
> while (pattern_data->pattern[pattern_data->count + repeat] == this)
> repeat++;
>
> mod_timer(&pattern_data->timer,
> jiffies + msecs_to_jiffies(pattern_data->delay_unit * repeat));
>
>
Working on it.

Thanks for your help.

Joe 		 	   		  

Re: [PATCH] Add LED pattern trigger

[Pavel Machek]

On Tue 2013-12-31 00:00:39, Joe Xue wrote:
> > What about something like this?
> >
> > Not shcheduling timer when nothing changed should save a bit of power/cpu...
> >
> > if (pattern_data->pattern[pattern_data->count] == '/') {
> > return;
> > }
> >
> > this = pattern_data->pattern[pattern_data->count]
> > if (this == '#')
> > new_brigtness = pattern_data->brightness_on;
> > if (this == ' ')
> > new_brigtness = LED_OFF;
> > repeat = 1;
> > while (pattern_data->pattern[pattern_data->count + repeat] == this)
> > repeat++;
> >
> > mod_timer(&pattern_data->timer,
> > jiffies + msecs_to_jiffies(pattern_data->delay_unit * repeat));
> >
> >
> Working on it.
> 
> Thanks for your help.

You are welcome :-).
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] Add LED pattern trigger

[Richard Weinberger]

On Tue, Dec 31, 2013 at 12:33 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Tue 2013-12-31 00:00:39, Joe Xue wrote:
>> > What about something like this?
>> >
>> > Not shcheduling timer when nothing changed should save a bit of power/cpu...
>> >
>> > if (pattern_data->pattern[pattern_data->count] == '/') {
>> > return;
>> > }
>> >
>> > this = pattern_data->pattern[pattern_data->count]
>> > if (this == '#')
>> > new_brigtness = pattern_data->brightness_on;
>> > if (this == ' ')
>> > new_brigtness = LED_OFF;
>> > repeat = 1;
>> > while (pattern_data->pattern[pattern_data->count + repeat] == this)
>> > repeat++;
>> >
>> > mod_timer(&pattern_data->timer,
>> > jiffies + msecs_to_jiffies(pattern_data->delay_unit * repeat));
>> >
>> >
>> Working on it.
>>
>> Thanks for your help.
>
> You are welcome :-).

Why do we need this within the kernel?
Patterns can easily created using a simple user space program.

-- 
Thanks,
//richard

RE: [PATCH] Add LED pattern trigger

[Joe Xue]

>> + * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
>> + * ledtrig-heartbeat.c and Shuah Khan's ledtrig-transient.c
>
> I stil think this belongs in user space except for platforms with hardware
> acceleration for it.

This can free the user space application from loop or thread.

>> +#define MAX_PATTEN_LEN 255
>
> Arbitary limits that are not needed if it was in userspace, and not it
> seems a sensible one - why not use 256 ?

Yes, will change it to kmalloc


>> +static DEVICE_ATTR(pattern, 0644, pattern_pattern_show, pattern_pattern_store);
>> +static DEVICE_ATTR(delay_unit, 0644,
>> +static void pattern_trig_deactivate(struct led_classdev *led_cdev)
>> +{
>> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
>> +
>> + if (led_cdev->activated) {
>> + del_timer_sync(&pattern_data->timer);
>> + device_remove_file(led_cdev->dev, &dev_attr_pattern);
>> + device_remove_file(led_cdev->dev, &dev_attr_delay_unit);
>
> This doesn't as far as I can see do what you think. If I have the file
> currently open then device_remove_file will not remove my existing access
> to it, but you just released the pattern data so I now write to free
> memory.

will add the mutex lock to avoid that 		 	   		  

Re: [PATCH] Add LED pattern trigger

[Pavel Machek]

On Tue 2013-12-31 13:29:21, Richard Weinberger wrote:
> On Tue, Dec 31, 2013 at 12:33 PM, Pavel Machek <pavel@ucw.cz> wrote:
> > On Tue 2013-12-31 00:00:39, Joe Xue wrote:
> >> > What about something like this?
> >> >
> >> > Not shcheduling timer when nothing changed should save a bit of power/cpu...
> >> >
> >> > if (pattern_data->pattern[pattern_data->count] == '/') {
> >> > return;
> >> > }
> >> >
> >> > this = pattern_data->pattern[pattern_data->count]
> >> > if (this == '#')
> >> > new_brigtness = pattern_data->brightness_on;
> >> > if (this == ' ')
> >> > new_brigtness = LED_OFF;
> >> > repeat = 1;
> >> > while (pattern_data->pattern[pattern_data->count + repeat] == this)
> >> > repeat++;
> >> >
> >> > mod_timer(&pattern_data->timer,
> >> > jiffies + msecs_to_jiffies(pattern_data->delay_unit * repeat));
> >> >
> >> >
> >> Working on it.
> >>
> >> Thanks for your help.
> >
> > You are welcome :-).
> 
> Why do we need this within the kernel?
> Patterns can easily created using a simple user space program.

Some machines (N900) can do blinking in hardware, and we want
consistent kernel-user interface. See the mailing list.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH v2 1/1] leds: Add LED pattern trigger

[Joe Xue]

From: Joe Xue <lgxue@hotmail.com>

The LED pattern trigger allows LEDs blink in user defined pattern.

v2: Change the pattern memory from fixed static to malloc
    Change the timer schedule way to save cpu time
    Add the mutex to protect the pattern operation

	new file:   Documentation/leds/ledtrig-pattern.txt
	modified:   drivers/leds/trigger/Kconfig
	modified:   drivers/leds/trigger/Makefile
	new file:   drivers/leds/trigger/ledtrig-pattern.c

Suggested-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Joe Xue <lgxue@hotmail.com>
---
 Documentation/leds/ledtrig-pattern.txt |  60 +++++++++
 drivers/leds/trigger/Kconfig           |   9 ++
 drivers/leds/trigger/Makefile          |   1 +
 drivers/leds/trigger/ledtrig-pattern.c | 233 +++++++++++++++++++++++++++++++++
 4 files changed, 303 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-pattern.txt
 create mode 100644 drivers/leds/trigger/ledtrig-pattern.c

diff --git a/Documentation/leds/ledtrig-pattern.txt b/Documentation/leds/ledtrig-pattern.txt
new file mode 100644
index 0000000..4b546c3
--- /dev/null
+++ b/Documentation/leds/ledtrig-pattern.txt
@@ -0,0 +1,60 @@
+LED Pattern Trigger
+===================
+
+0. Introduction
+
+LED Pattern trigger is designed to let LEDs indicate the patterns defined by
+users. This is very useful for those scenarios where only one non-color led
+needs to indicate different states.
+
+1. How to use
+
+Pattern trigger can be enabled and disabled from user space on led class
+devices that support this trigger as shown below:
+
+     echo pattern > trigger
+
+When the pattern trigger is activated, it will get the current brightness as
+its brightness if the led is on. Otherwise, it will use the maximum brightness.
+
+If the led supports different values rather than ON/OFF, the brightness can be
+set in advance before enabling the trigger.
+
+     echo 128 > brightness
+     echo pattern > trigger
+
+Two properties are exported. They are delay_unit and pattern.
+
+     delay_unit - a delay time unit, default value is 125 ms
+     pattern    - blink pattern, includes three legal characters
+                  '#', ' '(space), and '/'
+                  '#' let LED on and last delay_unit long time
+                  ' ' let LED off and last delay_unit long time
+                  '/' stop pattern
+                      pattern will be repeated without it
+
+3. Examples
+
+     Example 1
+
+     echo pattern > trigger
+     echo "# ## /"
+
+     The behaviour is like below:
+
+     on(125ms)off(125ms)on(250ms)off
+     This is Morse code 'A'
+
+     Example 2
+
+     echo pattern > trigger
+     echo 200 > delay_unit
+     echo "# #    "
+
+     The behaviour is like below:
+
+     on(200ms)off(200ms)on(200ms)off(800ms)
+     on(200ms)off(200ms)on(200ms)off(800ms)
+     ...(Repeat)
+
+     This is 2 times burst blinking.
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 49794b4..23d0967 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -108,4 +108,13 @@ config LEDS_TRIGGER_CAMERA
 	  This enables direct flash/torch on/off by the driver, kernel space.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_PATTERN
+	tristate "LED Pattern Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  This allows LEDs to blink in a user defined pattern controlled via
+          sysfs. It's useful to notify different states by using one led.
+	  For more details read Documentation/leds/leds-pattern.txt.
+	  If unsure, say Y.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 1abf48d..a739429 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_LEDS_TRIGGER_CPU)		+= ledtrig-cpu.o
 obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
 obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
+obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
new file mode 100644
index 0000000..9c688e7
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-pattern.c
@@ -0,0 +1,233 @@
+/*
+ * LED Kernel Morse Code Trigger
+ *
+ * Copyright (C) 2013 Joe Xue <lgxue@hotmail.com>
+ *
+ * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
+ * ledtrig-heartbeat.c and Shuah Khan's ledtrig-transient.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/slab.h>
+#include <linux/timer.h>
+#include <linux/leds.h>
+#include "../leds.h"
+
+struct pattern_trig_data {
+	unsigned long delay_unit;
+	unsigned long pattern_len;
+	unsigned long count;
+	int brightness_on;
+	char *pattern;
+	struct timer_list timer;
+	struct mutex pattern_mutex;
+};
+
+static void pattern_timer_function(unsigned long data)
+{
+	int repeat = 1;
+	struct led_classdev *led_cdev = (struct led_classdev *) data;
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+	char this = pattern_data->pattern[pattern_data->count];
+
+	if (this == '#') {
+		__led_set_brightness(led_cdev, pattern_data->brightness_on);
+	} else if (this == ' ') {
+		__led_set_brightness(led_cdev, LED_OFF);
+	/* stop blinking */
+	} else if (this == '/') {
+		return;
+	}
+
+	while (pattern_data->pattern[pattern_data->count + repeat] == this)
+		repeat++;
+
+	mod_timer(&pattern_data->timer,
+		jiffies + msecs_to_jiffies(pattern_data->delay_unit * repeat));
+
+	pattern_data->count += repeat;
+	if (pattern_data->count == pattern_data->pattern_len)
+		pattern_data->count = 0;
+}
+
+static ssize_t pattern_delay_unit_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	return sprintf(buf, "%lu\n", pattern_data->delay_unit);
+}
+
+static ssize_t pattern_delay_unit_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+	unsigned long state;
+	ssize_t ret = -EINVAL;
+
+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	/* this value can't be 0 */
+	if (state == 0)
+		return -EINVAL;
+
+	pattern_data->delay_unit = state;
+
+	return size;
+}
+
+static ssize_t pattern_pattern_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	if (pattern_data->pattern)
+		return scnprintf(buf, PAGE_SIZE, "%s\n",
+				pattern_data->pattern);
+	return 0;
+}
+
+static ssize_t pattern_pattern_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+	int i, len;
+	ssize_t ret = -EINVAL;
+
+	len = 0;
+
+	/* legality check */
+	for (i = 0; i < size; i++) {
+		if (buf[i] != ' ' && buf[i] != '#' && buf[i] != '/')
+			break;
+		len++;
+	}
+
+	if (len == 0)
+		return ret;
+
+	mutex_lock(&pattern_data->pattern_mutex);
+	kfree(pattern_data->pattern);
+	pattern_data->pattern = NULL;
+
+	pattern_data->pattern = kzalloc(len + 1, GFP_KERNEL);
+	if (!pattern_data->pattern) {
+		len = -ENOMEM;
+		goto unlock;
+	}
+	scnprintf(pattern_data->pattern, len + 1, "%s", buf);
+
+	del_timer_sync(&pattern_data->timer);
+
+	pattern_data->pattern_len = len;
+	pattern_data->count = 0;
+
+	mod_timer(&pattern_data->timer, jiffies + 1);
+unlock:
+	mutex_unlock(&pattern_data->pattern_mutex);
+	return size;
+}
+
+static DEVICE_ATTR(pattern, 0644, pattern_pattern_show, pattern_pattern_store);
+static DEVICE_ATTR(delay_unit, 0644,
+		pattern_delay_unit_show, pattern_delay_unit_store);
+
+static void pattern_trig_activate(struct led_classdev *led_cdev)
+{
+	int rc;
+	struct pattern_trig_data *tdata;
+
+	tdata = kzalloc(sizeof(struct pattern_trig_data), GFP_KERNEL);
+	if (!tdata) {
+		dev_err(led_cdev->dev,
+			"unable to allocate pattern trigger\n");
+		return;
+	}
+
+	led_cdev->trigger_data = tdata;
+
+	mutex_init(&tdata->pattern_mutex);
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_pattern);
+	if (rc)
+		goto err_out;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_delay_unit);
+	if (rc)
+		goto err_out_delay_unit;
+
+	/* default delay_unit 125ms */
+	tdata->delay_unit = 125;
+
+	setup_timer(&tdata->timer, pattern_timer_function,
+		    (unsigned long) led_cdev);
+
+	tdata->brightness_on = led_get_brightness(led_cdev);
+	if (tdata->brightness_on == LED_OFF)
+		tdata->brightness_on = led_cdev->max_brightness;
+
+	__led_set_brightness(led_cdev, LED_OFF);
+	led_cdev->activated = true;
+
+	return;
+
+err_out_delay_unit:
+	device_remove_file(led_cdev->dev, &dev_attr_pattern);
+err_out:
+	dev_err(led_cdev->dev, "unable to register pattern trigger\n");
+	led_cdev->trigger_data = NULL;
+	kfree(tdata);
+}
+
+static void pattern_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	if (led_cdev->activated) {
+		del_timer_sync(&pattern_data->timer);
+		device_remove_file(led_cdev->dev, &dev_attr_pattern);
+		device_remove_file(led_cdev->dev, &dev_attr_delay_unit);
+		led_cdev->trigger_data = NULL;
+		led_cdev->activated = false;
+		kfree(pattern_data->pattern);
+		kfree(pattern_data);
+	}
+	__led_set_brightness(led_cdev, LED_OFF);
+}
+
+static struct led_trigger pattern_trigger = {
+	.name     = "pattern",
+	.activate = pattern_trig_activate,
+	.deactivate = pattern_trig_deactivate,
+};
+
+static int __init pattern_trig_init(void)
+{
+	return led_trigger_register(&pattern_trigger);
+}
+
+static void __exit pattern_trig_exit(void)
+{
+	led_trigger_unregister(&pattern_trigger);
+}
+
+module_init(pattern_trig_init);
+module_exit(pattern_trig_exit);
+
+MODULE_AUTHOR("Joe Xue <lgxue@hotmail.com");
+MODULE_DESCRIPTION("Pattern LED trigger");
+MODULE_LICENSE("GPL");
-- 
1.8.3.2

[PATCH v2 1/1] Add LED pattern trigger

[Joe Xue]

I set the in-reply-to to try to put my new patch here, but failed. 
I need to figure it out.

From: Joe Xue <lgxue@hotmail.com>

The LED pattern trigger allows LEDs blink in user defined pattern.

v2: Change the pattern memory from fixed static to malloc
    Change the timer schedule way to save cpu time
    Add the mutex to protect the pattern operation

	new file:   Documentation/leds/ledtrig-pattern.txt
	modified:   drivers/leds/trigger/Kconfig
	modified:   drivers/leds/trigger/Makefile
	new file:   drivers/leds/trigger/ledtrig-pattern.c

Suggested-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Joe Xue <lgxue@hotmail.com>
---
 Documentation/leds/ledtrig-pattern.txt |  60 +++++++++
 drivers/leds/trigger/Kconfig           |   9 ++
 drivers/leds/trigger/Makefile          |   1 +
 drivers/leds/trigger/ledtrig-pattern.c | 233 +++++++++++++++++++++++++++++++++
 4 files changed, 303 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-pattern.txt
 create mode 100644 drivers/leds/trigger/ledtrig-pattern.c

diff --git a/Documentation/leds/ledtrig-pattern.txt b/Documentation/leds/ledtrig-pattern.txt
new file mode 100644
index 0000000..4b546c3
--- /dev/null
+++ b/Documentation/leds/ledtrig-pattern.txt
@@ -0,0 +1,60 @@
+LED Pattern Trigger
+===================
+
+0. Introduction
+
+LED Pattern trigger is designed to let LEDs indicate the patterns defined by
+users. This is very useful for those scenarios where only one non-color led
+needs to indicate different states.
+
+1. How to use
+
+Pattern trigger can be enabled and disabled from user space on led class
+devices that support this trigger as shown below:
+
+     echo pattern> trigger
+
+When the pattern trigger is activated, it will get the current brightness as
+its brightness if the led is on. Otherwise, it will use the maximum brightness.
+
+If the led supports different values rather than ON/OFF, the brightness can be
+set in advance before enabling the trigger.
+
+     echo 128> brightness
+     echo pattern> trigger
+
+Two properties are exported. They are delay_unit and pattern.
+
+     delay_unit - a delay time unit, default value is 125 ms
+     pattern    - blink pattern, includes three legal characters
+                  '#', ' '(space), and '/'
+                  '#' let LED on and last delay_unit long time
+                  ' ' let LED off and last delay_unit long time
+                  '/' stop pattern
+                      pattern will be repeated without it
+
+3. Examples
+
+     Example 1
+
+     echo pattern> trigger
+     echo "# ## /"
+
+     The behaviour is like below:
+
+     on(125ms)off(125ms)on(250ms)off
+     This is Morse code 'A'
+
+     Example 2
+
+     echo pattern> trigger
+     echo 200> delay_unit
+     echo "# #    "
+
+     The behaviour is like below:
+
+     on(200ms)off(200ms)on(200ms)off(800ms)
+     on(200ms)off(200ms)on(200ms)off(800ms)
+     ...(Repeat)
+
+     This is 2 times burst blinking.
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 49794b4..23d0967 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -108,4 +108,13 @@ config LEDS_TRIGGER_CAMERA
 	  This enables direct flash/torch on/off by the driver, kernel space.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_PATTERN
+	tristate "LED Pattern Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  This allows LEDs to blink in a user defined pattern controlled via
+          sysfs. It's useful to notify different states by using one led.
+	  For more details read Documentation/leds/leds-pattern.txt.
+	  If unsure, say Y.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 1abf48d..a739429 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_LEDS_TRIGGER_CPU)		+= ledtrig-cpu.o
 obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
 obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
+obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
new file mode 100644
index 0000000..9c688e7
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-pattern.c
@@ -0,0 +1,233 @@
+/*
+ * LED Kernel Morse Code Trigger
+ *
+ * Copyright (C) 2013 Joe Xue <lgxue@hotmail.com>
+ *
+ * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
+ * ledtrig-heartbeat.c and Shuah Khan's ledtrig-transient.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/slab.h>
+#include <linux/timer.h>
+#include <linux/leds.h>
+#include "../leds.h"
+
+struct pattern_trig_data {
+	unsigned long delay_unit;
+	unsigned long pattern_len;
+	unsigned long count;
+	int brightness_on;
+	char *pattern;
+	struct timer_list timer;
+	struct mutex pattern_mutex;
+};
+
+static void pattern_timer_function(unsigned long data)
+{
+	int repeat = 1;
+	struct led_classdev *led_cdev = (struct led_classdev *) data;
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+	char this = pattern_data->pattern[pattern_data->count];
+
+	if (this == '#') {
+		__led_set_brightness(led_cdev, pattern_data->brightness_on);
+	} else if (this == ' ') {
+		__led_set_brightness(led_cdev, LED_OFF);
+	/* stop blinking */
+	} else if (this == '/') {
+		return;
+	}
+
+	while (pattern_data->pattern[pattern_data->count + repeat] == this)
+		repeat++;
+
+	mod_timer(&pattern_data->timer,
+		jiffies + msecs_to_jiffies(pattern_data->delay_unit * repeat));
+
+	pattern_data->count += repeat;
+	if (pattern_data->count == pattern_data->pattern_len)
+		pattern_data->count = 0;
+}
+
+static ssize_t pattern_delay_unit_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	return sprintf(buf, "%lu\n", pattern_data->delay_unit);
+}
+
+static ssize_t pattern_delay_unit_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+	unsigned long state;
+	ssize_t ret = -EINVAL;
+
+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	/* this value can't be 0 */
+	if (state == 0)
+		return -EINVAL;
+
+	pattern_data->delay_unit = state;
+
+	return size;
+}
+
+static ssize_t pattern_pattern_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	if (pattern_data->pattern)
+		return scnprintf(buf, PAGE_SIZE, "%s\n",
+				pattern_data->pattern);
+	return 0;
+}
+
+static ssize_t pattern_pattern_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+	int i, len;
+	ssize_t ret = -EINVAL;
+
+	len = 0;
+
+	/* legality check */
+	for (i = 0; i < size; i++) {
+		if (buf[i] != ' ' && buf[i] != '#' && buf[i] != '/')
+			break;
+		len++;
+	}
+
+	if (len == 0)
+		return ret;
+
+	mutex_lock(&pattern_data->pattern_mutex);
+	kfree(pattern_data->pattern);
+	pattern_data->pattern = NULL;
+
+	pattern_data->pattern = kzalloc(len + 1, GFP_KERNEL);
+	if (!pattern_data->pattern) {
+		len = -ENOMEM;
+		goto unlock;
+	}
+	scnprintf(pattern_data->pattern, len + 1, "%s", buf);
+
+	del_timer_sync(&pattern_data->timer);
+
+	pattern_data->pattern_len = len;
+	pattern_data->count = 0;
+
+	mod_timer(&pattern_data->timer, jiffies + 1);
+unlock:
+	mutex_unlock(&pattern_data->pattern_mutex);
+	return size;
+}
+
+static DEVICE_ATTR(pattern, 0644, pattern_pattern_show, pattern_pattern_store);
+static DEVICE_ATTR(delay_unit, 0644,
+		pattern_delay_unit_show, pattern_delay_unit_store);
+
+static void pattern_trig_activate(struct led_classdev *led_cdev)
+{
+	int rc;
+	struct pattern_trig_data *tdata;
+
+	tdata = kzalloc(sizeof(struct pattern_trig_data), GFP_KERNEL);
+	if (!tdata) {
+		dev_err(led_cdev->dev,
+			"unable to allocate pattern trigger\n");
+		return;
+	}
+
+	led_cdev->trigger_data = tdata;
+
+	mutex_init(&tdata->pattern_mutex);
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_pattern);
+	if (rc)
+		goto err_out;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_delay_unit);
+	if (rc)
+		goto err_out_delay_unit;
+
+	/* default delay_unit 125ms */
+	tdata->delay_unit = 125;
+
+	setup_timer(&tdata->timer, pattern_timer_function,
+		    (unsigned long) led_cdev);
+
+	tdata->brightness_on = led_get_brightness(led_cdev);
+	if (tdata->brightness_on == LED_OFF)
+		tdata->brightness_on = led_cdev->max_brightness;
+
+	__led_set_brightness(led_cdev, LED_OFF);
+	led_cdev->activated = true;
+
+	return;
+
+err_out_delay_unit:
+	device_remove_file(led_cdev->dev, &dev_attr_pattern);
+err_out:
+	dev_err(led_cdev->dev, "unable to register pattern trigger\n");
+	led_cdev->trigger_data = NULL;
+	kfree(tdata);
+}
+
+static void pattern_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	if (led_cdev->activated) {
+		del_timer_sync(&pattern_data->timer);
+		device_remove_file(led_cdev->dev, &dev_attr_pattern);
+		device_remove_file(led_cdev->dev, &dev_attr_delay_unit);
+		led_cdev->trigger_data = NULL;
+		led_cdev->activated = false;
+		kfree(pattern_data->pattern);
+		kfree(pattern_data);
+	}
+	__led_set_brightness(led_cdev, LED_OFF);
+}
+
+static struct led_trigger pattern_trigger = {
+	.name     = "pattern",
+	.activate = pattern_trig_activate,
+	.deactivate = pattern_trig_deactivate,
+};
+
+static int __init pattern_trig_init(void)
+{
+	return led_trigger_register(&pattern_trigger);
+}
+
+static void __exit pattern_trig_exit(void)
+{
+	led_trigger_unregister(&pattern_trigger);
+}
+
+module_init(pattern_trig_init);
+module_exit(pattern_trig_exit);
+
+MODULE_AUTHOR("Joe Xue <lgxue@hotmail.com");
+MODULE_DESCRIPTION("Pattern LED trigger");
+MODULE_LICENSE("GPL");
-- 
1.8.3.2

----------------------------------------
> From: lgxue@hotmail.com
> To: cooloney@gmail.com; rpurdie@rpsys.net; rob@landley.net; milo.kim@ti.com; pavel@ucw.cz
> CC: lgxue@hotmail.com; linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org
> Subject: [PATCH] Add LED pattern trigger
> Date: Sun, 29 Dec 2013 19:11:15 -0500
>
> The LED pattern trigger allows LEDs blink in user defined pattern.
>
> new file: Documentation/leds/ledtrig-pattern.txt
> modified: drivers/leds/trigger/Kconfig
> modified: drivers/leds/trigger/Makefile
> new file: drivers/leds/trigger/ledtrig-pattern.c
>
> Suggested-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Joe Xue <lgxue@hotmail.com>
> ---
> Documentation/leds/ledtrig-pattern.txt | 60 ++++++++++
> drivers/leds/trigger/Kconfig | 9 ++
> drivers/leds/trigger/Makefile | 1 +
> drivers/leds/trigger/ledtrig-pattern.c | 207 +++++++++++++++++++++++++++++++++
> 4 files changed, 277 insertions(+)
> create mode 100644 Documentation/leds/ledtrig-pattern.txt
> create mode 100644 drivers/leds/trigger/ledtrig-pattern.c
>
> diff --git a/Documentation/leds/ledtrig-pattern.txt b/Documentation/leds/ledtrig-pattern.txt
> new file mode 100644
> index 0000000..4b546c3
> --- /dev/null
> +++ b/Documentation/leds/ledtrig-pattern.txt
> @@ -0,0 +1,60 @@
> +LED Pattern Trigger
> +===================
> +
> +0. Introduction
> +
> +LED Pattern trigger is designed to let LEDs indicate the patterns defined by
> +users. This is very useful for those scenarios where only one non-color led
> +needs to indicate different states.
> +
> +1. How to use
> +
> +Pattern trigger can be enabled and disabled from user space on led class
> +devices that support this trigger as shown below:
> +
> + echo pattern> trigger
> +
> +When the pattern trigger is activated, it will get the current brightness as
> +its brightness if the led is on. Otherwise, it will use the maximum brightness.
> +
> +If the led supports different values rather than ON/OFF, the brightness can be
> +set in advance before enabling the trigger.
> +
> + echo 128> brightness
> + echo pattern> trigger
> +
> +Two properties are exported. They are delay_unit and pattern.
> +
> + delay_unit - a delay time unit, default value is 125 ms
> + pattern - blink pattern, includes three legal characters
> + '#', ' '(space), and '/'
> + '#' let LED on and last delay_unit long time
> + ' ' let LED off and last delay_unit long time
> + '/' stop pattern
> + pattern will be repeated without it
> +
> +3. Examples
> +
> + Example 1
> +
> + echo pattern> trigger
> + echo "# ## /"
> +
> + The behaviour is like below:
> +
> + on(125ms)off(125ms)on(250ms)off
> + This is Morse code 'A'
> +
> + Example 2
> +
> + echo pattern> trigger
> + echo 200> delay_unit
> + echo "# # "
> +
> + The behaviour is like below:
> +
> + on(200ms)off(200ms)on(200ms)off(800ms)
> + on(200ms)off(200ms)on(200ms)off(800ms)
> + ...(Repeat)
> +
> + This is 2 times burst blinking.
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index 49794b4..23d0967 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -108,4 +108,13 @@ config LEDS_TRIGGER_CAMERA
> This enables direct flash/torch on/off by the driver, kernel space.
> If unsure, say Y.
>
> +config LEDS_TRIGGER_PATTERN
> + tristate "LED Pattern Trigger"
> + depends on LEDS_TRIGGERS
> + help
> + This allows LEDs to blink in a user defined pattern controlled via
> + sysfs. It's useful to notify different states by using one led.
> + For more details read Documentation/leds/leds-pattern.txt.
> + If unsure, say Y.
> +
> endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index 1abf48d..a739429 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_LEDS_TRIGGER_CPU) += ledtrig-cpu.o
> obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o
> obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o
> obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o
> +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o
> diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
> new file mode 100644
> index 0000000..80fc238
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-pattern.c
> @@ -0,0 +1,207 @@
> +/*
> + * LED Kernel Morse Code Trigger
> + *
> + * Copyright (C) 2013 Joe Xue <lgxue@hotmail.com>
> + *
> + * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
> + * ledtrig-heartbeat.c and Shuah Khan's ledtrig-transient.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/slab.h>
> +#include <linux/timer.h>
> +#include <linux/leds.h>
> +#include "../leds.h"
> +
> +#define MAX_PATTEN_LEN 255
> +
> +struct pattern_trig_data {
> + unsigned long delay_unit;
> + unsigned long pattern_len;
> + unsigned long count;
> + int brightness_on;
> + char pattern[MAX_PATTEN_LEN + 1];
> + struct timer_list timer;
> +};
> +
> +static void pattern_timer_function(unsigned long data)
> +{
> + struct led_classdev *led_cdev = (struct led_classdev *) data;
> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> +
> + if (pattern_data->pattern[pattern_data->count] == '#') {
> + __led_set_brightness(led_cdev, pattern_data->brightness_on);
> + mod_timer(&pattern_data->timer,
> + jiffies + msecs_to_jiffies(pattern_data->delay_unit));
> + } else if (pattern_data->pattern[pattern_data->count] == ' ') {
> + __led_set_brightness(led_cdev, LED_OFF);
> + mod_timer(&pattern_data->timer,
> + jiffies + msecs_to_jiffies(pattern_data->delay_unit));
> + /* stop blinking */
> + } else if (pattern_data->pattern[pattern_data->count] == '/') {
> + return;
> + }
> +
> + pattern_data->count++;
> + if (pattern_data->count == pattern_data->pattern_len)
> + pattern_data->count = 0;
> +}
> +
> +static ssize_t pattern_delay_unit_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> +
> + return sprintf(buf, "%lu\n", pattern_data->delay_unit);
> +}
> +
> +static ssize_t pattern_delay_unit_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t size)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> + unsigned long state;
> + ssize_t ret = -EINVAL;
> +
> + ret = kstrtoul(buf, 10, &state);
> + if (ret)
> + return ret;
> +
> + pattern_data->delay_unit = state;
> +
> + return size;
> +}
> +
> +static ssize_t pattern_pattern_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> +
> + return sprintf(buf, "%s\n", pattern_data->pattern);
> +}
> +
> +static ssize_t pattern_pattern_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t size)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> + int i;
> + ssize_t ret = -EINVAL;
> +
> + int len = (size> MAX_PATTEN_LEN) ? MAX_PATTEN_LEN : (size - 1);
> +
> + /* legality check */
> + for (i = 0; i < len; i++) {
> + if (buf[i] != ' ' && buf[i] != '#' && buf[i] != '/')
> + return ret;
> + }
> +
> + del_timer_sync(&pattern_data->timer);
> +
> + memcpy(pattern_data->pattern, buf, len);
> + pattern_data->pattern[len] = '\0';
> + pattern_data->pattern_len = len;
> + pattern_data->count = 0;
> +
> + mod_timer(&pattern_data->timer, jiffies + 1);
> +
> + return size;
> +}
> +
> +static DEVICE_ATTR(pattern, 0644, pattern_pattern_show, pattern_pattern_store);
> +static DEVICE_ATTR(delay_unit, 0644,
> + pattern_delay_unit_show, pattern_delay_unit_store);
> +
> +static void pattern_trig_activate(struct led_classdev *led_cdev)
> +{
> + int rc;
> + struct pattern_trig_data *tdata;
> +
> + tdata = kzalloc(sizeof(struct pattern_trig_data), GFP_KERNEL);
> + if (!tdata) {
> + dev_err(led_cdev->dev,
> + "unable to allocate pattern trigger\n");
> + return;
> + }
> +
> + led_cdev->trigger_data = tdata;
> +
> + rc = device_create_file(led_cdev->dev, &dev_attr_pattern);
> + if (rc)
> + goto err_out;
> +
> + rc = device_create_file(led_cdev->dev, &dev_attr_delay_unit);
> + if (rc)
> + goto err_out_delay_unit;
> +
> + memset(tdata->pattern, 0, MAX_PATTEN_LEN + 1);
> + /* default delay_unit 125ms */
> + tdata->delay_unit = 125;
> +
> + setup_timer(&tdata->timer, pattern_timer_function,
> + (unsigned long) led_cdev);
> +
> + tdata->brightness_on = led_get_brightness(led_cdev);
> + if (tdata->brightness_on == LED_OFF)
> + tdata->brightness_on = led_cdev->max_brightness;
> +
> + __led_set_brightness(led_cdev, LED_OFF);
> + led_cdev->activated = true;
> +
> + return;
> +
> +err_out_delay_unit:
> + device_remove_file(led_cdev->dev, &dev_attr_pattern);
> +err_out:
> + dev_err(led_cdev->dev, "unable to register pattern trigger\n");
> + led_cdev->trigger_data = NULL;
> + kfree(tdata);
> +}
> +
> +static void pattern_trig_deactivate(struct led_classdev *led_cdev)
> +{
> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> +
> + if (led_cdev->activated) {
> + del_timer_sync(&pattern_data->timer);
> + device_remove_file(led_cdev->dev, &dev_attr_pattern);
> + device_remove_file(led_cdev->dev, &dev_attr_delay_unit);
> + led_cdev->trigger_data = NULL;
> + led_cdev->activated = false;
> + kfree(pattern_data);
> + }
> + __led_set_brightness(led_cdev, LED_OFF);
> +}
> +
> +static struct led_trigger pattern_trigger = {
> + .name = "pattern",
> + .activate = pattern_trig_activate,
> + .deactivate = pattern_trig_deactivate,
> +};
> +
> +static int __init pattern_trig_init(void)
> +{
> + return led_trigger_register(&pattern_trigger);
> +}
> +
> +static void __exit pattern_trig_exit(void)
> +{
> + led_trigger_unregister(&pattern_trigger);
> +}
> +
> +module_init(pattern_trig_init);
> +module_exit(pattern_trig_exit);
> +
> +MODULE_AUTHOR("Joe Xue <lgxue@hotmail.com");
> +MODULE_DESCRIPTION("Patten LED trigger");
> +MODULE_LICENSE("GPL");
> --
> 1.8.1.2
>
> --
> 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] Add LED pattern trigger

[One Thousand Gnomes]

On Tue, 31 Dec 2013 13:48:50 -0500
Joe Xue <lgxue@hotmail.com> wrote:

> >> + * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
> >> + * ledtrig-heartbeat.c and Shuah Khan's ledtrig-transient.c
> >
> > I stil think this belongs in user space except for platforms with hardware
> > acceleration for it.
> 
> This can free the user space application from loop or thread.

Which is not a good reason for putting it in the kernel. I could make the
same argument for putting firefox in the kernel ...

> > This doesn't as far as I can see do what you think. If I have the file
> > currently open then device_remove_file will not remove my existing access
> > to it, but you just released the pattern data so I now write to free
> > memory.
> 
> will add the mutex lock to avoid that 		 	   		  

Ok

Re: [PATCH] Add LED pattern trigger

[One Thousand Gnomes]

> > Why do we need this within the kernel?
> > Patterns can easily created using a simple user space program.
> 
> Some machines (N900) can do blinking in hardware, and we want
> consistent kernel-user interface. See the mailing list.

We have things called "libraries", see the mailing list.

Alan

Re: [PATCH] Add LED pattern trigger

[David Lang]

On Wed, 1 Jan 2014, One Thousand Gnomes wrote:

> On Tue, 31 Dec 2013 13:48:50 -0500
> Joe Xue <lgxue@hotmail.com> wrote:
>
>>>> + * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
>>>> + * ledtrig-heartbeat.c and Shuah Khan's ledtrig-transient.c
>>>
>>> I stil think this belongs in user space except for platforms with hardware
>>> acceleration for it.
>>
>> This can free the user space application from loop or thread.
>
> Which is not a good reason for putting it in the kernel. I could make the
> same argument for putting firefox in the kernel ...
>

I agree with you that this isn't a good reason, but I think the performance 
could be a reason.

whatever mechanism is created for toggling LEDs should be able to toggle 
arbitrary GPIO pins, and there is a problem with the speed of the standard 
access mechanisms in /sysfs. see this post on hackaday for an example

http://hackaday.com/2013/12/07/speeding-up-beaglebone-black-gpio-a-thousand-times/

now, it may be that telling people to access /dev/mem is deemed a better option, 
but I would hope not.

Also, since there are a number of cases where this is hardware accelerated, it 
seems like there should be an abstration that userspace can use that doesn't 
care if or how it's accelerated, setup the output and tell the system to do it 
without worrying about the specific hardware details. Isn't that a large part of 
what the kernel is supposed to be doing?

David Lang

Re: [PATCH] Add LED pattern trigger

[One Thousand Gnomes]

> whatever mechanism is created for toggling LEDs should be able to toggle 
> arbitrary GPIO pins, and there is a problem with the speed of the standard 
> access mechanisms in /sysfs. see this post on hackaday for an example
> 
> http://hackaday.com/2013/12/07/speeding-up-beaglebone-black-gpio-a-thousand-times/

The usage described is short human speed flashing patterns for things like
"my brain fell out" from devices, not trying to do 1KHz PWM dimming.
Dimming might actually be one case you want the kernel interface,
although it'll kill your power management.

> Also, since there are a number of cases where this is hardware accelerated, it 
> seems like there should be an abstration that userspace can use that doesn't 
> care if or how it's accelerated, setup the output and tell the system to do it 
> without worrying about the specific hardware details. Isn't that a large part of 
> what the kernel is supposed to be doing?

Not usually. The kernel is supposed to be providing a consistent interface
to hardware, not emulating bits you don't have. Now and then it does (Eg
FPU emulation) but in general the job it does is "make all the network
cards look the same" not "make pretend network cards out of string and
cups". It's not a hard and fast rule in either direction. There are cases
the kernel doesn't try and create a common interface for the hardware
because the abstraction that can be done at kernel level would be
nonsensical.

A library also allows a higher level of abstraction and better security,
and it allows consistency a kernel interface cannot provide as well as
not pinning down memory which at least in embedded space is valuable (and
may become more so in the 'internet of things' world of lower and lower
power and cheaper and cheaper widgets)

At best a kernel interface would mean people writing "post 3.15 do this,
pre 3.15 do the other". A library interface avoids that as it will work
with old kernels too, and can be taught to interface with acceleration
features, or even with other device types. A kernel interface cannot
drive X.10 for example, drive a remote bluetooth display, beep the code or
flash the LED patterns as an overlay on a monitor. Nor can it do things
like automatically routing the alert based upon stuff like "is the
display on", "is the management interface up" etc.

The library interface can also be made to do sensible things in virtual
environments, on other operating systems and so forth.

Alan

Re: [PATCH] Add LED pattern trigger

[David Lang]

On Wed, 1 Jan 2014, One Thousand Gnomes wrote:

>> whatever mechanism is created for toggling LEDs should be able to toggle
>> arbitrary GPIO pins, and there is a problem with the speed of the standard
>> access mechanisms in /sysfs. see this post on hackaday for an example
>>
>> http://hackaday.com/2013/12/07/speeding-up-beaglebone-black-gpio-a-thousand-times/
>
> The usage described is short human speed flashing patterns for things like
> "my brain fell out" from devices, not trying to do 1KHz PWM dimming.
> Dimming might actually be one case you want the kernel interface,
> although it'll kill your power management.

any high speed signalling would probably also need the kernel interface.

>> Also, since there are a number of cases where this is hardware accelerated, it
>> seems like there should be an abstration that userspace can use that doesn't
>> care if or how it's accelerated, setup the output and tell the system to do it
>> without worrying about the specific hardware details. Isn't that a large part of
>> what the kernel is supposed to be doing?
>
> Not usually. The kernel is supposed to be providing a consistent interface
> to hardware, not emulating bits you don't have. Now and then it does (Eg
> FPU emulation) but in general the job it does is "make all the network
> cards look the same" not "make pretend network cards out of string and
> cups". It's not a hard and fast rule in either direction. There are cases
> the kernel doesn't try and create a common interface for the hardware
> because the abstraction that can be done at kernel level would be
> nonsensical.

fair enough, would it make sense to redirect the discussion to focus on what a 
good interface would be for the cases where there is special hardware 
assistance? Then the discussion of if the same interface could/should be used to 
emulate harsdware when it isn't there can be a seprate discussion.

And while this use case the original developer had in mind was the 'I've lost my 
mind' notification to a human, I think it makes sense to consider other uses for 
repeating pattern toggling of GPIO ports.

David Lang

Re: [PATCH] Add LED pattern trigger

[Bryan Wu]

On Wed, Jan 1, 2014 at 3:51 PM, David Lang <david@lang.hm> wrote:
> On Wed, 1 Jan 2014, One Thousand Gnomes wrote:
>
>>> whatever mechanism is created for toggling LEDs should be able to toggle
>>> arbitrary GPIO pins, and there is a problem with the speed of the
>>> standard
>>> access mechanisms in /sysfs. see this post on hackaday for an example
>>>
>>>
>>> http://hackaday.com/2013/12/07/speeding-up-beaglebone-black-gpio-a-thousand-times/
>>
>>
>> The usage described is short human speed flashing patterns for things like
>> "my brain fell out" from devices, not trying to do 1KHz PWM dimming.
>> Dimming might actually be one case you want the kernel interface,
>> although it'll kill your power management.
>
>
> any high speed signalling would probably also need the kernel interface.
>
>
>>> Also, since there are a number of cases where this is hardware
>>> accelerated, it
>>> seems like there should be an abstration that userspace can use that
>>> doesn't
>>> care if or how it's accelerated, setup the output and tell the system to
>>> do it
>>> without worrying about the specific hardware details. Isn't that a large
>>> part of
>>> what the kernel is supposed to be doing?
>>
>>
>> Not usually. The kernel is supposed to be providing a consistent interface
>> to hardware, not emulating bits you don't have. Now and then it does (Eg
>> FPU emulation) but in general the job it does is "make all the network
>> cards look the same" not "make pretend network cards out of string and
>> cups". It's not a hard and fast rule in either direction. There are cases
>> the kernel doesn't try and create a common interface for the hardware
>> because the abstraction that can be done at kernel level would be
>> nonsensical.
>
>
> fair enough, would it make sense to redirect the discussion to focus on what
> a good interface would be for the cases where there is special hardware
> assistance? Then the discussion of if the same interface could/should be
> used to emulate harsdware when it isn't there can be a seprate discussion.
>
> And while this use case the original developer had in mind was the 'I've
> lost my mind' notification to a human, I think it makes sense to consider
> other uses for repeating pattern toggling of GPIO ports.
>
> David Lang

Hi folks,

I probably missed this hot discussion during the holiday in our
linux-leds community. After read all the emails of this topic, I think
this is a good idea to take this pattern drivers as a led trigger.

Actually we will see more new LED chips with hardware acceleration and
most of them support like data pattern operations. (see
drivers/leds/leds-lp55xx-common.c) Having a generic led trigger driver
will be good for those LED chip drivers.

Also for user space application, I think we don't have any user space
LED library, if I'm wrong please correct me. Why there is no such
library, since we don't need it.
led trigger driver can provide most generic led operations and people
don't need to know what's kind of hardware is under neath. If it has
hardware acceleration, LED framework will use it otherwise just use
normal operation. Take a look at other trigger drivers, heartbeat etc.
All of the operations can be done via sysfs.

So for the user space application, we just simply need shell scripts.
1. load LED pattern trigger module
2. setup pattern trigger to LED /sys interface
3. then use it.

IMHO, firstly we should take this trigger into kernel, most time it
works as a module. But we need to define a good interface between
kernel and user space.

Thanks,
-Bryan

Re: [PATCH] Add LED pattern trigger

[Richard Weinberger]

On Fri, Jan 3, 2014 at 1:14 AM, Bryan Wu <cooloney@gmail.com> wrote:
> IMHO, firstly we should take this trigger into kernel, most time it
> works as a module. But we need to define a good interface between
> kernel and user space.

Put it in first place into the kernel such that it becomes ABI and
nobody is allowed
to remove it later?

Better design a sane interface such that such complex LED operations
can be achieved
in userspace using an helper library.

-- 
Thanks,
//richard

Re: [PATCH] Add LED pattern trigger

[One Thousand Gnomes]

> Also for user space application, I think we don't have any user space
> LED library, if I'm wrong please correct me. Why there is no such
> library, since we don't need it.

No - rght now it is a case of "we don't have a kernel driver because we
don't need one"

> IMHO, firstly we should take this trigger into kernel, most time it
> works as a module. But we need to define a good interface between
> kernel and user space.

You need the interface defined first. To do that it needs to reflect the
actual hardware accelerated devices, and also to deal with resource
management for those devices if necessary (eg if they can only manage one
led of a set at a time).

Your API can't handle things like brightness level, cross-fades
(which require multiple LEDs handled as one unit) and the like.

So the starting point has to be the hardware accelerated devices, whether
you then support software emulation in kernel or user space is a follow
on discussion. What the kernel/user API is also has to be a follow on
discussion from understanding what the hardware accelerated devices can
do and what their limits are.

Alan

n900 led compiler (was Re: [PATCH] Add LED pattern trigger)

[Pavel Machek]

Hi!

> > IMHO, firstly we should take this trigger into kernel, most time it
> > works as a module. But we need to define a good interface between
> > kernel and user space.
> 
> You need the interface defined first. To do that it needs to reflect the
> actual hardware accelerated devices, and also to deal with resource
> management for those devices if necessary (eg if they can only manage one
> led of a set at a time).

Hardware can do quite a lot:

http://wiki.maemo.org/LED_patterns#Lysti_Format_Engine_Patterns_and_Commands

(and more).

I implemented compiler for it (should we put it into tools/ somewhere?)

https://gitorious.org/tui/tui/source/5b3f5cacf8e208d3ea50d6066e549940d85e55be:maemo/notcc.py

It can do quite a lot, including prime number computation. This uses
33% of program memory and only one of three execution units; but it
does not work, maybe I made mistake somewhere or maybe our kernel
can't take program this long. It only has 3 writable variables, which
is quite limiting.

start()
a = 1
next_number: a += 1
b = 1
next_divisor: b += 1
br = b
if (b==a) goto is_prime
c = 0
c = c+b
test_prime: if (c==a) goto not_prime
if (a<c) goto not_divisor
c = c+b
goto test_prime
not_divisor: goto next_divisor
not_prime: goto next_number
is_prime: b = 0
show_prime: c = 255
br = c
ramp_up_long(30,0)
c = 0
br = c
ramp_up_long(30,0)
b += 1
if (b == a) goto next_number2
goto show_prime
next_number2: goto next_number

> So the starting point has to be the hardware accelerated devices, whether
> you then support software emulation in kernel or user space is a follow
> on discussion. What the kernel/user API is also has to be a follow on

Well... this one is turing complete, and I have a compiler, but I
don't think it is good interface, either for library or kernel.

I believe series of RGB values might be good interface, maybe with
additional "want interpolation between these".
     		      	  	      	     	      	      	     Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

n900 led compiler (was Re: [PATCH] Add LED pattern trigger)

[Pavel Machek]

Hi!

> > IMHO, firstly we should take this trigger into kernel, most time it
> > works as a module. But we need to define a good interface between
> > kernel and user space.
> 
> You need the interface defined first. To do that it needs to reflect the
> actual hardware accelerated devices, and also to deal with resource
> management for those devices if necessary (eg if they can only manage one
> led of a set at a time).

Hardware can do quite a lot:

http://wiki.maemo.org/LED_patterns#Lysti_Format_Engine_Patterns_and_Commands

(and more).

I implemented compiler for it (should we put it into tools/ somewhere?)

https://gitorious.org/tui/tui/source/5b3f5cacf8e208d3ea50d6066e549940d85e55be:maemo/notcc.py

It can do quite a lot, including prime number computation. This uses
33% of program memory and only one of three execution units; but it
does not work, maybe I made mistake somewhere or maybe our kernel
can't take program this long. It only has 3 writable variables, which
is quite limiting.

start()
a = 1
next_number: a += 1
b = 1
next_divisor: b += 1
br = b
if (b==a) goto is_prime
c = 0
c = c+b
test_prime: if (c==a) goto not_prime
if (a<c) goto not_divisor
c = c+b
goto test_prime
not_divisor: goto next_divisor
not_prime: goto next_number
is_prime: b = 0
show_prime: c = 255
br = c
ramp_up_long(30,0)
c = 0
br = c
ramp_up_long(30,0)
b += 1
if (b == a) goto next_number2
goto show_prime
next_number2: goto next_number

> So the starting point has to be the hardware accelerated devices, whether
> you then support software emulation in kernel or user space is a follow
> on discussion. What the kernel/user API is also has to be a follow on

Well... this one is turing complete, and I have a compiler, but I
don't think it is good interface, either for library or kernel.

I believe series of RGB values might be good interface, maybe with
additional "want interpolation between these".
     		      	  	      	     	      	      	     Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH v3 1/1] leds: Add LED pattern trigger

[lgxue]

From: Joe Xue <lgxue@hotmail.com>

The LED pattern trigger allows LEDs blink in user defined pattern.

v2: Change the pattern memory from fixed static to malloc
    Change the timer schedule way to save cpu time
    Add the mutex to protect the pattern operation

v3: Tiny fix, add the missed parameter in example of document

	new file:   Documentation/leds/ledtrig-pattern.txt
	modified:   drivers/leds/trigger/Kconfig
	modified:   drivers/leds/trigger/Makefile
	new file:   drivers/leds/trigger/ledtrig-pattern.c

Signed-off-by: Joe Xue <lgxue@hotmail.com>
---
 Documentation/leds/ledtrig-pattern.txt |  60 +++++++++
 drivers/leds/trigger/Kconfig           |   9 ++
 drivers/leds/trigger/Makefile          |   1 +
 drivers/leds/trigger/ledtrig-pattern.c | 233 +++++++++++++++++++++++++++++++++
 4 files changed, 303 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-pattern.txt
 create mode 100644 drivers/leds/trigger/ledtrig-pattern.c

diff --git a/Documentation/leds/ledtrig-pattern.txt b/Documentation/leds/ledtrig-pattern.txt
new file mode 100644
index 0000000..ac0877c
--- /dev/null
+++ b/Documentation/leds/ledtrig-pattern.txt
@@ -0,0 +1,60 @@
+LED Pattern Trigger
+===================
+
+0. Introduction
+
+LED Pattern trigger is designed to let LEDs indicate the patterns defined by
+users. This is very useful for those scenarios where only one non-color led
+needs to indicate different states.
+
+1. How to use
+
+Pattern trigger can be enabled and disabled from user space on led class
+devices that support this trigger as shown below:
+
+     echo pattern > trigger
+
+When the pattern trigger is activated, it will get the current brightness as
+its brightness if the led is on. Otherwise, it will use the maximum brightness.
+
+If the led supports different values rather than ON/OFF, the brightness can be
+set in advance before enabling the trigger.
+
+     echo 128 > brightness
+     echo pattern > trigger
+
+Two properties are exported. They are delay_unit and pattern.
+
+     delay_unit - a delay time unit, default value is 125 ms
+     pattern    - blink pattern, includes three legal characters
+                  '#', ' '(space), and '/'
+                  '#' let LED on and last delay_unit long time
+                  ' ' let LED off and last delay_unit long time
+                  '/' stop pattern
+                      pattern will be repeated without it
+
+3. Examples
+
+     Example 1
+
+     echo pattern > trigger
+     echo "# ## /" > pattern
+
+     The behaviour is like below:
+
+     on(125ms)off(125ms)on(250ms)off
+     This is Morse code 'A'
+
+     Example 2
+
+     echo pattern > trigger
+     echo 200 > delay_unit
+     echo "# #    " > pattern
+
+     The behaviour is like below:
+
+     on(200ms)off(200ms)on(200ms)off(800ms)
+     on(200ms)off(200ms)on(200ms)off(800ms)
+     ...(Repeat)
+
+     This is 2 times burst blinking.
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 49794b4..23d0967 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -108,4 +108,13 @@ config LEDS_TRIGGER_CAMERA
 	  This enables direct flash/torch on/off by the driver, kernel space.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_PATTERN
+	tristate "LED Pattern Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  This allows LEDs to blink in a user defined pattern controlled via
+          sysfs. It's useful to notify different states by using one led.
+	  For more details read Documentation/leds/leds-pattern.txt.
+	  If unsure, say Y.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 1abf48d..a739429 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_LEDS_TRIGGER_CPU)		+= ledtrig-cpu.o
 obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
 obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
+obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
new file mode 100644
index 0000000..9c688e7
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-pattern.c
@@ -0,0 +1,233 @@
+/*
+ * LED Kernel Morse Code Trigger
+ *
+ * Copyright (C) 2013 Joe Xue <lgxue@hotmail.com>
+ *
+ * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
+ * ledtrig-heartbeat.c and Shuah Khan's ledtrig-transient.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/slab.h>
+#include <linux/timer.h>
+#include <linux/leds.h>
+#include "../leds.h"
+
+struct pattern_trig_data {
+	unsigned long delay_unit;
+	unsigned long pattern_len;
+	unsigned long count;
+	int brightness_on;
+	char *pattern;
+	struct timer_list timer;
+	struct mutex pattern_mutex;
+};
+
+static void pattern_timer_function(unsigned long data)
+{
+	int repeat = 1;
+	struct led_classdev *led_cdev = (struct led_classdev *) data;
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+	char this = pattern_data->pattern[pattern_data->count];
+
+	if (this == '#') {
+		__led_set_brightness(led_cdev, pattern_data->brightness_on);
+	} else if (this == ' ') {
+		__led_set_brightness(led_cdev, LED_OFF);
+	/* stop blinking */
+	} else if (this == '/') {
+		return;
+	}
+
+	while (pattern_data->pattern[pattern_data->count + repeat] == this)
+		repeat++;
+
+	mod_timer(&pattern_data->timer,
+		jiffies + msecs_to_jiffies(pattern_data->delay_unit * repeat));
+
+	pattern_data->count += repeat;
+	if (pattern_data->count == pattern_data->pattern_len)
+		pattern_data->count = 0;
+}
+
+static ssize_t pattern_delay_unit_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	return sprintf(buf, "%lu\n", pattern_data->delay_unit);
+}
+
+static ssize_t pattern_delay_unit_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+	unsigned long state;
+	ssize_t ret = -EINVAL;
+
+	ret = kstrtoul(buf, 10, &state);
+	if (ret)
+		return ret;
+
+	/* this value can't be 0 */
+	if (state == 0)
+		return -EINVAL;
+
+	pattern_data->delay_unit = state;
+
+	return size;
+}
+
+static ssize_t pattern_pattern_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	if (pattern_data->pattern)
+		return scnprintf(buf, PAGE_SIZE, "%s\n",
+				pattern_data->pattern);
+	return 0;
+}
+
+static ssize_t pattern_pattern_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+	int i, len;
+	ssize_t ret = -EINVAL;
+
+	len = 0;
+
+	/* legality check */
+	for (i = 0; i < size; i++) {
+		if (buf[i] != ' ' && buf[i] != '#' && buf[i] != '/')
+			break;
+		len++;
+	}
+
+	if (len == 0)
+		return ret;
+
+	mutex_lock(&pattern_data->pattern_mutex);
+	kfree(pattern_data->pattern);
+	pattern_data->pattern = NULL;
+
+	pattern_data->pattern = kzalloc(len + 1, GFP_KERNEL);
+	if (!pattern_data->pattern) {
+		len = -ENOMEM;
+		goto unlock;
+	}
+	scnprintf(pattern_data->pattern, len + 1, "%s", buf);
+
+	del_timer_sync(&pattern_data->timer);
+
+	pattern_data->pattern_len = len;
+	pattern_data->count = 0;
+
+	mod_timer(&pattern_data->timer, jiffies + 1);
+unlock:
+	mutex_unlock(&pattern_data->pattern_mutex);
+	return size;
+}
+
+static DEVICE_ATTR(pattern, 0644, pattern_pattern_show, pattern_pattern_store);
+static DEVICE_ATTR(delay_unit, 0644,
+		pattern_delay_unit_show, pattern_delay_unit_store);
+
+static void pattern_trig_activate(struct led_classdev *led_cdev)
+{
+	int rc;
+	struct pattern_trig_data *tdata;
+
+	tdata = kzalloc(sizeof(struct pattern_trig_data), GFP_KERNEL);
+	if (!tdata) {
+		dev_err(led_cdev->dev,
+			"unable to allocate pattern trigger\n");
+		return;
+	}
+
+	led_cdev->trigger_data = tdata;
+
+	mutex_init(&tdata->pattern_mutex);
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_pattern);
+	if (rc)
+		goto err_out;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_delay_unit);
+	if (rc)
+		goto err_out_delay_unit;
+
+	/* default delay_unit 125ms */
+	tdata->delay_unit = 125;
+
+	setup_timer(&tdata->timer, pattern_timer_function,
+		    (unsigned long) led_cdev);
+
+	tdata->brightness_on = led_get_brightness(led_cdev);
+	if (tdata->brightness_on == LED_OFF)
+		tdata->brightness_on = led_cdev->max_brightness;
+
+	__led_set_brightness(led_cdev, LED_OFF);
+	led_cdev->activated = true;
+
+	return;
+
+err_out_delay_unit:
+	device_remove_file(led_cdev->dev, &dev_attr_pattern);
+err_out:
+	dev_err(led_cdev->dev, "unable to register pattern trigger\n");
+	led_cdev->trigger_data = NULL;
+	kfree(tdata);
+}
+
+static void pattern_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
+
+	if (led_cdev->activated) {
+		del_timer_sync(&pattern_data->timer);
+		device_remove_file(led_cdev->dev, &dev_attr_pattern);
+		device_remove_file(led_cdev->dev, &dev_attr_delay_unit);
+		led_cdev->trigger_data = NULL;
+		led_cdev->activated = false;
+		kfree(pattern_data->pattern);
+		kfree(pattern_data);
+	}
+	__led_set_brightness(led_cdev, LED_OFF);
+}
+
+static struct led_trigger pattern_trigger = {
+	.name     = "pattern",
+	.activate = pattern_trig_activate,
+	.deactivate = pattern_trig_deactivate,
+};
+
+static int __init pattern_trig_init(void)
+{
+	return led_trigger_register(&pattern_trigger);
+}
+
+static void __exit pattern_trig_exit(void)
+{
+	led_trigger_unregister(&pattern_trigger);
+}
+
+module_init(pattern_trig_init);
+module_exit(pattern_trig_exit);
+
+MODULE_AUTHOR("Joe Xue <lgxue@hotmail.com");
+MODULE_DESCRIPTION("Pattern LED trigger");
+MODULE_LICENSE("GPL");
-- 
1.8.1.2

RE: [PATCH v3 1/1] leds: Add LED pattern trigger

[Joe Xue]

> From: Joe Xue <lgxue@hotmail.com>
>
> The LED pattern trigger allows LEDs blink in user defined pattern.
>
> v2: Change the pattern memory from fixed static to malloc
> Change the timer schedule way to save cpu time
> Add the mutex to protect the pattern operation
>
> v3: Tiny fix, add the missed parameter in example of document
>
> new file: Documentation/leds/ledtrig-pattern.txt
> modified: drivers/leds/trigger/Kconfig
> modified: drivers/leds/trigger/Makefile
> new file: drivers/leds/trigger/ledtrig-pattern.c
>
> Signed-off-by: Joe Xue <lgxue@hotmail.com>
> ---

Forgot to add suggested-by line:

Suggested-by: Pavel Machek <pavel@ucw.cz>

I would like to give too much credit to this buddy.

Joe 		 	   		  

Re: n900 led compiler (was Re: [PATCH] Add LED pattern trigger)

[Linus Walleij]

On Sun, Jan 5, 2014 at 11:23 PM, Pavel Machek <pavel@ucw.cz> wrote:

> I implemented compiler for it (should we put it into tools/ somewhere?)

We have a precedent for putting firmware compilers into the kernel
tree:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/aic7xxx/aicasm

But that one used FLEX. And I think the assumption is that then
you store the firmware source with the driver, and the sysfs (or similar)
interface would just load and trigger one of the pre-defined firmware
programs, not have it be sent in from userspace.

Yours,
Linus Walleij