[PATCH v3 0/2] Multicolor PWM LED support

[PATCH v3 0/2] Multicolor PWM LED support

[sven]

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

Hi,

This patch series is getting mature. I have removed the RFC tag for this
version. The initial discussion happened here [1].

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

Best regards,
Sven

[1]:https://lore.kernel.org/linux-leds/37540afd-f2f1-52dd-f4f1-6e7b436e9595@svenschwermer.de/

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

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

Interdiff against v2:
diff --git a/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml b/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
index b82b26f2e140..5a7ed5e1bb9f 100644
--- a/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
@@ -17,8 +17,7 @@ properties:
   compatible:
     const: pwm-leds-multicolor
 
-patternProperties:
-  '^multi-led@[0-9a-f]$':
+  multi-led:
     type: object
     allOf:
       - $ref: leds-class-multicolor.yaml#
@@ -51,7 +50,7 @@ examples:
     rgb-led {
         compatible = "pwm-leds-multicolor";
 
-        multi-led@0 {
+        multi-led {
           color = <LED_COLOR_ID_RGB>;
           function = LED_FUNCTION_INDICATOR;
           max-brightness = <65535>;
diff --git a/drivers/leds/leds-pwm-multicolor.c b/drivers/leds/leds-pwm-multicolor.c
index c54bed4536d3..bc4d21ddd74a 100644
--- a/drivers/leds/leds-pwm-multicolor.c
+++ b/drivers/leds/leds-pwm-multicolor.c
@@ -5,18 +5,18 @@
  * Copyright 2022 Sven Schwermer <sven.schwermer@disruptive-technologies.com>
  */
 
-#include <linux/module.h>
+#include <linux/err.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/module.h>
 #include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
 
 struct pwm_led {
 	struct pwm_device *pwm;
-	struct pwm_state pwmstate;
+	struct pwm_state state;
 };
 
 struct pwm_mc_led {
@@ -39,14 +39,14 @@ static int led_pwm_mc_set(struct led_classdev *cdev,
 	mutex_lock(&priv->lock);
 
 	for (i = 0; i < mc_cdev->num_colors; ++i) {
-		duty = priv->leds[i].pwmstate.period;
+		duty = priv->leds[i].state.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;
+		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].pwmstate);
+				      &priv->leds[i].state);
 		if (ret)
 			break;
 	}
@@ -83,7 +83,7 @@ static int led_pwm_mc_probe(struct platform_device *pdev)
 			    GFP_KERNEL);
 	if (!priv) {
 		ret = -ENOMEM;
-		goto out;
+		goto release_mcnode;
 	}
 	mutex_init(&priv->lock);
 
@@ -96,8 +96,6 @@ static int led_pwm_mc_probe(struct platform_device *pdev)
 
 	/* 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;
@@ -110,19 +108,19 @@ static int led_pwm_mc_probe(struct platform_device *pdev)
 		if (IS_ERR(pwmled->pwm)) {
 			ret = PTR_ERR(pwmled->pwm);
 			dev_err(&pdev->dev, "unable to request PWM: %d\n", ret);
+			fwnode_handle_put(fwnode);
 			goto destroy_mutex;
 		}
-		pwm_init_state(pwmled->pwm, &pwmled->pwmstate);
+		pwm_init_state(pwmled->pwm, &pwmled->state);
 
 		ret = fwnode_property_read_u32(fwnode, "color", &color);
 		if (ret) {
 			dev_err(&pdev->dev, "cannot read color: %d\n", ret);
+			fwnode_handle_put(fwnode);
 			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;
 	}
 
@@ -149,6 +147,8 @@ static int led_pwm_mc_probe(struct platform_device *pdev)
 
 destroy_mutex:
 	mutex_destroy(&priv->lock);
+release_mcnode:
+	fwnode_handle_put(mcnode);
 out:
 	return ret;
 }
-- 
2.35.0

[PATCH v3 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>
---

Notes:
    Changes in v3:
    * Remove multi-led unit name

 .../bindings/leds/leds-pwm-multicolor.yaml    | 75 +++++++++++++++++++
 1 file changed, 75 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..5a7ed5e1bb9f
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
@@ -0,0 +1,75 @@
+# 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
+    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:
+          - pwms
+          - color
+
+required:
+  - compatible
+
+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.0

[PATCH v3 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>
---

Notes:
    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               |   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 6090e647daee..bae1f63f6195 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -552,6 +552,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 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..bc4d21ddd74a
--- /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/err.h>
+#include <linux/kernel.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.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)
+{
+	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].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 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 release_mcnode;
+	}
+	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_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);
+			fwnode_handle_put(fwnode);
+			goto destroy_mutex;
+		}
+		pwm_init_state(pwmled->pwm, &pwmled->state);
+
+		ret = fwnode_property_read_u32(fwnode, "color", &color);
+		if (ret) {
+			dev_err(&pdev->dev, "cannot read color: %d\n", ret);
+			fwnode_handle_put(fwnode);
+			goto destroy_mutex;
+		}
+
+		subled[priv->mc_cdev.num_colors].color_index = color;
+		++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);
+release_mcnode:
+	fwnode_handle_put(mcnode);
+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: [PATCH v3 1/2] dt-bindings: leds: Add multicolor PWM LED bindings

[Jacek Anaszewski]

Hi Sven,

On 1/26/22 11:48 AM, 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>
> ---
[...]
> +
> +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>;

It doesn't make much sense to have such a big resolution of global
multi color brightness. 255 will be sufficient.

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

-- 
Best regards,
Jacek Anaszewski

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

[Marek Behún]

On Thu, 27 Jan 2022 22:24:21 +0100
Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

> Hi Sven,
> 
> On 1/26/22 11:48 AM, 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>
> > ---  
> [...]
> > +
> > +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>;  
> 
> It doesn't make much sense to have such a big resolution of global
> multi color brightness. 255 will be sufficient.

If the PWM supports it, why not?
On Omnia the default is 255, and since it is PWM, the change from 0/255
to 1/255 is much bigger then from, say, 15/255 to 16/255. So if 1/255
is too bright, you are then unable to set it less bright. I think 1024
or ever 65535 makes sense with PWMs.

Marek

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

[Jacek Anaszewski]

On 1/28/22 9:36 PM, Marek Behún wrote:
> On Thu, 27 Jan 2022 22:24:21 +0100
> Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> 
>> Hi Sven,
>>
>> On 1/26/22 11:48 AM, 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>
>>> ---
>> [...]
>>> +
>>> +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>;
>>
>> It doesn't make much sense to have such a big resolution of global
>> multi color brightness. 255 will be sufficient.
> 
> If the PWM supports it, why not?
> On Omnia the default is 255, and since it is PWM, the change from 0/255
> to 1/255 is much bigger then from, say, 15/255 to 16/255. So if 1/255
> is too bright, you are then unable to set it less bright. I think 1024
> or ever 65535 makes sense with PWMs.

With values other than 255 we will not achieve 24-bit RGB, which is one
problem, and the other one is non-linear brightness that can be achieved
with PWM. So probably we would need to add an additional note in the
documentation [0], saying that changing global brightness allows to
preserve combined LED hue only when all sub-leds are linear, and that it
will not be the case for PWM LEDs.

And I propose to change multi-led 'color' DT property value from
LED_COLOR_ID_RGB to LED_COLOR_ID_MULTI to avoid the impression that it
will work as traditional 24-bit RGB.

[0] Documentation/leds/leds-class-multicolor.rst

-- 
Best regards,
Jacek Anaszewski

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

[Marek Behún]

On Sat, 29 Jan 2022 00:04:01 +0100
Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

> On 1/28/22 9:36 PM, Marek Behún wrote:
> > On Thu, 27 Jan 2022 22:24:21 +0100
> > Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> >   
> >> Hi Sven,
> >>
> >> On 1/26/22 11:48 AM, 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>
> >>> ---  
> >> [...]  
> >>> +
> >>> +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>;  
> >>
> >> It doesn't make much sense to have such a big resolution of global
> >> multi color brightness. 255 will be sufficient.  
> > 
> > If the PWM supports it, why not?
> > On Omnia the default is 255, and since it is PWM, the change from 0/255
> > to 1/255 is much bigger then from, say, 15/255 to 16/255. So if 1/255
> > is too bright, you are then unable to set it less bright. I think 1024
> > or ever 65535 makes sense with PWMs.  
> 
> With values other than 255 we will not achieve 24-bit RGB, which is one
> problem, and the other one is non-linear brightness that can be achieved
> with PWM. So probably we would need to add an additional note in the
> documentation [0], saying that changing global brightness allows to
> preserve combined LED hue only when all sub-leds are linear, and that it
> will not be the case for PWM LEDs.
> 
> And I propose to change multi-led 'color' DT property value from
> LED_COLOR_ID_RGB to LED_COLOR_ID_MULTI to avoid the impression that it
> will work as traditional 24-bit RGB.
> 
> [0] Documentation/leds/leds-class-multicolor.rst

I know that color curves were being discussed at the time multicolor
was being introduced, and AFAIK Pavel didn't like it, but I don't
remember the reasons anymore.

As far as I understand it though, for PWM LEDs there is an equation for
gamma correction. So either we need to rename this LED to MULTI, or the
driver needs to do gamma correction so that the LED behaves RGB.

Pavel, what is your opinion on this?

Marek

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

[Alexander Dahl]

Hello,

Am Sat, Jan 29, 2022 at 12:26:39AM +0100 schrieb Marek Behún:
> On Sat, 29 Jan 2022 00:04:01 +0100
> Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> 
> > On 1/28/22 9:36 PM, Marek Behún wrote:
> > > On Thu, 27 Jan 2022 22:24:21 +0100
> > > Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> > >   
> > >> Hi Sven,
> > >>
> > >> On 1/26/22 11:48 AM, 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>
> > >>> ---  
> > >> [...]  
> > >>> +
> > >>> +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>;  
> > >>
> > >> It doesn't make much sense to have such a big resolution of global
> > >> multi color brightness. 255 will be sufficient.  
> > > 
> > > If the PWM supports it, why not?
> > > On Omnia the default is 255, and since it is PWM, the change from 0/255
> > > to 1/255 is much bigger then from, say, 15/255 to 16/255. So if 1/255
> > > is too bright, you are then unable to set it less bright. I think 1024
> > > or ever 65535 makes sense with PWMs.  
> > 
> > With values other than 255 we will not achieve 24-bit RGB, which is one
> > problem, and the other one is non-linear brightness that can be achieved
> > with PWM. So probably we would need to add an additional note in the
> > documentation [0], saying that changing global brightness allows to
> > preserve combined LED hue only when all sub-leds are linear, and that it
> > will not be the case for PWM LEDs.
> > 
> > And I propose to change multi-led 'color' DT property value from
> > LED_COLOR_ID_RGB to LED_COLOR_ID_MULTI to avoid the impression that it
> > will work as traditional 24-bit RGB.
> > 
> > [0] Documentation/leds/leds-class-multicolor.rst
> 
> I know that color curves were being discussed at the time multicolor
> was being introduced, and AFAIK Pavel didn't like it, but I don't
> remember the reasons anymore.
> 
> As far as I understand it though, for PWM LEDs there is an equation for
> gamma correction. 

That's right, and it gets a little more complicated if you have RGB
instead of a single LED.  A start for reading might be this:

https://hackaday.com/2016/08/23/rgb-leds-how-to-master-gamma-and-hue-for-perfect-brightness/

(I had bookmarked that back when I was hacking on firmware for an 8bit
microcontroller controlling an RGB LED through soft PWM few years
ago.  A very simple solution is a precalculated static lookup table.)

> So either we need to rename this LED to MULTI, or the
> driver needs to do gamma correction so that the LED behaves RGB.

Do those devices marked as RGB currently, have that gamma correction
integrated on chip? Examples?

Greets
Alex

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

[Sven Schwermer]

Hi,

On 1/31/22 08:10, Alexander Dahl wrote:
>> I know that color curves were being discussed at the time multicolor
>> was being introduced, and AFAIK Pavel didn't like it, but I don't
>> remember the reasons anymore.
>>
>> As far as I understand it though, for PWM LEDs there is an equation for
>> gamma correction.
> 
> That's right, and it gets a little more complicated if you have RGB
> instead of a single LED.  A start for reading might be this:
> 
> https://hackaday.com/2016/08/23/rgb-leds-how-to-master-gamma-and-hue-for-perfect-brightness/
> 
> (I had bookmarked that back when I was hacking on firmware for an 8bit
> microcontroller controlling an RGB LED through soft PWM few years
> ago.  A very simple solution is a precalculated static lookup table.)

This is a very interesting topic which I was gonna ask about on this 
list anyways. IMHO, the Linux leds subsystem needs a uniform way to deal 
with lightness/gamma correction. There is some previous work in pwm_bl 
and led_bl with their `brightness-levels` device tree property and 
cie1931 correction function (drivers/video/backlight/pwm_bl.c). It would 
be very nice to have this functionality in the led class. Having 
something similar for multi-color as well would be fantastic.

Sven

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

[Andy Shevchenko]

On Wed, Jan 26, 2022 at 11:05 PM <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.

...

> +       help
> +         This option enables support for PWM driven monochrome LEDs that are
> +         grouped into multicolor LEDs.

What would be the module name if compiled as a module?

...

> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>

mod_devicetable.h

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>

property.h

> +#include <linux/pwm.h>

...

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

Can we order in reversed xmas tree order?

       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;

Same for other functions.

...

> +               dev_err(&pdev->dev, "expected multi-led node\n");
> +               ret = -ENODEV;
> +               goto out;

return dev_err_probe(dev, -ENODEV, ...);

...

> +       /* count the nodes inside the multi-led node */
> +       fwnode_for_each_child_node(mcnode, fwnode)
> +               ++count;

Postincrement shall work the same way.

...

> +                       ret = PTR_ERR(pwmled->pwm);
> +                       dev_err(&pdev->dev, "unable to request PWM: %d\n", ret);
> +                       fwnode_handle_put(fwnode);
> +                       goto destroy_mutex;

fwnode_handle_put();
return dev_err_probe(...);

...

> +                       dev_err(&pdev->dev, "cannot read color: %d\n", ret);
> +                       fwnode_handle_put(fwnode);
> +                       goto destroy_mutex;

fwnode_handle_put();
return dev_err_probe();

> +               }

