Quoting Stefan Assmann (2014-07-31 05:04:29)

On 31.07.2014 13:05, Mark Brown wrote:

quoted

On Thu, Jul 31, 2014 at 11:56:15AM +0200, Stefan Assmann wrote:

quoted

On 30.07.2014 19:50, Mark Brown wrote:

quoted

On Wed, Jul 30, 2014 at 04:02:29PM +0200, Stefan Assmann wrote:

quoted

quoted

quoted

+static int twl6030_clk32kg_is_prepared(struct clk_hw *hw)
+{
+  struct twl6030_desc *desc = to_twl6030_desc(hw);
+
+  return desc->enabled;
+}

quoted

quoted

Why not just check the register map - can't the register be cached?  If
that's not possible a comment would be good.

quoted

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.

quoted

quoted

quoted

quoted

+  else
+          clk_prepare(clk);

quoted

quoted

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.

quoted

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html