Messages in this thread | | | Subject | Re: [RFC PATCH v2] clk: Use a new helper in managed functions | From | Marc Gonzalez <> | Date | Thu, 23 Jan 2020 11:13:33 +0100 |
| |
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.
| |