[PATCH v5 0/3] leds: pwm: Make automatic labels work

[PATCH v5 0/3] leds: pwm: Make automatic labels work

[Alexander Dahl]

Hei hei,

for leds-gpio you can use the properties 'function' and 'color' in the
devicetree node and omit 'label', the label is constructed
automatically.  This is a common feature supposed to be working for all
LED drivers.  However it did not yet work for the 'leds-pwm' driver.

This series removes platform_data support for the leds-pwm driver and
takes the opportunity to update the leds-pwm dt-bindings accordingly.

v5 was tested on a at91 sama5d2 based platform with LEDs connected to
GPIO and PWM.

Greets
Alex

v5:
- replaced patch 1/3 by a new patch removing platform_data support for
  the leds-pwm driver
- little rewording of commit message in patch 2/3
- updated patch 3/3 based on feedback by Rob Herring
- added Marek Behún to Cc, because he also works on removing
  platform_data support
- rebased series on pavel/for-next

v4:
- added led-class patch handling fwnode passing differently (patch 1/3)
- adapted leds-pwm patch to new led-class (patch 2/3)
- contacted original author of leds-pwm dt binding on license issue
  (patch 3/3)

v3:
- series rebased on v5.9-rc4
- changed license of .yaml file to recommended one (patch 2/2)
- added Acked-by to both patches

v2:
- series rebased on v5.9-rc3
- added the dt-bindings update patch (2/2)

v1:
- based on v5.9-rc2
- backport on v5.4.59 tested and working

Alexander Dahl (3):
  leds: pwm: Remove platform_data support
  leds: pwm: Allow automatic labels for DT based devices
  dt-bindings: leds: Convert pwm to yaml

 .../devicetree/bindings/leds/leds-pwm.txt     | 50 -----------
 .../devicetree/bindings/leds/leds-pwm.yaml    | 82 +++++++++++++++++++
 drivers/leds/leds-pwm.c                       | 33 ++------
 3 files changed, 89 insertions(+), 76 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.yaml


base-commit: 03eb2ca44a95105d1482d5e7471016cf8b383f97
-- 
2.20.1

[PATCH v5 1/3] leds: pwm: Remove platform_data support

[Alexander Dahl]

Since commit 141f15c66d94 ("leds: pwm: remove header") that platform
interface is not usable from outside and there seems to be no in tree
user anymore.  All in-tree users of the leds-pwm driver seem to use DT
currently.  Getting rid of the old platform interface will allow the
leds-pwm driver to switch over from 'devm_led_classdev_register()' to
'devm_led_classdev_register_ext()' later.

Signed-off-by: Alexander Dahl <post@lespocky.de>
Cc: Denis Osterland-Heim <denis.osterland@diehl.com>
Cc: Marek Behún <marek.behun@nic.cz>
---

Notes:
    v5:
     * added this patch to series (replacing another patch with a not
       working, different approach)

 drivers/leds/leds-pwm.c | 30 +++++-------------------------
 1 file changed, 5 insertions(+), 25 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index e35a97c1d828..4e9954f8f7eb 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -25,11 +25,6 @@ struct led_pwm {
 	unsigned int	max_brightness;
 };
 
-struct led_pwm_platform_data {
-	int		num_leds;
-	struct led_pwm	*leds;
-};
-
 struct led_pwm_data {
 	struct led_classdev	cdev;
 	struct pwm_device	*pwm;
@@ -61,6 +56,7 @@ static int led_pwm_set(struct led_classdev *led_cdev,
 	return pwm_apply_state(led_dat->pwm, &led_dat->pwmstate);
 }
 
+__attribute__((nonnull))
 static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 		       struct led_pwm *led, struct fwnode_handle *fwnode)
 {
@@ -74,10 +70,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 	led_data->cdev.max_brightness = led->max_brightness;
 	led_data->cdev.flags = LED_CORE_SUSPENDRESUME;
 
-	if (fwnode)
-		led_data->pwm = devm_fwnode_pwm_get(dev, fwnode, NULL);
-	else
-		led_data->pwm = devm_pwm_get(dev, led->name);
+	led_data->pwm = devm_fwnode_pwm_get(dev, fwnode, NULL);
 	if (IS_ERR(led_data->pwm))
 		return dev_err_probe(dev, PTR_ERR(led_data->pwm),
 				     "unable to request PWM for %s\n",
@@ -143,15 +136,11 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
 
 static int led_pwm_probe(struct platform_device *pdev)
 {
-	struct led_pwm_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct led_pwm_priv *priv;
-	int count, i;
 	int ret = 0;
+	int count;
 
-	if (pdata)
-		count = pdata->num_leds;
-	else
-		count = device_get_child_node_count(&pdev->dev);
+	count = device_get_child_node_count(&pdev->dev);
 
 	if (!count)
 		return -EINVAL;
@@ -161,16 +150,7 @@ static int led_pwm_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
-	if (pdata) {
-		for (i = 0; i < count; i++) {
-			ret = led_pwm_add(&pdev->dev, priv, &pdata->leds[i],
-					  NULL);
-			if (ret)
-				break;
-		}
-	} else {
-		ret = led_pwm_create_fwnode(&pdev->dev, priv);
-	}
+	ret = led_pwm_create_fwnode(&pdev->dev, priv);
 
 	if (ret)
 		return ret;
-- 
2.20.1

Re: [PATCH v5 1/3] leds: pwm: Remove platform_data support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

Hi!

> Since commit 141f15c66d94 ("leds: pwm: remove header") that platform
> interface is not usable from outside and there seems to be no in tree
> user anymore.  All in-tree users of the leds-pwm driver seem to use DT
> currently.  Getting rid of the old platform interface will allow the
> leds-pwm driver to switch over from 'devm_led_classdev_register()' to
> 'devm_led_classdev_register_ext()' later.

> @@ -61,6 +56,7 @@ static int led_pwm_set(struct led_classdev *led_cdev,
>  	return pwm_apply_state(led_dat->pwm, &led_dat->pwmstate);
>  }
>  
> +__attribute__((nonnull))
>  static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>  		       struct led_pwm *led, struct fwnode_handle *fwnode)
>  {

This normally goes elsewhere -- right? I'd expect:


  static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
  		       struct led_pwm *led, struct fwnode_handle *fwnode)
	  __attribute__((nonnull))

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v5 1/3] leds: pwm: Remove platform_data support

[Alexander Dahl]

Hello Pavel,

Am Samstag, 19. September 2020, 11:44:18 CEST schrieb Pavel Machek:
> > Since commit 141f15c66d94 ("leds: pwm: remove header") that platform
> > interface is not usable from outside and there seems to be no in tree
> > user anymore.  All in-tree users of the leds-pwm driver seem to use DT
> > currently.  Getting rid of the old platform interface will allow the
> > leds-pwm driver to switch over from 'devm_led_classdev_register()' to
> > 'devm_led_classdev_register_ext()' later.
> > 
> > @@ -61,6 +56,7 @@ static int led_pwm_set(struct led_classdev *led_cdev,
> > 
> >  	return pwm_apply_state(led_dat->pwm, &led_dat->pwmstate);
> >  
> >  }
> > 
> > +__attribute__((nonnull))
> > 
> >  static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
> >  
> >  		       struct led_pwm *led, struct fwnode_handle *fwnode)
> >  
> >  {
> 
> This normally goes elsewhere -- right? I'd expect:
> 
> 
>   static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>   		       struct led_pwm *led, struct fwnode_handle *fwnode)
> 	  __attribute__((nonnull))

I found both variants in kernel code.  I can live with both variants and have 
no strong preference.

My initial intention to add it was to get a compiler warning in case someone 
does not pass a fwnode here, e.g. when using that old platform_data approach 
(which is supposed to be removed with this patch).  You might call it a self 
check on my own changes.  I can also drop that attribute if you don't want 
that kind of stuff in linux-leds.

Greets
Alex

Re: [PATCH v5 1/3] leds: pwm: Remove platform_data support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1158 bytes --]

Hi!

> > > +__attribute__((nonnull))
> > > 
> > >  static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
> > >  
> > >  		       struct led_pwm *led, struct fwnode_handle *fwnode)
> > >  
> > >  {
> > 
> > This normally goes elsewhere -- right? I'd expect:
> > 
> > 
> >   static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
> >   		       struct led_pwm *led, struct fwnode_handle *fwnode)
> > 	  __attribute__((nonnull))
> 
> I found both variants in kernel code.  I can live with both variants and have 
> no strong preference.
> 
> My initial intention to add it was to get a compiler warning in case someone 
> does not pass a fwnode here, e.g. when using that old platform_data approach 
> (which is supposed to be removed with this patch).  You might call it a self 
> check on my own changes.  I can also drop that attribute if you don't want 
> that kind of stuff in linux-leds.

I'm okay with it at the second place :-).

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v5 1/3] leds: pwm: Remove platform_data support

[Alexander Dahl]

[-- Attachment #1: Type: text/plain, Size: 1952 bytes --]

Hello Pavel,

On Wed, Sep 30, 2020 at 07:24:41PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > +__attribute__((nonnull))
> > > > 
> > > >  static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
> > > >  
> > > >  		       struct led_pwm *led, struct fwnode_handle *fwnode)
> > > >  
> > > >  {
> > > 
> > > This normally goes elsewhere -- right? I'd expect:
> > > 
> > > 
> > >   static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
> > >   		       struct led_pwm *led, struct fwnode_handle *fwnode)
> > > 	  __attribute__((nonnull))
> > 
> > I found both variants in kernel code.  I can live with both variants and have 
> > no strong preference.
> > 
> > My initial intention to add it was to get a compiler warning in case someone 
> > does not pass a fwnode here, e.g. when using that old platform_data approach 
> > (which is supposed to be removed with this patch).  You might call it a self 
> > check on my own changes.  I can also drop that attribute if you don't want 
> > that kind of stuff in linux-leds.
> 
> I'm okay with it at the second place :-).

Should have tried this before, but I actually did now. O:-)

If I move the attribute behind, I get this on a W=1 build:

    CC      drivers/leds/leds-pwm.o
  /home/alex/src/linux/leds/drivers/leds/leds-pwm.c:58:1: error: attributes should be specified before the declarator in a function definition
   static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
   ^~~~~~

Because it won't build then, I'll keep it where it is.  Meanwhile I
worked on all the DT remarks by Rob and I will send v6 soon.

Greets
Alex

-- 
/"\ ASCII RIBBON | »With the first link, the chain is forged. The first
\ / CAMPAIGN     | speech censured, the first thought forbidden, the
 X  AGAINST      | first freedom denied, chains us all irrevocably.«
/ \ HTML MAIL    | (Jean-Luc Picard, quoting Judge Aaron Satie)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 1/3] leds: pwm: Remove platform_data support

[Marek Behun]

Besides Pavel's note about the __attribute__((nonnull)) position

Reviewed-by: Marek Behún <marek.behun@nic.cz>

[PATCH v5 2/3] leds: pwm: Allow automatic labels for DT based devices

[Alexander Dahl]

If LEDs are configured through device tree and the property 'label' is
omitted, the label is supposed to be generated from the properties
'function' and 'color' if present.  While this works fine for e.g. the
'leds-gpio' driver, it did not for 'leds-pwm'.

The reason is, you get this label naming magic only if you add a LED
device through 'devm_led_classdev_register_ext()' and pass a pointer to
the current device tree node.

For the following node from dts the LED appeared as 'led-5' in sysfs
before and as 'red:debug' after this change.

        pwm_leds {
                compatible = "pwm-leds";

                led-5 {
                        function = LED_FUNCTION_DEBUG;
                        color = <LED_COLOR_ID_RED>;
                        pwms = <&pwm0 2 10000000 0>;
                        max-brightness = <127>;

                        linux,default-trigger = "heartbeat";
                        panic-indicator;
                };
        };

Signed-off-by: Alexander Dahl <post@lespocky.de>
Cc: Marek Behún <marek.behun@nic.cz>
---

Notes:
    v4 -> v5:
      * updated commit message
    
    v3 -> v4:
      * simplified implementation based on a new change in led-core
      * removed Acked-by due to changed implementation
    
    v2 -> v3:
      * added Acked-by
    
    v1 -> v2:
      * no change to this patch

 drivers/leds/leds-pwm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 4e9954f8f7eb..8881d465701d 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -61,6 +61,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 		       struct led_pwm *led, struct fwnode_handle *fwnode)
 {
 	struct led_pwm_data *led_data = &priv->leds[priv->num_leds];
+	struct led_init_data init_data = { .fwnode = fwnode };
 	int ret;
 
 	led_data->active_low = led->active_low;
@@ -80,7 +81,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 
 	pwm_init_state(led_data->pwm, &led_data->pwmstate);
 
-	ret = devm_led_classdev_register(dev, &led_data->cdev);
+	ret = devm_led_classdev_register_ext(dev, &led_data->cdev, &init_data);
 	if (ret) {
 		dev_err(dev, "failed to register PWM led for %s: %d\n",
 			led->name, ret);
-- 
2.20.1

Re: [PATCH v5 2/3] leds: pwm: Allow automatic labels for DT based devices

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

On Sat 2020-09-19 07:31:44, Alexander Dahl wrote:
> If LEDs are configured through device tree and the property 'label' is
> omitted, the label is supposed to be generated from the properties
> 'function' and 'color' if present.  While this works fine for e.g. the
> 'leds-gpio' driver, it did not for 'leds-pwm'.
> 
> The reason is, you get this label naming magic only if you add a LED
> device through 'devm_led_classdev_register_ext()' and pass a pointer to
> the current device tree node.
> 
> For the following node from dts the LED appeared as 'led-5' in sysfs
> before and as 'red:debug' after this change.

Thanks, applied.
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Alexander Dahl]

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

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

[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/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.

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

[Alexander Dahl]

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

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

[Rob Herring]

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

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

[Rob Herring]

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
> 

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

[Alexander Dahl]

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

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

[Rob Herring]

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