Re: [PATCHv2] LEDS-One-Shot-Timer-Trigger-implementation

From: Shuah Khan
Date: Fri Mar 30 2012 - 11:01:04 EST


Neil, Richard,

How about this patch? Does this good now? Anything else that
needs attention?

Thanks,
-- Shuah

On Wed, 2012-03-28 at 17:28 -0600, Shuah Khan wrote:
> Neil,
>
> I also added documentation file in this second version. Thanks again for
> brainstorming ideas and reviews. Leaving the subject line unchanged to
> avoid confusion, and the documentation and code are updated to change
> one-shot-timer to timer-no-default.
>
> >
> > A separate patch is certainly a good idea.
> > I would do the clean-up patches first, and then have the big change at the
> > end of the series. That way checkpatch won't complain about anything in your
> > change.
> > But you should do what you are most comfortable with.
>
> Decided to do the simple_strtoul() cleanup patch later - after this one
> goes through.
> >
> > >
> > > >
> > > > > +
> > > >
> > > > Do you need to led_trigger_unregister(&timer_led_trigger) if the registration
> > > > of one_shot_timer_led_trigger fails?
> > >
> > > Yes, considered that while I was changing this routine, and thought
> > > might be better to leave the registered timer alone when the second
> > > registration fails. I can go either way.
> >
> > The important point is that if timer_trig_init fails, then timer_trig_exit
> > will never be called.
> > So when timer_trig_init fails, it must ensure that it has un-done any bits
> > that it successfully did.
>
> Fixed.
>
> -- Shuah
>
> LED infrastructure lacks support for timer-no-default trigger and activation.
> The current support allows for setting two timers, one for specifying how
> long a state to be on, and the second for how long the state to be off. For
> example, delay_on value specifies the time period an LED should stay in on
> state, followed by a delay_off value that specifies how long the LED should
> stay in off state. The on and off cycle repeats until the trigger gets
> deactivated. There is no provision for one time activation to implement
> features that require an on or off state to be held just once and then stay
> in the original state forever.
>
> This feature will help implement vibrate functionality which requires one
> time activation of vibrate mode without a continuous vibrate on/off cycles.
>
> This patch implements the timer-no-default trigger support by enhancing the
> current led-class, led-core, and ledtrig-timer drivers to
>
> 1. Add support for forever timer case. forever tag can be written to delay_on
> or delay_off files. Internally forever is mapped to ULONG_MAX with no timer
> associated with it.
>
> 2. The led_blink_set() which takes two pointers to times one each for delay_on
> and delay_off has been extended so that a NULL instead of a pointer means
> "forever".
>
> 3. Add a new timer-no-default trigger to ledtrig-timer
>
>
> The above enhancements support the following use-cases:
>
> use-case 1:
> echo timer-no-default > /sys/class/leds/SOMELED/trigger
> echo forever > /sys/class/leds/SOMELED/delay_off
> echo 2000 > /sys/class/leds/SOMELED/delay_on
>
> When timer-no-default is activated in step1, unlike the timer trigger case,
> timer-no-default activate routine activates the trigger without starting
> any timers. The default 1 HZ delay_on and delay_off timers won't be started
> like in the case of timer trigger activation. Not starting timers ensures
> that the one time state isn't stuck if some error occurs before actual timer
> periods are specified. delay_on and delay_off files get created with 0
> values. Please note that it is important to set delay_off to forever prior
> to setting delay_on value. If the order is reversed, the LED will be turned
> on, with no timer set to turn it off.
>
> When delay_off value is specified in step 2, delay_off_store recognizes the
> special forever tag and records it and returns without starting any timer.
> Internally forever maps to ULONG_MAX. The led_blink_set() which takes
> two pointers to times one each for delay_on and delay_off has been extended so
> that a NULL instead of a pointer means "forever".
>
> When delay_on value is specified in step 3, a timer gets started for delay_on
> period, and delay_off stays at ULONG_MAX with no timer associated with it.
>
> use-case 2:
> echo timer-no-default > /sys/class/leds/SOMELED/trigger
> echo forever > /sys/class/leds/SOMELED/delay_on
> echo 2000 > /sys/class/leds/SOMELED/delay_off
>
> When timer-no-default is activated in step1, unlike the timer trigger case,
> timer-no-default activate routine activates the trigger without starting
> any timers. The default 1 HZ delay_on and delay_off timers won't be started
> like in the case of timer trigger activation. Not starting timers ensures
> that the one time state isn't stuck if some error occurs before actual timer
> periods are specified. delay_on and delay_off files get created with 0
> values. Please note that it is important to set delay_on to forever prior
> to setting delay_off value. If the order is reversed, the LED will be turned
> off, with no timer set to turn it back on.
>
> When delay_on value is specified in step 2, delay_on_store recognizes the
> special forever tag and records it and returns without starting any timer.
> Internally forever maps to ULONG_MAX.
>
> When delay_off value is specified in step 3, a timer gets started for delay_off
> period, and delay_on stays at ULONG_MAX with no timer associated with it.
>
> From f7e8a55fb9ffa10daad1bbb80f2e9e0d81ad359f Mon Sep 17 00:00:00 2001
> From: Shuah Khan <shuahkhan@xxxxxxxxx>
> Date: Wed, 28 Mar 2012 15:40:33 -0600
> Subject: [PATCH] LEDS-timer-no-default-implementation
>
>
> Signed-off-by: Shuah Khan <shuahkhan@xxxxxxxxx>
> ---
> .../leds/leds-timer_no_default_trigger.txt | 79 +++++++++++++++
> drivers/leds/led-class.c | 4 +-
> drivers/leds/led-core.c | 26 ++++-
> drivers/leds/leds.h | 2 +
> drivers/leds/ledtrig-timer.c | 104 ++++++++++++++------
> 5 files changed, 180 insertions(+), 35 deletions(-)
> create mode 100644 Documentation/leds/leds-timer_no_default_trigger.txt
>
> diff --git a/Documentation/leds/leds-timer_no_default_trigger.txt b/Documentation/leds/leds-timer_no_default_trigger.txt
> new file mode 100644
> index 0000000..ff3fdad
> --- /dev/null
> +++ b/Documentation/leds/leds-timer_no_default_trigger.txt
> @@ -0,0 +1,79 @@
> +
> +LED timer no default trigger feature (one shot timer)
> +=====================================================
> +
> +LED infrastructure lacks support for timer-no-default trigger and activation.
> +The current support allows for setting two timers, one for specifying how
> +long a state to be on, and the second for how long the state to be off. For
> +example, delay_on value specifies the time period an LED should stay in on
> +state, followed by a delay_off value that specifies how long the LED should
> +stay in off state. The on and off cycle repeats until the trigger gets
> +deactivated. There is no provision for one time activation to implement
> +features that require an on or off state to be held just once and then stay
> +in the original state forever.
> +
> +This feature will help implement vibrate functionality which requires one
> +time activation of vibrate mode without a continuous vibrate on/off cycles.
> +
> +This patch implements the timer-no-default trigger support by enhancing the
> +current led-class, led-core, and ledtrig-timer drivers to:
> +
> +- Add support for forever timer case. forever tag can be written to delay_on
> + or delay_off files. Internally forever is mapped to ULONG_MAX with no timer
> + associated with it.
> +
> +- The led_blink_set() which takes two pointers to times one each for delay_on
> + and delay_off has been extended so that a NULL instead of a pointer means
> + "forever".
> +
> +- Add a new timer-no-default trigger to ledtrig-timer
> +
> +The above enhancements support the following use-cases:
> +
> +use-case 1:
> +echo timer-no-default > /sys/class/leds/SOMELED/trigger
> +echo forever > /sys/class/leds/SOMELED/delay_off
> +echo 2000 > /sys/class/leds/SOMELED/delay_on
> +
> +When timer-no-default is activated in step1, unlike the timer trigger case,
> +timer-no-default activate routine activates the trigger without starting
> +any timers. The default 1 HZ delay_on and delay_off timers won't be started
> +like in the case of timer trigger activation. Not starting timers ensures
> +that the one time state isn't stuck if some error occurs before actual timer
> +periods are specified. delay_on and delay_off files get created with 0
> +values. Please note that it is important to set delay_off to forever prior
> +to setting delay_on value. If the order is reversed, the LED will be turned
> +on, with no timer set to turn it off.
> +
> +When delay_off value is specified in step 2, delay_off_store recognizes the
> +special forever tag and records it and returns without starting any timer.
> +Internally forever maps to ULONG_MAX. The led_blink_set() which takes
> +two pointers to times one each for delay_on and delay_off has been extended
> +so that a NULL instead of a pointer means "forever".
> +
> +When delay_on value is specified in step 3, a timer gets started for
> +delay_on period, and delay_off stays at ULONG_MAX with no timer associated
> +with it.
> +
> +use-case 2:
> +echo timer-no-default > /sys/class/leds/SOMELED/trigger
> +echo forever > /sys/class/leds/SOMELED/delay_on
> +echo 2000 > /sys/class/leds/SOMELED/delay_off
> +
> +When timer-no-default is activated in step1, unlike the timer trigger case,
> +timer-no-default activate routine activates the trigger without starting
> +any timers. The default 1 HZ delay_on and delay_off timers won't be started
> +like in the case of timer trigger activation. Not starting timers ensures
> +that the one time state isn't stuck if some error occurs before actual timer
> +periods are specified. delay_on and delay_off files get created with 0
> +values. Please note that it is important to set delay_on to forever prior
> +to setting delay_off value. If the order is reversed, the LED will be turned
> +off, with no timer set to turn it back on.
> +
> +When delay_on value is specified in step 2, delay_on_store recognizes the
> +special forever tag and records it and returns without starting any timer.
> +Internally forever maps to ULONG_MAX.
> +
> +When delay_off value is specified in step 3, a timer gets started for
> +delay_off period, and delay_on stays at ULONG_MAX with no timer associated
> +with it.
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 5bff843..ed123ba 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -107,7 +107,9 @@ static void led_timer_function(unsigned long data)
>
> led_set_brightness(led_cdev, brightness);
>
> - mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
> + if (delay != LED_TIMER_FOREVER)
> + mod_timer(&led_cdev->blink_timer,
> + jiffies + msecs_to_jiffies(delay));
> }
>
> /**
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index d686004..419b0bc 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -72,17 +72,35 @@ void led_blink_set(struct led_classdev *led_cdev,
> unsigned long *delay_on,
> unsigned long *delay_off)
> {
> + unsigned long val_on;
> + unsigned long val_off;
> +
> del_timer_sync(&led_cdev->blink_timer);
>
> - if (led_cdev->blink_set &&
> + if (delay_on && delay_off && led_cdev->blink_set &&
> !led_cdev->blink_set(led_cdev, delay_on, delay_off))
> return;
>
> + /* if delay_on is null, leave it on forever after delay_off period
> + if delay_off is null, leave it off forever after delay on period */
> + if (!delay_on)
> + val_on = LED_TIMER_FOREVER;
> + else
> + val_on = *delay_on;
> +
> + if (!delay_off)
> + val_off = LED_TIMER_FOREVER;
> + else
> + val_off = *delay_off;
> +
> /* blink with 1 Hz as default if nothing specified */
> - if (!*delay_on && !*delay_off)
> - *delay_on = *delay_off = 500;
> + if (!val_on && !val_off) {
> + val_on = val_off = 500;
> + *delay_on = 500;
> + *delay_off = 500;
> + }
>
> - led_set_software_blink(led_cdev, *delay_on, *delay_off);
> + led_set_software_blink(led_cdev, val_on, val_off);
> }
> EXPORT_SYMBOL(led_blink_set);
>
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index e77c7f8..b2cda9f 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -17,6 +17,8 @@
> #include <linux/rwsem.h>
> #include <linux/leds.h>
>
> +#define LED_TIMER_FOREVER ULONG_MAX
> +
> static inline void led_set_brightness(struct led_classdev *led_cdev,
> enum led_brightness value)
> {
> diff --git a/drivers/leds/ledtrig-timer.c b/drivers/leds/ledtrig-timer.c
> index 328c64c..e323cf2 100644
> --- a/drivers/leds/ledtrig-timer.c
> +++ b/drivers/leds/ledtrig-timer.c
> @@ -24,6 +24,9 @@ static ssize_t led_delay_on_show(struct device *dev,
> {
> struct led_classdev *led_cdev = dev_get_drvdata(dev);
>
> + if (led_cdev->blink_delay_on == LED_TIMER_FOREVER)
> + return sprintf(buf, "forever\n");
> +
> return sprintf(buf, "%lu\n", led_cdev->blink_delay_on);
> }
>
> @@ -32,17 +35,25 @@ static ssize_t led_delay_on_store(struct device *dev,
> {
> struct led_classdev *led_cdev = dev_get_drvdata(dev);
> int ret = -EINVAL;
> - char *after;
> - unsigned long state = simple_strtoul(buf, &after, 10);
> - size_t count = after - buf;
> -
> - if (isspace(*after))
> - count++;
>
> - if (count == size) {
> - led_blink_set(led_cdev, &state, &led_cdev->blink_delay_off);
> - led_cdev->blink_delay_on = state;
> - ret = count;
> + if (strncmp(buf, "forever", 7) == 0) {
> + led_blink_set(led_cdev, NULL, &led_cdev->blink_delay_off);
> + led_cdev->blink_delay_on = LED_TIMER_FOREVER;
> + ret = size;
> + } else {
> + char *after;
> + unsigned long state = simple_strtoul(buf, &after, 10);
> + size_t count = after - buf;
> +
> + if (isspace(*after))
> + count++;
> +
> + if (count == size) {
> + led_blink_set(led_cdev, &state,
> + &led_cdev->blink_delay_off);
> + led_cdev->blink_delay_on = state;
> + ret = count;
> + }
> }
>
> return ret;
> @@ -53,6 +64,9 @@ static ssize_t led_delay_off_show(struct device *dev,
> {
> struct led_classdev *led_cdev = dev_get_drvdata(dev);
>
> + if (led_cdev->blink_delay_off == LED_TIMER_FOREVER)
> + return sprintf(buf, "forever\n");
> +
> return sprintf(buf, "%lu\n", led_cdev->blink_delay_off);
> }
>
> @@ -61,17 +75,24 @@ static ssize_t led_delay_off_store(struct device *dev,
> {
> struct led_classdev *led_cdev = dev_get_drvdata(dev);
> int ret = -EINVAL;
> - char *after;
> - unsigned long state = simple_strtoul(buf, &after, 10);
> - size_t count = after - buf;
>
> - if (isspace(*after))
> - count++;
> -
> - if (count == size) {
> - led_blink_set(led_cdev, &led_cdev->blink_delay_on, &state);
> - led_cdev->blink_delay_off = state;
> - ret = count;
> + if (strncmp(buf, "forever", 7) == 0) {
> + led_blink_set(led_cdev, &led_cdev->blink_delay_on, NULL);
> + led_cdev->blink_delay_off = LED_TIMER_FOREVER;
> + } else {
> + char *after;
> + unsigned long state = simple_strtoul(buf, &after, 10);
> + size_t count = after - buf;
> +
> + if (isspace(*after))
> + count++;
> +
> + if (count == size) {
> + led_blink_set(led_cdev, &led_cdev->blink_delay_on,
> + &state);
> + led_cdev->blink_delay_off = state;
> + ret = count;
> + }
> }
>
> return ret;
> @@ -80,7 +101,7 @@ static ssize_t led_delay_off_store(struct device *dev,
> static DEVICE_ATTR(delay_on, 0644, led_delay_on_show, led_delay_on_store);
> static DEVICE_ATTR(delay_off, 0644, led_delay_off_show, led_delay_off_store);
>
> -static void timer_trig_activate(struct led_classdev *led_cdev)
> +static void timer_trig_activate_common(struct led_classdev *led_cdev)
> {
> int rc;
>
> @@ -91,17 +112,24 @@ static void timer_trig_activate(struct led_classdev *led_cdev)
> return;
> rc = device_create_file(led_cdev->dev, &dev_attr_delay_off);
> if (rc)
> - goto err_out_delayon;
> -
> - led_blink_set(led_cdev, &led_cdev->blink_delay_on,
> - &led_cdev->blink_delay_off);
> + device_remove_file(led_cdev->dev, &dev_attr_delay_on);
>
> - led_cdev->trigger_data = (void *)1;
> + else
> + led_cdev->trigger_data = (void *)1;
> +}
>
> - return;
> +static void timer_trig_activate_timer_no_default(struct led_classdev *led_cdev)
> +{
> + timer_trig_activate_common(led_cdev);
> +}
>
> -err_out_delayon:
> - device_remove_file(led_cdev->dev, &dev_attr_delay_on);
> +static void timer_trig_activate(struct led_classdev *led_cdev)
> +{
> + timer_trig_activate_common(led_cdev);
> + if (led_cdev->trigger_data) {
> + led_blink_set(led_cdev, &led_cdev->blink_delay_on,
> + &led_cdev->blink_delay_off);
> + }
> }
>
> static void timer_trig_deactivate(struct led_classdev *led_cdev)
> @@ -121,14 +149,30 @@ static struct led_trigger timer_led_trigger = {
> .deactivate = timer_trig_deactivate,
> };
>
> +static struct led_trigger timer_no_default_led_trigger = {
> + .name = "timer-no-default",
> + .activate = timer_trig_activate_timer_no_default,
> + .deactivate = timer_trig_deactivate,
> +};
> +
> static int __init timer_trig_init(void)
> {
> - return led_trigger_register(&timer_led_trigger);
> + int rc = 0;
> +
> + rc = led_trigger_register(&timer_led_trigger);
> + if (!rc) {
> + rc = led_trigger_register(&timer_no_default_led_trigger);
> + if (rc)
> + led_trigger_unregister(&timer_led_trigger);
> + }
> +
> + return rc;
> }
>
> static void __exit timer_trig_exit(void)
> {
> led_trigger_unregister(&timer_led_trigger);
> + led_trigger_unregister(&timer_no_default_led_trigger);
> }
>
> module_init(timer_trig_init);


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