...

> +               ++priv->mc_cdev.num_colors;

Postincrement shall work the same way.

> +       }

...

> +               dev_err(&pdev->dev,
> +                       "failed to register multicolor PWM led for %s: %d\n",
> +                       cdev->name, ret);
> +               goto destroy_mutex;

return dev_err_probe();

...

> +               dev_err(&pdev->dev, "failed to set led PWM value for %s: %d",
> +                       cdev->name, ret);
> +               goto destroy_mutex;

return dev_err_probe();

...

> +destroy_mutex:
> +       mutex_destroy(&priv->lock);

Wrong ordering here and in ->remove().

Don't mix devm_* with non-devm_* calls.

> +release_mcnode:
> +       fwnode_handle_put(mcnode);

> +out:
> +       return ret;

Return directly.

> +}
> +
> +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", },

> +       {},

No comma needed for terminator entry.

> +};

...

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

> +

Redundant blank line.

> +module_platform_driver(led_pwm_mc_driver);


--
With Best Regards,
Andy Shevchenko

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

[Sven Schwermer]

Hi Andy,

Thanks for looking through my patch. I have a couple of follow-up 
questions about your feedback:

On 2/2/22 13:33, Andy Shevchenko wrote:
>> +                       ret = PTR_ERR(pwmled->pwm);
>> +                       dev_err(&pdev->dev, "unable to request PWM: %d\n", ret);
>> +                       fwnode_handle_put(fwnode);
>> +                       goto destroy_mutex;
> 
> fwnode_handle_put();
> return dev_err_probe(...);
This would skip the destruction of the mutex and releasing of mcnode. 
Isn't that problematic? The same goes for all of your comments of this kind.

