[PATCH] property: Use tidy for_each_named_* macros

[PATCH] property: Use tidy for_each_named_* macros

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2575 bytes --]

Implementing if-conditions inside for_each_x() macros requires some
thinking to avoid side effects in the calling code. Resulting code
may look somewhat awkward, and there are couple of different ways it is
usually done.

Standardizing this to one way can help making it more obvious for a code
reader and writer. The newly added for_each_if() is a way to achieve this.

Use for_each_if() to make these macros look like many others which
should in the long run help reading the code.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
The patch was crafted against the IIO/testing branch, and it depends on
the 76125d7801e5 ("property: Add functions to iterate named child").
Hence I'd suggest taking this via IIO tree (if this gets accepted).

 include/linux/property.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/property.h b/include/linux/property.h
index 3e83babac0b0..d937502a22d6 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -17,6 +17,7 @@
 #include <linux/fwnode.h>
 #include <linux/stddef.h>
 #include <linux/types.h>
+#include <linux/util_macros.h>
 
 struct device;
 
@@ -169,7 +170,7 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
 
 #define fwnode_for_each_named_child_node(fwnode, child, name)		\
 	fwnode_for_each_child_node(fwnode, child)			\
-		if (!fwnode_name_eq(child, name)) { } else
+		for_each_if(fwnode_name_eq(child, name))
 
 #define fwnode_for_each_available_child_node(fwnode, child)		       \
 	for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\
@@ -184,7 +185,7 @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev,
 
 #define device_for_each_named_child_node(dev, child, name)		\
 	device_for_each_child_node(dev, child)				\
-		if (!fwnode_name_eq(child, name)) { } else
+		for_each_if(fwnode_name_eq(child, name))
 
 #define device_for_each_child_node_scoped(dev, child)			\
 	for (struct fwnode_handle *child __free(fwnode_handle) =	\
@@ -193,7 +194,7 @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev,
 
 #define device_for_each_named_child_node_scoped(dev, child, name)	\
 	device_for_each_child_node_scoped(dev, child)			\
-		if (!fwnode_name_eq(child, name)) { } else
+		for_each_if(fwnode_name_eq(child, name))
 
 struct fwnode_handle *fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
 						  const char *childname);

base-commit: 1c2409fe38d5c19015d69851d15ba543d1911932
-- 
2.49.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] property: Use tidy for_each_named_* macros

[Sakari Ailus]

On Thu, Apr 10, 2025 at 02:52:00PM +0300, Matti Vaittinen wrote:
> Implementing if-conditions inside for_each_x() macros requires some
> thinking to avoid side effects in the calling code. Resulting code
> may look somewhat awkward, and there are couple of different ways it is
> usually done.
> 
> Standardizing this to one way can help making it more obvious for a code
> reader and writer. The newly added for_each_if() is a way to achieve this.
> 
> Use for_each_if() to make these macros look like many others which
> should in the long run help reading the code.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

Re: [PATCH] property: Use tidy for_each_named_* macros

[Andy Shevchenko]

On Thu, Apr 10, 2025 at 02:52:00PM +0300, Matti Vaittinen wrote:
> Implementing if-conditions inside for_each_x() macros requires some
> thinking to avoid side effects in the calling code. Resulting code
> may look somewhat awkward, and there are couple of different ways it is
> usually done.
> 
> Standardizing this to one way can help making it more obvious for a code
> reader and writer. The newly added for_each_if() is a way to achieve this.
> 
> Use for_each_if() to make these macros look like many others which
> should in the long run help reading the code.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Thanks for cleaning these up!

> ---
> The patch was crafted against the IIO/testing branch, and it depends on
> the 76125d7801e5 ("property: Add functions to iterate named child").
> Hence I'd suggest taking this via IIO tree (if this gets accepted).

I'm not sure why. The for_each_if() is part of v6.15-rc1.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] property: Use tidy for_each_named_* macros

[Andy Shevchenko]

On Mon, Apr 14, 2025 at 09:46:14AM +0300, Andy Shevchenko wrote:
> On Thu, Apr 10, 2025 at 02:52:00PM +0300, Matti Vaittinen wrote:
> > Implementing if-conditions inside for_each_x() macros requires some
> > thinking to avoid side effects in the calling code. Resulting code
> > may look somewhat awkward, and there are couple of different ways it is
> > usually done.
> > 
> > Standardizing this to one way can help making it more obvious for a code
> > reader and writer. The newly added for_each_if() is a way to achieve this.
> > 
> > Use for_each_if() to make these macros look like many others which
> > should in the long run help reading the code.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Thanks for cleaning these up!
> 
> > ---
> > The patch was crafted against the IIO/testing branch, and it depends on
> > the 76125d7801e5 ("property: Add functions to iterate named child").
> > Hence I'd suggest taking this via IIO tree (if this gets accepted).
> 
> I'm not sure why. The for_each_if() is part of v6.15-rc1.

