Re: [PATCH v25 03/16] dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers

[Dan Murphy <dmurphy@ti.com>]

Rob

On 5/26/20 8:59 PM, Rob Herring wrote:
> On Tue, May 26, 2020 at 11:46:39AM -0500, Dan Murphy wrote:
>> Introduce the bindings for the Texas Instruments LP5036, LP5030, LP5024,
>> LP5018, LP5012 and LP5009 RGB LED device driver.  The LP5036/30/24/18/12/9
>> can control RGB LEDs individually or as part of a control bank group.
>> These devices have the ability to adjust the mixing control for the RGB
>> LEDs to obtain different colors independent of the overall brightness of
>> the LED grouping.
>>
>> Datasheet:
>> http://www.ti.com/lit/ds/symlink/lp5012.pdf
>> http://www.ti.com/lit/ds/symlink/lp5024.pdf
>> http://www.ti.com/lit/ds/symlink/lp5036.pdf
>>
>> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   .../devicetree/bindings/leds/leds-lp50xx.yaml | 180 ++++++++++++++++++
>>   1 file changed, 180 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
>> new file mode 100644
>> index 000000000000..a2ea03e07f6d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
>> @@ -0,0 +1,180 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/leds/leds-lp50xx.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: LED driver for LP50XX RGB LED from Texas Instruments.
>> +
>> +maintainers:
>> +  - Dan Murphy <dmurphy@ti.com>
>> +
>> +description: |
>> +  The LP50XX is multi-channel, I2C RGB LED Drivers that can group RGB LEDs into
>> +  a LED group or control them individually.
>> +
>> +  The difference in these RGB LED drivers is the number of supported RGB
>> +  modules.
>> +
>> +  For more product information please see the link below:
>> +  http://www.ti.com/lit/ds/symlink/lp5012.pdf
>> +  http://www.ti.com/lit/ds/symlink/lp5024.pdf
>> +  http://www.ti.com/lit/ds/symlink/lp5036.pdf
>> +
>> +properties:
>> +  compatible:
>> +     oneOf:
>> +      - const: ti,lp5009
>> +      - const: ti,lp5012
>> +      - const: ti,lp5018
>> +      - const: ti,lp5024
>> +      - const: ti,lp5030
>> +      - const: ti,lp5036
> Use enum rather than oneOf+const.
Ok
>
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description:
>> +      I2C slave address
>> +      lp5009/12 - 0x14, 0x15, 0x16, 0x17
>> +      lp5018/24 - 0x28, 0x29, 0x2a, 0x2b
>> +      lp5030/36 - 0x30, 0x31, 0x32, 0x33
>> +
>> +  enable-gpios:
>> +    description: GPIO pin to enable/disable the device.
> How many? (maxItems: 1)
1
>
>> +
>> +  vled-supply:
>> +    description: LED supply.
>> +
>> +  child-node:
> This literally requires a node called 'child-node'. Not what you want.
>
> You need a $ref to the multi-color schema in here and then only define
> what's specific to this chip.
Ok
>
>> +    type: object
>> +    properties:
>> +      reg:
>> +        description: This is the LED module number.
> Constraints?

What type of constraints are needed here?  They vary based on what LED 
device you are using.


>
>> +
>> +      color:
>> +        description: Must be LED_COLOR_ID_MULTI
>> +
>> +      function:
>> +        description: see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +      ti,led-bank:
>> +        description:
>> +          This property denotes the LED module numbers that will be controlled as
>> +          a single RGB cluster.  Each LED module number will be controlled by a
>> +          single LED class instance.
>> +          There can only be one instance of the ti,led-bank
>> +          property for each device node.  This is a required node is the LED
>> +          modules are to be backed.
>> +        $ref: /schemas/types.yaml#definitions/uint32-array
> What is reg then? Some made up index? Can't you do:
>
> reg = <1 2 3>;
> led@1 {};
> led@2 {};
> led@2 {};

reg becomes the first LED module number in the banked LED group.

This chip has the ability to group or bank and control RGB LED clusters 
via a single register.

Or the device can control a single RGB LED cluster.  The device needs to 
be programmed with what LED modules are banked

together.  The bank numbers and LED module numbers and output numbers 
are not the same.  So this property indicates what modules are banked as 
in the

multi-led@2 example.

     #size-cells = <0>;
+            reg = <2>;
+            color = <LED_COLOR_ID_MULTI>;
+            function = LED_FUNCTION_STANDBY;
+            ti,led-bank = <2 3 5>;
+
+            led@6 {
+              reg = <0x6>;
+              color = <LED_COLOR_ID_RED>;
+              led-sources = <6 9 15>;
+            };
+
+            led@7 {
+              reg = <0x7>;
+              color = <LED_COLOR_ID_GREEN>;
+              led-sources = <7 10 16>;
+            };
+
+            led@8 {
+              reg = <0x8>;
+              color = <LED_COLOR_ID_BLUE>;
+              led-sources = <8 11 17>;
+            };
+         };

>> +
>> +    required:
>> +      - reg
>> +      - color
>> +      - function
>> +
>> +  grandchild-node:
> Again, no.
ok
>
>> +    type: object
>> +    properties:
>> +      reg:
>> +        description:
>> +          A single entry denoting the LED output that controls the monochrome LED.
> Constraints?

Same as above


>> +
>> +      color:
>> +        description:
>> +          see Documentation/devicetree/bindings/leds/common.txt
> Have you read this file recently? Don't add new references to it. (And
> generally freeform references to other files are wrong with schemas).

These patchsets and versions have been around for a very long time.

So this and all references to the common.txt are artifacts prior to the 
text file being obsoleted.

I will reference the common.yaml file.

Dan