Re: [PATCH v8 02/10] property: Add functions to iterate named child

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

Hei Matti,

On Wed, Mar 19, 2025 at 08:02:24AM +0200, Matti Vaittinen wrote:
> Moro Sakari,
> 
> Thanks for the review.
> 
> On 18/03/2025 17:24, Sakari Ailus wrote:
> > Moi,
> > 
> > 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:
> > > 
> > > fwnode_for_each_child_node()
> > > 	if (fwnode_name_eq())
> > > 		...
> > > 
> > > can be found from a various drivers/subsystems. Adding a macro for this
> > > can simplify things a bit.
> > > 
> > > In a few cases the data from the found nodes is later added to an array,
> > > which is allocated based on the number of found nodes. One example of
> > > such use is the IIO subsystem's ADC channel nodes, where the relevant
> > > nodes are named as channel[@N].
> > > 
> > > Add helpers for iterating and counting device's sub-nodes with certain
> > > name instead of open-coding this in every user.
> > > 
> > > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
> > > ---
> > > Revision history:
> > > v7 => v8:
> > >   - Fix the example in fwnode_get_named_child_node_count() documentation
> > >     to use the fwnode_get_named_child_node_count() and not the
> > >     device_get_named_child_node_count()
> > >   - Fix the rest of the new macro's indentiations
> > > v6 => v7:
> > >   - Improve kerneldoc
> > >   - Inline device_get_named_child_node_count() and change it to call
> > >     fwnode_get_named_child_node_count() inside
> > >   - Fix indentiation of the new macros
> > > v5 => v6:
> > >   - Add helpers to also iterate through the nodes.
> > > v4 => v5:
> > >   - Use given name instead of string 'channel' when counting the nodes
> > >   - Add also fwnode_get_child_node_count_named() as suggested by Rob.
> > > v3 => v4:
> > >   - New patch as suggested by Jonathan, see discussion in:
> > > https://lore.kernel.org/lkml/20250223161338.5c896280@jic23-huawei/
> > > ---
> > >   drivers/base/property.c  | 27 +++++++++++++++++++++++++++
> > >   include/linux/property.h | 24 ++++++++++++++++++++++++
> > >   2 files changed, 51 insertions(+)
> > > 
> > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > index c1392743df9c..f42f32ff45fc 100644
> > > --- a/drivers/base/property.c
> > > +++ b/drivers/base/property.c
> > > @@ -945,6 +945,33 @@ unsigned int device_get_child_node_count(const struct device *dev)
> > >   }
> > >   EXPORT_SYMBOL_GPL(device_get_child_node_count);
> > > +/**
> > > + * fwnode_get_named_child_node_count - number of child nodes with given name
> > > + * @fwnode: Node which child nodes are counted.
> > > + * @name: String to match child node name against.
> > > + *
> > > + * Scan child nodes and count all the nodes with a specific name. Potential
> > > + * 'number' -ending after the 'at sign' for scanned names is ignored.
> > > + * E.g.::
> > > + *   fwnode_get_named_child_node_count(fwnode, "channel");
> > > + * would match all the nodes::
> > > + *   channel { }, channel@0 {}, channel@0xabba {}...
> > > + *
> > > + * Return: the number of child nodes with a matching name for a given device.
> > > + */
> > > +unsigned int fwnode_get_named_child_node_count(const struct fwnode_handle *fwnode,
> > > +					       const char *name)
> > > +{
> > > +	struct fwnode_handle *child;
> > > +	unsigned int count = 0;
> > > +
> > > +	fwnode_for_each_named_child_node(fwnode, child, name)
> > > +		count++;
> > > +
> > > +	return count;
> > > +}
> > > +EXPORT_SYMBOL_GPL(fwnode_get_named_child_node_count);
> > > +
> > >   bool device_dma_supported(const struct device *dev)
> > >   {
> > >   	return fwnode_call_bool_op(dev_fwnode(dev), device_dma_supported);
> > > diff --git a/include/linux/property.h b/include/linux/property.h
> > > index e214ecd241eb..a1856e6b714c 100644
> > > --- 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.

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.

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.

-- 
Terveisin,

Sakari Ailus