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

[PATCH v9 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 ACPI and interoperate with other drivers.

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                      |  18 +++
 drivers/hid/usbhid/hid-core.c                 |   2 +
 3 files changed, 133 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml

-- 
2.25.1

[PATCH v9 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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../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

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

[Krzysztof Kozlowski]

On 19/03/2023 21:48, 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>

From where did you get it? There was no such tag at v7:
https://lore.kernel.org/all/20230223213147.268-2-kaehndan@gmail.com/
nor at v6:
https://lore.kernel.org/all/20230217184904.1290-2-kaehndan@gmail.com/

???

Best regards,
Krzysztof

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

[Daniel Kaehn]

On Mon, Mar 20, 2023, 1:44 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 19/03/2023 21:48, 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>
>
> From where did you get it? There was no such tag at v7:
> https://lore.kernel.org/all/20230223213147.268-2-kaehndan@gmail.com/
> nor at v6:
> https://lore.kernel.org/all/20230217184904.1290-2-kaehndan@gmail.com/
>
> ???

Hi Krzysztof,

My apologies. It looks like you reviewed this in v4 [1].

I had received feedback on v7 that I should be carrying tags from
previous reviews
forward if minimal / no changes have been made to the patch reviewed,
out of respect
for reviewers who look at lots of patches (like yourself) [2], and
hadn't retroactively applied
your tag until v8.

Since v4, I'd only made a few changes suggested by Rob, plus added an example of
specifying bus recovery GPIOs in the binding, and thought those would
fall within the
scope of minor changes -- but my apologies if this was a bad
assumption! I can remove
and re-submit if desired.

Thanks,

Danny Kaehn

[1]: https://lore.kernel.org/all/c999c387-401a-e3a1-f431-2770930c5ecc@linaro.org/
[2]: https://lore.kernel.org/all/Y%2FjpME2mb5CqPooj@smile.fi.intel.com/

[PATCH v9 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>
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 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 v9 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 <kaehndan@gmail.com>
---
 drivers/hid/hid-cp2112.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 27cadadda7c9..9e327763fd90 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1234,6 +1234,10 @@ 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;
+	struct fwnode_handle *child;
+	u32 addr;
+	const char *name;
 	int ret;
 
 	dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
@@ -1247,6 +1251,17 @@ 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);
+		ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
+
+		if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0))
+			device_set_node(&dev->adap.dev, child);
+		else if ((name && strcmp("gpio", name)) == 0 ||
+					(!ret && addr == 1))
+			dev->gc.fwnode = child;
+	}
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "parse failed\n");
@@ -1292,6 +1307,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 v9 3/3] HID: cp2112: Fwnode Support

[Andy Shevchenko]

On Sun, Mar 19, 2023 at 03:48:02PM -0500, 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.

Thank you for the update, my comments below.

...

> +	struct i2c_timings timings;
> +	struct fwnode_handle *child;

Longer line first easier to read.

> +	u32 addr;
> +	const char *name;

Ditto.

...

> +	device_for_each_child_node(&hdev->dev, child) {
> +		name = fwnode_get_name(child);
> +		ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> +
> +		if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0))
> +			device_set_node(&dev->adap.dev, child);
> +		else if ((name && strcmp("gpio", name)) == 0 ||
> +					(!ret && addr == 1))
> +			dev->gc.fwnode = child;
> +	}

Please, make addresses defined explicitly. You may also do it with node naming
schema:

#define CP2112_I2C_ADR		0
#define CP2112_GPIO_ADR		1

static const char * const cp2112_cell_names[] = {
	[CP2112_I2C_ADR]	= "i2c",
	[CP2112_GPIO_ADR]	= "gpio",
};

	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;
		} else
			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
		if (ret < 0)
			...error handling if needed...

		switch (addr) {
		case CP2112_I2C_ADR:
			device_set_node(&dev->adap.dev, child);
			break;
		case CP2112_GPIO_ADR:
			dev->gc.fwnode = child;
			break;
		default:
			...error handling...
		}
	}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko wrote:
> On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote:

...

