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.
...
> @@ -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.
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).
Similar comments for clkdm_hwmod_disable.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
