[PATCH] 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 <redacted>

diff --git a/drivers/leds/trigger/ledtrig-backlight.c b/drivers/leds/trigger/ledtrig-backlight.c
index 487577d..6e6bc78 100644
--- a/drivers/leds/trigger/ledtrig-backlight.c
+++ b/drivers/leds/trigger/ledtrig-backlight.c

@@ -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);
 }
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Hi Pavel,

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

Also...

On Thu, 2019-07-18 at 21:08 +0200, Pavel Machek wrote:

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

Hi!

quoted

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.

quoted

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

@@ -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.

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Hi Pavel,

On 7/22/19 9:50 AM, Pavel Machek wrote:

Hi!

quoted

quoted

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.

quoted

quoted

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

@@ -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.

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.

-- 
Best regards,
Jacek Anaszewski

Hi!

quoted

quoted

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.

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Hi!

quoted

quoted

quoted

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

@@ -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.

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

On 7/24/19 10:33 AM, Pavel Machek wrote:

Hi!

quoted

quoted

quoted

quoted

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

@@ -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.

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

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

-- 
Best regards,
Jacek Anaszewski