On Tue, May 17, 2022 at 11:08:08AM +0200, Krzysztof Kozlowski wrote: > On 13/05/2022 21:04, Kyle Swenson wrote: > > Add device-tree bindings for Awinic's aw21024 24 channel RGB LED Driver. > > > > Datasheet: > > https://www.awinic.com/Public/Uploads/uploadfile/files/20200511/20200511165751_5eb9138fcd9e3.PDF > > > > Signed-off-by: Kyle Swenson <kyle.swenson@xxxxxxxx> > > --- > > .../bindings/leds/leds-aw21024.yaml | 157 ++++++++++++++++++ > > 1 file changed, 157 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/leds/leds-aw21024.yaml > > > > diff --git a/Documentation/devicetree/bindings/leds/leds-aw21024.yaml b/Documentation/devicetree/bindings/leds/leds-aw21024.yaml > > new file mode 100644 > > index 000000000000..1180c02b5d21 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/leds/leds-aw21024.yaml > > @@ -0,0 +1,157 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/leds/leds-aw21024.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: AWINIC AW21024 24-channel LED Driver > > + > > +maintainers: > > + - Kyle Swenson <kyle.swenson@xxxxxxxx> > > + > > +description: | > > + The AW21024 is a 24-channel LED driver with an I2C interface. > > + > > + For more product information please see the link below: > > + https://www.awinic.com/en/index/pageview/catid/19/id/28.html > > + > > +properties: > > + compatible: > > + const: awinic,aw21024 > > + > > + reg: > > + maxItems: 1 > > + description: > > + I2C peripheral address > > Skip description, it's obvious. Okay. > > > + > > + enable-gpios: > > + maxItems: 1 > > + description: GPIO pin to enable/disable the device. > > Skip description, it's obvious. Sounds good, will do. > > > + > > + '#address-cells': > > + const: 1 > > + > > + '#size-cells': > > + const: 0 > > + > > +patternProperties: > > + '^multi-led@[0-9a-f]$': > > + type: object > > + $ref: leds-class-multicolor.yaml# > > + properties: > > + reg: > > + minItems: 1 > > + maxItems: 24 > > + description: > > + Denotes the LED indicies that should be grouped into a > > + single multi-color LED. > > + > > + patternProperties: > > + "(^led-[0-9a-f]$|led)": > > How does this pass your own bindings? In the DTS you use underscofer > which is not here... > > You need to test the bindings before sending them to people. > So honestly, and you've probably guessed as much from this patch and Rob's bot, that it doesn't pass the binding checks. I learned about make dt_binding_check shortly after Rob's bot pointed it out to me, and I apologize for my ignorance wasting your time. > > + type: object > > + $ref: common.yaml# > > + > > +patternProperties: > > + "^led@[0-2]$": > > + type: object > > + $ref: common.yaml# > > + > > + properties: > > + reg: > > + description: Index of the LED. > > + minimum: 0 > > + maximum: 23 > > + > > + description: > > + Specifies a single LED at the specified index > > + > > + > > Just one line. Plus errors pointed out by Rob's bot. Yep, got it. > > +required: > > + - compatible > > + - reg > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + #include <dt-bindings/leds/common.h> > > + > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + led-controller@30 { > > + compatible = "awinic,aw21024"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x30>; > > reg after compatible. Okay. > > + enable-gpios = <&gpio1 23>; > > + > > + multi-led@1 { > > + #address-cells = <1>; > > + #size-cells = <2>; > > + reg = <0x0 0x1 0x2>; > > This is confusing. Does not match unit address and address/size cells. > Perhaps you wanted three separate regs? The wrong address and size cells and not matching the unit address is a mistake on my part, and the next version will actually pass make dt_binding_check. That said, it's not clear to me how best to handle a combination of multi-leds and individual LEDs on a particular board. For example, a particular board with this driver might have the first six outputs connected to two RGB LEDs, and then the remainder of the outputs connected to individual LEDs. My (poor) attempt at handling this resulted in this approach where I (ab)used the 'reg' property to be able to address each individual LED of a multi-led. I'm sure this problem has been solved before, but I'm struggling finding a driver in the tree that has solved it. Any advice or pointers will be welcome, and in the mean time I'll plan on fixing the (now obvious) issues with the binding. At the very least, cleaning up the binding will make the problem I'm trying to solve more clear. > > + color = <LED_COLOR_ID_RGB>; > > + label = "RGB_LED1"; > > + > > + led-0 { > > + color = <LED_COLOR_ID_RED>; > > + }; > > + > > + led-1 { > > + color = <LED_COLOR_ID_GREEN>; > > + }; > > + > > + led-2 { > > + color = <LED_COLOR_ID_BLUE>; > > + }; > > + > > + }; > > + multi-led@2 { > > + #address-cells = <1>; > > + #size-cells = <3>; > > + reg = <0x3 0x4 0x5 0x6>; > > The same Yep, will fix > > > + color = <LED_COLOR_ID_RGB>; > > + label = "RGBW_LED1"; > > Why labels are upper-case? No reason, they won't be in the next version, sorry. > > > + > > + led-4 { > > + color = <LED_COLOR_ID_RED>; > > + }; > > + > > + led-5 { > > + color = <LED_COLOR_ID_GREEN>; > > + }; > > + > > + led-6 { > > + color = <LED_COLOR_ID_BLUE>; > > + }; > > + > > + led-7 { > > + color = <LED_COLOR_ID_WHITE>; > > + }; > > + }; > > + ready_led@3 { > > No underscores in node names. Generic node name, so just led. Understood, I'll fix this and all the other node names. > > > + #address-cells = <1>; > > + #size-cells = <1>; > > + reg = <0x7 0x8>; > > The same problem with reg. Yep, will fix. > > > + label = "READY"; > > + color = <LED_COLOR_ID_MULTI>; > > + > > + led-8 { > > + color = <LED_COLOR_ID_RED>; > > + }; > > + > > + led-9 { > > + color = <LED_COLOR_ID_GREEN>; > > + }; > > + }; > > + connected_led@4 { > > + reg = <0x9>; > > + label = "CONNECTED"; > > + color = <LED_COLOR_ID_BLUE>; > > + }; > > + }; > > + }; > > + > > +... > > > Best regards, > Krzysztof Thanks so much for even looking at this, despite it obviously not being tested- that won't happen again. Thanks, Kyle
