Re: add bcm47xx watchdog driver

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 04 Jun 2009 22:24:56 +0200
matthieu castet <castet.matthieu@free.fr> wrote:

> 
>
> This add watchdog driver for broadcom 47xx device.
> It uses the ssb subsytem to access embeded watchdog device.
> 
> Because the watchdog timeout is very short (about 2s), a soft timer is used
> to increase the watchdog period.
> 
> Note : A patch for exporting the ssb_watchdog_timer_set will
> be submitted on next linux-mips merge. Without this patch it can't 
> be build as a module.
> 
>
> ...
>
> --- linux-2.6.orig/drivers/watchdog/Kconfig	2009-05-25 22:22:02.000000000 +0200
> +++ linux-2.6/drivers/watchdog/Kconfig	2009-05-25 22:26:06.000000000 +0200
> @@ -764,6 +764,12 @@
>  	help
>  	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
>  
> +config BCM47XX_WDT
> +    tristate "Broadcom BCM47xx Watchdog Timer"
> +    depends on BCM47XX
> +    help
> +      Hardware driver for the Broadcom BCM47xx Watchog Timer.
> +

Please indent the kconfig body with a single tab character.

>
> ...
>
> +#define DRV_NAME		"bcm47xx_wdt"
> +
> +#define WDT_DEFAULT_TIME	30	/* seconds */
> +#define WDT_MAX_TIME		256	/* seconds */
> +
> +static int wdt_time = WDT_DEFAULT_TIME;
> +static int nowayout = WATCHDOG_NOWAYOUT;
> +
> +module_param(wdt_time, int, 0);
> +MODULE_PARM_DESC(wdt_time, "Watchdog time in seconds. (default="
> +				__MODULE_STRING(WDT_DEFAULT_TIME) ")");
> +
> +#ifdef CONFIG_WATCHDOG_NOWAYOUT
> +module_param(nowayout, int, 0);
> +MODULE_PARM_DESC(nowayout,
> +		"Watchdog cannot be stopped once started (default="
> +				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +#endif

hm, now what's happening with the third arg to module_param()?

> +static struct platform_device *bcm47xx_wdt_platform_device;
> +
> +static unsigned long bcm47xx_wdt_busy;
> +static char expect_release;
> +static struct timer_list wdt_timer;
> +static atomic_t ticks;
> +
> +static inline void bcm47xx_wdt_hw_start(void)
> +{
> +	/* this is 2,5s on 100Mhz clock  and 2s on 133 Mhz */
> +	ssb_watchdog_timer_set(&ssb_bcm47xx, 0xfffffff);
> +}
> +
> +static inline int bcm47xx_wdt_hw_stop(void)
> +{
> +	return ssb_watchdog_timer_set(&ssb_bcm47xx, 0);
> +}
> +
> +static void bcm47xx_timer_tick(unsigned long unused)
> +{
> +	if(!atomic_dec_and_test(&ticks)) {

Please pass this patch (and all others) through scripts/checkpatch.pl
and review the resulting output.

> +		bcm47xx_wdt_hw_start();
> +		mod_timer(&wdt_timer, jiffies + HZ);
> +	}
> +	else {
> +		printk(KERN_CRIT PFX "Watchdog will fire soon!!!.\n");
> +	}
> +}
> +
> +static inline void bcm47xx_wdt_pet(void)
> +{
> +	atomic_set(&ticks, wdt_time);
> +}

What does "pet" stand for?

> +static void bcm47xx_wdt_start(void)
> +{
> +	bcm47xx_wdt_pet();
> +	bcm47xx_timer_tick(0);
> +}
> +
> +static void bcm47xx_wdt_pause(void)
> +{
> +	del_timer(&wdt_timer);

Should this be del_timer_sync()?  The timer callback can still be
executing after del_timer() returns.

> +	bcm47xx_wdt_hw_stop();
> +}
> +
> +static void bcm47xx_wdt_stop(void)
> +{
> +	bcm47xx_wdt_pause();
> +}
> +
> +static int bcm47xx_wdt_settimeout(int new_time)
> +{
> +	if ((new_time <= 0) || (new_time > WDT_MAX_TIME))
> +		return -EINVAL;
> +
> +	wdt_time = new_time;
> +	return 0;
> +}
> +
> +static int bcm47xx_wdt_open(struct inode *inode, struct file *file)
> +{
> +	if (test_and_set_bit(0, &bcm47xx_wdt_busy))
> +		return -EBUSY;
> +
> +	bcm47xx_wdt_start();
> +	return nonseekable_open(inode, file);
> +}
> +
> +static int bcm47xx_wdt_release(struct inode *inode, struct file *file)
> +{
> +	if (expect_release == 42) {
> +		bcm47xx_wdt_stop();
> +	} else {
> +		printk(KERN_CRIT DRV_NAME ": Unexpected close, not stopping watchdog!\n");

Can this happen?

> +		bcm47xx_wdt_start();
> +	}
> +
> +	clear_bit(0, &bcm47xx_wdt_busy);
> +	expect_release = 0;
> +	return 0;
> +}
> +
>
> ...
>