linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v12 0/3] Firmware Support for USB-HID Devices and CP2112
@ 2025-11-26 17:05 Danny Kaehn
  2025-11-26 17:05 ` [PATCH v12 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge Danny Kaehn
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Danny Kaehn @ 2025-11-26 17:05 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Benjamin Tissoires,
	Andy Shevchenko, Danny Kaehn, Andi Shyti, Conor Dooley
  Cc: Jiri Kosina, devicetree, linux-input, Dmitry Torokhov,
	Bartosz Golaszewski, Ethan Twardy, linux-i2c, linux-kernel,
	Leo Huang, Arun D Patil, Willie Thai, Ting-Kai Chen

This patchset allows USB-HID devices to have Firmware 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 ACPI and interoperate with other drivers.

Changes in v12:
- dt-binding changes:
  - Drop "on the host controller" from top-level description based on
      comment from Rob H.
  - Correct "Properties must precede subnodes" dt_binding_check error by
      moving gpio_chip-related properties above the i2c subnode in the
      binding and in the example.
  - Include `interrupt-controller` property in the example
- Modify hid-cp2112.c to support separate schemas for DT vs. ACPI - DT
  combines gpio subnode with the CP2112's node, but will have an I2C
  subnode; while ACPI will maintain separate child nodes for the GPIO
  I2C devices

Changes in v11:
- Eliminate 'gpio' subnode for DT and ACPI for the CP2112 per comment
    from Rob H.
- Edit hid-cp2112.c to match for ACPI index and fall back to matching by
    name (instead of the other way around)
- Separate CP2112 I2C bus speed configuration into a separate patch

Changes in v10:
- Define an enumeration and mapping for CP2112 ACPI _ADRs and devicetree
    child node names, and use these in the scanning of child nodes
- Address other miscellaneous

Changes in v9:
- Add _ADR-based ACPI binding of child nodes (I2C is _ADR Zero, GPIO is _ADR One)
- Use a loop-based approach for assigning child nodes within probe().
    As a consequence, hid-cp2112.c no longer maintains references to the
    child fwnodes during the lifetime of the device. (plese correct if this
    is actually needed for this use-case)

Changes in v8:
- Apply Review tags retroactively to patches previously reviewed

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
   - Remove ngpios property
   - Constrain the hog pattern to a single naming scheme
   - Remove unneeded properties from the gpio hog which are provided by
       the parent schema
 - 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                      |  50 ++++++++
 drivers/hid/usbhid/hid-core.c                 |   2 +
 3 files changed, 165 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml

--
2.25.1

---
Danny Kaehn (3):
      dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge
      HID: cp2112: Fwnode Support
      HID: cp2112: Configure I2C Bus Speed from Firmware

 .../devicetree/bindings/i2c/silabs,cp2112.yaml     | 107 +++++++++++++++++++++
 drivers/hid/hid-cp2112.c                           |  36 +++++++
 2 files changed, 143 insertions(+)
---
base-commit: 1c772200c9dcb23a304f84a9334fe2e0d9529ab0
change-id: 20240605-cp2112-dt-7cdc95448e8a

Best regards,
-- 
Danny Kaehn <danny.kaehn@plexus.com>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v12 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge
  2025-11-26 17:05 [PATCH v12 0/3] Firmware Support for USB-HID Devices and CP2112 Danny Kaehn
@ 2025-11-26 17:05 ` Danny Kaehn
  2025-11-27  7:24   ` Krzysztof Kozlowski
  2025-11-26 17:05 ` [PATCH v12 2/3] HID: cp2112: Fwnode Support Danny Kaehn
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Danny Kaehn @ 2025-11-26 17:05 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Benjamin Tissoires,
	Andy Shevchenko, Danny Kaehn, Andi Shyti, Conor Dooley
  Cc: Jiri Kosina, devicetree, linux-input, Dmitry Torokhov,
	Bartosz Golaszewski, Ethan Twardy, linux-i2c, linux-kernel,
	Leo Huang, Arun D Patil, Willie Thai, Ting-Kai Chen

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,
with the i2c controller being bound to a subnode named "i2c". This is
intended to be used in configurations where the CP2112 is permanently
connected in hardware.

