Re: [PATCH] Let chip-driver set brightness if, software blink not used.

Re: [PATCH] Let chip-driver set brightness if, software blink not used.

[Jacek Anaszewski]

Hi Tony,

Thanks for the patch. I'd like to improve the commit message.

Please let me know if you have any suggestions related to the below:

     led: core: Fix brightness setting upon hardware blinking enabled

     Commit 76931edd54 ('leds: fix brightness changing when software 
blinking
     is active') changed the semantics of led_set_brightness() which 
according
     to the documentation should disable blinking on any brightness setting.
     Moreover it made it different for soft blink case, where it was allowed
     for blink brightness change, and for hardware blink case, where setting
     any brightness greater than 0 was ignored.

     While the change itself is against the documentation claims, it was 
driven
     also by the fact that timer trigger remained active after turning 
blinking
     off. Fixing that would have required major refactoring in the led-core,
     led-class, and led-triggers because of cyclic dependencies.

     Finally, it has been decided that allowing for brightness change during
     blinking is beneficial as it can be accomplished without disturbing
     blink rhythm.

     The change in brightness setting semantics will not affect existing
     LED class drivers that implement blink_set op thanks to the 
LED_BLINK_SW
     flag introduced by this patch. The flag state will be from now on 
checked
     in led_set_brightness() which will allow to distinguish between 
hardware
     and software blink mode. In the former case the control will be passed
     directly to the drivers which apply their semantics on brightness set,
     which is disable the blinking in case of most such drivers. New drivers
     will apply new semantics and just change the brightness while hardware
     blinking is on, if possible.

     Due to the later LED core improvements this patch can't be applied 
directly
     on the patch that originally introduced the problem, but to the 
later one,
     that touched the affected code.

     Fixes: f1e80c07416a ("leds: core: Add two new LED_BLINK_ flags")
     Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com>
     Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>


We will need also to modify the documentation, but this can
be done in a separate patch.

On 05/17/2016 06:28 PM, Tony Makkiel wrote:
> With this patch, chip drivers will be given option to
> set brightness while, blink is active.
>
> Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com>
> ---
>   drivers/leds/led-core.c |  7 +++++--
>   include/linux/leds.h    | 19 ++++++++++---------
>   2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 3495d5d..b595eb9 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -53,11 +53,12 @@ static void led_timer_function(unsigned long data)
>
>   	if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
>   		led_set_brightness_nosleep(led_cdev, LED_OFF);
> +		led_cdev->flags &= ~LED_BLINK_SW;
>   		return;
>   	}
>
>   	if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) {
> -		led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
> +		led_cdev->flags &=  ~(LED_BLINK_ONESHOT_STOP | LED_BLINK_SW);
>   		return;
>   	}
>
> @@ -151,6 +152,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>   		return;
>   	}
>
> +	led_cdev->flags |= LED_BLINK_SW;
>   	mod_timer(&led_cdev->blink_timer, jiffies + 1);
>   }
>
> @@ -219,6 +221,7 @@ void led_stop_software_blink(struct led_classdev *led_cdev)
>   	del_timer_sync(&led_cdev->blink_timer);
>   	led_cdev->blink_delay_on = 0;
>   	led_cdev->blink_delay_off = 0;
> +	led_cdev->flags &= ~LED_BLINK_SW;
>   }
>   EXPORT_SYMBOL_GPL(led_stop_software_blink);
>
> @@ -229,7 +232,7 @@ void led_set_brightness(struct led_classdev *led_cdev,
>   	 * In case blinking is on delay brightness setting

We need to make this comment more precise:

s/blinking/soft blink/g

>   	 * until the next timer tick.
>   	 */
> -	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
> +	if (led_cdev->flags & LED_BLINK_SW) {
>   		/*
>   		 * If we need to disable soft blinking delegate this to the
>   		 * work queue task to avoid problems in case we are called
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index d2b1306..99b1f0b 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -42,15 +42,16 @@ struct led_classdev {
>   #define LED_UNREGISTERING	(1 << 1)
>   	/* 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)
> -#define LED_BLINK_BRIGHTNESS_CHANGE (1 << 20)
> -#define LED_BLINK_DISABLE	(1 << 21)
> -#define LED_SYSFS_DISABLE	(1 << 22)
> -#define LED_DEV_CAP_FLASH	(1 << 23)
> -#define LED_HW_PLUGGABLE	(1 << 24)
> -#define LED_PANIC_INDICATOR	(1 << 25)
> +#define LED_BLINK_SW		(1 << 17)
> +#define LED_BLINK_ONESHOT	(1 << 18)
> +#define LED_BLINK_ONESHOT_STOP	(1 << 19)
> +#define LED_BLINK_INVERT	(1 << 20)
> +#define LED_BLINK_BRIGHTNESS_CHANGE (1 << 21)
> +#define LED_BLINK_DISABLE	(1 << 22)
> +#define LED_SYSFS_DISABLE	(1 << 23)
> +#define LED_DEV_CAP_FLASH	(1 << 24)
> +#define LED_HW_PLUGGABLE	(1 << 25)
> +#define LED_PANIC_INDICATOR	(1 << 26)
>
>   	/* Set LED brightness level
>   	 * Must not sleep. Use brightness_set_blocking for drivers
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] Let chip-driver set brightness if, software blink not used.

[Tony Makkiel]

On 18/05/16 10:03, Jacek Anaszewski wrote:
> Hi Tony,
> 
> Thanks for the patch. I'd like to improve the commit message.
> 
> Please let me know if you have any suggestions related to the below:
> 
>     led: core: Fix brightness setting upon hardware blinking enabled
> 
>     Commit 76931edd54 ('leds: fix brightness changing when software blinking
>     is active') changed the semantics of led_set_brightness() which according
>     to the documentation should disable blinking on any brightness setting.
>     Moreover it made it different for soft blink case, where it was allowed
>     for blink brightness change, and for hardware blink case, where setting
>     any brightness greater than 0 was ignored.
> 
>     While the change itself is against the documentation claims, it was driven
>     also by the fact that timer trigger remained active after turning blinking
>     off. Fixing that would have required major refactoring in the led-core,
>     led-class, and led-triggers because of cyclic dependencies.
> 
>     Finally, it has been decided that allowing for brightness change during
>     blinking is beneficial as it can be accomplished without disturbing
>     blink rhythm.
> 
>     The change in brightness setting semantics will not affect existing
>     LED class drivers that implement blink_set op thanks to the LED_BLINK_SW
>     flag introduced by this patch. The flag state will be from now on checked
>     in led_set_brightness() which will allow to distinguish between hardware
>     and software blink mode. In the former case the control will be passed
>     directly to the drivers which apply their semantics on brightness set,
>     which is disable the blinking in case of most such drivers. New drivers
>     will apply new semantics and just change the brightness while hardware
>     blinking is on, if possible.
> 
>     Due to the later LED core improvements this patch can't be applied directly
>     on the patch that originally introduced the problem, but to the later one,
>     that touched the affected code.
> 
>     Fixes: f1e80c07416a ("leds: core: Add two new LED_BLINK_ flags")
>     Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com>
>     Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> 
> 
It looks good to me. Do you want me to resubmit the patch with new commit message?
 Or can you edit it while merging the patch.

> We will need also to modify the documentation, but this can
> be done in a separate patch.

Will make another patch for it. Does the following look ok?

diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
index d406d98..44f5e6b 100644
--- a/Documentation/leds/leds-class.txt
+++ b/Documentation/leds/leds-class.txt
@@ -74,8 +74,8 @@ blink_set() function (see <linux/leds.h>). To set an LED to blinking,
 however, it is better to use the API function led_blink_set(), as it
 will check and implement software fallback if necessary.
 
-To turn off blinking again, use the API function led_brightness_set()
-as that will not just set the LED brightness but also stop any software
+To turn off blinking, use the API function led_brightness_set()
+with brightness value LED_OFF, which should stop any software
 timers that may have been required for blinking.
 
 The blink_set() function should choose a user friendly blinking value

> 
> On 05/17/2016 06:28 PM, Tony Makkiel wrote:
>> With this patch, chip drivers will be given option to
>> set brightness while, blink is active.
>>
>> Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com>
>> ---
>>   drivers/leds/led-core.c |  7 +++++--
>>   include/linux/leds.h    | 19 ++++++++++---------
>>   2 files changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index 3495d5d..b595eb9 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -53,11 +53,12 @@ static void led_timer_function(unsigned long data)
>>
>>       if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
>>           led_set_brightness_nosleep(led_cdev, LED_OFF);
>> +        led_cdev->flags &= ~LED_BLINK_SW;
>>           return;
>>       }
>>
>>       if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) {
>> -        led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
>> +        led_cdev->flags &=  ~(LED_BLINK_ONESHOT_STOP | LED_BLINK_SW);
>>           return;
>>       }
>>
>> @@ -151,6 +152,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>>           return;
>>       }
>>
>> +    led_cdev->flags |= LED_BLINK_SW;
>>       mod_timer(&led_cdev->blink_timer, jiffies + 1);
>>   }
>>
>> @@ -219,6 +221,7 @@ void led_stop_software_blink(struct led_classdev *led_cdev)
>>       del_timer_sync(&led_cdev->blink_timer);
>>       led_cdev->blink_delay_on = 0;
>>       led_cdev->blink_delay_off = 0;
>> +    led_cdev->flags &= ~LED_BLINK_SW;
>>   }
>>   EXPORT_SYMBOL_GPL(led_stop_software_blink);
>>
>> @@ -229,7 +232,7 @@ void led_set_brightness(struct led_classdev *led_cdev,
>>        * In case blinking is on delay brightness setting
> 
> We need to make this comment more precise:

Will include the following along with documentation patch, unless require modification.

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index b595eb9..f3af7a6 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -229,7 +229,7 @@ void led_set_brightness(struct led_classdev *led_cdev,
                        enum led_brightness brightness)
 {
        /*
-        * In case blinking is on delay brightness setting
+        * If software blink active, delay brightness setting
         * until the next timer tick.
         */
        if (led_cdev->flags & LED_BLINK_SW) {

> 
> s/blinking/soft blink/g
> 
>>        * until the next timer tick.
>>        */
>> -    if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>> +    if (led_cdev->flags & LED_BLINK_SW) {
>>           /*
>>            * If we need to disable soft blinking delegate this to the
>>            * work queue task to avoid problems in case we are called
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index d2b1306..99b1f0b 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -42,15 +42,16 @@ struct led_classdev {
>>   #define LED_UNREGISTERING    (1 << 1)
>>       /* 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)
>> -#define LED_BLINK_BRIGHTNESS_CHANGE (1 << 20)
>> -#define LED_BLINK_DISABLE    (1 << 21)
>> -#define LED_SYSFS_DISABLE    (1 << 22)
>> -#define LED_DEV_CAP_FLASH    (1 << 23)
>> -#define LED_HW_PLUGGABLE    (1 << 24)
>> -#define LED_PANIC_INDICATOR    (1 << 25)
>> +#define LED_BLINK_SW        (1 << 17)
>> +#define LED_BLINK_ONESHOT    (1 << 18)
>> +#define LED_BLINK_ONESHOT_STOP    (1 << 19)
>> +#define LED_BLINK_INVERT    (1 << 20)
>> +#define LED_BLINK_BRIGHTNESS_CHANGE (1 << 21)
>> +#define LED_BLINK_DISABLE    (1 << 22)
>> +#define LED_SYSFS_DISABLE    (1 << 23)
>> +#define LED_DEV_CAP_FLASH    (1 << 24)
>> +#define LED_HW_PLUGGABLE    (1 << 25)
>> +#define LED_PANIC_INDICATOR    (1 << 26)
>>
>>       /* Set LED brightness level
>>        * Must not sleep. Use brightness_set_blocking for drivers
>>
> 
> 

Re: [PATCH] Let chip-driver set brightness if, software blink not used.

[Jacek Anaszewski]

On 05/18/2016 12:33 PM, Tony Makkiel wrote:
>
>
> On 18/05/16 10:03, Jacek Anaszewski wrote:
>> Hi Tony,
>>
>> Thanks for the patch. I'd like to improve the commit message.
>>
>> Please let me know if you have any suggestions related to the below:
>>
>>      led: core: Fix brightness setting upon hardware blinking enabled
>>
>>      Commit 76931edd54 ('leds: fix brightness changing when software blinking
>>      is active') changed the semantics of led_set_brightness() which according
>>      to the documentation should disable blinking on any brightness setting.
>>      Moreover it made it different for soft blink case, where it was allowed
>>      for blink brightness change, and for hardware blink case, where setting
>>      any brightness greater than 0 was ignored.
>>
>>      While the change itself is against the documentation claims, it was driven
>>      also by the fact that timer trigger remained active after turning blinking
>>      off. Fixing that would have required major refactoring in the led-core,
>>      led-class, and led-triggers because of cyclic dependencies.
>>
>>      Finally, it has been decided that allowing for brightness change during
>>      blinking is beneficial as it can be accomplished without disturbing
>>      blink rhythm.
>>
>>      The change in brightness setting semantics will not affect existing
>>      LED class drivers that implement blink_set op thanks to the LED_BLINK_SW
>>      flag introduced by this patch. The flag state will be from now on checked
>>      in led_set_brightness() which will allow to distinguish between hardware
>>      and software blink mode. In the former case the control will be passed
>>      directly to the drivers which apply their semantics on brightness set,
>>      which is disable the blinking in case of most such drivers. New drivers
>>      will apply new semantics and just change the brightness while hardware
>>      blinking is on, if possible.
>>
>>      Due to the later LED core improvements this patch can't be applied directly
>>      on the patch that originally introduced the problem, but to the later one,
>>      that touched the affected code.
>>
>>      Fixes: f1e80c07416a ("leds: core: Add two new LED_BLINK_ flags")
>>      Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com>
>>      Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>>
>>
> It looks good to me. Do you want me to resubmit the patch with new commit message?
>   Or can you edit it while merging the patch.

Yes please, if it's not a problem.

>> We will need also to modify the documentation, but this can
>> be done in a separate patch.
>
> Will make another patch for it. Does the following look ok?
>
> diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
> index d406d98..44f5e6b 100644
> --- a/Documentation/leds/leds-class.txt
> +++ b/Documentation/leds/leds-class.txt
> @@ -74,8 +74,8 @@ blink_set() function (see <linux/leds.h>). To set an LED to blinking,
>   however, it is better to use the API function led_blink_set(), as it
>   will check and implement software fallback if necessary.
>
> -To turn off blinking again, use the API function led_brightness_set()
> -as that will not just set the LED brightness but also stop any software
> +To turn off blinking, use the API function led_brightness_set()
> +with brightness value LED_OFF, which should stop any software
>   timers that may have been required for blinking.

Seems good.

>   The blink_set() function should choose a user friendly blinking value
>
>>
>> On 05/17/2016 06:28 PM, Tony Makkiel wrote:
>>> With this patch, chip drivers will be given option to
>>> set brightness while, blink is active.
>>>
>>> Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com>
>>> ---
>>>    drivers/leds/led-core.c |  7 +++++--
>>>    include/linux/leds.h    | 19 ++++++++++---------
>>>    2 files changed, 15 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>> index 3495d5d..b595eb9 100644
>>> --- a/drivers/leds/led-core.c
>>> +++ b/drivers/leds/led-core.c
>>> @@ -53,11 +53,12 @@ static void led_timer_function(unsigned long data)
>>>
>>>        if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
>>>            led_set_brightness_nosleep(led_cdev, LED_OFF);
>>> +        led_cdev->flags &= ~LED_BLINK_SW;
>>>            return;
>>>        }
>>>
>>>        if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) {
>>> -        led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
>>> +        led_cdev->flags &=  ~(LED_BLINK_ONESHOT_STOP | LED_BLINK_SW);
>>>            return;
>>>        }
>>>
>>> @@ -151,6 +152,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>>>            return;
>>>        }
>>>
>>> +    led_cdev->flags |= LED_BLINK_SW;
>>>        mod_timer(&led_cdev->blink_timer, jiffies + 1);
>>>    }
>>>
>>> @@ -219,6 +221,7 @@ void led_stop_software_blink(struct led_classdev *led_cdev)
>>>        del_timer_sync(&led_cdev->blink_timer);
>>>        led_cdev->blink_delay_on = 0;
>>>        led_cdev->blink_delay_off = 0;
>>> +    led_cdev->flags &= ~LED_BLINK_SW;
>>>    }
>>>    EXPORT_SYMBOL_GPL(led_stop_software_blink);
>>>
>>> @@ -229,7 +232,7 @@ void led_set_brightness(struct led_classdev *led_cdev,
>>>         * In case blinking is on delay brightness setting
>>
>> We need to make this comment more precise:
>
> Will include the following along with documentation patch, unless require modification.
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index b595eb9..f3af7a6 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -229,7 +229,7 @@ void led_set_brightness(struct led_classdev *led_cdev,
>                          enum led_brightness brightness)
>   {
>          /*
> -        * In case blinking is on delay brightness setting
> +        * If software blink active, delay brightness setting

s/active/is active/

>           * until the next timer tick.
>           */
>          if (led_cdev->flags & LED_BLINK_SW) {
>
>>
>> s/blinking/soft blink/g
>>
>>>         * until the next timer tick.
>>>         */
>>> -    if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>>> +    if (led_cdev->flags & LED_BLINK_SW) {
>>>            /*
>>>             * If we need to disable soft blinking delegate this to the
>>>             * work queue task to avoid problems in case we are called
>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>> index d2b1306..99b1f0b 100644
>>> --- a/include/linux/leds.h
>>> +++ b/include/linux/leds.h
>>> @@ -42,15 +42,16 @@ struct led_classdev {
>>>    #define LED_UNREGISTERING    (1 << 1)
>>>        /* 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)
>>> -#define LED_BLINK_BRIGHTNESS_CHANGE (1 << 20)
>>> -#define LED_BLINK_DISABLE    (1 << 21)
>>> -#define LED_SYSFS_DISABLE    (1 << 22)
>>> -#define LED_DEV_CAP_FLASH    (1 << 23)
>>> -#define LED_HW_PLUGGABLE    (1 << 24)
>>> -#define LED_PANIC_INDICATOR    (1 << 25)
>>> +#define LED_BLINK_SW        (1 << 17)
>>> +#define LED_BLINK_ONESHOT    (1 << 18)
>>> +#define LED_BLINK_ONESHOT_STOP    (1 << 19)
>>> +#define LED_BLINK_INVERT    (1 << 20)
>>> +#define LED_BLINK_BRIGHTNESS_CHANGE (1 << 21)
>>> +#define LED_BLINK_DISABLE    (1 << 22)
>>> +#define LED_SYSFS_DISABLE    (1 << 23)
>>> +#define LED_DEV_CAP_FLASH    (1 << 24)
>>> +#define LED_HW_PLUGGABLE    (1 << 25)
>>> +#define LED_PANIC_INDICATOR    (1 << 26)
>>>
>>>        /* Set LED brightness level
>>>         * Must not sleep. Use brightness_set_blocking for drivers
>>>
>>
>>
>
>


-- 
Best regards,
Jacek Anaszewski