>> +destroy_mutex:
>> +       mutex_destroy(&priv->lock);
> 
> Wrong ordering here and in ->remove().
> 
> Don't mix devm_* with non-devm_* calls.
What do you mean by this?

Best regards,
Sven

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

[Sven Schwermer]

> Skipping mutex destruction is not critical, but in general if you wish 
> to free allocated resource, it should be done in reversed order...
As far as I can tell, the ordering is already reversed:

mcnode = device_get_named_child_node(&pdev->dev, "multi-led");
/* ... */
mutex_init(&priv->lock);
/* ... */
fwnode_for_each_child_node(mcnode, fwnode) {
     /* ... */
     fwnode_handle_put(fwnode);
     goto destroy_mutex;
     /* ... */
}
/* ... */
destroy_mutex:
     mutex_destroy(&priv->lock);
release_mcnode:
     fwnode_handle_put(mcnode);
out:
     return ret;

>             +destroy_mutex:
>             +       mutex_destroy(&priv->lock);
> 
> 
>         Wrong ordering here and in ->remove().
> 
>         Don't mix devm_* with non-devm_* calls.
> 
>     What do you mean by this?
> 
> 
> ...which is exactly the issue with this code because of the use of 
> devm_*() calls mixed with non-devm_*() ones.
> 
> TL;DR: ordering is broken here. And to fix it you need either a) to 
> convert all calls to be devm_*(), or b) make them all non-devm, or c) 
> regroup resource allocation so that all devm followed by non-devm ones.Which non-devm calls are you referring to?

