Re: [PATCH 1/2] driver core: provide device_match_fwnode_ext()

[Rafael J. Wysocki]

On Fri, Feb 20, 2026 at 3:35 PM Bartosz Golaszewski <brgl@xxxxxxxxxx> wrote:
>
> On Fri, Feb 20, 2026 at 1:08 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> >
> > On Fri, Feb 20, 2026 at 12:25 PM Bartosz Golaszewski <brgl@xxxxxxxxxx> wrote:
> > >
> > > On Fri, 20 Feb 2026 08:36:58 +0100, Andy Shevchenko
> > > <andriy.shevchenko@xxxxxxxxxxxxxxx> said:
> > > > On Thu, Feb 19, 2026 at 04:21:59PM -0500, Bartosz Golaszewski wrote:
> > > >> On Thu, 19 Feb 2026 20:58:32 +0100, Andy Shevchenko
> > > >> <andriy.shevchenko@xxxxxxxxxxxxxxx> said:
> > > >> > On Thu, Feb 19, 2026 at 05:31:22PM +0100, Bartosz Golaszewski wrote:
> > > >> >> Provide an extended variant of device_match_fwnode() that also tries to
> > > >> >> match the device's secondary fwnode.
> > > >
> > > > ...
> > > >
> > > >> >> +int device_match_fwnode_ext(struct device *dev, const void *fwnode)
> > > >> >> +{
> > > >> >> + struct fwnode_handle *dev_node = dev_fwnode(dev);
> > > >> >
> > > >> >> + if (!fwnode)
> > > >> >
> > > >> > IS_ERR_OR_NULL()
> > > >> > If supplied @fwnode is secondary, it might be an error pointer.
> > > >>
> > > >> I mirrored existing device_match_fwnode(), should it be fixed too?
> > > >
> > > > The answer is "I don't know". Strictly speaking this should be done everywhere
> > > > in the generic code when we can't guarantee that fwnode that comes to the
> > > > function is pure NULL or valid one.
> > > >
> > > >> >> +         return 0;
> > > >> >
> > > >> >> + if (dev_node == fwnode)
> > > >> >> +         return 1;
> > > >> >> +
> > > >> >> + return fwnode_is_primary(dev_node) && dev_node->secondary == fwnode;
> > > >> >> +}
> > > >> >
> > > >> > I think we can refactor this.
> > > >> >
> > > >> >    struct fwnode_handle *node;
> > > >> >
> > > >> > // I would name it like this, because in 3 cases in drivers/base/property.c
> > > >> > // 2 with node and 1 with dev_node when the same API is called.
> > > >>
> > > >> Haystack's node is "node" and the needle is "fwnode"? Seems confusing to me.
> > > >
> > > > But we need some consistency. drivers/base/property.c is inconsistent to begin
> > > > with and here the code chose the least used one for unknown reasons to me.
> > > >
> > > > I'm fine with "node" that is inside the function.
> > > >
> > > >> >    if (IS_ERR(fwnode))
> > > >> >            return 0;
> > > >> >
> > > >> >    if (device_match_fwnode(dev, fwnode)) // NULL check is inside
> > > >> >            return 1;
> > > >>
> > > >> Yeah, and it too can be supplied a secondary fwnode. Let's say we resolve
> > > >> a reference to a secondary software node and try to lookup a GPIO through it,
> > > >> we'll end up with an IS_ERR() fwnode with existing code, right?
> > > >
> > > > I'm not sure I understood the use case you are trying to describe here.
> > > >
> > > > The very first check guarantees that fwnode is either NULL or valid one.
> > > > When it's a valid one, the comparison with error pointer will be false.
> > > > What did I miss?
> > > >
> > >
> > > I mean: device_match_fwnode() has a NULL check but not an IS_ERR() check and
> > > can be passed a secondary fwnode as argument and that can be -ENODEV. It will
> > > probably not fail terribly but is still incorrect.
> > >
> > > I was speaking about the existing implementation, not addressing your comments.
> > >
> > > >> >    node = dev_fwnode(dev);
> > > >> >
> > > >> >    return fwnode_is_primary(node) && node->secondary == fwnode; // NULL check is inside
> > > >> >
> > > >> >
> > > >> >> + if (!fwnode)
> > > >> >> +         return 0;
> > > >> >
> > > >> >> + if (dev_node == fwnode)
> > > >> >> +         return 1;
> > > >> >> +
> > > >> >> + return fwnode_is_primary(dev_node) && dev_node->secondary == fwnode;
> > > >> >> +}
> > > >
> > > > ...
> > > >
> > > >> >> +int device_match_fwnode_ext(struct device *dev, const void *fwnode);
> > > >> >
> > > >> > Perhaps ext --> or_secondary ?
> > > >>
> > > >> I thought about it but it would make it sound like it only matches the
> > > >> secondary to me. Maybe device_match_all_fwnodes()? Would be future-proof if
> > > >> we end up doing the linked list approach.
> > > >
> > > > Danilo proposed _full, but in my opinion it's not better than _ext unless you
> > > > know very deep how fwnode structure is designed. Same with _all. It's confusing.
> > > >
> > > > fwnode_or_secondary (the key part is "or") sounds more precise. But if you come
> > > > up with something else that makes less ambiguity I will be glad.
> > > >
> > >
> > > device_match_fwnode_or_secondary() sounds good to me but shouldn't we try to
> > > limit the propagation of the "secondary" token in namespaces if our goal is to
> > > get rid of the whole "secondary fwnode" concept?
> >
> > I'm a bit late to this, but here's some background.
> >
> > Secondary fwnodes were not intended for device matching in the first
> > place.  The idea was to use them as a secondary supply of device
> > properties that may be missing from the primary node (think of an ACPI
> > device object accompanied by a software node supplying properties that
> > cannot be derived from the former).  They could be added by a driver
> > after matching the primary node for the benefit of a generic
> > framework.
> >
> > IMV, the example with children inheriting the fwnode from their parent
> > where the user wants to add a different secondary node to each child
> > doesn't really match the picture described above.  At least it was not
> > anticipated.  The idea was to allow the parent's fwnode to be extended
> > and then (possibly) inherited.
> >
> > That's why the secondary fwnode pointer is there under the primary one.
> >
>
> That's evolution in practice for you. :) It turned out software nodes
> are a good alternative for platform data in board files or MFD
> sub-devices and can serve as the primary firmware node of a device.
> That's alright - we have a common fwnode interface and it works fine.
>
> > So all of this goes beyond the original anticipated use of secondary
> > fwnodes and it seems to be calling for a list of equivalent (not
> > primary and secondary) fwnodes in struct device, but then of course
> > there's the question about duplicate properties and whether or not the
> > fwnode used for driver binding should be preferred (I don't see why
> > not).
> >
>
> I think that was Andy's initial proposal: treat the DT or ACPI node as
> the primary "main" node and any software node as an additional source
> of properties. In the absence of "real" nodes, I propose to treat the
> first software node as the primary.

IMV what really matters is which fwnode is used for picking up the
driver because that's the one that holds the device identification
matching the driver in the first place, so whatever properties come
along with that identification should take precedence over properties
supplied by the other fwnodes.  Typically, that would be either a DT
or an ACPI (device) node, but it need not be any of those in
principle.  It need not be the first one either.

> > Until all of this is resolved, I wouldn't even add a generic helper
> > for matching secondary nodes.  I'd just put this code directly into
> > gpio_chip_match_by_fwnode() along with a big fat comment explaining
> > why exactly secondary nodes need to be taken into account there.
> >
>
> IMO that's a recipe for keeping it local forever but I don't mind it.

If there is a move toward replacing the fwnode pointer in struct
device with a list of fwnodes, that code will need to be updated
anyway and that would be the time to use a helper IMO.  Otherwise, it
may as well stay local.

> I think I proposed to add the helper because Andy suggested doing it
> in driver core.

I see.