Re: [PATCH v13 2/3] HID: cp2112: Fwnode Support

[Andy Shevchenko]

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