Re: [PATCH 2.6] Watchdog timer for Intel IXP4xx CPUs

[Jeff Garzik]

Deepak Saxena wrote:
> On May 11 2004, at 14:33, Andrew Morton was caught saying:
> +#ifdef CONFIG_WATCHDOG_NOWAYOUT
> +static int nowayout = 1;
> +#else
> +static int nowayout = 0;
> +#endif
> +static int heartbeat = 60;	/* (secs) Default is 1 minute */
> +static unsigned long wdt_status = 0;	
> +static unsigned long boot_status = 0;

Wastes bss(?) space to explicitly zero statics.


> +#define WDT_TICK_RATE (IXP4XX_PERIPHERAL_BUS_CLOCK * 1000000UL)
> +
> +#define	WDT_IN_USE		0
> +#define	WDT_OK_TO_CLOSE		1
> +
> +static void
> +wdt_enable(void)
> +{
> +	*IXP4XX_OSWK = IXP4XX_WDT_KEY;
> +	*IXP4XX_OSWE = 0;
> +	*IXP4XX_OSWT = WDT_TICK_RATE * heartbeat;
> +	*IXP4XX_OSWE = IXP4XX_WDT_COUNT_ENABLE | IXP4XX_WDT_RESET_ENABLE;
> +	*IXP4XX_OSWK = 0;
> +}
> +
> +static void 
> +wdt_disable(void)
> +{
> +	*IXP4XX_OSWK = IXP4XX_WDT_KEY;
> +	*IXP4XX_OSWE = 0;
> +	*IXP4XX_OSWK = 0;
> +}

Are these magic CPU addresses you're writing to?  Normally one uses bus 
macros to read/write to devices.  But SoC and embedded stuff is often 
special...


> +static int
> +ixp4xx_wdt_open(struct inode *inode, struct file *file)
> +{
> +	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
> +		return -EBUSY;
> +
> +	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
> +
> +	wdt_enable();
> +
> +	return 0;
> +}

You typically want semaphores rather than bit tests.

And what about blocking on multiple openers?


> +static ssize_t 
> +ixp4xx_wdt_write(struct file *file, const char *data, size_t len, loff_t *ppos)
> +{
> +	/* Can't seek (pwrite) on this device  */
> +	if (ppos != &file->f_pos)
> +		return -ESPIPE;
> +
> +	if (len) {
> +		if (!nowayout) {
> +			size_t i;
> +
> +			clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
> +
> +			for (i = 0; i != len; i++) {
> +				char c;
> +
> +				if (get_user(c, data + i))
> +					return -EFAULT;
> +				if (c == 'V')
> +					set_bit(WDT_OK_TO_CLOSE, &wdt_status);
> +			}
> +		}
> +		wdt_enable();
> +	}

do other watchdog drivers really twiddle a bit like this on each write?

Otherwise looks OK.

	Jeff



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/