[PATCH v1] leds: lp50xx: add setting of default intensity from DT

[PATCH v1] leds: lp50xx: add setting of default intensity from DT

[Sven Schuchmann]

In order to use a multicolor-led together with a trigger
from DT the led needs to have an intensity set to see something.
The trigger changes the brightness of the led but if there
is no intensity we actually see nothing.

This patch adds the ability to set the default intensity
of each led so that it is turned on from DT.

Signed-off-by: Sven Schuchmann <schuchmann@schleissheimer.de>
---
 Documentation/devicetree/bindings/leds/leds-lp50xx.yaml | 8 +++++++-
 drivers/leds/leds-lp50xx.c                              | 4 ++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
index c192b5feadc7..5ad2a0c3c052 100644
--- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
@@ -69,7 +69,12 @@ patternProperties:
     patternProperties:
       "(^led-[0-9a-f]$|led)":
         type: object
-        $ref: common.yaml#
+        allOf:
+          - $ref: common.yaml#
+        properties:
+          default-intensity:
+            maxItems: 1
+            description: The intensity the LED gets initialised with.
 
 required:
   - compatible
@@ -102,6 +107,7 @@ examples:
 
                led-0 {
                    color = <LED_COLOR_ID_RED>;
+                   default-intensity = <100>;
                };
 
                led-1 {
diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
index f13117eed976..ba760fa33bdc 100644
--- a/drivers/leds/leds-lp50xx.c
+++ b/drivers/leds/leds-lp50xx.c
@@ -501,6 +501,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
 			}
 
 			mc_led_info[num_colors].color_index = color_id;
+
+			fwnode_property_read_u32(led_node, "default-intensity",
+						 &mc_led_info[num_colors].intensity);
+
 			num_colors++;
 		}
 
-- 
2.17.1

AW: [PATCH v1] leds: lp50xx: add setting of default intensity from DT

[Sven Schuchmann]

Hi,
sorry to ask but was someone able to look at this? Any thoughts?

Best Regards,

   Sven

> -----Ursprüngliche Nachricht-----
> Von: Sven Schuchmann <schuchmann@schleissheimer.de>
> Gesendet: Dienstag, 19. Januar 2021 11:53
> An: Sven Schuchmann <schuchmann@schleissheimer.de>
> Cc: Pavel Machek <pavel@ucw.cz>; Dan Murphy <dmurphy@ti.com>; Rob Herring <robh+dt@kernel.org>; linux-
> leds@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Betreff: [PATCH v1] leds: lp50xx: add setting of default intensity from DT
> 
> In order to use a multicolor-led together with a trigger
> from DT the led needs to have an intensity set to see something.
> The trigger changes the brightness of the led but if there
> is no intensity we actually see nothing.
> 
> This patch adds the ability to set the default intensity
> of each led so that it is turned on from DT.
> 
> Signed-off-by: Sven Schuchmann <schuchmann@schleissheimer.de>
> ---
>  Documentation/devicetree/bindings/leds/leds-lp50xx.yaml | 8 +++++++-
>  drivers/leds/leds-lp50xx.c                              | 4 ++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> index c192b5feadc7..5ad2a0c3c052 100644
> --- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> @@ -69,7 +69,12 @@ patternProperties:
>      patternProperties:
>        "(^led-[0-9a-f]$|led)":
>          type: object
> -        $ref: common.yaml#
> +        allOf:
> +          - $ref: common.yaml#
> +        properties:
> +          default-intensity:
> +            maxItems: 1
> +            description: The intensity the LED gets initialised with.
> 
>  required:
>    - compatible
> @@ -102,6 +107,7 @@ examples:
> 
>                 led-0 {
>                     color = <LED_COLOR_ID_RED>;
> +                   default-intensity = <100>;
>                 };
> 
>                 led-1 {
> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> index f13117eed976..ba760fa33bdc 100644
> --- a/drivers/leds/leds-lp50xx.c
> +++ b/drivers/leds/leds-lp50xx.c
> @@ -501,6 +501,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
>  			}
> 
>  			mc_led_info[num_colors].color_index = color_id;
> +
> +			fwnode_property_read_u32(led_node, "default-intensity",
> +						 &mc_led_info[num_colors].intensity);
> +
>  			num_colors++;
>  		}
> 
> --
> 2.17.1

Re: [PATCH v1] leds: lp50xx: add setting of default intensity from DT

[Jacek Anaszewski]

Hi Sven,

