Skip to contentSkip to conversation

Enable backlight when trigger is activated

PATCH In review Pavel Machek 3 participants 7 messages Jul 18, 2019 – Jul 24, 2019

Subsystems (2)
Framebuffer layer LED
lore
Share & tools
lore

Pavel Machek

author
#0 lore raw

PATCH 1/1 Enable backlight when trigger is activated

Configuring backlight trigger from dts results in backlight off during boot. Machine looks dead upon boot, which is not good.

Fix that by enabling LED on trigger activation.

Signed-off-by Pavel Machek <[email protected]>
drivers/leds/trigger/ledtrig-backlight.c +3 -0
index 487577d..6e6bc78 100644
@@ -114,6 +114,8 @@ static int bl_trig_activate(struct led_classdev *led)
 	n->old_status = UNBLANK;
 	n->notifier.notifier_call = fb_notifier_callback;
 
+	led_set_brightness(led, LED_ON);
+
 	ret = fb_register_client(&n->notifier);
 	if (ret)
 		dev_err(led->dev, "unable to register backlight trigger\n");

@@ -126,6 +128,7 @@ static void bl_trig_deactivate(struct led_classdev *led)
 	struct bl_trig_notifier *n = led_get_trigger_data(led);
 
 	fb_unregister_client(&n->notifier);
+	led_set_brightness(led, LED_OFF);
 	kfree(n);
 }
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#1 lore raw

Hi Pavel,

The commit log is lacking the proper "leds: triggers: ".

Also...

Quoted 12 lines · Pavel Machek

Configuring backlight trigger from dts results in backlight off during boot. Machine looks dead upon boot, which is not good.

Fix that by enabling LED on trigger activation.

Signed-off-by Pavel Machek <[email protected]>
drivers/leds/trigger/ledtrig-backlight.c +0 -0
index 487577d..6e6bc78 100644

drivers/leds/trigger/ledtrig-backlight.c static int bl_trig_activate(struct led_classdev *led) · line 118

Quoted 5 lines
drivers/leds/trigger/ledtrig-backlight.c +2 -0
@@ -114,6 +114,8 @@ static int bl_trig_activate(struct led_classdev *led)
 	n->old_status = UNBLANK;
 	n->notifier.notifier_call = fb_notifier_callback;
 
+	led_set_brightness(led, LED_ON);
+

This looks fishy.

