Re: [PATCH v2] leds: fix brightness changing when software blinking is active

Re: [PATCH v2] leds: fix brightness changing when software blinking is active

[Pavel Machek]

On Thu 2015-05-14 18:24:02, Stas Sergeev wrote:
> 
> The following sequence:
> echo timer >/sys/class/leds/<name>/trigger
> echo 1 >/sys/class/leds/<name>/brightness
> should change the ON brightness for blinking.
> The function led_set_brightness() was mistakenly initiating the
> delayed blink stop procedure, which resulted in no blinking with
> the timer trigger still active.
> 
> This patch fixes the problem by changing led_set_brightness()
> to not initiate the delayed blink stop when brightness is not 0.

Could we get this part of API documented? It is quite non-obvious... 0 clears the trigger,
other values do not, I thought it is a bug when I saw it...

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH v2] leds: fix brightness changing when software blinking is active

[Stas Sergeev]

The following sequence:
echo timer >/sys/class/leds/<name>/trigger
echo 1 >/sys/class/leds/<name>/brightness
should change the ON brightness for blinking.
The function led_set_brightness() was mistakenly initiating the
delayed blink stop procedure, which resulted in no blinking with
the timer trigger still active.

This patch fixes the problem by changing led_set_brightness()
to not initiate the delayed blink stop when brightness is not 0.

