Re: [PATCH] dt-bindings: leds: Convert LP8860 into YAML format

[Andrew Davis <afd@ti.com>]

On 12/9/24 8:35 AM, Sverdlin, Alexander wrote:
> Hi Andrew,
> 
> thanks for the prompt review!
> 
> On Mon, 2024-12-09 at 08:29 -0600, Andrew Davis wrote:
>>> +  reg:
>>> +    maxItems: 1
>>> +    description: I2C slave address
>>> +
>>> +  "#address-cells":
>>> +    const: 1
>>> +
>>> +  "#size-cells":
>>> +    const: 0
>>> +
>>> +  enable-gpios:
>>> +    maxItems: 1
>>> +    description: GPIO pin to enable (active high) / disable the device
>>> +
>>> +  vled-supply:
>>> +    description: LED supply
>>> +
>>> +patternProperties:
>>> +  "^led@[0]$":
>>> +    type: object
>>> +    $ref: common.yaml#
>>> +    unevaluatedProperties: false
>>> +
>>> +    properties:
>>> +      reg:
>>> +        description:
>>> +          Index of the LED.
>>> +        const: 0
>>> +
>>> +      function: true
>>> +      color: true
>>> +      label: true
>>> +      linux,default-trigger: true
>>> +
>>> +    required:
>>> +      - reg
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - "#address-cells"
>>> +  - "#size-cells"
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/gpio/gpio.h>
>>> +    #include <dt-bindings/leds/common.h>
>>> +
>>> +    i2c {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        led-controller@2d {
>>> +            compatible = "ti,lp8860";
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +            reg = <0x2d>;
>>> +            enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
>>> +            vled-supply = <&vbatt>;
>>> +
>>> +            led@0 {
>>
>> So same comment I made in the pre-public review, lets see what the DT
>> folks think:
>>
>> I don't think we want to have the "@0" node naming. It forces us to
>> add the "reg =" below, and that then forces us to add the #*-cells above.
>> All this to work around not just calling the node "led-0". The driver
>> doesn't care either way, and there are no in-tree users of the old way,
>> so now should be a safe time to fix this while converting the binding.
> 
> If I understood you correctly here:
> 
>>> And one channel controlling the others is only in this "Display Mode",
>>> but the currents to the others can be independently controlled in a
>>> different mode (seems these modes have less features which is probably
>>> why the driver doesn't make use of that today).
> 
> then some mapping between subnode and HW channel would be required.
> We probably don't want to parse a node name in this case and carve out "-0"
> part of it, in such case a well-defined property, such as "reg" would be
> required.
> 

We are not the only driver to do the "-#" method. Although yes just
using reg is easier for the Linux driver.

> So preserving the status quo looks more future-proof IMO.
> 

There will only ever be up to 4 LEDs, but you give fair points. No
strong preference from me then, and everything else looks good to me,

Acked-by: Andrew Davis <afd@ti.com>