[PATCH v6 1/3] dt-bindings: leds: Optional multi-led unit address

[PATCH v6 1/3] dt-bindings: leds: Optional multi-led unit address

[sven]

From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

The unit address does not make sense in all cases the multi-led node is
used, e.g. for the upcoming PWM multi-color LED driver.

Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
---

Notes:
    Added in v6

 .../devicetree/bindings/leds/leds-class-multicolor.yaml         | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
index 37445c68cdef..f41d021ed677 100644
--- a/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
@@ -20,7 +20,7 @@ description: |
   within this documentation directory.
 
 patternProperties:
-  "^multi-led@([0-9a-f])$":
+  "^multi-led(@[0-9a-f])?$":
     type: object
     description: Represents the LEDs that are to be grouped.
     properties:
-- 
2.35.1

[PATCH v6 2/3] dt-bindings: leds: Add multicolor PWM LED bindings

[sven]

From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

This allows to group multiple PWM-connected monochrome LEDs into
multicolor LEDs, e.g. RGB LEDs.

Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
---

Notes:
    Changes in v6:
    * Fix device tree binding schema
    
    Changes in v5:
    * (no changes)
    
    Changes in v4:
    * (no changes)
    
    Changes in v3:
    * Remove multi-led unit name

 .../bindings/leds/leds-pwm-multicolor.yaml    | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml b/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
new file mode 100644
index 000000000000..f7ce29c8ae63
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-pwm-multicolor.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Multi-color LEDs connected to PWM
+
+maintainers:
+  - Sven Schwermer <sven.schwermer@disruptive-technologies.com>
+
+description: |
+  This driver combines several monochrome PWM LEDs into one multi-color
+  LED using the multicolor LED class.
+
+properties:
+  compatible:
+    const: pwm-leds-multicolor
+  multi-led:
+    type: object
+    patternProperties:
+      "^led-[0-9a-z]+$":
+        type: object
+        $ref: common.yaml#
+        properties:
+          pwms:
+            maxItems: 1
+          pwm-names: true
+          color: true
+        required:
+          - pwms
+          - color
+        additionalProperties: false
+required:
+  - compatible
+allOf:
+  - $ref: leds-class-multicolor.yaml#
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    rgb-led {
+        compatible = "pwm-leds-multicolor";
+
+        multi-led {
+          color = <LED_COLOR_ID_RGB>;
+          function = LED_FUNCTION_INDICATOR;
+          max-brightness = <65535>;
+
+          led-red {
+              pwms = <&pwm1 0 1000000>;
+              color = <LED_COLOR_ID_RED>;
+          };
+
+          led-green {
+              pwms = <&pwm2 0 1000000>;
+              color = <LED_COLOR_ID_GREEN>;
+          };
+
+          led-blue {
+              pwms = <&pwm3 0 1000000>;
+              color = <LED_COLOR_ID_BLUE>;
+          };
+        };
+    };
+
+...
-- 
2.35.1

[PATCH v6 3/3] leds: Add PWM multicolor driver

[sven]

From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

By allowing to group multiple monochrome PWM LEDs into multicolor LEDs,
all involved LEDs can be controlled in-sync. This enables using effects
using triggers, etc.

Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---

Notes:
    Changes in v6:
    * (no changes)
    
    Changes in v5:
    * Factor iteration over subleds out into function
    
    Changes in v4:
    * Remove mutex destruction and remove function
    * Include missing headers
    * Use post-increment instead of pre-increment
    * Variable declarations in reverse xmas tree order
    * Use dev_err_probe where possible
    * Return immediately where possible
    * Cosmetic changes
    * Document LKM name
    
    Changes in v3:
    * Release fwnode handles
    * Sort header includes
    * Remove deprecated device tree properties
    * Remove deprecated LED_OFF
    * Remove subled channel assignment
    * s/pwmstate/state/

 drivers/leds/Kconfig               |  11 ++
 drivers/leds/Makefile              |   1 +
 drivers/leds/leds-pwm-multicolor.c | 186 +++++++++++++++++++++++++++++
 3 files changed, 198 insertions(+)
 create mode 100644 drivers/leds/leds-pwm-multicolor.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6090e647daee..e70a46704076 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -552,6 +552,17 @@ config LEDS_PWM
 	help
 	  This option enables support for pwm driven LEDs
 
