| Message ID | cover.1741610847.git.mazziesaccount@gmail.com |
|---|---|
| Headers | show |
| Series | Support ROHM BD79124 ADC | expand |
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@linux.intel.com> 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@linux.intel.com> --- 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)
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. > 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. You also may update .clang-format.
> > > + 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 Unless this cycle goes long (i.e. an rc8) very unlikely I'll sneak the series in now due to lack of time to soak in next. > > #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... Nice - first time I've seen this :) Jonathan