On 1/19/21 11:53 AM, Sven Schuchmann wrote:
> In order to use a multicolor-led together with a trigger
> from DT the led needs to have an intensity set to see something.
> The trigger changes the brightness of the led but if there
> is no intensity we actually see nothing.
> 
> This patch adds the ability to set the default intensity
> of each led so that it is turned on from DT.
> 
> Signed-off-by: Sven Schuchmann <schuchmann@schleissheimer.de>
> ---
>   Documentation/devicetree/bindings/leds/leds-lp50xx.yaml | 8 +++++++-
>   drivers/leds/leds-lp50xx.c                              | 4 ++++
>   2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> index c192b5feadc7..5ad2a0c3c052 100644
> --- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> @@ -69,7 +69,12 @@ patternProperties:
>       patternProperties:
>         "(^led-[0-9a-f]$|led)":
>           type: object
> -        $ref: common.yaml#
> +        allOf:
> +          - $ref: common.yaml#
> +        properties:
> +          default-intensity:
> +            maxItems: 1
> +            description: The intensity the LED gets initialised with.
>   
>   required:
>     - compatible
> @@ -102,6 +107,7 @@ examples:
>   
>                  led-0 {
>                      color = <LED_COLOR_ID_RED>;
> +                   default-intensity = <100>;
>                  };
>   
>                  led-1 {

Please split this into a separate patch, preceding this one in the series.

> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> index f13117eed976..ba760fa33bdc 100644
> --- a/drivers/leds/leds-lp50xx.c
> +++ b/drivers/leds/leds-lp50xx.c
> @@ -501,6 +501,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
>   			}
>   
>   			mc_led_info[num_colors].color_index = color_id;
> +
> +			fwnode_property_read_u32(led_node, "default-intensity",
> +						 &mc_led_info[num_colors].intensity);
> +
>   			num_colors++;
>   		}
>   
> 

For this part:

Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v1] leds: lp50xx: add setting of default intensity from DT

[Pavel Machek]

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

Hi!

> In order to use a multicolor-led together with a trigger
> from DT the led needs to have an intensity set to see something.
> The trigger changes the brightness of the led but if there
> is no intensity we actually see nothing.
> 
> This patch adds the ability to set the default intensity
> of each led so that it is turned on from DT.

Do we need this to be configurable from device tree? Can we just set
it to max or something?

Aha, this basically sets the initial color for LEDs the monochromatic
triggers, right?

								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

AW: [PATCH v1] leds: lp50xx: add setting of default intensity from DT

[Sven Schuchmann]

Hello Pavel,

> > In order to use a multicolor-led together with a trigger
> > from DT the led needs to have an intensity set to see something.
> > The trigger changes the brightness of the led but if there
> > is no intensity we actually see nothing.
> >
> > This patch adds the ability to set the default intensity
> > of each led so that it is turned on from DT.
> 
> Do we need this to be configurable from device tree? Can we just set
> it to max or something?
> 
> Aha, this basically sets the initial color for LEDs the monochromatic
> triggers, right?

Let me try to explain in other words: I have one RGB-LED
which consists of 3 Colors. Each of the three colors (Red, Green, Blue) you have
to define in the DT. For example this is my setup for one RGB-LED which I wanted
to show the heartbeat in Red (half intensity):

                                multi-led@3 {
                                        #address-cells = <1>;
                                        #size-cells = <0>;
                                        reg = <0x3>;
                                        color = <LED_COLOR_ID_RGB>;

                                        linux,default-trigger = "heartbeat";
                                        function = LED_FUNCTION_HEARTBEAT;

                                        led-9 {
                                                color = <LED_COLOR_ID_RED>;
                                                default-intensity = <100>;
                                        };

                                        led-10 {
                                                color = <LED_COLOR_ID_GREEN>;
                                        };

                                        led-11 {
                                                color = <LED_COLOR_ID_BLUE>;
                                        };
                                };

If I would not have the default-intensity I would actually see nothing,
since the intensity (which goes from 0-255) of each led is initialized with 0.

I hope I could clarify this a little more?

Best Regards,

   Sven

Re: [PATCH v1] leds: lp50xx: add setting of default intensity from DT

[Pavel Machek]

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

