Messages in this thread Patch in this message | | | Date | Tue, 17 Oct 2006 16:31:16 -0700 | From | Andrew Morton <> | Subject | Re: [patch 1/1] watchdog driver for Digital-Logic MSM-P5XEN PC104 unit |
| |
On 17 Oct 2006 12:34:40 -0000 ggaleotti@interfree.it wrote:
> > > A simple watchdog driver for Digital-Logic's MSM-P5XEN PC104 unit. > The watchdog is a LTC1232 controlled by a single I/O port @ 0x1037. > The watchdog must be refreshed (writing a single byte) to the device > at least every 600 msecs (which is a little of overhead, but PC104 > industrial applications requires a high degree of safety/reliability.) >
Thanks. A few coding-style and minor things:
> --- linux-2.6.18.1/drivers/char/watchdog/dlpc104.c.orig 2006-10-17 14:48:39.000000000 +0200 > +++ linux-2.6.18.1/drivers/char/watchdog/dlpc104.c 2006-10-17 14:48:27.000000000 +0200 > @@ -0,0 +1,228 @@ > + > +/* > + * dlpc104: watchdog driver for Digital-Logic MSM-P5XEN PC104 unit > + * > + * Copyright (c) 2006 Gabriele Galeotti <ggaleotti@interfree.it> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version > + * 2 of the License, or (at your option) any later version. > + * > + * Version 0.1 (06/10/2006): > + * - first version > + * > + * Please see ggaleotti.interfree.it for latest version. > + */ > + > +#include <linux/config.h> > +#include <linux/version.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/init.h> > +#include <asm/io.h> > +#include <linux/delay.h> > +#include <asm/byteorder.h> > +#include <asm/uaccess.h> > + > +#include <linux/atmdev.h> Why does it need atmdev.h?
> +#include <linux/miscdevice.h> > + > +#include <linux/watchdog.h> > +#include <linux/reboot.h> > + > +#define WDT_IOBASE 0x1000 > +#define WDT_OFFSET 0x37 > + > +static int io_base = WDT_IOBASE; > +module_param(io_base, int, 0); > +MODULE_PARM_DESC(io_base, "I/O base address (default=0x1000)"); > + > +static struct timer_list wdt_timer; > + > +static void > +wdt_enable(unsigned long data)
Please put the declaration on a single line:
static void wdt_enable(unsigned long data)
> +{ > + (void)data;
This is unneeded.
> + outb(inb(io_base + WDT_OFFSET) & 0xf7, io_base + WDT_OFFSET); > +} > + > +static void > +wdt_disable(void) > +{ > + outb(inb(io_base + WDT_OFFSET) | 0x08, io_base + WDT_OFFSET); > +} > + > +static void > +wdt_ping(void) > +{ > + /* > + * Clear-pulse trailing edge scheduling. > + * > + * We use mod_timer() rather than add_timer() because a timer could > + * be already activated. > + * kernel/timer.c: > + * "... since add_timer() cannot modify an already running timer." > + */ > + mod_timer(&wdt_timer, jiffies + (HZ / 10)); > + > + wdt_disable(); > +} > + > +static int > +dl_wdt_open(struct inode *inode, struct file *file) > +{ > + wdt_enable(0); > + > + return nonseekable_open(inode, file); > +} > + > +static ssize_t > +dl_wdt_write(struct file *file, const char *data, size_t length, loff_t *ppos) > +{ > + (void)file; > + (void)data; > + (void)ppos;
Those three statements are unneeded.
> + if (length != 0) > + {
Please do
if (length != 0) { (entire patch)
> + wdt_ping(); > + } > + > + return length; > +} > + > +static int > +dl_wdt_close(struct inode *inode, struct file *file) > +{ > + del_timer(&wdt_timer);
This might need to be del_timer_sync().
> + wdt_disable(); > + > + return 0; > +} > + > +static struct watchdog_info identity = { > + .options = 0, > + .identity = "DLPC104 Watchdog", > + };
That closing brace should be in column 1.
> +static int > +dl_wdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg) > +{
We prefer to fit code into an 80-col window, so this could be laid out thus:
static int dl_wdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
> + int return_status; > + > + switch (cmd) > + { > + case WDIOC_GETSUPPORT: > + return_status = copy_to_user((struct watchdog_info *)arg, &identity, sizeof(identity)) ? -EFAULT : 0; > + break; > + default: > + return_status = -ENOIOCTLCMD; > + break; > + }
We normally indent the body of the switch one tabstop less than this. So:
switch (cmd) { case WDIOC_GETSUPPORT: return_status = copy_to_user((struct watchdog_info *)arg, &identity, sizeof(identity)); if (return_status) return_status = -EFAULT; break; default: return_status = -ENOIOCTLCMD; break; }
also, `arg' should have been cast to `struct watchdog_info __user *'.
> + return return_status; > +} > + > +static int > +wdt_notify_sys(struct notifier_block *this, unsigned long code, void *unused) > +{ > + if (code == SYS_DOWN || code == SYS_HALT) > + { > + del_timer(&wdt_timer); > + wdt_disable(); > + } > + > + return NOTIFY_DONE; > +}
Similar things here.
> +static struct file_operations dlmodule_fops = { > + .owner = THIS_MODULE, > + .llseek = no_llseek, > + .write = dl_wdt_write, > + .ioctl = dl_wdt_ioctl, > + .open = dl_wdt_open, > + .release = dl_wdt_close > + }; > + > +static struct miscdevice dlmodule_miscdevice = { > + .minor = WATCHDOG_MINOR, > + .name = "watchdog", > + .fops = &dlmodule_fops, > + }; > + > +/* > + * The WDT needs to learn about soft shutdowns in order to turn the timebomb > + * registers off. > + */ > +static struct notifier_block wdt_notifier = { > + .notifier_call = wdt_notify_sys, > + }; > + > +static int __init > +dlmodule_init(void) > +{ > + int result; > + > + if (!request_region(io_base + WDT_OFFSET, 1, "DLPC104 Watchdog")) > + { > + printk(KERN_ERR "DLPC104 Watchdog: I/O region busy.\n"); > + result = -EBUSY; > + goto exit_0; > + } > + > + result = register_reboot_notifier(&wdt_notifier); > + if (result != 0) > + { > + printk (KERN_ERR "DLPC104 Watchdog: cannot register reboot notifier.\n"); > + goto exit_1; > + } > + > + result = misc_register(&dlmodule_miscdevice); > + if (result != 0) > + { > + printk(KERN_ERR "DLPC104 Watchdog: cannot register.\n"); > + goto exit_2; > + } > + > + init_timer(&wdt_timer); > + wdt_timer.function = wdt_enable; > + wdt_timer.data = 0;
setup_timer() can be used here.
> + printk("Digital-Logic MSM-P5XEN PC104 Watchdog driver loaded.\n"); > + > + return 0; > + > +exit_2: unregister_reboot_notifier(&wdt_notifier); > +exit_1: release_region(io_base + WDT_OFFSET, 1); > +exit_0: return result;
Like this, please:
exit_2: unregister_reboot_notifier(&wdt_notifier); exit_1: release_region(io_base + WDT_OFFSET, 1); exit_0: return result;
> +} > + > +static void __exit > +dlmodule_exit(void) > +{ > + del_timer(&wdt_timer);
del_timer_sync().
> + wdt_disable(); > + > + release_region(io_base + WDT_OFFSET, 1); > + > + unregister_reboot_notifier(&wdt_notifier); > + > + misc_deregister(&dlmodule_miscdevice); > +} > + > +/* > + * Module parameters & definitions. > + */ > + > +MODULE_AUTHOR("Gabriele Galeotti <ggaleotti@interfree.it>"); > +MODULE_DESCRIPTION("Digital-Logic PC104 watchdog driver"); > +MODULE_LICENSE("GPL"); > +MODULE_VERSION("0.1"); > +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR); > + > +module_init(dlmodule_init); > +module_exit(dlmodule_exit);
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |