lkml.org 
[lkml]   [2015]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/5] leds: core: Add LED_BLINK_CHANGE and LED_BLINK_DISABLE flags
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@samsung.com>
>> ---
>> 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


\
 
 \ /
  Last update: 2015-09-23 10:21    [W:0.064 / U:0.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site