lkml.org 
[lkml]   [2020]   [Jan]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH v2] clk: Use a new helper in managed functions
From
Date
On 22/01/2020 14:33, Geert Uytterhoeven wrote:

> On Wed, Jan 22, 2020 at 2:02 PM Marc Gonzalez 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)
>
> Is there any advantage of using dr_release_t over "void (*action)(void *)",
> like devm_add_action() does? The latter lacks the "device *" parameter.

(I did forget to mention that v1 used devm_add_action.)
https://patchwork.kernel.org/patch/11262685/

A limitation of devm_add_action is that it stores the void *data argument "as is".
Users cannot pass the address of a struct on the stack. devm_add() addresses that
specific use-case, while being a minimal wrapper around devres_alloc + devres_add.
(devm_add_action adds an extra level of indirection.)

>> +{
>> + 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 { ...}

Ah yes, scripts/checkpatch.pl needs --strict to point this out.

>> +
>> + return data;
>
> Why return data or NULL, instead of 0 or -Efoo, like devm_add_action()?

My intent is to make devm_add a minimal wrapper (it even started out as
a macro). As such, I just transparently pass the result of devres_alloc.

Do you see an advantage in processing the result?

>> @@ -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).

I'd like to hear the maintainers' opinion. I defer to their preference.

>> +
>> + 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;

I like the simplicity of this code.


> (+ consider devm_add() returning the error code instead, cfr. above).

Some functions return an int, some a pointer, some might store the
result through a pointer.


> 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.

There could be a helper for the "pass-a-struct" use-case, using a compound literal:

#define helper(dev, func, type, args...) devm_add(dev, func, &(type){args}, sizeof(type))

Regards.

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