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

[Krzysztof Kozlowski <krzk@kernel.org>]

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.

> 
>>> +
>>> +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.

> They are not required properties because we can control entirely the LEDs of LP5812 through the indexes (regs property),

I did not ask about this.

> 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.

> 
>>
>> 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?

Best regards,
Krzysztof