LinuxLists.cc - [PATCH v2 1/3] leds: add oneshot blink functions

[permalink] [raw]

Subject: [PATCH 2/3] ledtrig-ide-disk: use generic one-shot blink api

Convert ledtrig-ide-disk code to use the generic API for one-shot LED
blinking.

This patch changes slightly the behaviour of the trigger, as while the
original version kept the LED on under heavy activity, the new one keeps
a constant on-off blink at 1 / (2 * BLINK_DELAY) Hz.

Signed-off-by: Fabio Baltieri <[email protected]>
Cc: Bryan Wu <[email protected]>
---
drivers/leds/ledtrig-ide-disk.c | 23 ++++-------------------
1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/leds/ledtrig-ide-disk.c b/drivers/leds/ledtrig-ide-disk.c
index ec099fc..0ff16fb 100644
--- a/drivers/leds/ledtrig-ide-disk.c
+++ b/drivers/leds/ledtrig-ide-disk.c
@@ -18,33 +18,18 @@
#include <linux/timer.h>
#include <linux/leds.h>

-static void ledtrig_ide_timerfunc(unsigned long data);
+#define BLINK_DELAY 30

DEFINE_LED_TRIGGER(ledtrig_ide);
-static DEFINE_TIMER(ledtrig_ide_timer, ledtrig_ide_timerfunc, 0, 0);
-static int ide_activity;
-static int ide_lastactivity;
+static unsigned long ide_blink_delay = BLINK_DELAY;

void ledtrig_ide_activity(void)
{
- ide_activity++;
- if (!timer_pending(&ledtrig_ide_timer))
- mod_timer(&ledtrig_ide_timer, jiffies + msecs_to_jiffies(10));
+ led_trigger_blink_oneshot(ledtrig_ide,
+ &ide_blink_delay, &ide_blink_delay, 0);
}
EXPORT_SYMBOL(ledtrig_ide_activity);

