Re: [PATCH] Added MIPS RM9K watchdog driver

[Dave Jones <davej@redhat.com>]

On Thu, Aug 10, 2006 at 11:19:13PM +0200, thomas@koeller.dyndns.org wrote:
 > This is a driver for the on-chip watchdog device found on some
 > MIPS RM9000 processors.
 > 
 > Signed-off-by: Thomas Koeller <thomas.koeller@baslerweb.com>

Mostly same nit-picking comments as your other driver..

 > +++ b/drivers/char/watchdog/rm9k_wdt.c
 > ... 
 > + 
 > +#include <linux/config.h>

not needed.

 > +/* Function prototypes */
 > +static int __init wdt_gpi_probe(struct device *);
 > +static int __exit wdt_gpi_remove(struct device *);
 > +static void wdt_gpi_set_timeout(unsigned int);
 > +static int wdt_gpi_open(struct inode *, struct file *);
 > +static int wdt_gpi_release(struct inode *, struct file *);
 > +static ssize_t wdt_gpi_write(struct file *, const char __user *, size_t, 
 > loff_t *);
 > +static long wdt_gpi_ioctl(struct file *, unsigned int, unsigned long);
 > +static const struct resource *wdt_gpi_get_resource(struct platform_device *, 
 > const char *, unsigned int);
 > +static int wdt_gpi_notify(struct notifier_block *, unsigned long, void *);
 > +static irqreturn_t wdt_gpi_irqhdl(int, void *, struct pt_regs *);

Can probably (mostly?) go away with some creative reordering.

 > +static int locked = 0;

unneeded initialisation.

 > +static int nowayout =
 > +#if defined(CONFIG_WATCHDOG_NOWAYOUT)
 > +	1;
 > +#else
 > +	0;
 > +#endif

static int nowayout = CONFIG_WATCHDOG_NOWAYOUT;

should work.

 > +static void wdt_gpi_set_timeout(unsigned int to)
 > +{
 > +	u32 reg;
 > +	const u32 wdval = (to * CLOCK) & ~0x0000000f;
 > +
 > +	lock_titan_regs();
 > +	reg = titan_readl(CPCCR) & ~(0xf << (wd_ctr * 4));
 > +	titan_writel(reg, CPCCR);
 > +	wmb();
 > +	__raw_writel(wdval, wd_regs + 0x0000);
 > +	wmb();
 > +	titan_writel(reg | (0x2 << (wd_ctr * 4)), CPCCR);
 > +	wmb();
 > +	titan_writel(reg | (0x5 << (wd_ctr * 4)), CPCCR);
 > +	iob();
 > +	unlock_titan_regs();
 > +}

As in the previous driver, are these barriers strong enough?
Or do they need explicit reads of the written addresses to flush the write?
 
		Dave

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