watchdog: ep93xx: Use the WatchDog Timer Driver Core.

RFC PATCH Superseded H Hartley Sweeten 3 participants 10 messages Aug 1, 2011 – Aug 3, 2011 Watchdog device drivers

lore

Share & tools

lore

H Hartley Sweeten

author

2011-08-01 20:58 UTC #0 lore raw

RFC PATCH 1/1 watchdog: ep93xx: Use the WatchDog Timer Driver Core.

Signed-off-by H Hartley Sweeten <[email protected]>

Cc Wim Van Sebroeck <[email protected]> (maintainer:WATCHDOG DEVICE D...)

---

drivers/watchdog/ep93xx_wdt.c +39 ~~-135~~

index 726b7df..bb97351 100644
@@ -24,11 +24,9 @@
  */
 
 #include <linux/module.h>
-#include <linux/fs.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>
 #include <linux/timer.h>
-#include <linux/uaccess.h>
 #include <linux/io.h>
 #include <mach/hardware.h>

@@ -39,15 +37,17 @@
 #define WDT_TIMEOUT 30
 
 static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started");
+
 static int timeout = WDT_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout,
+	"Watchdog timeout in seconds. (1<=timeout<=3600, default="
+				__MODULE_STRING(WDT_TIMEOUT) ")");
 
 static struct timer_list timer;
 static unsigned long next_heartbeat;
-static unsigned long wdt_status;
-static unsigned long boot_status;
-
-#define WDT_IN_USE		0
-#define WDT_OK_TO_CLOSE		1
 
 #define EP93XX_WDT_REG(x)	(EP93XX_WATCHDOG_BASE + (x))
 #define EP93XX_WDT_WATCHDOG	EP93XX_WDT_REG(0x00)

@@ -71,130 +71,46 @@ static inline void wdt_ping(void)
 	__raw_writew(0x5555, EP93XX_WDT_WATCHDOG);
 }
 
-static void wdt_startup(void)
+static int void ep93xx_wdt_start(struct watchdog_device *wdd)
 {
-	next_heartbeat = jiffies + (timeout * HZ);
+	next_heartbeat = jiffies + (wdd->timeout * HZ);
 
 	wdt_enable();
 	mod_timer(&timer, jiffies + WDT_INTERVAL);
+	return 0;
 }
 
-static void wdt_shutdown(void)
+static int ep93xx_wdt_stop(struct watchdog_device *wdd)
 {
 	del_timer_sync(&timer);
 	wdt_disable();
+	return 0;
 }
 
-static void wdt_keepalive(void)
+static int ep93xx_wdt_keepalive(struct watchdog_device *wdd)
 {
 	/* user land ping */
-	next_heartbeat = jiffies + (timeout * HZ);
-}
-
-static int ep93xx_wdt_open(struct inode *inode, struct file *file)
-{
-	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
-		return -EBUSY;
-
-	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-
-	wdt_startup();
-
-	return nonseekable_open(inode, file);
-}
-
-static ssize_t
-ep93xx_wdt_write(struct file *file, const char __user *data, size_t len,
-		 loff_t *ppos)
-{
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-
-			for (i = 0; i != len; i++) {
-				char c;
-
-				if (get_user(c, data + i))
-					return -EFAULT;
-
-				if (c == 'V')
-					set_bit(WDT_OK_TO_CLOSE, &wdt_status);
-				else
-					clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-			}
-		}
-		wdt_keepalive();
-	}
-
-	return len;
+	next_heartbeat = jiffies + (wdd->timeout * HZ);
+	return 0;
 }
 
