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.
+ (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
--
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
