Hi, 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 >Could we put the "e" back... exclusive 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 ... 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