[PATCH 0/2] leds: some issues with hardware blinking

[PATCH 0/2] leds: some issues with hardware blinking

[Antonio Ospite]

Hi,

having software fall-back for blinking when hardware blinking does not
work or is not available is really good, however there are some cases
which are not covered in the current implementation. See the two
following patches.

The first one is needed to make the "timer" trigger work at all when
hardware blinking is available.

The second one is more like an RFC, since I am not sure I like the way I
fixed it, but the issue is there: mixing hardware and software blinking
can leave the timer on when not needed.

Thanks,
   Antonio

Antonio Ospite (2):
  leds: save the delay values after a successful call to blink_set()
  leds: turn the blink_timer off before starting to blink

 drivers/leds/led-class.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

--
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[PATCH 1/2] leds: save the delay values after a successful call to blink_set()

[Antonio Ospite]

When calling the hardware blinking function implemented by blink_set(), 
the delay_on and delay_off values are not preserved across calls.

Fix that and make the "timer" trigger work as expected when hardware 
blinking is available.

BEFORE the fix:
  $ cd /sys/class/leds/someled
  $ echo timer > trigger
  $ cat delay_on delay_off
  0
  0
  $ echo 100 > delay_on
  $ cat delay_on delay_off
  0
  0
  $ echo 100 > delay_off
  $ cat delay_on delay_off
  0
  0

AFTER the fix:
  $ cd /sys/class/leds/someled
  $ echo timer > trigger
  $ cat delay_on delay_off
  0
  0
  $ echo 100 > delay_on
  $ cat delay_on delay_off
  100
  0
  $ echo 100 > delay_off
  $ cat delay_on delay_off
  100
  100

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/leds/led-class.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index d5a4ade..939f24a 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -267,8 +267,11 @@ void led_blink_set(struct led_classdev *led_cdev,
 		   unsigned long *delay_off)
 {
 	if (led_cdev->blink_set &&
-	    !led_cdev->blink_set(led_cdev, delay_on, delay_off))
+	    !led_cdev->blink_set(led_cdev, delay_on, delay_off)) {
+		led_cdev->blink_delay_on = *delay_on;
+		led_cdev->blink_delay_off = *delay_off;
 		return;
+	}
 
 	/* blink with 1 Hz as default if nothing specified */
 	if (!*delay_on && !*delay_off)
-- 
1.7.6.3

Re: [PATCH 1/2] leds: save the delay values after a successful call to blink_set()

[Johannes Berg]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2373 bytes --]

On Thu, 2011-10-06 at 22:03 +0000, Antonio Ospite wrote:
> When calling the hardware blinking function implemented by blink_set(),
> the delay_on and delay_off values are not preserved across calls.
> 
> Fix that and make the "timer" trigger work as expected when hardware
> blinking is available.
> 
> BEFORE the fix:
>   $ cd /sys/class/leds/someled
>   $ echo timer > trigger
>   $ cat delay_on delay_off
>   0
>   0
>   $ echo 100 > delay_on
>   $ cat delay_on delay_off
>   0
>   0
>   $ echo 100 > delay_off
>   $ cat delay_on delay_off
>   0
>   0
> 
> AFTER the fix:
>   $ cd /sys/class/leds/someled
>   $ echo timer > trigger
>   $ cat delay_on delay_off
>   0
>   0
>   $ echo 100 > delay_on
>   $ cat delay_on delay_off
>   100
>   0
>   $ echo 100 > delay_off
>   $ cat delay_on delay_off
>   100
>   100
> 
> Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
> ---
>  drivers/leds/led-class.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index d5a4ade..939f24a 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -267,8 +267,11 @@ void led_blink_set(struct led_classdev *led_cdev,
>                    unsigned long *delay_off)
>  {
>         if (led_cdev->blink_set &&
> -           !led_cdev->blink_set(led_cdev, delay_on, delay_off))
> +           !led_cdev->blink_set(led_cdev, delay_on, delay_off)) {
> +               led_cdev->blink_delay_on = *delay_on;
> +               led_cdev->blink_delay_off = *delay_off;
>                 return;
> +       }

Looks good to me. I suspect I created this bug because originally it
might have put the values into there first and then called blink_set().

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

johannes

--------------------------------------------------------------------------------------
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland 
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH] Revert "leds: save the delay values after a successful call to blink_set()"

[Johan Hovold]

This reverts commit 6123b0e274503a0d3588e84fbe07c9aa01bfaf5d.

The problem this patch intends to solve has already been fixed by commit
7a5caabd090b8f7 (drivers/leds/ledtrig-timer.c: fix broken sysfs delay
handling).

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---

Hi, 

A fix for the problem with the delay parameters not being stored is already in
3.1 (and stable). Rather than storing both values at every call to
led_blink_set only the updated value is stored in led_delay_{on,off}_store in
ledtrig-timer.c.

Thanks,
Johan

 drivers/leds/led-class.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 661b692..6d5628b 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -270,11 +270,8 @@ void led_blink_set(struct led_classdev *led_cdev,
 	del_timer_sync(&led_cdev->blink_timer);
 
 	if (led_cdev->blink_set &&
-	    !led_cdev->blink_set(led_cdev, delay_on, delay_off)) {
-		led_cdev->blink_delay_on = *delay_on;
-		led_cdev->blink_delay_off = *delay_off;
+	    !led_cdev->blink_set(led_cdev, delay_on, delay_off))
 		return;
-	}
 
 	/* blink with 1 Hz as default if nothing specified */
 	if (!*delay_on && !*delay_off)
-- 
1.7.6

Re: [PATCH] Revert "leds: save the delay values after a successful call to blink_set()"

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 2302 bytes --]

On Mon,  7 Nov 2011 11:36:37 +0100
Johan Hovold <jhovold@gmail.com> wrote:

> This reverts commit 6123b0e274503a0d3588e84fbe07c9aa01bfaf5d.
> 
> The problem this patch intends to solve has already been fixed by commit
> 7a5caabd090b8f7 (drivers/leds/ledtrig-timer.c: fix broken sysfs delay
> handling).
> 
> Signed-off-by: Johan Hovold <jhovold@gmail.com>

Sorry for not replying earlier, I was on vacation.

Thanks Johan, I've just verified the timer trigger works fine on 3.1
indeed even without my changes. But keep reading, please.

> ---
> 
> Hi, 
> 
> A fix for the problem with the delay parameters not being stored is already in
> 3.1 (and stable). Rather than storing both values at every call to
> led_blink_set only the updated value is stored in led_delay_{on,off}_store in
> ledtrig-timer.c.
>

I thought that this was better fixed in the led_blink_set() directly,
as delay_on and delay_off are led_cdev fields after all, not
necessarily specific to ledtrig-timer, what about other users of them?
For example I can see led_blink_set() used in: net/mac80211/led.c
and led_trigger_blink(), which uses led_blink_set() in:
drivers/power/power_supply_leds.c

Maybe I am missing something.

Regards,
   Antonio

> Thanks,
> Johan
> 
>  drivers/leds/led-class.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 661b692..6d5628b 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -270,11 +270,8 @@ void led_blink_set(struct led_classdev *led_cdev,
>  	del_timer_sync(&led_cdev->blink_timer);
>  
>  	if (led_cdev->blink_set &&
> -	    !led_cdev->blink_set(led_cdev, delay_on, delay_off)) {
> -		led_cdev->blink_delay_on = *delay_on;
> -		led_cdev->blink_delay_off = *delay_off;
> +	    !led_cdev->blink_set(led_cdev, delay_on, delay_off))
>  		return;
> -	}
>  
>  	/* blink with 1 Hz as default if nothing specified */
>  	if (!*delay_on && !*delay_off)
> -- 
> 1.7.6
> 
> 

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Revert "leds: save the delay values after a successful call to blink_set()"

[Johan Hovold]

