On Thu, 20 Mar 2025 08:43:44 +0200
Matti Vaittinen [off-list ref] wrote:

On 19/03/2025 17:23, Sakari Ailus wrote:

quoted

On Wed, Mar 19, 2025 at 08:02:24AM +0200, Matti Vaittinen wrote:  

quoted

On 18/03/2025 17:24, Sakari Ailus wrote:  

quoted

On Mon, Mar 17, 2025 at 05:50:38PM +0200, Matti Vaittinen wrote:  

quoted

There are a few use-cases where child nodes with a specific name need to
be parsed. Code like:  

...

quoted

quoted

quoted

quoted

--- a/include/linux/property.h
+++ b/include/linux/property.h

@@ -167,10 +167,18 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
   	for (child = fwnode_get_next_child_node(fwnode, NULL); child;	\
   	     child = fwnode_get_next_child_node(fwnode, child))
+#define fwnode_for_each_named_child_node(fwnode, child, name)		\
+	fwnode_for_each_child_node(fwnode, child)			\
+		if (!fwnode_name_eq(child, name)) { } else
+
   #define fwnode_for_each_available_child_node(fwnode, child)		       \
   	for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\
   	     child = fwnode_get_next_available_child_node(fwnode, child))
+#define fwnode_for_each_available_named_child_node(fwnode, child, name)	\
+	fwnode_for_each_available_child_node(fwnode, child)		\
+		if (!fwnode_name_eq(child, name)) { } else
+  

OF only enumerates available nodes via the fwnode API, software nodes don't
have the concept but on ACPI I guess you could have a difference in nodes
where you have device sub-nodes that aren't available. Still, these ACPI
device nodes don't have meaningful names in this context (they're
4-character object names) so you wouldn't use them like this anyway.  

I believe you have far better understanding on these concepts than I do. The
reason behind adding fwnode_for_each_available_child_node() was the patch
10/10:

-	fwnode_for_each_available_child_node(sensors, node) {
-		if (fwnode_name_eq(node, "sensor")) {
-			if (!thp7312_sensor_parse_dt(thp7312, node))
-				num_sensors++;
-		}
+	fwnode_for_each_available_named_child_node(sensors, node, "sensor") {
+		if (!thp7312_sensor_parse_dt(thp7312, node))
+			num_sensors++;
  	}

 

quoted

So my question is: is it useful to provide this besides
fwnode_for_each_named_child_node(), given that both are effectively the
same?  

So, I suppose you're saying the existing thp7312 -driver has no real reason
to use the 'fwnode_for_each_available_child_node()', but it could be using
fwnode_for_each_child_node() instead?

If so, I am Ok with dropping the
'fwnode_for_each_available_named_child_node()' and changing the 10/10 to:

-	fwnode_for_each_available_child_node(sensors, node) {
-		if (fwnode_name_eq(node, "sensor")) {
-			if (!thp7312_sensor_parse_dt(thp7312, node))
-				num_sensors++;
-		}
+	fwnode_for_each_named_child_node(sensors, node, "sensor") {
+		if (!thp7312_sensor_parse_dt(thp7312, node))
+			num_sensors++;
  	}

Do you think that'd be correct?  

I'd say so. Feel free to cc me to the last patch as well.  

Thanks. I'll drop the fwnode_for_each_available_named_child_node() then.

quoted

I guess one way to make this clearer is to switch to
fwnode_for_each_child_node() in a separate patch before
fwnode_for_each_named_child_node() conversion.  

I suppose this makes sense.

this _available_ thing is ancient history that has tripped us up
many times before. I've very keen to not see another case sneaking
in.  Whether we can definitely 'fix' all existing cases is a different
question..

I think this series can't make it to 6.15-rc1. Meaning, these 
*_named_*() APIs perhaps land in 6.16-rc1. I assume these *_named_*() 
APIs will go through the IIO. This rather simple IIO driver's review 
took longer than I predicted, with more versions I intended (as always) 
- and I kind of dislike respinning the whole series, with this large 
audience, when changes are not interesting to the most.

Maybe it is simplest to drop the thp7312 (and gianfar) from this series, 
and respin them only when the 6.16-rc1 is out. It's going to be couple 
of months though - so there's always a risk that I forget.

The proposed change for the thp7312, from 
fwnode_for_each_available_child_node() to fwnode_for_each_child_node() 
can be done earlier though.

quoted

There are also just a handful of users of
fwnode_for_each_available_child_node() and I guess these could be
converted, too, but I think it's outside the scope of the set.  

Definitely not in the scope of the bd79124 support :)

Agreed. Break this series up however you like and entirely up to
you whether you do further cleanup of other bits of the kernel!

Jonathan

Yours,
	-- Matti