> > +	device_for_each_child_node(&hdev->dev, child) {
> > +		name = fwnode_get_name(child);
> > +		ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > +
> > +		if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0))
> > +			device_set_node(&dev->adap.dev, child);
> > +		else if ((name && strcmp("gpio", name)) == 0 ||
> > +					(!ret && addr == 1))
> > +			dev->gc.fwnode = child;
> > +	}
> 
> Please, make addresses defined explicitly. You may also do it with node naming
> schema:
> 
> #define CP2112_I2C_ADR		0
> #define CP2112_GPIO_ADR		1
> 
> static const char * const cp2112_cell_names[] = {
> 	[CP2112_I2C_ADR]	= "i2c",
> 	[CP2112_GPIO_ADR]	= "gpio",
> };
> 
> 	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;
> 		} else
> 			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> 		if (ret < 0)
> 			...error handling if needed...
> 
> 		switch (addr) {
> 		case CP2112_I2C_ADR:
> 			device_set_node(&dev->adap.dev, child);
> 			break;
> 		case CP2112_GPIO_ADR:
> 			dev->gc.fwnode = child;
> 			break;
> 		default:
> 			...error handling...
> 		}
> 	}

Btw, don't you use "reg" property for the child nodes? It would be better from
de facto used patterns (we have a couple of mode drivers that have a common
code to read "reg" or _ADR() and that code can be split into a helper and used
here).

-- 
With Best Regards,
Andy Shevchenko

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

[Daniel Kaehn]

On Mon, Mar 20, 2023 at 8:00 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko wrote:
> > On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote:
>
> ...
>
> > > +   device_for_each_child_node(&hdev->dev, child) {
> > > +           name = fwnode_get_name(child);
> > > +           ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > > +
> > > +           if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0))
> > > +                   device_set_node(&dev->adap.dev, child);
> > > +           else if ((name && strcmp("gpio", name)) == 0 ||
> > > +                                   (!ret && addr == 1))
> > > +                   dev->gc.fwnode = child;
> > > +   }
> >
> > Please, make addresses defined explicitly. You may also do it with node naming
> > schema:
> >
> > #define CP2112_I2C_ADR                0
> > #define CP2112_GPIO_ADR               1
> >
> > static const char * const cp2112_cell_names[] = {
> >       [CP2112_I2C_ADR]        = "i2c",
> >       [CP2112_GPIO_ADR]       = "gpio",
> > };
> >
> >       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;
> >               } else
> >                       ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> >               if (ret < 0)
> >                       ...error handling if needed...
> >
> >               switch (addr) {
> >               case CP2112_I2C_ADR:
> >                       device_set_node(&dev->adap.dev, child);
> >                       break;
> >               case CP2112_GPIO_ADR:
> >                       dev->gc.fwnode = child;
> >                       break;
> >               default:
> >                       ...error handling...
> >               }
> >       }
>
> Btw, don't you use "reg" property for the child nodes? It would be better from
> de facto used patterns (we have a couple of mode drivers that have a common
> code to read "reg" or _ADR() and that code can be split into a helper and used
> here).
>

Named nodes _seem_ to be preferred in DT for when there isn't a
logical / natural
numbering to the child nodes. A.e. for USB, reg is used to specify
which port, for I2C,
 which address on the bus, but for two parallel and independent
functions on the same
device, it seems named nodes would make more sense in DT. Many examples exist
in mainline where named nodes are used in DT in this way. One example
is network cards
which provide an mdio bus bind through the child "mdio". One example
of a specifically a
child i2c controller being bound to "i2c" can be found in
pine64,pinephone-keyboard.yaml.
But it's certainly possible this isn't the desired direction moving
forward in DT -- my opinion
should definitely be taken with a grain of salt. Maybe this is
something I should follow up on
with DT folks on that DT vs. ACPI thread made earlier.

One thing I did notice when looking at the mfd subsystem is that most
DT drivers actually
match on the compatible string of the child nodes, a.e.
"silabs,cp2112", "silabs,cp2112-gpio".
"silabs,cp2112-i2c". We could implement that here, but I think that
would make more sense if
we were to actually split the cp2112 into mfd & platform drivers, and
additionally split the DT
binding by function.

Thanks,

Danny Kaehn

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

[Andy Shevchenko]

On Mon, Mar 20, 2023 at 08:40:07AM -0500, Daniel Kaehn wrote:
> On Mon, Mar 20, 2023 at 8:00 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko wrote:
> > > On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote:

...

