diff mbox series

[v5,3/3] dt-bindings: leds: Convert pwm to yaml

Message ID 20200919053145.7564-4-post@lespocky.de
State Superseded
Headers show
Series leds: pwm: Make automatic labels work | expand

Checks

Context Check Description
robh/dt-meta-schema fail build log
robh/checkpatch warning total: 0 errors, 2 warnings, 82 lines checked

Commit Message

Alexander Dahl Sept. 19, 2020, 5:31 a.m. UTC
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

Comments

Rob Herring Sept. 22, 2020, 3:42 p.m. UTC | #1
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.
Rob Herring Sept. 22, 2020, 3:57 p.m. UTC | #2
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
>
Alexander Dahl Sept. 28, 2020, 11:19 a.m. UTC | #3
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
Rob Herring Sept. 28, 2020, 4:11 p.m. UTC | #4
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
Alexander Dahl Sept. 29, 2020, 7:39 a.m. UTC | #5
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
Rob Herring Sept. 29, 2020, 1:58 p.m. UTC | #6
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 mbox series

Patch

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