linux-mips
[Top] [All Lists]

Re: [PATCH] Added MIPS RM9K watchdog driver

To: thomas@koeller.dyndns.org
Subject: Re: [PATCH] Added MIPS RM9K watchdog driver
From: Dave Jones <davej@redhat.com>
Date: Fri, 11 Aug 2006 16:56:39 -0400
Cc: wim@iguana.be, linux-kernel@vger.kernel.org, Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org
In-reply-to: <200608102319.13679.thomas@koeller.dyndns.org>
Mail-followup-to: Dave Jones <davej@redhat.com>, thomas@koeller.dyndns.org, wim@iguana.be, linux-kernel@vger.kernel.org, Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <200608102319.13679.thomas@koeller.dyndns.org>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.4.2.2i
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

<Prev in Thread] Current Thread [Next in Thread>