Re: [PATCH RESEND] leds: add oneshot blink functions

From: Bryan Wu
Date: Fri May 25 2012 - 10:39:58 EST


Hi Fabio,

Bacially I like this idea to extend the API and delete duplicate code.
Could you please post some use case patches based on this API? I guess
we can convert led ide-disk trigger to use it or your ledtrig-can
patch.
And please rebase this patch on top of my for-next branch.
git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds.git for-next

And I think this patch missed this 3.5-rc1 merge window, but we can
make out for next one.

-Bryan

On Tue, May 22, 2012 at 5:15 AM, Fabio Baltieri
<fabio.baltieri@xxxxxxxxx> 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
> events, 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, blinking the led off, than on
> and leaving the led on after the sequence.
>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@xxxxxxxxx>
> Cc: Bryan Wu <bryan.wu@xxxxxxxxxxxxx>
> Cc: Richard Purdie <rpurdie@xxxxxxxxx>
> Cc: linux-leds@xxxxxxxxxxxxxxx
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
> Resend with new maintainer and new list in Cc - thanks Shuah.
>
> Hi all!
>
> Since merge window is open I'm resending this PATCH/RFC. First mail was a bit
> long so I'll try to make a long story short.
>
> This is a proposal to add a LED-class one-shot library function, to be used by
> one-shot event triggers, like ide-disk or xt_LED, which currently implements
> framework-specific led control code.
>
> This implementation have been developed after a discussion on the linux-can list
> on a patch I proposed which used another framework-specific implementation of a
> one-shot trigger.
>
> That's the link of the linux-can patch where the discussion started:
> http://article.gmane.org/gmane.linux.can/1085
>
> And that's my original post of this one with a more detailed description:
> https://lkml.org/lkml/2012/5/8/41
>
> So, my question is, what do you think is better between framework-specific
> implementation (simple/light) and generic led-class level trigger - like this
> patch - (less code duplication, useful in other frameworks) for one-shot event
> triggers?
>
> Personally, I'm voting for the first version, as in my patch in linux-can,
> because the implementation is cleaner, but some other opinion would be
> valuable.
>
> If we'll get to a point, I'll post a v3 of my canbus-led-triggers patch for
> proposed integration, based either on the original version or on this patch.
>
> Regards,
> 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 46b4c76..351cbde 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -230,9 +230,11 @@ void led_trigger_event(struct led_trigger *trigger,
>  }
>  EXPORT_SYMBOL_GPL(led_trigger_event);
>
> -void led_trigger_blink(struct led_trigger *trigger,
> -                      unsigned long *delay_on,
> -                      unsigned long *delay_off)
> +void led_trigger_blink_setup(struct led_trigger *trigger,
> +                            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 *trigger,
>                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(&trigger->leddev_list_lock);
>  }
> +
> +void led_trigger_blink(struct led_trigger *trigger,
> +                      unsigned long *delay_on,
> +                      unsigned long *delay_off)
> +{
> +       led_trigger_blink_setup(trigger, delay_on, delay_off, 0, 0);
> +}
>  EXPORT_SYMBOL_GPL(led_trigger_blink);
>
> +void led_trigger_blink_oneshot(struct led_trigger *trigger,
> +                              unsigned long *delay_on,
> +                              unsigned long *delay_off,
> +                              int invert)
> +{
> +       led_trigger_blink_setup(trigger, 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 *trigger;
> 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
>
> --
> 1.7.10.2.520.g6a4a482.dirty
>



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