| Message ID | 20230128202622.12676-2-kaehndan@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | DeviceTree Support for USB-HID Devices and CP2112 | expand |
On 28/01/2023 21:26, Danny Kaehn wrote: > This is a USB HID device which includes an I2C controller and 8 GPIO pins. > Thank you for your patch. There is something to discuss/improve. > The binding allows describing the chip's gpio and i2c controller in DT > using the subnodes named "gpio" and "i2c", respectively. This is > intended to be used in configurations where the CP2112 is permanently > connected in hardware. > > Signed-off-by: Danny Kaehn <kaehndan@gmail.com> > --- > .../bindings/hid/silabs,cp2112.yaml | 82 +++++++++++++++++++ There is no "hid" directory, so I think such devices where going to different place, didn't they? > 1 file changed, 82 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hid/silabs,cp2112.yaml > > diff --git a/Documentation/devicetree/bindings/hid/silabs,cp2112.yaml b/Documentation/devicetree/bindings/hid/silabs,cp2112.yaml > new file mode 100644 > index 000000000000..49287927c63f > --- /dev/null > +++ b/Documentation/devicetree/bindings/hid/silabs,cp2112.yaml > @@ -0,0 +1,82 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/hid/silabs,cp2112.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: CP2112 HID USB to SMBus/I2C Bridge > + > +maintainers: > + - Danny Kaehn <kaehndan@gmail.com> > + > +description: > + This is a USB HID device which includes an I2C controller and 8 GPIO pins. s/This is/CP2112 is/ > + While USB devices typically aren't described in DeviceTree, doing so with the > + CP2112 allows use of its i2c and gpio controllers with other DT nodes when > + the chip is expected to be found on a USB port. Drop these three and replace with description of the hardware. > + > +properties: > + compatible: > + const: usb10c4,ea90 So this is an USB device, so I guess they all go to usb? Missing blank line. > + reg: > + maxItems: 1 > + description: The USB port number on the host controller Blank line > + i2c: > + $ref: /schemas/i2c/i2c-controller.yaml# This is not specific enough. What controller is there? Missing unevaluatedProperties: false, anyway. > + gpio: > + $ref: /schemas/gpio/gpio.yaml# Same comments. > + > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/input/input.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + usb1 { > + #address-cells = <1>; > + #size-cells = <0>; Drop, not related. > + > + usb@1 { > + compatible = "usb424,2514"; > + reg = <1>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + device@1 { /* CP2112 I2C Bridge */ > + compatible = "usb10c4,ea90"; > + reg = <1>; > + > + cp2112_i2c0: i2c { Drop unneeded labels. > + #address-cells = <1>; > + #size-cells = <0>; > + /* Child I2C Devices can be described as normal here */ > + temp@48 { > + compatible = "national,lm75"; > + reg = <0x48>; > + }; > + }; > + > + cp2112_gpio0: gpio { > + gpio-controller; > + interrupt-controller; > + #gpio-cells = <2>; > + gpio-line-names = > + "TEST0", > + "TEST1", > + "TEST2", > + "TEST3", > + "TEST4", > + "TEST5", > + "TEST6", > + "TEST7"; > + }; > + }; > + }; > + }; Best regards, Krzysztof
On 29/01/2023 12:05, Krzysztof Kozlowski wrote: > On 28/01/2023 21:26, Danny Kaehn wrote: >> This is a USB HID device which includes an I2C controller and 8 GPIO pins. >> > Thank you for your patch. There is something to discuss/improve. > >> The binding allows describing the chip's gpio and i2c controller in DT >> using the subnodes named "gpio" and "i2c", respectively. This is >> intended to be used in configurations where the CP2112 is permanently >> connected in hardware. >> >> Signed-off-by: Danny Kaehn <kaehndan@gmail.com> >> --- >> .../bindings/hid/silabs,cp2112.yaml | 82 +++++++++++++++++++ > > There is no "hid" directory, so I think such devices where going to > different place, didn't they? > >> 1 file changed, 82 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/hid/silabs,cp2112.yaml >> >> diff --git a/Documentation/devicetree/bindings/hid/silabs,cp2112.yaml b/Documentation/devicetree/bindings/hid/silabs,cp2112.yaml >> new file mode 100644 >> index 000000000000..49287927c63f >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/hid/silabs,cp2112.yaml >> @@ -0,0 +1,82 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/hid/silabs,cp2112.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: CP2112 HID USB to SMBus/I2C Bridge >> + >> +maintainers: >> + - Danny Kaehn <kaehndan@gmail.com> >> + >> +description: >> + This is a USB HID device which includes an I2C controller and 8 GPIO pins. > > s/This is/CP2112 is/ > >> + While USB devices typically aren't described in DeviceTree, doing so with the >> + CP2112 allows use of its i2c and gpio controllers with other DT nodes when >> + the chip is expected to be found on a USB port. > > Drop these three and replace with description of the hardware. > >> + >> +properties: >> + compatible: >> + const: usb10c4,ea90 > > So this is an USB device, so I guess they all go to usb? > > Missing blank line. > >> + reg: >> + maxItems: 1 >> + description: The USB port number on the host controller > > Blank line > >> + i2c: >> + $ref: /schemas/i2c/i2c-controller.yaml# > > This is not specific enough. What controller is there? OK, assuming this is tightly wired (with cp2112 I2C controller), then the compatible could be skipped as it is inferred from parent one. Yet still you need description and unevaluatedProperties. > > Missing unevaluatedProperties: false, anyway. > >> + gpio: >> + $ref: /schemas/gpio/gpio.yaml# > > Same comments. Description, unevaluatedProperties and constraints on properties (line names, reserved ranges, ranges). > >> + >> + >> +required: >> + - compatible >> + - reg >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/input/input.h> >> + #include <dt-bindings/interrupt-controller/irq.h> >> + >> + usb1 { >> + #address-cells = <1>; >> + #size-cells = <0>; > > Drop, not related. > >> + >> + usb@1 { >> + compatible = "usb424,2514"; >> + reg = <1>; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + device@1 { /* CP2112 I2C Bridge */ >> + compatible = "usb10c4,ea90"; >> + reg = <1>; >> + >> + cp2112_i2c0: i2c { > > Drop unneeded labels. > >> + #address-cells = <1>; >> + #size-cells = <0>; >> + /* Child I2C Devices can be described as normal here */ Drop also this comment. Best regards, Krzysztof
Thanks for all of the comments. All feedback is ACK'd and will be added in v2 -- what follows is just commentary on some comments. On Sun, Jan 29, 2023 at 5:33 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 29/01/2023 12:05, Krzysztof Kozlowski wrote: > > On 28/01/2023 21:26, Danny Kaehn wrote: > >> This is a USB HID device which includes an I2C controller and 8 GPIO pins. > >> > > Thank you for your patch. There is something to discuss/improve. > > > >> The binding allows describing the chip's gpio and i2c controller in DT > >> using the subnodes named "gpio" and "i2c", respectively. This is > >> intended to be used in configurations where the CP2112 is permanently > >> connected in hardware. > >> > >> Signed-off-by: Danny Kaehn <kaehndan@gmail.com> > >> --- > >> .../bindings/hid/silabs,cp2112.yaml | 82 +++++++++++++++++++ > > > > There is no "hid" directory, so I think such devices where going to > > different place, didn't they? Good point, I didn't notice other hid-related bindings went into input/ -- will change > > > >> + While USB devices typically aren't described in DeviceTree, doing so with the > >> + CP2112 allows use of its i2c and gpio controllers with other DT nodes when > >> + the chip is expected to be found on a USB port. > > > > Drop these three and replace with description of the hardware. Understood. I noticed that a similar usb-based binding included a similar description (net/marvell,mvusb.yaml) but I understand why we would not want this in new bindings. > > > >> + i2c: > >> + $ref: /schemas/i2c/i2c-controller.yaml# > > > > This is not specific enough. What controller is there? > > OK, assuming this is tightly wired (with cp2112 I2C controller), then > the compatible could be skipped as it is inferred from parent one. Yet > still you need description and unevaluatedProperties. > Great point, will update -- I didn't quite understand that child nodes of the root could have properties/unevaluatedProperties/etc.. but I see now that that is well-documented (just not often done in existing bindings)! > > > > Missing unevaluatedProperties: false, anyway. > > > >> + gpio: > >> + $ref: /schemas/gpio/gpio.yaml# > > > > Same comments. > > Description, unevaluatedProperties and constraints on properties (line > names, reserved ranges, ranges). > Will add. Thanks, Danny Kaehn
diff --git a/Documentation/devicetree/bindings/hid/silabs,cp2112.yaml b/Documentation/devicetree/bindings/hid/silabs,cp2112.yaml new file mode 100644 index 000000000000..49287927c63f --- /dev/null +++ b/Documentation/devicetree/bindings/hid/silabs,cp2112.yaml @@ -0,0 +1,82 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/hid/silabs,cp2112.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: CP2112 HID USB to SMBus/I2C Bridge + +maintainers: + - Danny Kaehn <kaehndan@gmail.com> + +description: + This is a USB HID device which includes an I2C controller and 8 GPIO pins. + While USB devices typically aren't described in DeviceTree, doing so with the + CP2112 allows use of its i2c and gpio controllers with other DT nodes when + the chip is expected to be found on a USB port. + +properties: + compatible: + const: usb10c4,ea90 + reg: + maxItems: 1 + description: The USB port number on the host controller + i2c: + $ref: /schemas/i2c/i2c-controller.yaml# + gpio: + $ref: /schemas/gpio/gpio.yaml# + + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + #include <dt-bindings/input/input.h> + #include <dt-bindings/interrupt-controller/irq.h> + + usb1 { + #address-cells = <1>; + #size-cells = <0>; + + usb@1 { + compatible = "usb424,2514"; + reg = <1>; + + #address-cells = <1>; + #size-cells = <0>; + + device@1 { /* CP2112 I2C Bridge */ + compatible = "usb10c4,ea90"; + reg = <1>; + + cp2112_i2c0: i2c { + #address-cells = <1>; + #size-cells = <0>; + /* Child I2C Devices can be described as normal here */ + temp@48 { + compatible = "national,lm75"; + reg = <0x48>; + }; + }; + + cp2112_gpio0: gpio { + gpio-controller; + interrupt-controller; + #gpio-cells = <2>; + gpio-line-names = + "TEST0", + "TEST1", + "TEST2", + "TEST3", + "TEST4", + "TEST5", + "TEST6", + "TEST7"; + }; + }; + }; + };
This is a USB HID device which includes an I2C controller and 8 GPIO pins. The binding allows describing the chip's gpio and i2c controller in DT using the subnodes named "gpio" and "i2c", respectively. This is intended to be used in configurations where the CP2112 is permanently connected in hardware. Signed-off-by: Danny Kaehn <kaehndan@gmail.com> --- .../bindings/hid/silabs,cp2112.yaml | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 Documentation/devicetree/bindings/hid/silabs,cp2112.yaml