Signed-off-by: Danny Kaehn <danny.kaehn@plexus.com>
---
 .../devicetree/bindings/i2c/silabs,cp2112.yaml     | 107 +++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
new file mode 100644
index 000000000000..4b5c1af3673d
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
@@ -0,0 +1,107 @@
+# 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 <danny.kaehn@plexus.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
+
+  interrupt-controller: true
+  "#interrupt-cells":
+    const: 2
+
+  gpio-controller: true
+  "#gpio-cells":
+    const: 2
+
+  gpio-line-names:
+    minItems: 1
+    maxItems: 8
+
+  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
+
+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>;
+
+      cp2112: device@1 {
+        compatible = "usb10c4,ea90";
+        reg = <1>;
+
+        gpio-controller;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        #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";
+        };
+
+        i2c {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          sda-gpios = <&cp2112 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+          scl-gpios = <&cp2112 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+
+          temp@48 {
+            compatible = "national,lm75";
+            reg = <0x48>;
+          };
+        };
+
+      };
+    };

-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v12 2/3] HID: cp2112: Fwnode Support
  2025-11-26 17:05 [PATCH v12 0/3] Firmware Support for USB-HID Devices and CP2112 Danny Kaehn
  2025-11-26 17:05 ` [PATCH v12 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge Danny Kaehn
@ 2025-11-26 17:05 ` Danny Kaehn
  2025-11-26 18:27   ` Andy Shevchenko
  2025-11-26 17:05 ` [PATCH v12 3/3] HID: cp2112: Configure I2C Bus Speed from Firmware Danny Kaehn
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Danny Kaehn @ 2025-11-26 17:05 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Benjamin Tissoires,
	Andy Shevchenko, Danny Kaehn, Andi Shyti, Conor Dooley
  Cc: Jiri Kosina, devicetree, linux-input, Dmitry Torokhov,
	Bartosz Golaszewski, Ethan Twardy, linux-i2c, linux-kernel,
	Leo Huang, Arun D Patil, Willie Thai, Ting-Kai Chen

Support describing the CP2112's I2C and GPIO interfaces in firmware.

Bindings between the firmware nodes and the functions of the device
are distinct between ACPI and DeviceTree.

For ACPI, the i2c_adapter will use the child with _ADR Zero and the
gpio_chip will use the child with _ADR One. For DeviceTree, the
i2c_adapter will use the child with name "i2c", but the gpio_chip
will share a firmware node with the CP2112.

Signed-off-by: Danny Kaehn <danny.kaehn@plexus.com>
---
 drivers/hid/hid-cp2112.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 803b883ae875..fb301c27c712 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -29,6 +29,16 @@
 #include <linux/usb/ch9.h>
 #include "hid-ids.h"
 
+/**
+ * enum cp2112_child_acpi_cell_addrs - Child ACPI addresses for CP2112 sub-functions
+ * @CP2112_I2C_ADR: Address for I2C node
+ * @CP2112_GPIO_ADR: Address for GPIO node
+ */
+enum cp2112_child_acpi_cell_addrs {
+	CP2112_I2C_ADR = 0,
+	CP2112_GPIO_ADR = 1,
+};
+
 #define CP2112_REPORT_MAX_LENGTH		64
 #define CP2112_GPIO_CONFIG_LENGTH		5
 #define CP2112_GPIO_GET_LENGTH			2
@@ -1208,7 +1218,9 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	struct cp2112_device *dev;
 	u8 buf[3];
 	struct cp2112_smbus_config_report config;
+	struct fwnode_handle *child;
 	struct gpio_irq_chip *girq;
+	u32 addr;
 	int ret;
 
 	dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
@@ -1226,6 +1238,26 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
+	if (is_acpi_device_node(hdev->dev.fwnode)) {
+		device_for_each_child_node(&hdev->dev, child) {
+			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
+			if (ret)
+				continue;
+
+			switch (addr) {
+			case CP2112_I2C_ADR:
+				device_set_node(&dev->adap.dev, child);
+				break;
+			case CP2112_GPIO_ADR:
+				dev->gc.fwnode = child;
+				break;
+			}
+		}
+	} else {
+		device_set_node(&dev->adap.dev,
+			device_get_named_child_node(&hdev->dev, "i2c"));
+	}
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "parse failed\n");

-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v12 3/3] HID: cp2112: Configure I2C Bus Speed from Firmware
  2025-11-26 17:05 [PATCH v12 0/3] Firmware Support for USB-HID Devices and CP2112 Danny Kaehn
  2025-11-26 17:05 ` [PATCH v12 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge Danny Kaehn
  2025-11-26 17:05 ` [PATCH v12 2/3] HID: cp2112: Fwnode Support Danny Kaehn
@ 2025-11-26 17:05 ` Danny Kaehn
  2025-11-26 18:28   ` Andy Shevchenko
  2025-11-26 18:29 ` [PATCH v12 0/3] Firmware Support for USB-HID Devices and CP2112 Andy Shevchenko
  2025-11-27  7:20 ` Krzysztof Kozlowski
  4 siblings, 1 reply; 11+ messages in thread
From: Danny Kaehn @ 2025-11-26 17:05 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Benjamin Tissoires,
	Andy Shevchenko, Danny Kaehn, Andi Shyti, Conor Dooley
  Cc: Jiri Kosina, devicetree, linux-input, Dmitry Torokhov,
	Bartosz Golaszewski, Ethan Twardy, linux-i2c, linux-kernel,
	Leo Huang, Arun D Patil, Willie Thai, Ting-Kai Chen

Now that the I2C adapter on the CP2112 can have an associated firmware
node, set the bus speed based on firmware configuration

Signed-off-by: Danny Kaehn <danny.kaehn@plexus.com>
---
 drivers/hid/hid-cp2112.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index fb301c27c712..801fe3ccad5e 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1220,6 +1220,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	struct cp2112_smbus_config_report config;
 	struct fwnode_handle *child;
 	struct gpio_irq_chip *girq;
+	struct i2c_timings timings;
 	u32 addr;
 	int ret;
 
@@ -1303,6 +1304,9 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto err_power_normal;
 	}
 
+	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),

-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v12 2/3] HID: cp2112: Fwnode Support
  2025-11-26 17:05 ` [PATCH v12 2/3] HID: cp2112: Fwnode Support Danny Kaehn
@ 2025-11-26 18:27   ` Andy Shevchenko
  2025-11-26 19:32     ` Danny Kaehn
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2025-11-26 18:27 UTC (permalink / raw)
  To: Danny Kaehn
  Cc: Rob Herring, Krzysztof Kozlowski, Benjamin Tissoires, Andi Shyti,
	Conor Dooley, Jiri Kosina, devicetree, linux-input,
	Dmitry Torokhov, Bartosz Golaszewski, Ethan Twardy, linux-i2c,
	linux-kernel, Leo Huang, Arun D Patil, Willie Thai, Ting-Kai Chen

On Wed, Nov 26, 2025 at 11:05:25AM -0600, Danny Kaehn wrote:
> Support describing the CP2112's I2C and GPIO interfaces in firmware.
> 
> Bindings between the firmware nodes and the functions of the device
> are distinct between ACPI and DeviceTree.
> 
> For ACPI, the i2c_adapter will use the child with _ADR Zero and the
> gpio_chip will use the child with _ADR One. For DeviceTree, the
> i2c_adapter will use the child with name "i2c", but the gpio_chip
> will share a firmware node with the CP2112.

Hmm... Is there any explanation why DT decided to go that way?

...

> +	if (is_acpi_device_node(hdev->dev.fwnode)) {

Please, do not dereference fwnode, use dev_fwnode() or other APIs for that
(actually the same applies to OF node, but people too much neglect that).

> +		device_for_each_child_node(&hdev->dev, child) {
> +			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> +			if (ret)
> +				continue;
> +
> +			switch (addr) {
> +			case CP2112_I2C_ADR:
> +				device_set_node(&dev->adap.dev, child);
> +				break;
> +			case CP2112_GPIO_ADR:
> +				dev->gc.fwnode = child;
> +				break;

If by any chance we have malformed table and there are more devices with
the same address? Maybe we don't need to address this right now, just
asking... (I believe ACPI compiler won't allow that, but table can be
crafted directly in the binary format.)

> +			}
> +		}
> +	} else {
> +		device_set_node(&dev->adap.dev,
> +			device_get_named_child_node(&hdev->dev, "i2c"));

Here we bump the reference count, where is it going to be dropped?

Note, in the other branch (ACPI) the reference count is not bumped in
the current code.

> +	}

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v12 3/3] HID: cp2112: Configure I2C Bus Speed from Firmware
  2025-11-26 17:05 ` [PATCH v12 3/3] HID: cp2112: Configure I2C Bus Speed from Firmware Danny Kaehn
