Re: [PATCH] leds-pwm: the startup brightness can be specified

[Jacek Anaszewski <jacek.anaszewski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi Jelle.

On 02/14/2017 11:34 AM, Jelle Martijn Kok wrote:
> 
> On 10-02-17 21:56, Pavel Machek wrote:
>> Hi!
>>
>>>> +        led.default_brightness = LED_OFF;
>>>> +        of_property_read_u32(child, "brightness",
>>>> +                     &led.default_brightness);
>>> At first you would have to submit a patch for
>>> Documentation/devicetree/bindings/leds/common.txt that would add
>>> brightness property. The question is whether it is really needed?
>>> You can set brightness from userspace via sysfs API.
>>>
>>> By the way, I have a question to DT maintainers: is DT a proper
>>> place for defining this type of configuration that can be set via
>>> userspace scripts? Shouldn't DT describe only hardware properties and
>>> constraints resulting from board configuration?
>> Well, if the hardware has label "half - power, full - transmitting" on
>> a LED, we might want kernel to turn it to half power on bootup.
>>
>> If you have a "disk activity LED" on a PC, it is driven by
>> hardware. On arm notebook, it would be nice if "disk activity LED"
>> worked, too. Preferably even when running fsck in init=/bin/bash
>> mode. We already provide that, AFAICT, so having ability to set
>> constant brightness sounds sane to me.
> There is also some delay between kernel and user space where the leds
> are too bright or simply off... (which is quite ugly for a simple "power
> on" led)

It seems to be sufficient justification for introducing the property.
Please submit a patch adding DT documentation for it. I'd call it
default-brightness to match the one used already in backlight subsystem.

> One thing that I have seen is that several led triggers do not play
> along with the brightness. For example, the "heartbeat" trigger always
> sets the brightness to LED_FULL. Which negates the set brightness on
> each blink... 

I posted the patch [0] addressing that few months ago, but I must have
forgotten to reapply it to linux-leds.git after previously withdrawing
it due to set_brightness_delayed work locking issues.

I've just applied this patch to the for-next branch of linux-leds.git.

> So would it not be an idea to add a "led_set_on_off(led,
> state)" function which retains "brightness" and additionally takes an
> "invert" parameter into account. This might also simplify code in other
> led triggers.

[0] https://patchwork.kernel.org/patch/9418701/

-- 
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html