[PATCH v3 0/2] leds: pwm: Make automatic labels work

[PATCH v3 0/2] 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 fixes the driver and takes the opportunity to update the
dt-bindings accordingly.

v3:
- rebased on v5.9-rc4
- changed license of .yaml file to recommended one
- added Acked-by

v2:
- rebased on v5.9-rc3
- added the dt-bindings update patch

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

Greets
Alex

Alexander Dahl (2):
  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/leds-pwm.c                       |  9 +-
 3 files changed, 93 insertions(+), 51 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 v3 1/2] 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 was adopted from
the 'leds-gpio' driver.

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>
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
---

Notes:
    v2 -> v3:
      * added Acked-by

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

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index ef7b91bd2064..a27a1d75a3e9 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 = {};
 	int ret;
 
 	led_data->active_low = led->active_low;
@@ -90,7 +91,13 @@ 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);
+	if (fwnode) {
+		init_data.fwnode = fwnode;
+		ret = devm_led_classdev_register_ext(dev, &led_data->cdev,
+						     &init_data);
+	} else {
+		ret = devm_led_classdev_register(dev, &led_data->cdev);
+	}
 	if (ret) {
 		dev_err(dev, "failed to register PWM led for %s: %d\n",
 			led->name, ret);
-- 
2.20.1

[PATCH v3 2/2] dt-bindings: leds: Convert pwm to yaml

[Alexander Dahl]

The example was adapted slightly to make use of the 'function' and
'color' properties.

Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Signed-off-by: Alexander Dahl <post@lespocky.de>
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
---

Notes:
    v2 -> v3:
      * change license identifier to recommended one
      * added Acked-by

 .../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 v3 1/2] leds: pwm: Allow automatic labels for DT based devices

[Pavel Machek]

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

Hi!

>  	pwm_init_state(led_data->pwm, &led_data->pwmstate);
>  
> -	ret = devm_led_classdev_register(dev, &led_data->cdev);
> +	if (fwnode) {
> +		init_data.fwnode = fwnode;
> +		ret = devm_led_classdev_register_ext(dev, &led_data->cdev,
> +						     &init_data);
> +	} else {
> +		ret = devm_led_classdev_register(dev, &led_data->cdev);
> +	}

Can you always use _ext version, even with null fwnode? If not, can
you fix the core to accept that? Having that conditional in driver is
ugly.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Alexander Dahl]

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

Hei hei,

On Wed, Sep 09, 2020 at 11:07:36AM +0200, Pavel Machek wrote:
> Hi!
> 
> >  	pwm_init_state(led_data->pwm, &led_data->pwmstate);
> >  
> > -	ret = devm_led_classdev_register(dev, &led_data->cdev);
> > +	if (fwnode) {
> > +		init_data.fwnode = fwnode;
> > +		ret = devm_led_classdev_register_ext(dev, &led_data->cdev,
> > +						     &init_data);
> > +	} else {
> > +		ret = devm_led_classdev_register(dev, &led_data->cdev);
> > +	}
> 
> Can you always use _ext version, even with null fwnode? 

I did not try on real hardware, but from reading the code I would say
the following would happen: led_classdev_register_ext() calls
led_compose_name(parent, init_data, composed_name) which itself calls
led_parse_fwnode_props(dev, fwnode, &props); that returns early due to
fwnode==NULL without changing props, thus this stays as initialized
with {}, so led_compose_name() would return -EINVAL which would let
led_classdev_register_ext() fail, too.

> If not, can you fix the core to accept that? Having that conditional
> in driver is ugly.

It is ugly, although the approach is inspired by the leds-gpio driver.
I'll see if I can come up with a change to led-core, but I'm also open
for suggestions. ;-)

fyi: Peter Ujfalusi answered and would give his Ack to the changed
dual license for the yaml file.  You can expect that for v4.

Stay tuned
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 v3 1/2] leds: pwm: Allow automatic labels for DT based devices

[Jacek Anaszewski]

Hi Alexander,