> > > > +   device_for_each_child_node(&hdev->dev, child) {
> > > > +           name = fwnode_get_name(child);
> > > > +           ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > > > +
> > > > +           if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0))
> > > > +                   device_set_node(&dev->adap.dev, child);
> > > > +           else if ((name && strcmp("gpio", name)) == 0 ||
> > > > +                                   (!ret && addr == 1))
> > > > +                   dev->gc.fwnode = child;
> > > > +   }
> > >
> > > Please, make addresses defined explicitly. You may also do it with node naming
> > > schema:
> > >
> > > #define CP2112_I2C_ADR                0
> > > #define CP2112_GPIO_ADR               1
> > >
> > > static const char * const cp2112_cell_names[] = {
> > >       [CP2112_I2C_ADR]        = "i2c",
> > >       [CP2112_GPIO_ADR]       = "gpio",
> > > };
> > >
> > >       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;
> > >               } else
> > >                       ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > >               if (ret < 0)
> > >                       ...error handling if needed...
> > >
> > >               switch (addr) {
> > >               case CP2112_I2C_ADR:
> > >                       device_set_node(&dev->adap.dev, child);
> > >                       break;
> > >               case CP2112_GPIO_ADR:
> > >                       dev->gc.fwnode = child;
> > >                       break;
> > >               default:
> > >                       ...error handling...
> > >               }
> > >       }
> >
> > Btw, don't you use "reg" property for the child nodes? It would be better from
> > de facto used patterns (we have a couple of mode drivers that have a common
> > code to read "reg" or _ADR() and that code can be split into a helper and used
> > here).
> >
> 
> Named nodes _seem_ to be preferred in DT for when there isn't a logical /
> natural numbering to the child nodes. A.e. for USB, reg is used to specify
> which port, for I2C, which address on the bus, but for two parallel and
> independent functions on the same device, it seems named nodes would make
> more sense in DT. Many examples exist in mainline where named nodes are used
> in DT in this way.

Okay, I'm not an expert in the DT preferable schemas, so I believe DT people
should answer on this.

> One example is network cards which provide an mdio bus
> bind through the child "mdio". One example of a specifically a
> child i2c controller being bound to "i2c" can be found in
> pine64,pinephone-keyboard.yaml.
> But it's certainly possible this isn't the desired direction moving forward
> in DT -- my opinion should definitely be taken with a grain of salt. Maybe
> this is something I should follow up on with DT folks on that DT vs. ACPI
> thread made earlier.
> 
> One thing I did notice when looking at the mfd subsystem is that most DT
> drivers actually match on the compatible string of the child nodes, a.e.
> "silabs,cp2112", "silabs,cp2112-gpio".  "silabs,cp2112-i2c". We could
> implement that here, but I think that would make more sense if we were to
> actually split the cp2112 into mfd & platform drivers, and additionally split
> the DT binding by function.

IIRC (but might be very well mistaken) the compatible strings for children
are discouraged.

-- 
With Best Regards,
Andy Shevchenko

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

[Daniel Kaehn]

On Mon, Mar 20, 2023 at 9:10 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Mar 20, 2023 at 08:40:07AM -0500, Daniel Kaehn wrote:
> > On Mon, Mar 20, 2023 at 8:00 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko wrote:
> > > > On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote:
>
> ...
>
> > > > > +   device_for_each_child_node(&hdev->dev, child) {
> > > > > +           name = fwnode_get_name(child);
> > > > > +           ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > > > > +
> > > > > +           if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0))
> > > > > +                   device_set_node(&dev->adap.dev, child);
> > > > > +           else if ((name && strcmp("gpio", name)) == 0 ||
> > > > > +                                   (!ret && addr == 1))
> > > > > +                   dev->gc.fwnode = child;
> > > > > +   }
> > > >
> > > > Please, make addresses defined explicitly. You may also do it with node naming
> > > > schema:
> > > >
> > > > #define CP2112_I2C_ADR                0
> > > > #define CP2112_GPIO_ADR               1
> > > >
> > > > static const char * const cp2112_cell_names[] = {
> > > >       [CP2112_I2C_ADR]        = "i2c",
> > > >       [CP2112_GPIO_ADR]       = "gpio",
> > > > };
> > > >
> > > >       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;
> > > >               } else
> > > >                       ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > > >               if (ret < 0)
> > > >                       ...error handling if needed...
> > > >
> > > >               switch (addr) {
> > > >               case CP2112_I2C_ADR:
> > > >                       device_set_node(&dev->adap.dev, child);
> > > >                       break;
> > > >               case CP2112_GPIO_ADR:
> > > >                       dev->gc.fwnode = child;
> > > >                       break;
> > > >               default:
> > > >                       ...error handling...
> > > >               }
> > > >       }
> > >
> > > Btw, don't you use "reg" property for the child nodes? It would be better from
> > > de facto used patterns (we have a couple of mode drivers that have a common
> > > code to read "reg" or _ADR() and that code can be split into a helper and used
> > > here).
> > >
> >
> > Named nodes _seem_ to be preferred in DT for when there isn't a logical /
> > natural numbering to the child nodes. A.e. for USB, reg is used to specify
> > which port, for I2C, which address on the bus, but for two parallel and
> > independent functions on the same device, it seems named nodes would make
> > more sense in DT. Many examples exist in mainline where named nodes are used
> > in DT in this way.
>
> Okay, I'm not an expert in the DT preferable schemas, so I believe DT people
> should answer on this.
>

