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

[Andy Shevchenko]

On Mon, Mar 10, 2025 at 02:55:53PM +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 a helpers for iterating and counting device's sub-nodes with certain
> name instead, of open-coding this in every user.

Almost good, I doubt we need the exported function for the device as it can be
derived from the fwnode_*() API by supplying dev_fwnode(dev).

Perhaps we want also this for the completeness (other comments are below):

 From f52dbbe97ff0cdf835eef29506e482433f0a50a9 Mon Sep 17 00:00:00 2001
 From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
 Date: Mon, 10 Mar 2025 16:20:35 +0200
 Subject: [PATCH 1/1] device property: Split fwnode_get_child_node_count()

The new helper is introduced to allow counting the child firmware nodes
of their parent without requiring a device to be passed. This also makes
the fwnode and device property API more symmetrical with the rest.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
---
 drivers/base/property.c  | 12 ++++++------
 include/linux/property.h |  7 ++++++-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index c1392743df9c..805f75b35115 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -928,22 +928,22 @@ bool fwnode_device_is_available(const struct fwnode_handle *fwnode)
 EXPORT_SYMBOL_GPL(fwnode_device_is_available);
 
 /**
- * device_get_child_node_count - return the number of child nodes for device
- * @dev: Device to count the child nodes for
+ * fwnode_get_child_node_count - return the number of child nodes for a given firmware node
+ * @fwnode: Pointer to the parent firmware node
  *
- * Return: the number of child nodes for a given device.
+ * Return: the number of child nodes for a given firmware node.
  */
-unsigned int device_get_child_node_count(const struct device *dev)
+unsigned int fwnode_get_child_node_count(const struct fwnode_handle *fwnode)
 {
 	struct fwnode_handle *child;
 	unsigned int count = 0;
 
-	device_for_each_child_node(dev, child)
+	fwnode_for_each_child_node(fwnode, child)
 		count++;
 
 	return count;
 }
-EXPORT_SYMBOL_GPL(device_get_child_node_count);
+EXPORT_SYMBOL_GPL(fwnode_get_child_node_count);
 
 bool device_dma_supported(const struct device *dev)
 {
diff --git a/include/linux/property.h b/include/linux/property.h
index e214ecd241eb..bc5bfc98176b 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -208,7 +208,12 @@ DEFINE_FREE(fwnode_handle, struct fwnode_handle *, fwnode_handle_put(_T))
 int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
 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_child_node_count(const struct fwnode_handle *fwnode);
+
+static inline unsigned int device_get_child_node_count(const struct device *dev)
+{
+	return fwnode_get_child_node_count(dev_fwnode(dev));
+}
 
 static inline int device_property_read_u8(const struct device *dev,
 					  const char *propname, u8 *val)

...

> Please note, the checkpatch.pl was not happy about the for_each...()
> macros. I tried to make them to follow the existing convention. I am
> open to suggestions how to improve.

checkpatch is known for false-positives and/or false-negatives.

...

> +/**
> + * 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. Return the

You already have a return section below. Perhaps rephrase somehow?

> + * number of found nodes. Potential '@number' -ending for scanned names is

Not sure how this "'@foo' -ending" part is being rendered in HTML/PDF...
Also kernel-doc might be not happy with the @ character followed by an
immediate text as it might be misinterpreted as a reference to a parameter.

> + * ignored. Eg,

E.g.,

(because it's the phrase in Latin: exempli gratia)

> + * device_get_child_node_count(dev, "channel");

Shift right to make sure it's not a text, but a code, better even to use reST
approach, i.e. (note additional leading space(s) and double colon):

 * ignored. E.g.::
 *
 *   fwnode_get_child_node_count(dev, "channel"); // you have a copy'n'paste typo
 *
 * would match all the nodes::
 *
 *   ...
 *

> + * would match all the nodes:
> + * channel { }, channel@0 {}, channel@0xabba {}...
> + *
> + * Return: the number of child nodes with a matching name for a given device.
> + */

...

> +/**
> + * device_get_named_child_node_count - number of child nodes with given name
> + * @dev: Device to count the child nodes for.
> + * @name: String to match child node name against.
> + *
> + * Scan device's child nodes and find all the nodes with a specific name and
> + * return the number of found nodes. Potential '@number' -ending for scanned
> + * names is ignored. Eg,
> + * device_get_child_node_count(dev, "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.

Similar comments as per above.

> + */

...

> +#define fwnode_for_each_named_child_node(fwnode, child, name)		\
> +		fwnode_for_each_child_node(fwnode, child)		\

One TAB too much.

> +			if (!fwnode_name_eq(child, name)) { } else

Ditto.

Note, I believe this won't get v6.15-rc1, so there will be for_each_if()
available and these will become

#define fwnode_for_each_named_child_node(fwnode, child, name)	\
	fwnode_for_each_child_node(fwnode, child)		\
		for_each_if(fwnode_name_eq(child, name))

and so on...

...

>  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);
> +unsigned int device_get_named_child_node_count(const struct device *dev,
> +					       const char *name);

I would add a blank line.

unsigned int device_get_child_node_count(const struct device *dev);
// here is a blank line
unsigned int device_get_named_child_node_count(const struct device *dev,
					       const char *name);
unsigned int fwnode_get_named_child_node_count(const struct fwnode_handle *fwnode,
					       const char *name);

But see above the proposed additional patch that you may include in your next
version.

>  static inline int device_property_read_u8(const struct device *dev,
>  					  const char *propname, u8 *val)

-- 
With Best Regards,
Andy Shevchenko