diff mbox series

[v2,1/2] dt-bindings: leds: Add multi-color default-intensities property

Message ID 364df52a196fa0ae5db07e599995fcf8dfafb43e.1651577132.git.sven.schwermer@disruptive-technologies.com
State Superseded
Headers show
Series [v2,1/2] dt-bindings: leds: Add multi-color default-intensities property | expand

Commit Message

Sven Schwermer May 3, 2022, 11:27 a.m. UTC 
From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

This allows to assign intensity values to the indivisual sub LEDs
(colors) at driver probe time, i.e. most commonly at kernel boot time.
This is crucial for setting a specific color early in the boot process.

Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
---

Notes:
    V1->V2: no changes

 .../devicetree/bindings/leds/leds-class-multicolor.yaml    | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Sven Schwermer May 3, 2022, 6:58 p.m. UTC | #1 
Hi Sven,

Thanks for making me aware of your patch series. My series would work 
similar to yours, i.e. the default-intensities property would be on the 
same level as color = <LED_COLOR_ID_RGB>.

However, the concern voiced by Jacek is relevant for my patch as well, 
see 
https://lore.kernel.org/all/d5631e35-cd62-106f-2ec4-de3163367bc0@gmail.com/
However, I do not know how to resolve the issue. Perhaps somebody from 
the list has ideas?

Best regards,
Sven

On 5/3/22 15:50, Sven Schuchmann wrote:
> Hello Sven,
> 
> tried this some time ago for the LP50XX
> https://lore.kernel.org/all/20210204143726.27977-1-schuchmann@schleissheimer.de/
> 
> Your solution looks much better.
> Could you give an example of how to use it?
> 
> My Configuration at this time looks like this:
> 
> multi-led@0 {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		reg = <0x0>;
> 		color = <LED_COLOR_ID_RGB>;
> 		function = "eee-led-status";
> 
> 		led-0 {
> 			color = <LED_COLOR_ID_RED>;
> 		};
> 
> 		led-1 {
> 			color = <LED_COLOR_ID_GREEN>;
> 		};
> 
> 		led-2 {
> 			color = <LED_COLOR_ID_BLUE>;
> 		};
> 	};
> 
> 
> Where do I put the "default-intensities"?
> 
> Regards,
> 
>     Sven
> 
>> -----Ursprüngliche Nachricht-----
>> Von: Sven Schwermer <sven@svenschwermer.de>
>> Gesendet: Dienstag, 3. Mai 2022 13:27
>> An: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; pavel@ucw.cz;
>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; dmurphy@ti.com;
>> devicetree@vger.kernel.org
>> Cc: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
>> Betreff: [PATCH v2 1/2] dt-bindings: leds: Add multi-color default-intensities property
>>
>> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
>>
>> This allows to assign intensity values to the indivisual sub LEDs
>> (colors) at driver probe time, i.e. most commonly at kernel boot time.
>> This is crucial for setting a specific color early in the boot process.
>>
>> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
>> ---
>>
>> Notes:
>>      V1->V2: no changes
>>
>>   .../devicetree/bindings/leds/leds-class-multicolor.yaml    | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
>> b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
>> index 37445c68cdef..c483967a847c 100644
>> --- a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
>> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
>> @@ -31,6 +31,13 @@ patternProperties:
>>             include/linux/leds/common.h.
>>           enum: [ 8, 9 ]
>>
>> +      default-intensities:
>> +        description: |
>> +          This parameter, if present, sets the initial intensities of the
>> +          individual colors. This array must have the same length as the
>> +          multi-color LED has sub LEDs (colors).
>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>> +
>>       $ref: "common.yaml#"
>>
>>       required:
>> --
>> 2.36.0
>
Sven Schuchmann May 4, 2022, 7:17 a.m. UTC | #2 
Hello Sven,


