Re: Brightness control irrespective of blink state.

From: Jacek Anaszewski
Date: Mon May 16 2016 - 10:23:23 EST

On 05/16/2016 03:43 PM, Tony Makkiel wrote:


On 16/05/16 10:21, Jacek Anaszewski wrote:
Hi Tony,

On 05/13/2016 04:20 PM, Tony Makkiel wrote:


On 12/05/16 11:26, Jacek Anaszewski wrote:
On 05/11/2016 03:42 PM, Tony Makkiel wrote:


On 11/05/16 10:41, Jacek Anaszewski wrote:
On 05/10/2016 06:55 PM, Tony Makkiel wrote:


On 10/05/16 14:26, Jacek Anaszewski wrote:
On 05/10/2016 11:36 AM, Tony Makkiel wrote:


On 09/05/16 15:45, Jacek Anaszewski wrote:
Hi Tony,

On 05/09/2016 03:27 PM, Tony Makkiel wrote:
Hi Jacek,
Thank you for getting back. I updated my kernel to 4.5 and
have
the
updated "led_set_brightness" now.

It sets
led_cdev->flags |= LED_BLINK_BRIGHTNESS_CHANGE;
led_cdev->blink_brightness = brightness;

The new implementation requires hardware specific
drivers to
poll
for flag change. Shouldn't the led-core driver be calling the
hardware
specific brightness_set (led_set_brightness_nosleep)
irrespective of
the
blink settings?

Unfortunately, it place additional requirement on drivers, to
implement
a polling mechanism which won't be needed otherwise. Why are the
brightness calls dependent on blink settings?

If your question is still:

"Is there a reason for rejecting brightness change requests when
either of the blink_delays are set?"

then the answer is: yes, brightness setting is deferred until
the next timer tick to avoid avoid problems in case we are called
from hard irq context. It should work fine for software blinking.


Sorry, was focused debugging 'hardware accelerated blink' on the
driver
I am working on, I missed the software blinking implementation.


Nonetheless, your question, made it obvious that we have a
problem
here in case of hardware accelerated blinking, i.e. drivers that
implement blink_set op. Is this your use case?


