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

[PATCH v4 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.

Changes in v4:
 - Moved silabs,cp2112.yaml to /Documentation/devicetree/bindings/i2c

Changes in v3:
 - Additional fixups to silabs,cp2112.yaml to address comments

Changes in v2:
 - Added more detail to silabs,cp2112.yaml dt-binding
 - Moved silabs,cp2112.yaml to /Documentation/devicetree/bindings/input
 - Added support for setting smbus clock-frequency from DT in hid-cp2112.c
 - Added freeing of of_nodes on error paths of _probe in hid-cp2112.c

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

 .../bindings/i2c/silabs,cp2112.yaml           | 112 ++++++++++++++++++
 drivers/hid/hid-cp2112.c                      |  23 +++-
 drivers/hid/usbhid/hid-core.c                 |   2 +
 3 files changed, 135 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml

-- 
2.25.1

[PATCH v4 1/4] dt-bindings: i2c: 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/i2c/silabs,cp2112.yaml           | 112 ++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml

diff --git a/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
new file mode 100644
index 000000000000..286e4dbafd69
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
@@ -0,0 +1,112 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/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:
+  The CP2112 is a USB HID device which includes an integrated I2C controller
+  and 8 GPIO pins. Its GPIO pins can each be configured as inputs, open-drain
+  outputs, or push-pull outputs.
+
+properties:
+  compatible:
+    const: usb10c4,ea90
+
+  reg:
+    maxItems: 1
+    description: The USB port number on the host controller
+
+  i2c:
+    description: The SMBus/I2C controller node for the CP2112
+    $ref: /schemas/i2c/i2c-controller.yaml#
+    unevaluatedProperties: false
+    properties:
+      clock-frequency:
+        minimum: 10000
+        default: 100000
+        maximum: 400000
+
+  gpio:
+    description: The GPIO controller node for the CP2112
+    type: object
+    properties:
+      interrupt-controller: true
+      "#interrupt-cells":
+        const: 2
+
+      gpio-controller: true
+      "#gpio-cells":
+        const: 2
+
+      ngpios:
+        const: 8
+
+      gpio-line-names:
+        minItems: 1
+        maxItems: 8
+
+    patternProperties:
+      "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
+        type: object
+        properties:
+          gpio-hog: true
+          input: true
+          output-high: true
+          output-low: true
+          line-name: true
+          gpios:
+            minItems: 1
+            maxItems: 8
+
+        required:
+          - gpio-hog
+          - gpios
+
+        additionalProperties: false
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/input/input.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    usb {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      device@1 {
+        compatible = "usb10c4,ea90";
+        reg = <1>;
+
+        i2c {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          temp@48 {
+            compatible = "national,lm75";
+            reg = <0x48>;
+          };
+        };
+
+        gpio {
+          gpio-controller;
+          interrupt-controller;
+          #gpio-cells = <2>;
+          gpio-line-names = "TEST0", "TEST1", "TEST2",
+            "TEST3", "TEST4", "TEST5", "TEST6", "TEST7";
+        };
+      };
+    };
-- 
2.25.1

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

[Krzysztof Kozlowski]

On 06/02/2023 14:50, Danny Kaehn wrote:
> 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>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Rob Herring]

On Mon, Feb 06, 2023 at 07:50:13AM -0600, Danny Kaehn wrote:
> 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.

My comments on v3 still apply. Please slow down your pace of sending new 
versions so folks have change to review.

Rob

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

[Daniel Kaehn]

On Tue, Feb 7, 2023 at 12:50 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Feb 06, 2023 at 07:50:13AM -0600, Danny Kaehn wrote:
> > 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.
>
> My comments on v3 still apply. Please slow down your pace of sending new
> versions so folks have change to review.
>
> Rob

Thanks for the correction -- I definitely see how that could be
frustrating / problematic. (until now I'd thought the ideal case was
that comments were addressed as soon as possible, so that folks
wouldn't be reviewing now-obsolete code that might change due to other
comments anyways)

Thanks,

Danny Kaehn

[PATCH v4 2/4] HID: usbhid: 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 v4 2/4] HID: usbhid: Share USB device devicetree node with child HID device

[Dmitry Torokhov]

Hi Danny,

On Mon, Feb 06, 2023 at 07:50:14AM -0600, 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).

Why do we do that only for OF? Can we use device_set_node() instead?

> 
> 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
> 

Thanks.

-- 
Dmitry

[PATCH v4 3/4] HID: cp2112: Fix 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

