Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Marc Gonzalez]

Doh! Your reply never made it to my inbox, and I never thought to check
the mailing list...

On 15/07/2019 23:46, Bjorn Andersson wrote:

> On Mon 15 Jul 08:34 PDT 2019, Marc Gonzalez wrote:
> 
> [..]
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index c0990703ce54..5e85548357c0 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -914,6 +914,18 @@ int clk_prepare(struct clk *clk)
>>  }
>>  EXPORT_SYMBOL_GPL(clk_prepare);
>>  
>> +static void unprepare(void *clk)
> 
> This deserves a less generic name.

Fair enough. Though it's only because of C's function pointer idiosyncrasies
that a function wrapper is even needed.


> clk_enable() is used in code that can't sleep, in what scenario do you
> envision it being useful to enable a clock from such region until devres
> cleans up the associated device?

The use-case I had in mind was
"Device drivers that call
1) clk_prepare_enable from probe()
2) clk_disable_unprepare() in remove()"

(Russell King has pointed out the short-comings of such an approach
in a different sub-thread.)


>> +int devm_clk_prepare(struct device *dev, struct clk *clk);
>> +int devm_clk_enable(struct device *dev, struct clk *clk);
>> +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk)
> 
> devm_clk_prepare_enable() sounds very useful, devm_clk_prepare() might
> be useful, so keep those and drop devm_clk_enable().

Oooh, I think I understand what you mean...

I saw clk_prepare_enable() defined as clk_prepare() + clk_enable(),
and figured I'd define devm_clk_prepare_enable() as
devm_clk_prepare() + devm_clk_enable() without realizing that
devm_clk_enable() made no sense.

Solution: drop devm_clk_enable() from include/linux/clk.h
Consequence devm_clk_prepare_enable() cannot be static inline,
but that may not be a big deal...

Regards.