Hi Jelle.

On 02/14/2017 11:34 AM, Jelle Martijn Kok wrote:

On 10-02-17 21:56, Pavel Machek wrote:

quoted

Hi!

quoted

quoted

+        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