On Wed, Nov 16, 2011 at 10:21:00PM +0100, Antonio Ospite wrote:
> On Mon,  7 Nov 2011 11:36:37 +0100
> Johan Hovold <jhovold@gmail.com> wrote:
> 
> > This reverts commit 6123b0e274503a0d3588e84fbe07c9aa01bfaf5d.
> > 
> > The problem this patch intends to solve has already been fixed by commit
> > 7a5caabd090b8f7 (drivers/leds/ledtrig-timer.c: fix broken sysfs delay
> > handling).
> > 
> > Signed-off-by: Johan Hovold <jhovold@gmail.com>
> 
> Sorry for not replying earlier, I was on vacation.
> 
> Thanks Johan, I've just verified the timer trigger works fine on 3.1
> indeed even without my changes. But keep reading, please.
> 
> > ---
> > 
> > Hi, 
> > 
> > A fix for the problem with the delay parameters not being stored is already in
> > 3.1 (and stable). Rather than storing both values at every call to
> > led_blink_set only the updated value is stored in led_delay_{on,off}_store in
> > ledtrig-timer.c.
> >
> 
> I thought that this was better fixed in the led_blink_set() directly,
> as delay_on and delay_off are led_cdev fields after all, not
> necessarily specific to ledtrig-timer, what about other users of them?
> For example I can see led_blink_set() used in: net/mac80211/led.c
> and led_trigger_blink(), which uses led_blink_set() in:
> drivers/power/power_supply_leds.c
> 
> Maybe I am missing something.

When ledtrig-timer was generalised to led_set_blink() with fallback to
software blinking, the blink_delay_{on,off} parameters were moved to
led_classdev and now have two, partly overlapping uses: to store the
ledtrig-timer delay parameters and to implement generic software
blinking.

Note that both uses are internal -- drivers implementing hardware
blinking through blink_set() receives both parameters at each call and
need not access these fields directly. And apart from the ledtrig-timer
sysfs interface there is no other way to read them back either.

In particular, there is currently no need to update them at each call to
led_blink_set() as far as I can see.

Thanks,
Johan

[PATCH 2/2] leds: turn the blink_timer off before starting to blink

[Antonio Ospite]

Depending on the implementation of the hardware blinking function in
blink_set(), the led can support hardware blinking for some values of
delay_on and delay_off and fall-back to software blinking for some other
values.

Turning off the blink_timer unconditionally before starting to blink 
make sure that a sequence like:

  OFF
  hardware blinking
  software blinking
  hardware blinking

does not leave the software blinking timer active.

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/leds/led-class.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 939f24a..a7f0b29 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -266,6 +266,8 @@ void led_blink_set(struct led_classdev *led_cdev,
 		   unsigned long *delay_on,
 		   unsigned long *delay_off)
 {
+	del_timer_sync(&led_cdev->blink_timer);
+
 	if (led_cdev->blink_set &&
 	    !led_cdev->blink_set(led_cdev, delay_on, delay_off)) {
 		led_cdev->blink_delay_on = *delay_on;
-- 
1.7.6.3

Re: [PATCH 2/2] leds: turn the blink_timer off before starting to blink

[Johannes Berg]

On Thu, 2011-10-06 at 22:03 +0000, Antonio Ospite wrote:
> Depending on the implementation of the hardware blinking function in
> blink_set(), the led can support hardware blinking for some values of
> delay_on and delay_off and fall-back to software blinking for some other
> values.
> 
> Turning off the blink_timer unconditionally before starting to blink
> make sure that a sequence like:
> 
>   OFF
>   hardware blinking
>   software blinking
>   hardware blinking
> 
> does not leave the software blinking timer active.
> 
> Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
> ---
>  drivers/leds/led-class.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 939f24a..a7f0b29 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -266,6 +266,8 @@ void led_blink_set(struct led_classdev *led_cdev,
>                    unsigned long *delay_on,
>                    unsigned long *delay_off)
>  {
> +       del_timer_sync(&led_cdev->blink_timer);
> +
>         if (led_cdev->blink_set &&
>             !led_cdev->blink_set(led_cdev, delay_on, delay_off)) {
>                 led_cdev->blink_delay_on = *delay_on;

Makes sense, good catch!

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

johannes