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

[Conor Dooley <conor@kernel.org>]

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

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@oss.qualcomm.com>
> ---
>  .../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.
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.

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?
If so, why does your regex permit values greater than 15?

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
> 

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