Re: [PATCH v4 3/4] HID: cp2112: Fix driver not registering GPIO IRQ chip as threaded

[Dmitry Torokhov]

On Mon, Feb 06, 2023 at 07:50:15AM -0600, Danny Kaehn wrote:
> 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.

This looks like a bugfix not dependent on anything else in the series
and can be applied separately...

> 
> 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
> 

-- 
Dmitry

Re: [PATCH v4 3/4] HID: cp2112: Fix driver not registering GPIO IRQ chip as threaded

[Daniel Kaehn]

On Mon, Feb 6, 2023 at 5:15 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Mon, Feb 06, 2023 at 07:50:15AM -0600, Danny Kaehn wrote:
> > 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.
>
> This looks like a bugfix not dependent on anything else in the series
> and can be applied separately...

This is correct (though usage of this patchset to instantiate drivers
which request interrupts will most of the time be broken without this
patch). Does this mean I should submit this patch independently from
the rest of the series? Or should I just include a message to the
maintainer describing what you said (that this can be applied
separately)?

Thanks,

Danny Kaehn

>
> >
> > 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
> >
>
> --
> Dmitry

Re: [PATCH v4 3/4] HID: cp2112: Fix driver not registering GPIO IRQ chip as threaded

[Dmitry Torokhov]

On Tue, Feb 07, 2023 at 06:34:32AM -0600, Daniel Kaehn wrote:
> On Mon, Feb 6, 2023 at 5:15 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Mon, Feb 06, 2023 at 07:50:15AM -0600, Danny Kaehn wrote:
> > > 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.
> >
> > This looks like a bugfix not dependent on anything else in the series
> > and can be applied separately...
> 
> This is correct (though usage of this patchset to instantiate drivers
> which request interrupts will most of the time be broken without this
> patch). Does this mean I should submit this patch independently from
> the rest of the series? Or should I just include a message to the
> maintainer describing what you said (that this can be applied
> separately)?

I'd simply sent it separately and then you do not need to explain ;)

Thanks.

-- 
Dmitry

[PATCH v4 4/4] HID: 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. Additionally, support configuring the
i2c bus speed from the clock-frequency property.

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

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 27cadadda7c9..aa634accdfb0 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1234,6 +1234,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	u8 buf[3];
 	struct cp2112_smbus_config_report config;
 	struct gpio_irq_chip *girq;
+	struct i2c_timings timings;
 	int ret;
 
 	dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
@@ -1292,6 +1293,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto err_power_normal;
 	}
 
+	dev->adap.dev.of_node   = of_get_child_by_name(hdev->dev.of_node, "i2c");
+	i2c_parse_fw_timings(&dev->adap.dev, &timings, true);
+
+	config.clock_speed = cpu_to_be32(timings.bus_freq_hz);
 	config.retry_time = cpu_to_be16(1);
 
 	ret = cp2112_hid_output(hdev, (u8 *)&config, sizeof(config),
@@ -1300,7 +1305,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		hid_err(hdev, "error setting SMBus config\n");
 		if (ret >= 0)
 			ret = -EIO;
-		goto err_power_normal;
+		goto err_free_i2c_of;
 	}
 
 	hid_set_drvdata(hdev, (void *)dev);
@@ -1322,7 +1327,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	if (ret) {
 		hid_err(hdev, "error registering i2c adapter\n");
-		goto err_power_normal;
+		goto err_free_i2c_of;
 	}
 
 	hid_dbg(hdev, "adapter registered\n");
