[PATCH 0/4] DeviceTree Support for USB-HID Devices and CP2112

[PATCH 0/4] DeviceTree Support for USB-HID Devices and CP2112

[Danny Kaehn]

This patchset allows USB-HID devices to have DeviceTree bindings through sharing
the USB of_node with the HID driver, and adds such a binding and driver
implementation for the CP2112 USB to SMBus Bridge (which necessitated the
USB-HID change). This change allows a CP2112 permanently attached in hardware to
be described in DT and interoperate with other drivers, and exposed the threaded
interrupt bug fixed in patch 0003.

Plese correct if the assumption made that there is a 1:1 correlation between
a USB device and its HID device is not always true. If so, patch 0002 would
then need to be reworked.


Danny Kaehn (4):
  dt-bindings: hid: Add CP2112 HID USB to SMBus Bridge
  Share USB device devicetree node with child HID device
  Fix CP2112 driver not registering GPIO IRQ chip as threaded
  CP2112 Devicetree Support

 .../bindings/hid/silabs,cp2112.yaml           | 82 +++++++++++++++++++
 drivers/hid/hid-cp2112.c                      | 10 +++
 drivers/hid/usbhid/hid-core.c                 |  2 +
 3 files changed, 94 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hid/silabs,cp2112.yaml

-- 
2.25.1

[PATCH 1/4] dt-bindings: hid: Add CP2112 HID USB to SMBus Bridge

[Danny Kaehn]

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

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";
+          };
+        };
+      };
+    };
-- 
2.25.1

Re: [PATCH 1/4] dt-bindings: hid: Add CP2112 HID USB to SMBus Bridge

[Krzysztof Kozlowski]

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

Re: [PATCH 1/4] dt-bindings: hid: Add CP2112 HID USB to SMBus Bridge

[Krzysztof Kozlowski]

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

Re: [PATCH 1/4] dt-bindings: hid: Add CP2112 HID USB to SMBus Bridge

[Daniel Kaehn]

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

[PATCH 2/4] Share USB device devicetree node with child HID device

[Danny Kaehn]

