On 31.07.2014 13:05, Mark Brown wrote:
> On Thu, Jul 31, 2014 at 11:56:15AM +0200, Stefan Assmann wrote:
>> On 30.07.2014 19:50, Mark Brown wrote:
>>> On Wed, Jul 30, 2014 at 04:02:29PM +0200, Stefan Assmann wrote:
>
>>>> +static int twl6030_clk32kg_is_prepared(struct clk_hw *hw)
>>>> +{
>>>> + struct twl6030_desc *desc = to_twl6030_desc(hw);
>>>> +
>>>> + return desc->enabled;
>>>> +}
>
>>> Why not just check the register map - can't the register be cached? If
>>> that's not possible a comment would be good.
>
>> I just took atl_clk_is_enabled() as template. If you say it's better
>> to read the value, that can be arranged.
>
> It might be worth doing this if you have to go to hardware to check the
> status, if you can read a cache then just using the register is less
> error prone.
Ok.
>
>>>> + else
>>>> + clk_prepare(clk);
>
>>> Why is the clock driver defaulting to enabling the clock, and if it
>>> needs to shouldn't it be doing a prepere_enable() even if the enable
>>> happens not to do anything to the hardware? Otherwise child clocks
>>> might get confused.
>
>> Mike advised me to convert the functions from enable/disable to
>> prepare/unprepare because i2c transactions may sleep. That's what I did.
>> The code no longer enables the clock and just prepares it. So IIUC the
>> call to clk_prepare() should be fine.
>
> That's not going to help consumers of the clock, you do need to move the
> operations to prepare() but users shouldn't need to know what happens in
> prepare() and what happens in enable().
>
> You've also not addressed the comment about defaulting to enabling the
> clock in the first place.
Maybe I misinterpreted your previous comment, sorry. So if I got it right
this time you're saying that the prepare/enable, disable/unprepare should
be seen as a single step. If that's the case then using prepare_enable()
should be used indeed.
Tero suggested to look into making this a generic i2c clock driver. I'll
look into that and report back.
Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