Maybe you should use a default-state = "keep" instead? (and you'll have to support it in the LED driver).

That'll give you proper "don't touch the LED if it was turned on" behavior, which is what you seem to want.

Regards,
Eze

author
#2 lore raw

Hi!

Quoted 15 lines

drivers/leds/trigger/ledtrig-backlight.c static int bl_trig_activate(struct led_classdev *led) · line 118

Quoted 12 lines

Configuring backlight trigger from dts results in backlight off during boot. Machine looks dead upon boot, which is not good.

Fix that by enabling LED on trigger activation.

+++ b/drivers/leds/trigger/ledtrig-backlight.c

drivers/leds/trigger/ledtrig-backlight.c +2 -0
@@ -114,6 +114,8 @@ static int bl_trig_activate(struct led_classdev *led)
 	n->old_status = UNBLANK;
 	n->notifier.notifier_call = fb_notifier_callback;
 
+	led_set_brightness(led, LED_ON);
+

This looks fishy.

Quoted 5 lines

Maybe you should use a default-state = "keep" instead? (and you'll have to support it in the LED driver).

That'll give you proper "don't touch the LED if it was turned on" behavior, which is what you seem to want.

Actually no, that's not what I want. LED should go on if the display is active, as soon as trigger is activated.

Unfortunately, I have see no good way to tell if the display is active (and display is usually active when trigger is activated).

Thanks,
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#3 lore raw

Hi Pavel,

Quoted 20 lines · Pavel Machek

Hi!

Quoted 18 lines

drivers/leds/trigger/ledtrig-backlight.c static int bl_trig_activate(struct led_classdev *led) · line 118

Quoted 12 lines

Configuring backlight trigger from dts results in backlight off during boot. Machine looks dead upon boot, which is not good.

Fix that by enabling LED on trigger activation.

+++ b/drivers/leds/trigger/ledtrig-backlight.c

drivers/leds/trigger/ledtrig-backlight.c +2 -0
@@ -114,6 +114,8 @@ static int bl_trig_activate(struct led_classdev *led)
 	n->old_status = UNBLANK;
 	n->notifier.notifier_call = fb_notifier_callback;
 
+	led_set_brightness(led, LED_ON);
+

This looks fishy.

Maybe you should use a default-state = "keep" instead? (and you'll have to support it in the LED driver).

Quoted 8 lines
Quoted 2 lines

That'll give you proper "don't touch the LED if it was turned on" behavior, which is what you seem to want.

Actually no, that's not what I want. LED should go on if the display is active, as soon as trigger is activated.

Unfortunately, I have see no good way to tell if the display is active (and display is usually active when trigger is activated).

default-state DT property can be also set to "on" (see Documentation/devicetree/bindings/leds/common.txt).

You could make use of LED_INIT_DEFAULT_TRIGGER flag and parse DT property in the activate op. Similar approach has been applied e.g. in ledtrig-pattern.c.

Signature · 2 lines
Best regards,
Jacek Anaszewski
author
#4 lore raw

Hi!

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines

This looks fishy.

Maybe you should use a default-state = "keep" instead? (and you'll have to support it in the LED driver).

That'll give you proper "don't touch the LED if it was turned on" behavior, which is what you seem to want.

Quoted 8 lines
Quoted 5 lines

Actually no, that's not what I want. LED should go on if the display is active, as soon as trigger is activated.

Unfortunately, I have see no good way to tell if the display is active (and display is usually active when trigger is activated).

default-state DT property can be also set to "on" (see Documentation/devicetree/bindings/leds/common.txt).

Ok, thanks for the hint, that could work. (I thought we were using default trigger to set the LED "on").

Now...this gives me option of 0% or 100% brightness, while best would be 10% brightness.... but I guess we can live with that ;-).

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
author
#5 lore raw
In reply to Jacek Anaszewski

Hi!

Quoted 16 lines
Quoted 16 lines
Quoted 15 lines

drivers/leds/trigger/ledtrig-backlight.c static int bl_trig_activate(struct led_classdev *led) · line 118

Quoted 7 lines

+++ b/drivers/leds/trigger/ledtrig-backlight.c

drivers/leds/trigger/ledtrig-backlight.c +2 -0
@@ -114,6 +114,8 @@ static int bl_trig_activate(struct led_classdev *led)
 	n->old_status = UNBLANK;
 	n->notifier.notifier_call = fb_notifier_callback;
 
+	led_set_brightness(led, LED_ON);
+

This looks fishy.

Maybe you should use a default-state = "keep" instead? (and you'll have to support it in the LED driver).

That'll give you proper "don't touch the LED if it was turned on" behavior, which is what you seem to want.

Quoted 8 lines
Quoted 5 lines

Actually no, that's not what I want. LED should go on if the display is active, as soon as trigger is activated.

Unfortunately, I have see no good way to tell if the display is active (and display is usually active when trigger is activated).

default-state DT property can be also set to "on" (see Documentation/devicetree/bindings/leds/common.txt).

Yes, except that it does not work with all drivers :-(. In particular, it does not work with lm3532.

We should really move more of the device tree parsing into core, so that there's one place to fix...
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#6 lore raw
Quoted 24 lines · Pavel Machek

Hi!

Quoted 22 lines
Quoted 21 lines
Quoted 10 lines

drivers/leds/trigger/ledtrig-backlight.c static int bl_trig_activate(struct led_classdev *led) · line 118

Quoted 7 lines

+++ b/drivers/leds/trigger/ledtrig-backlight.c

drivers/leds/trigger/ledtrig-backlight.c +2 -0
@@ -114,6 +114,8 @@ static int bl_trig_activate(struct led_classdev *led)
 	n->old_status = UNBLANK;
 	n->notifier.notifier_call = fb_notifier_callback;
 
+	led_set_brightness(led, LED_ON);
+

This looks fishy.

Quoted 5 lines

Maybe you should use a default-state = "keep" instead? (and you'll have to support it in the LED driver).

That'll give you proper "don't touch the LED if it was turned on" behavior, which is what you seem to want.

Actually no, that's not what I want. LED should go on if the display is active, as soon as trigger is activated.

Unfortunately, I have see no good way to tell if the display is active (and display is usually active when trigger is activated).

Quoted 8 lines
Quoted 2 lines

default-state DT property can be also set to "on" (see Documentation/devicetree/bindings/leds/common.txt).

Yes, except that it does not work with all drivers :-(. In particular, it does not work with lm3532.

We should really move more of the device tree parsing into core, so that there's one place to fix...

Right. We could have something similar to led_get_default_pattern(). led_get_default_state() ?

Signature · 2 lines
Best regards,
Jacek Anaszewski