+config LEDS_PWM_MULTICOLOR
+	tristate "PWM driven multi-color LED Support"
+	depends on LEDS_CLASS_MULTICOLOR
+	depends on PWM
+	help
+	  This option enables support for PWM driven monochrome LEDs that are
+	  grouped into multicolor LEDs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called leds-pwm-multicolor.
+
 config LEDS_REGULATOR
 	tristate "REGULATOR driven LED support"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index e58ecb36360f..ba2c2c1edf12 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -73,6 +73,7 @@ obj-$(CONFIG_LEDS_PCA963X)		+= leds-pca963x.o
 obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
 obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
 obj-$(CONFIG_LEDS_PWM)			+= leds-pwm.o
+obj-$(CONFIG_LEDS_PWM_MULTICOLOR)	+= leds-pwm-multicolor.o
 obj-$(CONFIG_LEDS_REGULATOR)		+= leds-regulator.o
 obj-$(CONFIG_LEDS_S3C24XX)		+= leds-s3c24xx.o
 obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
diff --git a/drivers/leds/leds-pwm-multicolor.c b/drivers/leds/leds-pwm-multicolor.c
new file mode 100644
index 000000000000..45e38708ecb1
--- /dev/null
+++ b/drivers/leds/leds-pwm-multicolor.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * PWM-based multi-color LED control
+ *
+ * Copyright 2022 Sven Schwermer <sven.schwermer@disruptive-technologies.com>
+ */
+
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/leds.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/pwm.h>
+
+struct pwm_led {
+	struct pwm_device *pwm;
+	struct pwm_state state;
+};
+
+struct pwm_mc_led {
+	struct led_classdev_mc mc_cdev;
+	struct mutex lock;
+	struct pwm_led leds[];
+};
+
+static int led_pwm_mc_set(struct led_classdev *cdev,
+			  enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct pwm_mc_led *priv = container_of(mc_cdev, struct pwm_mc_led, mc_cdev);
+	unsigned long long duty;
+	int ret = 0;
+	int i;
+
+	led_mc_calc_color_components(mc_cdev, brightness);
+
+	mutex_lock(&priv->lock);
+
+	for (i = 0; i < mc_cdev->num_colors; i++) {
+		duty = priv->leds[i].state.period;
+		duty *= mc_cdev->subled_info[i].brightness;
+		do_div(duty, cdev->max_brightness);
+
+		priv->leds[i].state.duty_cycle = duty;
+		priv->leds[i].state.enabled = duty > 0;
+		ret = pwm_apply_state(priv->leds[i].pwm,
+				      &priv->leds[i].state);
+		if (ret)
+			break;
+	}
+
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
+static int iterate_subleds(struct device *dev, struct pwm_mc_led *priv,
+			   struct fwnode_handle *mcnode)
+{
+	struct mc_subled *subled = priv->mc_cdev.subled_info;
+	struct fwnode_handle *fwnode;
+	struct pwm_led *pwmled;
+	u32 color;
+	int ret;
+
+	/* iterate over the nodes inside the multi-led node */
+	fwnode_for_each_child_node(mcnode, fwnode) {
+		pwmled = &priv->leds[priv->mc_cdev.num_colors];
+		pwmled->pwm = devm_fwnode_pwm_get(dev, fwnode, NULL);
+		if (IS_ERR(pwmled->pwm)) {
+			ret = PTR_ERR(pwmled->pwm);
+			dev_err(dev, "unable to request PWM: %d\n", ret);
+			goto release_fwnode;
+		}
+		pwm_init_state(pwmled->pwm, &pwmled->state);
+
+		ret = fwnode_property_read_u32(fwnode, "color", &color);
+		if (ret) {
+			dev_err(dev, "cannot read color: %d\n", ret);
+			goto release_fwnode;
+		}
+
+		subled[priv->mc_cdev.num_colors].color_index = color;
+		priv->mc_cdev.num_colors++;
+	}
+
+	return 0;
+
+release_fwnode:
+	fwnode_handle_put(fwnode);
+	return ret;
+}
+
+static int led_pwm_mc_probe(struct platform_device *pdev)
+{
+	struct fwnode_handle *mcnode, *fwnode;
+	struct led_init_data init_data = {};
+	struct led_classdev *cdev;
+	struct mc_subled *subled;
+	struct pwm_mc_led *priv;
+	int count = 0;
+	int ret = 0;
+
+	mcnode = device_get_named_child_node(&pdev->dev, "multi-led");
+	if (!mcnode)
+		return dev_err_probe(&pdev->dev, -ENODEV,
+				     "expected multi-led node\n");
+
+	/* count the nodes inside the multi-led node */
+	fwnode_for_each_child_node(mcnode, fwnode)
+		count++;
+
+	priv = devm_kzalloc(&pdev->dev, struct_size(priv, leds, count),
+			    GFP_KERNEL);
+	if (!priv) {
+		ret = -ENOMEM;
+		goto release_mcnode;
+	}
+	mutex_init(&priv->lock);
+
+	subled = devm_kcalloc(&pdev->dev, count, sizeof(*subled), GFP_KERNEL);
+	if (!subled) {
+		ret = -ENOMEM;
+		goto release_mcnode;
+	}
+	priv->mc_cdev.subled_info = subled;
+
+	/* init the multicolor's LED class device */
+	cdev = &priv->mc_cdev.led_cdev;
+	fwnode_property_read_u32(mcnode, "max-brightness",
+				 &cdev->max_brightness);
+	cdev->flags = LED_CORE_SUSPENDRESUME;
+	cdev->brightness_set_blocking = led_pwm_mc_set;
+
+	ret = iterate_subleds(&pdev->dev, priv, mcnode);
+	if (ret)
+		goto release_mcnode;
+
+	init_data.fwnode = mcnode;
+	ret = devm_led_classdev_multicolor_register_ext(&pdev->dev,
+							&priv->mc_cdev,
+							&init_data);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to register multicolor PWM led for %s: %d\n",
+			cdev->name, ret);
+		goto release_mcnode;
+	}
+
+	ret = led_pwm_mc_set(cdev, cdev->brightness);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to set led PWM value for %s: %d",
+				     cdev->name, ret);
+
+	platform_set_drvdata(pdev, priv);
+	return 0;
+
+release_mcnode:
+	fwnode_handle_put(mcnode);
+	return ret;
+}
+
+static const struct of_device_id of_pwm_leds_mc_match[] = {
+	{ .compatible = "pwm-leds-multicolor", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_pwm_leds_mc_match);
+
+static struct platform_driver led_pwm_mc_driver = {
+	.probe		= led_pwm_mc_probe,
+	.driver		= {
+		.name	= "leds_pwm_multicolor",
+		.of_match_table = of_pwm_leds_mc_match,
+	},
+};
+module_platform_driver(led_pwm_mc_driver);
+
+MODULE_AUTHOR("Sven Schwermer <sven.schwermer@disruptive-technologies.com>");
+MODULE_DESCRIPTION("multi-color PWM LED driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:leds-pwm-multicolor");
-- 
2.35.1

Re: [PATCH v6 2/3] dt-bindings: leds: Add multicolor PWM LED bindings

[Alexander Dahl]

Hello Sven,

Am Tue, Feb 08, 2022 at 08:12:35PM +0100 schrieb sven@svenschwermer.de:
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> This allows to group multiple PWM-connected monochrome LEDs into
> multicolor LEDs, e.g. RGB LEDs.
> 
> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> ---
> 
> Notes:
>     Changes in v6:
>     * Fix device tree binding schema
>     
>     Changes in v5:
>     * (no changes)
>     
>     Changes in v4:
>     * (no changes)
>     
>     Changes in v3:
>     * Remove multi-led unit name
> 
>  .../bindings/leds/leds-pwm-multicolor.yaml    | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml b/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
> new file mode 100644
> index 000000000000..f7ce29c8ae63
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-pwm-multicolor.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Multi-color LEDs connected to PWM
> +
> +maintainers:
> +  - Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> +
> +description: |
> +  This driver combines several monochrome PWM LEDs into one multi-color
> +  LED using the multicolor LED class.
> +
> +properties:
> +  compatible:
> +    const: pwm-leds-multicolor
> +  multi-led:
> +    type: object
> +    patternProperties:
> +      "^led-[0-9a-z]+$":
> +        type: object
> +        $ref: common.yaml#
> +        properties:
> +          pwms:
> +            maxItems: 1
> +          pwm-names: true
> +          color: true
> +        required:
> +          - pwms
> +          - color
> +        additionalProperties: false
> +required:
> +  - compatible
> +allOf:
> +  - $ref: leds-class-multicolor.yaml#
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    rgb-led {

I think this should be 'led-controller'. See
Documentation/devicetree/bindings/leds/common.yaml for reference.

> +        compatible = "pwm-leds-multicolor";
> +
> +        multi-led {
> +          color = <LED_COLOR_ID_RGB>;
> +          function = LED_FUNCTION_INDICATOR;
> +          max-brightness = <65535>;
> +
> +          led-red {
> +              pwms = <&pwm1 0 1000000>;
> +              color = <LED_COLOR_ID_RED>;
> +          };
> +
> +          led-green {
> +              pwms = <&pwm2 0 1000000>;
> +              color = <LED_COLOR_ID_GREEN>;
> +          };
> +
> +          led-blue {
> +              pwms = <&pwm3 0 1000000>;
> +              color = <LED_COLOR_ID_BLUE>;
> +          };

Not sure if those node names should be more generic like led-0, led-1
etc.?  At least the color information is redundant here.  This would
make it more similar to bindings of other LED drivers.

And how is it supposed to be named if you have multiple
"multi-led"s, e.g. one on three PWM channels, and another one on three
different PWM channels?

Greets
Alex

> +        };
> +    };
> +
> +...
> -- 
> 2.35.1
> 

Re: [PATCH v6 1/3] dt-bindings: leds: Optional multi-led unit address

[Rob Herring]

On Tue, 08 Feb 2022 20:12:34 +0100, sven@svenschwermer.de wrote:
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> The unit address does not make sense in all cases the multi-led node is
> used, e.g. for the upcoming PWM multi-color LED driver.
> 
> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> ---
> 
> Notes:
>     Added in v6
> 
>  .../devicetree/bindings/leds/leds-class-multicolor.yaml         | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v6 2/3] dt-bindings: leds: Add multicolor PWM LED bindings