@ 2025-11-26 18:28   ` Andy Shevchenko
  0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2025-11-26 18:28 UTC (permalink / raw)
  To: Danny Kaehn
  Cc: Rob Herring, Krzysztof Kozlowski, Benjamin Tissoires, Andi Shyti,
	Conor Dooley, Jiri Kosina, devicetree, linux-input,
	Dmitry Torokhov, Bartosz Golaszewski, Ethan Twardy, linux-i2c,
	linux-kernel, Leo Huang, Arun D Patil, Willie Thai, Ting-Kai Chen

On Wed, Nov 26, 2025 at 11:05:26AM -0600, Danny Kaehn wrote:
> Now that the I2C adapter on the CP2112 can have an associated firmware
> node, set the bus speed based on firmware configuration

Missed period at the end.

With that being addressed,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v12 0/3] Firmware Support for USB-HID Devices and CP2112
  2025-11-26 17:05 [PATCH v12 0/3] Firmware Support for USB-HID Devices and CP2112 Danny Kaehn
                   ` (2 preceding siblings ...)
  2025-11-26 17:05 ` [PATCH v12 3/3] HID: cp2112: Configure I2C Bus Speed from Firmware Danny Kaehn
@ 2025-11-26 18:29 ` Andy Shevchenko
  2025-11-27  7:20 ` Krzysztof Kozlowski
  4 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2025-11-26 18:29 UTC (permalink / raw)
  To: Danny Kaehn
  Cc: Rob Herring, Krzysztof Kozlowski, Benjamin Tissoires, Andi Shyti,
	Conor Dooley, Jiri Kosina, devicetree, linux-input,
	Dmitry Torokhov, Bartosz Golaszewski, Ethan Twardy, linux-i2c,
	linux-kernel, Leo Huang, Arun D Patil, Willie Thai, Ting-Kai Chen

On Wed, Nov 26, 2025 at 11:05:23AM -0600, Danny Kaehn wrote:
> This patchset allows USB-HID devices to have Firmware 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 ACPI and interoperate with other drivers.
> 
> Changes in v12:
> - dt-binding changes:
>   - Drop "on the host controller" from top-level description based on
>       comment from Rob H.
>   - Correct "Properties must precede subnodes" dt_binding_check error by
>       moving gpio_chip-related properties above the i2c subnode in the
>       binding and in the example.
>   - Include `interrupt-controller` property in the example
> - Modify hid-cp2112.c to support separate schemas for DT vs. ACPI - DT
>   combines gpio subnode with the CP2112's node, but will have an I2C
>   subnode; while ACPI will maintain separate child nodes for the GPIO
>   I2C devices

Thanks for pursuing this! I have a few comments, but in general I'm fine with
the design.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v12 2/3] HID: cp2112: Fwnode Support
  2025-11-26 18:27   ` Andy Shevchenko
