On Jan 10, 2008 10:11 AM, Marc Pignat <[EMAIL PROTECTED]> wrote: > watchdog driver for embedded systems with a supervisor watchdog (MAX823 or so) > connected to a gpio. This is the platform_driver and needs platform_data for > defining the gpio pin and the watchdog timeout.
generic gpio watchdog is good stuff ... > #include <linux/gpio_wdt.h> perhaps "watchdog" rather than "wdt" considering it's already "linux/watchdog.h" ? > --- a/drivers/watchdog/gpio_wdt.c 1970-01-01 01:00:00.000000000 +0100 > +++ b/drivers/watchdog/gpio_wdt.c 2008-01-10 16:06:09.000000000 +0100 > +#include <asm/arch/gpio.h> this should be <asm/gpio.h> > +static struct watchdog_info ident = { > + .firmware_version = 0, no point in setting this i dont think ... > +static int gpio_wdt_ioctl(struct inode *inode, struct file *file, > + unsigned int cmd, unsigned long arg) > +{ > + case WDIOC_KEEPALIVE: > + gpio_wdt_keepalive(wdt); > + return 0; this two lines should be merged. > + default: > + return -ENOIOCTLCMD; should be -ENOTTY like all the other watchdogs > +static char banner[] __initdata = KERN_INFO PFX "fixed %d.%03d seconds > timeout (nowayout= %d)\n"; this only gets used once in the init function ... having it be broken out like this is kind of silly > +static int __init gpio_wdt_probe(struct platform_device *pdev) shouldnt this be __devinit ? > + if (watchdog) { > + printk(KERN_ERR PFX "only one device supported\n"); > + return -ENODEV; > + } why ? it'd be trivial to abstract this driver away from a global "watchdog" state into a proper arbitrary # of gpio watchdogs. > +static int __exit gpio_wdt_remove(struct platform_device *pdev) shouldnt this be __devexit ? > +#define gpio_wdt_suspend() > +#define gpio_wdt_resume() i'm pretty sure this is incorrect and instead should read: #define gpio_wdt_suspend NULL #define gpio_wdt_resume NULL > +static struct platform_driver gpio_wdt_driver = { > + .probe = gpio_wdt_probe, > + .remove = __exit_p(gpio_wdt_remove), once you fix the remove to be __devexit, this should be __devexit_p() > --- a/drivers/watchdog/Kconfig 2008-01-08 17:30:30.000000000 +0100 > +++ b/drivers/watchdog/Kconfig 2008-01-10 14:00:30.000000000 +0100 > +config GPIO_WATCHDOG > + tristate "Support for GPIO connected watchdog" > + depends on GENERIC_GPIO > + help > + This option enables support for a MAX823-like watchdog connected to > + GPIO input/output. this is misleading/confusing. suggested rewording: This option enables support for a watchdog connected via GPIO lines (such as the MAX823). To compile this driver as a module, choose M here: the module will be called gpio_wdt. > --- a/include/linux/gpio_wdt.h 1970-01-01 01:00:00.000000000 +0100 > +++ b/include/linux/gpio_wdt.h 2008-01-10 15:48:42.000000000 +0100 > +#include <asm/arch/hardware.h> no idea why you're including this as it is surely incorrect and not needed. please remove it and add <linux/types.h>. > +#define GPIO_WDT_DRIVER_NAME "gpio_wdt" this belongs in the driver C file, not the header. -mike -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/