On 3/7/12, Russell King - ARM Linux [off-list ref] wrote:

On Wed, Mar 07, 2012 at 03:57:55PM +0530, Viresh Kumar wrote:

quoted

@@ -156,10 +159,22 @@ static int mpcore_wdt_open(struct inode *inode,

struct file *file)

quoted

+		ret = clk_enable(wdt->clk);

What about preparing the clock?

I missed that. Will update prepare/unprepare of clk across driver.

quoted

+	wdt->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(wdt->clk)) {
+		dev_warn(&pdev->dev, "Clock not found\n");
+		wdt->clk = NULL;

Remove this line.

Actually this was very much intentional, so that i can do
    if (wdt->clk).

Also, this makes more sense as some platform doesn't support clk,
so it is not an error for them if clk_get() fails. And thus the check

    if (wdt->clk)                /* i.e. clock is supported or not */

make more sense than

    if (!IS_ERR(wdt->clk))             /* i.e. there is an error while
getting clock */

from readability point of view. So, i would like to keep it as it is.

quoted

@@ -399,6 +446,9 @@ static int mpcore_wdt_suspend(struct device *dev)

 	if (test_bit(0, &wdt->timer_alive))
 		mpcore_wdt_stop(wdt);		/* Turn the WDT off */
+
+	if (wdt->clk)
+		clk_disable(wdt->clk);

Same comments...  And what if the watchdog is not open?  Doesn't this
disable an already disabled clock?

Yes, that'a a BUG. Will fix it.

--
viresh