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

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

Best Regards,

   Sven

Hi Sven,

On 1/19/21 11:53 AM, Sven Schuchmann wrote:

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

For this part:

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

-- 
Best regards,
Jacek Anaszewski

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

Hello Pavel,

quoted

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

On Wed 2021-02-03 15:39:59, Sven Schuchmann wrote:

Hello Pavel,

quoted

quoted

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

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

Hi!

quoted

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

Helo Pavel,

quoted

quoted

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.

quoted

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

On Wed, 3 Feb 2021 17:35:55 +0100
Pavel Machek [off-list ref] wrote:

On Wed 2021-02-03 15:39:59, Sven Schuchmann wrote:

quoted

Hello Pavel,
  

quoted

quoted

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

Hi!

quoted

quoted

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

Hello Pavel, hello Marek

quoted

Is the property default-intensity documented in DT bindings?

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

quoted

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