linux-kernel - Re: [PATCH v4 1/5] 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: <20250410155946.5326-1-trannamatk@gmail.com>
Date: Thu, 10 Apr 2025 22:59:46 +0700
From: Nam Tran <trannamatk@...il.com>
To: krzk+dt@...nel.org
Cc: pavel@...nel.org,
	lee@...nel.org,
	robh@...nel.org,
	conor+dt@...nel.org,
	corbet@....net,
	devicetree@...r.kernel.org,
	linux-leds@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/5] dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver

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