leds: pwm: Make automatic labels work

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.

Quoted 19 lines

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.

drivers/leds/leds-pwm.c +1 -0

@@ -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)

Quoted 1 line

{

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))

Quoted 6 lines

Quoted 6 lines

drivers/leds/leds-pwm.c +1 -0

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

Quoted 8 lines

Quoted 1 line

{

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.

Hi!

Quoted 18 lines

Quoted 6 lines

Quoted 6 lines

drivers/leds/leds-pwm.c +1 -0

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

Quoted 8 lines

Quoted 1 line

{

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.

Quoted 5 lines

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 :-).

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.

The example was adapted slightly to make use of the 'function' and 'color' properties. License discussed with the original author.

Suggested-by Jacek Anaszewski <[email protected]>

Signed-off-by Alexander Dahl <[email protected]>

Cc Peter Ujfalusi <[email protected]>

---

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

Quoted 20 lines

The example was adapted slightly to make use of the 'function' and 'color' properties. License discussed with the original author.

Suggested-by Jacek Anaszewski <[email protected]>

Signed-off-by Alexander Dahl <[email protected]>

Cc Peter Ujfalusi <[email protected]>

---

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

Quoted 8 lines

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

Please check and re-submit.

Hello Rob,

Am Dienstag, 22. September 2020, 17:42:58 CEST schrieb Rob Herring:

Quoted 30 lines · Alexander Dahl

Quoted 20 lines

The example was adapted slightly to make use of the 'function' and 'color' properties. License discussed with the original author.

Suggested-by Jacek Anaszewski <[email protected]>

Signed-off-by Alexander Dahl <[email protected]>

Cc Peter Ujfalusi <[email protected]>

---

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

Quoted 8 lines

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

Quoted 7 lines

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?

The example was adapted slightly to make use of the 'function' and 'color' properties. License discussed with the original author.

Suggested-by Jacek Anaszewski <[email protected]>

Signed-off-by Alexander Dahl <[email protected]>

Cc Peter Ujfalusi <[email protected]>

---

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

Documentation/devicetree/bindings/leds/leds-pwm.txt deleted +0 -50

deleted file mode 100644
index 6c6583c35f2f..000000000000
@@ -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>;
-	};
-};

Documentation/devicetree/bindings/leds/leds-pwm.yaml new file +53 -0

new file mode 100644
index 000000000000..c9316811c7f1
@@ -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 <[email protected]>
+
+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 {

Documentation/devicetree/bindings/leds/leds-pwm.yaml new file +17 -0

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

Signature · 1 line

2.20.1

Use generic node names:

led-controller {

Quoted 9 lines

Documentation/devicetree/bindings/leds/leds-pwm.yaml new file +9 -0

+        compatible = "pwm-leds";
+
+        led-1 {
+            label = "omap4::keypad";
+            pwms = <&twl_pwm 0 7812500>;
+            max-brightness = <127>;
+        };
+
+        led-2 {

Documentation/devicetree/bindings/leds/leds-pwm.yaml line 82

Quoted 8 lines

Documentation/devicetree/bindings/leds/leds-pwm.yaml new file +8 -0

+            color = <LED_COLOR_ID_GREEN>;
+            function = LED_FUNCTION_CHARGING;
+            pwms = <&twl_pwmled 0 7812500>;
+            max-brightness = <255>;
+        };
+    };
+
+...

Hei hei,

Am Dienstag, 22. September 2020, 17:57:47 CEST schrieb Rob Herring:

Quoted 13 lines

Use generic node names:

led-controller {

Quoted 9 lines

Documentation/devicetree/bindings/leds/leds-pwm.yaml new file +9 -0

+        compatible = "pwm-leds";
+
+        led-1 {
+            label = "omap4::keypad";
+            pwms = <&twl_pwm 0 7812500>;
+            max-brightness = <127>;
+        };
+
+        led-2 {

Quoted 8 lines

Documentation/devicetree/bindings/leds/leds-pwm.yaml line 82

Quoted 8 lines

Documentation/devicetree/bindings/leds/leds-pwm.yaml new file +8 -0

+            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 {
    …
  };