linux-kernel - Re: [PATCH v5 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: <20250417020622.1562-1-trannamatk@gmail.com>
Date: Thu, 17 Apr 2025 09:06:22 +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 v5 1/5] dt-bindings: leds: add TI/National Semiconductor LP5812 LED Driver

On Tue, 15 Apr 2025, Krzysztof Kozlowski wrote:

>On 15/04/2025 11:53, Nam Tran wrote:
>> On Mon, 14 Apr 2025, Krzysztof Kozlowski wrote:
>> 
>>> On 14/04/2025 16:57, Nam Tran wrote:
>>>> +
>>>> +description: |
>>>> +  The LP5812 is an I2C LED Driver that can support LED matrix 4x3.
>>>> +  For more product information please see the link below:
>>>> +  https://www.ti.com/product/LP5812#tech-docs
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: ti,lp5812
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  "#address-cells":
>>>> +    const: 1
>>>> +
>>>> +  "#size-cells":
>>>> +    const: 0
>>>
>>> No need for supply?
>> 
>> Since the hardware uses an external power supply,
>> we decide not to include the supply property in the binding.
>
>So there is power supply? If so, must be in the binding. Bindings
>describe given hardware (LP5812), not your particular board/setup.

Thank you for the clarification.
The LP5812 is externally powered and has a dedicated VCC pin.
I'll update the binding to include a `vcc-supply` property.

>> 
>>>> +
>>>> +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
>>>
>>> My comment stay valid. I don't think LEDs have channels, datasheet also
>>> has nothing about channels, so again - use existing properties. Or
>>> better drop it - I don't see any point in the name. The reg already
>>> defines it.
>> 
>> The channel was named for the output channel to each LED, not the LED channels.
>
>I don't understand what you want to say. Please explain why existing
>label property is not correct here.

I understand that the label property is deprecated and that the preferred approach now is to use function and color instead.
However, in the case of the LP5812, which is a matrix LED driver, these properties are not a good fit.
The LP5812 does not associate each output with a specific function (like "status", "activity"),
and the LEDs driven by LP5812 are not fixed to a particular color.

>> but the person who wants to develop LP5812's matrix-related features can use the "channels" for easy mapping.
>
>easy mapping of what? Please show me the usage.

You're right — I cannot provide a meaningful usage example for chan-name.
The chan-name property was intended to give a more descriptive name for each LED channel, mainly for convenience in user space.
But since this isn’t standard and you advised against introducing such a property, we’ve decided to drop it.

>> 
>>>
>>> However after dropping this, your example has nodes with only reg -
>>> what's the point of them? Why no properties from common.yaml are
>>> applicable? If they are not applicable, then the entire subnode should
>>> be dropped - you don't need them to describe the hardware.
>> 
>> Actually, the "color" property can be applied, but the LP5812 is a matrix LED,
>> so specifying a particular LED color is not necessary when developing LP5812 features.
>
>This does not help me much and based on this I see no points in
>describing individual LEDs, because the only missing information is
>number of them but even that is fixed for given device, isn't it?

Actually, the number of LED outputs on the LP5812 is not strictly fixed — it depends on the selected operating mode.
This mode is configurable by the end user at runtime through sysfs interfaces provided by the driver.

I understand your point — if no additional properties from common.yaml are applicable, these subnodes may not be necessary.
Therefore, we’ve decided to drop them.

Best regards,
Nam Tran