lkml.org 
[lkml]   [2008]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2008-01-14 09:07    [W:0.033 / U:121.584 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site