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

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

[Danny Kaehn]

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

[PATCH v10 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 <danny.kaehn@plexus.com>
---

Note -- Reviewed-By tags have been removed as suggested by Benjamin, since
1. It has been 6+ months since this binding was reviewed, and a lot can
change upstream in that time
2. There has been some contention between using named child nodes to
identify i2c and gpio nodes, and also making the driver implementing this
binding compatible with ACPI, since names aren't significant for ACPI
nodes, and ACPI names are always automatically uppercased. It has been
suggested that perhaps the DT binding should use child nodes with
addressable `reg` properties to identify the child nodes, instead of by
name [1].

Of course, I acknowledge that other firmware languages and kernel details
shouldn't impact DT bindings, but it also seems that there should
be some consistent way to specify sub-functions like this accross DT
and ACPI. Some additional commentary / requests for comment about the
seemingly missing glue here can be found in [2].

Any comments from Rob/Krzysztof/other DT folks would be greatly appreciated

[1] https://lore.kernel.org/all/ZBhoHzTr5l38u%2FkX@smile.fi.intel.com/
[2] https://lore.kernel.org/all/CAP+ZCCd0cD+q7=ngyEzScAte2VT9R00mqCQxB3K2TMbeg8UAfA@mail.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 v10 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 <danny.kaehn@plexus.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 a90ed2ceae84..cb687ea7325c 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 v10 3/3] HID: cp2112: Fwnode Support

[Danny Kaehn]

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

I2C and GPIO child nodes can either be children with names "i2c" and
"gpio", or, for ACPI, device nodes with _ADR Zero and One,
respectively.

Additionally, support configuring the I2C bus speed from the
clock-frequency device property.

Signed-off-by: Danny Kaehn <danny.kaehn@plexus.com>
---

Modeled this version based on Andy's email [1], but made the following
primary changes:
1. Use enum instead of #defines at Benjamin's request
2. Change if() else on checking name existence to allow a fwnode to
have a name that doesn't match to still be checked for its ACPI address
(since fwnode_get_name() _does_ still return a name for ACPI nodes)
3. Continue gracefully / silently if matching fwnodes fails

ACPI compatibility re-tested in QEMU as per conversations in v8.

NOTE: now that I'm not using device_get_named_child_node(), I am no longer
being left with a fwnode reference. I am not entirely sure if I
_need_ one for how I am using the handles, so I have left out the calls
to fwnode_handle_get() and fwnode_hand_put() for now. Plese correct me if
this is a situation where a reference should be held until the driver
is removed. Note that this has been present since v9, but I intended to
include this comment on that patch.

[1] https://lore.kernel.org/all/ZBhYXwjPeRiZwxMT@smile.fi.intel.com/

 drivers/hid/hid-cp2112.c | 50 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 20a0d1315d90..2ec0e5b95845 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -27,6 +27,25 @@
 #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,
+};
+
+/**
+ * CP2112 Fwnode child names.
+ * CP2112 sub-functions can be described by named fwnode children or by ACPI _ADR
+ */
+static const char * const cp2112_cell_names[] = {
+	[CP2112_I2C_ADR]	= "i2c",
+	[CP2112_GPIO_ADR]	= "gpio",
+};
+
 #define CP2112_REPORT_MAX_LENGTH		64
 #define CP2112_GPIO_CONFIG_LENGTH		5
 #define CP2112_GPIO_GET_LENGTH			2
@@ -1195,7 +1214,11 @@ 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;
+	struct i2c_timings timings;
+	const char *name;
+	u32 addr;
 	int ret;
 
 	dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
@@ -1209,6 +1232,30 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	mutex_init(&dev->lock);
 
+	device_for_each_child_node(&hdev->dev, child) {
+		name = fwnode_get_name(child);
+		if (name) {
+			ret = match_string(cp2112_cell_names,
+							ARRAY_SIZE(cp2112_cell_names), name);
+			if (ret >= 0)
+				addr = ret;
+		}
+		if (!name || ret < 0)
+			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
+
+		if (ret < 0)
+			continue;
+
+		switch (addr) {
+		case CP2112_I2C_ADR:
+			device_set_node(&dev->adap.dev, child);
+			break;
+		case CP2112_GPIO_ADR:
+			dev->gc.fwnode = child;
+			break;
+		}
+	}
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "parse failed\n");
@@ -1254,6 +1301,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

