Messages in this thread Patch in this message | | | From | Geert Uytterhoeven <> | Date | Wed, 22 Jan 2020 14:33:29 +0100 | Subject | Re: [RFC PATCH v2] clk: Use a new helper in managed functions |
| |
Hi Marc,
On Wed, Jan 22, 2020 at 2:02 PM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > Introduce devm_add() to factorize devres_alloc/devres_add calls. > > Using that helper produces simpler code and smaller object size: > > 1 file changed, 27 insertions(+), 66 deletions(-) > > text data bss dec hex filename > - 1708 80 0 1788 6fc drivers/clk/clk-devres.o > + 1508 80 0 1588 634 drivers/clk/clk-devres.o > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
Thanks for your patch!
> --- a/drivers/base/devres.c > +++ b/drivers/base/devres.c > @@ -685,6 +685,20 @@ int devres_release_group(struct device *dev, void *id) > } > EXPORT_SYMBOL_GPL(devres_release_group); > > +void *devm_add(struct device *dev, dr_release_t func, void *arg, size_t size)
I there any advantage of using dr_release_t over "void (*action)(void *)", like devm_add_action() does? The latter lacks the "device *" parameter.
> +{ > + void *data = devres_alloc(func, size, GFP_KERNEL); > + > + if (data) { > + memcpy(data, arg, size); > + devres_add(dev, data); > + } else > + func(dev, arg);
Both branchs should use { ...}
> + > + return data;
Why return data or NULL, instead of 0 or -Efoo, like devm_add_action()?
> +} > +EXPORT_SYMBOL_GPL(devm_add); > + > /* > * Custom devres actions allow inserting a simple function call > * into the teadown sequence. > diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c > index be160764911b..582fda9ad6a6 100644 > --- a/drivers/clk/clk-devres.c > +++ b/drivers/clk/clk-devres.c
> @@ -33,10 +25,7 @@ struct clk *devm_clk_get_optional(struct device *dev, const char *id) > { > struct clk *clk = devm_clk_get(dev, id); > > - if (clk == ERR_PTR(-ENOENT)) > - return NULL; > - > - return clk; > + return clk == ERR_PTR(-ENOENT) ? NULL : clk;
Unrelated change (which is less readable than the original, IMHO).
> } > EXPORT_SYMBOL(devm_clk_get_optional); > > @@ -45,7 +34,7 @@ struct clk_bulk_devres { > int num_clks; > }; > > -static void devm_clk_bulk_release(struct device *dev, void *res) > +static void wrap_clk_bulk_put(struct device *dev, void *res) > { > struct clk_bulk_devres *devres = res; > > @@ -55,25 +44,17 @@ static void devm_clk_bulk_release(struct device *dev, void *res) > static int __devm_clk_bulk_get(struct device *dev, int num_clks, > struct clk_bulk_data *clks, bool optional) > { > - struct clk_bulk_devres *devres; > int ret; > - > - devres = devres_alloc(devm_clk_bulk_release, > - sizeof(*devres), GFP_KERNEL); > - if (!devres) > - return -ENOMEM; > + struct clk_bulk_devres arg = { clks, num_clks }; > > if (optional) > ret = clk_bulk_get_optional(dev, num_clks, clks); > else > ret = clk_bulk_get(dev, num_clks, clks); > - if (!ret) { > - devres->clks = clks; > - devres->num_clks = num_clks; > - devres_add(dev, devres); > - } else { > - devres_free(devres); > - } > + > + if (!ret) > + if (!devm_add(dev, wrap_clk_bulk_put, &arg, sizeof(arg))) > + ret = -ENOMEM;
Nested ifs are easier to read when the outer one uses curly braces:
if (!ret) { if (!devm_add(dev, wrap_clk_bulk_put, &arg, sizeof(arg))) ret = -ENOMEM; }
Or merge the condition with &&.
> > return ret;
But in this case, I would write it as:
if (ret) return ret;
if (!devm_add(dev, wrap_clk_bulk_put, &arg, sizeof(arg))) return -ENOMEM;
return 0;
(+ consider devm_add() returning the error code instead, cfr. above).
BTW, I'm still wondering if the varargs macro discussed on #armlinux would help. I.e.
devm_add(dev, wrap_clk_bulk_put, struct clk_bulk_devres, clks, num_clks)
would create and populate the temporary arg variable.
That would require defining an argument struct for the use in devm_clk_get(), though.
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
| |