[PATCH] pinctrl: renesas: rzn1: Use for_each_child_of_node_scoped()

[PATCH] pinctrl: renesas: rzn1: Use for_each_child_of_node_scoped()

[Geert Uytterhoeven]

Use the scoped variant of for_each_child_of_node() to simplify the code.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
To be queued in renesas-pinctrl for v6.11.

 drivers/pinctrl/renesas/pinctrl-rzn1.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pinctrl/renesas/pinctrl-rzn1.c b/drivers/pinctrl/renesas/pinctrl-rzn1.c
index e1b4203c66c6f836..39af1fe79c8462eb 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzn1.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzn1.c
@@ -737,13 +737,12 @@ static int rzn1_pinctrl_parse_groups(struct device_node *np,
 
 static int rzn1_pinctrl_count_function_groups(struct device_node *np)
 {
-	struct device_node *child;
 	int count = 0;
 
 	if (of_property_count_u32_elems(np, RZN1_PINS_PROP) > 0)
 		count++;
 
-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		if (of_property_count_u32_elems(child, RZN1_PINS_PROP) > 0)
 			count++;
 	}
-- 
2.34.1

Re: [PATCH] pinctrl: renesas: rzn1: Use for_each_child_of_node_scoped()

[Andy Shevchenko]

Thu, May 30, 2024 at 11:19:29AM +0200, Geert Uytterhoeven kirjoitti:
> Use the scoped variant of for_each_child_of_node() to simplify the code.

I do not see the point of this patch. This makes code actually more
complicated, and I'm not sure the code generation is the same and not worse.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: renesas: rzn1: Use for_each_child_of_node_scoped()

[Geert Uytterhoeven]

Hi Andy,

On Thu, May 30, 2024 at 11:26 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> Thu, May 30, 2024 at 11:19:29AM +0200, Geert Uytterhoeven kirjoitti:
> > Use the scoped variant of for_each_child_of_node() to simplify the code.
>
> I do not see the point of this patch. This makes code actually more
> complicated, and I'm not sure the code generation is the same and not worse.

On arm32, a conversion to for_each_child_of_node_scoped() seems to
cost ca. 48 bytes of additional code.

BTW, the same is true for cases where the conversion does simplify
cleanup.

I checked "pinctrl: renesas: Use scope based of_node_put() cleanups",
and all but the conversions in *_dt_node_to_map() cost 48 bytes each.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] pinctrl: renesas: rzn1: Use for_each_child_of_node_scoped()

[Andy Shevchenko]

On Thu, May 30, 2024 at 2:52 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, May 30, 2024 at 11:26 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > Thu, May 30, 2024 at 11:19:29AM +0200, Geert Uytterhoeven kirjoitti:
> > > Use the scoped variant of for_each_child_of_node() to simplify the code.
> >
> > I do not see the point of this patch. This makes code actually more
> > complicated, and I'm not sure the code generation is the same and not worse.
>
> On arm32, a conversion to for_each_child_of_node_scoped() seems to
> cost ca. 48 bytes of additional code.
>
> BTW, the same is true for cases where the conversion does simplify
> cleanup.
>
> I checked "pinctrl: renesas: Use scope based of_node_put() cleanups",
> and all but the conversions in *_dt_node_to_map() cost 48 bytes each.

Yeah. so for the cases where there are no returns from inside the loop
I prefer not to use _scoped.

P.S. Thank you for checking, btw!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: renesas: rzn1: Use for_each_child_of_node_scoped()

[Dan Carpenter]

On Thu, May 30, 2024 at 04:36:59PM +0300, Andy Shevchenko wrote:
> On Thu, May 30, 2024 at 2:52 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, May 30, 2024 at 11:26 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > Thu, May 30, 2024 at 11:19:29AM +0200, Geert Uytterhoeven kirjoitti:
> > > > Use the scoped variant of for_each_child_of_node() to simplify the code.
> > >
> > > I do not see the point of this patch. This makes code actually more
> > > complicated, and I'm not sure the code generation is the same and not worse.
> >
> > On arm32, a conversion to for_each_child_of_node_scoped() seems to
> > cost ca. 48 bytes of additional code.
> >
> > BTW, the same is true for cases where the conversion does simplify
> > cleanup.
> >
> > I checked "pinctrl: renesas: Use scope based of_node_put() cleanups",
> > and all but the conversions in *_dt_node_to_map() cost 48 bytes each.
> 
> Yeah. so for the cases where there are no returns from inside the loop
> I prefer not to use _scoped.

Eventually _scoped() loops will become the norm.  Leaving some unscoped
loops will be a fun surprise for the first person to introduce a return
-EINVAL.

regards,
dan carpenter

Re: [PATCH] pinctrl: renesas: rzn1: Use for_each_child_of_node_scoped()

[Andy Shevchenko]

