Quoting Stefan Assmann (2014-07-31 05:04:29) > 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. It's not so much that they are single step. The point is that the way we ungate a clock signal using the Linux clk.h api is by calling clk_enable(). If you read the code for clk_enable you'll see that it depends on the clock already having been prepared via clk_prepare(). The fact that your particular implementation of this clock driver actually ungates the physical signal via clk_prepare is irrelevant from the api's perspective. clk_enable is how we turn gateable clocks on, even if the hardware really gets affected by the prior call to clk_prepare. It would be a real mess if drivers knew the details of their underlying clock hardware and only called clk_prepare and never clk_enable to gate/ungate their clocks. Regards, Mike > > 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