-static void ledtrig_ide_timerfunc(unsigned long data)
-{
- if (ide_lastactivity != ide_activity) {
- ide_lastactivity = ide_activity;
- /* INT_MAX will set each LED to its maximum brightness */
- led_trigger_event(ledtrig_ide, INT_MAX);
- mod_timer(&ledtrig_ide_timer, jiffies + msecs_to_jiffies(10));
- } else {
- led_trigger_event(ledtrig_ide, LED_OFF);
- }
-}
-
static int __init ledtrig_ide_init(void)
{
led_trigger_register_simple("ide-disk", &ledtrig_ide);
--
1.7.10.2.520.g6a4a482.dirty

2012-05-26 23:19:07

[permalink] [raw]

Subject: [PATCH 3/3] led: add oneshot trigger

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

Signed-off-by: Fabio Baltieri <[email protected]>
Cc: Bryan Wu <[email protected]>
Cc: Shuah Khan <[email protected]>
---

That's (a cleaned up version of) what I used to test the first patch. Should
we also consider this one for integration? Shuah Khan was working on
something similar, I'll add him in CC.

Fabio

drivers/leds/Kconfig | 9 ++
drivers/leds/Makefile | 1 +
drivers/leds/ledtrig-oneshot.c | 190 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 200 insertions(+)
create mode 100644 drivers/leds/ledtrig-oneshot.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index ff4b8cf..17c3cf2 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -422,6 +422,15 @@ config LEDS_TRIGGER_TIMER

If unsure, say Y.

+config LEDS_TRIGGER_ONESHOT
+ tristate "One-Shot Trigger"
+ depends on LEDS_TRIGGERS
+ help
+ This allows LEDs to blink in one-shot pulses with parameters
+ controlled via sysfs.
+
+ 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 890481c..ec8ff79 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -51,6 +51,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..7d9a052
--- /dev/null
+++ b/drivers/leds/ledtrig-oneshot.c
@@ -0,0 +1,190 @@
+/*
+ * One-Shot LED Trigger
+ *
+ * Copyright 2012, Fabio Baltieri <[email protected]>
+ *
+ * Based on ledtrig-timer.c by Richard Purdie <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#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>
+
+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 = -EINVAL;
+
+ 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 = -EINVAL;
+
+ 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 = -EINVAL;
+
+ 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;
+
+ 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 (oneshot_data) {
+ 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(led_cdev->trigger_data);
+ }
+
+ /* 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 <[email protected]>");
+MODULE_DESCRIPTION("One-Shot LED trigger");
+MODULE_LICENSE("GPL");
--
1.7.10.2.520.g6a4a482.dirty

2012-05-27 16:09:54

[permalink] [raw]

Subject: Re: [PATCH 3/3] led: add oneshot trigger

On Sun, 2012-05-27 at 01:19 +0200, Fabio Baltieri wrote:
> Add oneshot trigger to blink a led with configurable parameters via
> sysfs.
>
> Signed-off-by: Fabio Baltieri <[email protected]>
> Cc: Bryan Wu <[email protected]>
> Cc: Shuah Khan <[email protected]>
> ---
>
> That's (a cleaned up version of) what I used to test the first patch. Should
> we also consider this one for integration? Shuah Khan was working on
> something similar, I'll add him in CC.

Fabio,

Thanks for including me in the review. If you haven't already looked at
the the LEDS_TRIGGER_TRANSIENT I implemented, please take a look. It is
in linux-next and is queued up for 3.5-rc1 inclusion. It is also a one
shot timer, howver it holds the LED in a transient state (could be on or
off depending on its permanent state) for a specified period of time and
then goes back to the original state. Your use-case is different in that
the trigger you are implementing it holds it in blink state. These two
are distinct use-cases that address different needs and we will need
both.

However, the in the interest of reducing confusion, I would recommend
changing the name of the trigger you are adding to something that
reflects that it holds the blink state. You could call it
LED_TRIGGER_PULSEONCE or LED_TRIGGER_BLINKONCE.

Also could you please outline the use-cases. Might be a good idea to add
a documentation file for this new trigger.

Did you look at LEDS_TRIGGER_TIMER to see if the name-space you are
using conflicts with that triggers name-space. I think it does unless I
am missing something.

Also you probably need to base your patch on linux-next so you can pick
up one other infrastructure change I made to add a new activated flag to
led_classdev structure. This new flag is used to keep track of trigger
activated state and free memory and do cleanup.

More comments in-line.

>
> Fabio
>
> drivers/leds/Kconfig | 9 ++
> drivers/leds/Makefile | 1 +
> drivers/leds/ledtrig-oneshot.c | 190 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 200 insertions(+)
> create mode 100644 drivers/leds/ledtrig-oneshot.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index ff4b8cf..17c3cf2 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -422,6 +422,15 @@ config LEDS_TRIGGER_TIMER
>
> If unsure, say Y.
>
> +config LEDS_TRIGGER_ONESHOT
> + tristate "One-Shot Trigger"
> + depends on LEDS_TRIGGERS
> + help
> + This allows LEDs to blink in one-shot pulses with parameters
> + controlled via sysfs.
> +
> + 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 890481c..ec8ff79 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -51,6 +51,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..7d9a052
> --- /dev/null
> +++ b/drivers/leds/ledtrig-oneshot.c
> @@ -0,0 +1,190 @@
> +/*
> + * One-Shot LED Trigger
> + *
> + * Copyright 2012, Fabio Baltieri <[email protected]>
> + *
> + * Based on ledtrig-timer.c by Richard Purdie <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#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>
> +
> +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 = -EINVAL;
This initialization is not necessary.
> +
> + 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 = -EINVAL;
This initialization is not necessary.
> +
> + 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 = -EINVAL;

This initialization is not necessary.
> +
> + 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);

Doesn't this conflict with LEDS_TRIGGER_TIMER namespace

> +static DEVICE_ATTR(delay_off, 0644, led_delay_off_show, led_delay_off_store);

Doesn't this conflict with LEDS_TRIGGER_TIMER namespace
> +static DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);

This probably conflict with
> +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;
> +

This is where you can set activated flag to true and check the flag from
your deactivate routine. Please check linux-next leds code. I also
updated all led triggers to use this new flag.

> + 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 (oneshot_data) {

You can check activated flag instead of onshot_data like the rest of the
led triggers.

> + 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(led_cdev->trigger_data);
> + }
> +
> + /* 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 <[email protected]>");
> +MODULE_DESCRIPTION("One-Shot LED trigger");
> +MODULE_LICENSE("GPL");

2012-05-27 16:13:50

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] leds: add oneshot blink functions

On Sun, 2012-05-27 at 01:19 +0200, Fabio Baltieri wrote:
> Add two new functions, led_blink_set_oneshot and
> led_trigger_blink_oneshot, to be used by triggers for one-shot blink of
> led devices.
>
> This is implemented extending the existing software-blink code, and uses
> the same timer and handler function.
>
> The behavior of the code is to do a blink-on, blink-off sequence when
> the function is called, ignoring other calls until the sequence is
> completed so that the leds keep blinking at constant rate if the
> functions are called repeatedly.
>
> This is meant to be used by drivers which needs to trigger on sporadic
> event, but doesn't have clear busy/idle trigger points.
>
> After the blink sequence the led remains off. This behavior can be
> inverted setting the "invert" argument, which blink the led off, than on
> and leave the led on after the sequence.
>
> Signed-off-by: Fabio Baltieri <[email protected]>
> Cc: Bryan Wu <[email protected]>
> ---
>
> Same patch as v1 but rebased on Bryan's linux-leds tree.

Fabio/Bryan,

Please take a look at linux-next. I am concerned that this work
conflicts with LEDS_TRIGGER_TIMER implementation. Anyways using
linux-next is more appropriate here, as there were a few patches that
went in while Andrew Morton was acting as a maintainer for leds
sub-system.

-- Shuah
>
> More info in original thread:
> http://marc.info/?l=linux-kernel&m=133763247601381&w=2
>
> Tested on an x86 system with a custom usb-led device (http://wp.me/p1M1k7-fZ
> for the curious).
>
> Fabio
>
> drivers/leds/led-class.c | 19 +++++++++++++++++
> drivers/leds/led-core.c | 50 ++++++++++++++++++++++++++++++++++-----------
> drivers/leds/led-triggers.c | 30 +++++++++++++++++++++++----
> include/linux/leds.h | 25 +++++++++++++++++++++++
> 4 files changed, 108 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 5bff843..42d9359 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -91,6 +91,11 @@ static void led_timer_function(unsigned long data)
> return;
> }
>
> + if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) {
> + led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> + return;
> + }
> +
> brightness = led_get_brightness(led_cdev);
> if (!brightness) {
> /* Time to switch the LED on. */
> @@ -107,6 +112,20 @@ static void led_timer_function(unsigned long data)
>
> led_set_brightness(led_cdev, brightness);
>
> + /* Return in next iteration if led is in one-shot mode and we are in
> + * the final blink state so that the led is toggled each delay_on +
> + * delay_off milliseconds in worst case.
> + */
> + if (led_cdev->flags & LED_BLINK_ONESHOT) {
> + if (led_cdev->flags & LED_BLINK_INVERT) {
> + if (brightness)
> + led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
> + } else {
> + if (!brightness)
> + led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
> + }
> + }
> +
> mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
> }
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index d686004..579eb78 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -27,7 +27,6 @@ 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;
> }
> @@ -44,11 +43,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;
>
> - if (led_get_trigger_data(led_cdev) &&
> - delay_on == led_cdev->blink_delay_on &&
> - delay_off == led_cdev->blink_delay_off)
> - return;
> -
> led_stop_software_blink(led_cdev);
>
> led_cdev->blink_delay_on = delay_on;
> @@ -68,13 +62,12 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
> }
>
>
> -void led_blink_set(struct led_classdev *led_cdev,
> - unsigned long *delay_on,
> - unsigned long *delay_off)
> +void led_blink_setup(struct led_classdev *led_cdev,
> + unsigned long *delay_on,
> + unsigned long *delay_off)
> {
> - del_timer_sync(&led_cdev->blink_timer);
> -
> - if (led_cdev->blink_set &&
> + if (!(led_cdev->flags & LED_BLINK_ONESHOT) &&
> + led_cdev->blink_set &&
> !led_cdev->blink_set(led_cdev, delay_on, delay_off))
> return;
>
> @@ -84,8 +77,41 @@ void led_blink_set(struct led_classdev *led_cdev,
>
> led_set_software_blink(led_cdev, *delay_on, *delay_off);
> }
> +
> +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_cdev->flags &= ~LED_BLINK_ONESHOT;
> + led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> +
> + led_blink_setup(led_cdev, delay_on, delay_off);
> +}
> EXPORT_SYMBOL(led_blink_set);
>
> +void led_blink_set_oneshot(struct led_classdev *led_cdev,
> + unsigned long *delay_on,
> + unsigned long *delay_off,
> + int invert)
> +{
> + if ((led_cdev->flags & LED_BLINK_ONESHOT) &&
> + timer_pending(&led_cdev->blink_timer))
> + return;
> +
> + led_cdev->flags |= LED_BLINK_ONESHOT;
> + led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> +
> + if (invert)
> + led_cdev->flags |= LED_BLINK_INVERT;
> + else
> + led_cdev->flags &= ~LED_BLINK_INVERT;
> +
> + led_blink_setup(led_cdev, delay_on, delay_off);
> +}
> +EXPORT_SYMBOL(led_blink_set_oneshot);
> +
> void led_brightness_set(struct led_classdev *led_cdev,
> enum led_brightness brightness)
> {
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index b449ed8..fa0b9be 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -230,9 +230,11 @@ void led_trigger_event(struct led_trigger *trig,
> }
> EXPORT_SYMBOL_GPL(led_trigger_event);
>
> -void led_trigger_blink(struct led_trigger *trig,
> - unsigned long *delay_on,
> - unsigned long *delay_off)
> +void led_trigger_blink_setup(struct led_trigger *trig,
> + unsigned long *delay_on,
> + unsigned long *delay_off,
> + int oneshot,
> + int invert)
> {
> struct list_head *entry;
>
> @@ -244,12 +246,32 @@ void led_trigger_blink(struct led_trigger *trig,
> struct led_classdev *led_cdev;
>
> led_cdev = list_entry(entry, struct led_classdev, trig_list);
> - led_blink_set(led_cdev, delay_on, delay_off);
> + if (oneshot)
> + led_blink_set_oneshot(led_cdev, delay_on, delay_off,
> + invert);
> + else
> + led_blink_set(led_cdev, delay_on, delay_off);
> }
> read_unlock(&trig->leddev_list_lock);
> }
> +
> +void led_trigger_blink(struct led_trigger *trig,
> + unsigned long *delay_on,
> + unsigned long *delay_off)
> +{
> + led_trigger_blink_setup(trig, delay_on, delay_off, 0, 0);
> +}
> EXPORT_SYMBOL_GPL(led_trigger_blink);
>
> +void led_trigger_blink_oneshot(struct led_trigger *trig,
> + unsigned long *delay_on,
> + unsigned long *delay_off,
> + int invert)
> +{
> + led_trigger_blink_setup(trig, delay_on, delay_off, 1, invert);
> +}
> +EXPORT_SYMBOL_GPL(led_trigger_blink_oneshot);
> +
> void led_trigger_register_simple(const char *name, struct led_trigger **tp)
> {
> struct led_trigger *trig;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 5884def..f252438 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -38,6 +38,9 @@ struct led_classdev {
> #define LED_SUSPENDED (1 << 0)
> /* Upper 16 bits reflect control information */
> #define LED_CORE_SUSPENDRESUME (1 << 16)
> +#define LED_BLINK_ONESHOT (1 << 17)
> +#define LED_BLINK_ONESHOT_STOP (1 << 18)
> +#define LED_BLINK_INVERT (1 << 19)
>
> /* Set LED brightness level */
> /* Must not sleep, use a workqueue if needed */
> @@ -101,6 +104,24 @@ extern void led_blink_set(struct led_classdev *led_cdev,
> unsigned long *delay_on,
> unsigned long *delay_off);
> /**
> + * led_blink_set_oneshot - do a oneshot software blink
> + * @led_cdev: the LED to start blinking
> + * @delay_on: the time it should be on (in ms)
> + * @delay_off: the time it should ble off (in ms)
> + * @invert: blink off, then on, leaving the led on
> + *
> + * This function makes the LED blink one time for delay_on +
> + * delay_off time, ignoring the request if another one-shot
> + * blink is already in progress.
> + *
> + * If invert is set, led blinks for delay_off first, then for
> + * delay_on and leave the led on after the on-off cycle.
> + */
> +extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
> + unsigned long *delay_on,
> + unsigned long *delay_off,
> + int invert);
> +/**
> * led_brightness_set - set LED brightness
> * @led_cdev: the LED to set
> * @brightness: the brightness to set it to
> @@ -148,6 +169,10 @@ extern void led_trigger_event(struct led_trigger *trigger,
> extern void led_trigger_blink(struct led_trigger *trigger,
> unsigned long *delay_on,
> unsigned long *delay_off);
> +extern void led_trigger_blink_oneshot(struct led_trigger *trigger,
> + unsigned long *delay_on,
> + unsigned long *delay_off,
> + int invert);
>
> #else
>

2012-05-31 21:00:06

[permalink] [raw]

Subject: Re: [PATCH 3/3] led: add oneshot trigger

Hello Shuah,

On Sun, May 27, 2012 at 10:09:49AM -0600, Shuah Khan wrote:
> On Sun, 2012-05-27 at 01:19 +0200, Fabio Baltieri wrote:
> > Add oneshot trigger to blink a led with configurable parameters via
> > sysfs.
> >
> > Signed-off-by: Fabio Baltieri <[email protected]>
> > Cc: Bryan Wu <[email protected]>
> > Cc: Shuah Khan <[email protected]>
> > ---
> >
> > That's (a cleaned up version of) what I used to test the first patch. Should
> > we also consider this one for integration? Shuah Khan was working on
> > something similar, I'll add him in CC.
>
> Fabio,
>
> Thanks for including me in the review. If you haven't already looked at
> the the LEDS_TRIGGER_TRANSIENT I implemented, please take a look. It is
> in linux-next and is queued up for 3.5-rc1 inclusion. It is also a one
> shot timer, howver it holds the LED in a transient state (could be on or
> off depending on its permanent state) for a specified period of time and
> then goes back to the original state. Your use-case is different in that
> the trigger you are implementing it holds it in blink state. These two
> are distinct use-cases that address different needs and we will need
> both.
>
> However, the in the interest of reducing confusion, I would recommend
> changing the name of the trigger you are adding to something that
> reflects that it holds the blink state. You could call it
> LED_TRIGGER_PULSEONCE or LED_TRIGGER_BLINKONCE.
>
> Also could you please outline the use-cases. Might be a good idea to add
> a documentation file for this new trigger.
>
> Did you look at LEDS_TRIGGER_TIMER to see if the name-space you are
> using conflicts with that triggers name-space. I think it does unless I
> am missing something.
>
> Also you probably need to base your patch on linux-next so you can pick
> up one other infrastructure change I made to add a new activated flag to
> led_classdev structure. This new flag is used to keep track of trigger
> activated state and free memory and do cleanup.

Ok, anyway as your patch has been mainlined and this (totally?) overlaps
with it I think it's ok to just use this to test the API of first patch
of this set.

Fabio

> More comments in-line.
>
>
> >
> > Fabio
> >
> > drivers/leds/Kconfig | 9 ++
> > drivers/leds/Makefile | 1 +
> > drivers/leds/ledtrig-oneshot.c | 190 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 200 insertions(+)
> > create mode 100644 drivers/leds/ledtrig-oneshot.c
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index ff4b8cf..17c3cf2 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -422,6 +422,15 @@ config LEDS_TRIGGER_TIMER
> >
> > If unsure, say Y.
> >
> > +config LEDS_TRIGGER_ONESHOT
> > + tristate "One-Shot Trigger"
> > + depends on LEDS_TRIGGERS
> > + help
> > + This allows LEDs to blink in one-shot pulses with parameters
> > + controlled via sysfs.
> > +
> > + 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 890481c..ec8ff79 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -51,6 +51,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..7d9a052
> > --- /dev/null
> > +++ b/drivers/leds/ledtrig-oneshot.c
> > @@ -0,0 +1,190 @@
> > +/*
> > + * One-Shot LED Trigger
> > + *
> > + * Copyright 2012, Fabio Baltieri <[email protected]>
> > + *
> > + * Based on ledtrig-timer.c by Richard Purdie <[email protected]>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > +
> > +#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>
> > +
> > +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 = -EINVAL;
> This initialization is not necessary.
> > +
> > + 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 = -EINVAL;
> This initialization is not necessary.
> > +
> > + 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 = -EINVAL;
>
> This initialization is not necessary.
> > +
> > + 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);
>
> Doesn't this conflict with LEDS_TRIGGER_TIMER namespace
>
> > +static DEVICE_ATTR(delay_off, 0644, led_delay_off_show, led_delay_off_store);
>
> Doesn't this conflict with LEDS_TRIGGER_TIMER namespace
> > +static DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);
>
> This probably conflict with
> > +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;
> > +
>
> This is where you can set activated flag to true and check the flag from
> your deactivate routine. Please check linux-next leds code. I also
> updated all led triggers to use this new flag.
>
>
> > + 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 (oneshot_data) {
>
> You can check activated flag instead of onshot_data like the rest of the
> led triggers.
>
> > + 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(led_cdev->trigger_data);
> > + }
> > +
> > + /* 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 <[email protected]>");
> > +MODULE_DESCRIPTION("One-Shot LED trigger");
> > +MODULE_LICENSE("GPL");
>
>

2012-05-31 21:06:53

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] leds: add oneshot blink functions

Hi Shuah,

On Sun, May 27, 2012 at 10:13:46AM -0600, Shuah Khan wrote:
> On Sun, 2012-05-27 at 01:19 +0200, Fabio Baltieri wrote:
> > Add two new functions, led_blink_set_oneshot and
> > led_trigger_blink_oneshot, to be used by triggers for one-shot blink of
> > led devices.
> >
> > This is implemented extending the existing software-blink code, and uses
> > the same timer and handler function.
> >
> > The behavior of the code is to do a blink-on, blink-off sequence when
> > the function is called, ignoring other calls until the sequence is
> > completed so that the leds keep blinking at constant rate if the
> > functions are called repeatedly.
> >
> > This is meant to be used by drivers which needs to trigger on sporadic
> > event, but doesn't have clear busy/idle trigger points.
> >
> > After the blink sequence the led remains off. This behavior can be
> > inverted setting the "invert" argument, which blink the led off, than on
> > and leave the led on after the sequence.
> >
> > Signed-off-by: Fabio Baltieri <[email protected]>
> > Cc: Bryan Wu <[email protected]>
> > ---
> >
> > Same patch as v1 but rebased on Bryan's linux-leds tree.
>
> Fabio/Bryan,
>
> Please take a look at linux-next. I am concerned that this work
> conflicts with LEDS_TRIGGER_TIMER implementation. Anyways using

There should be no compatibity issues with current timer-trigger calls,
can you be more specific?

> linux-next is more appropriate here, as there were a few patches that
> went in while Andrew Morton was acting as a maintainer for leds
> sub-system.

Ok, anyway v1 of the patch should apply cleanly on current mainline
until linus pulls from Bryan.

Fabio

> -- Shuah
> >
> > More info in original thread:
> > http://marc.info/?l=linux-kernel&m=133763247601381&w=2
> >
> > Tested on an x86 system with a custom usb-led device (http://wp.me/p1M1k7-fZ
> > for the curious).
> >
> > Fabio
> >
> > drivers/leds/led-class.c | 19 +++++++++++++++++
> > drivers/leds/led-core.c | 50 ++++++++++++++++++++++++++++++++++-----------
> > drivers/leds/led-triggers.c | 30 +++++++++++++++++++++++----
> > include/linux/leds.h | 25 +++++++++++++++++++++++
> > 4 files changed, 108 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index 5bff843..42d9359 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -91,6 +91,11 @@ static void led_timer_function(unsigned long data)
> > return;
> > }
> >
> > + if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) {
> > + led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> > + return;
> > + }
> > +
> > brightness = led_get_brightness(led_cdev);
> > if (!brightness) {
> > /* Time to switch the LED on. */
> > @@ -107,6 +112,20 @@ static void led_timer_function(unsigned long data)
> >
> > led_set_brightness(led_cdev, brightness);
> >
> > + /* Return in next iteration if led is in one-shot mode and we are in
> > + * the final blink state so that the led is toggled each delay_on +
> > + * delay_off milliseconds in worst case.
> > + */
> > + if (led_cdev->flags & LED_BLINK_ONESHOT) {
> > + if (led_cdev->flags & LED_BLINK_INVERT) {
> > + if (brightness)
> > + led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
> > + } else {
> > + if (!brightness)
> > + led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
> > + }
> > + }
> > +
> > mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
> > }
> >
> > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> > index d686004..579eb78 100644
> > --- a/drivers/leds/led-core.c
> > +++ b/drivers/leds/led-core.c
> > @@ -27,7 +27,6 @@ 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;
> > }
> > @@ -44,11 +43,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;
> >
> > - if (led_get_trigger_data(led_cdev) &&
> > - delay_on == led_cdev->blink_delay_on &&
> > - delay_off == led_cdev->blink_delay_off)
> > - return;
> > -
> > led_stop_software_blink(led_cdev);
> >
> > led_cdev->blink_delay_on = delay_on;
> > @@ -68,13 +62,12 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
> > }
> >
> >
> > -void led_blink_set(struct led_classdev *led_cdev,
> > - unsigned long *delay_on,
> > - unsigned long *delay_off)
> > +void led_blink_setup(struct led_classdev *led_cdev,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off)
> > {
> > - del_timer_sync(&led_cdev->blink_timer);
> > -
> > - if (led_cdev->blink_set &&
> > + if (!(led_cdev->flags & LED_BLINK_ONESHOT) &&
> > + led_cdev->blink_set &&
> > !led_cdev->blink_set(led_cdev, delay_on, delay_off))
> > return;
> >
> > @@ -84,8 +77,41 @@ void led_blink_set(struct led_classdev *led_cdev,
> >
> > led_set_software_blink(led_cdev, *delay_on, *delay_off);
> > }
> > +
> > +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_cdev->flags &= ~LED_BLINK_ONESHOT;
> > + led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> > +
> > + led_blink_setup(led_cdev, delay_on, delay_off);
> > +}
> > EXPORT_SYMBOL(led_blink_set);
> >
> > +void led_blink_set_oneshot(struct led_classdev *led_cdev,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off,
> > + int invert)
> > +{
> > + if ((led_cdev->flags & LED_BLINK_ONESHOT) &&
> > + timer_pending(&led_cdev->blink_timer))
> > + return;
> > +
> > + led_cdev->flags |= LED_BLINK_ONESHOT;
> > + led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> > +
> > + if (invert)
> > + led_cdev->flags |= LED_BLINK_INVERT;
> > + else
> > + led_cdev->flags &= ~LED_BLINK_INVERT;
> > +
> > + led_blink_setup(led_cdev, delay_on, delay_off);
> > +}
> > +EXPORT_SYMBOL(led_blink_set_oneshot);
> > +
> > void led_brightness_set(struct led_classdev *led_cdev,
> > enum led_brightness brightness)
> > {
> > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > index b449ed8..fa0b9be 100644
> > --- a/drivers/leds/led-triggers.c
> > +++ b/drivers/leds/led-triggers.c
> > @@ -230,9 +230,11 @@ void led_trigger_event(struct led_trigger *trig,
> > }
> > EXPORT_SYMBOL_GPL(led_trigger_event);
> >
> > -void led_trigger_blink(struct led_trigger *trig,
> > - unsigned long *delay_on,
> > - unsigned long *delay_off)
> > +void led_trigger_blink_setup(struct led_trigger *trig,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off,
> > + int oneshot,
> > + int invert)
> > {
> > struct list_head *entry;
> >
> > @@ -244,12 +246,32 @@ void led_trigger_blink(struct led_trigger *trig,
> > struct led_classdev *led_cdev;
> >
> > led_cdev = list_entry(entry, struct led_classdev, trig_list);
> > - led_blink_set(led_cdev, delay_on, delay_off);
> > + if (oneshot)
> > + led_blink_set_oneshot(led_cdev, delay_on, delay_off,
> > + invert);
> > + else
> > + led_blink_set(led_cdev, delay_on, delay_off);
> > }
> > read_unlock(&trig->leddev_list_lock);
> > }
> > +
> > +void led_trigger_blink(struct led_trigger *trig,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off)
> > +{
> > + led_trigger_blink_setup(trig, delay_on, delay_off, 0, 0);
> > +}
> > EXPORT_SYMBOL_GPL(led_trigger_blink);
> >
> > +void led_trigger_blink_oneshot(struct led_trigger *trig,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off,
> > + int invert)
> > +{
> > + led_trigger_blink_setup(trig, delay_on, delay_off, 1, invert);
> > +}
> > +EXPORT_SYMBOL_GPL(led_trigger_blink_oneshot);
> > +
> > void led_trigger_register_simple(const char *name, struct led_trigger **tp)
> > {
> > struct led_trigger *trig;
> > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > index 5884def..f252438 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -38,6 +38,9 @@ struct led_classdev {
> > #define LED_SUSPENDED (1 << 0)
> > /* Upper 16 bits reflect control information */
> > #define LED_CORE_SUSPENDRESUME (1 << 16)
> > +#define LED_BLINK_ONESHOT (1 << 17)
> > +#define LED_BLINK_ONESHOT_STOP (1 << 18)
> > +#define LED_BLINK_INVERT (1 << 19)
> >
> > /* Set LED brightness level */
> > /* Must not sleep, use a workqueue if needed */
> > @@ -101,6 +104,24 @@ extern void led_blink_set(struct led_classdev *led_cdev,
> > unsigned long *delay_on,
> > unsigned long *delay_off);
> > /**
> > + * led_blink_set_oneshot - do a oneshot software blink
> > + * @led_cdev: the LED to start blinking
> > + * @delay_on: the time it should be on (in ms)
> > + * @delay_off: the time it should ble off (in ms)
> > + * @invert: blink off, then on, leaving the led on
> > + *
> > + * This function makes the LED blink one time for delay_on +
> > + * delay_off time, ignoring the request if another one-shot
> > + * blink is already in progress.
> > + *
> > + * If invert is set, led blinks for delay_off first, then for
> > + * delay_on and leave the led on after the on-off cycle.
> > + */
> > +extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off,
> > + int invert);
> > +/**
> > * led_brightness_set - set LED brightness
> > * @led_cdev: the LED to set
> > * @brightness: the brightness to set it to
> > @@ -148,6 +169,10 @@ extern void led_trigger_event(struct led_trigger *trigger,
> > extern void led_trigger_blink(struct led_trigger *trigger,
> > unsigned long *delay_on,
> > unsigned long *delay_off);
> > +extern void led_trigger_blink_oneshot(struct led_trigger *trigger,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off,
> > + int invert);
> >
> > #else
> >
>
>

2012-06-01 15:39:44

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] leds: add oneshot blink functions

>
> There should be no compatibity issues with current timer-trigger calls,
> can you be more specific?

My initial thought was that maybe you could create new namespace for
this trigger instead of using delay_on and delay_off. However, after
looking closely at the patch set you are proposing, since you are using
delay_on and delay_off in led_cdev, I think you are ok.

Please make sure there is no regression in the ledtrig-timer use-cases,
with the changes made to led-class and led-core.

-- Shuah

2012-06-04 21:36:06

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] leds: add oneshot blink functions

On Fri, Jun 01, 2012 at 09:39:40AM -0600, Shuah Khan wrote:
>
> >
> > There should be no compatibity issues with current timer-trigger calls,
> > can you be more specific?
>
> My initial thought was that maybe you could create new namespace for
> this trigger instead of using delay_on and delay_off. However, after
> looking closely at the patch set you are proposing, since you are using
> delay_on and delay_off in led_cdev, I think you are ok.

Ok, the patch should integrate pretty well with existing software-timer
code, and delay_{on,off} retains the same meaning.

> Please make sure there is no regression in the ledtrig-timer use-cases,
> with the changes made to led-class and led-core.

Of course, I've made many tests with mixes of led_trigger_blink() and
led_trigger_blink_oneshot() calls, and they seems to work samelessly.
Did you have any chance to test it?

If you think that the patch is good I'd like to consider that for
integration, so I can repost a modified version of my can-trigger
patch on the linux-can mailing list.

Fabio

2012-06-04 23:12:13

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] leds: add oneshot blink functions

>
> Of course, I've made many tests with mixes of led_trigger_blink() and
> led_trigger_blink_oneshot() calls, and they seems to work samelessly.
Great.
>
> If you think that the patch is good I'd like to consider that for
> integration, so I can repost a modified version of my can-trigger
> patch on the linux-can mailing list.

Sounds good to me.

-- Shuah

2012-06-05 03:05:45

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] leds: add oneshot blink functions

On Tue, Jun 5, 2012 at 7:12 AM, Shuah Khan <[email protected]> wrote:
>
>>
>> Of course, I've made many tests with mixes of led_trigger_blink() and
>> led_trigger_blink_oneshot() calls, and they seems to work samelessly.
> Great.
>>
>> If you think that the patch is good I'd like to consider that for
>> integration, so I can repost a modified version of my can-trigger
>> patch on the linux-can mailing list.
>
> Sounds good to me.
>

Hi Fabio,

I'm going to apply the patch to my for-next branch. and Shuah, can I
add your ack to this patch?

Thanks,
--
Bryan Wu <[email protected]>
Kernel Developer ? ?+86.186-168-78255 Mobile
Canonical Ltd. ? ? ?http://www.canonical.com
Ubuntu - Linux for human beings | http://www.ubuntu.com

2012-06-05 07:20:40

[permalink] [raw]

Subject: Re: [PATCH 2/3] ledtrig-ide-disk: use generic one-shot blink api

On Sun, May 27, 2012 at 7:19 AM, Fabio Baltieri
<[email protected]> wrote:
> Convert ledtrig-ide-disk code to use the generic API for one-shot LED
> blinking.
>
> This patch changes slightly the behaviour of the trigger, as while the
> original version kept the LED on under heavy activity, the new one keeps
> a constant on-off blink at 1 / (2 * BLINK_DELAY) Hz.
>

Applied to my for-next branch after removing 2 useless header files,
since with this patch we don't need them.

-Bryan

> Signed-off-by: Fabio Baltieri <[email protected]>
> Cc: Bryan Wu <[email protected]>
> ---
> ?drivers/leds/ledtrig-ide-disk.c | 23 ++++-------------------
> ?1 file changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/leds/ledtrig-ide-disk.c b/drivers/leds/ledtrig-ide-disk.c
> index ec099fc..0ff16fb 100644
> --- a/drivers/leds/ledtrig-ide-disk.c
> +++ b/drivers/leds/ledtrig-ide-disk.c
> @@ -18,33 +18,18 @@
> ?#include <linux/timer.h>
> ?#include <linux/leds.h>
>
> -static void ledtrig_ide_timerfunc(unsigned long data);
> +#define BLINK_DELAY 30
>
> ?DEFINE_LED_TRIGGER(ledtrig_ide);
> -static DEFINE_TIMER(ledtrig_ide_timer, ledtrig_ide_timerfunc, 0, 0);
> -static int ide_activity;
> -static int ide_lastactivity;
> +static unsigned long ide_blink_delay = BLINK_DELAY;
>
> ?void ledtrig_ide_activity(void)
> ?{
> - ? ? ? ide_activity++;
> - ? ? ? if (!timer_pending(&ledtrig_ide_timer))
> - ? ? ? ? ? ? ? mod_timer(&ledtrig_ide_timer, jiffies + msecs_to_jiffies(10));
> + ? ? ? led_trigger_blink_oneshot(ledtrig_ide,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &ide_blink_delay, &ide_blink_delay, 0);
> ?}
> ?EXPORT_SYMBOL(ledtrig_ide_activity);
>
> -static void ledtrig_ide_timerfunc(unsigned long data)
> -{
> - ? ? ? if (ide_lastactivity != ide_activity) {
> - ? ? ? ? ? ? ? ide_lastactivity = ide_activity;
> - ? ? ? ? ? ? ? /* INT_MAX will set each LED to its maximum brightness */
> - ? ? ? ? ? ? ? led_trigger_event(ledtrig_ide, INT_MAX);
> - ? ? ? ? ? ? ? mod_timer(&ledtrig_ide_timer, jiffies + msecs_to_jiffies(10));
> - ? ? ? } else {
> - ? ? ? ? ? ? ? led_trigger_event(ledtrig_ide, LED_OFF);
> - ? ? ? }
> -}
> -
> ?static int __init ledtrig_ide_init(void)
> ?{
> ? ? ? ?led_trigger_register_simple("ide-disk", &ledtrig_ide);
> --
> 1.7.10.2.520.g6a4a482.dirty
>

--
Bryan Wu <[email protected]>
Kernel Developer ? ?+86.186-168-78255 Mobile
Canonical Ltd. ? ? ?http://www.canonical.com
Ubuntu - Linux for human beings | http://www.ubuntu.com

2012-06-05 07:38:03

[permalink] [raw]

Subject: Re: [PATCH 3/3] led: add oneshot trigger

On Fri, Jun 1, 2012 at 5:01 AM, Fabio Baltieri <[email protected]> wrote:
> Hello Shuah,
>
> On Sun, May 27, 2012 at 10:09:49AM -0600, Shuah Khan wrote:
>> On Sun, 2012-05-27 at 01:19 +0200, Fabio Baltieri wrote:
>> > Add oneshot trigger to blink a led with configurable parameters via
>> > sysfs.
>> >
>> > Signed-off-by: Fabio Baltieri <[email protected]>
>> > Cc: Bryan Wu <[email protected]>
>> > Cc: Shuah Khan <[email protected]>
>> > ---
>> >
>> > That's (a cleaned up version of) what I used to test the first patch. Should
>> > we also consider this one for integration? ?Shuah Khan was working on
>> > something similar, I'll add him in CC.
>>
>> Fabio,
>>
>> Thanks for including me in the review. If you haven't already looked at
>> the the LEDS_TRIGGER_TRANSIENT I implemented, please take a look. It is
>> in linux-next and is queued up for 3.5-rc1 inclusion. It is also a one
>> shot timer, howver it holds the LED in a transient state (could be on or
>> off depending on its permanent state) for a specified period of time and
>> then goes back to the original state. Your use-case is different in that
>> the trigger you are implementing it holds it in blink state. These two
>> are distinct use-cases that address different needs and we will need
>> both.
>>
>> However, the in the interest of reducing confusion, I would recommend
>> changing the name of the trigger you are adding to something that
>> reflects that it holds the blink state. You could call it
>> LED_TRIGGER_PULSEONCE or LED_TRIGGER_BLINKONCE.
>>
>> Also could you please outline the use-cases. Might be a good idea to add
>> a documentation file for this new trigger.
>>
>> Did you look at LEDS_TRIGGER_TIMER to see if the name-space you are
>> using conflicts with that triggers name-space. I think it does unless I
>> am missing something.
>>
>> Also you probably need to base your patch on linux-next so you can pick
>> up one other infrastructure change I made to add a new activated flag to
>> led_classdev structure. This new flag is used to keep track of trigger
>> activated state and free memory and do cleanup.
>
> Ok, anyway as your patch has been mainlined and this (totally?) overlaps
> with it I think it's ok to just use this to test the API of first patch
> of this set.
>

Shuah's leds patches are all in mainline now. But I think this trigger
is still useful for an good example to use the new API.
And it is easy for user to use the oneshot trigger from here.

I've already merged 2 patches from you, so if you like, please follow
Shuah's advice to cleanup this trigger and resend.

Thanks,
-Bryan

>> More comments in-line.
>>
>>
>> >
>> > Fabio
>> >
>> > ?drivers/leds/Kconfig ? ? ? ? ? | ? 9 ++
>> > ?drivers/leds/Makefile ? ? ? ? ?| ? 1 +
>> > ?drivers/leds/ledtrig-oneshot.c | 190 +++++++++++++++++++++++++++++++++++++++++
>> > ?3 files changed, 200 insertions(+)
>> > ?create mode 100644 drivers/leds/ledtrig-oneshot.c
>> >
>> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> > index ff4b8cf..17c3cf2 100644
>> > --- a/drivers/leds/Kconfig
>> > +++ b/drivers/leds/Kconfig
>> > @@ -422,6 +422,15 @@ config LEDS_TRIGGER_TIMER
>> >
>> > ? ? ? If unsure, say Y.
>> >
>> > +config LEDS_TRIGGER_ONESHOT
>> > + ? tristate "One-Shot Trigger"
>> > + ? depends on LEDS_TRIGGERS
>> > + ? help
>> > + ? ? This allows LEDs to blink in one-shot pulses with parameters
>> > + ? ? controlled via sysfs.
>> > +
>> > + ? ? 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 890481c..ec8ff79 100644
>> > --- a/drivers/leds/Makefile
>> > +++ b/drivers/leds/Makefile
>> > @@ -51,6 +51,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..7d9a052
>> > --- /dev/null
>> > +++ b/drivers/leds/ledtrig-oneshot.c
>> > @@ -0,0 +1,190 @@
>> > +/*
>> > + * One-Shot LED Trigger
>> > + *
>> > + * Copyright 2012, Fabio Baltieri <[email protected]>
>> > + *
>> > + * Based on ledtrig-timer.c by Richard Purdie <[email protected]>
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2 as
>> > + * published by the Free Software Foundation.
>> > + *
>> > + */
>> > +
>> > +#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>
>> > +
>> > +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 = -EINVAL;
>> This initialization is not necessary.
>> > +
>> > + ? 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 = -EINVAL;
>> This initialization is not necessary.
>> > +
>> > + ? 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 = -EINVAL;
>>
>> This initialization is not necessary.
>> > +
>> > + ? 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);
>>
>> Doesn't this conflict with LEDS_TRIGGER_TIMER namespace
>>
>> > +static DEVICE_ATTR(delay_off, 0644, led_delay_off_show, led_delay_off_store);
>>
>> Doesn't this conflict with LEDS_TRIGGER_TIMER namespace
>> > +static DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);
>>
>> This probably conflict with
>> > +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;
>> > +
>>
>> This is where you can set activated flag to true and check the flag from
>> your deactivate routine. Please check linux-next leds code. I also
>> updated all led triggers to use this new flag.
>>
>>
>> > + ? 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 (oneshot_data) {
>>
>> You can check activated flag instead of onshot_data like the rest of the
>> led triggers.
>>
>> > + ? ? ? ? ? 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(led_cdev->trigger_data);
>> > + ? }
>> > +
>> > + ? /* 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 <[email protected]>");
>> > +MODULE_DESCRIPTION("One-Shot LED trigger");
>> > +MODULE_LICENSE("GPL");
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

--
Bryan Wu <[email protected]>
Kernel Developer ? ?+86.186-168-78255 Mobile
Canonical Ltd. ? ? ?http://www.canonical.com
Ubuntu - Linux for human beings | http://www.ubuntu.com

2012-06-05 14:59:42

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] leds: add oneshot blink functions

On Tue, 2012-06-05 at 11:05 +0800, Bryan Wu wrote:
> On Tue, Jun 5, 2012 at 7:12 AM, Shuah Khan <[email protected]> wrote:
> >
> >>
> >> Of course, I've made many tests with mixes of led_trigger_blink() and
> >> led_trigger_blink_oneshot() calls, and they seems to work samelessly.
> > Great.
> >>
> >> If you think that the patch is good I'd like to consider that for
> >> integration, so I can repost a modified version of my can-trigger
> >> patch on the linux-can mailing list.
> >
> > Sounds good to me.
> >
>
> Hi Fabio,
>
> I'm going to apply the patch to my for-next branch. and Shuah, can I
> add your ack to this patch?

Bryan,

Yes. You can add my ack to this patch.

Thanks,
-- Shuah

2012-06-05 20:53:30

[permalink] [raw]

Subject: Re: [PATCH 3/3] led: add oneshot trigger

Hi Bryan,

On Tue, Jun 05, 2012 at 03:37:39PM +0800, Bryan Wu wrote:
> >
> > Ok, anyway as your patch has been mainlined and this (totally?) overlaps
> > with it I think it's ok to just use this to test the API of first patch
> > of this set.
> >
>
> Shuah's leds patches are all in mainline now. But I think this trigger
> is still useful for an good example to use the new API.
> And it is easy for user to use the oneshot trigger from here.
>
> I've already merged 2 patches from you, so if you like, please follow
> Shuah's advice to cleanup this trigger and resend.

Ok, I found another small bug so I'm sending a v2 patchest on a separate
thread for clearness.

Fabio

>
>
> --
> Bryan Wu <[email protected]>
> Kernel Developer ? ?+86.186-168-78255 Mobile
> Canonical Ltd. ? ? ?http://www.canonical.com
> Ubuntu - Linux for human beings | http://www.ubuntu.com

2012-06-06 02:45:03