> -----Ursprüngliche Nachricht-----
> Von: Sven Schwermer <sven@svenschwermer.de>
> Gesendet: Dienstag, 3. Mai 2022 20:59
> An: Sven Schuchmann <schuchmann@schleissheimer.de>; linux-leds@vger.kernel.org
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>; pavel@ucw.cz
> Betreff: Re: AW: [PATCH v2 1/2] dt-bindings: leds: Add multi-color default-intensities
> property
> 
> Hi Sven,
> 
> Thanks for making me aware of your patch series. My series would work
> similar to yours, i.e. the default-intensities property would be on the
> same level as color = <LED_COLOR_ID_RGB>.
> 
> However, the concern voiced by Jacek is relevant for my patch as well,
> see
> https://lore.kernel.org/all/d5631e35-cd62-106f-2ec4-de3163367bc0@gmail.com/
> However, I do not know how to resolve the issue. Perhaps somebody from
> the list has ideas?

I also do not have an idea. But maybe we can talk about how the definition 
should look like in DT. As far as I understood with your patch I would 
have define the LED as follows:

multi-led@0 {
	#address-cells = <1>;
	#size-cells = <0>;
	reg = <0x0>;
	color = <LED_COLOR_ID_RGB>;
	default-intensities = <100 0 0>  <----
	function = "eee-led-status";
	led-0 {
		color = <LED_COLOR_ID_RED>;
	};
	led-1 {
		color = <LED_COLOR_ID_GREEN>;
	};
	led-2 {
		color = <LED_COLOR_ID_BLUE>;
	};
};


Maybe it is better to define per Color like this:

multi-led@0 {
	#address-cells = <1>;
	#size-cells = <0>;
	reg = <0x0>;
	color = <LED_COLOR_ID_RGB>;
	function = "eee-led-status";
	led-0 {
		color = <LED_COLOR_ID_RED>;
		default-intensity = 100
	};
	led-1 {
		color = <LED_COLOR_ID_GREEN>;
		default-intensity = 0
	};
	led-2 {
		color = <LED_COLOR_ID_BLUE>;
		default-intensity = 0
	};
};


I think this could then be handled by RGBW LEDs also.

Best Regards,

   Sven



> 
> Best regards,
> Sven
> 
> On 5/3/22 15:50, Sven Schuchmann wrote:
> > Hello Sven,
> >
> > tried this some time ago for the LP50XX
> > https://lore.kernel.org/all/20210204143726.27977-1-schuchmann@schleissheimer.de/
> >
> > Your solution looks much better.
> > Could you give an example of how to use it?
> >
> > My Configuration at this time looks like this:
> >
> > multi-led@0 {
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 		reg = <0x0>;
> > 		color = <LED_COLOR_ID_RGB>;
> > 		function = "eee-led-status";
> >
> > 		led-0 {
> > 			color = <LED_COLOR_ID_RED>;
> > 		};
> >
> > 		led-1 {
> > 			color = <LED_COLOR_ID_GREEN>;
> > 		};
> >
> > 		led-2 {
> > 			color = <LED_COLOR_ID_BLUE>;
> > 		};
> > 	};
> >
> >
> > Where do I put the "default-intensities"?
> >
> > Regards,
> >
> >     Sven
> >
> >> -----Ursprüngliche Nachricht-----
> >> Von: Sven Schwermer <sven@svenschwermer.de>
> >> Gesendet: Dienstag, 3. Mai 2022 13:27
> >> An: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; pavel@ucw.cz;
> >> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; dmurphy@ti.com;
> >> devicetree@vger.kernel.org
> >> Cc: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> >> Betreff: [PATCH v2 1/2] dt-bindings: leds: Add multi-color default-intensities property
> >>
> >> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> >>
> >> This allows to assign intensity values to the indivisual sub LEDs
> >> (colors) at driver probe time, i.e. most commonly at kernel boot time.
> >> This is crucial for setting a specific color early in the boot process.
> >>
> >> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> >> ---
> >>
> >> Notes:
> >>      V1->V2: no changes
> >>
> >>   .../devicetree/bindings/leds/leds-class-multicolor.yaml    | 7 +++++++
> >>   1 file changed, 7 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
> >> b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
> >> index 37445c68cdef..c483967a847c 100644
> >> --- a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
> >> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
> >> @@ -31,6 +31,13 @@ patternProperties:
> >>             include/linux/leds/common.h.
> >>           enum: [ 8, 9 ]
> >>
> >> +      default-intensities:
> >> +        description: |
> >> +          This parameter, if present, sets the initial intensities of the
> >> +          individual colors. This array must have the same length as the
> >> +          multi-color LED has sub LEDs (colors).
> >> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> >> +
> >>       $ref: "common.yaml#"
> >>
> >>       required:
> >> --
> >> 2.36.0
> >
Sven Schwermer May 4, 2022, 9:24 a.m. UTC | #3 
Hi Sven,

