Re: [RFC PATCH v2] clk: Use a new helper in managed functions

[Geert Uytterhoeven]

Hi Marc,

On Wed, Jan 22, 2020 at 2:02 PM Marc Gonzalez <marc.w.gonzalez@xxxxxxx> 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@xxxxxxx>

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@xxxxxxxxxxxxxx

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