USB HID core now shares its devicetree of_node with its child HID device.
Since there can only be one HID device on a USB interface, it is redundant
to specify a hid node under the USB device (and further, binding this way
isn't currently possible, as hid_driver does not support of_match_table).

Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
---
 drivers/hid/usbhid/hid-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index be4c731aaa65..b6c968af258f 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -33,6 +33,7 @@
 #include <linux/hiddev.h>
 #include <linux/hid-debug.h>
 #include <linux/hidraw.h>
+#include <linux/device.h>
 #include "usbhid.h"
 
 /*
@@ -1369,6 +1370,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
 	hid->hiddev_report_event = hiddev_report_event;
 #endif
 	hid->dev.parent = &intf->dev;
+	device_set_of_node_from_dev(&hid->dev, &intf->dev);
 	hid->bus = BUS_USB;
 	hid->vendor = le16_to_cpu(dev->descriptor.idVendor);
 	hid->product = le16_to_cpu(dev->descriptor.idProduct);
-- 
2.25.1

Re: [PATCH 2/4] Share USB device devicetree node with child HID device

[Krzysztof Kozlowski]

On 28/01/2023 21:26, Danny Kaehn wrote:
> USB HID core now shares its devicetree of_node with its child HID device.
> Since there can only be one HID device on a USB interface, it is redundant
> to specify a hid node under the USB device (and further, binding this way
> isn't currently possible, as hid_driver does not support of_match_table).
> 

Here and in other patches:

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

Best regards,
Krzysztof

[PATCH 3/4] Fix CP2112 driver not registering GPIO IRQ chip as threaded

[Danny Kaehn]

The CP2112 generates interrupts from a polling routine on a thread,
and can only support threaded interrupts. This patch configures the
gpiochip irq chip with this flag, disallowing consumers to request
a hard IRQ from this driver, which resulted in a segfault previously.

Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
---
 drivers/hid/hid-cp2112.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 1e16b0fa310d..27cadadda7c9 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1354,6 +1354,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	girq->parents = NULL;
 	girq->default_type = IRQ_TYPE_NONE;
 	girq->handler = handle_simple_irq;
+	girq->threaded = true;
 
 	ret = gpiochip_add_data(&dev->gc, dev);
 	if (ret < 0) {
-- 
2.25.1

[PATCH 4/4] CP2112 Devicetree Support

[Danny Kaehn]

Bind i2c and gpio interfaces to subnodes with names
"i2c" and "gpio" if they exist, respectively. This
allows the gpio and i2c controllers to be described
in DT as usual.

Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
---
 drivers/hid/hid-cp2112.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 27cadadda7c9..99e8043e1c34 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1310,6 +1310,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	dev->adap.algo		= &smbus_algorithm;
 	dev->adap.algo_data	= dev;
 	dev->adap.dev.parent	= &hdev->dev;
+	dev->adap.dev.of_node   = of_get_child_by_name(hdev->dev.of_node, "i2c");
 	snprintf(dev->adap.name, sizeof(dev->adap.name),
 		 "CP2112 SMBus Bridge on hidraw%d",
 		 ((struct hidraw *)hdev->hidraw)->minor);
@@ -1336,6 +1337,9 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	dev->gc.ngpio			= 8;
 	dev->gc.can_sleep		= 1;
 	dev->gc.parent			= &hdev->dev;
+#if defined(CONFIG_OF_GPIO)
+	dev->gc.of_node			= of_get_child_by_name(hdev->dev.of_node, "gpio");
+#endif
 
 	dev->irq.name = "cp2112-gpio";
 	dev->irq.irq_startup = cp2112_gpio_irq_startup;
@@ -1391,6 +1395,11 @@ static void cp2112_remove(struct hid_device *hdev)
 	struct cp2112_device *dev = hid_get_drvdata(hdev);
 	int i;
 
+	of_node_put(dev->adap.dev.of_node);
+#if defined(CONFIG_OF_GPIO)
+	of_node_put(dev->gc.of_node);
+#endif
+
 	sysfs_remove_group(&hdev->dev.kobj, &cp2112_attr_group);
 	i2c_del_adapter(&dev->adap);
 
-- 
2.25.1

Re: [PATCH 4/4] CP2112 Devicetree Support

[Krzysztof Kozlowski]

On 28/01/2023 21:26, Danny Kaehn wrote:
> Bind i2c and gpio interfaces to subnodes with names
> "i2c" and "gpio" if they exist, respectively. This
> allows the gpio and i2c controllers to be described
> in DT as usual.
> 
> Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
> ---
>  drivers/hid/hid-cp2112.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 27cadadda7c9..99e8043e1c34 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -1310,6 +1310,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	dev->adap.algo		= &smbus_algorithm;
>  	dev->adap.algo_data	= dev;
>  	dev->adap.dev.parent	= &hdev->dev;
> +	dev->adap.dev.of_node   = of_get_child_by_name(hdev->dev.of_node, "i2c");
>  	snprintf(dev->adap.name, sizeof(dev->adap.name),
>  		 "CP2112 SMBus Bridge on hidraw%d",
>  		 ((struct hidraw *)hdev->hidraw)->minor);
> @@ -1336,6 +1337,9 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	dev->gc.ngpio			= 8;
>  	dev->gc.can_sleep		= 1;
>  	dev->gc.parent			= &hdev->dev;
> +#if defined(CONFIG_OF_GPIO)

Don't use #if, but IS_ENABLED(). I think it should work here.

> +	dev->gc.of_node			= of_get_child_by_name(hdev->dev.of_node, "gpio");

You leak it now on error paths.

Best regards,
Krzysztof

Re: [PATCH 4/4] CP2112 Devicetree Support

[Daniel Kaehn]

On Sun, Jan 29, 2023 at 5:06 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 28/01/2023 21:26, Danny Kaehn wrote:
> > +#if defined(CONFIG_OF_GPIO)
>
> Don't use #if, but IS_ENABLED(). I think it should work here.

I think I will still need to use an #if / some sort of preprocessor directive,
since of_node is only a member of the gpio_chip struct if that is enabled
(and thus causes a compile error if done outside of the preprocessor)...
Unless I'm misinterpreting your comment?

>
> > +     dev->gc.of_node                 = of_get_child_by_name(hdev->dev.of_node, "gpio");
>
> You leak it now on error paths.

Ah, good point. Will fix!

Thanks,

Danny Kaehn

Re: [PATCH 4/4] CP2112 Devicetree Support

[Krzysztof Kozlowski]

On 31/01/2023 02:06, Daniel Kaehn wrote:
> On Sun, Jan 29, 2023 at 5:06 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 28/01/2023 21:26, Danny Kaehn wrote:
>>> +#if defined(CONFIG_OF_GPIO)
>>
>> Don't use #if, but IS_ENABLED(). I think it should work here.
> 
> I think I will still need to use an #if / some sort of preprocessor directive,
> since of_node is only a member of the gpio_chip struct if that is enabled
> (and thus causes a compile error if done outside of the preprocessor)...
> Unless I'm misinterpreting your comment?

If of_node in gpio_chip is indeed hidden by #ifdef, then the code is ok.

Best regards,
Krzysztof

Re: [PATCH 0/4] DeviceTree Support for USB-HID Devices and CP2112

[Benjamin Tissoires]

Hi Danny,

On Sat, Jan 28, 2023 at 9:26 PM Danny Kaehn <kaehndan@gmail.com> wrote:
>
> This patchset allows USB-HID devices to have DeviceTree bindings through sharing
> the USB of_node with the HID driver, and adds such a binding and driver
> implementation for the CP2112 USB to SMBus Bridge (which necessitated the
> USB-HID change). This change allows a CP2112 permanently attached in hardware to
> be described in DT and interoperate with other drivers, and exposed the threaded
> interrupt bug fixed in patch 0003.

That series is very interesting. I always wondered how I could declare
an I2C device attached to the CP2112 over USB. Ideally if you can make
this compatible with ACPI SSDT, that would be even better :) (one can
always dream).

>
> Plese correct if the assumption made that there is a 1:1 correlation between
> a USB device and its HID device is not always true. If so, patch 0002 would
> then need to be reworked.

I am not sure I understand patch 2 completely, but if your assumption
is that each struct usb_interface can have at most one hid device, it
seems that it is the case. However, nothing prevents another hid
driver to add one more hid device on top of that USB dev. For
instance, hid-logitech-dj does that: when it enumerates the devices
connected to the wireless receiver, it creates matching HID devices
with the parent being the current HID dev.

AFAICT, we already have DT enumeration for i2c-hid devices, so
probably your solution is correct. Though the DT enumeration in
i2c-hid-of.c relies on .of_match_table, which seems a little bit more
integrated than this series (but I don't know enough of DT
unfortunately).

So I personally won't push against that series, but I'd still like to
have a rough idea on what is missing in patch 2 if we consider that
your assumption might not always be the case.

Maybe (just random brain fart) we could have a separate usbhid-of.c in
the usbhid subdir that builds up the same OF matching that
i2c-hid-of.c is doing?

Cheers,
Benjamin

>
>
> Danny Kaehn (4):
>   dt-bindings: hid: Add CP2112 HID USB to SMBus Bridge
>   Share USB device devicetree node with child HID device
>   Fix CP2112 driver not registering GPIO IRQ chip as threaded
>   CP2112 Devicetree Support
>
>  .../bindings/hid/silabs,cp2112.yaml           | 82 +++++++++++++++++++
>  drivers/hid/hid-cp2112.c                      | 10 +++
>  drivers/hid/usbhid/hid-core.c                 |  2 +
>  3 files changed, 94 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hid/silabs,cp2112.yaml
>
> --
> 2.25.1
>