-static const struct watchdog_info ident = {
-	.options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE,
-	.identity = "EP93xx Watchdog",
+static const struct watchdog_info ep93xx_wdt_ident = {
+	.options	= WDIOF_CARDRESET | WDIOF_MAGICCLOSE,
+	.identity	= "EP93xx Watchdog",
 };
 
-static long ep93xx_wdt_ioctl(struct file *file,
-					unsigned int cmd, unsigned long arg)
-{
-	int ret = -ENOTTY;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		ret = copy_to_user((struct watchdog_info __user *)arg, &ident,
-				sizeof(ident)) ? -EFAULT : 0;
-		break;
-
-	case WDIOC_GETSTATUS:
-		ret = put_user(0, (int __user *)arg);
-		break;
-
-	case WDIOC_GETBOOTSTATUS:
-		ret = put_user(boot_status, (int __user *)arg);
-		break;
-
-	case WDIOC_KEEPALIVE:
-		wdt_keepalive();
-		ret = 0;
-		break;
-
-	case WDIOC_GETTIMEOUT:
-		/* actually, it is 0.250 seconds.... */
-		ret = put_user(1, (int __user *)arg);
-		break;
-	}
-	return ret;
-}
-
-static int ep93xx_wdt_release(struct inode *inode, struct file *file)
-{
-	if (test_bit(WDT_OK_TO_CLOSE, &wdt_status))
-		wdt_shutdown();
-	else
-		printk(KERN_CRIT PFX
-			"Device closed unexpectedly - timer will not stop\n");
-
-	clear_bit(WDT_IN_USE, &wdt_status);
-	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-
-	return 0;
-}
-
-static const struct file_operations ep93xx_wdt_fops = {
+static const struct watchdog_ops ep93xx_wdt_ops = {
 	.owner		= THIS_MODULE,
-	.write		= ep93xx_wdt_write,
-	.unlocked_ioctl	= ep93xx_wdt_ioctl,
-	.open		= ep93xx_wdt_open,
-	.release	= ep93xx_wdt_release,
-	.llseek		= no_llseek,
+	.start		= ep93xx_wdt_start,
+	.stop		= ep93xx_wdt_stop,
+	.ping		= ep93xx_wdt_keepalive,
 };
 
-static struct miscdevice ep93xx_wdt_miscdev = {
-	.minor		= WATCHDOG_MINOR,
-	.name		= "watchdog",
-	.fops		= &ep93xx_wdt_fops,
+static struct watchdog_device ep93xx_wdd = {
+	.info		= &ep93xx_wdt_ident,
+	.ops		= &ep93xx_wdt_ops,
+	.min_timeout	= 1,
+	.max_timeout	= 3600,
 };
 
 static void ep93xx_timer_ping(unsigned long data)

@@ -210,43 +126,31 @@ static int __init ep93xx_wdt_init(void)
 {
 	int err;
 
-	err = misc_register(&ep93xx_wdt_miscdev);
+	ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG);
+	ep93xx_wdd.timeout = timeout;
+
+	err = watchdog_register_device(&ep93xx_wdd);
+	if (err)
+		return err;
 
-	boot_status = __raw_readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0;
+	setup_timer(&timer, ep93xx_timer_ping, 1);
 
 	printk(KERN_INFO PFX "EP93XX watchdog, driver version "
 		WDT_VERSION "%s\n",
-		(__raw_readl(EP93XX_WDT_WATCHDOG) & 0x08)
+		(ep93xx_wdd.bootstatus & 0x08)
 		? " (nCS1 disable detected)" : "");
 
-	if (timeout < 1 || timeout > 3600) {
-		timeout = WDT_TIMEOUT;
-		printk(KERN_INFO PFX
-			"timeout value must be 1<=x<=3600, using %d\n",
-			timeout);
-	}
-
-	setup_timer(&timer, ep93xx_timer_ping, 1);
-	return err;
+	return 0;
 }
+module_init(ep93xx_wdt_init);
 
 static void __exit ep93xx_wdt_exit(void)
 {
-	wdt_shutdown();
-	misc_deregister(&ep93xx_wdt_miscdev);
+	wdt_shutdown(&ep93xx_wdd);
+	watchdog_unregister_device(&ep93xx_wdd);
 }
-
-module_init(ep93xx_wdt_init);
 module_exit(ep93xx_wdt_exit);
 
-module_param(nowayout, int, 0);
-MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started");
-
-module_param(timeout, int, 0);
-MODULE_PARM_DESC(timeout,
-	"Watchdog timeout in seconds. (1<=timeout<=3600, default="
-				__MODULE_STRING(WDT_TIMEOUT) ")");
-
 MODULE_AUTHOR("Ray Lehtiniemi <[email protected]>,"
 		"Alessandro Zummo <[email protected]>");
 MODULE_DESCRIPTION("EP93xx Watchdog");

Mika Westerberg

2011-08-02 09:16 UTC #1 lore raw

Quoted 2 lines · H Hartley Sweeten

Signed-off-by H Hartley Sweeten <[email protected]>
Cc Wim Van Sebroeck <[email protected]> (maintainer:WATCHDOG DEVICE D...)

Nice. I'll try to test the patch later today.

I have few minor comments, see below.

drivers/watchdog/ep93xx_wdt.c line 27

Quoted 13 lines

---

drivers/watchdog/ep93xx_wdt.c +0 -1

index 726b7df..bb97351 100644
@@ -24,11 +24,9 @@
  */
 
 #include <linux/module.h>
-#include <linux/fs.h>
 #include <linux/miscdevice.h>

Is the above header still needed?

Quoted 30 lines

drivers/watchdog/ep93xx_wdt.c +7 -6

 #include <linux/watchdog.h>
 #include <linux/timer.h>
-#include <linux/uaccess.h>
 #include <linux/io.h>
 #include <mach/hardware.h>

@@ -39,15 +37,17 @@
 #define WDT_TIMEOUT 30
 
 static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started");
+
 static int timeout = WDT_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout,
+	"Watchdog timeout in seconds. (1<=timeout<=3600, default="
+				__MODULE_STRING(WDT_TIMEOUT) ")");
 
 static struct timer_list timer;
 static unsigned long next_heartbeat;
-static unsigned long wdt_status;
-static unsigned long boot_status;
-
-#define WDT_IN_USE		0
-#define WDT_OK_TO_CLOSE		1
 
 #define EP93XX_WDT_REG(x)	(EP93XX_WATCHDOG_BASE + (x))
 #define EP93XX_WDT_WATCHDOG	EP93XX_WDT_REG(0x00)

