Re: [PATCH v4 1/5] dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver

[Nam Tran <trannamatk@gmail.com>]

On Sun, 6 Apr 2025, Krzysztof Kozlowski wrote:

> On 05/04/2025 20:32, Nam Tran wrote:
> > +properties:
> > +  compatible:
> > +    const: ti,lp5812
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description:
> > +      I2C slave address
> > +      lp5812/12- 0x1b
> 
> Drop description, redundant.

I will drop it.

> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^led@[0-9a-b]$":
> > +    type: object
> > +    $ref: common.yaml#
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        minimum: 0
> > +        maximum: 0xb
> > +
> > +      chan-name:
> > +        $ref: /schemas/types.yaml#/definitions/string
> > +        description: LED channel name
> 
> Isn't this existing label property? Or node name? You don't need this
> and instead whatever currently LED subsystem is expecting (label got
> discouraged so maybe there is something else now).

It isn't common led and multi-led. LP5812 is new 4x3 matrix leds on mainline.
Therefore, it isn't same as existing led driver common.

> There is no multi-led support in the device? Datasheet this can work as
> matrix and as direct drive of 4 LEDs, so binding looks incomplete. Not
> sure what you exactly miss here - check other recent devices with
> similar features.

My device doesn't support multi-led. Leds are controlled by the control register index directly.
I will list more D0 - D3 leds. However, the maximum of leds are 12 LEDS.
It depends on the operation mode that the end user selects.

> > +
> > +    required:
> > +      - reg
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/leds/common.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        led-controller@1b {
> > +            compatible = "ti,lp5812";
> > +            reg = <0x1b>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            led@0 {
> > +                    reg = <0x0>;
> > +                    chan-name = "a0";
> 
> Mixed up indentation.

I will fix the indentation.

Thanks for your detailed review.
Appreciate your time and feedback!

Best regards,
Nam Tran