From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Kagstrom <simon.kagstrom@netinsight.net>
Date: Wed, 28 Oct 2009 15:49:25 +0100
Subject: [U-Boot] [PATCH 1/2]: common: Add a watchdog CLI command
In-Reply-To: <73173D32E9439E4ABB5151606C3E19E202F200E72E@SC-VEXCH1.marvell.com>
References: <20091028151424.6afeb1b4@marrow.netinsight.se>
	<73173D32E9439E4ABB5151606C3E19E202F200E72E@SC-VEXCH1.marvell.com>
Message-ID: <20091028154925.1dd1a8a9@marrow.netinsight.se>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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