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

From: Dan Murphy
Date: Wed May 27 2020 - 13:27:17 EST

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@xxxxxxxxx>
Signed-off-by: Dan Murphy <dmurphy@xxxxxx>
---
.../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@xxxxxx>
+
+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