diff mbox series

[v2,03/14] device property: Introduce device_for_each_child_node_scoped()

Message ID 20240211192540.340682-4-jic23@kernel.org
State Superseded
Headers show
Series device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling. | expand

Commit Message

Jonathan Cameron Feb. 11, 2024, 7:25 p.m. UTC 
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Similar to recently propose for_each_child_of_node_scoped() this
new version of the loop macro instantiates a new local
struct fwnode_handle * that uses the __free(fwnode_handle) auto
cleanup handling so that if a reference to a node is held on early
exit from the loop the reference will be released. If the loop
runs to completion, the child pointer will be NULL and no action will
be taken.

The reason this is useful is that it removes the need for
fwnode_handle_put() on early loop exits.  If there is a need
to retain the reference, then return_ptr(child) or no_free_ptr(child)
may be used to safely disable the auto cleanup.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/linux/property.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Andy Shevchenko Feb. 12, 2024, 12:10 p.m. UTC | #1 
On Sun, Feb 11, 2024 at 07:25:29PM +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Similar to recently propose for_each_child_of_node_scoped() this
> new version of the loop macro instantiates a new local
> struct fwnode_handle * that uses the __free(fwnode_handle) auto
> cleanup handling so that if a reference to a node is held on early
> exit from the loop the reference will be released. If the loop
> runs to completion, the child pointer will be NULL and no action will
> be taken.
> 
> The reason this is useful is that it removes the need for
> fwnode_handle_put() on early loop exits.  If there is a need
> to retain the reference, then return_ptr(child) or no_free_ptr(child)
> may be used to safely disable the auto cleanup.

...

> +#define device_for_each_child_node_scoped(dev, child)\

Missing space before backslash, but I would rather to make them to be TABed to
the same column.

> +	for (struct fwnode_handle *child __free(fwnode_handle) = \
> +	     device_get_next_child_node(dev, NULL); child; \

Please, move child to a separate line, so we will easily see the all three
parameters of the for-loop. That said, indent the assignment to the right as
well.

> +	     child = device_get_next_child_node(dev, child))

With the above addressed,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Jonathan Cameron Feb. 13, 2024, 10:25 a.m. UTC | #2 
On Mon, 12 Feb 2024 14:10:57 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sun, Feb 11, 2024 at 07:25:29PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Similar to recently propose for_each_child_of_node_scoped() this
> > new version of the loop macro instantiates a new local
> > struct fwnode_handle * that uses the __free(fwnode_handle) auto
> > cleanup handling so that if a reference to a node is held on early
> > exit from the loop the reference will be released. If the loop
> > runs to completion, the child pointer will be NULL and no action will
> > be taken.
> > 
> > The reason this is useful is that it removes the need for
> > fwnode_handle_put() on early loop exits.  If there is a need
> > to retain the reference, then return_ptr(child) or no_free_ptr(child)
> > may be used to safely disable the auto cleanup.  
> 
> ...
> 
> > +#define device_for_each_child_node_scoped(dev, child)\  
> 
> Missing space before backslash, but I would rather to make them to be TABed to
> the same column.

Oops. I spotted I messed this up bug clearly failed to fix it before sending out.

> 
> > +	for (struct fwnode_handle *child __free(fwnode_handle) = \
> > +	     device_get_next_child_node(dev, NULL); child; \  
> 
> Please, move child to a separate line, so we will easily see the all three
> parameters of the for-loop. That said, indent the assignment to the right as
> well.
Indent makes sense - but (to save another respin) how far?
Next tab stop will be a bit random looking but I guess nothing else
makes more sense.

> 
> > +	     child = device_get_next_child_node(dev, child))  
> 
> With the above addressed,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks,
>
Andy Shevchenko Feb. 13, 2024, 5:12 p.m. UTC | #3 
On Tue, Feb 13, 2024 at 10:25:29AM +0000, Jonathan Cameron wrote:
> On Mon, 12 Feb 2024 14:10:57 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Sun, Feb 11, 2024 at 07:25:29PM +0000, Jonathan Cameron wrote:

...

> > > +	for (struct fwnode_handle *child __free(fwnode_handle) = \
> > > +	     device_get_next_child_node(dev, NULL); child; \  

> > Please, move child to a separate line, so we will easily see the all three
> > parameters of the for-loop.

Oh, I should withdraw above, we have other for_each macros there with
a child being combined with previous line.

> > That said, indent the assignment to the right as
> > well.

> Indent makes sense - but (to save another respin) how far?
> Next tab stop will be a bit random looking but I guess nothing else
> makes more sense.

Just make whatever TAB stop that doesn't require adding any spaces.
Jonathan Cameron Feb. 16, 2024, 5:38 p.m. UTC | #4 
On Tue, 13 Feb 2024 19:12:46 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, Feb 13, 2024 at 10:25:29AM +0000, Jonathan Cameron wrote:
> > On Mon, 12 Feb 2024 14:10:57 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> > > On Sun, Feb 11, 2024 at 07:25:29PM +0000, Jonathan Cameron wrote:  
> 
> ...
> 
> > > > +	for (struct fwnode_handle *child __free(fwnode_handle) = \
> > > > +	     device_get_next_child_node(dev, NULL); child; \    
> 
> > > Please, move child to a separate line, so we will easily see the all three
> > > parameters of the for-loop.  
> 
> Oh, I should withdraw above, we have other for_each macros there with
> a child being combined with previous line.

I ended up moving it down to the next line (so it shares with the update
term).

That seemed better than having it on the end of the line that is still finishing
the initialization term and felt similar enough to local style.


> 
> > > That said, indent the assignment to the right as
> > > well.  
> 
> > Indent makes sense - but (to save another respin) how far?
> > Next tab stop will be a bit random looking but I guess nothing else
> > makes more sense.  
> 
> Just make whatever TAB stop that doesn't require adding any spaces.
>
diff mbox series

Patch

diff --git a/include/linux/property.h b/include/linux/property.h
index bcda028f1a33..e76b8c6646bd 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -182,6 +182,11 @@  struct fwnode_handle *device_get_next_child_node(const struct device *dev,
 	for (child = device_get_next_child_node(dev, NULL); child;	\
 	     child = device_get_next_child_node(dev, child))
 
+#define device_for_each_child_node_scoped(dev, child)\
+	for (struct fwnode_handle *child __free(fwnode_handle) = \
+	     device_get_next_child_node(dev, NULL); child; \
+	     child = device_get_next_child_node(dev, child))
+
 struct fwnode_handle *fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
 						  const char *childname);
 struct fwnode_handle *device_get_named_child_node(const struct device *dev,