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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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