I did consider placing the property into the multicolor's sub nodes. 
However, multicolor LEDs are not required to have firmware sub nodes. At 
least the multicolor class API does not make any assumptions about this.

One possible solution that I came up with is to do something like this:

multi-led {
	color = <LED_COLOR_ID_RGB>;
	default-intensities = <
		LED_COLOR_ID_RED 100
		LED_COLOR_ID_GREEN 0
		LED_COLOR_ID_BLUE 0
	>;
	led-0 {
		color = <LED_COLOR_ID_RED>;
	};
	led-1 {
		color = <LED_COLOR_ID_GREEN>;
	};
	led-2 {
		color = <LED_COLOR_ID_BLUE>;
	};
};

This requires the array to be twice as long as the number of colors (sub 
LEDs). Each color identifier would be paired with the initial intensity. 
This would limit number of LEDs per color to 1. However, I believe, this 
limitation is already there today.

Thoughts?

Best regards,
Sven

On 5/4/22 09:17, Sven Schuchmann wrote:
> Hello Sven,
> 
> 
>> -----Ursprüngliche Nachricht-----
>> Von: Sven Schwermer <sven@svenschwermer.de>
>> Gesendet: Dienstag, 3. Mai 2022 20:59
>> An: Sven Schuchmann <schuchmann@schleissheimer.de>; linux-leds@vger.kernel.org
>> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>; pavel@ucw.cz
>> Betreff: Re: AW: [PATCH v2 1/2] dt-bindings: leds: Add multi-color default-intensities
>> property
>>
>> Hi Sven,
>>
>> Thanks for making me aware of your patch series. My series would work
>> similar to yours, i.e. the default-intensities property would be on the
>> same level as color = <LED_COLOR_ID_RGB>.
>>
>> However, the concern voiced by Jacek is relevant for my patch as well,
>> see
>> https://lore.kernel.org/all/d5631e35-cd62-106f-2ec4-de3163367bc0@gmail.com/
>> However, I do not know how to resolve the issue. Perhaps somebody from
>> the list has ideas?
> 
> I also do not have an idea. But maybe we can talk about how the definition
> should look like in DT. As far as I understood with your patch I would
> have define the LED as follows:
> 
> multi-led@0 {
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	reg = <0x0>;
> 	color = <LED_COLOR_ID_RGB>;
> 	default-intensities = <100 0 0>  <----
> 	function = "eee-led-status";
> 	led-0 {
> 		color = <LED_COLOR_ID_RED>;
> 	};
> 	led-1 {
> 		color = <LED_COLOR_ID_GREEN>;
> 	};
> 	led-2 {
> 		color = <LED_COLOR_ID_BLUE>;
> 	};
> };
> 
> 
> Maybe it is better to define per Color like this:
> 
> multi-led@0 {
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	reg = <0x0>;
> 	color = <LED_COLOR_ID_RGB>;
> 	function = "eee-led-status";
> 	led-0 {
> 		color = <LED_COLOR_ID_RED>;
> 		default-intensity = 100
> 	};
> 	led-1 {
> 		color = <LED_COLOR_ID_GREEN>;
> 		default-intensity = 0
> 	};
> 	led-2 {
> 		color = <LED_COLOR_ID_BLUE>;
> 		default-intensity = 0
> 	};
> };
> 
> 
> I think this could then be handled by RGBW LEDs also.
> 
> Best Regards,
> 
>     Sven
> 
> 
> 
>>
>> Best regards,
>> Sven
>>
>> On 5/3/22 15:50, Sven Schuchmann wrote:
>>> Hello Sven,
>>>
>>> tried this some time ago for the LP50XX
>>> https://lore.kernel.org/all/20210204143726.27977-1-schuchmann@schleissheimer.de/
>>>
>>> Your solution looks much better.
>>> Could you give an example of how to use it?
>>>
>>> My Configuration at this time looks like this:
>>>
>>> multi-led@0 {
>>> 		#address-cells = <1>;
>>> 		#size-cells = <0>;
>>> 		reg = <0x0>;
>>> 		color = <LED_COLOR_ID_RGB>;
>>> 		function = "eee-led-status";
>>>
>>> 		led-0 {
>>> 			color = <LED_COLOR_ID_RED>;
>>> 		};
>>>
>>> 		led-1 {
>>> 			color = <LED_COLOR_ID_GREEN>;
>>> 		};
>>>
>>> 		led-2 {
>>> 			color = <LED_COLOR_ID_BLUE>;
>>> 		};
>>> 	};
>>>
>>>
>>> Where do I put the "default-intensities"?
>>>
>>> Regards,
>>>
>>>      Sven
>>>
>>>> -----Ursprüngliche Nachricht-----
>>>> Von: Sven Schwermer <sven@svenschwermer.de>
>>>> Gesendet: Dienstag, 3. Mai 2022 13:27
>>>> An: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; pavel@ucw.cz;
>>>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; dmurphy@ti.com;
>>>> devicetree@vger.kernel.org
>>>> Cc: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
>>>> Betreff: [PATCH v2 1/2] dt-bindings: leds: Add multi-color default-intensities property
>>>>
>>>> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
>>>>
>>>> This allows to assign intensity values to the indivisual sub LEDs
>>>> (colors) at driver probe time, i.e. most commonly at kernel boot time.
>>>> This is crucial for setting a specific color early in the boot process.
>>>>
>>>> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
>>>> ---
>>>>
>>>> Notes:
>>>>       V1->V2: no changes
>>>>
>>>>    .../devicetree/bindings/leds/leds-class-multicolor.yaml    | 7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
>>>> b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
>>>> index 37445c68cdef..c483967a847c 100644
>>>> --- a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
>>>> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
>>>> @@ -31,6 +31,13 @@ patternProperties:
>>>>              include/linux/leds/common.h.
>>>>            enum: [ 8, 9 ]
>>>>
>>>> +      default-intensities:
>>>> +        description: |
>>>> +          This parameter, if present, sets the initial intensities of the
>>>> +          individual colors. This array must have the same length as the
>>>> +          multi-color LED has sub LEDs (colors).
>>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>>>> +
>>>>        $ref: "common.yaml#"
>>>>
>>>>        required:
>>>> --
>>>> 2.36.0
>>>
Jacek Anaszewski May 8, 2022, 7:55 p.m. UTC | #4 
Hi Sven and Sven,

