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

[Danny Kaehn <danny.kaehn@plexus.com>]

Hi Andy,

Finally was able to perform the last tests needed before sending out
v13, but wanted to close the loop on your final questions from v12:

On Wed, Nov 26, 2025 at 11:23:11PM +0200, Andy Shevchenko wrote:
> 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.
>

Will attempt to see if I can get them to weigh in.

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

Finally got a chance to test this, attempting to apply SSDT overlays at
runtime via configfs to instantiate an I2C device on the CP2112's I2C
bus. I didn't dig terribly deep, but both with and without the fwnode
reference count bumped, the I2C device instantiation happened as
expected when loading the overlay. I'm sure there's more nuanced cases
and things to dig in here, but seems like leving those references
dropped is the right thing, as you say.



Thanks,

Danny Kaehn