| Message ID | 29ec24f1498392cafbecc0e0c0e23e1ce3289565.1740421248.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support ROHM BD79124 ADC | expand |
Hi, > +/** > + * device_get_child_node_count_named - number of child nodes with given name > + * > + * 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 {}... > + * > + * @dev: Device to count the child nodes for > + * > + * Return: the number of child nodes with a matching name for a given device. > + */ > +unsigned int device_get_child_node_count_named(const struct device *dev, > + const char *name) > +{ > + struct fwnode_handle *child; > + unsigned int count = 0; > + > + device_for_each_child_node(dev, child) > + if (fwnode_name_eq(child, "channel")) s/"channel"/name/ ? > + count++; > + > + return count; > +} > +EXPORT_SYMBOL_GPL(device_get_child_node_count_named); I did not check how many users are you proposing for this, but if there's only one, then IMO this should not be a global function yet. It just feels to special case to me. But let's see what the others think. thanks,
On 25/02/2025 11:40, Heikki Krogerus wrote: > Hi, > >> +/** >> + * device_get_child_node_count_named - number of child nodes with given name >> + * >> + * 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 {}... >> + * >> + * @dev: Device to count the child nodes for >> + * >> + * Return: the number of child nodes with a matching name for a given device. >> + */ >> +unsigned int device_get_child_node_count_named(const struct device *dev, >> + const char *name) >> +{ >> + struct fwnode_handle *child; >> + unsigned int count = 0; >> + >> + device_for_each_child_node(dev, child) >> + if (fwnode_name_eq(child, "channel")) > > s/"channel"/name/ ? Thanks Heikki for spotting this brainfart! :) > >> + count++; >> + >> + return count; >> +} >> +EXPORT_SYMBOL_GPL(device_get_child_node_count_named); > > I did not check how many users are you proposing for this, but if > there's only one, then IMO this should not be a global function yet. I have no strong opinion on this. It starts with just 1 user (IIO ADC channel stuff), but I've a feeling there are other areas which do look-up nodes by name. I suppose "channels" are looked-up in other areas of IIO as well. Lookups are probably done outside the IIO as well. I haven't audited this, but I wouldn't be surprized if at least LEDs (and perhaps clks/regulators?) could find this useful too. > It just feels to special case to me. But let's see what the others > think. Yeah :) And thanks for spotting the "channel" -thing :) Yours, -- Matti
On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote: > > +/** > > + * device_get_child_node_count_named - number of child nodes with given name > > + * > > + * 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 {}... > > + * > > + * @dev: Device to count the child nodes for This has an inconsistent kernel doc structure in comparison to the rest in this file. > > + * Return: the number of child nodes with a matching name for a given device. > > + */ > > +unsigned int device_get_child_node_count_named(const struct device *dev, > > + const char *name) > > +{ > > + struct fwnode_handle *child; > > + unsigned int count = 0; > > + > > + device_for_each_child_node(dev, child) > > + if (fwnode_name_eq(child, "channel")) > > s/"channel"/name/ ? > > > + count++; > > + > > + return count; > > +} > > +EXPORT_SYMBOL_GPL(device_get_child_node_count_named); > > I did not check how many users are you proposing for this, but if > there's only one, then IMO this should not be a global function yet. > It just feels to special case to me. But let's see what the others > think. The problem is that if somebody hides it, we might potentially see a duplication in the future. So I _slightly_ prefer to publish and then drop that after a few cycles if no users appear. Also this misses the test cases.
On 25/02/2025 12:21, Andy Shevchenko wrote: > On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote: >>> +/** >>> + * device_get_child_node_count_named - number of child nodes with given name >>> + * >>> + * 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 {}... >>> + * >>> + * @dev: Device to count the child nodes for > > This has an inconsistent kernel doc structure in comparison to the rest in this > file. > >>> + * Return: the number of child nodes with a matching name for a given device. >>> + */ >>> +unsigned int device_get_child_node_count_named(const struct device *dev, >>> + const char *name) >>> +{ >>> + struct fwnode_handle *child; >>> + unsigned int count = 0; >>> + >>> + device_for_each_child_node(dev, child) >>> + if (fwnode_name_eq(child, "channel")) >> >> s/"channel"/name/ ? >> >>> + count++; >>> + >>> + return count; >>> +} >>> +EXPORT_SYMBOL_GPL(device_get_child_node_count_named); >> >> I did not check how many users are you proposing for this, but if >> there's only one, then IMO this should not be a global function yet. >> It just feels to special case to me. But let's see what the others >> think. > > The problem is that if somebody hides it, we might potentially see > a duplication in the future. So I _slightly_ prefer to publish and > then drop that after a few cycles if no users appear. After taking a very quick grep I spotted one other existing place where we might be able to do direct conversion to use this function. drivers/net/ethernet/freescale/gianfar.c That'd be 2 users. While I looked at it, it seems that a 'device_for_each_named_child_node()' -construct would have a few users. Yours, -- Matti
On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote: > On 25/02/2025 12:21, Andy Shevchenko wrote: > > On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote: > > > > +/** > > > > + * device_get_child_node_count_named - number of child nodes with given name > > > > + * > > > > + * 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 {}... > > > > + * > > > > + * @dev: Device to count the child nodes for > > > > This has an inconsistent kernel doc structure in comparison to the rest in this > > file. > > > > > > + * Return: the number of child nodes with a matching name for a given device. > > > > + */ > > > > +unsigned int device_get_child_node_count_named(const struct device *dev, > > > > + const char *name) > > > > +{ > > > > + struct fwnode_handle *child; > > > > + unsigned int count = 0; > > > > + > > > > + device_for_each_child_node(dev, child) > > > > + if (fwnode_name_eq(child, "channel")) > > > > > > s/"channel"/name/ ? > > > > > > > + count++; > > > > + > > > > + return count; > > > > +} > > > > +EXPORT_SYMBOL_GPL(device_get_child_node_count_named); > > > > > > I did not check how many users are you proposing for this, but if > > > there's only one, then IMO this should not be a global function yet. > > > It just feels to special case to me. But let's see what the others > > > think. > > > > The problem is that if somebody hides it, we might potentially see > > a duplication in the future. So I _slightly_ prefer to publish and > > then drop that after a few cycles if no users appear. > > After taking a very quick grep I spotted one other existing place where we > might be able to do direct conversion to use this function. > > drivers/net/ethernet/freescale/gianfar.c > > That'd be 2 users. I haven't checked myself, I believe your judgement, but can you add a (rfc?) patch at the end of this series to show that? With the luckily event of acking by the network people we may have it already done. > While I looked at it, it seems that a 'device_for_each_named_child_node()' > -construct would have a few users.
On 25/02/2025 12:39, Andy Shevchenko wrote: > On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote: >> On 25/02/2025 12:21, Andy Shevchenko wrote: >>> On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote: >>>>> +/** >>>>> + * device_get_child_node_count_named - number of child nodes with given name >>>>> + * >>>>> + * 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 {}... >>>>> + * >>>>> + * @dev: Device to count the child nodes for ... >>>> I did not check how many users are you proposing for this, but if >>>> there's only one, then IMO this should not be a global function yet. >>>> It just feels to special case to me. But let's see what the others >>>> think. >>> >>> The problem is that if somebody hides it, we might potentially see >>> a duplication in the future. So I _slightly_ prefer to publish and >>> then drop that after a few cycles if no users appear. >> >> After taking a very quick grep I spotted one other existing place where we >> might be able to do direct conversion to use this function. >> >> drivers/net/ethernet/freescale/gianfar.c >> >> That'd be 2 users. > > I haven't checked myself, I believe your judgement, but can you add a (rfc?) > patch at the end of this series to show that? With the luckily event of acking > by the network people we may have it already done. Sure. I can add a patch to gianfar when sending the v5 - if this new function is not completely NACK'd before that :) Yours, -- Matti
On 25/02/2025 12:21, Andy Shevchenko wrote: > On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote: >>> +/** >>> + * device_get_child_node_count_named - number of child nodes with given name >>> + * >>> + * 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 {}... >>> + * >>> + * @dev: Device to count the child nodes for > > This has an inconsistent kernel doc structure in comparison to the rest in this > file. Hmm. I'll take a look at the differences for v5. >>> + * Return: the number of child nodes with a matching name for a given device. >>> + */ >>> +unsigned int device_get_child_node_count_named(const struct device *dev, >>> + const char *name) >>> +{ >>> + struct fwnode_handle *child; >>> + unsigned int count = 0; >>> + >>> + device_for_each_child_node(dev, child) >>> + if (fwnode_name_eq(child, "channel")) >> >> s/"channel"/name/ ? >> >>> + count++; >>> + >>> + return count; >>> +} >>> +EXPORT_SYMBOL_GPL(device_get_child_node_count_named); >> >> I did not check how many users are you proposing for this, but if >> there's only one, then IMO this should not be a global function yet. >> It just feels to special case to me. But let's see what the others >> think. > > The problem is that if somebody hides it, we might potentially see > a duplication in the future. So I _slightly_ prefer to publish and > then drop that after a few cycles if no users appear. > > Also this misses the test cases. I'll also take a look at the tests, but I have a bit of an attitude problem what comes to unit testing. Adding tests for the sake of having tests just hinders the development. It makes improving functions less appealing (as tests need to be changed as well) and adds bunch of inertia & maintenance cost. Sure, on complex functions having tests increases the confidence that changes work - but I don't see much value here. Do we have tests for all the property.h functions? Yours, -- Matti
On 25/02/2025 12:39, Andy Shevchenko wrote: > On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote: >> On 25/02/2025 12:21, Andy Shevchenko wrote: >>> On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote: ... >>>> >>>> I did not check how many users are you proposing for this, but if >>>> there's only one, then IMO this should not be a global function yet. >>>> It just feels to special case to me. But let's see what the others >>>> think. >>> >>> The problem is that if somebody hides it, we might potentially see >>> a duplication in the future. So I _slightly_ prefer to publish and >>> then drop that after a few cycles if no users appear. >> >> After taking a very quick grep I spotted one other existing place where we >> might be able to do direct conversion to use this function. >> >> drivers/net/ethernet/freescale/gianfar.c >> >> That'd be 2 users. > > I haven't checked myself, I believe your judgement, I took a better look and you obviously shouldn't believe :) The gianfar used of_node instead of the fwnode. So, it'd be a single caller at starters. Yours, -- Matti
On Tue, Feb 25, 2025 at 03:29:17PM +0200, Matti Vaittinen wrote: > On 25/02/2025 12:39, Andy Shevchenko wrote: > > On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote: > > > On 25/02/2025 12:21, Andy Shevchenko wrote: > > > > On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote: ... > > > > > > > > > > I did not check how many users are you proposing for this, but if > > > > > there's only one, then IMO this should not be a global function yet. > > > > > It just feels to special case to me. But let's see what the others > > > > > think. > > > > > > > > The problem is that if somebody hides it, we might potentially see > > > > a duplication in the future. So I _slightly_ prefer to publish and > > > > then drop that after a few cycles if no users appear. > > > > > > After taking a very quick grep I spotted one other existing place where we > > > might be able to do direct conversion to use this function. > > > > > > drivers/net/ethernet/freescale/gianfar.c > > > > > > That'd be 2 users. > > > > I haven't checked myself, I believe your judgement, > > I took a better look and you obviously shouldn't believe :) The gianfar used > of_node instead of the fwnode. So, it'd be a single caller at starters. ...which is the same as dev_of_node(), which means that you can use your function there.
On 25/02/2025 15:59, Andy Shevchenko wrote: > On Tue, Feb 25, 2025 at 03:29:17PM +0200, Matti Vaittinen wrote: >> On 25/02/2025 12:39, Andy Shevchenko wrote: >>> On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote: >>>> On 25/02/2025 12:21, Andy Shevchenko wrote: >>>>> On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote: > > ... > >>>>>> >>>>>> I did not check how many users are you proposing for this, but if >>>>>> there's only one, then IMO this should not be a global function yet. >>>>>> It just feels to special case to me. But let's see what the others >>>>>> think. >>>>> >>>>> The problem is that if somebody hides it, we might potentially see >>>>> a duplication in the future. So I _slightly_ prefer to publish and >>>>> then drop that after a few cycles if no users appear. >>>> >>>> After taking a very quick grep I spotted one other existing place where we >>>> might be able to do direct conversion to use this function. >>>> >>>> drivers/net/ethernet/freescale/gianfar.c >>>> >>>> That'd be 2 users. >>> >>> I haven't checked myself, I believe your judgement, >> >> I took a better look and you obviously shouldn't believe :) The gianfar used >> of_node instead of the fwnode. So, it'd be a single caller at starters. > > ...which is the same as dev_of_node(), which means that you can use your > function there. I'm unsure what you mean. The proposed function device_get_child_node_count_named() takes device pointer. I don't see how dev_of_node() helps converting node to device? I think I could actually kill the whole gfar_of_group_count() function and replace it with a direct call to the device_get_child_node_count_named() - but I am not at all convinced that'd be worth including the property.h to a file which is currently using only of_* -stuff. Well, I suppose it can be asked from netdev peeps but I am not convinced they see it as a great idea. If I misunderstood your meaning - please elaborate. Yours, -- Matti
On Wed, Feb 26, 2025 at 04:04:02PM +0200, Matti Vaittinen wrote: > On 25/02/2025 15:59, Andy Shevchenko wrote: > > On Tue, Feb 25, 2025 at 03:29:17PM +0200, Matti Vaittinen wrote: > > > On 25/02/2025 12:39, Andy Shevchenko wrote: > > > > On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote: > > > > > On 25/02/2025 12:21, Andy Shevchenko wrote: > > > > > > On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote: ... > > > > > > > > > > > > > > I did not check how many users are you proposing for this, but if > > > > > > > there's only one, then IMO this should not be a global function yet. > > > > > > > It just feels to special case to me. But let's see what the others > > > > > > > think. > > > > > > > > > > > > The problem is that if somebody hides it, we might potentially see > > > > > > a duplication in the future. So I _slightly_ prefer to publish and > > > > > > then drop that after a few cycles if no users appear. > > > > > > > > > > After taking a very quick grep I spotted one other existing place where we > > > > > might be able to do direct conversion to use this function. > > > > > > > > > > drivers/net/ethernet/freescale/gianfar.c > > > > > > > > > > That'd be 2 users. > > > > > > > > I haven't checked myself, I believe your judgement, > > > > > > I took a better look and you obviously shouldn't believe :) The gianfar used > > > of_node instead of the fwnode. So, it'd be a single caller at starters. > > > > ...which is the same as dev_of_node(), which means that you can use your > > function there. > > I'm unsure what you mean. The proposed function > device_get_child_node_count_named() takes device pointer. I don't see how > dev_of_node() helps converting node to device? dev_of_node() takes the device pointer and dev_fwnode() takes that as well, it means that there is no difference which one to use OF-centric or fwnode API in this particular case. Just make sure that the function (and there is also a second loop AFAICS) takes struct device *dev instead of struct device_node *np as a parameter. > I think I could actually kill the whole gfar_of_group_count() function and > replace it with a direct call to the device_get_child_node_count_named() - > but I am not at all convinced that'd be worth including the property.h to a > file which is currently using only of_* -stuff. Well, I suppose it can be > asked from netdev peeps but I am not convinced they see it as a great idea. > > If I misunderstood your meaning - please elaborate. The driver is quite old and has a lot of room to improve. Briefly looking it may be almost fully converted to fwnode, but it's not your call (only if you wish). Nevertheless, using agnostic APIs if they reduce code base is fine. We have drivers that do OF and fwnode mixed approach (for various reasons, one of which is the new API that is absent in OF realm.
diff --git a/drivers/base/property.c b/drivers/base/property.c index c1392743df9c..3f85818183cd 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -945,6 +945,34 @@ unsigned int device_get_child_node_count(const struct device *dev) } EXPORT_SYMBOL_GPL(device_get_child_node_count); +/** + * device_get_child_node_count_named - number of child nodes with given name + * + * 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 {}... + * + * @dev: Device to count the child nodes for + * + * Return: the number of child nodes with a matching name for a given device. + */ +unsigned int device_get_child_node_count_named(const struct device *dev, + const char *name) +{ + struct fwnode_handle *child; + unsigned int count = 0; + + device_for_each_child_node(dev, child) + if (fwnode_name_eq(child, "channel")) + count++; + + return count; +} +EXPORT_SYMBOL_GPL(device_get_child_node_count_named); + 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..a2770197f76b 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -209,6 +209,8 @@ 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 device_get_child_node_count_named(const struct device *dev, + const char *name); static inline int device_property_read_u8(const struct device *dev, const char *propname, u8 *val)
There are some use-cases where child nodes with a specific name need to be parsed. In a few cases the data from the found nodes is 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 helper for 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> --- Revision history: v3 => v4: - New patch as suggested by Jonathan, see discussion in: https://lore.kernel.org/lkml/20250223161338.5c896280@jic23-huawei/ --- drivers/base/property.c | 28 ++++++++++++++++++++++++++++ include/linux/property.h | 2 ++ 2 files changed, 30 insertions(+)