Re: [PATCH v10 3/3] HID: cp2112: Fwnode Support

[Andy Shevchenko]

On Mon, Feb 05, 2024 at 11:09:22AM -0600, Danny Kaehn wrote:
> Support describing the CP2112's I2C and GPIO interfaces in firmware.
> 
> I2C and GPIO child nodes can either be children with names "i2c" and
> "gpio", or, for ACPI, device nodes with _ADR Zero and One,
> respectively.

> Additionally, support configuring the I2C bus speed from the
> clock-frequency device property.

This has to be in a separate patch, which may predecess this one.

...

> +		name = fwnode_get_name(child);
> +		if (name) {
> +			ret = match_string(cp2112_cell_names,
> +							ARRAY_SIZE(cp2112_cell_names), name);
> +			if (ret >= 0)
> +				addr = ret;
> +		}
> +		if (!name || ret < 0)
> +			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> +
> +		if (ret < 0)
> +			continue;

I don't like this piece (esp. due to possible matching with node name which
may not be so reliable), but I have no better solution right now.

Maybe this way (this doesn't particularly solve the issue but seems better
to me)?

		ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
		if (ret) {
			/* If no ACPI given or compiled, fallback to matching names */
			name = fwnode_get_name(child);
			if (!name)
				continue;

			ret = match_string(cp2112_cell_names, ARRAY_SIZE(cp2112_cell_names), name);
			if (ret < 0)
				continue;

			addr = ret;
		}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v10 3/3] HID: cp2112: Fwnode Support

[kernel test robot]

Hi Danny,

kernel test robot noticed the following build warnings:

[auto build test WARNING on hid/for-next]
[also build test WARNING on robh/for-next andi-shyti/i2c/i2c-host linus/master v6.8-rc3 next-20240206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Danny-Kaehn/dt-bindings-i2c-Add-CP2112-HID-USB-to-SMBus-Bridge/20240206-015922
base:   https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link:    https://lore.kernel.org/r/20240205170920.93499-4-danny.kaehn%40plexus.com
patch subject: [PATCH v10 3/3] HID: cp2112: Fwnode Support
config: x86_64-randconfig-102-20240206 (https://download.01.org/0day-ci/archive/20240206/202402062306.UUQJhqxi-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240206/202402062306.UUQJhqxi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402062306.UUQJhqxi-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/hid/hid-cp2112.c:41: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * CP2112 Fwnode child names.


vim +41 drivers/hid/hid-cp2112.c

    39	
    40	/**
  > 41	 * CP2112 Fwnode child names.
    42	 * CP2112 sub-functions can be described by named fwnode children or by ACPI _ADR
    43	 */
    44	static const char * const cp2112_cell_names[] = {
    45		[CP2112_I2C_ADR]	= "i2c",
    46		[CP2112_GPIO_ADR]	= "gpio",
    47	};
    48	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Rob Herring]

On Mon, Feb 05, 2024 at 11:09:20AM -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.
> 
> Signed-off-by: Danny Kaehn <danny.kaehn@plexus.com>
> ---
> 
> Note -- Reviewed-By tags have been removed as suggested by Benjamin, since
> 1. It has been 6+ months since this binding was reviewed, and a lot can
> change upstream in that time
> 2. There has been some contention between using named child nodes to
> identify i2c and gpio nodes, and also making the driver implementing this
> binding compatible with ACPI, since names aren't significant for ACPI
> nodes, and ACPI names are always automatically uppercased. It has been
> suggested that perhaps the DT binding should use child nodes with
> addressable `reg` properties to identify the child nodes, instead of by
> name [1].

'reg' only makes sense if there are values which relate to the h/w. If 
your addresses are indices, that will be suspect.

There's documented nodenames for specific device classes in DT. You have 
to use those whether there's 'reg' and a unit-address or not. I'm not 
really clear what the problem is.

> 
> Of course, I acknowledge that other firmware languages and kernel details
> shouldn't impact DT bindings, but it also seems that there should
> be some consistent way to specify sub-functions like this accross DT
> and ACPI. Some additional commentary / requests for comment about the
> seemingly missing glue here can be found in [2].

I have little interest in worrying about ACPI as I have limited 
knowledge in ACPI requirements, what I do know is the model for bindings 
are fundamentally differ, and no one has stepped up to maintain bindings 
from an ACPI perspective.

> Any comments from Rob/Krzysztof/other DT folks would be greatly appreciated
> 
> [1] https://lore.kernel.org/all/ZBhoHzTr5l38u%2FkX@smile.fi.intel.com/
> [2] https://lore.kernel.org/all/CAP+ZCCd0cD+q7=ngyEzScAte2VT9R00mqCQxB3K2TMbeg8UAfA@mail.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

Why do you have GPIOs if this is a proper controller?

> +
> +      clock-frequency:
> +        minimum: 10000
> +        default: 100000
> +        maximum: 400000
> +
> +  gpio:
> +    description: The GPIO controller node for the CP2112

There's no need for a child node here. All these properties can be part 
of the parent.


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

Re: [PATCH v10 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus

[Danny Kaehn]

Thanks for taking a look Rob.

On Tue, 2024-02-13 at 09:28 -0600, Rob Herring wrote:
> On Mon, Feb 05, 2024 at 11:09:20AM -0600, Danny Kaehn wrote:
> > This is a USB HID device which includes an I2C controller and 8 GPIO pins.
...
> > 2. There has been some contention between using named child nodes to
> > identify i2c and gpio nodes, and also making the driver implementing this
> > binding compatible with ACPI, since names aren't significant for ACPI
> > nodes, and ACPI names are always automatically uppercased. It has been
> > suggested that perhaps the DT binding should use child nodes with
> > addressable `reg` properties to identify the child nodes, instead of by
> > name [1].
> 
> 'reg' only makes sense if there are values which relate to the h/w. If 
> your addresses are indices, that will be suspect.
> 
> There's documented nodenames for specific device classes in DT. You have 
> to use those whether there's 'reg' and a unit-address or not. I'm not 
> really clear what the problem is.
> 
Ack. Mostly just forwarding on Andy Shevchenko's suggestion for making a more
consistent interface across ACPI and DT since ACPI doesn't support identifying
children by named nodes.

> > 
> > Of course, I acknowledge that other firmware languages and kernel details
> > shouldn't impact DT bindings, but it also seems that there should
> > be some consistent way to specify sub-functions like this accross DT
> > and ACPI. Some additional commentary / requests for comment about the
> > seemingly missing glue here can be found in [2].
> 
> I have little interest in worrying about ACPI as I have limited 
> knowledge in ACPI requirements, what I do know is the model for bindings 
> are fundamentally differ, and no one has stepped up to maintain bindings 
> from an ACPI perspective.
> 

Fair enough.

> > Any comments from Rob/Krzysztof/other DT folks would be greatly appreciated
> > 
> > [1] https://lore.kernel.org/all/ZBhoHzTr5l38u%2FkX@smile.fi.intel.com/
> > [2] https://lore.kernel.org/all/CAP+ZCCd0cD+q7=ngyEzScAte2VT9R00mqCQxB3K2TMbeg8UAfA@mail.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
> 
> Why do you have GPIOs if this is a proper controller?

This is exclusively for bus recovery (not implemented in the driver patch
sent here). I believe there's precedent for this in bindings like i2c-imx.yaml?

(skip if the above was all you needed):

Hopefully without going into more details than you're interested in, the
CP2112 hardware doesn't implement any runtime bus recovery algorithms.
Even just by bridging two of the CP2112's GPIOs with SCL and SDA,
I was able to use the generic GPIO bus recovery routine to recover a stuck bus.
This was especially important since v2 of the CP2112 hardware has an errata
which can cause uncompleted I2C transactions on a semi-regular basis.

> 
> > +
> > +      clock-frequency:
> > +        minimum: 10000
> > +        default: 100000
> > +        maximum: 400000
> > +
> > +  gpio:
> > +    description: The GPIO controller node for the CP2112
> 
> There's no need for a child node here. All these properties can be part 
> of the parent.

I had gone back and forth on this for quite some time. Would you suggest this
just because it _can_ be combined, due to the naming constraint on the "hog"
nodes? (as opposed to the i2c not being able to be combined, due to
unconstrained names of child nodes?).

I had initially thought this approach would scale better -- say there was a
similar chip with I2C, GPIO, SPI, and UART interfaces -- would GPIO still
share a node with the parent? And is there a reason that gpio is
special, or just it _can_ be combined due to the naming restrictions? Looking
at some of the bindings under mfd/ I see the GPIO controller broken into a
named child node, although I see they also have their own compatible strings...

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

Thanks,

Danny Kaehn