On 9/9/20 10:29 PM, Alexander Dahl wrote:
> Hei hei,
> 
> On Wed, Sep 09, 2020 at 11:07:36AM +0200, Pavel Machek wrote:
>> Hi!
>>
>>>   	pwm_init_state(led_data->pwm, &led_data->pwmstate);
>>>   
>>> -	ret = devm_led_classdev_register(dev, &led_data->cdev);
>>> +	if (fwnode) {
>>> +		init_data.fwnode = fwnode;
>>> +		ret = devm_led_classdev_register_ext(dev, &led_data->cdev,
>>> +						     &init_data);
>>> +	} else {
>>> +		ret = devm_led_classdev_register(dev, &led_data->cdev);
>>> +	}
>>
>> Can you always use _ext version, even with null fwnode?
> 
> I did not try on real hardware, but from reading the code I would say
> the following would happen: led_classdev_register_ext() calls
> led_compose_name(parent, init_data, composed_name) which itself calls
> led_parse_fwnode_props(dev, fwnode, &props); that returns early due to
> fwnode==NULL without changing props, thus this stays as initialized
> with {}, so led_compose_name() would return -EINVAL which would let
> led_classdev_register_ext() fail, too.
> 
>> If not, can you fix the core to accept that? Having that conditional
>> in driver is ugly.
> 
> It is ugly, although the approach is inspired by the leds-gpio driver.
> I'll see if I can come up with a change to led-core, but I'm also open
> for suggestions. ;-)

devm_led_classdev_register() calls devm_led_classdev_register_ext()
with NULL passed in place of init_data, so you could do something like
below to achieve the same without touching LED core:

struct led_init_data init_data_impl = { .fwnode = fwnode };
struct led_init_data *init_data = NULL;

if (fwnode)
	init_data = &init_data_impl;

devm_led_classdev_register_ext(dev, &led_data->cdev, init_data);

> fyi: Peter Ujfalusi answered and would give his Ack to the changed
> dual license for the yaml file.  You can expect that for v4.
> 
> Stay tuned
> Alex
> 

-- 
Best regards,
Jacek Anaszewski

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

[Pavel Machek]

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

Hi!

> >>>  	pwm_init_state(led_data->pwm, &led_data->pwmstate);
> >>>-	ret = devm_led_classdev_register(dev, &led_data->cdev);
> >>>+	if (fwnode) {
> >>>+		init_data.fwnode = fwnode;
> >>>+		ret = devm_led_classdev_register_ext(dev, &led_data->cdev,
> >>>+						     &init_data);
> >>>+	} else {
> >>>+		ret = devm_led_classdev_register(dev, &led_data->cdev);
> >>>+	}
> >>
> >>Can you always use _ext version, even with null fwnode?
> >
> >I did not try on real hardware, but from reading the code I would say
> >the following would happen: led_classdev_register_ext() calls
> >led_compose_name(parent, init_data, composed_name) which itself calls
> >led_parse_fwnode_props(dev, fwnode, &props); that returns early due to
> >fwnode==NULL without changing props, thus this stays as initialized
> >with {}, so led_compose_name() would return -EINVAL which would let
> >led_classdev_register_ext() fail, too.
> >
> >>If not, can you fix the core to accept that? Having that conditional
> >>in driver is ugly.
> >
> >It is ugly, although the approach is inspired by the leds-gpio driver.
> >I'll see if I can come up with a change to led-core, but I'm also open
> >for suggestions. ;-)
> 
> devm_led_classdev_register() calls devm_led_classdev_register_ext()
> with NULL passed in place of init_data, so you could do something like
> below to achieve the same without touching LED core:
> 
> struct led_init_data init_data_impl = { .fwnode = fwnode };
> struct led_init_data *init_data = NULL;
> 
> if (fwnode)
> 	init_data = &init_data_impl;
> 
> devm_led_classdev_register_ext(dev, &led_data->cdev, init_data);

Umm.. This is not too great, either. Maybe I'd really prefer the
change to the LED core.

> >fyi: Peter Ujfalusi answered and would give his Ack to the changed
> >dual license for the yaml file.  You can expect that for v4.

Good :-).

Best regards,
									pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]