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

quoted hunk

Hi Marc,

On Wed, Jan 22, 2020 at 2:02 PM Marc Gonzalez [off-list ref] 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 <redacted>
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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help