Re: [PATCH v3 2/4] dt-bindings: leds: nxp,pca963x: add multicolor LED support

[Loic Poulain]

Hi Conor,


On Mon, Jul 6, 2026 at 6:56 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> On Mon, Jul 06, 2026 at 05:49:56PM +0200, Loic Poulain wrote:
> > Add support for grouping individual PCA963x channels into a multicolor
> > LED by introducing a multi-led@N node pattern. This follows the
> > convention established by other multicolor LED drivers such as
> > kinetic,ktd202x.
> >
> > This is necessary to support and model hardware setups where multiple
> > PWM channels drive a single physical RGB LED.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/leds/nxp,pca963x.yaml      | 104 ++++++++++++++++++++-
> >  1 file changed, 101 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/nxp,pca963x.yaml b/Documentation/devicetree/bindings/leds/nxp,pca963x.yaml
> > index 4034139e3f770a1035208490a9436bdc33387279..dccdb1d9421199a51cac5c7f3f7da503caa35622 100644
> > --- a/Documentation/devicetree/bindings/leds/nxp,pca963x.yaml
> > +++ b/Documentation/devicetree/bindings/leds/nxp,pca963x.yaml
> > @@ -74,6 +74,40 @@ patternProperties:
> >      required:
> >        - reg
> >
> > +  "^multi-led@[0-9a-f]+$":
> > +    type: object
> > +    $ref: leds-class-multicolor.yaml#
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        minimum: 0
>
> I still don't understand this, minimum zero is literally useless since
> this cannot be negative.

Good point, yes. I just reused the exact same pattern already used for
the existing led@ property (minimum: 0, with the maximum only set in
the conditional sections).

> If anything, you should be constraining the maximum value. Remember,
> outside of conditional sections the maximum permitted values should be
> defined. Conditional sections should just narrow the constraints.

Ok I will just set `maximum:15` here and address additional
constraints in the conditional section.

>
> Same below.
> pw-bot: changes-requested
>
> > +
> > +      "#address-cells":
> > +        const: 1
> > +
> > +      "#size-cells":
> > +        const: 0
> > +
> > +    patternProperties:
> > +      "^led@[0-9a-f]+$":
> > +        type: object
> > +        $ref: common.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          reg:
> > +            minimum: 0
> > +
> > +        required:
> > +          - reg
> > +          - color
> > +
> > +    required:
> > +      - reg
> > +      - "#address-cells"
> > +      - "#size-cells"
> > +
> >  allOf:
> >    - if:
> >        properties:
> > @@ -84,10 +118,16 @@ allOf:
> >                - nxp,pca9633
> >      then:
> >        patternProperties:
> > -        "^led@[0-9a-f]+$":
> > +        "^.*led@[0-9a-f]+$":
> >            properties:
> >              reg:
> >                maximum: 3
> > +        "^multi-led@[0-9a-f]+$":
> > +          patternProperties:
> > +            "^led@[0-9a-f]+$":
> > +              properties:
> > +                reg:
> > +                  maximum: 3
> >    - if:
> >        properties:
> >          compatible:
> > @@ -96,16 +136,28 @@ allOf:
> >                - nxp,pca9634
> >      then:
> >        patternProperties:
> > -        "^led@[0-9a-f]+$":
> > +        "^.*led@[0-9a-f]+$":
> >            properties:
> >              reg:
> >                maximum: 7
> > +        "^multi-led@[0-9a-f]+$":
> > +          patternProperties:
> > +            "^led@[0-9a-f]+$":
> > +              properties:
> > +                reg:
> > +                  maximum: 7
> >      else:
> >        patternProperties:
> > -        "^led@[0-9a-f]+$":
> > +        "^.*led@[0-9a-f]+$":
> >            properties:
> >              reg:
> >                maximum: 15
> > +        "^multi-led@[0-9a-f]+$":
> > +          patternProperties:
> > +            "^led@[0-9a-f]+$":
> > +              properties:
> > +                reg:
> > +                  maximum: 15
>
> Is this 15 the maximum for all devices?

15 is the hardware max for pca9635, and serves as the default for any
future/unlisted compatible via the else branch. Per your comment
above, I'll move it into the non-conditional block so it's enforced
unconditionally instead of only through else.

> If so, why does your regex permit values greater than 15?

The unit-address regex only validates that the name is well-formed
hex, and not the numeric bound. DT convention requires the unit
address to equal the node's reg value in hex, so in practice this is
limited to 15.
This is again a copy of what the yaml already describes for other
device's constraints. However, I can tighten the regex itself instead
if you'd prefer that approach, but that would make the yaml a bit
inconsistent.

Regards,
Loic





>
> Cheers,
> Conor.
>
> >
> >  additionalProperties: false
> >
> > @@ -149,4 +201,50 @@ examples:
> >          };
> >      };
> >
> > +  - |
> > +    #include <dt-bindings/leds/common.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        led-controller@62 {
> > +            compatible = "nxp,pca9633";
> > +            reg = <0x62>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            /* Three channels controlling one RGB LED */
> > +            multi-led@0 {
> > +                    reg = <0>;
> > +                    color = <LED_COLOR_ID_RGB>;
> > +                    function = LED_FUNCTION_STATUS;
> > +                    #address-cells = <1>;
> > +                    #size-cells = <0>;
> > +
> > +                    led@0 {
> > +                            reg = <0>;
> > +                            color = <LED_COLOR_ID_RED>;
> > +                    };
> > +
> > +                    led@1 {
> > +                            reg = <1>;
> > +                            color = <LED_COLOR_ID_GREEN>;
> > +                    };
> > +
> > +                    led@2 {
> > +                            reg = <2>;
> > +                            color = <LED_COLOR_ID_BLUE>;
> > +                    };
> > +            };
> > +
> > +            /* Remaining channel used as a plain white LED */
> > +            led@3 {
> > +                    reg = <3>;
> > +                    color = <LED_COLOR_ID_WHITE>;
> > +                    function = LED_FUNCTION_STATUS;
> > +            };
> > +        };
> > +    };
> > +
> >  ...
> >
> > --
> > 2.34.1
> >