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

[Sakari Ailus]

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@xxxxxxxxxx>
> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>
> ---
> 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.

So my question is: is it useful to provide this besides
fwnode_for_each_named_child_node(), given that both are effectively the
same?

>  struct fwnode_handle *device_get_next_child_node(const struct device *dev,
>  						 struct fwnode_handle *child);
>  
> @@ -178,11 +186,19 @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev,
>  	for (child = device_get_next_child_node(dev, NULL); child;	\
>  	     child = device_get_next_child_node(dev, child))
>  
> +#define device_for_each_named_child_node(dev, child, name)		\
> +	device_for_each_child_node(dev, child)				\
> +		if (!fwnode_name_eq(child, name)) { } else
> +
>  #define device_for_each_child_node_scoped(dev, child)			\
>  	for (struct fwnode_handle *child __free(fwnode_handle) =	\
>  		device_get_next_child_node(dev, NULL);			\
>  	     child; child = device_get_next_child_node(dev, child))
>  
> +#define device_for_each_named_child_node_scoped(dev, child, name)	\
> +	device_for_each_child_node_scoped(dev, child)			\
> +		if (!fwnode_name_eq(child, name)) { } else
> +
>  struct fwnode_handle *fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
>  						  const char *childname);
>  struct fwnode_handle *device_get_named_child_node(const struct device *dev,
> @@ -210,6 +226,14 @@ int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);
>  
>  unsigned int device_get_child_node_count(const struct device *dev);
>  
> +unsigned int fwnode_get_named_child_node_count(const struct fwnode_handle *fwnode,
> +					       const char *name);
> +static inline unsigned int device_get_named_child_node_count(const struct device *dev,
> +							     const char *name)
> +{
> +	return fwnode_get_named_child_node_count(dev_fwnode(dev), name);
> +}
> +
>  static inline int device_property_read_u8(const struct device *dev,
>  					  const char *propname, u8 *val)
>  {

-- 
Terveisin,

Sakari Ailus