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

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

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

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

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

-- 
2.25.1

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

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

...

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

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

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

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

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

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

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

> +	}

-- 
With Best Regards,
Andy Shevchenko

Hi Andy,

Thanks for the review!

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

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

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

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

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

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


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

Thanks, will do.

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

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

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

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

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

Thanks,

Danny Kaehn

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

...

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

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

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

...

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

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

> > > +			}

...

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

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

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

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

-- 
With Best Regards,
Andy Shevchenko