| Message ID | 20240812-cleanup-h-of-node-put-memory-v1-1-5065a8f361d2@linaro.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | memory: simplify with scoped/cleanup.h for device nodes | expand |
On Mon, 12 Aug 2024 15:33:55 +0200 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > Obtain the device node reference with scoped/cleanup.h to reduce error > handling and make the code a bit simpler. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Hi, Comments inline. > --- > drivers/memory/atmel-ebi.c | 29 ++++++++++------------------- > 1 file changed, 10 insertions(+), 19 deletions(-) > > diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c > index e8bb5f37f5cb..fcbfc2655d8d 100644 > --- a/drivers/memory/atmel-ebi.c > +++ b/drivers/memory/atmel-ebi.c > @@ -6,6 +6,7 @@ > * Copyright (C) 2013 Jean-Jacques Hiblot <jjhiblot@traphandler.com> > */ > > +#include <linux/cleanup.h> > #include <linux/clk.h> > #include <linux/io.h> > #include <linux/mfd/syscon.h> > @@ -517,7 +518,7 @@ static int atmel_ebi_dev_disable(struct atmel_ebi *ebi, struct device_node *np) > static int atmel_ebi_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > - struct device_node *child, *np = dev->of_node, *smc_np; > + struct device_node *child, *np = dev->of_node; > struct atmel_ebi *ebi; > int ret, reg_cells; > struct clk *clk; > @@ -541,30 +542,24 @@ static int atmel_ebi_probe(struct platform_device *pdev) > > ebi->clk = clk; > > - smc_np = of_parse_phandle(dev->of_node, "atmel,smc", 0); > + struct device_node *smc_np __free(device_node) = of_parse_phandle(dev->of_node, > + "atmel,smc", 0); Trivial: I'd line break this as > + struct device_node *smc_np __free(device_node) = of_parse_phandle(dev->of_node, "atmel,smc", 0); > > ebi->smc.regmap = syscon_node_to_regmap(smc_np); > - if (IS_ERR(ebi->smc.regmap)) { > - ret = PTR_ERR(ebi->smc.regmap); > - goto put_node; > - } > + if (IS_ERR(ebi->smc.regmap)) > + return PTR_ERR(ebi->smc.regmap); > > ebi->smc.layout = atmel_hsmc_get_reg_layout(smc_np); > - if (IS_ERR(ebi->smc.layout)) { > - ret = PTR_ERR(ebi->smc.layout); > - goto put_node; > - } > + if (IS_ERR(ebi->smc.layout)) > + return PTR_ERR(ebi->smc.layout); > > ebi->smc.clk = of_clk_get(smc_np, 0); > if (IS_ERR(ebi->smc.clk)) { > - if (PTR_ERR(ebi->smc.clk) != -ENOENT) { > - ret = PTR_ERR(ebi->smc.clk); > - goto put_node; > - } > + if (PTR_ERR(ebi->smc.clk) != -ENOENT) > + return PTR_ERR(ebi->smc.clk); > > ebi->smc.clk = NULL; > } > - of_node_put(smc_np); The large change in scope is a bit inelegant as it now hangs on to the smc_np much longer than before. Maybe it's worth pulling out the modified code as a atem_eb_probe_smc(struct device_node *smc_np, struct atmel_ebi_smc *smc ) or something like with a struct_group to define the atmel_ebi_smc That would keep the tight scope for the data and generally simplify it a bit. > ret = clk_prepare_enable(ebi->smc.clk); > if (ret) > return ret; > @@ -615,10 +610,6 @@ static int atmel_ebi_probe(struct platform_device *pdev) > } > > return of_platform_populate(np, NULL, NULL, dev); > - > -put_node: > - of_node_put(smc_np); > - return ret; > } > > static __maybe_unused int atmel_ebi_resume(struct device *dev) >
On 14/08/2024 18:38, Jonathan Cameron wrote: > On Mon, 12 Aug 2024 15:33:55 +0200 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> Obtain the device node reference with scoped/cleanup.h to reduce error >> handling and make the code a bit simpler. >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Hi, > > Comments inline. >> --- >> drivers/memory/atmel-ebi.c | 29 ++++++++++------------------- >> 1 file changed, 10 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c >> index e8bb5f37f5cb..fcbfc2655d8d 100644 >> --- a/drivers/memory/atmel-ebi.c >> +++ b/drivers/memory/atmel-ebi.c >> @@ -6,6 +6,7 @@ >> * Copyright (C) 2013 Jean-Jacques Hiblot <jjhiblot@traphandler.com> >> */ >> >> +#include <linux/cleanup.h> >> #include <linux/clk.h> >> #include <linux/io.h> >> #include <linux/mfd/syscon.h> >> @@ -517,7 +518,7 @@ static int atmel_ebi_dev_disable(struct atmel_ebi *ebi, struct device_node *np) >> static int atmel_ebi_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> - struct device_node *child, *np = dev->of_node, *smc_np; >> + struct device_node *child, *np = dev->of_node; >> struct atmel_ebi *ebi; >> int ret, reg_cells; >> struct clk *clk; >> @@ -541,30 +542,24 @@ static int atmel_ebi_probe(struct platform_device *pdev) >> >> ebi->clk = clk; >> >> - smc_np = of_parse_phandle(dev->of_node, "atmel,smc", 0); >> + struct device_node *smc_np __free(device_node) = of_parse_phandle(dev->of_node, >> + "atmel,smc", 0); > Trivial: > I'd line break this as >> + struct device_node *smc_np __free(device_node) = > of_parse_phandle(dev->of_node, "atmel,smc", 0); Yeah, I have troubles with this constructor+destructor syntaxes. They are way past 80 and 100 column, so maybe indeed should be wrapped at '='. > >> >> ebi->smc.regmap = syscon_node_to_regmap(smc_np); >> - if (IS_ERR(ebi->smc.regmap)) { >> - ret = PTR_ERR(ebi->smc.regmap); >> - goto put_node; >> - } >> + if (IS_ERR(ebi->smc.regmap)) >> + return PTR_ERR(ebi->smc.regmap); >> >> ebi->smc.layout = atmel_hsmc_get_reg_layout(smc_np); >> - if (IS_ERR(ebi->smc.layout)) { >> - ret = PTR_ERR(ebi->smc.layout); >> - goto put_node; >> - } >> + if (IS_ERR(ebi->smc.layout)) >> + return PTR_ERR(ebi->smc.layout); >> >> ebi->smc.clk = of_clk_get(smc_np, 0); >> if (IS_ERR(ebi->smc.clk)) { >> - if (PTR_ERR(ebi->smc.clk) != -ENOENT) { >> - ret = PTR_ERR(ebi->smc.clk); >> - goto put_node; >> - } >> + if (PTR_ERR(ebi->smc.clk) != -ENOENT) >> + return PTR_ERR(ebi->smc.clk); >> >> ebi->smc.clk = NULL; >> } >> - of_node_put(smc_np); > > The large change in scope is a bit inelegant as it now hangs on to > the smc_np much longer than before. > > Maybe it's worth pulling out the modified code as a > atem_eb_probe_smc(struct device_node *smc_np, struct atmel_ebi_smc *smc ) > > or something like with a struct_group to define the atmel_ebi_smc > > That would keep the tight scope for the data and generally simplify it > a bit. Are you speaking about any particular code optimization/performance concerns or readability? Because scope in the latter, I believe, is not the problem here. The entire point of __free() is that you do not care about scope of variable or destructor. You know it will be freed, sooner or later. If it happens later - no problem, anyway we don't have to "think" about this variable or cleaning up because of __free(). Best regards, Krzysztof
diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c index e8bb5f37f5cb..fcbfc2655d8d 100644 --- a/drivers/memory/atmel-ebi.c +++ b/drivers/memory/atmel-ebi.c @@ -6,6 +6,7 @@ * Copyright (C) 2013 Jean-Jacques Hiblot <jjhiblot@traphandler.com> */ +#include <linux/cleanup.h> #include <linux/clk.h> #include <linux/io.h> #include <linux/mfd/syscon.h> @@ -517,7 +518,7 @@ static int atmel_ebi_dev_disable(struct atmel_ebi *ebi, struct device_node *np) static int atmel_ebi_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct device_node *child, *np = dev->of_node, *smc_np; + struct device_node *child, *np = dev->of_node; struct atmel_ebi *ebi; int ret, reg_cells; struct clk *clk; @@ -541,30 +542,24 @@ static int atmel_ebi_probe(struct platform_device *pdev) ebi->clk = clk; - smc_np = of_parse_phandle(dev->of_node, "atmel,smc", 0); + struct device_node *smc_np __free(device_node) = of_parse_phandle(dev->of_node, + "atmel,smc", 0); ebi->smc.regmap = syscon_node_to_regmap(smc_np); - if (IS_ERR(ebi->smc.regmap)) { - ret = PTR_ERR(ebi->smc.regmap); - goto put_node; - } + if (IS_ERR(ebi->smc.regmap)) + return PTR_ERR(ebi->smc.regmap); ebi->smc.layout = atmel_hsmc_get_reg_layout(smc_np); - if (IS_ERR(ebi->smc.layout)) { - ret = PTR_ERR(ebi->smc.layout); - goto put_node; - } + if (IS_ERR(ebi->smc.layout)) + return PTR_ERR(ebi->smc.layout); ebi->smc.clk = of_clk_get(smc_np, 0); if (IS_ERR(ebi->smc.clk)) { - if (PTR_ERR(ebi->smc.clk) != -ENOENT) { - ret = PTR_ERR(ebi->smc.clk); - goto put_node; - } + if (PTR_ERR(ebi->smc.clk) != -ENOENT) + return PTR_ERR(ebi->smc.clk); ebi->smc.clk = NULL; } - of_node_put(smc_np); ret = clk_prepare_enable(ebi->smc.clk); if (ret) return ret; @@ -615,10 +610,6 @@ static int atmel_ebi_probe(struct platform_device *pdev) } return of_platform_populate(np, NULL, NULL, dev); - -put_node: - of_node_put(smc_np); - return ret; } static __maybe_unused int atmel_ebi_resume(struct device *dev)
Obtain the device node reference with scoped/cleanup.h to reduce error handling and make the code a bit simpler. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/memory/atmel-ebi.c | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-)