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

[PATCH v4 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 changes led-core a bit to add a non-ugly fix for the
leds-pwm driver and takes the opportunity to update the leds-pwm
dt-bindings accordingly.

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

Greets
Alex

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: Require valid fwnode pointer for composing name
  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    | 85 +++++++++++++++++++
 drivers/leds/led-class.c                      |  2 +-
 drivers/leds/leds-pwm.c                       |  3 +-
 4 files changed, 88 insertions(+), 52 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.yaml

-- 
2.20.1

[PATCH v4 1/3] leds: Require valid fwnode pointer for composing name

[Alexander Dahl]

The function 'led_compose_name()' is called in
'led_classdev_register_ext(()' only and in its implementation it always
parses the fwnode passed with the init_data struct.  If there's no
fwnode, EINVAL is returned and 'led_classdev_register_ext()' returns
early.

If this is detected early the same fallback mechanism can be used , as
if init_data itself is NULL.  This will allow drivers to pass fully
populated 'init_data' or sparse initialized 'init_data' with a NULL
fwnode in a more elegant way with only one function call.

Fixes: bb4e9af0348d ("leds: core: Add support for composing LED class device names")
Suggested-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Alexander Dahl <post@lespocky.de>
---

Notes:
    v4:
      * added this patch to series (Suggested-by: Pavel Machek)

 drivers/leds/led-class.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index cc3929f858b6..3da50c7ecfe7 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -346,7 +346,7 @@ int led_classdev_register_ext(struct device *parent,
 	const char *proposed_name = composed_name;
 	int ret;
 
-	if (init_data) {
+	if (init_data && init_data->fwnode) {
 		if (init_data->devname_mandatory && !init_data->devicename) {
 			dev_err(parent, "Mandatory device name is missing");
 			return -EINVAL;
-- 
2.20.1

[PATCH v4 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.

The approach to fix this requires a change to led-class checking on
init_data->fwnode being NULL early.

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

        pwm_leds {
                compatible = "pwm-leds";

                led5 {
                        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>
---

Notes:
    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 ef7b91bd2064..9017d3e7dd94 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -65,6 +65,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;
@@ -90,7 +91,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

[PATCH v4 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>
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
---

Notes:
    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    | 85 +++++++++++++++++++
 2 files changed, 85 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..c74867492424
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-pwm.yaml
@@ -0,0 +1,85 @@
+# 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:
+  "^pwm-led-([0-9a-f])$":
+    type: object
+
+    $ref: common.yaml#
+
+    properties:
+      pwms:
+        description:
+          "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:
+        description:
+          "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:
+        description:
+          Maximum brightness possible for the LED
+
+      active-low:
+        description:
+          For PWMs where the LED is wired to supply rather than ground.
+
+    required:
+      - pwms
+      - max-brightness
+
+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";
+
+        pwm-led-1 {
+            label = "omap4::keypad";
+            pwms = <&twl_pwm 0 7812500>;
+            max-brightness = <127>;
+        };
+
+        pwm-led-2 {
+            color = <LED_COLOR_ID_GREEN>;
+            function = LED_FUNCTION_CHARGING;
+            pwms = <&twl_pwmled 0 7812500>;
+            max-brightness = <255>;
+        };
+    };
+
+...
-- 
2.20.1

Re: [PATCH v4 1/3] leds: Require valid fwnode pointer for composing name

[Jacek Anaszewski]

Hi Alexander,

On 9/11/20 5:40 PM, Alexander Dahl wrote:
> The function 'led_compose_name()' is called in
> 'led_classdev_register_ext(()' only and in its implementation it always
> parses the fwnode passed with the init_data struct.  If there's no
> fwnode, EINVAL is returned and 'led_classdev_register_ext()' returns
> early.
> 
> If this is detected early the same fallback mechanism can be used , as
> if init_data itself is NULL.  This will allow drivers to pass fully
> populated 'init_data' or sparse initialized 'init_data' with a NULL
> fwnode in a more elegant way with only one function call.
> 
> Fixes: bb4e9af0348d ("leds: core: Add support for composing LED class device names")
> Suggested-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Alexander Dahl <post@lespocky.de>
> ---
> 
> Notes:
>      v4:
>        * added this patch to series (Suggested-by: Pavel Machek)
> 
>   drivers/leds/led-class.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index cc3929f858b6..3da50c7ecfe7 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -346,7 +346,7 @@ int led_classdev_register_ext(struct device *parent,
>   	const char *proposed_name = composed_name;
>   	int ret;
>   
> -	if (init_data) {
> +	if (init_data && init_data->fwnode) {

This does not cover the case when we don't have fwnode but we
have init_data->default_label that led_compose_name() can make use of.

>   		if (init_data->devname_mandatory && !init_data->devicename) {
>   			dev_err(parent, "Mandatory device name is missing");
>   			return -EINVAL;
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 1/3] leds: Require valid fwnode pointer for composing name

[Alexander Dahl]

Hello Jacek,

thanks for your feedback. See below.

Am Freitag, 11. September 2020, 23:26:43 CEST schrieb Jacek Anaszewski:
> On 9/11/20 5:40 PM, Alexander Dahl wrote:
> > The function 'led_compose_name()' is called in
> > 'led_classdev_register_ext(()' only and in its implementation it always
> > parses the fwnode passed with the init_data struct.  If there's no
> > fwnode, EINVAL is returned and 'led_classdev_register_ext()' returns
> > early.
> > 
> > If this is detected early the same fallback mechanism can be used , as
> > if init_data itself is NULL.  This will allow drivers to pass fully
> > populated 'init_data' or sparse initialized 'init_data' with a NULL
> > fwnode in a more elegant way with only one function call.
> > 
> > Fixes: bb4e9af0348d ("leds: core: Add support for composing LED class
> > device names") Suggested-by: Pavel Machek <pavel@ucw.cz>
> > Signed-off-by: Alexander Dahl <post@lespocky.de>
> > ---
> > 
> > Notes:
> >      v4:
> >        * added this patch to series (Suggested-by: Pavel Machek)
> >   
> >   drivers/leds/led-class.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index cc3929f858b6..3da50c7ecfe7 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -346,7 +346,7 @@ int led_classdev_register_ext(struct device *parent,
> > 
> >   	const char *proposed_name = composed_name;
> >   	int ret;
> > 
> > -	if (init_data) {
> > +	if (init_data && init_data->fwnode) {
> 
> This does not cover the case when we don't have fwnode but we
> have init_data->default_label that led_compose_name() can make use of.
> 
> >   		if (init_data->devname_mandatory && !init_data->devicename) {
> >   		
> >   			dev_err(parent, "Mandatory device name is missing");
> >   			return -EINVAL;

You're right, I missed that part in that if/else if construct in 
led_compose_name() … I looked at the code for some more time now and could not 
come up with an elegant change to the led-core or led-class. :-/

However I also had another look at leds-pwm and for me it seems that it is 
used by fwnode (DT, ACPI, ??) based devices only.  I could not find a single 
user of leds-pwm as a platform driver, which is probably why 141f15c66d94 
("leds: pwm: remove header") was possible?

I had a look at the history of the leds-pwm driver and when introduced in 2009 
platform based board files where a thing, no dt, of, or fwnode yet, at least 
for arm, right?  Device tree support for leds-pwm was added in 2012 by Peter 
Ujfalusi.

So if those code paths in leds-pwm are not used anymore, what about dropping 
that platform support in leds-pwm driver?  That would mean we always have 
fwnode non-null and would not require a change in led-class at all?

Greets
Alex

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

[Rob Herring]

On Fri, Sep 11, 2020 at 05:40:04PM +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>
> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
> 
> Notes:
>     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    | 85 +++++++++++++++++++
>  2 files changed, 85 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..c74867492424
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-pwm.yaml
> @@ -0,0 +1,85 @@
> +# 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:
> +  "^pwm-led-([0-9a-f])$":

'^led-([0-9a-f])' would be my preference. A bit more on that below.

What about a single child case?

> +    type: object
> +
> +    $ref: common.yaml#
> +
> +    properties:
> +      pwms:
> +        description:
> +          "PWM property to point to the PWM device (phandle)/port (id)
> +          and to specify the period time to be used:
> +          <&phandle id period_ns>;"

No need to redefine a common property.

What is needed is how many pwms? I'd assume 1 only: 'maxItems: 1'

> +
> +      pwm-names:
> +        description:
> +          "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"

Same here.

> +
> +      max-brightness:
> +        description:
> +          Maximum brightness possible for the LED

Needs a type $ref.

> +
> +      active-low:
> +        description:
> +          For PWMs where the LED is wired to supply rather than ground.

type: boolean

> +
> +    required:
> +      - pwms
> +      - max-brightness

additionalProperties: false

That will cause errors if child node names were not consistent (no one 
checked, so they won't be). We could just allow anything, but I prefer 
to move things to be consistent yet try to capture any existing pattern.

> +
> +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";
> +
> +        pwm-led-1 {
> +            label = "omap4::keypad";
> +            pwms = <&twl_pwm 0 7812500>;
> +            max-brightness = <127>;
> +        };
> +
> +        pwm-led-2 {
> +            color = <LED_COLOR_ID_GREEN>;
> +            function = LED_FUNCTION_CHARGING;
> +            pwms = <&twl_pwmled 0 7812500>;
> +            max-brightness = <255>;
> +        };
> +    };
> +
> +...
> -- 
> 2.20.1
> 

Re: [PATCH v4 1/3] leds: Require valid fwnode pointer for composing name

[Jacek Anaszewski]

Hi Alexander,

On 9/15/20 11:14 AM, Alexander Dahl wrote:
> Hello Jacek,
> 
> thanks for your feedback. See below.
> 
> Am Freitag, 11. September 2020, 23:26:43 CEST schrieb Jacek Anaszewski:
>> On 9/11/20 5:40 PM, Alexander Dahl wrote:
>>> The function 'led_compose_name()' is called in
>>> 'led_classdev_register_ext(()' only and in its implementation it always
>>> parses the fwnode passed with the init_data struct.  If there's no
>>> fwnode, EINVAL is returned and 'led_classdev_register_ext()' returns
>>> early.
>>>
>>> If this is detected early the same fallback mechanism can be used , as
>>> if init_data itself is NULL.  This will allow drivers to pass fully
>>> populated 'init_data' or sparse initialized 'init_data' with a NULL
>>> fwnode in a more elegant way with only one function call.
>>>
>>> Fixes: bb4e9af0348d ("leds: core: Add support for composing LED class
>>> device names") Suggested-by: Pavel Machek <pavel@ucw.cz>
>>> Signed-off-by: Alexander Dahl <post@lespocky.de>
>>> ---
>>>
>>> Notes:
>>>       v4:
>>>         * added this patch to series (Suggested-by: Pavel Machek)
>>>    
>>>    drivers/leds/led-class.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index cc3929f858b6..3da50c7ecfe7 100644
>>> --- a/drivers/leds/led-class.c
>>> +++ b/drivers/leds/led-class.c
>>> @@ -346,7 +346,7 @@ int led_classdev_register_ext(struct device *parent,
>>>
>>>    	const char *proposed_name = composed_name;
>>>    	int ret;
>>>
>>> -	if (init_data) {
>>> +	if (init_data && init_data->fwnode) {
>>
>> This does not cover the case when we don't have fwnode but we
>> have init_data->default_label that led_compose_name() can make use of.
>>
>>>    		if (init_data->devname_mandatory && !init_data->devicename) {
>>>    		
>>>    			dev_err(parent, "Mandatory device name is missing");
>>>    			return -EINVAL;
> 
> You're right, I missed that part in that if/else if construct in
> led_compose_name() … I looked at the code for some more time now and could not
> come up with an elegant change to the led-core or led-class. :-/
> 
> However I also had another look at leds-pwm and for me it seems that it is
> used by fwnode (DT, ACPI, ??) based devices only.  I could not find a single
> user of leds-pwm as a platform driver, which is probably why 141f15c66d94
> ("leds: pwm: remove header") was possible?

In fact it looks like that patch was pointless, since it precluded the
use of struct led_pwm_platform_data anywhere besides the leds-pwm
driver.

> I had a look at the history of the leds-pwm driver and when introduced in 2009
> platform based board files where a thing, no dt, of, or fwnode yet, at least
> for arm, right?  Device tree support for leds-pwm was added in 2012 by Peter
> Ujfalusi.
> 
> So if those code paths in leds-pwm are not used anymore, what about dropping
> that platform support in leds-pwm driver?  That would mean we always have
> fwnode non-null and would not require a change in led-class at all?

git grep led_pwm_platform_data in fact shows only references in
leds-pwm.c, so yes, I think the platform support seems to be redundant.

-- 
Best regards,
Jacek Anaszewski

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

[Alexander Dahl]

Hello Rob,

thanks for your feedback. I have some questions/remarks on this new yaml 
binding stuff before sending v5 (which will also replace patch 1/3 with a 
different approach btw).

Am Dienstag, 15. September 2020, 22:37:35 CEST schrieb Rob Herring:
> On Fri, Sep 11, 2020 at 05:40:04PM +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>
> > Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > ---
> > 
> > Notes:
> >     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    | 85 +++++++++++++++++++
> >  2 files changed, 85 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..c74867492424
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/leds-pwm.yaml
> > @@ -0,0 +1,85 @@
> > +# 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:
> 
> > +  "^pwm-led-([0-9a-f])$":
> '^led-([0-9a-f])' would be my preference. A bit more on that below.

Fine for me.

> What about a single child case?

One child or multiple childs.  I found .dts files with one to four sub-nodes 
of the pwm-leds device in current master.

> > +    type: object
> > +
> > +    $ref: common.yaml#
> > +
> > +    properties:
> > +      pwms:
> > +        description:
> > +          "PWM property to point to the PWM device (phandle)/port (id)
> > +          and to specify the period time to be used:
> > +          <&phandle id period_ns>;"
> 
> No need to redefine a common property.

Should this look like in 'Documentation/devicetree/bindings/leds/backlight/
pwm-backlight.yaml' then?

> What is needed is how many pwms? I'd assume 1 only: 'maxItems: 1'

Yes, one pwm channel per LED.

> > +
> > +      pwm-names:
> > +        description:
> > +          "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"
> 
> Same here.
> 
> > +
> > +      max-brightness:
> > +        description:
> > +          Maximum brightness possible for the LED
> 
> Needs a type $ref.

fwnode_property_read_u32() is used to read this.

> 
> > +
> > +      active-low:
> > +        description:
> > +          For PWMs where the LED is wired to supply rather than ground.
> 
> type: boolean
> 
> > +
> > +    required:
> > +      - pwms
> > +      - max-brightness
> 
> additionalProperties: false
> 
> That will cause errors if child node names were not consistent (no one
> checked, so they won't be). We could just allow anything, but I prefer
> to move things to be consistent yet try to capture any existing pattern.

Child node names follow no scheme at all currently as far as I could see, 
examples from real current .dts files:

  panel, led-red, blueled, kpad, front, green, pwm_blue, ds1, network_red, 
alarm-brightness, pmu_stat, overo, heartbeat, power, …

Greets
Alex

> 
> > +
> > +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";
> > +
> > +        pwm-led-1 {
> > +            label = "omap4::keypad";
> > +            pwms = <&twl_pwm 0 7812500>;
> > +            max-brightness = <127>;
> > +        };
> > +
> > +        pwm-led-2 {
> > +            color = <LED_COLOR_ID_GREEN>;
> > +            function = LED_FUNCTION_CHARGING;
> > +            pwms = <&twl_pwmled 0 7812500>;
> > +            max-brightness = <255>;
> > +        };
> > +    };
> > +
> > +...

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

[Rob Herring]

On Fri, Sep 18, 2020 at 9:12 AM Alexander Dahl <ada@thorsis.com> wrote:
>
> Hello Rob,
>
> thanks for your feedback. I have some questions/remarks on this new yaml
> binding stuff before sending v5 (which will also replace patch 1/3 with a
> different approach btw).
>
> Am Dienstag, 15. September 2020, 22:37:35 CEST schrieb Rob Herring:
> > On Fri, Sep 11, 2020 at 05:40:04PM +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>
> > > Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > > ---
> > >
> > > Notes:
> > >     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    | 85 +++++++++++++++++++
> > >  2 files changed, 85 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..c74867492424
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/leds/leds-pwm.yaml
> > > @@ -0,0 +1,85 @@
> > > +# 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:
> >
> > > +  "^pwm-led-([0-9a-f])$":
> > '^led-([0-9a-f])' would be my preference. A bit more on that below.
>
> Fine for me.
>
> > What about a single child case?
>
> One child or multiple childs.  I found .dts files with one to four sub-nodes
> of the pwm-leds device in current master.

For the single child case, you need to allow for just 'led' then. So
this I think: '^led(-[0-9a-f]+)?$'

> > > +    type: object
> > > +
> > > +    $ref: common.yaml#
> > > +
> > > +    properties:
> > > +      pwms:
> > > +        description:
> > > +          "PWM property to point to the PWM device (phandle)/port (id)
> > > +          and to specify the period time to be used:
> > > +          <&phandle id period_ns>;"
> >
> > No need to redefine a common property.
>
> Should this look like in 'Documentation/devicetree/bindings/leds/backlight/
> pwm-backlight.yaml' then?

Yes.

>
> > What is needed is how many pwms? I'd assume 1 only: 'maxItems: 1'
>
> Yes, one pwm channel per LED.
>
> > > +
> > > +      pwm-names:
> > > +        description:
> > > +          "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"
> >
> > Same here.
> >
> > > +
> > > +      max-brightness:
> > > +        description:
> > > +          Maximum brightness possible for the LED
> >
> > Needs a type $ref.
>
> fwnode_property_read_u32() is used to read this.

$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
> >
> > That will cause errors if child node names were not consistent (no one
> > checked, so they won't be). We could just allow anything, but I prefer
> > to move things to be consistent yet try to capture any existing pattern.
>
> Child node names follow no scheme at all currently as far as I could see,
> examples from real current .dts files:
>
>   panel, led-red, blueled, kpad, front, green, pwm_blue, ds1, network_red,
> alarm-brightness, pmu_stat, overo, heartbeat, power, …

So you can do this to allow any child node name:

additonalProperties:
  type: object
  $ref: common.yaml#

  ... everything you have under pwm-led-*

Or how gpio-keys.yaml was done also works.

Rob