Yes, the brightness requests from user space
(/sys/class/leds/*/brightness) does not get passed to hardware
specific
driver via the blink_set implemented, unless led_cdev->flags is
polled.

Anyway, I've noticed a discrepancy between the LED core code and
both Documentation/leds/leds-class.txt and comment over
blink_set() op
in include/linux/leds.h, which say that blinking is deactivated
upon setting the brightness again. Many drivers apply this rule.

In effect, LED_BLINK_BRIGHTNESS_CHANGE will have to be removed,
and your question will be groundless, as changing the blink
brightness should be impossible by design.

In my opinion, disabling blink, when brightness change requested
doesn't
sound like the right thing to do. There could be cases in which
the
brightness of the blinking LED needs to be changed.

It could be accomplished with following sequence:

$ echo LED_FULL > brightness //set brightness
$ echo "timer" > trigger //enable blinking with
brightness=LED_FULL
$ echo 1 > brightness //stop blinking and set brightness
to 1
$ echo "timer" > trigger //enable blinking with brightness=1

The only drawback here would be interfered blinking rhythm while
resetting blink brightness. Most drivers that implement blink_set
op observe what documentation says and disable blinking when
new brightness is set. Unfortunately, led_set_brightness() after
modifications doesn't take into account drivers that implement
blink_set op. It needs to be fixed, i.e. made compatible with
the docs.

Maybe we can let the
hardware driver deal with the blink request if it has implemented
brightness_set? The change below seem to work.


Subject: [PATCH] led-core: Use hardware blink when available

At present hardware implemented brightness is not called
if blink delays are set.

Signed-off-by: Tony Makkiel <tony.makkiel@xxxxxxxxx>
---
drivers/leds/led-core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 19e1e60d..02dd0f6 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -208,8 +208,10 @@ void led_set_brightness(struct led_classdev
*led_cdev,
/*
* In case blinking is on delay brightness setting
* until the next timer tick.
+ * Or if brightness_set is defined, use the associated
implementation.
*/
- if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
+ if ((!led_cdev->brightness_set) &&

s/brightness_set/blink_set/ AFAICT

It wouldn't cover all cases as the fact that a driver implements
blink_set doesn't necessarily mean that hardware blinking is used
for current blinking parameters. There are drivers that resort to
using software fallback in case the LED controller device doesn't
support requested blink intervals.

I'm planning on addition of a LED_BLINKING_HW flag, that would
be set after successful execution of blink_set op. It would
allow to
distinguish between hardware and software blinking modes reliably.


Did you mean something like


diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 19e1e60d..4a8b46d 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -210,6 +210,13 @@ void led_set_brightness(struct led_classdev
*led_cdev,
* until the next timer tick.
*/
if (led_cdev->blink_delay_on ||
led_cdev->blink_delay_off) {
+ if (led_cdev->brightness_set)
+ led_set_brightness_nosleep(led_cdev,
brightness);

brightness_set is always initialized, probably you meant blink_set.

Your solution from your last comment below sounds better (so probably
can skip reading this section).

But no, It was not a mistake. Actually, copied from
'led_set_brightness_nopm' in case to protect from any drivers which
doesn't define it. The change should follow existing architecture, and
was hoping to work for both existing and new drivers.

Right, brightness_set can remain uninitialized while
brightness_set_blocking is provided.

Existing Chip drivers: Note, the added brightness_set is called only
when the blink is active. The flag LED_BLINKING_HW won't be set,
either
because driver is not updated to include the flag, or driver wants
software fallback to deal with it. The problems I can think of is

- the software flow will also call brightness_set later when the task
is serviced. But that is with the same values. So shouldn't make
difference to the user.

New Drivers with brightness support while blinking:- They set
brightness
and the flag. They wont need the software fallback. If for any reason
brightness couldn't be set, flag is not set and normal procedure
applies. Again problem I see here is

- Additional responsibility on chip drivers to set the flag, if

Ah, now I understand your approach.

successfully managed to set brightness while blink is active. This
shouldn't be a problem on new drivers, as they might just set the flag
every time brightness change is requested, irrespective of blink
settings. If they don't set the flag, it falls back to the software
flow
as in existing architecture.


+
+ if (led_cdev->flags & LED_BLINKING_HW) {
+ led_cdev->flags &= ~LED_BLINKING_HW;
+ return;
+ }

The dependencies are quite versatile if we wanted to properly
implement
what documentation claims. Setting brightness to any value while
blinking is on should stop blinking. It was so before the commit:

76931edd5 ('leds: fix brightness changing when software blinking is
active')

The problem was that timer trigger remained active after stopping
blinking, which led us to changing the semantics on new brightness
set, rather than solving the problem with unregistered trigger.
This was also against documentation claims, which was overlooked.

I tried yesterday to deactivate trigger on brightness set
executed during blinking, but there are circular dependencies,
since led_set_brightness(led_cdev, LED_OFF) is called on
deactivation.
It is also called from led_trigger_set in the trigger removal path.
Generally it seems non-trivial to enforce current documentation
claims
in case of software blinking.

The easiest thing to do now would be changing the semantics, so that
only setting brightness to LED_OFF would disable the trigger, which
in fact is true since few releases. The problem is that many drivers
that implement hardware blinking resets it on any brightness change.
We would have to left them intact, but apply a new semantics in the
LED core, that would allow for new drivers to just update hardware
blinking brightness upon updating the brightness.

If we followed this path then the LED_BLINKING_HW flag would have to
be set in led_blink_setup() after blink_set op returns 0. Thanks to
that, we could distinguish in led_set_brightness whether hardware
or software blinking is enabled. For !LED_BLINKING_HW case we would
proceed as currently, i.e. set the LED_BLINK_BRIGHTNESS_CHANGE flag
and defer brightness setting until next timer tick. For the opposite
case we wouldn't do anything and let the led_set_brightness_nosleep()
to call the appropriate brightness_set/brightness_set_blocking op.
Old drivers would proceed as currently, by disabling blinking
on brightness change, and new ones could apply new semantics by
changing brightness but leaving hardware blinking active.


This sounds better as we do not have to rely on drivers to set the
flag
and does not have the problems mentioned above. I tried the following
and works for my set up :) .


diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 19e1e60d..3698b67 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -141,8 +141,10 @@ static void led_blink_setup(struct led_classdev
*led_cdev,
{
if (!(led_cdev->flags & LED_BLINK_ONESHOT) &&
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->flags |= LED_BLINKING_HW;
return;
+ }

/* blink with 1 Hz as default if nothing specified */
if (!*delay_on && !*delay_off)
@@ -209,7 +211,8 @@ void led_set_brightness(struct led_classdev
*led_cdev,
* In case blinking is on delay brightness setting
* until the next timer tick.
*/
- if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
+ if (!(led_cdev->flags & LED_BLINKING_HW) &&
+ (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) {
/*
* If we need to disable soft blinking delegate
this to
the
* work queue task to avoid problems in case we are
called

We would have to also clear the flag upon blink deactivation, i.e.
when brightness to be set equals LED_OFF. Existing drivers that
implement blink_set op and deactivate blinking on any brightness set
would have to be modified to clear the LED_BLINKING_HW flag in their
brightness_{set|set_blocking} ops.



diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 19e1e60d..7a15035 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -98,6 +98,9 @@ static void set_brightness_delayed(struct work_struct
*ws)

led_cdev->delayed_set_value);
else
ret = -ENOTSUPP;
+
+ if (led_cdev->delayed_set_value == LED_OFF)
+ led_cdev->flags &= ~LED_BLINKING_HW;
if (ret < 0)
dev_err(led_cdev->dev,
"Setting an LED's brightness failed (%d)\n",
ret);
@@ -141,8 +144,10 @@ static void led_blink_setup(struct led_classdev
*led_cdev,
{
if (!(led_cdev->flags & LED_BLINK_ONESHOT) &&
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->flags |= LED_BLINKING_HW;
return;
+ }

/* blink with 1 Hz as default if nothing specified */
if (!*delay_on && !*delay_off)
@@ -209,7 +214,8 @@ void led_set_brightness(struct led_classdev
*led_cdev,
* In case blinking is on delay brightness setting
* until the next timer tick.
*/
- if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
+ if (!(led_cdev->flags & LED_BLINKING_HW) &&
+ (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) {
/*
* If we need to disable soft blinking delegate this to
the
* work queue task to avoid problems in case we are
called
@@ -235,6 +241,8 @@ void led_set_brightness_nopm(struct led_classdev
*led_cdev,
/* Use brightness_set op if available, it is guaranteed not to
sleep */
if (led_cdev->brightness_set) {
led_cdev->brightness_set(led_cdev, value);
+ if (value == LED_OFF)
+ led_cdev->flags &= ~LED_BLINKING_HW;
return;
}

@@ -267,6 +275,9 @@ int led_set_brightness_sync(struct led_classdev
*led_cdev,
if (led_cdev->flags & LED_SUSPENDED)
return 0;

+ if (value == LED_OFF)
+ led_cdev->flags &= ~LED_BLINKING_HW;
+
if (led_cdev->brightness_set_blocking)
return led_cdev->brightness_set_blocking(led_cdev,

led_cdev->brightness);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index bc1476f..f5fa566 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -48,6 +48,7 @@ struct led_classdev {
#define LED_BLINK_DISABLE (1 << 21)
#define LED_SYSFS_DISABLE (1 << 22)
#define LED_DEV_CAP_FLASH (1 << 23)
+#define LED_BLINKING_HW (1 << 24)

/* Set LED brightness level */
/* Must not sleep, use a workqueue if needed */
~


diff --git a/include/linux/leds.h b/include/linux/leds.h
index bc1476f..f5fa566 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -48,6 +48,7 @@ struct led_classdev {
#define LED_BLINK_DISABLE (1 << 21)
#define LED_SYSFS_DISABLE (1 << 22)
#define LED_DEV_CAP_FLASH (1 << 23)
+#define LED_BLINKING_HW (1 << 24)

/* Set LED brightness level */
/* Must not sleep, use a workqueue if needed */

I wonder if it would be safe to rely on timer_pending() instead of
introducing LED_BLINKING_HW flag...

How would that work? I am assuming this has something to do with
software blink? Does it take hardware blink to account?

This way we could distinguish between software and hardware blinking.
It wouldn't require modifications in drivers:

- if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
+ if (timer_pending(&led_cdev->blink_timer) &&
+ (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) {
/*
* If we need to disable soft blinking delegate this

It'd must be verified however if it isn't possible that
led_set_brightness is called when timer is already expired,
between two ticks.

if blink_set is successful, would work without problem,

When blink_set successful : The timer wont be triggered resulting the
function to return null all the time. --> No problem here

Software blink : I do not have hardware to actually test this case. I
tried simulating the case.But going through the code. Following are my
understanding.

You can test it by leaving blink_set op uninitialized in your driver.


Timer Active (Most of the time) : Work as normal. --> No problem here

Timer Expired :led_set_brightness_nopm called.
- Case in which brightness == LED_OFF, LED will be turned off.
led_cdev->blink_delay_on and delay_off will retain its value. The timer
will keep on running. So it will re-enable back blink. --> LED_OFF will
be ignored.

- Case in which brightness != LED_OFF, new brightness set and resume
normal operation. --> No problem here


Thanks for this analysis. I have a new idea - wouldn't it be more robust
if we added LED_BLINKING_SW flag and set it in led_set_software_blink()?

The line

if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)

in led_set_brightness() could be then changed to

if (led_cdev->flags & LED_BLINK_SW) .

LED_BLINK_SW flag would have to be cleared in led_stop_software_blink()
and in the first two conditions in the led_timer_function().


Yes, that will do with minimal changes. I tested the following, and works.

Fine, so could you please submit the patch officially?
Before that, please rebase your code on top of LED tree or linux-next
and change LED_BLINKING_SW to LED_BLINK_SW, to keep the same prefix for
each blinking related definition.

Date: Mon, 16 May 2016 14:18:42 +0100
Subject: [PATCH 1/1] Allow chip-driver to set brightness if, software
blink not used.

If software blink were active any brightness change request
were not sent to the chip driver.

Signed-off-by: Tony Makkiel <tony.makkiel@xxxxxxxxx>
---
drivers/leds/led-core.c | 7 +++++--
include/linux/leds.h | 1 +
2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 19e1e60d..376b5ea 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -33,11 +33,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_BLINKING_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_BLINKING_SW);
return;
}

@@ -131,6 +132,7 @@ static void led_set_software_blink(struct
led_classdev *led_cdev,
return;
}

+ led_cdev->flags |= LED_BLINKING_SW;
mod_timer(&led_cdev->blink_timer, jiffies + 1);
}

@@ -199,6 +201,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_BLINKING_SW;
}
EXPORT_SYMBOL_GPL(led_stop_software_blink);

@@ -209,7 +212,7 @@ void led_set_brightness(struct led_classdev *led_cdev,
* In case blinking is on delay brightness setting
* until the next timer tick.
*/
- if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
+ if (led_cdev->flags & LED_BLINKING_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 bc1476f..08ef6f4 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -48,6 +48,7 @@ struct led_classdev {
#define LED_BLINK_DISABLE (1 << 21)
#define LED_SYSFS_DISABLE (1 << 22)
#define LED_DEV_CAP_FLASH (1 << 23)
+#define LED_BLINKING_SW (1 << 24)

/* Set LED brightness level */
/* Must not sleep, use a workqueue if needed */

The above line looks different in the recent code.


--
Best regards,
Jacek Anaszewski