Thanks for your patience,
Sven

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

[Andy Shevchenko]

On Sun, Feb 6, 2022 at 1:04 PM Sven Schwermer <sven@svenschwermer.de> wrote:
>
> > Skipping mutex destruction is not critical, but in general if you wish
> > to free allocated resource, it should be done in reversed order...
> As far as I can tell, the ordering is already reversed:

I can't see it.

What I see is that mutex_destroy() is called before the LED class
unregisters (along with PWM and memory allocation). It means
potentially it may be the issue when during ->probe() error path or
->remove() somebody calls for LED functions which rely on mutex
presence (while it has been already destroyed).

Easiest way to fix this is to wrap mutex_destroy() to be a devm_*()
kind of function.

> >             +destroy_mutex:
> >             +       mutex_destroy(&priv->lock);
> >
> >
> >         Wrong ordering here and in ->remove().
> >
> >         Don't mix devm_* with non-devm_* calls.
> >
> >     What do you mean by this?
> >
> >
> > ...which is exactly the issue with this code because of the use of
> > devm_*() calls mixed with non-devm_*() ones.
> >
> > TL;DR: ordering is broken here. And to fix it you need either a) to
> > convert all calls to be devm_*(), or b) make them all non-devm, or c)
> > regroup resource allocation so that all devm followed by non-devm ones.Which non-devm calls are you referring to?


-- 
With Best Regards,
Andy Shevchenko

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

[Pavel Machek]

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

Hi!

> > > > Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> > > > ---
> > > [...]
> > > > +
> > > > +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>;
> > > 
> > > It doesn't make much sense to have such a big resolution of global
> > > multi color brightness. 255 will be sufficient.
> > 
> > If the PWM supports it, why not?
> > On Omnia the default is 255, and since it is PWM, the change from 0/255
> > to 1/255 is much bigger then from, say, 15/255 to 16/255. So if 1/255
> > is too bright, you are then unable to set it less bright. I think 1024
> > or ever 65535 makes sense with PWMs.
> 
> With values other than 255 we will not achieve 24-bit RGB, which is one
> problem, and the other one is non-linear brightness that can be achieved
> with PWM. So probably we would need to add an additional note in the
> documentation [0], saying that changing global brightness allows to
> preserve combined LED hue only when all sub-leds are linear, and that it
> will not be the case for PWM LEDs.

I have yet to see a LED that was anything but linear. If we'll want to
match colorspace of RGB displays, we'll need to do more work. (Often,
255/255/255 is not white with RGB LEDs, for example).

So 16-bit pwm makes sense to me.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]