drivers/watchdog/ep93xx_wdt.c static inline void wdt_ping(void) · line 74

Quoted 5 lines

drivers/watchdog/ep93xx_wdt.c +1 -1

@@ -71,130 +71,46 @@ static inline void wdt_ping(void)
 	__raw_writew(0x5555, EP93XX_WDT_WATCHDOG);
 }
 
-static void wdt_startup(void)
+static int void ep93xx_wdt_start(struct watchdog_device *wdd)

              ^^^^

extra 'void' above

Quoted 146 lines

drivers/watchdog/ep93xx_wdt.c +19 ~~-103~~

 {
-	next_heartbeat = jiffies + (timeout * HZ);
+	next_heartbeat = jiffies + (wdd->timeout * HZ);
 
 	wdt_enable();
 	mod_timer(&timer, jiffies + WDT_INTERVAL);
+	return 0;
 }
 
-static void wdt_shutdown(void)
+static int ep93xx_wdt_stop(struct watchdog_device *wdd)
 {
 	del_timer_sync(&timer);
 	wdt_disable();
+	return 0;
 }
 
-static void wdt_keepalive(void)
+static int ep93xx_wdt_keepalive(struct watchdog_device *wdd)
 {
 	/* user land ping */
-	next_heartbeat = jiffies + (timeout * HZ);
-}
-
-static int ep93xx_wdt_open(struct inode *inode, struct file *file)
-{
-	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
-		return -EBUSY;
-
-	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-
-	wdt_startup();
-
-	return nonseekable_open(inode, file);
-}
-
-static ssize_t
-ep93xx_wdt_write(struct file *file, const char __user *data, size_t len,
-		 loff_t *ppos)
-{
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-
-			for (i = 0; i != len; i++) {
-				char c;
-
-				if (get_user(c, data + i))
-					return -EFAULT;
-
-				if (c == 'V')
-					set_bit(WDT_OK_TO_CLOSE, &wdt_status);
-				else
-					clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-			}
-		}
-		wdt_keepalive();
-	}
-
-	return len;
+	next_heartbeat = jiffies + (wdd->timeout * HZ);
+	return 0;
 }
 
-static const struct watchdog_info ident = {
-	.options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE,
-	.identity = "EP93xx Watchdog",
+static const struct watchdog_info ep93xx_wdt_ident = {
+	.options	= WDIOF_CARDRESET | WDIOF_MAGICCLOSE,
+	.identity	= "EP93xx Watchdog",
 };
 
-static long ep93xx_wdt_ioctl(struct file *file,
-					unsigned int cmd, unsigned long arg)
-{
-	int ret = -ENOTTY;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		ret = copy_to_user((struct watchdog_info __user *)arg, &ident,
-				sizeof(ident)) ? -EFAULT : 0;
-		break;
-
-	case WDIOC_GETSTATUS:
-		ret = put_user(0, (int __user *)arg);
-		break;
-
-	case WDIOC_GETBOOTSTATUS:
-		ret = put_user(boot_status, (int __user *)arg);
-		break;
-
-	case WDIOC_KEEPALIVE:
-		wdt_keepalive();
-		ret = 0;
-		break;
-
-	case WDIOC_GETTIMEOUT:
-		/* actually, it is 0.250 seconds.... */
-		ret = put_user(1, (int __user *)arg);
-		break;
-	}
-	return ret;
-}
-
-static int ep93xx_wdt_release(struct inode *inode, struct file *file)
-{
-	if (test_bit(WDT_OK_TO_CLOSE, &wdt_status))
-		wdt_shutdown();
-	else
-		printk(KERN_CRIT PFX
-			"Device closed unexpectedly - timer will not stop\n");
-
-	clear_bit(WDT_IN_USE, &wdt_status);
-	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-
-	return 0;
-}
-
-static const struct file_operations ep93xx_wdt_fops = {
+static const struct watchdog_ops ep93xx_wdt_ops = {
 	.owner		= THIS_MODULE,
-	.write		= ep93xx_wdt_write,
-	.unlocked_ioctl	= ep93xx_wdt_ioctl,
-	.open		= ep93xx_wdt_open,
-	.release	= ep93xx_wdt_release,
-	.llseek		= no_llseek,
+	.start		= ep93xx_wdt_start,
+	.stop		= ep93xx_wdt_stop,
+	.ping		= ep93xx_wdt_keepalive,
 };
 
-static struct miscdevice ep93xx_wdt_miscdev = {
-	.minor		= WATCHDOG_MINOR,
-	.name		= "watchdog",
-	.fops		= &ep93xx_wdt_fops,
+static struct watchdog_device ep93xx_wdd = {
+	.info		= &ep93xx_wdt_ident,
+	.ops		= &ep93xx_wdt_ops,
+	.min_timeout	= 1,
+	.max_timeout	= 3600,
 };
 
 static void ep93xx_timer_ping(unsigned long data)

drivers/watchdog/ep93xx_wdt.c static int __init ep93xx_wdt_init(void) · line 130

Quoted 6 lines

drivers/watchdog/ep93xx_wdt.c +2 -1

@@ -210,43 +126,31 @@ static int __init ep93xx_wdt_init(void)
 {
 	int err;
 
-	err = misc_register(&ep93xx_wdt_miscdev);
+	ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG);
+	ep93xx_wdd.timeout = timeout;

Should you check that the given timeout is in valid range here, like it is done in the original driver?

Quoted 22 lines

drivers/watchdog/ep93xx_wdt.c +6 ~~-10~~

+
+	err = watchdog_register_device(&ep93xx_wdd);
+	if (err)
+		return err;
 
-	boot_status = __raw_readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0;
+	setup_timer(&timer, ep93xx_timer_ping, 1);
 
 	printk(KERN_INFO PFX "EP93XX watchdog, driver version "
 		WDT_VERSION "%s\n",
-		(__raw_readl(EP93XX_WDT_WATCHDOG) & 0x08)
+		(ep93xx_wdd.bootstatus & 0x08)
 		? " (nCS1 disable detected)" : "");
 
-	if (timeout < 1 || timeout > 3600) {
-		timeout = WDT_TIMEOUT;
-		printk(KERN_INFO PFX
-			"timeout value must be 1<=x<=3600, using %d\n",
-			timeout);
-	}
-
-	setup_timer(&timer, ep93xx_timer_ping, 1);

drivers/watchdog/ep93xx_wdt.c static int __init ep93xx_wdt_init(void) · line 149

Quoted 10 lines

drivers/watchdog/ep93xx_wdt.c +3 -3

-	return err;
+	return 0;
 }
+module_init(ep93xx_wdt_init);
 
 static void __exit ep93xx_wdt_exit(void)
 {
-	wdt_shutdown();
-	misc_deregister(&ep93xx_wdt_miscdev);
+	wdt_shutdown(&ep93xx_wdd);

ep93xx_wdt_stop(&ep93xx_wdd);

Quoted 22 lines

drivers/watchdog/ep93xx_wdt.c +1 ~~-11~~
+	watchdog_unregister_device(&ep93xx_wdd);
 }
-
-module_init(ep93xx_wdt_init);
 module_exit(ep93xx_wdt_exit);
 
-module_param(nowayout, int, 0);
-MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started");
-
-module_param(timeout, int, 0);
-MODULE_PARM_DESC(timeout,
-	"Watchdog timeout in seconds. (1<=timeout<=3600, default="
-				__MODULE_STRING(WDT_TIMEOUT) ")");
-
 MODULE_AUTHOR("Ray Lehtiniemi <[email protected]>,"
 		"Alessandro Zummo <[email protected]>");
 MODULE_DESCRIPTION("EP93xx Watchdog");
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

H Hartley Sweeten

author

2011-08-02 16:55 UTC #2 lore raw

Quoted 18 lines · Mika Westerberg

Quoted 2 lines · H Hartley Sweeten
Signed-off-by H Hartley Sweeten <[email protected]>
Cc Wim Van Sebroeck <[email protected]> (maintainer:WATCHDOG DEVICE D...)
Nice. I'll try to test the patch later today.
I have few minor comments, see below.
Quoted 10 lines
---
drivers/watchdog/ep93xx_wdt.c +0 -0
index 726b7df..bb97351 100644
@@ -24,11 +24,9 @@
  */
 

Quoted 5 lines

drivers/watchdog/ep93xx_wdt.c line 27
Quoted 3 lines
drivers/watchdog/ep93xx_wdt.c +0 -1
 #include <linux/module.h>
-#include <linux/fs.h>
 #include <linux/miscdevice.h>
Is the above header still needed?

I think so due to the MODULE_ALIAS_MISCDEV() at the end of the file. But, I'm not sure if that is really needed... Wim?

Quoted 33 lines

drivers/watchdog/ep93xx_wdt.c +7 -6

 #include <linux/watchdog.h>
 #include <linux/timer.h>
-#include <linux/uaccess.h>
 #include <linux/io.h>
 #include <mach/hardware.h>

@@ -39,15 +37,17 @@
 #define WDT_TIMEOUT 30
 
 static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started");
+
 static int timeout = WDT_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout,
+	"Watchdog timeout in seconds. (1<=timeout<=3600, default="
+				__MODULE_STRING(WDT_TIMEOUT) ")");
 
 static struct timer_list timer;
 static unsigned long next_heartbeat;
-static unsigned long wdt_status;
-static unsigned long boot_status;
-
-#define WDT_IN_USE		0
-#define WDT_OK_TO_CLOSE		1
 
 #define EP93XX_WDT_REG(x)	(EP93XX_WATCHDOG_BASE + (x))
 #define EP93XX_WDT_WATCHDOG	EP93XX_WDT_REG(0x00)

@@ -71,130 +71,46 @@ static inline void wdt_ping(void)
 	__raw_writew(0x5555, EP93XX_WDT_WATCHDOG);
 }

Quoted 4 lines

drivers/watchdog/ep93xx_wdt.c static inline void wdt_ping(void) · line 74
Quoted 2 lines
drivers/watchdog/ep93xx_wdt.c +1 -1
-static void wdt_startup(void)
+static int void ep93xx_wdt_start(struct watchdog_device *wdd)
             ^^^^
extra 'void' above

Argh... I need more sleep... I guess that's what happens when your first child is born...

Fixed.

Quoted 149 lines

drivers/watchdog/ep93xx_wdt.c +19 ~~-103~~

 {
-	next_heartbeat = jiffies + (timeout * HZ);
+	next_heartbeat = jiffies + (wdd->timeout * HZ);
 
 	wdt_enable();
 	mod_timer(&timer, jiffies + WDT_INTERVAL);
+	return 0;
 }
 
-static void wdt_shutdown(void)
+static int ep93xx_wdt_stop(struct watchdog_device *wdd)
 {
 	del_timer_sync(&timer);
 	wdt_disable();
+	return 0;
 }
 
-static void wdt_keepalive(void)
+static int ep93xx_wdt_keepalive(struct watchdog_device *wdd)
 {
 	/* user land ping */
-	next_heartbeat = jiffies + (timeout * HZ);
-}
-
-static int ep93xx_wdt_open(struct inode *inode, struct file *file)
-{
-	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
-		return -EBUSY;
-
-	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-
-	wdt_startup();
-
-	return nonseekable_open(inode, file);
-}
-
-static ssize_t
-ep93xx_wdt_write(struct file *file, const char __user *data, size_t len,
-		 loff_t *ppos)
-{
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-
-			for (i = 0; i != len; i++) {
-				char c;
-
-				if (get_user(c, data + i))
-					return -EFAULT;
-
-				if (c == 'V')
-					set_bit(WDT_OK_TO_CLOSE, &wdt_status);
-				else
-					clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-			}
-		}
-		wdt_keepalive();
-	}
-
-	return len;
+	next_heartbeat = jiffies + (wdd->timeout * HZ);
+	return 0;
 }
 
