linux-kernel - Re: [PATCH v2 1/2] dt-bindings: leds: Add LP5812 LED driver bindings

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: <20250226-delectable-gorgeous-anaconda-43eed8@krzk-bin>
Date: Wed, 26 Feb 2025 08:37:21 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Nam Tran <trannamatk@...il.com>
Cc: pavel@...nel.org, lee@...nel.org, krzk+dt@...nel.org, robh@...nel.org, 
	conor+dt@...nel.org, devicetree@...r.kernel.org, linux-leds@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] dt-bindings: leds: Add LP5812 LED driver bindings

On Wed, Feb 26, 2025 at 12:06:00AM +0700, Nam Tran wrote:
> This documentation ensures proper integration of the LP5812
> in Device Tree-based systems.

Nothing improved above.

Please read my previous comment and implement it or respond to it, if
you disagree.

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> Signed-off-by: Nam Tran <trannamatk@...il.com>
> ---
>  .../devicetree/bindings/leds/ti,lp5812.yaml   | 34 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++++
>  2 files changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/ti,lp5812.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/ti,lp5812.yaml b/Documentation/devicetree/bindings/leds/ti,lp5812.yaml
> new file mode 100644
> index 000000000000..1eb395230531
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/ti,lp5812.yaml
> @@ -0,0 +1,34 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +
> +title: LED driver for LP5812 LED from Texas Instruments.

LED driver as Linux driver? Or LP5812 LED driver as electrical driver?
Decide, but if first then rewrite it to describe hardware.

Also drop full stop. Titles do not have full stops.

> +
> +maintainers:
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The LP5812 is an I2C LED Driver that can support LED matrix 4x3.

So Linux driver? Drop driver references and describe hardware.

> +
> +properties:
> +  compatible:
> +    const: ti,lp5812
> +
> +  reg:
> +    description: I2C slave address

Drop description

> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg

No ref to LED common schema? No other properties? This is too
incomplete.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c1 {

i2c

> +      lp5812@1b {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +        compatible = "ti,lp5812";
> +        reg = <0x1b>;
> +      };
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4ff26fa94895..32312959d595 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23494,6 +23494,12 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/leds/backlight/ti,lp8864.yaml
>  F:	drivers/leds/leds-lp8864.c
>  
> +TEXAS INSTRUMENTS' LP5812 LED DRIVER

5 is before 8, so this does not look sorted.

And finally, please test before posting...

Best regards,
Krzysztof