Hi Todd,
On 6/26/2011 10:07 PM, Todd Poynor wrote:
On Fri, Jun 24, 2011 at 02:06:32PM +0200, Benoit Cousson wrote:
Duplicate the existing API for clockdomain enable from clock to enable
a clock domain from hwmod framework.
This will be needed when the hwmod framework will move from the current
clock centric approach to the module based approach.
...
+static int _clkdm_clk_hwmod_disable(struct clockdomain *clkdm)
+{
+ if (!clkdm || !arch_clkdm || !arch_clkdm->clkdm_clk_disable)
+ return -EINVAL;
+
+#ifdef DEBUG
+ if (atomic_read(&clkdm->usecount) == 0) {
+ WARN_ON(1); /* underflow */
+ return -ERANGE;
+ }
+#endif
+
+ if (atomic_dec_return(&clkdm->usecount)> 0)
+ return 0;
Suggest taking the error return out of the #ifdef DEBUG code (mainly
to lessen the chance that enabling debugging features makes problems
appear or disappear, although the WARN_ON should be a prominent clue
in this case), and maybe just have the function always handle
underflow.
If your final point is that we'd better remove the #ifdef, I do agree.
...
@@ -877,35 +911,93 @@ int clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk)
*/
int clkdm_clk_disable(struct clockdomain *clkdm, struct clk *clk)
{
+ int ret = 0;
+
/*
* XXX Rewrite this code to maintain a list of enabled
* downstream clocks for debugging purposes?
*/
- if (!clkdm || !clk)
+ if (!clk)
return -EINVAL;
- if (!arch_clkdm || !arch_clkdm->clkdm_clk_disable)
+ ret = _clkdm_clk_hwmod_disable(clkdm);
+
+ /* All downstream clocks of this clockdomain are now disabled */
+ pr_debug("clockdomain: clkdm %s: clk %s now disabled\n", clkdm->name,
+ clk->name);
The pr_debug should be printed only if ret == 0.
That's not even enough, because the function is returning 0 even if the
domain is already enabled.
The comment seems true only if the clock domain's usecount went to zero.
The terminology here may be a bit confusing: the function does not actually
disable the named downstream clock in the usual sense
(as in clk_disable(clk), or any action to individually gate that
clock) Similar comments for clkdm_clk_enable).
Yeah, I kept that from the original code, but in fact, I'll probably get
rid of the pr_debug here and put it in the two helper functions.
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