-static const struct watchdog_info ident = {
-	.options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE,
-	.identity = "EP93xx Watchdog",
+static const struct watchdog_info ep93xx_wdt_ident = {
+	.options	= WDIOF_CARDRESET | WDIOF_MAGICCLOSE,
+	.identity	= "EP93xx Watchdog",
 };
 
-static long ep93xx_wdt_ioctl(struct file *file,
-					unsigned int cmd, unsigned long arg)
-{
-	int ret = -ENOTTY;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		ret = copy_to_user((struct watchdog_info __user *)arg, &ident,
-				sizeof(ident)) ? -EFAULT : 0;
-		break;
-
-	case WDIOC_GETSTATUS:
-		ret = put_user(0, (int __user *)arg);
-		break;
-
-	case WDIOC_GETBOOTSTATUS:
-		ret = put_user(boot_status, (int __user *)arg);
-		break;
-
-	case WDIOC_KEEPALIVE:
-		wdt_keepalive();
-		ret = 0;
-		break;
-
-	case WDIOC_GETTIMEOUT:
-		/* actually, it is 0.250 seconds.... */
-		ret = put_user(1, (int __user *)arg);
-		break;
-	}
-	return ret;
-}
-
-static int ep93xx_wdt_release(struct inode *inode, struct file *file)
-{
-	if (test_bit(WDT_OK_TO_CLOSE, &wdt_status))
-		wdt_shutdown();
-	else
-		printk(KERN_CRIT PFX
-			"Device closed unexpectedly - timer will not stop\n");
-
-	clear_bit(WDT_IN_USE, &wdt_status);
-	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-
-	return 0;
-}
-
-static const struct file_operations ep93xx_wdt_fops = {
+static const struct watchdog_ops ep93xx_wdt_ops = {
 	.owner		= THIS_MODULE,
-	.write		= ep93xx_wdt_write,
-	.unlocked_ioctl	= ep93xx_wdt_ioctl,
-	.open		= ep93xx_wdt_open,
-	.release	= ep93xx_wdt_release,
-	.llseek		= no_llseek,
+	.start		= ep93xx_wdt_start,
+	.stop		= ep93xx_wdt_stop,
+	.ping		= ep93xx_wdt_keepalive,
 };
 
-static struct miscdevice ep93xx_wdt_miscdev = {
-	.minor		= WATCHDOG_MINOR,
-	.name		= "watchdog",
-	.fops		= &ep93xx_wdt_fops,
+static struct watchdog_device ep93xx_wdd = {
+	.info		= &ep93xx_wdt_ident,
+	.ops		= &ep93xx_wdt_ops,
+	.min_timeout	= 1,
+	.max_timeout	= 3600,
 };
 
 static void ep93xx_timer_ping(unsigned long data)

@@ -210,43 +126,31 @@ static int __init ep93xx_wdt_init(void)
 {
 	int err;

Quoted 6 lines

drivers/watchdog/ep93xx_wdt.c static int __init ep93xx_wdt_init(void) · line 130
Quoted 3 lines
drivers/watchdog/ep93xx_wdt.c +2 -1
-	err = misc_register(&ep93xx_wdt_miscdev);
+	ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG);
+	ep93xx_wdd.timeout = timeout;
Should you check that the given timeout is in valid range here, like it is done in the original driver?

I thought the core would handle that. If not maybe it should validate the timeout based on the min/max values. Wim?

Quoted 27 lines

drivers/watchdog/ep93xx_wdt.c +8 ~~-11~~

+
+	err = watchdog_register_device(&ep93xx_wdd);
+	if (err)
+		return err;
 
-	boot_status = __raw_readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0;
+	setup_timer(&timer, ep93xx_timer_ping, 1);
 
 	printk(KERN_INFO PFX "EP93XX watchdog, driver version "
 		WDT_VERSION "%s\n",
-		(__raw_readl(EP93XX_WDT_WATCHDOG) & 0x08)
+		(ep93xx_wdd.bootstatus & 0x08)
 		? " (nCS1 disable detected)" : "");
 
-	if (timeout < 1 || timeout > 3600) {
-		timeout = WDT_TIMEOUT;
-		printk(KERN_INFO PFX
-			"timeout value must be 1<=x<=3600, using %d\n",
-			timeout);
-	}
-
-	setup_timer(&timer, ep93xx_timer_ping, 1);
-	return err;
+	return 0;
 }
+module_init(ep93xx_wdt_init);

Quoted 7 lines

drivers/watchdog/ep93xx_wdt.c static int __init ep93xx_wdt_init(void) · line 149
Quoted 5 lines
drivers/watchdog/ep93xx_wdt.c +1 -2
 static void __exit ep93xx_wdt_exit(void)
 {
-	wdt_shutdown();
-	misc_deregister(&ep93xx_wdt_miscdev);
+	wdt_shutdown(&ep93xx_wdd);
ep93xx_wdt_stop(&ep93xx_wdd);

Oops.. You are correct. Fixed.

Quoted 6 lines

drivers/watchdog/ep93xx_wdt.c +1 -2

+	watchdog_unregister_device(&ep93xx_wdd);
 }
-
-module_init(ep93xx_wdt_init);
 module_exit(ep93xx_wdt_exit);

Quoted 11 lines

drivers/watchdog/ep93xx_wdt.c static int __init ep93xx_wdt_init(void) · line 156

Quoted 11 lines

drivers/watchdog/ep93xx_wdt.c +0 -8

-module_param(nowayout, int, 0);
-MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started");
-
-module_param(timeout, int, 0);
-MODULE_PARM_DESC(timeout,
-	"Watchdog timeout in seconds. (1<=timeout<=3600, default="
-				__MODULE_STRING(WDT_TIMEOUT) ")");
-
 MODULE_AUTHOR("Ray Lehtiniemi <[email protected]>,"
 		"Alessandro Zummo <[email protected]>");
 MODULE_DESCRIPTION("EP93xx Watchdog");

Thanks for the feedback. I'll post an updated patch later.

Regards,
Hartley--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mika Westerberg

2011-08-03 08:15 UTC #3 lore raw

Quoted 7 lines · H Hartley Sweeten

Quoted 7 lines

drivers/watchdog/ep93xx_wdt.c static inline void wdt_ping(void) · line 100

Quoted 7 lines

drivers/watchdog/ep93xx_wdt.c +3 -3

-static const struct watchdog_info ident = {
-	.options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE,
-	.identity = "EP93xx Watchdog",
+static const struct watchdog_info ep93xx_wdt_ident = {
+	.options	= WDIOF_CARDRESET | WDIOF_MAGICCLOSE,
+	.identity	= "EP93xx Watchdog",
 };

One more thing I noticed: you should add WDIOF_KEEPALIVEPING to the options field. Otherwise it doesn't support ioctl(..., WDIOC_KEEPALIVE) which was supported in the original driver.

H Hartley Sweeten

author

2011-08-03 16:23 UTC #4 lore raw

Quoted 12 lines · Mika Westerberg

Quoted 7 lines · H Hartley Sweeten
Quoted 7 lines
drivers/watchdog/ep93xx_wdt.c static inline void wdt_ping(void) · line 100
Quoted 7 lines
drivers/watchdog/ep93xx_wdt.c +3 -3
-static const struct watchdog_info ident = {
-	.options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE,
-	.identity = "EP93xx Watchdog",
+static const struct watchdog_info ep93xx_wdt_ident = {
+	.options	= WDIOF_CARDRESET | WDIOF_MAGICCLOSE,
+	.identity	= "EP93xx Watchdog",
 };
One more thing I noticed: you should add WDIOF_KEEPALIVEPING to the options field. Otherwise it doesn't support ioctl(..., WDIOC_KEEPALIVE) which was supported in the original driver.

I noticed that yesterday. I'll fix it and repost the patch shortly.

Thanks,
Hartley--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Wim Van Sebroeck

2011-08-03 09:56 UTC #5 lore raw

In reply to H Hartley Sweeten

Hi All,

Quoted 11 lines

Quoted 8 lines
drivers/watchdog/ep93xx_wdt.c line 27
Quoted 6 lines
drivers/watchdog/ep93xx_wdt.c +0 -1
@@ -24,11 +24,9 @@
  */
 
 #include <linux/module.h>
-#include <linux/fs.h>
 #include <linux/miscdevice.h>
Is the above header still needed?
I think so due to the MODULE_ALIAS_MISCDEV() at the end of the file. But, I'm not sure if that is really needed... Wim?

Yes it's till needed. Both MODULE_ALIAS_MISCDEV() and WATCHDOG_MINOR are defined in miscdevice.h .

Quoted 11 lines

Quoted 10 lines
drivers/watchdog/ep93xx_wdt.c static int __init ep93xx_wdt_init(void) · line 130
Quoted 7 lines
drivers/watchdog/ep93xx_wdt.c +2 -1
@@ -210,43 +126,31 @@ static int __init ep93xx_wdt_init(void)
 {
 	int err;
 
-	err = misc_register(&ep93xx_wdt_miscdev);
+	ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG);
+	ep93xx_wdd.timeout = timeout;
Should you check that the given timeout is in valid range here, like it is done in the original driver?

Quoted 2 lines

I thought the core would handle that. If not maybe it should validate the timeout based on the min/max values. Wim?

The core doesn't handle that.
What happens is: We set the parameter normally the same as the default value. The user can change this when loading the module by another value. We then normally need to check if this value makes any sense for the hardware. If not we reset it back to the default value.

The core is however not aware of the default value.
Several solutions are possible:
1) we add a timeout_default value into the struct (waste of space imho)
2) we set it to timeout_max if out of range
3) we set it to timeout_min if less then timeout_min or timeout_max if bigger then timeout_max
4) we can add an operation to handle this so that the driver can decide

For now I would leave the check in. In parallel we could indeed adopt a common behaviour and add it to the core.

Kind regards,
Wim.

PS: Congratulations with your first child!

Wim Van Sebroeck

2011-08-02 09:57 UTC #6 lore raw

In reply to H Hartley Sweeten

Hi H Hartley,

[...]

Quoted 4 lines

drivers/watchdog/ep93xx_wdt.c +0 -0

@@ -210,43 +126,31 @@ static int __init ep93xx_wdt_init(void)
 {
 	int err;

drivers/watchdog/ep93xx_wdt.c static int __init ep93xx_wdt_init(void) · line 136

Quoted 10 lines

drivers/watchdog/ep93xx_wdt.c +7 -2

-	err = misc_register(&ep93xx_wdt_miscdev);
+	ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG);
+	ep93xx_wdd.timeout = timeout;
+
+	err = watchdog_register_device(&ep93xx_wdd);
+	if (err)
+		return err;
 
-	boot_status = __raw_readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0;
+	setup_timer(&timer, ep93xx_timer_ping, 1);

Shouldn't the bootstatus setting be:
ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0; (or something similar with the WDIOF_OVERHEAT, ... flags).

Kind regards,
Wim.

H Hartley Sweeten

author

2011-08-02 17:02 UTC #7 lore raw

Quoted 1 line · Wim Van Sebroeck

Hi H Hartley,

Hello!

Quoted 13 lines

[...]

Quoted 12 lines

drivers/watchdog/ep93xx_wdt.c +6 -1

@@ -210,43 +126,31 @@ static int __init ep93xx_wdt_init(void)
 {
 	int err;
 
-	err = misc_register(&ep93xx_wdt_miscdev);
+	ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG);
+	ep93xx_wdd.timeout = timeout;
+
+	err = watchdog_register_device(&ep93xx_wdd);
+	if (err)
+		return err;

Quoted 6 lines

drivers/watchdog/ep93xx_wdt.c static int __init ep93xx_wdt_init(void) · line 136
Quoted 2 lines
drivers/watchdog/ep93xx_wdt.c +1 -1
-	boot_status = __raw_readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0;
+	setup_timer(&timer, ep93xx_timer_ping, 1);
Shouldn't the bootstatus setting be:
ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0; (or something similar with the WDIOF_OVERHEAT, ... flags).

The ep93xx watchdog status register doesn't line up nicely with the standard WDIOF_* flags. The register has these bits defined:

PLSDN 6 Pulse Disable Not
OVRID 5 Software Override of HWDIS
SWDIS 4 Software Watchdog Disable
HWDIS 3 Hardware Watchdog Disable
URST 2 User Reset Detected
3KRST 1 Three-Key Reset Detected
WD 0 Watchdog Reset Detected

The original bootstatus setting just checked for the WD bit. I would like to pass the full status register so that userspace can figure out what caused the reset. Is this an inappropriate abuse of WDIOC_GETBOOTSTATUS?

Wim Van Sebroeck

2011-08-03 10:07 UTC #8 lore raw

Hi H Hartley,

Quoted 23 lines

