[PATCH v7 0/3] Firmware Support for USB-HID Devices and CP2112

[PATCH v7 0/3] Firmware Support for USB-HID Devices and CP2112

[Danny Kaehn]

This patchset allows USB-HID devices to have DeviceTree bindings through sharing
the USB fwnode 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.

Changes in v7:
- Use dev_fwnode when calling fwnod_handle_put in i2c_adapter in hid-cp2112.c
- Capitalize I2C and GPIO in commit message for patch 0003

Changes in v6:
- Fix fwnode_handle reference leaks in hid-cp21112.c
- Simplify hog node pattern in silabs,cp2112.yaml

Changes in v5:
 - Use fwnode API instead of of_node api in hid-core.c and hid-cp2112.c
 - Include sda-gpios and scl-gpios in silabs,cp2112.yaml
 - Additional fixups to silabs,cp2112.yaml to address comments
 - Submit threaded interrupt bugfix separately from this patchset, as requested

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 (3):
  dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge
  HID: usbhid: Share USB device firmware node with child HID device
  HID: cp2112: Fwnode Support

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

-- 
2.25.1

[PATCH v7 1/3] 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           | 113 ++++++++++++++++++
 1 file changed, 113 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..a27509627804
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
@@ -0,0 +1,113 @@
+# 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:
+      sda-gpios:
+        maxItems: 1
+
+      scl-gpios:
+        maxItems: 1
+
+      clock-frequency:
+        minimum: 10000
+        default: 100000
+        maximum: 400000
+
+  gpio:
+    description: The GPIO controller node for the CP2112
+    type: object
+    unevaluatedProperties: false
+
+    properties:
+      interrupt-controller: true
+      "#interrupt-cells":
+        const: 2
+
+      gpio-controller: true
+      "#gpio-cells":
+        const: 2
+
+      gpio-line-names:
+        minItems: 1
+        maxItems: 8
+
+    patternProperties:
+      "-hog(-[0-9]+)?$":
+        type: object
+
+        required:
+          - gpio-hog
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    usb {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      device@1 {
+        compatible = "usb10c4,ea90";
+        reg = <1>;
+
+        i2c {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          sda-gpios = <&cp2112_gpio 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+          scl-gpios = <&cp2112_gpio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+
+          temp@48 {
+            compatible = "national,lm75";
+            reg = <0x48>;
+          };
+        };
+
+        cp2112_gpio: gpio {
+          gpio-controller;
+          interrupt-controller;
+          #gpio-cells = <2>;
+          gpio-line-names = "CP2112_SDA", "CP2112_SCL", "TEST2",
+            "TEST3","TEST4", "TEST5", "TEST6";
+
+          fan-rst-hog {
+              gpio-hog;
+              gpios = <7 GPIO_ACTIVE_HIGH>;
+              output-high;
+              line-name = "FAN_RST";
+          };
+        };
+      };
+    };
-- 
2.25.1

[PATCH v7 2/3] HID: usbhid: Share USB device firmware node with child HID device

[Danny Kaehn]

USB HID core now shares its fwnode 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. This allows usb HID device
drivers to be described in firmware and make use of device properties.

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 257dd73e37bf..090260d99c84 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -19,6 +19,7 @@
 #include <linux/list.h>
 #include <linux/mm.h>
 #include <linux/mutex.h>
+#include <linux/property.h>
 #include <linux/spinlock.h>
 #include <asm/unaligned.h>
 #include <asm/byteorder.h>
