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

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

[Jacek Anaszewski]

Hi Jelle,

Thanks for the patch.

On 02/09/2017 03:00 PM, Jelle Martijn Kok wrote:
> Upto now the leds-pwm are either on or off at startup. This allows the
> dts to specify the startup brightness
> 
> Signed-off-by: Jelle Martijn Kok <jmkok-i3KCcyIX/XtmR6Xm/wNWPw@public.gmane.org>
> ---
>  drivers/leds/leds-pwm.c  | 5 ++++-
>  include/linux/leds_pwm.h | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index a9145aa..4799f36 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -97,7 +97,7 @@ static int led_pwm_add(struct device *dev, struct
> led_pwm_priv *priv,
>      led_data->active_low = led->active_low;
>      led_data->cdev.name = led->name;
>      led_data->cdev.default_trigger = led->default_trigger;
> -    led_data->cdev.brightness = LED_OFF;
> +    led_data->cdev.brightness = led->default_brightness;
>      led_data->cdev.max_brightness = led->max_brightness;
>      led_data->cdev.flags = LED_CORE_SUSPENDRESUME;
>  @@ -159,6 +159,9 @@ static int led_pwm_create_of(struct device *dev,
> struct led_pwm_priv *priv)
>          led.active_low = of_property_read_bool(child, "active-low");
>          of_property_read_u32(child, "max-brightness",
>                       &led.max_brightness);
> +        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?

>           ret = led_pwm_add(dev, priv, &led, child);
>          if (ret) {
> diff --git a/include/linux/leds_pwm.h b/include/linux/leds_pwm.h
> index a65e964..7e70438 100644
> --- a/include/linux/leds_pwm.h
> +++ b/include/linux/leds_pwm.h
> @@ -11,6 +11,7 @@ struct led_pwm {
>      u8         active_low;
>      unsigned     max_brightness;
>      unsigned    pwm_period_ns;
> +    unsigned     default_brightness;
>  };
>   struct led_pwm_platform_data {

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

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

[Pavel Machek]

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

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.

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

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

[Jelle Martijn Kok]

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)

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

Jelle

-- 
------------------------------------------------------------------------
You/Com Audiocommunicatie b.v.
Motorenweg 5k
2623CR Delft
The Netherlands
tel. : (+31)15 262 59 55
fax. : (+31)15 257 15 95
mail : jmkok-i3KCcyIX/XtmR6Xm/wNWPw@public.gmane.org
http : http://www.youcom.nl
------------------------------------------------------------------------

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

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

[Jacek Anaszewski]

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