lkml.org 
[lkml]   [2020]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [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

    \
     
     \ /
      Last update: 2020-01-22 14:34    [W:2.278 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site