Message-ID: <20250330170749.7fe553cd@jic23-huawei>
Date: Sun, 30 Mar 2025 17:07:49 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, Matti Vaittinen
 <matti.vaittinen@...rohmeurope.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Daniel Scally <djrscally@...il.com>, Heikki Krogerus
 <heikki.krogerus@...ux.intel.com>, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 Danilo Krummrich <dakr@...nel.org>, Claudiu Manoil
 <claudiu.manoil@....com>, Andrew Lunn <andrew+netdev@...n.ch>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
 netdev@...r.kernel.org, Laurent Pinchart
 <laurent.pinchart@...asonboard.com>
Subject: Re: [PATCH v8 02/10] property: Add functions to iterate named child

On Thu, 20 Mar 2025 08:43:44 +0200
Matti Vaittinen <mazziesaccount@...il.com> wrote:

> On 19/03/2025 17:23, Sakari Ailus wrote:
> > On Wed, Mar 19, 2025 at 08:02:24AM +0200, Matti Vaittinen wrote:  
> >> On 18/03/2025 17:24, Sakari Ailus wrote:  
> >>> On Mon, Mar 17, 2025 at 05:50:38PM +0200, Matti Vaittinen wrote:  
> >>>> There are a few use-cases where child nodes with a specific name need to
> >>>> be parsed. Code like:  
> 
> ...
> 
> >>>> --- a/include/linux/property.h
> >>>> +++ b/include/linux/property.h
> >>>> @@ -167,10 +167,18 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
> >>>>    	for (child = fwnode_get_next_child_node(fwnode, NULL); child;	\
> >>>>    	     child = fwnode_get_next_child_node(fwnode, child))
> >>>> +#define fwnode_for_each_named_child_node(fwnode, child, name)		\
> >>>> +	fwnode_for_each_child_node(fwnode, child)			\
> >>>> +		if (!fwnode_name_eq(child, name)) { } else
> >>>> +
> >>>>    #define fwnode_for_each_available_child_node(fwnode, child)		       \
> >>>>    	for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\
> >>>>    	     child = fwnode_get_next_available_child_node(fwnode, child))
> >>>> +#define fwnode_for_each_available_named_child_node(fwnode, child, name)	\
> >>>> +	fwnode_for_each_available_child_node(fwnode, child)		\
> >>>> +		if (!fwnode_name_eq(child, name)) { } else
> >>>> +  
> >>>
> >>> OF only enumerates available nodes via the fwnode API, software nodes don't
> >>> have the concept but on ACPI I guess you could have a difference in nodes
> >>> where you have device sub-nodes that aren't available. Still, these ACPI
> >>> device nodes don't have meaningful names in this context (they're
> >>> 4-character object names) so you wouldn't use them like this anyway.  
> >>
> >> I believe you have far better understanding on these concepts than I do. The
> >> reason behind adding fwnode_for_each_available_child_node() was the patch
> >> 10/10:
> >>
> >> -	fwnode_for_each_available_child_node(sensors, node) {
> >> -		if (fwnode_name_eq(node, "sensor")) {
> >> -			if (!thp7312_sensor_parse_dt(thp7312, node))
> >> -				num_sensors++;
> >> -		}
> >> +	fwnode_for_each_available_named_child_node(sensors, node, "sensor") {
> >> +		if (!thp7312_sensor_parse_dt(thp7312, node))
> >> +			num_sensors++;
> >>   	}
> >>
> >>  
> >>> So my question is: is it useful to provide this besides
> >>> fwnode_for_each_named_child_node(), given that both are effectively the
> >>> same?  
> >>
> >> So, I suppose you're saying the existing thp7312 -driver has no real reason
> >> to use the 'fwnode_for_each_available_child_node()', but it could be using
> >> fwnode_for_each_child_node() instead?
> >>
> >> If so, I am Ok with dropping the
> >> 'fwnode_for_each_available_named_child_node()' and changing the 10/10 to:
> >>
> >> -	fwnode_for_each_available_child_node(sensors, node) {
> >> -		if (fwnode_name_eq(node, "sensor")) {
> >> -			if (!thp7312_sensor_parse_dt(thp7312, node))
> >> -				num_sensors++;
> >> -		}
> >> +	fwnode_for_each_named_child_node(sensors, node, "sensor") {
> >> +		if (!thp7312_sensor_parse_dt(thp7312, node))
> >> +			num_sensors++;
> >>   	}
> >>
> >> Do you think that'd be correct?  
> > 
> > I'd say so. Feel free to cc me to the last patch as well.  
> 
> Thanks. I'll drop the fwnode_for_each_available_named_child_node() then.
> 
> > I guess one way to make this clearer is to switch to
> > fwnode_for_each_child_node() in a separate patch before
> > fwnode_for_each_named_child_node() conversion.  
> 
> I suppose this makes sense.

this _available_ thing is ancient history that has tripped us up
many times before. I've very keen to not see another case sneaking
in.  Whether we can definitely 'fix' all existing cases is a different
question..

> 
> I think this series can't make it to 6.15-rc1. Meaning, these 
> *_named_*() APIs perhaps land in 6.16-rc1. I assume these *_named_*() 
> APIs will go through the IIO. This rather simple IIO driver's review 
> took longer than I predicted, with more versions I intended (as always) 
> - and I kind of dislike respinning the whole series, with this large 
> audience, when changes are not interesting to the most.
> 
> Maybe it is simplest to drop the thp7312 (and gianfar) from this series, 
> and respin them only when the 6.16-rc1 is out. It's going to be couple 
> of months though - so there's always a risk that I forget.
> 
> The proposed change for the thp7312, from 
> fwnode_for_each_available_child_node() to fwnode_for_each_child_node() 
> can be done earlier though.
> 
> > There are also just a handful of users of
> > fwnode_for_each_available_child_node() and I guess these could be
> > converted, too, but I think it's outside the scope of the set.  
> 
> Definitely not in the scope of the bd79124 support :)
Agreed. Break this series up however you like and entirely up to
you whether you do further cleanup of other bits of the kernel!

Jonathan

> 
> Yours,
> 	-- Matti

Powered by blists - more mailing lists