Hello,

Thanks for all the time spent reviewing this thus far. Following up to
see what my next steps might be.

It sounds like we might want some DT folks to weigh in on the strategy
used for identifying the child I2C and GPIO nodes for the CP2112
device before moving further toward applying this.

Since the DT list is on this thread (as well as Rob+Krzystof), and
this has sat for a little while, I'm assuming that the ball is in my
court to seek out an answer/opinion here. (I know folks get a lot of
email, so apologies if the correct move would have been to wait a bit
longer before following up! Not intending to be rude.)

Would it be appropriate / expected that I send a separate email thread
to the DT mailing list on their opinion here? Or would that create
more confusion/complexity in adding yet another thread? I did create a
separate email thread for the initial DT vs. ACPI conversation we had
about accessing children by name or index in a unified way due to the
differences in upper/lower case and use-cases, but that
(understandably) didn't seem to gain any traction.

Thanks for any insights!

Thanks,
Danny Kaehn

> > One example is network cards which provide an mdio bus
> > bind through the child "mdio". One example of a specifically a
> > child i2c controller being bound to "i2c" can be found in
> > pine64,pinephone-keyboard.yaml.
> > But it's certainly possible this isn't the desired direction moving forward
> > in DT -- my opinion should definitely be taken with a grain of salt. Maybe
> > this is something I should follow up on with DT folks on that DT vs. ACPI
> > thread made earlier.
> >
> > One thing I did notice when looking at the mfd subsystem is that most DT
> > drivers actually match on the compatible string of the child nodes, a.e.
> > "silabs,cp2112", "silabs,cp2112-gpio".  "silabs,cp2112-i2c". We could
> > implement that here, but I think that would make more sense if we were to
> > actually split the cp2112 into mfd & platform drivers, and additionally split
> > the DT binding by function.
>
> IIRC (but might be very well mistaken) the compatible strings for children
> are discouraged.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

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

[Andy Shevchenko]

+Cc: Niyas, who is working a lot on filling the gaps in ACPI in comparison
     to DT in the Linux kernel. Perhaps he has some ideas or even better
     solutions.

On Mon, May 01, 2023 at 06:35:44PM -0500, Daniel Kaehn wrote:
> On Mon, Mar 20, 2023 at 9:10 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Mar 20, 2023 at 08:40:07AM -0500, Daniel Kaehn wrote:
> > > On Mon, Mar 20, 2023 at 8:00 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko wrote:
> > > > > On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote:

...

> > > > > > +   device_for_each_child_node(&hdev->dev, child) {
> > > > > > +           name = fwnode_get_name(child);
> > > > > > +           ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > > > > > +
> > > > > > +           if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0))
> > > > > > +                   device_set_node(&dev->adap.dev, child);
> > > > > > +           else if ((name && strcmp("gpio", name)) == 0 ||
> > > > > > +                                   (!ret && addr == 1))
> > > > > > +                   dev->gc.fwnode = child;
> > > > > > +   }
> > > > >
> > > > > Please, make addresses defined explicitly. You may also do it with node naming
> > > > > schema:
> > > > >
> > > > > #define CP2112_I2C_ADR                0
> > > > > #define CP2112_GPIO_ADR               1
> > > > >
> > > > > static const char * const cp2112_cell_names[] = {
> > > > >       [CP2112_I2C_ADR]        = "i2c",
> > > > >       [CP2112_GPIO_ADR]       = "gpio",
> > > > > };
> > > > >
> > > > >       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;
> > > > >               } else
> > > > >                       ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > > > >               if (ret < 0)
> > > > >                       ...error handling if needed...
> > > > >
> > > > >               switch (addr) {
> > > > >               case CP2112_I2C_ADR:
> > > > >                       device_set_node(&dev->adap.dev, child);
> > > > >                       break;
> > > > >               case CP2112_GPIO_ADR:
> > > > >                       dev->gc.fwnode = child;
> > > > >                       break;
> > > > >               default:
> > > > >                       ...error handling...
> > > > >               }
> > > > >       }
> > > >
> > > > Btw, don't you use "reg" property for the child nodes? It would be better from
> > > > de facto used patterns (we have a couple of mode drivers that have a common
> > > > code to read "reg" or _ADR() and that code can be split into a helper and used
> > > > here).
> > >
> > > Named nodes _seem_ to be preferred in DT for when there isn't a logical /
> > > natural numbering to the child nodes. A.e. for USB, reg is used to specify
> > > which port, for I2C, which address on the bus, but for two parallel and
> > > independent functions on the same device, it seems named nodes would make
> > > more sense in DT. Many examples exist in mainline where named nodes are used
> > > in DT in this way.
> >
> > Okay, I'm not an expert in the DT preferable schemas, so I believe DT people
> > should answer on this.
> 
> Hello,
> 
> Thanks for all the time spent reviewing this thus far. Following up to
> see what my next steps might be.
> 
> It sounds like we might want some DT folks to weigh in on the strategy
> used for identifying the child I2C and GPIO nodes for the CP2112
> device before moving further toward applying this.
> 
> Since the DT list is on this thread (as well as Rob+Krzystof), and
> this has sat for a little while, I'm assuming that the ball is in my
> court to seek out an answer/opinion here. (I know folks get a lot of
> email, so apologies if the correct move would have been to wait a bit
> longer before following up! Not intending to be rude.)
> 
> Would it be appropriate / expected that I send a separate email thread
> to the DT mailing list on their opinion here? Or would that create
> more confusion/complexity in adding yet another thread? I did create a
> separate email thread for the initial DT vs. ACPI conversation we had
> about accessing children by name or index in a unified way due to the
> differences in upper/lower case and use-cases, but that
> (understandably) didn't seem to gain any traction.
> 
> Thanks for any insights!
> 
> Thanks,
> Danny Kaehn
> 
> > > One example is network cards which provide an mdio bus
> > > bind through the child "mdio". One example of a specifically a
> > > child i2c controller being bound to "i2c" can be found in
> > > pine64,pinephone-keyboard.yaml.
> > > But it's certainly possible this isn't the desired direction moving forward
> > > in DT -- my opinion should definitely be taken with a grain of salt. Maybe
> > > this is something I should follow up on with DT folks on that DT vs. ACPI
> > > thread made earlier.
> > >
> > > One thing I did notice when looking at the mfd subsystem is that most DT
> > > drivers actually match on the compatible string of the child nodes, a.e.
> > > "silabs,cp2112", "silabs,cp2112-gpio".  "silabs,cp2112-i2c". We could
> > > implement that here, but I think that would make more sense if we were to
> > > actually split the cp2112 into mfd & platform drivers, and additionally split
> > > the DT binding by function.
> >
> > IIRC (but might be very well mistaken) the compatible strings for children
> > are discouraged.

-- 
With Best Regards,
Andy Shevchenko

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

[Danny Kaehn]

Hello folks, wanted to give one more follow up on this
patch/discussion. Would a reasonable next step for me
to help nudge this forward be to submit a v10 addressing
Andy's most recent code comments? Again hoping I'm not being
rude or stepping on toes; just want to make sure I'm doing my
dilligence to move things forward. I'll assume that going ahead
and submitting a v10 with unresolved discussion here isn't a
terrible offense if I don't end up getting a response here in 
the next week or so.

Leaving some links to some of the more key points of the discussion
across the versions of this patch, since it's been ~5 months since
the last activity here.

Discussion began with discussion of using child nodes by name
across DT with ACPI, for binding fwnodes for the CP2112's I2C
and GPIO controllers; since  ACPI requires uppercase names (and
names should specifically not be meaningful in ACPI)
https://lore.kernel.org/all/Y%2F9oO1AE6GK6CQmp@smile.fi.intel.com/

Andy suggested I use _ADR to identify the child node by index
for ACPI
https://lore.kernel.org/all/ZAi4NjqXTbLpVhPo@smile.fi.intel.com/

v9 implemented matching by child node name OR by address depnding
on the type of firmware used
https://lore.kernel.org/all/20230319204802.1364-4-kaehndan@gmail.com/

Some additional discussion on whether matching child nodes by name
is the best approach even for the DT side
(also within the in-line body of this email)
https://lore.kernel.org/all/ZBhoHzTr5l38u%2FkX@smile.fi.intel.com/


The DT binding patch in question
https://lore.kernel.org/all/20230319204802.1364-2-kaehndan@gmail.com/

Thanks,

Danny Kaehn




