| Message ID | c0a28f466c42d5d59c7fadfa1fd05fd512d43b6f.1717060708.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | pinctrl: renesas: rzn1: Use for_each_child_of_node_scoped() | expand |
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.
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
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!
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
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.
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
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
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++; }
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(-)