@ 2025-11-26 19:32     ` Danny Kaehn
  2025-11-26 21:23       ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Danny Kaehn @ 2025-11-26 19:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rob Herring, Krzysztof Kozlowski, Benjamin Tissoires, Andi Shyti,
	Conor Dooley, Jiri Kosina, devicetree, linux-input,
	Dmitry Torokhov, Bartosz Golaszewski, Ethan Twardy, linux-i2c,
	linux-kernel, Leo Huang, Arun D Patil, Willie Thai, Ting-Kai Chen

Hi Andy,

Thanks for the review!

On Wed, Nov 26, 2025 at 08:27:19PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 26, 2025 at 11:05:25AM -0600, Danny Kaehn wrote:
> > Support describing the CP2112's I2C and GPIO interfaces in firmware.
> > 
> > Bindings between the firmware nodes and the functions of the device
> > are distinct between ACPI and DeviceTree.
> > 
> > For ACPI, the i2c_adapter will use the child with _ADR Zero and the
> > gpio_chip will use the child with _ADR One. For DeviceTree, the
> > i2c_adapter will use the child with name "i2c", but the gpio_chip
> > will share a firmware node with the CP2112.
> 
> Hmm... Is there any explanation why DT decided to go that way?
>

I don't have an explanation, but Rob H. had directed that I make this
change in [1].

In v11, I then removed that child node for both ACPI and DT, hoping to
maintain unity, but you had directed that wouldn't be intuitive for ACPI
in [2].

Thus, in this v12, I have just entirely split the two, as it seemed
unlikely that any compromise to unify the schema between the two
firmware languages would be possible for a change/driver this
inconsquential to the overall kernel.

[1]:
https://lore.kernel.org/all/20240213152825.GA1223720-robh@kernel.org/

[2]:
https://lore.kernel.org/all/ZmISaEIGlxZVK_jf@smile.fi.intel.com/


> ...
> 
> > +	if (is_acpi_device_node(hdev->dev.fwnode)) {
> 
> Please, do not dereference fwnode, use dev_fwnode() or other APIs for that
> (actually the same applies to OF node, but people too much neglect that).
>

Thanks, will do.

> > +		device_for_each_child_node(&hdev->dev, child) {
> > +			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > +			if (ret)
> > +				continue;
> > +
> > +			switch (addr) {
> > +			case CP2112_I2C_ADR:
> > +				device_set_node(&dev->adap.dev, child);
> > +				break;
> > +			case CP2112_GPIO_ADR:
> > +				dev->gc.fwnode = child;
> > +				break;
> 
> If by any chance we have malformed table and there are more devices with
> the same address? Maybe we don't need to address this right now, just
> asking... (I believe ACPI compiler won't allow that, but table can be
> crafted directly in the binary format.)
>

You're sugggesting perhaps that we explicitly keep track of which
addresses have been encountered, and refuse to do any fwnode parsing
if we detect the same address used twice? I believe the current behavior
would be that the "last node wins"; not sure if it should be a "first node
wins" or a full error scenario...

> > +			}
> > +		}
> > +	} else {
> > +		device_set_node(&dev->adap.dev,
> > +			device_get_named_child_node(&hdev->dev, "i2c"));
> 
> Here we bump the reference count, where is it going to be dropped?
> 
> Note, in the other branch (ACPI) the reference count is not bumped in
> the current code.
>

Great point, forgot that I had dropped that handling in v9. The old
behavior was that the CP2112 driver maintained a reference to each node
during the lifetime of the device (and released during probe errors,
etc..). I'm still a bit confused as to whether that is correct or not,
or if the references should immediately be dropped once they're done
being parsed during probe()... My understanding previously was that I
should keep the reference count for the child fwnodes for the lifetime
of the CP2112, since the pointers to those are stored in the child
devices but would usually be managed by the parent bus-level code, does
that seem correct?

> > +	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Thanks,

Danny Kaehn


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v12 2/3] HID: cp2112: Fwnode Support
  2025-11-26 19:32     ` Danny Kaehn
@ 2025-11-26 21:23       ` Andy Shevchenko
  0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2025-11-26 21:23 UTC (permalink / raw)
  To: Danny Kaehn
  Cc: Rob Herring, Krzysztof Kozlowski, Benjamin Tissoires, Andi Shyti,
	Conor Dooley, Jiri Kosina, devicetree, linux-input,
	Dmitry Torokhov, Bartosz Golaszewski, Ethan Twardy, linux-i2c,
	linux-kernel, Leo Huang, Arun D Patil, Willie Thai, Ting-Kai Chen

On Wed, Nov 26, 2025 at 01:32:51PM -0600, Danny Kaehn wrote:
> On Wed, Nov 26, 2025 at 08:27:19PM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 26, 2025 at 11:05:25AM -0600, Danny Kaehn wrote:

...

> > > For ACPI, the i2c_adapter will use the child with _ADR Zero and the
> > > gpio_chip will use the child with _ADR One. For DeviceTree, the
> > > i2c_adapter will use the child with name "i2c", but the gpio_chip
> > > will share a firmware node with the CP2112.
> > 
> > Hmm... Is there any explanation why DT decided to go that way?
> 
> I don't have an explanation, but Rob H. had directed that I make this
> change in [1].
> 
> In v11, I then removed that child node for both ACPI and DT, hoping to
> maintain unity, but you had directed that wouldn't be intuitive for ACPI
> in [2].
> 
> Thus, in this v12, I have just entirely split the two, as it seemed
> unlikely that any compromise to unify the schema between the two
> firmware languages would be possible for a change/driver this
> inconsquential to the overall kernel.

Even though, would be nice to try to get a rationale from Rob on this.
Then we can put it in the commit message to explain. Otherwise it will
confuse history diggers in the future.

> [1]:
> https://lore.kernel.org/all/20240213152825.GA1223720-robh@kernel.org/
> 
> [2]:
> https://lore.kernel.org/all/ZmISaEIGlxZVK_jf@smile.fi.intel.com/

...

> > > +			switch (addr) {
> > > +			case CP2112_I2C_ADR:
> > > +				device_set_node(&dev->adap.dev, child);
> > > +				break;
> > > +			case CP2112_GPIO_ADR:
> > > +				dev->gc.fwnode = child;
> > > +				break;
> > 
> > If by any chance we have malformed table and there are more devices with
> > the same address? Maybe we don't need to address this right now, just
> > asking... (I believe ACPI compiler won't allow that, but table can be
> > crafted directly in the binary format.)
> >
> 
> You're sugggesting perhaps that we explicitly keep track of which
> addresses have been encountered, and refuse to do any fwnode parsing
> if we detect the same address used twice? I believe the current behavior
> would be that the "last node wins"; not sure if it should be a "first node
> wins" or a full error scenario...

I'm suggesting to think about this, not acting right now. I don't believe in
such a case IRL.

> > > +			}

...

> > > +		device_set_node(&dev->adap.dev,
> > > +			device_get_named_child_node(&hdev->dev, "i2c"));
> > 
> > Here we bump the reference count, where is it going to be dropped?
> > 
> > Note, in the other branch (ACPI) the reference count is not bumped in
> > the current code.
> 
> Great point, forgot that I had dropped that handling in v9. The old
> behavior was that the CP2112 driver maintained a reference to each node
> during the lifetime of the device (and released during probe errors,
> etc..). I'm still a bit confused as to whether that is correct or not,
> or if the references should immediately be dropped once they're done
> being parsed during probe()... My understanding previously was that I
> should keep the reference count for the child fwnodes for the lifetime
> of the CP2112, since the pointers to those are stored in the child
> devices but would usually be managed by the parent bus-level code, does
> that seem correct?

While there is a (theoretical) possibility to have lifetime of fwnode shorter
than a device's, I don't think we have or ever will have such a practical
example. So, assumption is that, the fwnode that struct device holds has
the same or longer lifetime.

Note, I haven't investigated overlays (DT and ACPI) behaviour. IIRC you
experimented with ACPI SSDT on this device, perhaps you can try to see
what happens if there is a confirmed that the above is not only a theoretical
problem.

TL;DR: I would drop reference count just after we got a respective fwnode.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v12 0/3] Firmware Support for USB-HID Devices and CP2112
  2025-11-26 17:05 [PATCH v12 0/3] Firmware Support for USB-HID Devices and CP2112 Danny Kaehn
                   ` (3 preceding siblings ...)
  2025-11-26 18:29 ` [PATCH v12 0/3] Firmware Support for USB-HID Devices and CP2112 Andy Shevchenko
@ 2025-11-27  7:20 ` Krzysztof Kozlowski
  4 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2025-11-27  7:20 UTC (permalink / raw)
  To: Danny Kaehn
  Cc: Rob Herring, Krzysztof Kozlowski, Benjamin Tissoires,
	Andy Shevchenko, Andi Shyti, Conor Dooley, Jiri Kosina,
	devicetree, linux-input, Dmitry Torokhov, Bartosz Golaszewski,
	Ethan Twardy, linux-i2c, linux-kernel, Leo Huang, Arun D Patil,
	Willie Thai, Ting-Kai Chen