[Rob Herring]

On Tue, Feb 08, 2022 at 08:12:35PM +0100, sven@svenschwermer.de wrote:
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> This allows to group multiple PWM-connected monochrome LEDs into
> multicolor LEDs, e.g. RGB LEDs.
> 
> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> ---
> 
> Notes:
>     Changes in v6:
>     * Fix device tree binding schema
>     
>     Changes in v5:
>     * (no changes)
>     
>     Changes in v4:
>     * (no changes)
>     
>     Changes in v3:
>     * Remove multi-led unit name
> 
>  .../bindings/leds/leds-pwm-multicolor.yaml    | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml b/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
> new file mode 100644
> index 000000000000..f7ce29c8ae63
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-pwm-multicolor.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Multi-color LEDs connected to PWM
> +
> +maintainers:
> +  - Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> +
> +description: |
> +  This driver combines several monochrome PWM LEDs into one multi-color
> +  LED using the multicolor LED class.
> +
> +properties:
> +  compatible:
> +    const: pwm-leds-multicolor

blank line

> +  multi-led:
> +    type: object

blank line

> +    patternProperties:
> +      "^led-[0-9a-z]+$":
> +        type: object
> +        $ref: common.yaml#

blank line

> +        properties:
> +          pwms:
> +            maxItems: 1

