[RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[H Hartley Sweeten]

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Wim Van Sebroeck <wim@iguana.be> (maintainer:WATCHDOG DEVICE D...)

---

diff --git a/drivers/watchdog/ep93xx_wdt.c b/drivers/watchdog/ep93xx_wdt.c
index 726b7df..bb97351 100644
--- a/drivers/watchdog/ep93xx_wdt.c
+++ b/drivers/watchdog/ep93xx_wdt.c
@@ -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 <rayl@mail.com>,"
 		"Alessandro Zummo <a.zummo@towertech.it>");
 MODULE_DESCRIPTION("EP93xx Watchdog");

Re: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[Mika Westerberg]

On Mon, Aug 01, 2011 at 01:57:24PM -0700, H Hartley Sweeten wrote:
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: Wim Van Sebroeck <wim@iguana.be> (maintainer:WATCHDOG DEVICE D...)

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

I have few minor comments, see below.

> 
> ---
> 
> diff --git a/drivers/watchdog/ep93xx_wdt.c b/drivers/watchdog/ep93xx_wdt.c
> index 726b7df..bb97351 100644
> --- a/drivers/watchdog/ep93xx_wdt.c
> +++ b/drivers/watchdog/ep93xx_wdt.c
> @@ -24,11 +24,9 @@
>   */
>  
>  #include <linux/module.h>
> -#include <linux/fs.h>
>  #include <linux/miscdevice.h>

Is the above header still needed?

>  #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)
              ^^^^
extra 'void' above

>  {
> -	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;

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

> +
> +	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);

ep93xx_wdt_stop(&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 <rayl@mail.com>,"
>  		"Alessandro Zummo <a.zummo@towertech.it>");
>  MODULE_DESCRIPTION("EP93xx Watchdog");
> --
> 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/

Re: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[Wim Van Sebroeck]

Hi H Hartley,

[...]
> @@ -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);

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.

RE: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[H Hartley Sweeten]

On Tuesday, August 02, 2011 2:17 AM, Mika Westerberg wrote:
> On Mon, Aug 01, 2011 at 01:57:24PM -0700, H Hartley Sweeten wrote:
>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>> Cc: Wim Van Sebroeck <wim@iguana.be> (maintainer:WATCHDOG DEVICE D...)
>
> Nice. I'll try to test the patch later today.
>
> I have few minor comments, see below.
>
>> 
>> ---
>> 
>> diff --git a/drivers/watchdog/ep93xx_wdt.c b/drivers/watchdog/ep93xx_wdt.c
>> index 726b7df..bb97351 100644
>> --- a/drivers/watchdog/ep93xx_wdt.c
>> +++ b/drivers/watchdog/ep93xx_wdt.c
>> @@ -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?

>>  #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)
>              ^^^^
> extra 'void' above

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

Fixed.

>>  {
>> -	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;
>
> 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?

>> +
>> +	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);
>
> ep93xx_wdt_stop(&ep93xx_wdd);

Oops.. You are correct.  Fixed.

>> +	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 <rayl@mail.com>,"
>>  		"Alessandro Zummo <a.zummo@towertech.it>");
>>  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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[H Hartley Sweeten]

On Tuesday, August 02, 2011 2:17 AM, Mika Westerberg wrote:
> On Mon, Aug 01, 2011 at 01:57:24PM -0700, H Hartley Sweeten wrote:
>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>> Cc: Wim Van Sebroeck <wim@iguana.be> (maintainer:WATCHDOG DEVICE D...)
>
> Nice. I'll try to test the patch later today.
>
> I have few minor comments, see below.
>
>> 
>> ---
>> 
>> diff --git a/drivers/watchdog/ep93xx_wdt.c b/drivers/watchdog/ep93xx_wdt.c
>> index 726b7df..bb97351 100644
>> --- a/drivers/watchdog/ep93xx_wdt.c
>> +++ b/drivers/watchdog/ep93xx_wdt.c
>> @@ -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?

>>  #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)
>              ^^^^
> extra 'void' above

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

Fixed.

>>  {
>> -	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;
>
> 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?

>> +
>> +	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);
>
> ep93xx_wdt_stop(&ep93xx_wdd);

Oops.. You are correct.  Fixed.

>> +	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 <rayl@mail.com>,"
>>  		"Alessandro Zummo <a.zummo@towertech.it>");
>>  MODULE_DESCRIPTION("EP93xx Watchdog");

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

Regards,
Hartley

RE: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[H Hartley Sweeten]

On Tuesday, August 02, 2011 2:58 AM, Wim Van Sebroeck wrote:
> Hi H Hartley,

Hello!

> [...]
>> @@ -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);
>
> 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?

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

RE: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[H Hartley Sweeten]

On Tuesday, August 02, 2011 2:58 AM, Wim Van Sebroeck wrote:
> Hi H Hartley,

Hello!

> [...]
>> @@ -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);
>
> 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?

Regards,
Hartley

Re: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[Mika Westerberg]

On Tue, Aug 02, 2011 at 11:55:36AM -0500, H Hartley Sweeten wrote:
> >> -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.

Re: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[Wim Van Sebroeck]

Hi All,

> >> @@ -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 .

> >> @@ -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?
> 
> 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!

Re: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[Wim Van Sebroeck]

Hi H Hartley,

> > [...]
> >> @@ -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);
> >
> > 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

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)...

> 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.

RE: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[H Hartley Sweeten]

On Wednesday, August 03, 2011 1:16 AM, Mika Westerberg wrote:
> On Tue, Aug 02, 2011 at 11:55:36AM -0500, H Hartley Sweeten wrote:
>>>> -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[H Hartley Sweeten]

On Wednesday, August 03, 2011 1:16 AM, Mika Westerberg wrote:
> On Tue, Aug 02, 2011 at 11:55:36AM -0500, H Hartley Sweeten wrote:
>>>> -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

RE: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[H Hartley Sweeten]

On Wednesday, August 03, 2011 3:07 AM, Wim Van Sebroeck wrote:
> Hi H Hartley,
>
>>> [...]
>>>> @@ -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);
>>>
>>> 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
>
> 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.

> 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.

>> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[H Hartley Sweeten]

On Wednesday, August 03, 2011 3:07 AM, Wim Van Sebroeck wrote:
> Hi H Hartley,
>
>>> [...]
>>>> @@ -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);
>>>
>>> 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
>
> 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.

> 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.

>> 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