@@ -1374,6 +1375,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_node(&hid->dev, dev_fwnode(&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

[PATCH v7 3/3] HID: cp2112: Fwnode 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 firmware as usual. Additionally, support configuring the
I2C bus speed from the clock-frequency device property.

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

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 27cadadda7c9..491e3c83af12 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;
 	}
 
+	device_set_node(&dev->adap.dev, device_get_named_child_node(&hdev->dev, "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,7 @@ 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;
+	dev->gc.fwnode			= device_get_named_child_node(&hdev->dev, "gpio");
 
 	dev->irq.name = "cp2112-gpio";
 	dev->irq.irq_startup = cp2112_gpio_irq_startup;
@@ -1376,7 +1382,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 err_gpiochip_remove:
 	gpiochip_remove(&dev->gc);
 err_free_i2c:
+	fwnode_handle_put(dev->gc.fwnode);
 	i2c_del_adapter(&dev->adap);
+err_free_i2c_of:
+	fwnode_handle_put(dev_fwnode(&dev->adap.dev));
 err_power_normal:
 	hid_hw_power(hdev, PM_HINT_NORMAL);
 err_hid_close:
@@ -1391,6 +1400,8 @@ static void cp2112_remove(struct hid_device *hdev)
 	struct cp2112_device *dev = hid_get_drvdata(hdev);
 	int i;
 
+	fwnode_handle_put(dev->gc.fwnode);
+	fwnode_handle_put(dev_fwnode(&dev->adap.dev));
 	sysfs_remove_group(&hdev->dev.kobj, &cp2112_attr_group);
 	i2c_del_adapter(&dev->adap);
 
-- 
2.25.1

Re: [PATCH v7 0/3] Firmware Support for USB-HID Devices and CP2112

[Andy Shevchenko]

On Thu, Feb 23, 2023 at 03:31:44PM -0600, Danny Kaehn wrote:
> This patchset allows USB-HID devices to have DeviceTree bindings through sharing
> the USB fwnode 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.

It's your responsibility to carry the tags you have got in the previous rounds
of the review. Please, be respectful to the reviewers who spent a lot of time
on yours, in particular, code. Otherwise why to bother with it (upstreaming)
at all?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 0/3] Firmware Support for USB-HID Devices and CP2112

[Daniel Kaehn]

On Fri, Feb 24, 2023 at 10:43 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Feb 23, 2023 at 03:31:44PM -0600, Danny Kaehn wrote:
> > This patchset allows USB-HID devices to have DeviceTree bindings through sharing
> > the USB fwnode 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.
>
> It's your responsibility to carry the tags you have got in the previous rounds
> of the review. Please, be respectful to the reviewers who spent a lot of time
> on yours, in particular, code. Otherwise why to bother with it (upstreaming)
> at all?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Hello Andy,

My sincerest apologies on this! I wasn't actually aware that this is
something I could do / am responsible for doing. No disrespect is
intended, though I see how this would be frustrating for reviewers (I
previously thought that maintainers used some sort of automated tool
to keep track of who approved/acked what in previous versions, but
didn't really know how that would work).

If I'm understanding correctly, this means that whenever someone
responds to my patch with a "Reviewed-by", etc.. I should be adding
that tag to the end of the commit message of that patch if a future
revision is needed? I assume this only applies on future revisions
where patches other than the one initially reviewed are changed, and
that any tags I take with should be dropped if that patch is changed?
Apologies about these questions - - I looked for guidance on this in
the various "submitting patches to the kernel" guides out there, and
wasn't able to find much.

Thanks,
Danny Kaehn

Re: [PATCH v7 0/3] Firmware Support for USB-HID Devices and CP2112

[Andy Shevchenko]

On Fri, Feb 24, 2023 at 11:48:02AM -0600, Daniel Kaehn wrote:
> On Fri, Feb 24, 2023 at 10:43 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Feb 23, 2023 at 03:31:44PM -0600, Danny Kaehn wrote:
> > > This patchset allows USB-HID devices to have DeviceTree bindings through sharing
> > > the USB fwnode 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.
> >
> > It's your responsibility to carry the tags you have got in the previous rounds
> > of the review. Please, be respectful to the reviewers who spent a lot of time
> > on yours, in particular, code. Otherwise why to bother with it (upstreaming)
> > at all?

> Hello Andy,
> 
> My sincerest apologies on this! I wasn't actually aware that this is
> something I could do / am responsible for doing. No disrespect is
> intended, though I see how this would be frustrating for reviewers (I
> previously thought that maintainers used some sort of automated tool
> to keep track of who approved/acked what in previous versions, but
> didn't really know how that would work).

It's works only in the case if you have no more comments to address.
Indeed, `b4` tool may harvest tags from emails.

However, if you by your own initiative send a new version, to address
or change something, you need to take into account what was done in
the previous rounds of review. If you consider that tag can't be applied —
too many changes that doesn't match the code to the previous version(s), —
you should express why in the cover letter.

> If I'm understanding correctly, this means that whenever someone
> responds to my patch with a "Reviewed-by", etc.. I should be adding
> that tag to the end of the commit message of that patch if a future
> revision is needed?

Correct and you may use `b4` tool yourself to simplify that. It does
not require anything special (permissions, status, access, etc).

> I assume this only applies on future revisions
> where patches other than the one initially reviewed are changed, and
> that any tags I take with should be dropped if that patch is changed?

Depends on how big they are. In most cases the changes are not so drastically
big, so tags survive them.

> Apologies about these questions - - I looked for guidance on this in
> the various "submitting patches to the kernel" guides out there, and
> wasn't able to find much.

Understood. Now we will wait for v8 where you fix the mistakes.

Thank you for your patience and contribution!

-- 
With Best Regards,
Andy Shevchenko