blank line and so on. (There not a hard rule, but roughly it's 
blank lines between DT property sub-schemas, top-level keywords, and 
properties/patternProperties/required.)

> +          pwm-names: true
> +          color: true
> +        required:
> +          - pwms
> +          - color

> +        additionalProperties: false

While the top level goes at the end, I find it is easier to read putting 
the indented cases before properties/patternProperties. 

> +required:
> +  - compatible
> +allOf:
> +  - $ref: leds-class-multicolor.yaml#
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    rgb-led {
> +        compatible = "pwm-leds-multicolor";
> +
> +        multi-led {
> +          color = <LED_COLOR_ID_RGB>;
> +          function = LED_FUNCTION_INDICATOR;
> +          max-brightness = <65535>;
> +
> +          led-red {
> +              pwms = <&pwm1 0 1000000>;
> +              color = <LED_COLOR_ID_RED>;
> +          };
> +
> +          led-green {
> +              pwms = <&pwm2 0 1000000>;
> +              color = <LED_COLOR_ID_GREEN>;
> +          };
> +
> +          led-blue {
> +              pwms = <&pwm3 0 1000000>;
> +              color = <LED_COLOR_ID_BLUE>;
> +          };
> +        };
> +    };
> +
> +...
> -- 
> 2.35.1
> 
> 

Re: [PATCH v6 2/3] dt-bindings: leds: Add multicolor PWM LED bindings

[Sven Schwermer]

Hi Alex,

On 2/9/22 10:17, Alexander Dahl wrote:
>> +    rgb-led {
> 
> I think this should be 'led-controller'. See
> Documentation/devicetree/bindings/leds/common.yaml for reference.

Sure, I don't have a preference.

>> +        multi-led {
>> +          color = <LED_COLOR_ID_RGB>;
>> +          function = LED_FUNCTION_INDICATOR;
>> +          max-brightness = <65535>;
>> +
>> +          led-red {
>> +              pwms = <&pwm1 0 1000000>;
>> +              color = <LED_COLOR_ID_RED>;
>> +          };
>> +
>> +          led-green {
>> +              pwms = <&pwm2 0 1000000>;
>> +              color = <LED_COLOR_ID_GREEN>;
>> +          };
>> +
>> +          led-blue {
>> +              pwms = <&pwm3 0 1000000>;
>> +              color = <LED_COLOR_ID_BLUE>;
>> +          };
> 
> Not sure if those node names should be more generic like led-0, led-1
> etc.?  At least the color information is redundant here.  This would
> make it more similar to bindings of other LED drivers.

I don't see how naming them led-{0,1,2} would be better in any way, 
please elaborate.

> And how is it supposed to be named if you have multiple
> "multi-led"s, e.g. one on three PWM channels, and another one on three
> different PWM channels?

I'm not 100% sure what you mean. If you want multiple instances of these 
multi-color PWM LEDs, you'd do something like this:

indicator-led-controller {
     compatible = "pwm-leds-multicolor";
     multi-led {
       color = <LED_COLOR_ID_RGB>;
       function = LED_FUNCTION_INDICATOR;
       max-brightness = <65535>;
       led-red {
           pwms = <&pwm1 0 1000000>;
           color = <LED_COLOR_ID_RED>;
       };
       led-green {
           pwms = <&pwm2 0 1000000>;
           color = <LED_COLOR_ID_GREEN>;
       };
       led-blue {
           pwms = <&pwm3 0 1000000>;
           color = <LED_COLOR_ID_BLUE>;
       };
     };
};
status-led-controller {
     compatible = "pwm-leds-multicolor";
     multi-led {
       color = <LED_COLOR_ID_MULTI>;
       function = LED_FUNCTION_STATUS;
       max-brightness = <255>;
       led-red {
           pwms = <&pwm4 0 1000000>;
           color = <LED_COLOR_ID_RED>;
       };
       led-amber {
           pwms = <&pwm5 0 1000000>;
           color = <LED_COLOR_ID_AMBER>;
       };
     };
};

Re: [PATCH v6 2/3] dt-bindings: leds: Add multicolor PWM LED bindings

[Alexander Dahl]

Hei hei,

Am Thu, Feb 10, 2022 at 08:55:07AM +0100 schrieb Sven Schwermer:
> Hi Alex,
> 
> On 2/9/22 10:17, Alexander Dahl wrote:
> > > +    rgb-led {
> > 
> > I think this should be 'led-controller'. See
> > Documentation/devicetree/bindings/leds/common.yaml for reference.
> 
> Sure, I don't have a preference.
> 
> > > +        multi-led {
> > > +          color = <LED_COLOR_ID_RGB>;
> > > +          function = LED_FUNCTION_INDICATOR;
> > > +          max-brightness = <65535>;
> > > +
> > > +          led-red {
> > > +              pwms = <&pwm1 0 1000000>;
> > > +              color = <LED_COLOR_ID_RED>;
> > > +          };
> > > +
> > > +          led-green {
> > > +              pwms = <&pwm2 0 1000000>;
> > > +              color = <LED_COLOR_ID_GREEN>;
> > > +          };
> > > +
> > > +          led-blue {
> > > +              pwms = <&pwm3 0 1000000>;
> > > +              color = <LED_COLOR_ID_BLUE>;
> > > +          };
> > 
> > Not sure if those node names should be more generic like led-0, led-1
> > etc.?  At least the color information is redundant here.  This would
> > make it more similar to bindings of other LED drivers.
> 
> I don't see how naming them led-{0,1,2} would be better in any way, please
> elaborate.

- consistency with other LED driver bindings
- spot the number of "sub"-LEDs more easily
- prevent all kinds of different names people will come up with, if
  all is allowed instead of a clear scheme

The color is in the color property anyways.

> > And how is it supposed to be named if you have multiple
> > "multi-led"s, e.g. one on three PWM channels, and another one on three
> > different PWM channels?
> 
> I'm not 100% sure what you mean. If you want multiple instances of these
> multi-color PWM LEDs, you'd do something like this:
> 
> indicator-led-controller {
>     compatible = "pwm-leds-multicolor";
>     multi-led {
>       color = <LED_COLOR_ID_RGB>;
>       function = LED_FUNCTION_INDICATOR;
>       max-brightness = <65535>;
>       led-red {
>           pwms = <&pwm1 0 1000000>;
>           color = <LED_COLOR_ID_RED>;
>       };
>       led-green {
>           pwms = <&pwm2 0 1000000>;
>           color = <LED_COLOR_ID_GREEN>;
>       };
>       led-blue {
>           pwms = <&pwm3 0 1000000>;
>           color = <LED_COLOR_ID_BLUE>;
>       };
>     };
> };
> status-led-controller {
>     compatible = "pwm-leds-multicolor";
>     multi-led {
>       color = <LED_COLOR_ID_MULTI>;
>       function = LED_FUNCTION_STATUS;
>       max-brightness = <255>;
>       led-red {
>           pwms = <&pwm4 0 1000000>;
>           color = <LED_COLOR_ID_RED>;
>       };
>       led-amber {
>           pwms = <&pwm5 0 1000000>;
>           color = <LED_COLOR_ID_AMBER>;
>       };
>     };
> };

I would have expected something like this:

  led-controller-0 {
      compatible = "pwm-leds-multicolor";

      multi-led-0 {
        color = <LED_COLOR_ID_RGB>;
        function = LED_FUNCTION_INDICATOR;
        max-brightness = <65535>;

        led-0 {
            pwms = <&pwm1 0 1000000>;
            color = <LED_COLOR_ID_RED>;
        };

        led-1 {
            pwms = <&pwm2 0 1000000>;
            color = <LED_COLOR_ID_GREEN>;
        };

        led-2 {
            pwms = <&pwm3 0 1000000>;
            color = <LED_COLOR_ID_BLUE>;
        };
      };

      multi-led-1 {
        color = <LED_COLOR_ID_RGB>;
        function = LED_FUNCTION_INDICATOR;
        max-brightness = <65535>;

        led-0 {
            pwms = <&pwm1 0 1000000>;
            color = <LED_COLOR_ID_RED>;
        };

        led-1 {
            pwms = <&pwm2 0 1000000>;
            color = <LED_COLOR_ID_GREEN>;
        };

        led-2 {
            pwms = <&pwm3 0 1000000>;
            color = <LED_COLOR_ID_BLUE>;
        };
      };
  };

Greets
Alex

Re: [PATCH v6 2/3] dt-bindings: leds: Add multicolor PWM LED bindings

[Sven Schwermer]

Hi Alex,

On 2/10/22 10:33, Alexander Dahl wrote:
>> I don't see how naming them led-{0,1,2} would be better in any way, please
>> elaborate.
> 
> - consistency with other LED driver bindings
> - spot the number of "sub"-LEDs more easily
> - prevent all kinds of different names people will come up with, if
>    all is allowed instead of a clear scheme
> 
> The color is in the color property anyways.

Let's hear what others on this list say. I don't feel too strongly about 
this either way.

> I would have expected something like this:
> 
>    led-controller-0 {
>        compatible = "pwm-leds-multicolor";
> 
>        multi-led-0 {
>          color = <LED_COLOR_ID_RGB>;
>          function = LED_FUNCTION_INDICATOR;
>          max-brightness = <65535>;
> 
>          led-0 {
>              pwms = <&pwm1 0 1000000>;
>              color = <LED_COLOR_ID_RED>;
>          };
> 
>          led-1 {
>              pwms = <&pwm2 0 1000000>;
>              color = <LED_COLOR_ID_GREEN>;
>          };
> 
>          led-2 {
>              pwms = <&pwm3 0 1000000>;
>              color = <LED_COLOR_ID_BLUE>;
>          };
>        };
> 
>        multi-led-1 {
>          color = <LED_COLOR_ID_RGB>;
>          function = LED_FUNCTION_INDICATOR;
>          max-brightness = <65535>;
> 
>          led-0 {
>              pwms = <&pwm1 0 1000000>;
>              color = <LED_COLOR_ID_RED>;
>          };
> 
>          led-1 {
>              pwms = <&pwm2 0 1000000>;
>              color = <LED_COLOR_ID_GREEN>;
>          };
> 
>          led-2 {
>              pwms = <&pwm3 0 1000000>;
>              color = <LED_COLOR_ID_BLUE>;
>          };
>        };
>    };

That would make the driver more complex. I'm not sure that would be 
worth it. Additionally, we'd have to change the node name pattern for 
the multi LED class. Other opinions?

Best regards,
Sven