On Wed 2021-02-03 15:39:59, Sven Schuchmann wrote:
> Hello Pavel,
> 
> > > In order to use a multicolor-led together with a trigger
> > > from DT the led needs to have an intensity set to see something.
> > > The trigger changes the brightness of the led but if there
> > > is no intensity we actually see nothing.
> > >
> > > This patch adds the ability to set the default intensity
> > > of each led so that it is turned on from DT.
> > 
> > Do we need this to be configurable from device tree? Can we just set
> > it to max or something?
> > 
> > Aha, this basically sets the initial color for LEDs the monochromatic
> > triggers, right?
> 
> Let me try to explain in other words: I have one RGB-LED
> which consists of 3 Colors. Each of the three colors (Red, Green, Blue) you have
> to define in the DT. For example this is my setup for one RGB-LED which I wanted
> to show the heartbeat in Red (half intensity):
> 
>                                 multi-led@3 {
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
>                                         reg = <0x3>;
>                                         color = <LED_COLOR_ID_RGB>;
> 
>                                         linux,default-trigger = "heartbeat";
>                                         function = LED_FUNCTION_HEARTBEAT;
> 
>                                         led-9 {
>                                                 color = <LED_COLOR_ID_RED>;
>                                                 default-intensity = <100>;
>                                         };
> 
>                                         led-10 {
>                                                 color = <LED_COLOR_ID_GREEN>;
>                                         };
> 
>                                         led-11 {
>                                                 color = <LED_COLOR_ID_BLUE>;
>                                         };
>                                 };
> 
> If I would not have the default-intensity I would actually see nothing,
> since the intensity (which goes from 0-255) of each led is initialized with 0.
> 
> I hope I could clarify this a little more?

Yes, sounds reasonable. Could we get default intensity of 100% on all
channels if nothing else is specified?

Or maybe simply "if intensity is not specified, start with 100%, and
use explicit =0 if other color is expected".

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

AW: [PATCH v1] leds: lp50xx: add setting of default intensity from DT

[Sven Schuchmann]

Hello Pavel,

> 
> Yes, sounds reasonable. Could we get default intensity of 100% on all
> channels if nothing else is specified?
> 
> Or maybe simply "if intensity is not specified, start with 100%, and
> use explicit =0 if other color is expected".
> 
Mh, if someone is already using the led driver and updates to a newer kernel 
we would then turn on all leds per default to the maximum intensity during boot 
until they are set the way they should be from userspace. I don't know if this
is what we want? If yes, sure, we could set them to maximum per default.

Also if we want to use Percentage Values (%) for setting the intensity
I think this should also be done for the userspace interfaces and not only from DT. 
So at this time setting the RGB-LED to white is done like this:
echo "255 255 255" > /sys/class/leds/rgb\:led/multi_intensity
would then get 
echo "100 100 100" > /sys/class/leds/rgb\:led/multi_intensity
Again over here: If we would change it would break earlier Applications
from userspace. Just my thoughts. What do you think?

Best Regards,

   Sven

Re: [PATCH v1] leds: lp50xx: add setting of default intensity from DT

[Marek Behun]

On Wed, 3 Feb 2021 17:35:55 +0100
Pavel Machek <pavel@ucw.cz> wrote:

> On Wed 2021-02-03 15:39:59, Sven Schuchmann wrote:
> > Hello Pavel,
> >   
> > > > In order to use a multicolor-led together with a trigger
> > > > from DT the led needs to have an intensity set to see something.
> > > > The trigger changes the brightness of the led but if there
> > > > is no intensity we actually see nothing.
> > > >
> > > > This patch adds the ability to set the default intensity
> > > > of each led so that it is turned on from DT.  
> > > 
> > > Do we need this to be configurable from device tree? Can we just set
> > > it to max or something?
> > > 
> > > Aha, this basically sets the initial color for LEDs the monochromatic
> > > triggers, right?  
> > 
> > Let me try to explain in other words: I have one RGB-LED
> > which consists of 3 Colors. Each of the three colors (Red, Green, Blue) you have
> > to define in the DT. For example this is my setup for one RGB-LED which I wanted
> > to show the heartbeat in Red (half intensity):
> > 
> >                                 multi-led@3 {
> >                                         #address-cells = <1>;
> >                                         #size-cells = <0>;
> >                                         reg = <0x3>;
> >                                         color = <LED_COLOR_ID_RGB>;
> > 
> >                                         linux,default-trigger = "heartbeat";
> >                                         function = LED_FUNCTION_HEARTBEAT;
> > 
> >                                         led-9 {
> >                                                 color = <LED_COLOR_ID_RED>;
> >                                                 default-intensity = <100>;
> >                                         };
> > 
> >                                         led-10 {
> >                                                 color = <LED_COLOR_ID_GREEN>;
> >                                         };
> > 
> >                                         led-11 {
> >                                                 color = <LED_COLOR_ID_BLUE>;
> >                                         };
> >                                 };
> > 
> > If I would not have the default-intensity I would actually see nothing,
> > since the intensity (which goes from 0-255) of each led is initialized with 0.
> > 
> > I hope I could clarify this a little more?  
> 
> Yes, sounds reasonable. Could we get default intensity of 100% on all
> channels if nothing else is specified?
> 
> Or maybe simply "if intensity is not specified, start with 100%, and
> use explicit =0 if other color is expected".
> 
> Best regards,
> 								Pavel

