Nits:
> ALl of this will be handled transparently from the
> led_blink_set() call so all current users can keep
> using that.
"All". Plus, your lines are rather short.
>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> drivers/leds/led-core.c | 49 ++++++++++++++++++++++++++++++++++++++---
> include/linux/leds.h | 9 ++++++++
> 2 files changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index ede4fa0ac2cc..94870a3d59af 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -108,6 +108,20 @@ static void set_brightness_delayed(struct work_struct *ws)
> container_of(ws, struct led_classdev, set_brightness_work);
> int ret = 0;
>
> + if (test_and_clear_bit(LED_BLINK_HW_SET, &led_cdev->work_flags)) {
> + if (led_cdev->blink_set)
> + ret = led_cdev->blink_set(led_cdev,
> + &led_cdev->blink_delay_on,
> + &led_cdev->blink_delay_off);
How can we get here?
> + else if (led_cdev->blink_set_blocking)
> + ret = led_cdev->blink_set_blocking(led_cdev,
> + &led_cdev->blink_delay_on,
> + &led_cdev->blink_delay_off);
> + if (ret)
> + dev_err(led_cdev->dev,
> + "setting hardware blink failed (%d)\n", ret);
> + }
This is a bit sad, as it disallows returning errors back to
userspace. And I'm pretty sure little hardware can support all the
possible delays....
> mod_timer(&led_cdev->blink_timer, jiffies + 1);
> }
>
> +static int led_set_hardware_blink_nosleep(struct led_classdev *led_cdev,
> + unsigned long *delay_on,
> + unsigned long *delay_off)
> +{
> + /* We have a non-blocking call, use it */
> + if (led_cdev->blink_set)
> + return led_cdev->blink_set(led_cdev, delay_on, delay_off);
> +
> + /* Tell the worker to set up the blinking hardware */
> + if (delay_on)
> + led_cdev->blink_delay_on = *delay_on;
> + else
> + led_cdev->blink_delay_on = 0;
> + if (delay_off)
> + led_cdev->blink_delay_off = *delay_off;
> + else
> + led_cdev->blink_delay_off = 0;
> + set_bit(LED_BLINK_HW_SET, &led_cdev->work_flags);
> + schedule_work(&led_cdev->set_brightness_work);
> +
> + return 0;
> +}
>
> static void led_blink_setup(struct led_classdev *led_cdev,
> unsigned long *delay_on,
> unsigned long *delay_off)
> {
> + /*
> + * If not oneshot, and we have hardware support for blinking,
> + * relay the request to the driver.
> + */
> if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) &&
> - led_cdev->blink_set &&
> - !led_cdev->blink_set(led_cdev, delay_on, delay_off))
> - return;
> + (led_cdev->blink_set || led_cdev->blink_set_blocking)) {
> + /* If this fails we fall back to software blink */
> + if (!led_set_hardware_blink_nosleep(led_cdev,
> + delay_on, delay_off))
> + return;
> + }
>
So, when can this actually be called from atomic context?
(And deferring to work queue to set hardware blinking for one shot
... does not make sense.)
led_trigger_blink_setup uses read_lock() -- unsafe from atomic AFAICT.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Attachment:
signature.asc
Description: Digital signature