Ah, I see, you are trying to fix newly introduced stuff? I would rather suggest
to make this straightforward against the current upstream and ask Jonathan to
rebase the testing to fold the fixes into a new APIs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] property: Use tidy for_each_named_* macros

[Jonathan Cameron]

On Mon, 14 Apr 2025 09:47:44 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Apr 14, 2025 at 09:46:14AM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 10, 2025 at 02:52:00PM +0300, Matti Vaittinen wrote:  
> > > Implementing if-conditions inside for_each_x() macros requires some
> > > thinking to avoid side effects in the calling code. Resulting code
> > > may look somewhat awkward, and there are couple of different ways it is
> > > usually done.
> > > 
> > > Standardizing this to one way can help making it more obvious for a code
> > > reader and writer. The newly added for_each_if() is a way to achieve this.
> > > 
> > > Use for_each_if() to make these macros look like many others which
> > > should in the long run help reading the code.  
> > 
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Thanks for cleaning these up!
> >   
> > > ---
> > > The patch was crafted against the IIO/testing branch, and it depends on
> > > the 76125d7801e5 ("property: Add functions to iterate named child").
> > > Hence I'd suggest taking this via IIO tree (if this gets accepted).  
> > 
> > I'm not sure why. The for_each_if() is part of v6.15-rc1.  
> 
> Ah, I see, you are trying to fix newly introduced stuff? I would rather suggest
> to make this straightforward against the current upstream and ask Jonathan to
> rebase the testing to fold the fixes into a new APIs.
> 

Or we just do this next cycle maybe.  Definitely not going to take anything
through IIO that hasn't been on the iio list btw.

Jonathan

Re: [PATCH] property: Use tidy for_each_named_* macros

[Matti Vaittinen]

On 14/04/2025 22:14, Jonathan Cameron wrote:
> On Mon, 14 Apr 2025 09:47:44 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
>> On Mon, Apr 14, 2025 at 09:46:14AM +0300, Andy Shevchenko wrote:
>>> On Thu, Apr 10, 2025 at 02:52:00PM +0300, Matti Vaittinen wrote:
>>>> Implementing if-conditions inside for_each_x() macros requires some
>>>> thinking to avoid side effects in the calling code. Resulting code
>>>> may look somewhat awkward, and there are couple of different ways it is
>>>> usually done.
>>>>
>>>> Standardizing this to one way can help making it more obvious for a code
>>>> reader and writer. The newly added for_each_if() is a way to achieve this.
>>>>
>>>> Use for_each_if() to make these macros look like many others which
>>>> should in the long run help reading the code.
>>>
>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> Thanks for cleaning these up!
>>>    
>>>> ---
>>>> The patch was crafted against the IIO/testing branch, and it depends on
>>>> the 76125d7801e5 ("property: Add functions to iterate named child").
>>>> Hence I'd suggest taking this via IIO tree (if this gets accepted).
>>>
>>> I'm not sure why. The for_each_if() is part of v6.15-rc1.
>>
>> Ah, I see, you are trying to fix newly introduced stuff? I would rather suggest
>> to make this straightforward against the current upstream and ask Jonathan to
>> rebase the testing to fold the fixes into a new APIs.
>>
> 
> Or we just do this next cycle maybe. 

I'm not against either of the approaches. I'm (mostly) staying away from 
the computer for this and the next week, so re-spinning this will in any 
case get delayed. In that regard, the next cycle won't be that far away.

> Definitely not going to take anything
> through IIO that hasn't been on the iio list btw.

Ah. Thanks for pointing this out Jonathan! I just used the 
get_maintainer.pl - and added You. I definitely should have added the 
IIO-list!

Yours,
	-- Matti

Re: [PATCH] property: Use tidy for_each_named_* macros

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 08:55:13AM +0300, Matti Vaittinen wrote:
> On 14/04/2025 22:14, Jonathan Cameron wrote:

...

> > Definitely not going to take anything
> > through IIO that hasn't been on the iio list btw.
> 
> Ah. Thanks for pointing this out Jonathan! I just used the get_maintainer.pl
> - and added You. I definitely should have added the IIO-list!

Also a side note, the Subject should start with "device property: ...".

-- 
With Best Regards,
Andy Shevchenko