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