On Wed, Nov 26, 2025 at 11:05:23AM -0600, Danny Kaehn wrote:
> This patchset allows USB-HID devices to have Firmware 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 ACPI and interoperate with other drivers.
> 
> Changes in v12:
> - dt-binding changes:
>   - Drop "on the host controller" from top-level description based on
>       comment from Rob H.
>   - Correct "Properties must precede subnodes" dt_binding_check error by
>       moving gpio_chip-related properties above the i2c subnode in the
>       binding and in the example.
>   - Include `interrupt-controller` property in the example
> - Modify hid-cp2112.c to support separate schemas for DT vs. ACPI - DT
>   combines gpio subnode with the CP2112's node, but will have an I2C
>   subnode; while ACPI will maintain separate child nodes for the GPIO
>   I2C devices

That's a b4 managed patch, so I wonder what happened with all the lore
links to previous versions.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v12 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge
  2025-11-26 17:05 ` [PATCH v12 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge Danny Kaehn
@ 2025-11-27  7:24   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2025-11-27  7:24 UTC (permalink / raw)
  To: Danny Kaehn
  Cc: Rob Herring, Krzysztof Kozlowski, Benjamin Tissoires,
	Andy Shevchenko, Andi Shyti, Conor Dooley, Jiri Kosina,
	devicetree, linux-input, Dmitry Torokhov, Bartosz Golaszewski,
	Ethan Twardy, linux-i2c, linux-kernel, Leo Huang, Arun D Patil,
	Willie Thai, Ting-Kai Chen

On Wed, Nov 26, 2025 at 11:05:24AM -0600, Danny Kaehn wrote:
> +  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

Neither scl nor sda-gpios are needed, they are allowed by i2c-controller
schema, so drop.

> +
> +      clock-frequency:
> +        minimum: 10000
> +        default: 100000
> +        maximum: 400000
> +
> +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>;
> +
> +      cp2112: device@1 {
> +        compatible = "usb10c4,ea90";
> +        reg = <1>;
> +
> +        gpio-controller;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +        #gpio-cells = <2>;
> +        gpio-line-names = "CP2112_SDA", "CP2112_SCL", "TEST2",
> +          "TEST3","TEST4", "TEST5", "TEST6";

Please align the next line with opening ". See also DTS coding style.

> +
> +        fan-rst-hog {
> +            gpio-hog;
> +            gpios = <7 GPIO_ACTIVE_HIGH>;
> +            output-high;
> +            line-name = "FAN_RST";
> +        };
> +
> +        i2c {
> +          #address-cells = <1>;

Messed indentation, stick to one, preferred is 4 spaces for the example.

> +          #size-cells = <0>;
> +          sda-gpios = <&cp2112 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> +          scl-gpios = <&cp2112 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> +
> +          temp@48 {
> +            compatible = "national,lm75";
> +            reg = <0x48>;
> +          };
> +        };
> +

Drop blank line

> +      };
> +    };
> 
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-11-27  7:24 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-26 17:05 [PATCH v12 0/3] Firmware Support for USB-HID Devices and CP2112 Danny Kaehn
2025-11-26 17:05 ` [PATCH v12 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge Danny Kaehn
2025-11-27  7:24   ` Krzysztof Kozlowski
2025-11-26 17:05 ` [PATCH v12 2/3] HID: cp2112: Fwnode Support Danny Kaehn
2025-11-26 18:27   ` Andy Shevchenko
2025-11-26 19:32     ` Danny Kaehn
2025-11-26 21:23       ` Andy Shevchenko
2025-11-26 17:05 ` [PATCH v12 3/3] HID: cp2112: Configure I2C Bus Speed from Firmware Danny Kaehn
2025-11-26 18:28   ` Andy Shevchenko
2025-11-26 18:29 ` [PATCH v12 0/3] Firmware Support for USB-HID Devices and CP2112 Andy Shevchenko
2025-11-27  7:20 ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).