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