On Fri, May 31, 2024 at 11:01 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> On Thu, May 30, 2024 at 04:36:59PM +0300, Andy Shevchenko wrote:
> > On Thu, May 30, 2024 at 2:52 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Thu, May 30, 2024 at 11:26 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > Thu, May 30, 2024 at 11:19:29AM +0200, Geert Uytterhoeven kirjoitti:
> > > > > Use the scoped variant of for_each_child_of_node() to simplify the code.
> > > >
> > > > I do not see the point of this patch. This makes code actually more
> > > > complicated, and I'm not sure the code generation is the same and not worse.
> > >
> > > On arm32, a conversion to for_each_child_of_node_scoped() seems to
> > > cost ca. 48 bytes of additional code.
> > >
> > > BTW, the same is true for cases where the conversion does simplify
> > > cleanup.
> > >
> > > I checked "pinctrl: renesas: Use scope based of_node_put() cleanups",
> > > and all but the conversions in *_dt_node_to_map() cost 48 bytes each.
> >
> > Yeah. so for the cases where there are no returns from inside the loop
> > I prefer not to use _scoped.
>
> Eventually _scoped() loops will become the norm.  Leaving some unscoped
> loops will be a fun surprise for the first person to introduce a return
> -EINVAL.

It makes no sense when we have no return / goto semantics from inside
of the loop. I don't know why we should do worse binary code for no
benefit.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: renesas: rzn1: Use for_each_child_of_node_scoped()

[Dan Carpenter]

On Fri, May 31, 2024 at 11:19:26AM +0300, Andy Shevchenko wrote:
> On Fri, May 31, 2024 at 11:01 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > On Thu, May 30, 2024 at 04:36:59PM +0300, Andy Shevchenko wrote:
> > > On Thu, May 30, 2024 at 2:52 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Thu, May 30, 2024 at 11:26 AM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > Thu, May 30, 2024 at 11:19:29AM +0200, Geert Uytterhoeven kirjoitti:
> > > > > > Use the scoped variant of for_each_child_of_node() to simplify the code.
> > > > >
> > > > > I do not see the point of this patch. This makes code actually more
> > > > > complicated, and I'm not sure the code generation is the same and not worse.
> > > >
> > > > On arm32, a conversion to for_each_child_of_node_scoped() seems to
> > > > cost ca. 48 bytes of additional code.
> > > >
> > > > BTW, the same is true for cases where the conversion does simplify
> > > > cleanup.
> > > >
> > > > I checked "pinctrl: renesas: Use scope based of_node_put() cleanups",
> > > > and all but the conversions in *_dt_node_to_map() cost 48 bytes each.
> > >
> > > Yeah. so for the cases where there are no returns from inside the loop
> > > I prefer not to use _scoped.
> >
> > Eventually _scoped() loops will become the norm.  Leaving some unscoped
> > loops will be a fun surprise for the first person to introduce a return
> > -EINVAL.
> 
> It makes no sense when we have no return / goto semantics from inside
> of the loop. I don't know why we should do worse binary code for no
> benefit.

The compiler ought to be able to determine when the cleanup function is
not required and save those 48 bytes.  That's why we have NULL checking
in __free_device_node() instead of using the NULL check in of_node_put().
The compiler is already removing all the calls from the return
statements where p is NULL.  It seems like a small thing to one step
further and delete the cleanup when it's not called on any path?

regards,
dan carpenter

Re: [PATCH] pinctrl: renesas: rzn1: Use for_each_child_of_node_scoped()

[Geert Uytterhoeven]

On Fri, May 31, 2024 at 10:01 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> On Thu, May 30, 2024 at 04:36:59PM +0300, Andy Shevchenko wrote:
> > On Thu, May 30, 2024 at 2:52 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Thu, May 30, 2024 at 11:26 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > Thu, May 30, 2024 at 11:19:29AM +0200, Geert Uytterhoeven kirjoitti:
> > > > > Use the scoped variant of for_each_child_of_node() to simplify the code.
> > > >
> > > > I do not see the point of this patch. This makes code actually more
> > > > complicated, and I'm not sure the code generation is the same and not worse.
> > >
> > > On arm32, a conversion to for_each_child_of_node_scoped() seems to
> > > cost ca. 48 bytes of additional code.
> > >
> > > BTW, the same is true for cases where the conversion does simplify
> > > cleanup.
> > >
> > > I checked "pinctrl: renesas: Use scope based of_node_put() cleanups",
> > > and all but the conversions in *_dt_node_to_map() cost 48 bytes each.
> >
> > Yeah. so for the cases where there are no returns from inside the loop
> > I prefer not to use _scoped.
>
> Eventually _scoped() loops will become the norm.  Leaving some unscoped
> loops will be a fun surprise for the first person to introduce a return
> -EINVAL.

Exactly. So I'm queuing this patch.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds