[RFC PATCH 0/2] Multicolor PWM LED support

[RFC PATCH 0/2] Multicolor PWM LED support

[sven]

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

Hi,

As previously discussed [1] on the linux-leds list I am missing
multicolor PWM LED support. In the mean time I have put together a
working prototype for such a driver. This is my first Linux driver
so I'm hoping for some feedback. Here are some questions that came up
while putting this thing together:

  1. Currently, the max-brightness property is expected as a property to
     the multi-led node. That seems consistent with the existing
     multicolor class code, but I'm wondering whether it would make
     sense to have a max-brigthness for the individual LEDs as well?
  2. The current multi-led node definition calls for a node index which
     would in turn require the reg property to be set within the node.
     In this context, that doesn't seem to make sense. Should this
     requirement be lifted from leds-class-multicolor.yaml?
  3. I'm not currently reusing any leds-pwm code because there aren't
     too many overlaps. Does anyone have suggestions what could be
     factored out into a common source file?

I would appreciate if anyone would test this code. It runs on my
i.MX6ULL-based hardware.

Best regards,
Sven

[1]: https://www.spinics.net/lists/linux-leds/msg19988.html

Sven Schwermer (2):
  dt-bindings: leds: Add multicolor PWM LED bindings
  leds: Add PWM multicolor driver

 .../bindings/leds/leds-pwm-multicolor.yaml    |  73 +++++++
 drivers/leds/Kconfig                          |   8 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-pwm-multicolor.c            | 184 ++++++++++++++++++
 4 files changed, 266 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
 create mode 100644 drivers/leds/leds-pwm-multicolor.c

-- 
2.35.0

[RFC PATCH 1/2] 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>
---
 .../bindings/leds/leds-pwm-multicolor.yaml    | 73 +++++++++++++++++++
 1 file changed, 73 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..8552a5498bdd
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
@@ -0,0 +1,73 @@
+# 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: pwm-leds-multicolor
+
+patternProperties:
+  '^multi-led@[0-9a-f]$':
+    type: object
+    allOf:
+      - $ref: leds-class-multicolor.yaml#
+
+    patternProperties:
+      "^led-[0-9a-z]+$":
+        type: object
+        properties:
+          pwms:
+            maxItems: 1
+
+          pwm-names: true
+
+          color:
+            $ref: common.yaml#/properties/color
+
+required:
+  - compatible
+  - pwms
+  - color
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    rgb-led {
+        compatible = "pwm-leds-multicolor";
+
+        multi-led@0 {
+          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.0

[RFC PATCH 2/2] 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>
---
 drivers/leds/Kconfig               |   8 ++
 drivers/leds/Makefile              |   1 +
 drivers/leds/leds-pwm-multicolor.c | 184 +++++++++++++++++++++++++++++
 3 files changed, 193 insertions(+)
 create mode 100644 drivers/leds/leds-pwm-multicolor.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b6742b4231bf..2e5058696fa6 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -586,6 +586,14 @@ 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.
+
 config LEDS_REGULATOR
 	tristate "REGULATOR driven LED support"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 2a698df9da57..152f69374632 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -79,6 +79,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..c54bed4536d3
--- /dev/null
+++ b/drivers/leds/leds-pwm-multicolor.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * PWM-based multi-color LED control
+ *
+ * Copyright 2022 Sven Schwermer <sven.schwermer@disruptive-technologies.com>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/leds.h>
+#include <linux/err.h>
+#include <linux/pwm.h>
+#include <linux/mutex.h>
+
+struct pwm_led {
+	struct pwm_device *pwm;
+	struct pwm_state pwmstate;
+};
+
+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)
+{
+	int i;
+	unsigned long long duty;
+	int ret = 0;
+	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);
+
+	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].pwmstate.period;
+		duty *= mc_cdev->subled_info[i].brightness;
+		do_div(duty, cdev->max_brightness);
+
+		priv->leds[i].pwmstate.duty_cycle = duty;
+		priv->leds[i].pwmstate.enabled = duty > 0;
+		ret = pwm_apply_state(priv->leds[i].pwm,
+				      &priv->leds[i].pwmstate);
+		if (ret)
+			break;
+	}
+
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
+static int led_pwm_mc_probe(struct platform_device *pdev)
+{
+	struct fwnode_handle *mcnode, *fwnode;
+	int count = 0;
+	struct pwm_mc_led *priv;
+	struct mc_subled *subled;
+	struct led_classdev *cdev;
+	struct pwm_led *pwmled;
+	u32 color;
+	int ret = 0;
+	struct led_init_data init_data = {};
+
+	mcnode = device_get_named_child_node(&pdev->dev, "multi-led");
+	if (!mcnode) {
+		dev_err(&pdev->dev, "expected multi-led node\n");
+		ret = -ENODEV;
+		goto out;
+	}
+
+	/* 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 out;
+	}
+	mutex_init(&priv->lock);
+
+	subled = devm_kcalloc(&pdev->dev, count, sizeof(*subled), GFP_KERNEL);
+	if (!subled) {
+		ret = -ENOMEM;
+		goto destroy_mutex;
+	}
+	priv->mc_cdev.subled_info = subled;
+
+	/* init the multicolor's LED class device */
+	cdev = &priv->mc_cdev.led_cdev;
+	fwnode_property_read_string(mcnode, "label", &cdev->name);
+	cdev->brightness = LED_OFF;
+	fwnode_property_read_u32(mcnode, "max-brightness",
+				 &cdev->max_brightness);
+	cdev->flags = LED_CORE_SUSPENDRESUME;
+	cdev->brightness_set_blocking = led_pwm_mc_set;
+
+	/* 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(&pdev->dev, fwnode, NULL);
+		if (IS_ERR(pwmled->pwm)) {
+			ret = PTR_ERR(pwmled->pwm);
+			dev_err(&pdev->dev, "unable to request PWM: %d\n", ret);
+			goto destroy_mutex;
+		}
+		pwm_init_state(pwmled->pwm, &pwmled->pwmstate);
+
+		ret = fwnode_property_read_u32(fwnode, "color", &color);
+		if (ret) {
+			dev_err(&pdev->dev, "cannot read color: %d\n", ret);
+			goto destroy_mutex;
+		}
+
+		subled[priv->mc_cdev.num_colors].color_index = color;
+		subled[priv->mc_cdev.num_colors].channel =
+			priv->mc_cdev.num_colors;
+		++priv->mc_cdev.num_colors;
+	}
+
+	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 destroy_mutex;
+	}
+
+	ret = led_pwm_mc_set(cdev, cdev->brightness);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to set led PWM value for %s: %d",
+			cdev->name, ret);
+		goto destroy_mutex;
+	}
+
+	platform_set_drvdata(pdev, priv);
+	return 0;
+
+destroy_mutex:
+	mutex_destroy(&priv->lock);
+out:
+	return ret;
+}
+
+static int led_pwm_mc_remove(struct platform_device *pdev)
+{
+	struct pwm_mc_led *priv = platform_get_drvdata(pdev);
+
+	mutex_destroy(&priv->lock);
+	return 0;
+}
+
+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,
+	.remove		= led_pwm_mc_remove,
+	.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.0

Re: [RFC PATCH 1/2] dt-bindings: leds: Add multicolor PWM LED bindings

[Rob Herring]

On Tue, 25 Jan 2022 10:22:38 +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>
> ---
>  .../bindings/leds/leds-pwm-multicolor.yaml    | 73 +++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml: properties:compatible: 'pwm-leds-multicolor' is not of type 'object', 'boolean'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml: ignoring, error in schema: properties: compatible
Documentation/devicetree/bindings/leds/leds-pwm-multicolor.example.dts:24.25-43.15: Warning (unit_address_vs_reg): /example-0/rgb-led/multi-led@0: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/leds/leds-pwm-multicolor.example.dt.yaml:0:0: /example-0/rgb-led: failed to match any schema with compatible: ['pwm-leds-multicolor']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1583948

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [RFC PATCH 0/2] Multicolor PWM LED support

[Jacek Anaszewski]

Hi Sven,

On 1/25/22 10:22 AM, sven@svenschwermer.de wrote:
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> Hi,
> 
> As previously discussed [1] on the linux-leds list I am missing
> multicolor PWM LED support. In the mean time I have put together a
> working prototype for such a driver. This is my first Linux driver
> so I'm hoping for some feedback. Here are some questions that came up
> while putting this thing together:
> 
>    1. Currently, the max-brightness property is expected as a property to
>       the multi-led node. That seems consistent with the existing
>       multicolor class code, but I'm wondering whether it would make
>       sense to have a max-brigthness for the individual LEDs as well?

For the proper mixed color calculation all sub-leds should have
the same max_brightness as the global max_brightness.

Look at how sub-led intensities are calculated in
led_mc_calc_color_components().

See also [0] and [1].

>    2. The current multi-led node definition calls for a node index which
>       would in turn require the reg property to be set within the node.
>       In this context, that doesn't seem to make sense. Should this
>       requirement be lifted from leds-class-multicolor.yaml?

reg is required for all DT nodes with address unit in the name.
If you skipped the address unit, then reg would be also not required.

>    3. I'm not currently reusing any leds-pwm code because there aren't
>       too many overlaps. Does anyone have suggestions what could be
>       factored out into a common source file?

I think that having a separate pwm driver for multicolor LEDs is a good
idea. leds-pwm.c is old and well tested driver, there's no need to
tinker at it for no vital reason. And there is not much code to share
as you've noticed.

> I would appreciate if anyone would test this code. It runs on my
> i.MX6ULL-based hardware.
> 
> Best regards,
> Sven
> 
> [1]: https://www.spinics.net/lists/linux-leds/msg19988.html
> 
> Sven Schwermer (2):
>    dt-bindings: leds: Add multicolor PWM LED bindings
>    leds: Add PWM multicolor driver
> 
>   .../bindings/leds/leds-pwm-multicolor.yaml    |  73 +++++++
>   drivers/leds/Kconfig                          |   8 +
>   drivers/leds/Makefile                         |   1 +
>   drivers/leds/leds-pwm-multicolor.c            | 184 ++++++++++++++++++
>   4 files changed, 266 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
>   create mode 100644 drivers/leds/leds-pwm-multicolor.c
> 

[0] Documentation/ABI/testing/sysfs-class-led-multicolor
[1] Documentation/leds/leds-class-multicolor.rst

-- 
Best regards,
Jacek Anaszewski

Re: [RFC PATCH 2/2] leds: Add PWM multicolor driver

[Jacek Anaszewski]

Hi Sven,

Thanks for the patch. Few comments below.

On 1/25/22 10:22 AM, sven@svenschwermer.de wrote:
> 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>
> ---
>   drivers/leds/Kconfig               |   8 ++
>   drivers/leds/Makefile              |   1 +
>   drivers/leds/leds-pwm-multicolor.c | 184 +++++++++++++++++++++++++++++
>   3 files changed, 193 insertions(+)
>   create mode 100644 drivers/leds/leds-pwm-multicolor.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index b6742b4231bf..2e5058696fa6 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -586,6 +586,14 @@ 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.
> +
>   config LEDS_REGULATOR
>   	tristate "REGULATOR driven LED support"
>   	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 2a698df9da57..152f69374632 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -79,6 +79,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..c54bed4536d3
> --- /dev/null
> +++ b/drivers/leds/leds-pwm-multicolor.c
> @@ -0,0 +1,184 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * PWM-based multi-color LED control
> + *
> + * Copyright 2022 Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
> +#include <linux/err.h>
> +#include <linux/pwm.h>
> +#include <linux/mutex.h>
> +
> +struct pwm_led {
> +	struct pwm_device *pwm;
> +	struct pwm_state pwmstate;
> +};
> +
> +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)
> +{
> +	int i;
> +	unsigned long long duty;
> +	int ret = 0;
> +	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);
> +
> +	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].pwmstate.period;
> +		duty *= mc_cdev->subled_info[i].brightness;
> +		do_div(duty, cdev->max_brightness);
> +
> +		priv->leds[i].pwmstate.duty_cycle = duty;
> +		priv->leds[i].pwmstate.enabled = duty > 0;
> +		ret = pwm_apply_state(priv->leds[i].pwm,
> +				      &priv->leds[i].pwmstate);
> +		if (ret)
> +			break;
> +	}
> +
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static int led_pwm_mc_probe(struct platform_device *pdev)
> +{
> +	struct fwnode_handle *mcnode, *fwnode;
> +	int count = 0;
> +	struct pwm_mc_led *priv;
> +	struct mc_subled *subled;
> +	struct led_classdev *cdev;
> +	struct pwm_led *pwmled;
> +	u32 color;
> +	int ret = 0;
> +	struct led_init_data init_data = {};
> +
> +	mcnode = device_get_named_child_node(&pdev->dev, "multi-led");
> +	if (!mcnode) {
> +		dev_err(&pdev->dev, "expected multi-led node\n");
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	/* 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 out;
> +	}
> +	mutex_init(&priv->lock);
> +
> +	subled = devm_kcalloc(&pdev->dev, count, sizeof(*subled), GFP_KERNEL);
> +	if (!subled) {
> +		ret = -ENOMEM;
> +		goto destroy_mutex;
> +	}
> +	priv->mc_cdev.subled_info = subled;
> +
> +	/* init the multicolor's LED class device */
> +	cdev = &priv->mc_cdev.led_cdev;
> +	fwnode_property_read_string(mcnode, "label", &cdev->name);

LED core has the support for parsing LED name, so you can skip it.

> +	cdev->brightness = LED_OFF;

devm_kzalloc() handles that.

> +	fwnode_property_read_u32(mcnode, "max-brightness",
> +				 &cdev->max_brightness);
> +	cdev->flags = LED_CORE_SUSPENDRESUME;
> +	cdev->brightness_set_blocking = led_pwm_mc_set;
> +
> +	/* 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(&pdev->dev, fwnode, NULL);
> +		if (IS_ERR(pwmled->pwm)) {
> +			ret = PTR_ERR(pwmled->pwm);
> +			dev_err(&pdev->dev, "unable to request PWM: %d\n", ret);
> +			goto destroy_mutex;
> +		}
> +		pwm_init_state(pwmled->pwm, &pwmled->pwmstate);
> +
> +		ret = fwnode_property_read_u32(fwnode, "color", &color);
> +		if (ret) {
> +			dev_err(&pdev->dev, "cannot read color: %d\n", ret);
> +			goto destroy_mutex;
> +		}
> +
> +		subled[priv->mc_cdev.num_colors].color_index = color;
> +		subled[priv->mc_cdev.num_colors].channel =
> +			priv->mc_cdev.num_colors;

channel property can be meaningful only for the LED controllers that can
control many LED outputs. In case of pwm controllers you're using LED id
to pwmstate mapping in led_pwm_mc_set().

> +		++priv->mc_cdev.num_colors;
> +	}
> +
> +	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 destroy_mutex;
> +	}
> +
> +	ret = led_pwm_mc_set(cdev, cdev->brightness);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to set led PWM value for %s: %d",
> +			cdev->name, ret);
> +		goto destroy_mutex;
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +	return 0;
> +
> +destroy_mutex:
> +	mutex_destroy(&priv->lock);
> +out:
> +	return ret;
> +}
> +
> +static int led_pwm_mc_remove(struct platform_device *pdev)
> +{
> +	struct pwm_mc_led *priv = platform_get_drvdata(pdev);
> +
> +	mutex_destroy(&priv->lock);
> +	return 0;
> +}
> +
> +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,
> +	.remove		= led_pwm_mc_remove,
> +	.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");
> 

-- 
Best regards,
Jacek Anaszewski

Re: [RFC PATCH 0/2] Multicolor PWM LED support

[Sven Schwermer]

Hi Jacek,

Thank you for your feedback!

On 1/25/22 23:31, Jacek Anaszewski wrote:

>>    1. Currently, the max-brightness property is expected as a property to
>>       the multi-led node. That seems consistent with the existing
>>       multicolor class code, but I'm wondering whether it would make
>>       sense to have a max-brigthness for the individual LEDs as well?
> 
> For the proper mixed color calculation all sub-leds should have
> the same max_brightness as the global max_brightness.
> 
> Look at how sub-led intensities are calculated in
> led_mc_calc_color_components().
> 
> See also [0] and [1].

OK, thanks. That makes sense.

>>    2. The current multi-led node definition calls for a node index which
>>       would in turn require the reg property to be set within the node.
>>       In this context, that doesn't seem to make sense. Should this
>>       requirement be lifted from leds-class-multicolor.yaml?
> 
> reg is required for all DT nodes with address unit in the name.
> If you skipped the address unit, then reg would be also not required.

Yes, I realize this. However, leds-class-multicolor.yaml [0] requires 
the address unit: "^multi-led@([0-9a-f])$"

Best regards,
Sven

[0] Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml

Re: [RFC PATCH 0/2] Multicolor PWM LED support

[Alexander Dahl]

Hello Sven,

Am Tue, Jan 25, 2022 at 10:22:37AM +0100 schrieb sven@svenschwermer.de:
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> As previously discussed [1] on the linux-leds list I am missing
> multicolor PWM LED support. In the mean time I have put together a
> working prototype for such a driver. This is my first Linux driver
> so I'm hoping for some feedback. Here are some questions that came up
> while putting this thing together:

Wow, I did not expect this so quickly.  Thank you. :-)

>   1. Currently, the max-brightness property is expected as a property to
>      the multi-led node. That seems consistent with the existing
>      multicolor class code, but I'm wondering whether it would make
>      sense to have a max-brigthness for the individual LEDs as well?
>   2. The current multi-led node definition calls for a node index which
>      would in turn require the reg property to be set within the node.
>      In this context, that doesn't seem to make sense. Should this
>      requirement be lifted from leds-class-multicolor.yaml?
>   3. I'm not currently reusing any leds-pwm code because there aren't
>      too many overlaps. Does anyone have suggestions what could be
>      factored out into a common source file?
> 
> I would appreciate if anyone would test this code. It runs on my
> i.MX6ULL-based hardware.
> 
> Best regards,
> Sven
> 
> [1]: https://www.spinics.net/lists/linux-leds/msg19988.html

You can use the lore.kernel.org archive, e.g. in this case:

https://lore.kernel.org/linux-leds/b48eed49-a18e-eed1-f1f4-77b9f1eab39b@gmail.com/T/#t

Greets
Alex

> 
> Sven Schwermer (2):
>   dt-bindings: leds: Add multicolor PWM LED bindings
>   leds: Add PWM multicolor driver
> 
>  .../bindings/leds/leds-pwm-multicolor.yaml    |  73 +++++++
>  drivers/leds/Kconfig                          |   8 +
>  drivers/leds/Makefile                         |   1 +
>  drivers/leds/leds-pwm-multicolor.c            | 184 ++++++++++++++++++
>  4 files changed, 266 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
>  create mode 100644 drivers/leds/leds-pwm-multicolor.c
> 
> -- 
> 2.35.0
> 

Re: [RFC PATCH 0/2] Multicolor PWM LED support

[Jacek Anaszewski]

Hi Sven,

On 1/26/22 8:51 AM, Sven Schwermer wrote:
> Hi Jacek,
> 
> Thank you for your feedback!
> 
> On 1/25/22 23:31, Jacek Anaszewski wrote:
> 
>>>    1. Currently, the max-brightness property is expected as a 
>>> property to
>>>       the multi-led node. That seems consistent with the existing
>>>       multicolor class code, but I'm wondering whether it would make
>>>       sense to have a max-brigthness for the individual LEDs as well?
>>
>> For the proper mixed color calculation all sub-leds should have
>> the same max_brightness as the global max_brightness.
>>
>> Look at how sub-led intensities are calculated in
>> led_mc_calc_color_components().
>>
>> See also [0] and [1].
> 
> OK, thanks. That makes sense.
> 
>>>    2. The current multi-led node definition calls for a node index which
>>>       would in turn require the reg property to be set within the node.
>>>       In this context, that doesn't seem to make sense. Should this
>>>       requirement be lifted from leds-class-multicolor.yaml?
>>
>> reg is required for all DT nodes with address unit in the name.
>> If you skipped the address unit, then reg would be also not required.
> 
> Yes, I realize this. However, leds-class-multicolor.yaml [0] requires 
> the address unit: "^multi-led@([0-9a-f])$"

This is only an example and nothing prevents you from dropping address
unit in leds-pwm-multicolor DT bindings. We don't have common DT parser
for multicolor LEDs and it will be hard to come up with something that
will fit neatly for all possible LED controllers anyway.

Dropping address unit from leds-class-multicolor.yaml would be too much
since it is useful in some cases, see e.g. [2].

[2] Documentation/devicetree/bindings/leds/leds-lp50xx.yaml

-- 
Best regards,
Jacek Anaszewski