On 5/4/22 11:24, Sven Schwermer wrote:
> Hi Sven,
> 
> I did consider placing the property into the multicolor's sub nodes. 
> However, multicolor LEDs are not required to have firmware sub nodes. At 
> least the multicolor class API does not make any assumptions about this.

So this is something to be clarified. The whole idea relies on having
sub-nodes in the multi-led node.

> One possible solution that I came up with is to do something like this:
> 
> multi-led {
>      color = <LED_COLOR_ID_RGB>;
>      default-intensities = <
>          LED_COLOR_ID_RED 100
>          LED_COLOR_ID_GREEN 0
>          LED_COLOR_ID_BLUE 0
>      >;
>      led-0 {
>          color = <LED_COLOR_ID_RED>;
>      };
>      led-1 {
>          color = <LED_COLOR_ID_GREEN>;
>      };
>      led-2 {
>          color = <LED_COLOR_ID_BLUE>;
>      };
> };
> 
[...]
>>
>>
>> Maybe it is better to define per Color like this:
>>
>> multi-led@0 {
>>     #address-cells = <1>;
>>     #size-cells = <0>;
>>     reg = <0x0>;
>>     color = <LED_COLOR_ID_RGB>;
>>     function = "eee-led-status";
>>     led-0 {
>>         color = <LED_COLOR_ID_RED>;
>>         default-intensity = 100
>>     };
>>     led-1 {
>>         color = <LED_COLOR_ID_GREEN>;
>>         default-intensity = 0
>>     };
>>     led-2 {
>>         color = <LED_COLOR_ID_BLUE>;
>>         default-intensity = 0
>>     };
>> };

