Hi,
A few minor comments inline. It's also worth running your patch through
scripts/checkpatch.pl as there are some useful warnings in there.
Jamie
On Tue, Jan 04, 2011 at 03:34:54PM +0800, Macpaul Lin wrote:
> Support Faraday ftwdt010 watchdog driver.
>
> Signed-off-by: Macpaul Lin <macpaul@xxxxxxxxxxxxx>
> ---
> drivers/watchdog/Kconfig | 6 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/ftwdt010_wdt.c | 178 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 185 insertions(+), 0 deletions(-)
> create mode 100644 drivers/watchdog/ftwdt010_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 3711b88..86deb80 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -55,6 +55,12 @@ config SOFT_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called softdog.
>
> +config FTWDT010_WATCHDOG
> + tristate "FTWDT010_WATCHDOG"
> + help
> + Support for Faraday ftwdt010 watchdog. When the watchdog triigers the
s/triigers/triggers
> + system will be reset.
> +
> config WM831X_WATCHDOG
> tristate "WM831x watchdog"
> depends on MFD_WM831X
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 699199b..a3b9bee 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -142,6 +142,7 @@ obj-$(CONFIG_WATCHDOG_CP1XXX) += cpwd.o
> # XTENSA Architecture
>
> # Architecture Independant
> +obj-$(CONFIG_FTWDT010_WATCHDOG) += ftwdt010_wdt.o
> obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o
> obj-$(CONFIG_WM8350_WATCHDOG) += wm8350_wdt.o
> obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o
> diff --git a/drivers/watchdog/ftwdt010_wdt.c b/drivers/watchdog/ftwdt010_wdt.c
> new file mode 100644
> index 0000000..41447d1
> --- /dev/null
> +++ b/drivers/watchdog/ftwdt010_wdt.c
> @@ -0,0 +1,178 @@
> +/*
> + * Watchdog driver for the FTWDT010 Watch Dog Driver
> + *
> + * (c) Copyright 2004 Faraday Technology Corp. (www.faraday-tech.com)
> + * Based on sa1100_wdt.c by Oleg Drokin <green@xxxxxxxxxx>
> + * Based on SoftDog driver by Alan Cox <alan@xxxxxxxxxx>
> + *
> + * 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.
> + *
> + * 27/11/2004 Initial release
> + */
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/watchdog.h>
> +#include <linux/smp_lock.h>
> +#include <linux/delay.h>
I don't think your driver needs smp_lock.h and delay.h.
> +#include <asm/uaccess.h>
> +
> +#define DEBUG( str, ...) \
> + do{ \
> + if( debug) \
> + printk( str, ##__VA_ARGS__); \
> + } while(0)
Best to use dev_dbg() rather than creating new macros for this.
> +#define wdcounter (*( volatile unsigned long *)( WDT_FTWDT010_VA_BASE + 0x00))
> +#define wdload (*( volatile unsigned long *)( WDT_FTWDT010_VA_BASE + 0x04))
> +#define wdrestart (*( volatile unsigned long *)( WDT_FTWDT010_VA_BASE + 0x08))
> +#define wdcr (*( volatile unsigned long *)( WDT_FTWDT010_VA_BASE + 0x0C))
> +#define wdstatus (*( volatile unsigned long *)( WDT_FTWDT010_VA_BASE + 0x10))
> +#define wdclear (*( volatile unsigned long *)( WDT_FTWDT010_VA_BASE + 0x14))
> +#define wdintrcter (*( volatile unsigned long *)( WDT_FTWDT010_VA_BASE + 0x18))
You shouldn't really be using direct accessors and volatile like this.
The usual way would be to have something like a platform_device and then
a platform_driver and ioremap() the memory. You can then use ioreadN()
and iowriteN() to access the registers and you can cope with platforms
where the WDT is in different locations.
> +#define TIMER_MARGIN 60 /* (secs) Default is 1 minute */
> +#define RESTART_MAGIC 0x5AB9
> +#define PCLK (AHB_CLK_IN / 2)
Do you need to use the clk API (linux/clk.h) to make sure that the pclk
is enabled? You could also use clk_get_rate() to make sure that this
driver works on different speed devices.
> +static int debug = 0;
> +static int timeout = TIMER_MARGIN; /* in seconds */
> +
> +module_param(debug, int, 0);
> +module_param(timeout, int, 0);
It's nice if you use MODULE_PARM_DESC() here to provide some
documentation for the timeout. If you use dev_dbg() then you can
probably lose the debug parameter.
> +
> +static int ftwdt010_dog_open(struct inode *inode, struct file *file){
> +
> +#if 0
> + /* Allow only one person to hold it open */
> + if( test_and_set_bit( 1, &ftwdt010_wdt_users))
> + return -EBUSY;
> +
> + ftwdt010_wdt_users = 1;
> +#endif
Why the #if 0?
> + DEBUG("Activating WDT..\n");
> +
> + wdcr = 0;
> + wdload = PCLK * timeout;
> + wdrestart = RESTART_MAGIC; /* Magic number */
> + wdcr = 0x03; /* Enable WDT */
> +
> + return 0;
return nonseekable_open()?
> +}
> +
> +static int ftwdt010_dog_release(struct inode *inode, struct file *file){
> +
> +#ifndef CONFIG_WATCHDOG_NOWAYOUT
> + /*
> + * Shut off the timer.
> + * Lock it in if it's a module and we defined ...NOWAYOUT
> + */
> + wdcr = 0;
> + DEBUG( "Deactivating WDT..\n");
> +#endif
> + return 0;
> +}
> +
> +static ssize_t ftwdt010_dog_write(struct file *file, const char *data, size_t len, loff_t *ppos){
> +
> + if(len){
> +
Extraneous blank line.
> + wdrestart = RESTART_MAGIC;
> + return 1;
Most other drivers return the length of the buffer rather than 1 here.
> + }
> +
> + return 0;
> +}
> +
> +static int ftwdt010_dog_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg){
We should be using unlocked_ioctl now so this should return a long.
> +
> + static struct watchdog_info ident = {
> + .identity = "FTWDT010 watchdog",
> + };
> +
> + int time = 0;
> + void __user *argp = (void __user *)arg;
> + int __user *p = argp;
> +
> + switch( cmd){
> +
> + case WDIOC_GETSUPPORT:
> + return copy_to_user(argp, &ident, sizeof( ident));
> +
> + case WDIOC_GETSTATUS:
> + return put_user(0, p);
> +
> + case WDIOC_GETBOOTSTATUS:
> + return put_user(wdstatus ? WDIOF_CARDRESET : 0, p);
> +
> + case WDIOC_SETTIMEOUT:
> +
> + if( get_user( time, p))
> + return 0;
> +
> + if( time <= 0 || time > 255)
> +
> + return -EINVAL;
> +
> + timeout = time;
> + wdload = PCLK * timeout;
> + wdrestart = RESTART_MAGIC;
> +
> + return put_user(timeout, p);
> +
> + case WDIOC_GETTIMEOUT:
> + return put_user(timeout, p);
> +
> + case WDIOC_KEEPALIVE:
> + wdrestart = RESTART_MAGIC;
> + return 0;
> +
> + default:
> + return -ENOIOCTLCMD;
> +
> + }
> +}
> +
> +static struct file_operations ftwdt010_dog_fops = {
> +
> + .owner = THIS_MODULE,
> + .llseek = no_llseek,
> + .write = ftwdt010_dog_write,
> + .ioctl = ftwdt010_dog_ioctl,
.unlocked_ioctl rather than .ioctl.
> + .open = ftwdt010_dog_open,
> + .release = ftwdt010_dog_release,
> +};
This could be const.
> +
> +static struct miscdevice ftwdt010_dog_miscdev = {
> +
> + WATCHDOG_MINOR,
> + "FTWDT010 watchdog",
> + &ftwdt010_dog_fops
> +};
Better to use explicit initializers here (.minor = WATCHDOG_MINOR...)
etc.
> +
> +static int __init ftwdt010_dog_init( void){
> +
> + int ret;
> +
> + ret = misc_register(&ftwdt010_dog_miscdev);
> +
> + if( ret)
> + return ret;
> +
> + DEBUG("FTWDT010 watchdog timer: timer timeout %d sec\n", timeout);
> +
> + return 0;
> +}
> +
> +static void __exit ftwdt010_dog_exit( void){
> +
> + misc_deregister(&ftwdt010_dog_miscdev);
> +}
> +
> +module_init(ftwdt010_dog_init);
> +module_exit(ftwdt010_dog_exit);
> +MODULE_AUTHOR("Faraday Corp.");
> +MODULE_LICENSE("GPL");
> --
> 1.7.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
