| Message ID | 20200919053145.7564-4-post@lespocky.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | leds: pwm: Make automatic labels work | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 2 warnings, 82 lines checked |
| robh/dt-meta-schema | fail | build log |
On Sat, 19 Sep 2020 07:31:45 +0200, Alexander Dahl wrote: > The example was adapted slightly to make use of the 'function' and > 'color' properties. License discussed with the original author. > > Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Signed-off-by: Alexander Dahl <post@lespocky.de> > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > > Notes: > v4 -> v5: > * updated based on feedback by Rob Herring > * removed Acked-by > > v3 -> v4: > * added Cc to original author of the binding > > v2 -> v3: > * changed license identifier to recommended one > * added Acked-by > > v2: > * added this patch to series (Suggested-by: Jacek Anaszewski) > > .../devicetree/bindings/leds/leds-pwm.txt | 50 ----------- > .../devicetree/bindings/leds/leds-pwm.yaml | 82 +++++++++++++++++++ > 2 files changed, 82 insertions(+), 50 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt > create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.yaml > My bot found errors running 'make dt_binding_check' on your patch: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/iqs62x.example.dt.yaml: pwmleds: 'panel' does not match any of the regexes: '^led(-[0-9a-f]+)?$', 'pinctrl-[0-9]+' From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-pwm.yaml See https://patchwork.ozlabs.org/patch/1367461 If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure dt-schema is up to date: pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade Please check and re-submit.
On Sat, Sep 19, 2020 at 07:31:45AM +0200, Alexander Dahl wrote: > The example was adapted slightly to make use of the 'function' and > 'color' properties. License discussed with the original author. > > Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Signed-off-by: Alexander Dahl <post@lespocky.de> > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > > Notes: > v4 -> v5: > * updated based on feedback by Rob Herring > * removed Acked-by > > v3 -> v4: > * added Cc to original author of the binding > > v2 -> v3: > * changed license identifier to recommended one > * added Acked-by > > v2: > * added this patch to series (Suggested-by: Jacek Anaszewski) > > .../devicetree/bindings/leds/leds-pwm.txt | 50 ----------- > .../devicetree/bindings/leds/leds-pwm.yaml | 82 +++++++++++++++++++ > 2 files changed, 82 insertions(+), 50 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt > create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.yaml > > diff --git a/Documentation/devicetree/bindings/leds/leds-pwm.txt b/Documentation/devicetree/bindings/leds/leds-pwm.txt > deleted file mode 100644 > index 6c6583c35f2f..000000000000 > --- a/Documentation/devicetree/bindings/leds/leds-pwm.txt > +++ /dev/null > @@ -1,50 +0,0 @@ > -LED connected to PWM > - > -Required properties: > -- compatible : should be "pwm-leds". > - > -Each LED is represented as a sub-node of the pwm-leds device. Each > -node's name represents the name of the corresponding LED. > - > -LED sub-node properties: > -- pwms : PWM property to point to the PWM device (phandle)/port (id) and to > - specify the period time to be used: <&phandle id period_ns>; > -- pwm-names : (optional) Name to be used by the PWM subsystem for the PWM device > - For the pwms and pwm-names property please refer to: > - Documentation/devicetree/bindings/pwm/pwm.txt > -- max-brightness : Maximum brightness possible for the LED > -- active-low : (optional) For PWMs where the LED is wired to supply > - rather than ground. > -- label : (optional) > - see Documentation/devicetree/bindings/leds/common.txt > -- linux,default-trigger : (optional) > - see Documentation/devicetree/bindings/leds/common.txt > - > -Example: > - > -twl_pwm: pwm { > - /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */ > - compatible = "ti,twl6030-pwm"; > - #pwm-cells = <2>; > -}; > - > -twl_pwmled: pwmled { > - /* provides one PWM (id 0 for Charing indicator LED) */ > - compatible = "ti,twl6030-pwmled"; > - #pwm-cells = <2>; > -}; > - > -pwmleds { > - compatible = "pwm-leds"; > - kpad { > - label = "omap4::keypad"; > - pwms = <&twl_pwm 0 7812500>; > - max-brightness = <127>; > - }; > - > - charging { > - label = "omap4:green:chrg"; > - pwms = <&twl_pwmled 0 7812500>; > - max-brightness = <255>; > - }; > -}; > diff --git a/Documentation/devicetree/bindings/leds/leds-pwm.yaml b/Documentation/devicetree/bindings/leds/leds-pwm.yaml > new file mode 100644 > index 000000000000..c9316811c7f1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/leds-pwm.yaml > @@ -0,0 +1,82 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/leds/leds-pwm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: LEDs connected to PWM > + > +maintainers: > + - Pavel Machek <pavel@ucw.cz> > + > +description: > + Each LED is represented as a sub-node of the pwm-leds device. Each > + node's name represents the name of the corresponding LED. > + > +properties: > + compatible: > + const: pwm-leds > + > +patternProperties: > + "^led(-[0-9a-f]+)?$": > + type: object > + > + $ref: common.yaml# > + > + properties: > + pwms: > + maxItems: 1 > + > + pwm-names: true > + > + max-brightness: > + description: > + Maximum brightness possible for the LED > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + active-low: > + description: > + For PWMs where the LED is wired to supply rather than ground. > + type: boolean > + > + required: > + - pwms > + - max-brightness > + > +additionalProperties: false > + > +examples: > + - | > + > + #include <dt-bindings/leds/common.h> > + > + twl_pwm: pwm { > + /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */ > + compatible = "ti,twl6030-pwm"; > + #pwm-cells = <2>; > + }; > + > + twl_pwmled: pwmled { > + /* provides one PWM (id 0 for Charing indicator LED) */ > + compatible = "ti,twl6030-pwmled"; > + #pwm-cells = <2>; > + }; It would be best to just remove these 2 nodes. The provider is not really relevant here and these will likely have schema errors when there's a schema for them. For example, they should be child nodes of the TWL6030 device. > + > + pwm_leds { Use generic node names: led-controller { > + compatible = "pwm-leds"; > + > + led-1 { > + label = "omap4::keypad"; > + pwms = <&twl_pwm 0 7812500>; > + max-brightness = <127>; > + }; > + > + led-2 { > + color = <LED_COLOR_ID_GREEN>; > + function = LED_FUNCTION_CHARGING; > + pwms = <&twl_pwmled 0 7812500>; > + max-brightness = <255>; > + }; > + }; > + > +... > -- > 2.20.1 >
Hello Rob, Am Dienstag, 22. September 2020, 17:42:58 CEST schrieb Rob Herring: > On Sat, 19 Sep 2020 07:31:45 +0200, Alexander Dahl wrote: > > The example was adapted slightly to make use of the 'function' and > > 'color' properties. License discussed with the original author. > > > > Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > > Signed-off-by: Alexander Dahl <post@lespocky.de> > > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > > --- > > > > Notes: > > v4 -> v5: > > * updated based on feedback by Rob Herring > > * removed Acked-by > > > > v3 -> v4: > > * added Cc to original author of the binding > > > > v2 -> v3: > > * changed license identifier to recommended one > > * added Acked-by > > > > v2: > > * added this patch to series (Suggested-by: Jacek Anaszewski) > > > > .../devicetree/bindings/leds/leds-pwm.txt | 50 ----------- > > .../devicetree/bindings/leds/leds-pwm.yaml | 82 +++++++++++++++++++ > > 2 files changed, 82 insertions(+), 50 deletions(-) > > delete mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt > > create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.yaml > > My bot found errors running 'make dt_binding_check' on your patch: > > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/iqs > 62x.example.dt.yaml: pwmleds: 'panel' does not match any of the regexes: > '^led(-[0-9a-f]+)?$', 'pinctrl-[0-9]+' From schema: > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/l > eds-pwm.yaml I somehow expected errors on those checks, because I got actually two different recommendations from you: In feedback on v4 of this patch (series) you recommended '^led(-[0-9a-f]+)?$' for the (pwm) led node name, which I used in v5. Or just allow any node name with ".*" like in gpio-keys.yaml … I just checked all in-tree dts files using "pwm-leds" and each also defines the "label" property, so renaming those nodes should not alter the paths in sysfs, if I understood everything correctly. So I see two options now: 1) Go with the stricter check and fix all failing dts files and examples. 2) Just use the very loose check. If 1), which patch would go first, renaming nodes in dts and examples or converting bindings to yaml enabling the stricter check? > Please check and re-submit. Will do, maybe I split the patch series and send both remaining patches separately? Alex
On Mon, Sep 28, 2020 at 6:19 AM Alexander Dahl <ada@thorsis.com> wrote: > > Hello Rob, > > Am Dienstag, 22. September 2020, 17:42:58 CEST schrieb Rob Herring: > > On Sat, 19 Sep 2020 07:31:45 +0200, Alexander Dahl wrote: > > > The example was adapted slightly to make use of the 'function' and > > > 'color' properties. License discussed with the original author. > > > > > > Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > > > Signed-off-by: Alexander Dahl <post@lespocky.de> > > > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > > > --- > > > > > > Notes: > > > v4 -> v5: > > > * updated based on feedback by Rob Herring > > > * removed Acked-by > > > > > > v3 -> v4: > > > * added Cc to original author of the binding > > > > > > v2 -> v3: > > > * changed license identifier to recommended one > > > * added Acked-by > > > > > > v2: > > > * added this patch to series (Suggested-by: Jacek Anaszewski) > > > > > > .../devicetree/bindings/leds/leds-pwm.txt | 50 ----------- > > > .../devicetree/bindings/leds/leds-pwm.yaml | 82 +++++++++++++++++++ > > > 2 files changed, 82 insertions(+), 50 deletions(-) > > > delete mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt > > > create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.yaml > > > > My bot found errors running 'make dt_binding_check' on your patch: > > > > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/iqs > > 62x.example.dt.yaml: pwmleds: 'panel' does not match any of the regexes: > > '^led(-[0-9a-f]+)?$', 'pinctrl-[0-9]+' From schema: > > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/l > > eds-pwm.yaml > > I somehow expected errors on those checks, because I got actually two > different recommendations from you: > > In feedback on v4 of this patch (series) you recommended '^led(-[0-9a-f]+)?$' > for the (pwm) led node name, which I used in v5. Or just allow any node name > with ".*" like in gpio-keys.yaml … > > I just checked all in-tree dts files using "pwm-leds" and each also defines > the "label" property, so renaming those nodes should not alter the paths in > sysfs, if I understood everything correctly. So I see two options now: > > 1) Go with the stricter check and fix all failing dts files and examples. > > 2) Just use the very loose check. Either one is fine. Given label is present and there's not a ton of cases, then I'd probably go with 1. > If 1), which patch would go first, renaming nodes in dts and examples or > converting bindings to yaml enabling the stricter check? There's currently no requirement on dts files being warning free. So the schema can come first and any dts fixes later. Rob
Hei hei, Am Dienstag, 22. September 2020, 17:57:47 CEST schrieb Rob Herring: > Use generic node names: > > led-controller { > > > + compatible = "pwm-leds"; > > + > > + led-1 { > > + label = "omap4::keypad"; > > + pwms = <&twl_pwm 0 7812500>; > > + max-brightness = <127>; > > + }; > > + > > + led-2 { > > + color = <LED_COLOR_ID_GREEN>; > > + function = LED_FUNCTION_CHARGING; > > + pwms = <&twl_pwmled 0 7812500>; > > + max-brightness = <255>; > > + }; > > + }; > > + > > +... This is clear for the "one led-controller" case. However, when trying to fix those node names in existing .dts files, I wondered how those should be named for multiple, different led-controllers, e.g. one using "pwm-leds" and another one using "gpio-leds"? See arch/arm/boot/dts/at91-kizbox3-hs.dts for example, the nodes are called "pwm_leds" and "leds" currently. If both were part of a .dtsi and both named the same, you could not overwrite/complement those in a .dts file including that .dtsi due to a name conflict. Just append a numerical index like this? led-controller-1 { … }; led-controller-2 { … }; Greets Alex
On Tue, Sep 29, 2020 at 2:39 AM Alexander Dahl <ada@thorsis.com> wrote: > > Hei hei, > > Am Dienstag, 22. September 2020, 17:57:47 CEST schrieb Rob Herring: > > Use generic node names: > > > > led-controller { > > > > > + compatible = "pwm-leds"; > > > + > > > + led-1 { > > > + label = "omap4::keypad"; > > > + pwms = <&twl_pwm 0 7812500>; > > > + max-brightness = <127>; > > > + }; > > > + > > > + led-2 { > > > + color = <LED_COLOR_ID_GREEN>; > > > + function = LED_FUNCTION_CHARGING; > > > + pwms = <&twl_pwmled 0 7812500>; > > > + max-brightness = <255>; > > > + }; > > > + }; > > > + > > > +... > > This is clear for the "one led-controller" case. However, when trying to fix > those node names in existing .dts files, I wondered how those should be named > for multiple, different led-controllers, e.g. one using "pwm-leds" and another > one using "gpio-leds"? > > See arch/arm/boot/dts/at91-kizbox3-hs.dts for example, the nodes are called > "pwm_leds" and "leds" currently. If both were part of a .dtsi and both named > the same, you could not overwrite/complement those in a .dts file including > that .dtsi due to a name conflict. > > Just append a numerical index like this? > > led-controller-1 { > … > }; > > led-controller-2 { > … > }; Yes, that's generally what we've been doing. Rob
diff --git a/Documentation/devicetree/bindings/leds/leds-pwm.txt b/Documentation/devicetree/bindings/leds/leds-pwm.txt deleted file mode 100644 index 6c6583c35f2f..000000000000 --- a/Documentation/devicetree/bindings/leds/leds-pwm.txt +++ /dev/null @@ -1,50 +0,0 @@ -LED connected to PWM - -Required properties: -- compatible : should be "pwm-leds". - -Each LED is represented as a sub-node of the pwm-leds device. Each -node's name represents the name of the corresponding LED. - -LED sub-node properties: -- pwms : PWM property to point to the PWM device (phandle)/port (id) and to - specify the period time to be used: <&phandle id period_ns>; -- pwm-names : (optional) Name to be used by the PWM subsystem for the PWM device - For the pwms and pwm-names property please refer to: - Documentation/devicetree/bindings/pwm/pwm.txt -- max-brightness : Maximum brightness possible for the LED -- active-low : (optional) For PWMs where the LED is wired to supply - rather than ground. -- label : (optional) - see Documentation/devicetree/bindings/leds/common.txt -- linux,default-trigger : (optional) - see Documentation/devicetree/bindings/leds/common.txt - -Example: - -twl_pwm: pwm { - /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */ - compatible = "ti,twl6030-pwm"; - #pwm-cells = <2>; -}; - -twl_pwmled: pwmled { - /* provides one PWM (id 0 for Charing indicator LED) */ - compatible = "ti,twl6030-pwmled"; - #pwm-cells = <2>; -}; - -pwmleds { - compatible = "pwm-leds"; - kpad { - label = "omap4::keypad"; - pwms = <&twl_pwm 0 7812500>; - max-brightness = <127>; - }; - - charging { - label = "omap4:green:chrg"; - pwms = <&twl_pwmled 0 7812500>; - max-brightness = <255>; - }; -}; diff --git a/Documentation/devicetree/bindings/leds/leds-pwm.yaml b/Documentation/devicetree/bindings/leds/leds-pwm.yaml new file mode 100644 index 000000000000..c9316811c7f1 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-pwm.yaml @@ -0,0 +1,82 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/leds/leds-pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: LEDs connected to PWM + +maintainers: + - Pavel Machek <pavel@ucw.cz> + +description: + Each LED is represented as a sub-node of the pwm-leds device. Each + node's name represents the name of the corresponding LED. + +properties: + compatible: + const: pwm-leds + +patternProperties: + "^led(-[0-9a-f]+)?$": + type: object + + $ref: common.yaml# + + properties: + pwms: + maxItems: 1 + + pwm-names: true + + max-brightness: + description: + Maximum brightness possible for the LED + $ref: /schemas/types.yaml#/definitions/uint32 + + active-low: + description: + For PWMs where the LED is wired to supply rather than ground. + type: boolean + + required: + - pwms + - max-brightness + +additionalProperties: false + +examples: + - | + + #include <dt-bindings/leds/common.h> + + twl_pwm: pwm { + /* provides two PWMs (id 0, 1 for PWM1 and PWM2) */ + compatible = "ti,twl6030-pwm"; + #pwm-cells = <2>; + }; + + twl_pwmled: pwmled { + /* provides one PWM (id 0 for Charing indicator LED) */ + compatible = "ti,twl6030-pwmled"; + #pwm-cells = <2>; + }; + + pwm_leds { + compatible = "pwm-leds"; + + led-1 { + label = "omap4::keypad"; + pwms = <&twl_pwm 0 7812500>; + max-brightness = <127>; + }; + + led-2 { + color = <LED_COLOR_ID_GREEN>; + function = LED_FUNCTION_CHARGING; + pwms = <&twl_pwmled 0 7812500>; + max-brightness = <255>; + }; + }; + +...
The example was adapted slightly to make use of the 'function' and 'color' properties. License discussed with the original author. Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> Signed-off-by: Alexander Dahl <post@lespocky.de> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> --- Notes: v4 -> v5: * updated based on feedback by Rob Herring * removed Acked-by v3 -> v4: * added Cc to original author of the binding v2 -> v3: * changed license identifier to recommended one * added Acked-by v2: * added this patch to series (Suggested-by: Jacek Anaszewski) .../devicetree/bindings/leds/leds-pwm.txt | 50 ----------- .../devicetree/bindings/leds/leds-pwm.yaml | 82 +++++++++++++++++++ 2 files changed, 82 insertions(+), 50 deletions(-) delete mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.yaml