diff mbox

[v5,0/2] Multicolor PWM LED support

Message ID 20220207100326.426940-1-sven@svenschwermer.de
State New
Headers show

Commit Message

Sven Schwermer Feb. 7, 2022, 10:03 a.m. UTC
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                          |  11 ++
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-pwm-multicolor.c            | 186 ++++++++++++++++++
 4 files changed, 273 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml
 create mode 100644 drivers/leds/leds-pwm-multicolor.c

Interdiff against v4:

Comments

Rob Herring Feb. 7, 2022, 8:21 p.m. UTC | #1
On Mon, Feb 07, 2022 at 11:03:25AM +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 v5:
>     * (no changes)
>     
>     Changes in v4:
>     * (no changes)
>     
>     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#

This schema says 'multi-led' here should have a child called 
"^multi-led@([0-9a-f])$". You are off a level.
> +
> +    patternProperties:
> +      "^led-[0-9a-z]+$":
> +        type: object

           $ref: common.yaml#
           additionalProperties: false


> +        properties:
> +          pwms:
> +            maxItems: 1
> +
> +          pwm-names: true
> +
> +          color:
> +            $ref: common.yaml#/properties/color

And then drop this ref.

> +
> +        required:
> +          - pwms
> +          - color
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    rgb-led {
> +        compatible = "pwm-leds-multicolor";
> +
> +        multi-led {

Can't this be collapsed into 1 level? I don't see "pwm-leds-multicolor" 
having other child nodes.

> +          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
> 
>
Sven Schwermer Feb. 7, 2022, 8:44 p.m. UTC | #2
Hi Rob,

Thanks for your comments.

On 2/7/22 21:21, Rob Herring wrote:
>> +properties:
>> +  compatible:
>> +    const: pwm-leds-multicolor
>> +
>> +  multi-led:
>> +    type: object
>> +    allOf:
>> +      - $ref: leds-class-multicolor.yaml#
> 
> This schema says 'multi-led' here should have a child called
> "^multi-led@([0-9a-f])$". You are off a level.

So it should have been?

properties:
   compatible:
     const: pwm-leds-multicolor
   allOf:
     - $ref: leds-class-multicolor.yaml#

This would imply that the multi-led node requires a unit address (reg 
property). That doesn't make sense in this case. How should we resolve this?

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

Sounds good.

>> +        properties:
>> +          pwms:
>> +            maxItems: 1
>> +
>> +          pwm-names: true
>> +
>> +          color:
>> +            $ref: common.yaml#/properties/color
> 
> And then drop this ref.

Curiosity question: why? Should I refer to an unsigned integer type instead?

>> +    rgb-led {
>> +        compatible = "pwm-leds-multicolor";
>> +
>> +        multi-led {
> 
> Can't this be collapsed into 1 level? I don't see "pwm-leds-multicolor"
> having other child nodes.

It could. The reason I added the multi-led level is because the 
leds-class-multicolor.yaml schema calls for it. Perhaps I missed the 
intention of that schema but isn't it there to create a uniform binding 
schema structure across drivers?

Best regards,
Sven
Rob Herring Feb. 7, 2022, 9:43 p.m. UTC | #3
On Mon, Feb 7, 2022 at 2:44 PM Sven Schwermer <sven@svenschwermer.de> wrote:
>
> Hi Rob,
>
> Thanks for your comments.
>
> On 2/7/22 21:21, Rob Herring wrote:
> >> +properties:
> >> +  compatible:
> >> +    const: pwm-leds-multicolor
> >> +
> >> +  multi-led:
> >> +    type: object
> >> +    allOf:
> >> +      - $ref: leds-class-multicolor.yaml#
> >
> > This schema says 'multi-led' here should have a child called
> > "^multi-led@([0-9a-f])$". You are off a level.
>
> So it should have been?
>
> properties:
>    compatible:
>      const: pwm-leds-multicolor
>    allOf:
>      - $ref: leds-class-multicolor.yaml#

Not quite. DT property names and json-schema vocab names should never
be at the same level. So allOf should be at the root level.

> This would imply that the multi-led node requires a unit address (reg
> property). That doesn't make sense in this case. How should we resolve this?

I meant to mention that. Update the regex pattern to allow just
'multi-led': "^multi-led(@[0-9a-f])?$"


> >> +    patternProperties:
> >> +      "^led-[0-9a-z]+$":
> >> +        type: object
> >
> >             $ref: common.yaml#
> >             additionalProperties: false
>
> Sounds good.
>
> >> +        properties:
> >> +          pwms:
> >> +            maxItems: 1
> >> +
> >> +          pwm-names: true
> >> +
> >> +          color:
> >> +            $ref: common.yaml#/properties/color
> >
> > And then drop this ref.
>
> Curiosity question: why? Should I refer to an unsigned integer type instead?

Generally we want schemas to apply to nodes rather than individual
properties. Think of each node as a class and nodes can be 1 or more
subclasses. It's more important when using 'unevaluatedProperties' in
combination with refs.

'color: true' is all you need here. So it's less duplication. Not so
much here since it is just 1 property, but in general.

>
> >> +    rgb-led {
> >> +        compatible = "pwm-leds-multicolor";
> >> +
> >> +        multi-led {
> >
> > Can't this be collapsed into 1 level? I don't see "pwm-leds-multicolor"
> > having other child nodes.
>
> It could. The reason I added the multi-led level is because the
> leds-class-multicolor.yaml schema calls for it. Perhaps I missed the
> intention of that schema but isn't it there to create a uniform binding
> schema structure across drivers?

Yeah, I guess that's a good enough reason.

Rob
diff mbox

Patch

diff --git a/drivers/leds/leds-pwm-multicolor.c b/drivers/leds/leds-pwm-multicolor.c
index 96712b8ca98e..45e38708ecb1 100644
--- a/drivers/leds/leds-pwm-multicolor.c
+++ b/drivers/leds/leds-pwm-multicolor.c
@@ -58,6 +58,43 @@  static int led_pwm_mc_set(struct led_classdev *cdev,
 	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;
@@ -65,10 +102,8 @@  static int led_pwm_mc_probe(struct platform_device *pdev)
 	struct led_classdev *cdev;
 	struct mc_subled *subled;
 	struct pwm_mc_led *priv;
-	struct pwm_led *pwmled;
 	int count = 0;
 	int ret = 0;
-	u32 color;
 
 	mcnode = device_get_named_child_node(&pdev->dev, "multi-led");
 	if (!mcnode)
@@ -101,28 +136,9 @@  static int led_pwm_mc_probe(struct platform_device *pdev)
 	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 release_mcnode;
-		}
-		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 release_mcnode;
-		}
-
-		subled[priv->mc_cdev.num_colors].color_index = color;
-		priv->mc_cdev.num_colors++;
-	}
+	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,