CC: Bryan Wu <cooloney@gmail.com>
CC: Richard Purdie <rpurdie@rpsys.net>
CC: Jacek Anaszewski <j.anaszewski@samsung.com>
CC: Kyungmin Park <kyungmin.park@samsung.com>
CC: linux-leds@vger.kernel.org
CC: linux-kernel@vger.kernel.org

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
 drivers/leds/led-class.c |    5 +++++
 drivers/leds/led-core.c  |    5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 795ec99..65c2c80 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -121,6 +121,11 @@ 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 {
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 356e851..a90dd26 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -119,10 +119,11 @@ void led_set_brightness(struct led_classdev *led_cdev,
 {
 	int ret = 0;

-	/* delay brightness setting if need to stop soft-blink timer */
+	/* delay brightness if soft-blink is active */
 	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
 		led_cdev->delayed_set_value = brightness;
-		schedule_work(&led_cdev->set_brightness_work);
+		if (brightness == LED_OFF)
+			schedule_work(&led_cdev->set_brightness_work);
 		return;
 	}

-- 
1.7.9.5

Re: [PATCH v2] leds: fix brightness changing when software blinking is active

[Jacek Anaszewski]

Hi Stas,

On 05/14/2015 05:24 PM, Stas Sergeev wrote:
>
> The following sequence:
> echo timer >/sys/class/leds/<name>/trigger
> echo 1 >/sys/class/leds/<name>/brightness
> should change the ON brightness for blinking.
> The function led_set_brightness() was mistakenly initiating the
> delayed blink stop procedure, which resulted in no blinking with
> the timer trigger still active.
>
> This patch fixes the problem by changing led_set_brightness()
> to not initiate the delayed blink stop when brightness is not 0.
>
> CC: Bryan Wu <cooloney@gmail.com>
> CC: Richard Purdie <rpurdie@rpsys.net>
> CC: Jacek Anaszewski <j.anaszewski@samsung.com>
> CC: Kyungmin Park <kyungmin.park@samsung.com>
> CC: linux-leds@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
>
> Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
> ---
>   drivers/leds/led-class.c |    5 +++++
>   drivers/leds/led-core.c  |    5 +++--
>   2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 795ec99..65c2c80 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -121,6 +121,11 @@ 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 {
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 356e851..a90dd26 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -119,10 +119,11 @@ void led_set_brightness(struct led_classdev *led_cdev,
>   {
>   	int ret = 0;
>
> -	/* delay brightness setting if need to stop soft-blink timer */
> +	/* delay brightness if soft-blink is active */
>   	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>   		led_cdev->delayed_set_value = brightness;
> -		schedule_work(&led_cdev->set_brightness_work);
> +		if (brightness == LED_OFF)
> +			schedule_work(&led_cdev->set_brightness_work);
>   		return;
>   	}
>

Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2] leds: fix brightness changing when software blinking is active

[Jacek Anaszewski]

On 05/15/2015 10:12 AM, Jacek Anaszewski wrote:
> Hi Stas,
>
> On 05/14/2015 05:24 PM, Stas Sergeev wrote:
>>
>> The following sequence:
>> echo timer >/sys/class/leds/<name>/trigger
>> echo 1 >/sys/class/leds/<name>/brightness
>> should change the ON brightness for blinking.
>> The function led_set_brightness() was mistakenly initiating the
>> delayed blink stop procedure, which resulted in no blinking with
>> the timer trigger still active.
>>
>> This patch fixes the problem by changing led_set_brightness()
>> to not initiate the delayed blink stop when brightness is not 0.

This commit message is not valid for this version of the patch.
Current patch just allows to change the LED brightness while
timer trigger is active. Please fix this description.

>>
>> CC: Bryan Wu <cooloney@gmail.com>
>> CC: Richard Purdie <rpurdie@rpsys.net>
>> CC: Jacek Anaszewski <j.anaszewski@samsung.com>
>> CC: Kyungmin Park <kyungmin.park@samsung.com>
>> CC: linux-leds@vger.kernel.org
>> CC: linux-kernel@vger.kernel.org
>>
>> Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
>> ---
>>   drivers/leds/led-class.c |    5 +++++
>>   drivers/leds/led-core.c  |    5 +++--
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 795ec99..65c2c80 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -121,6 +121,11 @@ 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 {
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index 356e851..a90dd26 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -119,10 +119,11 @@ void led_set_brightness(struct led_classdev
>> *led_cdev,
>>   {
>>       int ret = 0;
>>
>> -    /* delay brightness setting if need to stop soft-blink timer */
>> +    /* delay brightness if soft-blink is active */
>>       if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>>           led_cdev->delayed_set_value = brightness;
>> -        schedule_work(&led_cdev->set_brightness_work);
>> +        if (brightness == LED_OFF)
>> +            schedule_work(&led_cdev->set_brightness_work);
>>           return;
>>       }
>>
>
> Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2] leds: fix brightness changing when software blinking is active

[Stas Sergeev]

15.05.2015 17:09, Jacek Anaszewski пишет:
> On 05/15/2015 10:12 AM, Jacek Anaszewski wrote:
>> Hi Stas,
>>
>> On 05/14/2015 05:24 PM, Stas Sergeev wrote:
>>>
>>> The following sequence:
>>> echo timer >/sys/class/leds/<name>/trigger
>>> echo 1 >/sys/class/leds/<name>/brightness
>>> should change the ON brightness for blinking.
>>> The function led_set_brightness() was mistakenly initiating the
>>> delayed blink stop procedure, which resulted in no blinking with
>>> the timer trigger still active.
>>>
>>> This patch fixes the problem by changing led_set_brightness()
>>> to not initiate the delayed blink stop when brightness is not 0.
> 
> This commit message is not valid for this version of the patch.
Why do you think so?
---
-		schedule_work(&led_cdev->set_brightness_work);
+		if (brightness == LED_OFF)
+			schedule_work(&led_cdev->set_brightness_work);
---

LED_OFF is a 0 define.

Re: [PATCH v2] leds: fix brightness changing when software blinking is active

[Jacek Anaszewski]

On 05/15/2015 04:11 PM, Stas Sergeev wrote:
> 15.05.2015 17:09, Jacek Anaszewski пишет:
>> On 05/15/2015 10:12 AM, Jacek Anaszewski wrote:
>>> Hi Stas,
>>>
>>> On 05/14/2015 05:24 PM, Stas Sergeev wrote:
>>>>
>>>> The following sequence:
>>>> echo timer >/sys/class/leds/<name>/trigger
>>>> echo 1 >/sys/class/leds/<name>/brightness
>>>> should change the ON brightness for blinking.
>>>> The function led_set_brightness() was mistakenly initiating the
>>>> delayed blink stop procedure, which resulted in no blinking with
>>>> the timer trigger still active.
>>>>
>>>> This patch fixes the problem by changing led_set_brightness()
>>>> to not initiate the delayed blink stop when brightness is not 0.
>>
>> This commit message is not valid for this version of the patch.
> Why do you think so?
> ---
> -		schedule_work(&led_cdev->set_brightness_work);
> +		if (brightness == LED_OFF)
> +			schedule_work(&led_cdev->set_brightness_work);
> ---
>
> LED_OFF is a 0 define.
>

You're right, I was just looking at the issue from different
perspective.

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v2] leds: fix brightness changing when software blinking is active

[Bryan Wu]

On Fri, May 15, 2015 at 7:41 AM, Jacek Anaszewski
<j.anaszewski@samsung.com> wrote:
> On 05/15/2015 04:11 PM, Stas Sergeev wrote:
>>
>> 15.05.2015 17:09, Jacek Anaszewski пишет:
>>>
>>> On 05/15/2015 10:12 AM, Jacek Anaszewski wrote:
>>>>
>>>> Hi Stas,
>>>>
>>>> On 05/14/2015 05:24 PM, Stas Sergeev wrote:
>>>>>
>>>>>
>>>>> The following sequence:
>>>>> echo timer >/sys/class/leds/<name>/trigger
>>>>> echo 1 >/sys/class/leds/<name>/brightness
>>>>> should change the ON brightness for blinking.
>>>>> The function led_set_brightness() was mistakenly initiating the
>>>>> delayed blink stop procedure, which resulted in no blinking with
>>>>> the timer trigger still active.
>>>>>
>>>>> This patch fixes the problem by changing led_set_brightness()
>>>>> to not initiate the delayed blink stop when brightness is not 0.
>>>
>>>
>>> This commit message is not valid for this version of the patch.
>>
>> Why do you think so?
>> ---
>> -               schedule_work(&led_cdev->set_brightness_work);
>> +               if (brightness == LED_OFF)
>> +                       schedule_work(&led_cdev->set_brightness_work);
>> ---
>>
>> LED_OFF is a 0 define.
>>
>
> You're right, I was just looking at the issue from different
> perspective.
>

Applied into my tree. Thanks,
-Bryan

Re: [PATCH v2] leds: fix brightness changing when software blinking is active

[Stas Sergeev]

02.05.2015 17:59, Pavel Machek пишет:
> On Thu 2015-05-14 18:24:02, Stas Sergeev wrote:
>>
>> The following sequence:
>> echo timer >/sys/class/leds/<name>/trigger
>> echo 1 >/sys/class/leds/<name>/brightness
>> should change the ON brightness for blinking.
>> The function led_set_brightness() was mistakenly initiating the
>> delayed blink stop procedure, which resulted in no blinking with
>> the timer trigger still active.
>>
>> This patch fixes the problem by changing led_set_brightness()
>> to not initiate the delayed blink stop when brightness is not 0.
> 
> Could we get this part of API documented? It is quite non-obvious... 0 clears the trigger,
> other values do not, I thought it is a bug when I saw it...
I guess the thinking was that if ON brightness differs
from OFF brightness, you should not clear the trigger.
But if you put ON brightness similar to OFF brightness,
then you can as well stop blinking at all.
Not that I am amused by that kind of logic though.

Re: [PATCH v2] leds: fix brightness changing when software blinking is active

[Pavel Machek]

On Tue 2015-06-23 19:59:27, Stas Sergeev wrote:
> 02.05.2015 17:59, Pavel Machek пишет:
> > On Thu 2015-05-14 18:24:02, Stas Sergeev wrote:
> >>
> >> The following sequence:
> >> echo timer >/sys/class/leds/<name>/trigger
> >> echo 1 >/sys/class/leds/<name>/brightness
> >> should change the ON brightness for blinking.
> >> The function led_set_brightness() was mistakenly initiating the
> >> delayed blink stop procedure, which resulted in no blinking with
> >> the timer trigger still active.
> >>
> >> This patch fixes the problem by changing led_set_brightness()
> >> to not initiate the delayed blink stop when brightness is not 0.
> > 
> > Could we get this part of API documented? It is quite non-obvious... 0 clears the trigger,
> > other values do not, I thought it is a bug when I saw it...
> I guess the thinking was that if ON brightness differs
> from OFF brightness, you should not clear the trigger.
> But if you put ON brightness similar to OFF brightness,
> then you can as well stop blinking at all.

And none of this is documented. So what about this?

diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
index 3646ec8..b734d7f 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -4,16 +4,25 @@ KernelVersion:	2.6.17
 Contact:	Richard Purdie <rpurdie@rpsys.net>
 Description:
 		Set the brightness of the LED. Most LEDs don't
-		have hardware brightness support so will just be turned on for
+		have hardware brightness support, so will just be turned on for
 		non-zero brightness settings. The value is between 0 and
 		/sys/class/leds/<led>/max_brightness.
 
+		Writing 0 to this file clears active trigger.
+
+		Writing non-zero to this file while trigger is active changes the
+		top brightness trigger is going to use.
+		
+
 What:		/sys/class/leds/<led>/max_brightness
 Date:		March 2006
 KernelVersion:	2.6.17
 Contact:	Richard Purdie <rpurdie@rpsys.net>
 Description:
-		Maximum brightness level for this led, default is 255 (LED_FULL).
+		Maximum brightness level for this LED, default is 255 (LED_FULL).
+
+		If the LED does not support different brightness levels, this
+		should be 1.
 
 What:		/sys/class/leds/<led>/trigger
 Date:		March 2006
@@ -21,7 +30,7 @@ KernelVersion:	2.6.17
 Contact:	Richard Purdie <rpurdie@rpsys.net>
 Description:
 		Set the trigger for this LED. A trigger is a kernel based source
-		of led events.
+		of LED events.
 		You can change triggers in a similar manner to the way an IO
 		scheduler is chosen. Trigger specific parameters can appear in
 		/sys/class/leds/<led> once a given trigger is selected.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2] leds: fix brightness changing when software blinking is active

[Jacek Anaszewski]

On 06/24/2015 10:44 AM, Pavel Machek wrote:
> On Tue 2015-06-23 19:59:27, Stas Sergeev wrote:
>> 02.05.2015 17:59, Pavel Machek пишет:
>>> On Thu 2015-05-14 18:24:02, Stas Sergeev wrote:
>>>>
>>>> The following sequence:
>>>> echo timer >/sys/class/leds/<name>/trigger
>>>> echo 1 >/sys/class/leds/<name>/brightness
>>>> should change the ON brightness for blinking.
>>>> The function led_set_brightness() was mistakenly initiating the
>>>> delayed blink stop procedure, which resulted in no blinking with
>>>> the timer trigger still active.
>>>>
>>>> This patch fixes the problem by changing led_set_brightness()
>>>> to not initiate the delayed blink stop when brightness is not 0.
>>>
>>> Could we get this part of API documented? It is quite non-obvious... 0 clears the trigger,
>>> other values do not, I thought it is a bug when I saw it...
>> I guess the thinking was that if ON brightness differs
>> from OFF brightness, you should not clear the trigger.
>> But if you put ON brightness similar to OFF brightness,
>> then you can as well stop blinking at all.
>
> And none of this is documented. So what about this?

It is documented in Documentation/leds/leds-class.txt:

"However, if you set the brightness value to LED_OFF it will
also disable the timer trigger".

Nonetheless, obviously it should be also covered in the
ABI documentation.

>
> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> index 3646ec8..b734d7f 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -4,16 +4,25 @@ KernelVersion:	2.6.17
>   Contact:	Richard Purdie <rpurdie@rpsys.net>
>   Description:
>   		Set the brightness of the LED. Most LEDs don't
> -		have hardware brightness support so will just be turned on for
> +		have hardware brightness support, so will just be turned on for
>   		non-zero brightness settings. The value is between 0 and
>   		/sys/class/leds/<led>/max_brightness.
>
> +		Writing 0 to this file clears active trigger.
> +
> +		Writing non-zero to this file while trigger is active changes the
> +		top brightness trigger is going to use.

This currently will not be true e.g. for heartbeat trigger which
uses max_brightness.

> +		
> +
>   What:		/sys/class/leds/<led>/max_brightness
>   Date:		March 2006
>   KernelVersion:	2.6.17
>   Contact:	Richard Purdie <rpurdie@rpsys.net>
>   Description:
> -		Maximum brightness level for this led, default is 255 (LED_FULL).
> +		Maximum brightness level for this LED, default is 255 (LED_FULL).
> +
> +		If the LED does not support different brightness levels, this
> +		should be 1.
>
>   What:		/sys/class/leds/<led>/trigger
>   Date:		March 2006
> @@ -21,7 +30,7 @@ KernelVersion:	2.6.17
>   Contact:	Richard Purdie <rpurdie@rpsys.net>
>   Description:
>   		Set the trigger for this LED. A trigger is a kernel based source
> -		of led events.
> +		of LED events.
>   		You can change triggers in a similar manner to the way an IO
>   		scheduler is chosen. Trigger specific parameters can appear in
>   		/sys/class/leds/<led> once a given trigger is selected.
>


-- 
Best Regards,
Jacek Anaszewski