I would go for this. Seems to be the most straightforward solution.
Sven Schwermer May 10, 2022, 6:31 p.m. UTC | #5 
Hi Jacek,

On 5/8/22 21:55, Jacek Anaszewski wrote:
> Hi Sven and Sven,
> 
> On 5/4/22 11:24, Sven Schwermer wrote:
>> Hi Sven,
>>
>> I did consider placing the property into the multicolor's sub nodes. 
>> However, multicolor LEDs are not required to have firmware sub nodes. 
>> At least the multicolor class API does not make any assumptions about 
>> this.
> 
> So this is something to be clarified. The whole idea relies on having
> sub-nodes in the multi-led node.

As far as I understand, multi-color LEDs don't require actual OF 
sub-nodes. The Turris Omnia LED driver doesn't have sub-nodes, see 
Documentation/devicetree/bindings/leds/cznic,turris-omnia-leds.yaml

>>> Maybe it is better to define per Color like this:
>>>
>>> multi-led@0 {
>>>     #address-cells = <1>;
>>>     #size-cells = <0>;
>>>     reg = <0x0>;
>>>     color = <LED_COLOR_ID_RGB>;
>>>     function = "eee-led-status";
>>>     led-0 {
>>>         color = <LED_COLOR_ID_RED>;
>>>         default-intensity = 100
>>>     };
>>>     led-1 {
>>>         color = <LED_COLOR_ID_GREEN>;
>>>         default-intensity = 0
>>>     };
>>>     led-2 {
>>>         color = <LED_COLOR_ID_BLUE>;
>>>         default-intensity = 0
>>>     };
>>> };
> 
> I would go for this. Seems to be the most straightforward solution.

I agree, that this would be the best option. However, as noted above, 
this wouldn't be compatible with all existing multi-color drivers.

Best regards,
Sven
Jacek Anaszewski May 10, 2022, 10 p.m. UTC | #6 
On 5/10/22 20:31, Sven Schwermer wrote:
> Hi Jacek,
> 
> On 5/8/22 21:55, Jacek Anaszewski wrote:
>> Hi Sven and Sven,
>>
>> On 5/4/22 11:24, Sven Schwermer wrote:
>>> Hi Sven,
>>>
>>> I did consider placing the property into the multicolor's sub nodes. 
>>> However, multicolor LEDs are not required to have firmware sub nodes. 
>>> At least the multicolor class API does not make any assumptions about 
>>> this.
>>
>> So this is something to be clarified. The whole idea relies on having
>> sub-nodes in the multi-led node.
> 
> As far as I understand, multi-color LEDs don't require actual OF 
> sub-nodes. The Turris Omnia LED driver doesn't have sub-nodes, see 
> Documentation/devicetree/bindings/leds/cznic,turris-omnia-leds.yaml

Ah, I forgot about that one and the related discussion.
In this case, yes, global array will do.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
index 37445c68cdef..c483967a847c 100644
--- a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
@@ -31,6 +31,13 @@  patternProperties:
           include/linux/leds/common.h.
         enum: [ 8, 9 ]
 
+      default-intensities:
+        description: |
+          This parameter, if present, sets the initial intensities of the
+          individual colors. This array must have the same length as the
+          multi-color LED has sub LEDs (colors).
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+
     $ref: "common.yaml#"
 
     required: