Re: [PATCH 2/2] dt-bindings: leds: Add aw21024 binding

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 13/05/2022 21:04, Kyle Swenson wrote:
> Add device-tree bindings for Awinic's aw21024 24 channel RGB LED Driver.
> 
> Datasheet:
> https://www.awinic.com/Public/Uploads/uploadfile/files/20200511/20200511165751_5eb9138fcd9e3.PDF
> 
> Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> ---
>  .../bindings/leds/leds-aw21024.yaml           | 157 ++++++++++++++++++
>  1 file changed, 157 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-aw21024.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-aw21024.yaml b/Documentation/devicetree/bindings/leds/leds-aw21024.yaml
> new file mode 100644
> index 000000000000..1180c02b5d21
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-aw21024.yaml
> @@ -0,0 +1,157 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-aw21024.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AWINIC AW21024 24-channel LED Driver
> +
> +maintainers:
> +  - Kyle Swenson <kyle.swenson@est.tech>
> +
> +description: |
> +  The AW21024 is a 24-channel LED driver with an I2C interface.
> +
> +  For more product information please see the link below:
> +  https://www.awinic.com/en/index/pageview/catid/19/id/28.html
> +
> +properties:
> +  compatible:
> +    const: awinic,aw21024
> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      I2C peripheral address

Skip description, it's obvious.

> +
> +  enable-gpios:
> +    maxItems: 1
> +    description: GPIO pin to enable/disable the device.

Skip description, it's obvious.

> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  '^multi-led@[0-9a-f]$':
> +    type: object
> +    $ref: leds-class-multicolor.yaml#
> +    properties:
> +      reg:
> +        minItems: 1
> +        maxItems: 24
> +        description:
> +          Denotes the LED indicies that should be grouped into a
> +          single multi-color LED.
> +
> +    patternProperties:
> +      "(^led-[0-9a-f]$|led)":

How does this pass your own bindings? In the DTS you use underscofer
which is not here...

You need to test the bindings before sending them to people.

> +        type: object
> +        $ref: common.yaml#
> +
> +patternProperties:
> +  "^led@[0-2]$":
> +    type: object
> +    $ref: common.yaml#
> +
> +    properties:
> +      reg:
> +        description: Index of the LED.
> +        minimum: 0
> +        maximum: 23
> +
> +    description:
> +      Specifies a single LED at the specified index
> +
> +

Just one line. Plus errors pointed out by Rob's bot.

> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +   #include <dt-bindings/gpio/gpio.h>
> +   #include <dt-bindings/leds/common.h>
> +
> +   i2c {
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +
> +        led-controller@30 {
> +            compatible = "awinic,aw21024";
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <0x30>;

reg after compatible.

> +            enable-gpios = <&gpio1 23>;
> +
> +            multi-led@1 {
> +                #address-cells = <1>;
> +                #size-cells = <2>;
> +                reg = <0x0 0x1 0x2>;

This is confusing. Does not match unit address and address/size cells.
Perhaps you wanted three separate regs?


> +                color = <LED_COLOR_ID_RGB>;
> +                label = "RGB_LED1";
> +
> +                led-0 {
> +                    color = <LED_COLOR_ID_RED>;
> +                };
> +
> +                led-1 {
> +                    color = <LED_COLOR_ID_GREEN>;
> +                };
> +
> +                led-2 {
> +                    color = <LED_COLOR_ID_BLUE>;
> +                };
> +
> +            };
> +            multi-led@2 {
> +                #address-cells = <1>;
> +                #size-cells = <3>;
> +                reg = <0x3 0x4 0x5 0x6>;

The same

> +                color = <LED_COLOR_ID_RGB>;
> +                label = "RGBW_LED1";

Why labels are upper-case?

> +
> +                led-4 {
> +                    color = <LED_COLOR_ID_RED>;
> +                };
> +
> +                led-5 {
> +                    color = <LED_COLOR_ID_GREEN>;
> +                };
> +
> +                led-6 {
> +                    color = <LED_COLOR_ID_BLUE>;
> +                };
> +
> +                led-7 {
> +                    color = <LED_COLOR_ID_WHITE>;
> +                };
> +            };
> +            ready_led@3 {

No underscores in node names. Generic node name, so just led.

> +                #address-cells = <1>;
> +                #size-cells = <1>;
> +                reg = <0x7 0x8>;

The same problem with reg.

> +                label = "READY";
> +                color = <LED_COLOR_ID_MULTI>;
> +
> +                led-8 {
> +                  color = <LED_COLOR_ID_RED>;
> +                };
> +
> +                led-9 {
> +                  color = <LED_COLOR_ID_GREEN>;
> +                };
> +            };
> +            connected_led@4 {
> +                reg = <0x9>;
> +                label = "CONNECTED";
> +                color = <LED_COLOR_ID_BLUE>;
> +            };
> +        };
> +    };
> +
> +...


Best regards,
Krzysztof