Re: [PATCH 2/5] leds: core: Add LED_BLINK_CHANGE and LED_BLINK_DISABLE flags

[Jacek Anaszewski]

Hi Andrew,

Thanks for the review.

On 09/22/2015 08:44 PM, Andrew Lunn wrote:
> On Wed, Sep 16, 2015 at 12:47:41PM +0200, Jacek Anaszewski wrote:
> > This patch adds LED_BLINK_CHANGE flag to indicate that blink brightness
> > has changed
>
> Hi Jacek
>
> The name LED_BLINK_CHANGE does not make me think of changing
> brightness. Maybe a better name would be LED_BLINK_BRIGHTNESS_CHANGE,
> although that it getting a bit long.

I had the same dilemma. We could go for shortcut like
LED_BLINK_BR_CHANGE, but is is not too meaningful either.
I will rename LED_BLINK_CHANGE to LED_BLINK_BRIGHTNESS_CHANGE, unless
better option appears.

> , and LED_BLINK_DISABLE flag to indicate that blinking
> > deactivation has been requested. In order to use the flags
> > led_timer_function and set_brightness_delayed callbacks as well as
> > led_set_brightness function are being modified. The main goal of these
> > modifications is to prepare set_brightness_work for extension of the
> > scope of its responsibilities.
> >
> > Signed-off-by: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>
> > ---
> >   drivers/leds/led-core.c |   36 ++++++++++++++++++++++++++----------
> >   include/linux/leds.h    |   10 ++++++----
> >   2 files changed, 32 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> > index 2cb4e37..1968e24 100644
> > --- a/drivers/leds/led-core.c
> > +++ b/drivers/leds/led-core.c
> > @@ -44,18 +44,18 @@ static void led_timer_function(unsigned long data)
> >   	brightness = led_get_brightness(led_cdev);
> >   	if (!brightness) {
> >   		/* Time to switch the LED on. */
> > -		if (led_cdev->delayed_set_value) {
> > -			led_cdev->blink_brightness =
> > -					led_cdev->delayed_set_value;
> > -			led_cdev->delayed_set_value = 0;
> > -		}
> >   		brightness = led_cdev->blink_brightness;
> >   		delay = led_cdev->blink_delay_on;
> >   	} else {
> >   		/* Store the current brightness value to be able
> >   		 * to restore it when the delay_off period is over.
> > +		 * Do it only if there is no pending blink brightness
> > +		 * change, to avoid overwriting the new value.
> >   		 */
> > -		led_cdev->blink_brightness = brightness;
> > +		if (!(led_cdev->flags & LED_BLINK_CHANGE))
> > +			led_cdev->blink_brightness = brightness;
> > +		else
> > +			led_cdev->flags &= ~LED_BLINK_CHANGE;
> >   		brightness = LED_OFF;
> >   		delay = led_cdev->blink_delay_off;
> >   	}
> > @@ -84,7 +84,11 @@ static void set_brightness_delayed(struct work_struct *ws)
> >   	struct led_classdev *led_cdev =
> >   		container_of(ws, struct led_classdev, set_brightness_work);
> >
> > -	led_stop_software_blink(led_cdev);
> > +	if (led_cdev->flags & LED_BLINK_DISABLE) {
> > +		led_cdev->delayed_set_value = LED_OFF;
> > +		led_stop_software_blink(led_cdev);
> > +		led_cdev->flags &= ~LED_BLINK_DISABLE;
> > +	}
> >
> >   	led_set_brightness_async(led_cdev, led_cdev->delayed_set_value);
> >   }
> > @@ -192,11 +196,23 @@ void led_set_brightness(struct led_classdev *led_cdev,
> >   {
> >   	int ret = 0;
> >
> > -	/* delay brightness if soft-blink is active */
> > +	/*
> > +	 * In case blinking is on delay brightness setting
> > +	 * until the next timer tick.
> > +	 */
> >   	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
> > -		led_cdev->delayed_set_value = brightness;
> > -		if (brightness == LED_OFF)
> > +		/*
> > +		 * If need to disable soft blinking delegate this to the
>
> If _we_ need to...

OK.

> > +		 * work queue task to avoid problems in case we are
> > +		 * called from hard irq context.
> > +		 */
> > +		if (brightness == LED_OFF) {
> > +			led_cdev->flags |= LED_BLINK_DISABLE;
> >   			schedule_work(&led_cdev->set_brightness_work);
> > +		} else {
> > +			led_cdev->flags |= LED_BLINK_CHANGE;
> > +			led_cdev->blink_brightness = brightness;
> > +		}
> >   		return;
> >   	}
> >
> > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > index b122eea..188352c 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -44,10 +44,12 @@ struct led_classdev {
> >   #define LED_BLINK_ONESHOT	(1 << 17)
> >   #define LED_BLINK_ONESHOT_STOP	(1 << 18)
> >   #define LED_BLINK_INVERT	(1 << 19)
> > -#define LED_SYSFS_DISABLE	(1 << 20)
> > -#define SET_BRIGHTNESS_ASYNC	(1 << 21)
> > -#define SET_BRIGHTNESS_SYNC	(1 << 22)
> > -#define LED_DEV_CAP_FLASH	(1 << 23)
> > +#define LED_BLINK_CHANGE	(1 << 20)
> > +#define LED_BLINK_DISABLE	(1 << 21)
> > +#define LED_SYSFS_DISABLE	(1 << 22)
> > +#define SET_BRIGHTNESS_ASYNC	(1 << 23)
> > +#define SET_BRIGHTNESS_SYNC	(1 << 24)
> > +#define LED_DEV_CAP_FLASH	(1 << 25)
> >
> >   	/* Set LED brightness level */
> >   	/* Must not sleep, use a workqueue if needed */
> > --
> > 1.7.9.5


--
Best Regards,
Jacek Anaszewski
--
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/