Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] Enable backlight when trigger is activated
@ 2019-07-18 19:08 Pavel Machek
  2019-07-21 23:00 ` Ezequiel Garcia
  0 siblings, 1 reply; 7+ messages in thread
From: Pavel Machek @ 2019-07-18 19:08 UTC (permalink / raw)
  To: kernel list, linux-arm-kernel, linux-omap, tony, sre, nekit1000,
	mpartap, merlijn, jacek.anaszewski, linux-leds, b.zolnierkie,
	dri-devel, linux-fbdev

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

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 <pavel@ucw.cz>

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Enable backlight when trigger is activated
  2019-07-18 19:08 [PATCH] Enable backlight when trigger is activated Pavel Machek
@ 2019-07-21 23:00 ` Ezequiel Garcia
  2019-07-22  7:50   ` Pavel Machek
  0 siblings, 1 reply; 7+ messages in thread
From: Ezequiel Garcia @ 2019-07-21 23:00 UTC (permalink / raw)
  To: Pavel Machek, kernel list, linux-arm-kernel, linux-omap, tony,
	sre, nekit1000, mpartap, merlijn, jacek.anaszewski, linux-leds,
	b.zolnierkie, dri-devel, linux-fbdev

Hi Pavel,

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

Also...

On Thu, 2019-07-18 at 21:08 +0200, Pavel Machek wrote:
> 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 <pavel@ucw.cz>
> 
> 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);
> +

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Enable backlight when trigger is activated
  2019-07-21 23:00 ` Ezequiel Garcia
@ 2019-07-22  7:50   ` Pavel Machek
  2019-07-22 20:25     ` Jacek Anaszewski
  0 siblings, 1 reply; 7+ messages in thread
From: Pavel Machek @ 2019-07-22  7:50 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: kernel list, linux-arm-kernel, linux-omap, tony, sre, nekit1000,
	mpartap, merlijn, jacek.anaszewski, linux-leds, b.zolnierkie,
	dri-devel, linux-fbdev

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

Hi!

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Enable backlight when trigger is activated
  2019-07-22  7:50   ` Pavel Machek
@ 2019-07-22 20:25     ` Jacek Anaszewski
  2019-07-22 21:04       ` Pavel Machek
  2019-07-24  8:33       ` Pavel Machek
  0 siblings, 2 replies; 7+ messages in thread
From: Jacek Anaszewski @ 2019-07-22 20:25 UTC (permalink / raw)
  To: Pavel Machek, Ezequiel Garcia
  Cc: kernel list, linux-arm-kernel, linux-omap, tony, sre, nekit1000,
	mpartap, merlijn, linux-leds, b.zolnierkie, dri-devel,
	linux-fbdev

Hi Pavel,

On 7/22/19 9:50 AM, Pavel Machek wrote:
> Hi!
> 
>>> 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
>>> @@ -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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Enable backlight when trigger is activated
  2019-07-22 20:25     ` Jacek Anaszewski
@ 2019-07-22 21:04       ` Pavel Machek
  2019-07-24  8:33       ` Pavel Machek
  1 sibling, 0 replies; 7+ messages in thread
From: Pavel Machek @ 2019-07-22 21:04 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Ezequiel Garcia, kernel list, linux-arm-kernel, linux-omap, tony,
	sre, nekit1000, mpartap, merlijn, linux-leds, b.zolnierkie,
	dri-devel, linux-fbdev

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

Hi!

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Enable backlight when trigger is activated
  2019-07-22 20:25     ` Jacek Anaszewski
  2019-07-22 21:04       ` Pavel Machek
@ 2019-07-24  8:33       ` Pavel Machek
  2019-07-24 21:16         ` Jacek Anaszewski
  1 sibling, 1 reply; 7+ messages in thread
From: Pavel Machek @ 2019-07-24  8:33 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Ezequiel Garcia, kernel list, linux-arm-kernel, linux-omap, tony,
	sre, nekit1000, mpartap, merlijn, linux-leds, b.zolnierkie,
	dri-devel, linux-fbdev

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

Hi!

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Enable backlight when trigger is activated
  2019-07-24  8:33       ` Pavel Machek
@ 2019-07-24 21:16         ` Jacek Anaszewski
  0 siblings, 0 replies; 7+ messages in thread
From: Jacek Anaszewski @ 2019-07-24 21:16 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Ezequiel Garcia, kernel list, linux-arm-kernel, linux-omap, tony,
	sre, nekit1000, mpartap, merlijn, linux-leds, b.zolnierkie,
	dri-devel, linux-fbdev

On 7/24/19 10:33 AM, Pavel Machek wrote:
> Hi!
> 
>>>>> +++ 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-18 19:08 [PATCH] Enable backlight when trigger is activated Pavel Machek
2019-07-21 23:00 ` Ezequiel Garcia
2019-07-22  7:50   ` Pavel Machek
2019-07-22 20:25     ` Jacek Anaszewski
2019-07-22 21:04       ` Pavel Machek
2019-07-24  8:33       ` Pavel Machek
2019-07-24 21:16         ` Jacek Anaszewski

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org linux-leds@archiver.kernel.org
	public-inbox-index linux-leds


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/ public-inbox