Messages in this thread | | | Date | Mon, 14 Jan 2008 03:04:36 -0500 | From | "Mike Frysinger" <> | Subject | Re: [RFC, PATCH] watchdog on gpio |
| |
On Jan 10, 2008 10:11 AM, Marc Pignat <marc.pignat@hevs.ch> 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
| |