On Mon, Jul 3 2023 at 13:57:22 +0300 Andy Shevchenko write:
> On Mon, May 01, 2023 at 06:35:44PM -0500, Daniel Kaehn wrote:
> > On Mon, Mar 20, 2023 at 9:10 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Mar 20, 2023 at 08:40:07AM -0500, Daniel Kaehn wrote:
> > > > On Mon, Mar 20, 2023 at 8:00 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko
Wrote:
> > > > > > On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn
wrote:
> +Cc: Niyas, who is working a lot on filling the gaps in ACPI in
comparison
>      to DT in the Linux kernel. Perhaps he has some ideas or even
better
>      solutions.
> 
> 
> ...
> 
> > > > > > > +   device_for_each_child_node(&hdev->dev, child) {
> > > > > > > +           name = fwnode_get_name(child);
> > > > > > > +           ret =
acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > > > > > > +
> > > > > > > +           if ((name && strcmp("i2c", name) == 0) ||
(!ret && addr == 0))
> > > > > > > +                   device_set_node(&dev->adap.dev,
child);
> > > > > > > +           else if ((name && strcmp("gpio", name)) == 0
||
> > > > > > > +                                   (!ret && addr == 1))
> > > > > > > +                   dev->gc.fwnode = child;
> > > > > > > +   }
> > > > > >
> > > > > > Please, make addresses defined explicitly. You may also do
it with node naming
> > > > > > schema:
> > > > > >
> > > > > > #define CP2112_I2C_ADR                0
> > > > > > #define CP2112_GPIO_ADR               1
> > > > > >
> > > > > > static const char * const cp2112_cell_names[] = {
> > > > > >       [CP2112_I2C_ADR]        = "i2c",
> > > > > >       [CP2112_GPIO_ADR]       = "gpio",
> > > > > > };
> > > > > >
> > > > > >       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;
> > > > > >               } else
> > > > > >                       ret =
acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > > > > >               if (ret < 0)
> > > > > >                       ...error handling if needed...
> > > > > >
> > > > > >               switch (addr) {
> > > > > >               case CP2112_I2C_ADR:
> > > > > >                       device_set_node(&dev->adap.dev,
child);
> > > > > >                       break;
> > > > > >               case CP2112_GPIO_ADR:
> > > > > >                       dev->gc.fwnode = child;
> > > > > >                       break;
> > > > > >               default:
> > > > > >                       ...error handling...
> > > > > >               }
> > > > > >       }
> > > > >
> > > > > Btw, don't you use "reg" property for the child nodes? It
would be better from
> > > > > de facto used patterns (we have a couple of mode drivers that
have a common
> > > > > code to read "reg" or _ADR() and that code can be split into
a helper and used
> > > > > here).
> > > >
> > > > Named nodes _seem_ to be preferred in DT for when there isn't a
logical /
> > > > natural numbering to the child nodes. A.e. for USB, reg is used
to specify
> > > > which port, for I2C, which address on the bus, but for two
parallel and
> > > > independent functions on the same device, it seems named nodes
would make
> > > > more sense in DT. Many examples exist in mainline where named
nodes are used
> > > > in DT in this way.
> > >
> > > Okay, I'm not an expert in the DT preferable schemas, so I
believe DT people
> > > should answer on this.
> > 
> > Hello,
> > 
> > Thanks for all the time spent reviewing this thus far. Following up
to
> > see what my next steps might be.
> > 
> > It sounds like we might want some DT folks to weigh in on the
strategy
> > used for identifying the child I2C and GPIO nodes for the CP2112
> > device before moving further toward applying this.
> > 
> > Since the DT list is on this thread (as well as Rob+Krzystof), and
> > this has sat for a little while, I'm assuming that the ball is in
my
> > court to seek out an answer/opinion here. (I know folks get a lot
of
> > email, so apologies if the correct move would have been to wait a
bit
> > longer before following up! Not intending to be rude.)
> > 
> > Would it be appropriate / expected that I send a separate email
thread
> > to the DT mailing list on their opinion here? Or would that create
> > more confusion/complexity in adding yet another thread? I did
create a
> > separate email thread for the initial DT vs. ACPI conversation we
had
> > about accessing children by name or index in a unified way due to
the
> > differences in upper/lower case and use-cases, but that
> > (understandably) didn't seem to gain any traction.
> > 
> > Thanks for any insights!
> > 
> > Thanks,
> > Danny Kaehn
> > 
> > > > One example is network cards which provide an mdio bus
> > > > bind through the child "mdio". One example of a specifically a
> > > > child i2c controller being bound to "i2c" can be found in
> > > > pine64,pinephone-keyboard.yaml.
> > > > But it's certainly possible this isn't the desired direction
moving forward
> > > > in DT -- my opinion should definitely be taken with a grain of
salt. Maybe
> > > > this is something I should follow up on with DT folks on that
DT vs. ACPI
> > > > thread made earlier.
> > > >
> > > > One thing I did notice when looking at the mfd subsystem is
that most DT
> > > > drivers actually match on the compatible string of the child
nodes, a.e.
> > > > "silabs,cp2112", "silabs,cp2112-gpio".  "silabs,cp2112-i2c". We
could
> > > > implement that here, but I think that would make more sense if
we were to
> > > > actually split the cp2112 into mfd & platform drivers, and
additionally split
> > > > the DT binding by function.
> > >
> > > IIRC (but might be very well mistaken) the compatible strings for
children
> > > are discouraged.
> 

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

