On 3/7/12, Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx> wrote:
> On Wed, Mar 07, 2012 at 03:57:55PM +0530, Viresh Kumar wrote:
>> @@ -156,10 +159,22 @@ static int mpcore_wdt_open(struct inode *inode,
>> struct file *file)
>> + ret = clk_enable(wdt->clk);
>
> What about preparing the clock?
>
I missed that. Will update prepare/unprepare of clk across driver.
>> + 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.
>> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
