linux-kernel - Re: [PATCH v9 1/4] dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20250617162947.16955-1-trannamatk@gmail.com>
Date: Tue, 17 Jun 2025 23:29:47 +0700
From: Nam Tran <trannamatk@...il.com>
To: krzk+dt@...nel.org
Cc: lee@...nel.org,
	pavel@...nel.org,
	robh@...nel.org,
	conor+dt@...nel.org,
	corbet@....net,
	linux-leds@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v9 1/4] dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver

On Wed, 11 Jun 2025, Krzysztof Kozlowski wrote:

> > +patternProperties:
> > +  "^led@[0-3]$":
> > +    type: object
> > +    $ref: common.yaml#
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      led-cur:
> > +        $ref: /schemas/types.yaml#/definitions/uint8
> > +        description: |
> > +          LED current in 0.1 mA steps (e.g., 150 = 15.0 mA; 0 if not connected)
> > +        minimum: 0
> > +        maximum: 255
> > +
> > +      max-cur:
> > +        $ref: /schemas/types.yaml#/definitions/uint8
> > +        description: Maximum allowed current in 0.1 mA steps
> > +
> > +      reg:
> > +        minimum: 0
> > +        maximum: 3
> 
> Place properties according to DTS coding style.

Got it! I'll update the property order accordingly.

> > +  '^multi-led@[4-7]$':
> > +    type: object
> > +    $ref: leds-class-multicolor.yaml#
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        minimum: 4
> > +        maximum: 7
> > +
> > +      '#address-cells':
> 
> Don't mix quotes. Either ' or "

I'll use consistent ".

> > +        const: 1
> > +
> > +      '#size-cells':
> > +        const: 0
> > +
> > +    patternProperties:
> > +      "^led@[4-9a-f]$":
> > +        type: object
> > +        $ref: common.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          led-cur:
> > +            $ref: /schemas/types.yaml#/definitions/uint8
> 
> No, use existing led common properties. Also observe the units - this is
> not uint8 but a defined type for microamp, see property-units in
> dtschema.
> 
> > +            description: |
> > +              LED current in 0.1 mA steps (e.g., 150 = 15.0 mA; 0 if not connected)
> > +            minimum: 0
> > +            maximum: 255
> > +
> > +          max-cur:
> > +            $ref: /schemas/types.yaml#/definitions/uint8
> 
> No, use existing led common properties. Same everywhere.

I'll replace max-cur with the standard led-max-microamp.
I'll remove led-cur as there's no equivalent LED common property to represent it.
The LED current can be configured runtime via the led_current sysfs.

> > +examples:
> > +  - |
> > +    #include <dt-bindings/leds/common.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        led-controller@1b {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            compatible = "ti,lp5812";
> > +            reg = <0x1b>;
> > +            vcc-supply = <&vdd_3v3_reg>;
> > +
> > +            led@0 {
> > +              reg = <0x0>;
> 
> 
> Messed/mixed indentation.

I'll fix it.

> BTW, such significant binding change at v9, invalidting reviews and
> rewriting the binding completely, is surprising.

Understood. I restructured the binding in v9 to align with leds-class-multicolor.yaml
and better represent the LP5812 hierarchy.
I'll make sure to highlight such major changes more clearly in future revisions.

Appreciate your time and feedback.

Best regards,
Nam Tran