[Benjamin Tissoires]

Hi Danny,

On Jan 17 2024, Danny Kaehn wrote:
> Hello folks, wanted to give one more follow up on this
> patch/discussion. Would a reasonable next step for me
> to help nudge this forward be to submit a v10 addressing
> Andy's most recent code comments? Again hoping I'm not being
> rude or stepping on toes; just want to make sure I'm doing my
> dilligence to move things forward. I'll assume that going ahead
> and submitting a v10 with unresolved discussion here isn't a
> terrible offense if I don't end up getting a response here in 
> the next week or so.

Submitting a v10, even if there are still undecided points is definitely
the way forward. People probably have forgot a lot of things there and
need a refresh on the latest state of it :)

> 
> Leaving some links to some of the more key points of the discussion
> across the versions of this patch, since it's been ~5 months since
> the last activity here.
> 
> Discussion began with discussion of using child nodes by name
> across DT with ACPI, for binding fwnodes for the CP2112's I2C
> and GPIO controllers; since  ACPI requires uppercase names (and
> names should specifically not be meaningful in ACPI)
> https://lore.kernel.org/all/Y%2F9oO1AE6GK6CQmp@smile.fi.intel.com/

I think the DT part is fine. Please resubmit it in v10, but probably
drop the previous rev-by and explicitly mention you did so after the
first '---' below your signed-off-by. This should re-trigger a review
from them. Things may have changed since last year, and having another
review would be beneficial IMO.

> 
> Andy suggested I use _ADR to identify the child node by index
> for ACPI
> https://lore.kernel.org/all/ZAi4NjqXTbLpVhPo@smile.fi.intel.com/

I think I still prefer the "_DSD" approach with the cell-names, but
OTOH, it's not like there is an official ACPI description for this, and
we can basically define whatever we want. So please go ahead with the
_ADR approach IMO, with a couple of changes:

- mention about that in the DT bindings documentation
- please add an enum with those 2 addresses (with kernel doc), to
  document it in the code and not have magic constants in your checks

> 
> v9 implemented matching by child node name OR by address depnding
> on the type of firmware used
> https://lore.kernel.org/all/20230319204802.1364-4-kaehndan@gmail.com/

See my 2 comments above.

FWIW, I think 2/3 could go directly in as well, but the timing is not
ideal, we are in the middle of the Merge Window.

> 
> Some additional discussion on whether matching child nodes by name
> is the best approach even for the DT side
> (also within the in-line body of this email)
> https://lore.kernel.org/all/ZBhoHzTr5l38u%2FkX@smile.fi.intel.com/

Honestly, not sure we'll have too many users on the ACPI side (besides
myself). So if you really feel uncomfortable, you can always put a
warning that we are using _ADR in the ACPI world as a fallback, but that
we might revisit that in the future (with naming, if we reach to an
agreement).

Cheers,
Benjamin

