Re: [PATCH v2 2/2] gpiolib: match secondary fwnode too in gpio_device_find_by_fwnode()

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Andy,

On Tue, Feb 24, 2026 at 11:43:39AM +0200, Andy Shevchenko wrote:
> On Tue, Feb 24, 2026 at 10:56:16AM +0200, Sakari Ailus wrote:
> > On Tue, Feb 24, 2026 at 09:47:57AM +0100, Bartosz Golaszewski wrote:
> > > On Mon, Feb 23, 2026 at 11:07 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> 
> ...
> 
> > > > > > >  static int gpio_chip_match_by_fwnode(struct gpio_chip *gc, const void *fwnode)
> > > > > > >  {
> > > > > > > -     return device_match_fwnode(&gc->gpiodev->dev, fwnode);
> > > > > > > +     struct device *dev = &gc->gpiodev->dev;
> > > > > > > +     struct fwnode_handle *node = dev_fwnode(dev);
> > > > > > > +
> > > > > > > +     if (IS_ERR(fwnode))
> > > > > > > +             return 0;
> > > > > > > +
> > > > > > > +     if (device_match_fwnode(dev, fwnode))
> > > > > >
> > > > > > Could device_match_fwnode() match secondary fwnode as well?
> > > > >
> > > > > In the previous discussion on this, Andy was against doing that due to
> > > > > the concern that it might introduce subtle bugs, which I agree with.
> > > >
> > > > Could you elaborate or provide an example?
> 
> I believe you ask me. Okay, the sophisticated case I have in mind is the
> intel_quark_i2c_gpio.c which provides a GPIO device with a list of children.
> 
> First of all, it seems broken as it rewrites the secondary link for the
> I²C device. (Which makes me think that we need to have a copy of the
> [primary] fwnode in the children devices of MFD, but I don't know how
> to refcount that properly). The gpiolib-acpi-core.c has a matching function
> via ACPI_HANDLE(). So it might be not affected by this.
> 
> What I don't know is USB Type-C and USB DWC3 code where it's much more
> complicated. And I'm not in a position to state that the change won't
> affect those.

Any idea who has the hardware in these cases? There aren't that many users
of this function out there and I think at some point we do need to fix
this.

What we could also do is that we add another function that only cares about
the very fwnode you have at hand, switch the dubious cases to use that and
have the proper function test both available fwnodes. That'd get us on the
right path to fix this eventually, if not now.

> 
> > > > The function has some 27 users although few are individual drivers.
> > > >
> > > > My understanding is that we only have the secondary fwnode for being able
> > > > to attach objects from different backend to the same node. The fwnode API
> > > > in the meantime generally tries to hide the existence of the secondary
> > > > fwnode; a rewrite (which ideally would have happened perhaps a few years
> > > > ago?) would probably make the fwnode a linked list instead so we'd lose
> > > > that secondary pointer in the process.
> > > 
> > > It already is a (singly) linked list. Ideally it would be a
> > 
> > With two entries at most.
> 
> There is no technical limitation based on the data type.

There aren't any, no, but the current implementation assumes this, and I
wouldn't change this without changing the data structure as well.

> 
> > > doubly-linked list moved into struct device with struct fwnode_handle
> > > having no concept of primary and secondary nodes.
> > 
> > I'd think we had that list in struct fwnode_handle, which will still
> > represent nodes. But let's see the details when someone gets to implement
> > it. :-)
> 
> In the case above single or double linked list doesn't solve the issue of
> the corrupted (parent) fwnode. We need also to have a siblings list so it
> looks more like a tree.
> 

-- 
Regards,

Sakari Ailus