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 | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 803b883ae875..ea19b5cb58f9 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,27 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
+	if (is_acpi_device_node(dev_fwnode(&hdev->dev))) {
+		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 {
+		child = device_get_named_child_node(&hdev->dev, "i2c");
+		device_set_node(&dev->adap.dev, child);
+		fwnode_handle_put(child);
+	}
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "parse failed\n");

-- 
2.25.1

On Tue, Jan 27, 2026 at 08:47:49AM -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

_ADR equals to Zero

> gpio_chip will use the child with _ADR One. For DeviceTree, the

_ADR equals to One

> i2c_adapter will use the child with name "i2c", but the gpio_chip
> will share a firmware node with the CP2112.

...

Also it's interesting choice of capital letters in the Subject.

I would expect "...: Add fwnode support"

...

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

Probably you want to mention in the description of the enum (here) that
the assigned values are explicit since this is basically a protocol between
FW and OS. That's why we may not change this values without breaking
older firmware descriptions.

> + */
> +enum cp2112_child_acpi_cell_addrs {
> +	CP2112_I2C_ADR = 0,
> +	CP2112_GPIO_ADR = 1,
> +};

...

> +	if (is_acpi_device_node(dev_fwnode(&hdev->dev))) {

I'm wondering if we can avoid this (additional) check and use the result of one
of the branches.

> +		device_for_each_child_node(&hdev->dev, child) {

If we are still use the above check it will be dev_fwnode() duplication call,
so perhaps a temporary variable to collect the device's fwnode and use it
there, below (see below), and here as for

		fwnode_for_each_child_node()

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

I would still check if this is a proper (OF) node, in case we stick with the
ACPI check above. Because we might have swnode and if it triggers, it will be
really something unexpected.

	} else if (is_of_node(fwnode)) {


> +		child = device_get_named_child_node(&hdev->dev, "i2c");
> +		device_set_node(&dev->adap.dev, child);
> +		fwnode_handle_put(child);
> +	}

-- 
With Best Regards,
Andy Shevchenko

On Tue, Jan 27, 2026 at 10:06:27PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 27, 2026 at 08:47:49AM -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
> 
> _ADR equals to Zero
>
Ack, will change.

> > gpio_chip will use the child with _ADR One. For DeviceTree, the
> 
> _ADR equals to One
> 

Ack, will change.

> > i2c_adapter will use the child with name "i2c", but the gpio_chip
> > will share a firmware node with the CP2112.
> 
> ...
> 
> Also it's interesting choice of capital letters in the Subject.
> 
> I would expect "...: Add fwnode support"
> 

Ack, will change.

> ...
> 
> > +/**
> > + * 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
> 
> Probably you want to mention in the description of the enum (here) that
> the assigned values are explicit since this is basically a protocol between
> FW and OS. That's why we may not change this values without breaking
> older firmware descriptions.
> 
> > + */
> > +enum cp2112_child_acpi_cell_addrs {
> > +	CP2112_I2C_ADR = 0,
> > +	CP2112_GPIO_ADR = 1,
> > +};
> 
> ...
> 
> > +	if (is_acpi_device_node(dev_fwnode(&hdev->dev))) {
> 
> I'm wondering if we can avoid this (additional) check and use the result of one
> of the branches.
>

Meaning something like using the result of acpi_get_local_address() to
determine whether the node is ACPI vs. not? That is what it used to do,
before I needed to switch to different schemas for DT vs. ACPI. Now, it
doesn't really make sense to use the child node types to determine
whether the GPIO node is shared, but still possible if we store a bool
result from the *_for_each_child_node() loop, but needs more complex
logic to store that based on each child's type (and the loop is fully
unnecessary for the non-ACPI case anyways).


Following the discussion on the DT binding thread, do you still want
ACPI to follow this different schema with the separate GPIO child node,
or would you prefer to unify them?

> > +		device_for_each_child_node(&hdev->dev, child) {
> 
> If we are still use the above check it will be dev_fwnode() duplication call,
> so perhaps a temporary variable to collect the device's fwnode and use it
> there, below (see below), and here as for
> 
> 		fwnode_for_each_child_node()
>

Makes sense, will update. I initially assumed we wanted to use the
"device_*" API wherever possible.

> > +			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 {
> 
> I would still check if this is a proper (OF) node, in case we stick with the
> ACPI check above. Because we might have swnode and if it triggers, it will be
> really something unexpected.
>
> 	} else if (is_of_node(fwnode)) {
>

Wouldn't it be valid to use software nodes to describe the
CP2112's functions? Is there any reason to intentionally prevent that?

>
> > +		child = device_get_named_child_node(&hdev->dev, "i2c");
> > +		device_set_node(&dev->adap.dev, child);
> > +		fwnode_handle_put(child);
> > +	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Thanks,

Danny Kaehn

On Thu, Jan 29, 2026 at 12:36:50PM -0600, Danny Kaehn wrote:
> On Tue, Jan 27, 2026 at 10:06:27PM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 27, 2026 at 08:47:49AM -0600, Danny Kaehn wrote:

...

> > I'm wondering if we can avoid this (additional) check and use the result of one
> > of the branches.
> 
> Meaning something like using the result of acpi_get_local_address() to
> determine whether the node is ACPI vs. not? That is what it used to do,
> before I needed to switch to different schemas for DT vs. ACPI. Now, it
> doesn't really make sense to use the child node types to determine
> whether the GPIO node is shared, but still possible if we store a bool
> result from the *_for_each_child_node() loop, but needs more complex
> logic to store that based on each child's type (and the loop is fully
> unnecessary for the non-ACPI case anyways).
> 
> Following the discussion on the DT binding thread, do you still want
> ACPI to follow this different schema with the separate GPIO child node,
> or would you prefer to unify them?

Wouldn't it be a bit messy if we combine main Device object with the GPIO
and leave I²C as a separate node? Besides that it seems already established
practice to have GPIO + I²C controllers separated based on _ADR (see Intel
Galileo case, drivers/mfd/intel_quark_i2c_gpio.c). Even if we can combine
I prefer to use the existing schema to have less animals in the zoo, for
the consistency's sake.

...

> > > +		device_for_each_child_node(&hdev->dev, child) {
> > 
> > If we are still use the above check it will be dev_fwnode() duplication call,
> > so perhaps a temporary variable to collect the device's fwnode and use it
> > there, below (see below), and here as for
> > 
> > 		fwnode_for_each_child_node()
> 
> 
> Makes sense, will update. I initially assumed we wanted to use the
> "device_*" API wherever possible.

Yes, but use a common sense. If we have fwnode already available, why should we
still use device_*()?

> > > +			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 {
> > 
> > I would still check if this is a proper (OF) node, in case we stick with the
> > ACPI check above. Because we might have swnode and if it triggers, it will be
> > really something unexpected.
> >
> > 	} else if (is_of_node(fwnode)) {
> 
> Wouldn't it be valid to use software nodes to describe the
> CP2112's functions? Is there any reason to intentionally prevent that?

swnode:s are for quirks. I hope in this case we won't see them IRL.
In any case, let's enable them when we will have the case.

> > > +		child = device_get_named_child_node(&hdev->dev, "i2c");
> > > +		device_set_node(&dev->adap.dev, child);
> > > +		fwnode_handle_put(child);
> > > +	}

-- 
With Best Regards,
Andy Shevchenko