| Message ID | 20220207100326.426940-1-sven@svenschwermer.de |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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 --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,
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: