Messages in this thread | | | Date | Sun, 4 Nov 2007 20:11:33 +0100 | From | Wim Van Sebroeck <> | Subject | Re: [HP ProLiant WatchDog driver] hpwdt HP WatchDog Patch <resend> |
| |
Hi Thomas,
I reviewed yor code and have ome remarks: * All watchdog device drivers should work with seconds. Your driver works with minutes. Please change to seconds. * The misc_register function (which opens your driver towards user-space) should be the last iregister-action you take into the pci probe function. (So just before you do the printk where you say that the driver has been initialized). * The open and release functions should make sure that /dev/watchdog can only be opened once. * /dev/watchdog is a VFS (Virtual File System) so your open function should return with "return nonseekable_open(inode, file);". * please put the code to start the watchdog in a seperate function (this will make it easier to migrate your driver to the generic watchdog model in the future). * If you watchdog can't be stopped then you should use a timer to make sure that when /dev/watchdog is closed, that you continue to keepalive/ping the watchdog device. (please see mixcomwd.c (this device has the same problem)). * please put the code to change the timeout in a seperate function. * the default return value for the ioctl calls should be -ENOTTY (instead of -ENOIOCTLCMD). * the GETSTATUS call should also have a "put_user(0, (int *)arg);". * if possible please add sparse testing code into the ioctl handling. * Personnaly: I would rather see the generic smbios code somewhere in the arch/x86/ directory. We will probably need that also in other (non-watchdog) drivers in the future.
Greetings, Wim.
- 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/
| |