| Message ID | 20190718190849.GA11409@amd (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable backlight when trigger is activated | expand |
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
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
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.
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
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
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() ?
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); }
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>