On Wed, 28 Oct 2009 07:29:35 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
+static int do_watchdog(cmd_tbl_t *cmdtp, int flag, int argc,
- if (timeout < 0)
goto usage;How about passing zero value here, will it be a correct input for watchdog_enable?
Good point, I'll update the patch.
- /* Everything fine, enable the watchdog */
- watchdog_enable(timeout);
Can we check for some error code here from lower layer and dump some error message? For ex. Specified timeout value may be invalid for specific h/w
We could, but I'd like to keep the interface simple. Basically: tell the hardware driver to enable the watchdog "as good as possible", and then the hardware will enable a watchdog that will timeout "sometime".
This is hardly an end-user issue anyway: he/she will test the board properly to find a good timeout value anyway, and I believe the interface can be kept simple. I just like it since it makes it simple to enable the watchdog where you like it in boot scripts etc.
+#if defined(CONFIG_WATCHDOG) || defined(CONFIG_HW_WATCHDOG) +extern void watchdog_enable(unsigned int timeout_secs);
+extern void watchdog_disable(void); +#else +static inline void watchdog_enable(unsigned int timeout_secs) { } +static inline void watchdog_disable(void) { } +#endif
What does this means?
It was just a way of making the interface calls valid (but empty) if the watchdog support isn't there. The idea is to avoid #ifdefs in the code (like for WATCHDOG_RESET).
// Simon