| Message ID | 20220513190409.3682501-2-kyle.swenson@est.tech |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] leds: aw21024: Add support for Awinic's AW21024 | expand |
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@est.tech> > --- > .../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@est.tech> > + > +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. > + > + enable-gpios: > + maxItems: 1 > + description: GPIO pin to enable/disable the device. Skip description, it's obvious. > + > + '#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. > + 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. > +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. > + 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? > + 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 > + color = <LED_COLOR_ID_RGB>; > + label = "RGBW_LED1"; Why labels are upper-case? > + > + 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. > + #address-cells = <1>; > + #size-cells = <1>; > + reg = <0x7 0x8>; The same problem with reg. > + 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
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@est.tech> > > --- > > .../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@est.tech> > > + > > +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
On 17/05/2022 20:31, Kyle Swenson wrote: >>> + >>> + 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. The immediate solution to the DTS reg issue is to use the same unit address, so: multi-led@0 { reg = <0x0>, <0x1>, <0x2>; } However your case is partially (or entirely) covered by multicolor LEDs. You should add allOf:$ref with reference to leds-class-multicolor.yaml. I see exactly your pattern being used there - just the fixed one, I think. I'll send a patch for it and put you on Cc. Best regards, Krzysztof
On Wed, May 18, 2022 at 10:17:21AM +0200, Krzysztof Kozlowski wrote: > On 17/05/2022 20:31, Kyle Swenson wrote: > >>> + > >>> + 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. > > The immediate solution to the DTS reg issue is to use the same unit > address, so: > > multi-led@0 { > reg = <0x0>, <0x1>, <0x2>; > } > > However your case is partially (or entirely) covered by multicolor LEDs. > You should add allOf:$ref with reference to leds-class-multicolor.yaml. > I see exactly your pattern being used there - just the fixed one, I > think. I'll send a patch for it and put you on Cc. I suspect you're right: mutlicolor LEDs will do exactly what I need and the patch you cc'd me on teaches me how to specify it in the DTS. I'll make the changes and send up a v2 in a few days. > > Best regards, > Krzysztof Thanks again for your time and guidance. I happen to have a board with that lp50xx LED controller and I'll be happy to test out the example DTS from the binding if you'd like. Thanks, Kyle
On 18/05/2022 23:18, Kyle Swenson wrote: > Thanks again for your time and guidance. I happen to have a board with that > lp50xx LED controller and I'll be happy to test out the example DTS from the > binding if you'd like. Sure, that would be good. The DTS using that lp50xx LED follows different concept, so maybe the binding example is not correct. Best regards, Krzysztof
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@est.tech> + +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 + + enable-gpios: + maxItems: 1 + description: GPIO pin to enable/disable the device. + + '#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)": + 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 + + +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>; + enable-gpios = <&gpio1 23>; + + multi-led@1 { + #address-cells = <1>; + #size-cells = <2>; + reg = <0x0 0x1 0x2>; + 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>; + color = <LED_COLOR_ID_RGB>; + label = "RGBW_LED1"; + + 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 { + #address-cells = <1>; + #size-cells = <1>; + reg = <0x7 0x8>; + 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>; + }; + }; + }; + +...
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@est.tech> --- .../bindings/leds/leds-aw21024.yaml | 157 ++++++++++++++++++ 1 file changed, 157 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-aw21024.yaml