Quoted 13 lines
[...]
Quoted 12 lines
drivers/watchdog/ep93xx_wdt.c +6 -1
@@ -210,43 +126,31 @@ static int __init ep93xx_wdt_init(void)
 {
 	int err;
 
-	err = misc_register(&ep93xx_wdt_miscdev);
+	ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG);
+	ep93xx_wdd.timeout = timeout;
+
+	err = watchdog_register_device(&ep93xx_wdd);
+	if (err)
+		return err;
 
Quoted 6 lines
drivers/watchdog/ep93xx_wdt.c static int __init ep93xx_wdt_init(void) · line 136
Quoted 2 lines
drivers/watchdog/ep93xx_wdt.c +1 -1
-	boot_status = __raw_readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0;
+	setup_timer(&timer, ep93xx_timer_ping, 1);
Shouldn't the bootstatus setting be:
ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0; (or something similar with the WDIOF_OVERHEAT, ... flags).
The ep93xx watchdog status register doesn't line up nicely with the standard WDIOF_* flags. The register has these bits defined:

Quoted 7 lines

PLSDN 6 Pulse Disable Not
OVRID 5 Software Override of HWDIS
SWDIS 4 Software Watchdog Disable
HWDIS 3 Hardware Watchdog Disable
URST 2 User Reset Detected
3KRST 1 Three-Key Reset Detected
WD 0 Watchdog Reset Detected

Hmm, it would be nice to have this info also in the driver. Certainly at least a define for the WD bit.
Secondly: what it is doing now is allready incorrect: The WD bit is returned as bit 0 of the bootstatus value. This is actually: WDIOF_OVERHEAT (Reset due to CPU overheat) instead of
WDIOF_CARDRESET (0x0020 Card previously reset the CPU)...

Quoted 3 lines

The original bootstatus setting just checked for the WD bit. I would like to pass the full status register so that userspace can figure out what caused the reset. Is this an inappropriate abuse of WDIOC_GETBOOTSTATUS?

It's indeed an inappropriate abuse of WDIOC_GETBOOTSTATUS.
The WDIOC_GETBOOTSTATUS should return a result for all watchdog drivers.
The result is based on the WDIOF_* flags
This however does not mean that we can add flags. The 'User Reset detected' looks to me as something that can be usefull in embedded environments. We need to think about this and see what would be usefull in general.

Kind regards,
Wim.

H Hartley Sweeten

author

2011-08-03 16:35 UTC #9 lore raw

Quoted 33 lines · Wim Van Sebroeck

Hi H Hartley,
Quoted 23 lines
Quoted 13 lines
[...]
Quoted 12 lines
drivers/watchdog/ep93xx_wdt.c +6 -1
@@ -210,43 +126,31 @@ static int __init ep93xx_wdt_init(void)
 {
 	int err;
 
-	err = misc_register(&ep93xx_wdt_miscdev);
+	ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG);
+	ep93xx_wdd.timeout = timeout;
+
+	err = watchdog_register_device(&ep93xx_wdd);
+	if (err)
+		return err;
 
Quoted 6 lines
drivers/watchdog/ep93xx_wdt.c static int __init ep93xx_wdt_init(void) · line 136
Quoted 2 lines
drivers/watchdog/ep93xx_wdt.c +1 -1
-	boot_status = __raw_readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0;
+	setup_timer(&timer, ep93xx_timer_ping, 1);
Shouldn't the bootstatus setting be:
ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0; (or something similar with the WDIOF_OVERHEAT, ... flags).
The ep93xx watchdog status register doesn't line up nicely with the standard WDIOF_* flags. The register has these bits defined:
Quoted 7 lines
PLSDN 6 Pulse Disable Not
OVRID 5 Software Override of HWDIS
SWDIS 4 Software Watchdog Disable
HWDIS 3 Hardware Watchdog Disable
URST 2 User Reset Detected
3KRST 1 Three-Key Reset Detected
WD 0 Watchdog Reset Detected

Quoted 2 lines

Hmm, it would be nice to have this info also in the driver. Certainly at least a define for the WD bit.

I was doing to document these bits in a follow-up patch after the conversion. Instead I'll do it now in v2 of this patch. It does make the driver a bit clearer.

Quoted 4 lines

Secondly: what it is doing now is allready incorrect: The WD bit is returned as bit 0 of the bootstatus value.
This is actually: WDIOF_OVERHEAT (Reset due to CPU overheat) instead of WDIOF_CARDRESET (0x0020 Card previously reset the CPU)...

I agree, the current driver was already abusing the bootstatus use.

Quoted 10 lines

Quoted 3 lines
The original bootstatus setting just checked for the WD bit. I would like to pass the full status register so that userspace can figure out what caused the reset. Is this an inappropriate abuse of WDIOC_GETBOOTSTATUS?
It's indeed an inappropriate abuse of WDIOC_GETBOOTSTATUS.
The WDIOC_GETBOOTSTATUS should return a result for all watchdog drivers. The result is based on the WDIOF_* flags
This however does not mean that we can add flags. The 'User Reset detected' looks to me as something that can be usefull in embedded environments. We need to think about this and see what would be usefull in general.

I'll post a v2 of this patch shortly and try to address this and your other comments.

Thanks,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html