Hi Andrew, thank you for your recommendations and corrections. I've considered all and changed the code. But first: Add support for it8718, which made the driver slightly more complex. Now the driver supports it8716, it8718, it8726, it8712-j and it8712-k, which are all 16-bit watchdog timer on ITE lpc-super-io-chips. Andrew Morton wrote: > On Wed, 27 Feb 2008 15:55:54 +0100 > Oliver Schuster wrote: > >> Hi, >> this patch add a hardware watchdog device driver. It supports the >> 16 bit watchdog timer on superio chip it8712f-j, it 8712f-k, it8716f >> and it8726f. Today it was tested under vanilla 2.6.24.3, vanilla >> 2.6.24.2 and Mandriva with 2.6.24.2 and 2.6.24 kernel. >> >> ... >> > >> static int exclusiv = DEFAULT_EXCLUSIV; > > Could we put the "e" back into "exclusive" please? exclusive -- sorry 8) >> + timeout = t; >> + >> + if (wdt_status & BIT_MASK(WDTS_TIMER_RUN)) { >> + spin_lock_irqsave(&spinlock, flags); > > Are you sure we are OK testing WDTS_TIMER_RUN prior to taking the lock? > Should that test happen inside the lock? You are right, changed. About the spinlock, i use it primary to ensure that at the same time only one task can have access to the superio chip configuration register. > The driver ..., but it uses this > open-coded test when reading bits. ... test_bit() is now used. > Is this comment about `buf' true? Comment about wdt_write is changed. >> + if (c == 'V') >> + expect_close = WD_MAGIC; > > So we support a write of "VVVVVVVVVVV"? Seems odd. I prefer "echo V>/dev/watchdog" (in the case that nowayout is NOT set) to stop the watchdog. But any write which contain the magic can stop the watchdog on next close of the device. This code originates from softdog.c, now slightly changed. >> +/* >> + * wdt_ioctl: >> + * @inode: inode ... >> ... > > The comments in this file are _almost_ in kerneldoc format, but they > actually aren't. Please convert them fully (and test it!) Most of comments are from the code, that i used as template. These commentaries seems to be kerneldoc, but aren't. I changed this and add all needful. To extract the kerneldoc, i like this colorful way: scripts/kernel-doc -html ... | lynx -stdin >> +config IT87_WDT >> + tristate "IT87 Watchdog Timer" >> + depends on EXPERIMENTAL >> + ---help--- >> + This is the ... > > Surely this needs more dependencies. The driver is x86(?)-specific and > blindly pokes at IO ports prior to determining whether the device is > actually present. ... The driver hwmon/it87.c use the same way to detect the ITE superio chip by poking at IO ports. It dependence on HAS_IOMEM, but i decided to be more strictly and use X86, because the low pin count interface is created for this architecture and the IO base addresses used by these chips are X86 specific. Regards, Oliver