On Thu, Aug 10, 2006 at 11:19:13PM +0200, thomas@xxxxxxxxxxxxxxxxxx wrote:
> This is a driver for the on-chip watchdog device found on some
> MIPS RM9000 processors.
>
> Signed-off-by: Thomas Koeller <thomas.koeller@xxxxxxxxxxxxx>
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
