Re: [RFC, PATCH] watchdog on gpio

Mike Frysinger
2008-01-14T08:04:46Z

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