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

[Jacek Anaszewski]

On 05/13/2015 04:20 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. The callers that need
> a blink stop, were updated to explicitly call led_stop_software_blink().
>
> CC: Bryan Wu <cooloney@xxxxxxxxx>
> CC: Richard Purdie <rpurdie@xxxxxxxxx>
> CC: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>
> CC: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> CC: linux-leds@xxxxxxxxxxxxxxx
> CC: linux-kernel@xxxxxxxxxxxxxxx
>
> Signed-off-by: Stas Sergeev <stsp@xxxxxxxxxxxxxxxxxxxxx>
> ---
>   drivers/leds/led-class.c               |    5 +++++
>   drivers/leds/led-core.c                |    3 +--
>   drivers/leds/trigger/ledtrig-oneshot.c |    1 +
>   drivers/leds/trigger/ledtrig-timer.c   |    2 ++
>   4 files changed, 9 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 9886dac..3210020 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -119,10 +119,9 @@ void led_set_brightness(struct led_classdev *led_cdev,
>   {
>   	int ret = 0;
>
> -	/* delay brightness setting if need to stop soft-blink timer */
> +	/* delay brightness setting to next soft-blink */

How about:

	/* delay brightness setting if soft-blink is activated */

>   	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>   		led_cdev->delayed_set_value = brightness;
> -		schedule_work(&led_cdev->set_brightness_work);

This line is still required. Please refer to the patch d23a22a74.
The author however mistakenly assumes that setting brightness while
soft blink is activated should result in disabling the trigger. This is
contrary to the documentation, which says:

"You can change the brightness value of a LED independently of the timer
trigger. However, if you set the brightness value to LED_OFF it will
also disable the timer trigger."

What needs tweaking here is set_brightness_delayed function, which
should call led_stop_software_blink only if delayed_set_value is 0.

Please fix the patch accordingly.

>   		return;
>   	}
>
> diff --git a/drivers/leds/trigger/ledtrig-oneshot.c b/drivers/leds/trigger/ledtrig-oneshot.c
> index fbd02cd..c1bff10 100644
> --- a/drivers/leds/trigger/ledtrig-oneshot.c
> +++ b/drivers/leds/trigger/ledtrig-oneshot.c
> @@ -177,6 +177,7 @@ static void oneshot_trig_deactivate(struct led_classdev *led_cdev)
>   	}
>
>   	/* Stop blinking */
> +	led_stop_software_blink(led_cdev);

This won't be needed.

>   	led_set_brightness(led_cdev, LED_OFF);
>   }
>
> diff --git a/drivers/leds/trigger/ledtrig-timer.c b/drivers/leds/trigger/ledtrig-timer.c
> index 8d09327..3577c23 100644
> --- a/drivers/leds/trigger/ledtrig-timer.c
> +++ b/drivers/leds/trigger/ledtrig-timer.c
> @@ -17,6 +17,7 @@
>   #include <linux/device.h>
>   #include <linux/ctype.h>
>   #include <linux/leds.h>
> +#include "../leds.h"
>
>   static ssize_t led_delay_on_show(struct device *dev,
>   		struct device_attribute *attr, char *buf)
> @@ -103,6 +104,7 @@ static void timer_trig_deactivate(struct led_classdev *led_cdev)
>   	}
>
>   	/* Stop blinking */
> +	led_stop_software_blink(led_cdev);

This won't be needed.

>   	led_set_brightness(led_cdev, LED_OFF);
>   }


--
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/