> 
> 
> The DT binding patch in question
> https://lore.kernel.org/all/20230319204802.1364-2-kaehndan@gmail.com/
> 
> Thanks,
> 
> Danny Kaehn
> 
> 
> 
> 
> On Mon, Jul 3 2023 at 13:57:22 +0300 Andy Shevchenko write:
> > On Mon, May 01, 2023 at 06:35:44PM -0500, Daniel Kaehn wrote:
> > > On Mon, Mar 20, 2023 at 9:10 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Mar 20, 2023 at 08:40:07AM -0500, Daniel Kaehn wrote:
> > > > > On Mon, Mar 20, 2023 at 8:00 AM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko
> Wrote:
> > > > > > > On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn
> wrote:
> > +Cc: Niyas, who is working a lot on filling the gaps in ACPI in
> comparison
> >      to DT in the Linux kernel. Perhaps he has some ideas or even
> better
> >      solutions.
> > 
> > 
> > ...
> > 
> > > > > > > > +   device_for_each_child_node(&hdev->dev, child) {
> > > > > > > > +           name = fwnode_get_name(child);
> > > > > > > > +           ret =
> acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > > > > > > > +
> > > > > > > > +           if ((name && strcmp("i2c", name) == 0) ||
> (!ret && addr == 0))
> > > > > > > > +                   device_set_node(&dev->adap.dev,
> child);
> > > > > > > > +           else if ((name && strcmp("gpio", name)) == 0
> ||
> > > > > > > > +                                   (!ret && addr == 1))
> > > > > > > > +                   dev->gc.fwnode = child;
> > > > > > > > +   }
> > > > > > >
> > > > > > > Please, make addresses defined explicitly. You may also do
> it with node naming
> > > > > > > schema:
> > > > > > >
> > > > > > > #define CP2112_I2C_ADR                0
> > > > > > > #define CP2112_GPIO_ADR               1
> > > > > > >
> > > > > > > static const char * const cp2112_cell_names[] = {
> > > > > > >       [CP2112_I2C_ADR]        = "i2c",
> > > > > > >       [CP2112_GPIO_ADR]       = "gpio",
> > > > > > > };
> > > > > > >
> > > > > > >       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;
> > > > > > >               } else
> > > > > > >                       ret =
> acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > > > > > >               if (ret < 0)
> > > > > > >                       ...error handling if needed...
> > > > > > >
> > > > > > >               switch (addr) {
> > > > > > >               case CP2112_I2C_ADR:
> > > > > > >                       device_set_node(&dev->adap.dev,
> child);
> > > > > > >                       break;
> > > > > > >               case CP2112_GPIO_ADR:
> > > > > > >                       dev->gc.fwnode = child;
> > > > > > >                       break;
> > > > > > >               default:
> > > > > > >                       ...error handling...
> > > > > > >               }
> > > > > > >       }
> > > > > >
> > > > > > Btw, don't you use "reg" property for the child nodes? It
> would be better from
> > > > > > de facto used patterns (we have a couple of mode drivers that
> have a common
> > > > > > code to read "reg" or _ADR() and that code can be split into
> a helper and used
> > > > > > here).
> > > > >
> > > > > Named nodes _seem_ to be preferred in DT for when there isn't a
> logical /
> > > > > natural numbering to the child nodes. A.e. for USB, reg is used
> to specify
> > > > > which port, for I2C, which address on the bus, but for two
> parallel and
> > > > > independent functions on the same device, it seems named nodes
> would make
> > > > > more sense in DT. Many examples exist in mainline where named
> nodes are used
> > > > > in DT in this way.
> > > >
> > > > Okay, I'm not an expert in the DT preferable schemas, so I
> believe DT people
> > > > should answer on this.
> > > 
> > > Hello,
> > > 
> > > Thanks for all the time spent reviewing this thus far. Following up
> to
> > > see what my next steps might be.
> > > 
> > > It sounds like we might want some DT folks to weigh in on the
> strategy
> > > used for identifying the child I2C and GPIO nodes for the CP2112
> > > device before moving further toward applying this.
> > > 
> > > Since the DT list is on this thread (as well as Rob+Krzystof), and
> > > this has sat for a little while, I'm assuming that the ball is in
> my
> > > court to seek out an answer/opinion here. (I know folks get a lot
> of
> > > email, so apologies if the correct move would have been to wait a
> bit
> > > longer before following up! Not intending to be rude.)
> > > 
> > > Would it be appropriate / expected that I send a separate email
> thread
> > > to the DT mailing list on their opinion here? Or would that create
> > > more confusion/complexity in adding yet another thread? I did
> create a
> > > separate email thread for the initial DT vs. ACPI conversation we
> had
> > > about accessing children by name or index in a unified way due to
> the
> > > differences in upper/lower case and use-cases, but that
> > > (understandably) didn't seem to gain any traction.
> > > 
> > > Thanks for any insights!
> > > 
> > > Thanks,
> > > Danny Kaehn
> > > 
> > > > > One example is network cards which provide an mdio bus
> > > > > bind through the child "mdio". One example of a specifically a
> > > > > child i2c controller being bound to "i2c" can be found in
> > > > > pine64,pinephone-keyboard.yaml.
> > > > > But it's certainly possible this isn't the desired direction
> moving forward
> > > > > in DT -- my opinion should definitely be taken with a grain of
> salt. Maybe
> > > > > this is something I should follow up on with DT folks on that
> DT vs. ACPI
> > > > > thread made earlier.
> > > > >
> > > > > One thing I did notice when looking at the mfd subsystem is
> that most DT
> > > > > drivers actually match on the compatible string of the child
> nodes, a.e.
> > > > > "silabs,cp2112", "silabs,cp2112-gpio".  "silabs,cp2112-i2c". We
> could
> > > > > implement that here, but I think that would make more sense if
> we were to
> > > > > actually split the cp2112 into mfd & platform drivers, and
> additionally split
> > > > > the DT binding by function.
> > > >
> > > > IIRC (but might be very well mistaken) the compatible strings for
> children
> > > > are discouraged.
> > 
>