@@ -1336,6 +1341,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 IS_ENABLED(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;
@@ -1376,7 +1384,12 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 err_gpiochip_remove:
 	gpiochip_remove(&dev->gc);
 err_free_i2c:
+#if IS_ENABLED(CONFIG_OF_GPIO)
+	of_node_put(dev->gc.of_node);
+#endif
 	i2c_del_adapter(&dev->adap);
+err_free_i2c_of:
+	of_node_put(dev->adap.dev.of_node);
 err_power_normal:
 	hid_hw_power(hdev, PM_HINT_NORMAL);
 err_hid_close:
@@ -1391,6 +1404,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 IS_ENABLED(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 v4 4/4] HID: cp2112: Devicetree Support

[Dmitry Torokhov]

On Mon, Feb 06, 2023 at 07:50:16AM -0600, 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. Additionally, support configuring the
> i2c bus speed from the clock-frequency property.
> 
> Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
> ---
>  drivers/hid/hid-cp2112.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 27cadadda7c9..aa634accdfb0 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -1234,6 +1234,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	u8 buf[3];
>  	struct cp2112_smbus_config_report config;
>  	struct gpio_irq_chip *girq;
> +	struct i2c_timings timings;
>  	int ret;
>  
>  	dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
> @@ -1292,6 +1293,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		goto err_power_normal;
>  	}
>  
> +	dev->adap.dev.of_node   = of_get_child_by_name(hdev->dev.of_node, "i2c");
> +	i2c_parse_fw_timings(&dev->adap.dev, &timings, true);
> +
> +	config.clock_speed = cpu_to_be32(timings.bus_freq_hz);
>  	config.retry_time = cpu_to_be16(1);
>  
>  	ret = cp2112_hid_output(hdev, (u8 *)&config, sizeof(config),
> @@ -1300,7 +1305,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		hid_err(hdev, "error setting SMBus config\n");
>  		if (ret >= 0)
>  			ret = -EIO;
> -		goto err_power_normal;
> +		goto err_free_i2c_of;
>  	}
>  
>  	hid_set_drvdata(hdev, (void *)dev);
> @@ -1322,7 +1327,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  
>  	if (ret) {
>  		hid_err(hdev, "error registering i2c adapter\n");
> -		goto err_power_normal;
> +		goto err_free_i2c_of;
>  	}
>  
>  	hid_dbg(hdev, "adapter registered\n");
> @@ -1336,6 +1341,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 IS_ENABLED(CONFIG_OF_GPIO)
> +	dev->gc.of_node			= of_get_child_by_name(hdev->dev.of_node, "gpio");


I believe Andy is actively trying to get rid of of_node from GPIO chips.
And in general, we should be using fwnode and generic device properties
as much as possible.

> +#endif
>  
>  	dev->irq.name = "cp2112-gpio";
>  	dev->irq.irq_startup = cp2112_gpio_irq_startup;
> @@ -1376,7 +1384,12 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  err_gpiochip_remove:
>  	gpiochip_remove(&dev->gc);
>  err_free_i2c:
> +#if IS_ENABLED(CONFIG_OF_GPIO)
> +	of_node_put(dev->gc.of_node);
> +#endif
>  	i2c_del_adapter(&dev->adap);
> +err_free_i2c_of:
> +	of_node_put(dev->adap.dev.of_node);
>  err_power_normal:
>  	hid_hw_power(hdev, PM_HINT_NORMAL);
>  err_hid_close:
> @@ -1391,6 +1404,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 IS_ENABLED(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
> 

Thanks.

-- 
Dmitry

Re: [PATCH v4 4/4] HID: cp2112: Devicetree Support

[Andy Shevchenko]

On Mon, Feb 06, 2023 at 03:18:26PM -0800, Dmitry Torokhov wrote:
> On Mon, Feb 06, 2023 at 07:50:16AM -0600, Danny Kaehn wrote:

...

> > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > +	dev->gc.of_node			= of_get_child_by_name(hdev->dev.of_node, "gpio");
> 
> 
> I believe Andy is actively trying to get rid of of_node from GPIO chips.
> And in general, we should be using fwnode and generic device properties
> as much as possible.
> 
> > +#endif

Correct. And looking into the code of this patch I don't see any obstacles
to use fwnode APIs. You can Cc a v5 (which is supposed to be fwnode API based)
to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/4] HID: cp2112: Devicetree Support

[Daniel Kaehn]

On Tue, Feb 7, 2023 at 4:15 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Feb 06, 2023 at 03:18:26PM -0800, Dmitry Torokhov wrote:
> > On Mon, Feb 06, 2023 at 07:50:16AM -0600, Danny Kaehn wrote:
>
> ...
>
> > > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > > +   dev->gc.of_node                 = of_get_child_by_name(hdev->dev.of_node, "gpio");
> >
> >
> > I believe Andy is actively trying to get rid of of_node from GPIO chips.
> > And in general, we should be using fwnode and generic device properties
> > as much as possible.
> >
> > > +#endif
>
> Correct. And looking into the code of this patch I don't see any obstacles
> to use fwnode APIs. You can Cc a v5 (which is supposed to be fwnode API based)
> to me.
>

Sounds great, will do. I looked into doing this with the fwnode
initially, but thought since the capability to describe usb devices in
ACPI doesn't seem to be there, that I should be explicit that this
only works for devicetree--but makes sense that it's better to be
generic at the driver level if possible (especially if of_node is
being removed from gpio chips), so will do!

Thanks,

Danny Kaehn