| Message ID | 685cd1affabe50af45b767eeed9b9002d006b0fd.1740993491.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support ROHM BD79124 ADC | expand |
On Mon, Mar 03, 2025 at 01:34:49PM +0200, Matti Vaittinen wrote: > We can avoid open-coding the loop construct which counts firmware child > nodes with a specific name by using the newly added > device_get_child_node_count_named(). > > The gianfar driver has such open-coded loop. Replace it with the > device_get_child_node_count_named(). ... > It's fair to tell the pros and cons of this patch. > The simplification is there, but it's not a big one. It comes with a cost > of getting the property.h included in this driver which currently uses > exclusively the of_* APIs. I think it's a good step to the right direction. We might convert the rest (at least I don't see much impediments while briefly looking into the code). ... What about the second loop (in gfar_of_init)? I mean perhaps we want to have fwnode_for_each_named_child_node() and its device variant that may be also reused in the IIO code and here.
On 03/03/2025 13:51, Andy Shevchenko wrote: > On Mon, Mar 03, 2025 at 01:34:49PM +0200, Matti Vaittinen wrote: > > What about the second loop (in gfar_of_init)? > I mean perhaps we want to have fwnode_for_each_named_child_node() > and its device variant that may be also reused in the IIO code and here. > I agree the fwnode_for_each_named_child_node() would be useful. I think I said that already during the previous review rounds. There is plenty of code which could be converted to use it. This, however, is far more than I am willing to do in the context of a simple IIO driver addition. The "BD79124 ADC suupport" is already now 10 patches, 2 of which are directly related to it. I propose adding the for_each_named_child_node() as a separate series with bunch of users appended. That's be plenty of beans to count for those who like following the statistics :) Yours, -- Matti
On Mon, Mar 03, 2025 at 02:13:30PM +0200, Matti Vaittinen wrote: > On 03/03/2025 13:51, Andy Shevchenko wrote: > > On Mon, Mar 03, 2025 at 01:34:49PM +0200, Matti Vaittinen wrote: > > > What about the second loop (in gfar_of_init)? > > I mean perhaps we want to have fwnode_for_each_named_child_node() > > and its device variant that may be also reused in the IIO code and here. > > I agree the fwnode_for_each_named_child_node() would be useful. I think I > said that already during the previous review rounds. There is plenty of code > which could be converted to use it. > This, however, is far more than I am willing to do in the context of a > simple IIO driver addition. The "BD79124 ADC suupport" is already now 10 > patches, 2 of which are directly related to it. But you already will have at least one user (IIO code) and second as in RFC. I do not ask you to _add_ patches. > I propose adding the for_each_named_child_node() as a separate series with > bunch of users appended. That's be plenty of beans to count for those who > like following the statistics :) It would sound like an unneeded churn as we first introduce something that we already know needs a refactoring.
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index 435138f4699d..dfe012a5bc0a 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -97,6 +97,7 @@ #include <linux/phy_fixed.h> #include <linux/of.h> #include <linux/of_net.h> +#include <linux/property.h> #include "gianfar.h" @@ -571,18 +572,6 @@ static int gfar_parse_group(struct device_node *np, return 0; } -static int gfar_of_group_count(struct device_node *np) -{ - struct device_node *child; - int num = 0; - - for_each_available_child_of_node(np, child) - if (of_node_name_eq(child, "queue-group")) - num++; - - return num; -} - /* Reads the controller's registers to determine what interface * connects it to the PHY. */ @@ -654,8 +643,10 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev) num_rx_qs = 1; } else { /* MQ_MG_MODE */ /* get the actual number of supported groups */ - unsigned int num_grps = gfar_of_group_count(np); + unsigned int num_grps; + num_grps = device_get_child_node_count_named(&ofdev->dev, + "queue-group"); if (num_grps == 0 || num_grps > MAXGROUPS) { dev_err(&ofdev->dev, "Invalid # of int groups(%d)\n", num_grps);
We can avoid open-coding the loop construct which counts firmware child nodes with a specific name by using the newly added device_get_child_node_count_named(). The gianfar driver has such open-coded loop. Replace it with the device_get_child_node_count_named(). Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- It's fair to tell the pros and cons of this patch. The simplification is there, but it's not a big one. It comes with a cost of getting the property.h included in this driver which currently uses exclusively the of_* APIs. NOTE: This patch depends on the patch: [2/10] "property: Add functions to count named child nodes" Compile-tested only! --- drivers/net/ethernet/freescale/gianfar.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)