Is the property default-intensity documented in DT bindings?

Wouldn't it be better if the property was used in the multi-led node
instead of the channel node? I.e.
  multi-led@3 {
    color = <LED_COLOR_ID_RGB>;
    default-intensity = <100 0 0>;
  };

Re: [PATCH v1] leds: lp50xx: add setting of default intensity from DT

[Pavel Machek]

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

Hi!

> > > If I would not have the default-intensity I would actually see nothing,
> > > since the intensity (which goes from 0-255) of each led is initialized with 0.
> > > 
> > > I hope I could clarify this a little more?  
> > 
> > Yes, sounds reasonable. Could we get default intensity of 100% on all
> > channels if nothing else is specified?
> > 
> > Or maybe simply "if intensity is not specified, start with 100%, and
> > use explicit =0 if other color is expected".
> > 
> > Best regards,
> > 								Pavel
> 
> Is the property default-intensity documented in DT bindings?
> 
> Wouldn't it be better if the property was used in the multi-led node
> instead of the channel node? I.e.
>   multi-led@3 {
>     color = <LED_COLOR_ID_RGB>;
>     default-intensity = <100 0 0>;
>   };

Yes, this would be better.

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v1] leds: lp50xx: add setting of default intensity from DT

[Pavel Machek]

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

Hi!

> > Yes, sounds reasonable. Could we get default intensity of 100% on all
> > channels if nothing else is specified?
> > 
> > Or maybe simply "if intensity is not specified, start with 100%, and
> > use explicit =0 if other color is expected".
> > 
> Mh, if someone is already using the led driver and updates to a newer kernel 
> we would then turn on all leds per default to the maximum intensity during boot 
> until they are set the way they should be from userspace. I don't know if this
> is what we want? If yes, sure, we could set them to maximum per
> default.

Not really. If they don't have trigger configured, nothing will happen.

> Also if we want to use Percentage Values (%) for setting the intensity
> I think this should also be done for the userspace interfaces and
> not only from DT.

We don't want to use percentages in the API (but let me still use
percentages in discussion).

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

AW: [PATCH v1] leds: lp50xx: add setting of default intensity from DT

[Sven Schuchmann]

Hello Pavel, hello Marek

> > Is the property default-intensity documented in DT bindings?

I updated the documentation in leds-lp50xx.yaml. Is it this what you mean?

> > Wouldn't it be better if the property was used in the multi-led node
> > instead of the channel node? I.e.
> >   multi-led@3 {
> >     color = <LED_COLOR_ID_RGB>;
> >     default-intensity = <100 0 0>;
> >   };
> 
> Yes, this would be better.

Sound good, I will see what I can do here.

Regards, Sven

AW: [PATCH v1] leds: lp50xx: add setting of default intensity from DT

[Sven Schuchmann]

Helo Pavel,

> > > Yes, sounds reasonable. Could we get default intensity of 100% on all
> > > channels if nothing else is specified?
> > >
> > > Or maybe simply "if intensity is not specified, start with 100%, and
> > > use explicit =0 if other color is expected".
> > >
> > Mh, if someone is already using the led driver and updates to a newer kernel
> > we would then turn on all leds per default to the maximum intensity during boot
> > until they are set the way they should be from userspace. I don't know if this
> > is what we want? If yes, sure, we could set them to maximum per
> > default.
> 
> Not really. If they don't have trigger configured, nothing will happen.

Oops, you are right, my fault.

> 
> > Also if we want to use Percentage Values (%) for setting the intensity
> > I think this should also be done for the userspace interfaces and
> > not only from DT.
> 
> We don't want to use percentages in the API (but let me still use
> percentages in discussion).

No Problem.

Best Regards,

   Sven