| Message ID | 20240504-pinctrl-cleanup-v2-7-26c5f2dc1181@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | pinctrl: Use scope based of_node_put() cleanups | expand |
Hi Peng, On Sat, May 4, 2024 at 3:14 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > From: Peng Fan <peng.fan@nxp.com> > > Use scope based of_node_put() cleanup to simplify code. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> Thanks for your patch! Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/pinctrl/renesas/pinctrl-rzn1.c > +++ b/drivers/pinctrl/renesas/pinctrl-rzn1.c You missed one trivial conversion, presumably because no error handling and thus no of_node_put() is involved? @@ -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++; } If you prefer not to include this, I will send a small patch myself later. Gr{oetje,eeting}s, Geert
> Subject: Re: [PATCH v2 07/20] pinctrl: renesas: Use scope based > of_node_put() cleanups > > Hi Peng, > > On Sat, May 4, 2024 at 3:14 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> > wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > Use scope based of_node_put() cleanup to simplify code. > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > Thanks for your patch! > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > --- a/drivers/pinctrl/renesas/pinctrl-rzn1.c > > +++ b/drivers/pinctrl/renesas/pinctrl-rzn1.c > > You missed one trivial conversion, presumably because no error handling and > thus no of_node_put() is involved? You are right. > > @@ -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++; > } > > If you prefer not to include this, I will send a small patch myself later. I would not add it. If no major comments in this patchset, I will not do a v3. So, please do that with your follow up patch. Thanks, Peng. > > 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
On Mon, May 13, 2024 at 1:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Sat, May 4, 2024 at 3:14 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > Use scope based of_node_put() cleanup to simplify code. > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > Thanks for your patch! > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> Does this go into the Renesas patch stack? I think the patch stands fine without the rest of the series. Yours, Linus Walleij
Hi Linus, On Mon, May 13, 2024 at 10:51 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, May 13, 2024 at 1:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Sat, May 4, 2024 at 3:14 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > Use scope based of_node_put() cleanup to simplify code. > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > > Thanks for your patch! > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Does this go into the Renesas patch stack? > > I think the patch stands fine without the rest of the series. Sure, I can do that. From your positive response to v1, I thought that perhaps you just wanted to take the full series yourself? Gr{oetje,eeting}s, Geert
On Tue, May 14, 2024 at 8:36 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Does this go into the Renesas patch stack? > > > > I think the patch stands fine without the rest of the series. > > Sure, I can do that. Please apply it! > From your positive response to v1, I thought that perhaps you just > wanted to take the full series yourself? Sorry, I always prefer submaintainers to pick their stuff, they know what they are doing and they can test the entire patch stack properly. Yours, Linus Walleij
On Tue, May 14, 2024 at 9:33 AM Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, May 14, 2024 at 8:36 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > Does this go into the Renesas patch stack? > > > I think the patch stands fine without the rest of the series. > > > > Sure, I can do that. > > Please apply it! OK, will queue in renesas-pinctrl for v6.11. > > From your positive response to v1, I thought that perhaps you just > > wanted to take the full series yourself? > > Sorry, I always prefer submaintainers to pick their stuff, they > know what they are doing and they can test the entire patch > stack properly. OK, will (try to ;-) remember... Gr{oetje,eeting}s, Geert
Mon, May 13, 2024 at 01:59:03PM +0200, Geert Uytterhoeven kirjoitti: > On Sat, May 4, 2024 at 3:14 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: ... > You missed one trivial conversion, presumably because no error handling > and thus no of_node_put() is involved? Nothing is missed. The below is a redundant change. ... > - 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++; > } > > If you prefer not to include this I prefer this not to be included as it will give a misleading signals to the use of the original API(s).
Hi Andy, On Thu, May 30, 2024 at 11:03 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > Mon, May 13, 2024 at 01:59:03PM +0200, Geert Uytterhoeven kirjoitti: > > On Sat, May 4, 2024 at 3:14 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > You missed one trivial conversion, presumably because no error handling > > and thus no of_node_put() is involved? > > Nothing is missed. The below is a redundant change. > > ... > > > - 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++; > > } > > > > If you prefer not to include this > > I prefer this not to be included as it will give a misleading signals to the > use of the original API(s). Thank you for reminding me to send this out as a separate patch :-) https://lore.kernel.org/r/c0a28f466c42d5d59c7fadfa1fd05fd512d43b6f.1717060708.git.geert+renesas@glider.be Gr{oetje,eeting}s, Geert
diff --git a/drivers/pinctrl/renesas/pinctrl-rza1.c b/drivers/pinctrl/renesas/pinctrl-rza1.c index edcbe7c9ad56..6527872813dc 100644 --- a/drivers/pinctrl/renesas/pinctrl-rza1.c +++ b/drivers/pinctrl/renesas/pinctrl-rza1.c @@ -852,7 +852,6 @@ static const struct gpio_chip rza1_gpiochip_template = { */ static int rza1_dt_node_pin_count(struct device_node *np) { - struct device_node *child; struct property *of_pins; unsigned int npins; @@ -861,12 +860,10 @@ static int rza1_dt_node_pin_count(struct device_node *np) return of_pins->length / sizeof(u32); npins = 0; - for_each_child_of_node(np, child) { + for_each_child_of_node_scoped(np, child) { of_pins = of_find_property(child, "pinmux", NULL); - if (!of_pins) { - of_node_put(child); + if (!of_pins) return -EINVAL; - } npins += of_pins->length / sizeof(u32); } @@ -986,7 +983,6 @@ static int rza1_dt_node_to_map(struct pinctrl_dev *pctldev, struct rza1_pinctrl *rza1_pctl = pinctrl_dev_get_drvdata(pctldev); struct rza1_mux_conf *mux_confs, *mux_conf; unsigned int *grpins, *grpin; - struct device_node *child; const char *grpname; const char **fngrps; int ret, npins; @@ -1023,13 +1019,11 @@ static int rza1_dt_node_to_map(struct pinctrl_dev *pctldev, ret = rza1_parse_pinmux_node(rza1_pctl, np, mux_conf, grpin); if (ret == -ENOENT) - for_each_child_of_node(np, child) { + for_each_child_of_node_scoped(np, child) { ret = rza1_parse_pinmux_node(rza1_pctl, child, mux_conf, grpin); - if (ret < 0) { - of_node_put(child); + if (ret < 0) return ret; - } grpin += ret; mux_conf += ret; diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c index c3256bfde502..fc7f33d3c613 100644 --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c @@ -745,7 +745,6 @@ static int rzg2l_dt_node_to_map(struct pinctrl_dev *pctldev, unsigned int *num_maps) { struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); - struct device_node *child; unsigned int index; int ret; @@ -753,13 +752,11 @@ static int rzg2l_dt_node_to_map(struct pinctrl_dev *pctldev, *num_maps = 0; index = 0; - for_each_child_of_node(np, child) { + for_each_child_of_node_scoped(np, child) { ret = rzg2l_dt_subnode_to_map(pctldev, child, np, map, num_maps, &index); - if (ret < 0) { - of_node_put(child); + if (ret < 0) goto done; - } } if (*num_maps == 0) { diff --git a/drivers/pinctrl/renesas/pinctrl-rzn1.c b/drivers/pinctrl/renesas/pinctrl-rzn1.c index 4b2f107824fe..e1b4203c66c6 100644 --- a/drivers/pinctrl/renesas/pinctrl-rzn1.c +++ b/drivers/pinctrl/renesas/pinctrl-rzn1.c @@ -404,7 +404,6 @@ static int rzn1_dt_node_to_map(struct pinctrl_dev *pctldev, struct pinctrl_map **map, unsigned int *num_maps) { - struct device_node *child; int ret; *map = NULL; @@ -414,12 +413,10 @@ static int rzn1_dt_node_to_map(struct pinctrl_dev *pctldev, if (ret < 0) return ret; - for_each_child_of_node(np, child) { + for_each_child_of_node_scoped(np, child) { ret = rzn1_dt_node_to_map_one(pctldev, child, map, num_maps); - if (ret < 0) { - of_node_put(child); + if (ret < 0) return ret; - } } return 0; @@ -760,7 +757,6 @@ static int rzn1_pinctrl_parse_functions(struct device_node *np, { struct rzn1_pmx_func *func; struct rzn1_pin_group *grp; - struct device_node *child; unsigned int i = 0; int ret; @@ -793,15 +789,13 @@ static int rzn1_pinctrl_parse_functions(struct device_node *np, ipctl->ngroups++; } - for_each_child_of_node(np, child) { + for_each_child_of_node_scoped(np, child) { func->groups[i] = child->name; grp = &ipctl->groups[ipctl->ngroups]; grp->func = func->name; ret = rzn1_pinctrl_parse_groups(child, grp, ipctl); - if (ret < 0) { - of_node_put(child); + if (ret < 0) return ret; - } i++; ipctl->ngroups++; } @@ -816,7 +810,6 @@ static int rzn1_pinctrl_probe_dt(struct platform_device *pdev, struct rzn1_pinctrl *ipctl) { struct device_node *np = pdev->dev.of_node; - struct device_node *child; unsigned int maxgroups = 0; unsigned int i = 0; int nfuncs = 0; @@ -834,7 +827,7 @@ static int rzn1_pinctrl_probe_dt(struct platform_device *pdev, return -ENOMEM; ipctl->ngroups = 0; - for_each_child_of_node(np, child) + for_each_child_of_node_scoped(np, child) maxgroups += rzn1_pinctrl_count_function_groups(child); ipctl->groups = devm_kmalloc_array(&pdev->dev, @@ -844,12 +837,10 @@ static int rzn1_pinctrl_probe_dt(struct platform_device *pdev, if (!ipctl->groups) return -ENOMEM; - for_each_child_of_node(np, child) { + for_each_child_of_node_scoped(np, child) { ret = rzn1_pinctrl_parse_functions(child, ipctl, i++); - if (ret < 0) { - of_node_put(child); + if (ret < 0) return ret; - } } return 0; diff --git a/drivers/pinctrl/renesas/pinctrl-rzv2m.c b/drivers/pinctrl/renesas/pinctrl-rzv2m.c index 0767a5ac23e0..0cae5472ac67 100644 --- a/drivers/pinctrl/renesas/pinctrl-rzv2m.c +++ b/drivers/pinctrl/renesas/pinctrl-rzv2m.c @@ -388,7 +388,6 @@ static int rzv2m_dt_node_to_map(struct pinctrl_dev *pctldev, unsigned int *num_maps) { struct rzv2m_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); - struct device_node *child; unsigned int index; int ret; @@ -396,13 +395,11 @@ static int rzv2m_dt_node_to_map(struct pinctrl_dev *pctldev, *num_maps = 0; index = 0; - for_each_child_of_node(np, child) { + for_each_child_of_node_scoped(np, child) { ret = rzv2m_dt_subnode_to_map(pctldev, child, np, map, num_maps, &index); - if (ret < 0) { - of_node_put(child); + if (ret < 0) goto done; - } } if (*num_maps == 0) { diff --git a/drivers/pinctrl/renesas/pinctrl.c b/drivers/pinctrl/renesas/pinctrl.c index 4d9d58fc1356..03e9bdbc82b9 100644 --- a/drivers/pinctrl/renesas/pinctrl.c +++ b/drivers/pinctrl/renesas/pinctrl.c @@ -241,7 +241,6 @@ static int sh_pfc_dt_node_to_map(struct pinctrl_dev *pctldev, { struct sh_pfc_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); struct device *dev = pmx->pfc->dev; - struct device_node *child; unsigned int index; int ret; @@ -249,13 +248,11 @@ static int sh_pfc_dt_node_to_map(struct pinctrl_dev *pctldev, *num_maps = 0; index = 0; - for_each_child_of_node(np, child) { + for_each_child_of_node_scoped(np, child) { ret = sh_pfc_dt_subnode_to_map(pctldev, child, map, num_maps, &index); - if (ret < 0) { - of_node_put(child); + if (ret < 0) goto done; - } } /* If no mapping has been found in child nodes try the config node. */