Re: [RFC, PATCH] watchdog on gpio

[Mike Frysinger]

On Jan 10, 2008 10:11 AM, Marc Pignat <marc.pignat@xxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/