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

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

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@linux.intel.com> 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?

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

-- 
With Best Regards,
Andy Shevchenko