diff mbox series

[v7,10/12] ARM: dts: stm32: Fix schema warnings for pwm-leds

Message ID 20201005203451.9985-11-post@lespocky.de (mailing list archive)
State Superseded
Headers show
Series leds: pwm: Make automatic labels work | expand

Commit Message

Alexander Dahl Oct. 5, 2020, 8:34 p.m. UTC
The node names for devices using the pwm-leds driver follow a certain
naming scheme (now).  Parent node name is not enforced, but recommended
by DT project.

  DTC     arch/arm/boot/dts/stm32mp157c-lxa-mc1.dt.yaml
  CHECK   arch/arm/boot/dts/stm32mp157c-lxa-mc1.dt.yaml
/home/alex/build/linux/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dt.yaml: led-rgb: 'led-blue', 'led-green', 'led-red' do not match any of the regexes: '^led(-[0-9a-f]+)?$', 'pinctrl-[0-9]+'
        From schema: /home/alex/src/linux/leds/Documentation/devicetree/bindings/leds/leds-pwm.yaml

Signed-off-by: Alexander Dahl <post@lespocky.de>
---

Notes:
    v6 -> v7:
      * split up patch (one per sub arch)
      * added actual warnings to commit message

 arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Ahmad Fatoum Oct. 27, 2020, 7:03 a.m. UTC | #1
Hello Alexander,

On 10/5/20 10:34 PM, Alexander Dahl wrote:
> The node names for devices using the pwm-leds driver follow a certain
> naming scheme (now).  Parent node name is not enforced, but recommended
> by DT project.
> 
>   DTC     arch/arm/boot/dts/stm32mp157c-lxa-mc1.dt.yaml
>   CHECK   arch/arm/boot/dts/stm32mp157c-lxa-mc1.dt.yaml
> /home/alex/build/linux/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dt.yaml: led-rgb: 'led-blue', 'led-green', 'led-red' do not match any of the regexes: '^led(-[0-9a-f]+)?$', 'pinctrl-[0-9]+'
>         From schema: /home/alex/src/linux/leds/Documentation/devicetree/bindings/leds/leds-pwm.yaml
> 
> Signed-off-by: Alexander Dahl <post@lespocky.de>

Acked-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

But got two questions below:

> ---
> 
> Notes:
>     v6 -> v7:
>       * split up patch (one per sub arch)
>       * added actual warnings to commit message
> 
>  arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
> index 5700e6b700d3..25d548cb975b 100644
> --- a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
> +++ b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
> @@ -36,34 +36,34 @@
>  		stdout-path = &uart4;
>  	};
>  
> -	led-act {
> +	led-controller-1 {
>  		compatible = "gpio-leds";
>  
> -		led-green {
> +		led-1 {
>  			label = "mc1:green:act";
>  			gpios = <&gpioa 13 GPIO_ACTIVE_LOW>;
>  			linux,default-trigger = "heartbeat";
>  		};
>  	};
>  
> -	led-rgb {
> +	led-controller-2 {

Is a single RGB LED really a controller?

>  		compatible = "pwm-leds";
>  
> -		led-red {
> +		led-2 {

Shouldn't this have been led-1 as well or is the numbering "global" ?

>  			label = "mc1:red:rgb";
>  			pwms = <&leds_pwm 1 1000000 0>;
>  			max-brightness = <255>;
>  			active-low;
>  		};
>  
> -		led-green {
> +		led-3 {
>  			label = "mc1:green:rgb";
>  			pwms = <&leds_pwm 2 1000000 0>;
>  			max-brightness = <255>;
>  			active-low;
>  		};
>  
> -		led-blue {
> +		led-4 {
>  			label = "mc1:blue:rgb";
>  			pwms = <&leds_pwm 3 1000000 0>;
>  			max-brightness = <255>;
>
Alexander Dahl Oct. 27, 2020, 10:05 a.m. UTC | #2
Hello Ahmad,

thanks for your feedback, comments below.

On Tue, Oct 27, 2020 at 08:03:40AM +0100, Ahmad Fatoum wrote:
> Hello Alexander,
> 
> On 10/5/20 10:34 PM, Alexander Dahl wrote:
> > The node names for devices using the pwm-leds driver follow a certain
> > naming scheme (now).  Parent node name is not enforced, but recommended
> > by DT project.
> > 
> >   DTC     arch/arm/boot/dts/stm32mp157c-lxa-mc1.dt.yaml
> >   CHECK   arch/arm/boot/dts/stm32mp157c-lxa-mc1.dt.yaml
> > /home/alex/build/linux/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dt.yaml: led-rgb: 'led-blue', 'led-green', 'led-red' do not match any of the regexes: '^led(-[0-9a-f]+)?$', 'pinctrl-[0-9]+'
> >         From schema: /home/alex/src/linux/leds/Documentation/devicetree/bindings/leds/leds-pwm.yaml
> > 
> > Signed-off-by: Alexander Dahl <post@lespocky.de>
> 
> Acked-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> But got two questions below:
> 
> > ---
> > 
> > Notes:
> >     v6 -> v7:
> >       * split up patch (one per sub arch)
> >       * added actual warnings to commit message
> > 
> >  arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
> > index 5700e6b700d3..25d548cb975b 100644
> > --- a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
> > +++ b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
> > @@ -36,34 +36,34 @@
> >  		stdout-path = &uart4;
> >  	};
> >  
> > -	led-act {
> > +	led-controller-1 {
> >  		compatible = "gpio-leds";
> >  
> > -		led-green {
> > +		led-1 {
> >  			label = "mc1:green:act";
> >  			gpios = <&gpioa 13 GPIO_ACTIVE_LOW>;
> >  			linux,default-trigger = "heartbeat";
> >  		};
> >  	};
> >  
> > -	led-rgb {
> > +	led-controller-2 {
> 
> Is a single RGB LED really a controller?

I just followed the recommendations by Rob here. 

> >  		compatible = "pwm-leds";
> >  
> > -		led-red {
> > +		led-2 {
> 
> Shouldn't this have been led-1 as well or is the numbering "global" ?

Also good question. This numbering is for dts only, it usually does
not correspond with LEDs on the board, so it could be numbered per
led-controller as well?

Greets
Alex

> 
> >  			label = "mc1:red:rgb";
> >  			pwms = <&leds_pwm 1 1000000 0>;
> >  			max-brightness = <255>;
> >  			active-low;
> >  		};
> >  
> > -		led-green {
> > +		led-3 {
> >  			label = "mc1:green:rgb";
> >  			pwms = <&leds_pwm 2 1000000 0>;
> >  			max-brightness = <255>;
> >  			active-low;
> >  		};
> >  
> > -		led-blue {
> > +		led-4 {
> >  			label = "mc1:blue:rgb";
> >  			pwms = <&leds_pwm 3 1000000 0>;
> >  			max-brightness = <255>;
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Ahmad Fatoum Oct. 27, 2020, 10:58 a.m. UTC | #3
Hello,

On 10/27/20 11:05 AM, Alexander Dahl wrote:
> Hello Ahmad,
> 
> thanks for your feedback, comments below.
> 

>>> -	led-rgb {
>>> +	led-controller-2 {
>>
>> Is a single RGB LED really a controller?
> 
> I just followed the recommendations by Rob here.

Do you happen to know if the new multicolor LED support could be used here?

I find it unfortunate that the device tree loses information relevant to humans
to adhere to a fixed nomenclature. Apparently led-controller isn't even codified
in the YAML binding (It's just in the examples). If you respin, please add a
comment that this is a single RGB led. I'd prefer to keep the information
in the DTB as well though.



> 
>>>  		compatible = "pwm-leds";
>>>  
>>> -		led-red {
>>> +		led-2 {
>>
>> Shouldn't this have been led-1 as well or is the numbering "global" ?
> 
> Also good question. This numbering is for dts only, it usually does
> not correspond with LEDs on the board, so it could be numbered per
> led-controller as well?

I'd prefer that it starts by 1. That way it's aligned with PWM channel
ID.

Thanks for fixing the dtschema warnings by the way!

Cheers,
Ahmad

> 
> Greets
> Alex
> 
>>
>>>  			label = "mc1:red:rgb";
>>>  			pwms = <&leds_pwm 1 1000000 0>;
>>>  			max-brightness = <255>;
>>>  			active-low;
>>>  		};
>>>  
>>> -		led-green {
>>> +		led-3 {
>>>  			label = "mc1:green:rgb";
>>>  			pwms = <&leds_pwm 2 1000000 0>;
>>>  			max-brightness = <255>;
>>>  			active-low;
>>>  		};
>>>  
>>> -		led-blue {
>>> +		led-4 {
>>>  			label = "mc1:blue:rgb";
>>>  			pwms = <&leds_pwm 3 1000000 0>;
>>>  			max-brightness = <255>;
>>>
>>
>> -- 
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>
Alexander Dahl Oct. 28, 2020, 7:34 a.m. UTC | #4
Hello Ahmad,

Am Dienstag, 27. Oktober 2020, 11:58:10 CET schrieb Ahmad Fatoum:
> Hello,
> 
> On 10/27/20 11:05 AM, Alexander Dahl wrote:
> > Hello Ahmad,
> > 
> > thanks for your feedback, comments below.
> > 
> >>> -	led-rgb {
> >>> +	led-controller-2 {
> >> 
> >> Is a single RGB LED really a controller?
> > 
> > I just followed the recommendations by Rob here.
> 
> Do you happen to know if the new multicolor LED support could be used here?

AFAIK not yet. The multicolor class should be ready and it is used by some 
drivers for I²C connected LED controllers, but if I understood Pavel 
correctly, additional work has to be done for a gpio and/or pwm multicolor 
driver. See this thread from August for example:

https://lore.kernel.org/linux-leds/2530787.iFCFyWWcSu@g550jk/

> 
> I find it unfortunate that the device tree loses information relevant to
> humans to adhere to a fixed nomenclature. Apparently led-controller isn't
> even codified in the YAML binding (It's just in the examples). If you
> respin, please add a comment that this is a single RGB led. I'd prefer to
> keep the information in the DTB as well though.

The "new" attributes 'function' and 'color' attributes should cover this 
information. IIRC those were introduced sometime before v5.4 and documentation 
is in the leds/common.yaml binding. I don't see it in the scope of this patch 
series, but if we would merge this warning fix first, the information is lost, 
so maybe those attributes should be added before? 

My heuristics on that would be looking at the label and if there's a distinct 
color in it, add the color property. I could do that for all pwm LEDs known to 
the tree currently. That would be a bigger task for GPIO leds though. ;-)

> 
> >>>  		compatible = "pwm-leds";
> >>> 
> >>> -		led-red {
> >>> +		led-2 {
> >> 
> >> Shouldn't this have been led-1 as well or is the numbering "global" ?
> > 
> > Also good question. This numbering is for dts only, it usually does
> > not correspond with LEDs on the board, so it could be numbered per
> > led-controller as well?
> 
> I'd prefer that it starts by 1. That way it's aligned with PWM channel
> ID.

Ack.

> 
> Thanks for fixing the dtschema warnings by the way!

Well, I "introduced" them by converting the leds-pwm binding to yaml (not 
merged yet), so I could as well fix the warnings then? ;-)

Greets
Alex

> 
> Cheers,
> Ahmad
> 
> > Greets
> > Alex
> > 
> >>>  			label = "mc1:red:rgb";
> >>>  			pwms = <&leds_pwm 1 1000000 0>;
> >>>  			max-brightness = <255>;
> >>>  			active-low;
> >>>  		
> >>>  		};
> >>> 
> >>> -		led-green {
> >>> +		led-3 {
> >>> 
> >>>  			label = "mc1:green:rgb";
> >>>  			pwms = <&leds_pwm 2 1000000 0>;
> >>>  			max-brightness = <255>;
> >>>  			active-low;
> >>>  		
> >>>  		};
> >>> 
> >>> -		led-blue {
> >>> +		led-4 {
> >>> 
> >>>  			label = "mc1:blue:rgb";
> >>>  			pwms = <&leds_pwm 3 1000000 0>;
> >>>  			max-brightness = <255>;


--
Ahmad Fatoum Nov. 2, 2020, 10:47 a.m. UTC | #5
Hello Alexander,

On 10/28/20 8:34 AM, Alexander Dahl wrote:
> Hello Ahmad,
> 
> Am Dienstag, 27. Oktober 2020, 11:58:10 CET schrieb Ahmad Fatoum:
>> Hello,
>>
>> On 10/27/20 11:05 AM, Alexander Dahl wrote:
>>> Hello Ahmad,
>>>
>>> thanks for your feedback, comments below.
>>>
>>>>> -	led-rgb {
>>>>> +	led-controller-2 {
>>>>
>>>> Is a single RGB LED really a controller?
>>>
>>> I just followed the recommendations by Rob here.
>>
>> Do you happen to know if the new multicolor LED support could be used here?
> 
> AFAIK not yet. The multicolor class should be ready and it is used by some 
> drivers for I²C connected LED controllers, but if I understood Pavel 
> correctly, additional work has to be done for a gpio and/or pwm multicolor 
> driver. See this thread from August for example:
> 
> https://lore.kernel.org/linux-leds/2530787.iFCFyWWcSu@g550jk/

I see. Thanks for the info.

>> I find it unfortunate that the device tree loses information relevant to
>> humans to adhere to a fixed nomenclature. Apparently led-controller isn't
>> even codified in the YAML binding (It's just in the examples). If you
>> respin, please add a comment that this is a single RGB led. I'd prefer to
>> keep the information in the DTB as well though.
> 
> The "new" attributes 'function' and 'color' attributes should cover this 
> information. IIRC those were introduced sometime before v5.4 and documentation 
> is in the leds/common.yaml binding. I don't see it in the scope of this patch 
> series, but if we would merge this warning fix first, the information is lost, 
> so maybe those attributes should be added before?

Does it? The label already says it's a green LED, but the information that
it's a single physical LED 'bulb' is lost.

> 
> My heuristics on that would be looking at the label and if there's a distinct 
> color in it, add the color property. I could do that for all pwm LEDs known to 
> the tree currently. That would be a bigger task for GPIO leds though. ;-)

I would be ok with just the led-containing node hinting that it's a single RGB led.

Cheers,
Ahmad

> 
>>
>>>>>  		compatible = "pwm-leds";
>>>>>
>>>>> -		led-red {
>>>>> +		led-2 {
>>>>
>>>> Shouldn't this have been led-1 as well or is the numbering "global" ?
>>>
>>> Also good question. This numbering is for dts only, it usually does
>>> not correspond with LEDs on the board, so it could be numbered per
>>> led-controller as well?
>>
>> I'd prefer that it starts by 1. That way it's aligned with PWM channel
>> ID.
> 
> Ack.
> 
>>
>> Thanks for fixing the dtschema warnings by the way!
> 
> Well, I "introduced" them by converting the leds-pwm binding to yaml (not 
> merged yet), so I could as well fix the warnings then? ;-)
> 
> Greets
> Alex
> 
>>
>> Cheers,
>> Ahmad
>>
>>> Greets
>>> Alex
>>>
>>>>>  			label = "mc1:red:rgb";
>>>>>  			pwms = <&leds_pwm 1 1000000 0>;
>>>>>  			max-brightness = <255>;
>>>>>  			active-low;
>>>>>  		
>>>>>  		};
>>>>>
>>>>> -		led-green {
>>>>> +		led-3 {
>>>>>
>>>>>  			label = "mc1:green:rgb";
>>>>>  			pwms = <&leds_pwm 2 1000000 0>;
>>>>>  			max-brightness = <255>;
>>>>>  			active-low;
>>>>>  		
>>>>>  		};
>>>>>
>>>>> -		led-blue {
>>>>> +		led-4 {
>>>>>
>>>>>  			label = "mc1:blue:rgb";
>>>>>  			pwms = <&leds_pwm 3 1000000 0>;
>>>>>  			max-brightness = <255>;
> 
>
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
index 5700e6b700d3..25d548cb975b 100644
--- a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
+++ b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
@@ -36,34 +36,34 @@ 
 		stdout-path = &uart4;
 	};
 
-	led-act {
+	led-controller-1 {
 		compatible = "gpio-leds";
 
-		led-green {
+		led-1 {
 			label = "mc1:green:act";
 			gpios = <&gpioa 13 GPIO_ACTIVE_LOW>;
 			linux,default-trigger = "heartbeat";
 		};
 	};
 
-	led-rgb {
+	led-controller-2 {
 		compatible = "pwm-leds";
 
-		led-red {
+		led-2 {
 			label = "mc1:red:rgb";
 			pwms = <&leds_pwm 1 1000000 0>;
 			max-brightness = <255>;
 			active-low;
 		};
 
-		led-green {
+		led-3 {
 			label = "mc1:green:rgb";
 			pwms = <&leds_pwm 2 1000000 0>;
 			max-brightness = <255>;
 			active-low;
 		};
 
-		led-blue {
+		led-4 {
 			label = "mc1:blue:rgb";
 			pwms = <&leds_pwm 3 1000000 0>;
 			max-brightness = <255>;