[PATCH v4 1/8] watchdog: add the function watchdog_is_open

[PATCH v4 1/8] watchdog: add the function watchdog_is_open

[Wenyou Yang]

Add the function watchdog_is_open to check whether or not
the /dev/watchdog? is opened

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
Cc: wim@iguana.be
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 include/linux/watchdog.h |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index e40cc2b..7ea4465 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -111,6 +111,14 @@ static inline bool watchdog_active(struct watchdog_device *wdd)
 	return test_bit(WDOG_ACTIVE, &wdd->status);
 }
 
+/* Use the following function to check whether or not
+ * the /dev/watchdog? is opened
+ */
+static inline bool watchdog_is_open(struct watchdog_device *wddev)
+{
+	return test_bit(WDOG_DEV_OPEN, &wddev->status);
+}
+
 /* Use the following function to set the nowayout feature */
 static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout)
 {
-- 
1.7.9.5

Re: [PATCH v4 1/8] watchdog: add the function watchdog_is_open

[Wim Van Sebroeck]

Hi Wenyou,

> Add the function watchdog_is_open to check whether or not
> the /dev/watchdog? is opened
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> Cc: wim@iguana.be
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  include/linux/watchdog.h |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index e40cc2b..7ea4465 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -111,6 +111,14 @@ static inline bool watchdog_active(struct watchdog_device *wdd)
>  	return test_bit(WDOG_ACTIVE, &wdd->status);
>  }
>  
> +/* Use the following function to check whether or not
> + * the /dev/watchdog? is opened
> + */
> +static inline bool watchdog_is_open(struct watchdog_device *wddev)
> +{
> +	return test_bit(WDOG_DEV_OPEN, &wddev->status);
> +}
> +
>  /* Use the following function to set the nowayout feature */
>  static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout)
>  {

NAK, this is not good. You should use watchdog_active instead.
Reason: your device could have been opened as for instance /dev/watchdog0.

Kind regards,
Wim.

[PATCH v4 2/8] watchdog/at91sam9_wdt: Remove at91wdt_private and add at91wdt_drvdata struct

[Wenyou Yang]

Remove the global variable at91wdt_private, add the struct at91wdt_drvdata
as a substitute, and set it as the driver data of the at91wdt_wdd.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
Cc: wim@iguana.be
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/at91sam9_wdt.c |   88 +++++++++++++++++++++------------------
 1 file changed, 47 insertions(+), 41 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 61129fc..66d3afb 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -38,11 +38,6 @@
 
 #define DRV_NAME "AT91SAM9 Watchdog"
 
-#define wdt_read(field) \
-	__raw_readl(at91wdt_private.base + field)
-#define wdt_write(field, val) \
-	__raw_writel((val), at91wdt_private.base + field)
-
 /* AT91SAM9 watchdog runs a 12bit counter @ 256Hz,
  * use this to convert a watchdog
  * value from/to milliseconds.
@@ -72,23 +67,33 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 
 static void at91_ping(unsigned long data);
 
-static struct {
-	void __iomem *base;
-	unsigned long next_heartbeat;	/* the next_heartbeat for the timer */
-	unsigned long open;
-	char expect_close;
-	struct timer_list timer;	/* The timer that pings the watchdog */
-} at91wdt_private;
+struct at91wdt_drvdata {
+	void __iomem	*phybase;
+	bool		is_enable;	/* indicate if the watchdog is eabled */
+	unsigned long	next_heartbeat;	/* the next_heartbeat for the timer */
+	struct timer_list	timer;	/* The timer that pings the watchdog */
+};
 
 /* ......................................................................... */
 
+static inline unsigned int wdt_read(struct at91wdt_drvdata *driver_data,
+					unsigned int field)
+{
+	return readl_relaxed(driver_data->phybase + field);
+}
+
+static inline void wdt_write(struct at91wdt_drvdata *driver_data,
+				unsigned int field, unsigned int val)
+{
+	writel_relaxed((val), driver_data->phybase + field);
+}
 
 /*
  * Reload the watchdog timer.  (ie, pat the watchdog)
  */
-static inline void at91_wdt_reset(void)
+static inline void at91_wdt_reset(struct at91wdt_drvdata *driver_data)
 {
-	wdt_write(AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
+	wdt_write(driver_data, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
 }
 
 /*
@@ -96,10 +101,12 @@ static inline void at91_wdt_reset(void)
  */
 static void at91_ping(unsigned long data)
 {
-	if (time_before(jiffies, at91wdt_private.next_heartbeat) ||
-			(!nowayout && !at91wdt_private.open)) {
-		at91_wdt_reset();
-		mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
+	struct watchdog_device *wddev = (struct watchdog_device *)data;
+	struct at91wdt_drvdata *driver_data = watchdog_get_drvdata(wddev);
+
+	if (time_before(jiffies, driver_data->next_heartbeat)) {
+		at91_wdt_reset(driver_data);
+		mod_timer(&driver_data->timer, jiffies + WDT_TIMEOUT);
 	} else
 		pr_crit("I will reset your machine !\n");
 }
@@ -109,11 +116,8 @@ static void at91_ping(unsigned long data)
  */
 static int at91_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(0, &at91wdt_private.open))
-		return -EBUSY;
-
-	at91wdt_private.next_heartbeat = jiffies + heartbeat * HZ;
-	mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
+	driver_data->next_heartbeat = jiffies + heartbeat * HZ;
+	mod_timer(&driver_data->timer, jiffies + WDT_TIMEOUT);
 
 	return nonseekable_open(inode, file);
 }
@@ -123,13 +127,8 @@ static int at91_wdt_open(struct inode *inode, struct file *file)
  */
 static int at91_wdt_close(struct inode *inode, struct file *file)
 {
-	clear_bit(0, &at91wdt_private.open);
+	del_timer(&driver_data->timer);
 
-	/* stop internal ping */
-	if (!at91wdt_private.expect_close)
-		del_timer(&at91wdt_private.timer);
-
-	at91wdt_private.expect_close = 0;
 	return 0;
 }
 
@@ -191,7 +190,7 @@ static long at91_wdt_ioctl(struct file *file,
 		return put_user(0, p);
 
 	case WDIOC_KEEPALIVE:
-		at91wdt_private.next_heartbeat = jiffies + heartbeat * HZ;
+		driver_data->next_heartbeat = jiffies + heartbeat * HZ;
 		return 0;
 
 	case WDIOC_SETTIMEOUT:
@@ -199,7 +198,7 @@ static long at91_wdt_ioctl(struct file *file,
 			return -EFAULT;
 
 		heartbeat = new_value;
-		at91wdt_private.next_heartbeat = jiffies + heartbeat * HZ;
+		driver_data->next_heartbeat = jiffies + heartbeat * HZ;
 
 		return put_user(new_value, p);  /* return current value */
 
@@ -222,20 +221,16 @@ static ssize_t at91_wdt_write(struct file *file, const char *data, size_t len,
 	if (!nowayout) {
 		size_t i;
 
-		at91wdt_private.expect_close = 0;
 
 		for (i = 0; i < len; i++) {
 			char c;
 			if (get_user(c, data + i))
 				return -EFAULT;
-			if (c == 'V') {
-				at91wdt_private.expect_close = 42;
-				break;
 			}
 		}
 	}
 
-	at91wdt_private.next_heartbeat = jiffies + heartbeat * HZ;
+	driver_data->next_heartbeat = jiffies + heartbeat * HZ;
 
 	return len;
 }
@@ -265,9 +260,19 @@ static struct watchdog_device at91wdt_wdd __initdata = {
 
 static int __init at91wdt_probe(struct platform_device *pdev)
 {
+	struct at91wdt_drvdata *driver_data;
 	struct resource	*r;
 	int res;
 
+	driver_data = devm_kzalloc(&pdev->dev,
+				sizeof(*driver_data), GFP_KERNEL);
+	if (!driver_data) {
+		dev_err(&pdev->dev, "Unable to alloacate watchdog device\n");
+		return -ENOMEM;
+	}
+
+	watchdog_set_drvdata(&at91wdt_wdd, driver_data);
+
 	if (at91wdt_miscdev.parent)
 		return -EBUSY;
 	at91wdt_miscdev.parent = &pdev->dev;
@@ -275,8 +280,8 @@ static int __init at91wdt_probe(struct platform_device *pdev)
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!r)
 		return -ENODEV;
-	at91wdt_private.base = ioremap(r->start, resource_size(r));
-	if (!at91wdt_private.base) {
+	driver_data->phybase = ioremap(r->start, resource_size(r));
+	if (!driver_data->phybase) {
 		dev_err(&pdev->dev, "failed to map registers, aborting.\n");
 		return -ENOMEM;
 	}
@@ -292,9 +297,10 @@ static int __init at91wdt_probe(struct platform_device *pdev)
 	if (res)
 		return res;
 
-	at91wdt_private.next_heartbeat = jiffies + at91wdt_wdd.timeout * HZ;
-	setup_timer(&at91wdt_private.timer, at91_ping, 0);
-	mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
+	driver_data->next_heartbeat = jiffies + at91wdt_wdd.timeout * HZ;
+	setup_timer(&driver_data->timer, at91_ping,
+					(unsigned long)&at91wdt_wdd);
+	mod_timer(&driver_data->timer, jiffies + WDT_TIMEOUT);
 
 	pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
 		at91wdt_wdd.timeout, nowayout);
-- 
1.7.9.5

Re: [PATCH v4 2/8] watchdog/at91sam9_wdt: Remove at91wdt_private and add at91wdt_drvdata struct

[Wim Van Sebroeck]

Hi Wenyou,

> Remove the global variable at91wdt_private, add the struct at91wdt_drvdata
> as a substitute, and set it as the driver data of the at91wdt_wdd.

I rather have this after the conversion of the watchdog to the new framework.

Kind regards,
Wim.

RE: [PATCH v4 2/8] watchdog/at91sam9_wdt: Remove at91wdt_private and add at91wdt_drvdata struct

[Yang, Wenyou]

Hi Wim

> I rather have this after the conversion of the watchdog to the new framework.
> 
Thanks a lot.

OK , I will do it later.

Best Regards,
Wenyou Yang

[PATCH v4 3/8] watchdog/at91sam9_wdt: Convert to use the watchdog framework

[Wenyou Yang]

According to Documentation/watchdog/convert_drivers_to_kernel_api.txt,
remove the file_operations struct, miscdevice, and obsolete includes

Since the at91sam watchdog inherent characteristics, add the watchdog
operations: at91wdt_start, at91wdt_stop and at91wdt_ping.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
Cc: wim@iguana.be
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/at91sam9_wdt.c |  199 ++++++++++++++-------------------------
 1 file changed, 72 insertions(+), 127 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 66d3afb..ce7930b 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -13,16 +13,17 @@
  * The Watchdog Timer Mode Register can be only written to once. If the
  * timeout need to be set from Linux, be sure that the bootstrap or the
  * bootloader doesn't write to this register.
+ * The Watchdog Timer default is running with maximum counter value
+ * (WDV=0xfff) at reset, i.e., at power-up. It MUST be either disabled
+ * or be reprogrammed within the maxinum margin(16s).
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/errno.h>
-#include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
-#include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/platform_device.h>
@@ -31,7 +32,6 @@
 #include <linux/jiffies.h>
 #include <linux/timer.h>
 #include <linux/bitops.h>
-#include <linux/uaccess.h>
 #include <linux/of.h>
 
 #include "at91sam9_wdt.h"
@@ -65,8 +65,6 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-static void at91_ping(unsigned long data);
-
 struct at91wdt_drvdata {
 	void __iomem	*phybase;
 	bool		is_enable;	/* indicate if the watchdog is eabled */
@@ -99,7 +97,7 @@ static inline void at91_wdt_reset(struct at91wdt_drvdata *driver_data)
 /*
  * Timer tick
  */
-static void at91_ping(unsigned long data)
+static void at91wdt_timer_tick(unsigned long data)
 {
 	struct watchdog_device *wddev = (struct watchdog_device *)data;
 	struct at91wdt_drvdata *driver_data = watchdog_get_drvdata(wddev);
@@ -107,45 +105,31 @@ static void at91_ping(unsigned long data)
 	if (time_before(jiffies, driver_data->next_heartbeat)) {
 		at91_wdt_reset(driver_data);
 		mod_timer(&driver_data->timer, jiffies + WDT_TIMEOUT);
-	} else
-		pr_crit("I will reset your machine !\n");
-}
-
-/*
- * Watchdog device is opened, and watchdog starts running.
- */
-static int at91_wdt_open(struct inode *inode, struct file *file)
-{
-	driver_data->next_heartbeat = jiffies + heartbeat * HZ;
-	mod_timer(&driver_data->timer, jiffies + WDT_TIMEOUT);
 
-	return nonseekable_open(inode, file);
-}
-
-/*
- * Close the watchdog device.
- */
-static int at91_wdt_close(struct inode *inode, struct file *file)
-{
-	del_timer(&driver_data->timer);
-
-	return 0;
+		if (!watchdog_is_open(wddev))
+			driver_data->next_heartbeat = jiffies
+						+ wddev->timeout * HZ;
+	} else {
+		pr_crit("I will reset your machine !\n");
+	}
 }
 
-/*
- * Set the watchdog time interval in 1/256Hz (write-once)
- * Counter is 12 bit.
- */
-static int at91_wdt_settimeout(unsigned int timeout)
+static int at91wdt_enable(struct watchdog_device *wddev, unsigned int timeout)
 {
+	struct at91wdt_drvdata *driver_data = watchdog_get_drvdata(wddev);
 	unsigned int reg;
-	unsigned int mr;
 
-	/* Check if disabled */
-	mr = wdt_read(AT91_WDT_MR);
-	if (mr & AT91_WDT_WDDIS) {
-		pr_err("sorry, watchdog is disabled\n");
-		return -EIO;
+	/*
+	 * Check if the watchdog is disabled,
+	 * if disabled, the reason is the bootstrap or the bootloader has
+	 * written the Watchdog Timer Mode Register to disable the
+	 * watchdog timer
+	 */
+	reg = wdt_read(driver_data, AT91_WDT_MR);
+	if (reg & AT91_WDT_WDDIS) {
+		driver_data->is_enable = false;
+		pr_info("sorry, watchdog is disabled\n");
+		return -1;
 	}
 
 	/*
@@ -159,7 +143,9 @@ static int at91_wdt_settimeout(unsigned int timeout)
 		| AT91_WDT_WDDBGHLT	/* disabled in debug mode */
 		| AT91_WDT_WDD		/* restart at any time */
 		| (timeout & AT91_WDT_WDV);  /* timer value */
-	wdt_write(AT91_WDT_MR, reg);
+	wdt_write(driver_data, AT91_WDT_MR, reg);
+
+	driver_data->is_enable = true;
 
 	return 0;
 }
@@ -170,99 +156,63 @@ static const struct watchdog_info at91_wdt_info = {
 						WDIOF_MAGICCLOSE,
 };
 
-/*
- * Handle commands from user-space.
- */
-static long at91_wdt_ioctl(struct file *file,
-		unsigned int cmd, unsigned long arg)
+static int at91wdt_start(struct watchdog_device *wddev)
 {
-	void __user *argp = (void __user *)arg;
-	int __user *p = argp;
-	int new_value;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(argp, &at91_wdt_info,
-				    sizeof(at91_wdt_info)) ? -EFAULT : 0;
-
-	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(0, p);
+	struct at91wdt_drvdata *driver_data = watchdog_get_drvdata(wddev);
 
-	case WDIOC_KEEPALIVE:
-		driver_data->next_heartbeat = jiffies + heartbeat * HZ;
+	if (driver_data->is_enable) {
+		driver_data->next_heartbeat = jiffies + wddev->timeout * HZ;
+		mod_timer(&driver_data->timer, jiffies + WDT_TIMEOUT);
 		return 0;
-
-	case WDIOC_SETTIMEOUT:
-		if (get_user(new_value, p))
-			return -EFAULT;
-
-		heartbeat = new_value;
-		driver_data->next_heartbeat = jiffies + heartbeat * HZ;
-
-		return put_user(new_value, p);  /* return current value */
-
-	case WDIOC_GETTIMEOUT:
-		return put_user(heartbeat, p);
+	} else {
+		return -EIO;
 	}
-	return -ENOTTY;
 }
 
-/*
- * Pat the watchdog whenever device is written to.
- */
-static ssize_t at91_wdt_write(struct file *file, const char *data, size_t len,
-								loff_t *ppos)
+static int at91wdt_stop(struct watchdog_device *wddev)
 {
-	if (!len)
-		return 0;
+	struct at91wdt_drvdata *driver_data = watchdog_get_drvdata(wddev);
 
-	/* Scan for magic character */
-	if (!nowayout) {
-		size_t i;
+	if (driver_data->is_enable)
+		return -EIO;
+	else
+		return 0;
+}
 
+static int at91wdt_ping(struct watchdog_device *wddev)
+{
+	struct at91wdt_drvdata *driver_data = watchdog_get_drvdata(wddev);
 
-		for (i = 0; i < len; i++) {
-			char c;
-			if (get_user(c, data + i))
-				return -EFAULT;
-			}
-		}
+	if (driver_data->is_enable) {
+		driver_data->next_heartbeat = jiffies + wddev->timeout * HZ;
+		 mod_timer(&driver_data->timer, jiffies + WDT_TIMEOUT);
+		return 0;
+	} else {
+		return -EIO;
 	}
-
-	driver_data->next_heartbeat = jiffies + heartbeat * HZ;
-
-	return len;
 }
-
 /* ......................................................................... */
 
-static const struct file_operations at91wdt_fops = {
-	.owner			= THIS_MODULE,
-	.llseek			= no_llseek,
-	.unlocked_ioctl	= at91_wdt_ioctl,
-	.open			= at91_wdt_open,
-	.release		= at91_wdt_close,
-	.write			= at91_wdt_write,
-};
-
-static struct miscdevice at91wdt_miscdev = {
-	.minor		= WATCHDOG_MINOR,
-	.name		= "watchdog",
-	.fops		= &at91wdt_fops,
+static struct watchdog_ops at91wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = at91wdt_start,
+	.stop = at91wdt_stop,
+	.ping = at91wdt_ping,
 };
 
 static struct watchdog_device at91wdt_wdd __initdata = {
 	.timeout = WDT_HEARTBEAT,
 	.min_timeout = MIN_HEARTBEAT,
 	.max_timeout = MAX_HEARTBEAT,
+	.info = &at91_wdt_info,
+	.ops = &at91wdt_ops,
 };
 
 static int __init at91wdt_probe(struct platform_device *pdev)
 {
 	struct at91wdt_drvdata *driver_data;
 	struct resource	*r;
-	int res;
+	int ret;
 
 	driver_data = devm_kzalloc(&pdev->dev,
 				sizeof(*driver_data), GFP_KERNEL);
@@ -273,32 +223,32 @@ static int __init at91wdt_probe(struct platform_device *pdev)
 
 	watchdog_set_drvdata(&at91wdt_wdd, driver_data);
 
-	if (at91wdt_miscdev.parent)
-		return -EBUSY;
-	at91wdt_miscdev.parent = &pdev->dev;
-
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!r)
 		return -ENODEV;
+
 	driver_data->phybase = ioremap(r->start, resource_size(r));
 	if (!driver_data->phybase) {
 		dev_err(&pdev->dev, "failed to map registers, aborting.\n");
 		return -ENOMEM;
 	}
 
-	watchdog_init_timeout(&at91wdt_wdd, heartbeat, pdev->dev.of_node);
+	ret = watchdog_register_device(&at91wdt_wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "cannot register watchdog (%d)\n", ret);
+		return ret;
+	}
 
-	/* Set watchdog */
-	res = at91_wdt_settimeout(ms_to_ticks(WDT_HW_TIMEOUT * 1000));
-	if (res)
-		return res;
+	watchdog_init_timeout(&at91wdt_wdd, heartbeat, pdev->dev.of_node);
 
-	res = misc_register(&at91wdt_miscdev);
-	if (res)
-		return res;
+	ret = at91wdt_enable(&at91wdt_wdd, ms_to_ticks(WDT_HW_TIMEOUT * 1000));
+	if (ret) {
+		pr_info("the watchdog has been disabled\n");
+		return 0;
+	}
 
 	driver_data->next_heartbeat = jiffies + at91wdt_wdd.timeout * HZ;
-	setup_timer(&driver_data->timer, at91_ping,
+	setup_timer(&driver_data->timer, at91wdt_timer_tick,
 					(unsigned long)&at91wdt_wdd);
 	mod_timer(&driver_data->timer, jiffies + WDT_TIMEOUT);
 
@@ -310,13 +260,9 @@ static int __init at91wdt_probe(struct platform_device *pdev)
 
 static int __exit at91wdt_remove(struct platform_device *pdev)
 {
-	int res;
-
-	res = misc_deregister(&at91wdt_miscdev);
-	if (!res)
-		at91wdt_miscdev.parent = NULL;
+	watchdog_unregister_device(&at91wdt_wdd);
 
-	return res;
+	return 0;
 }
 
 #if defined(CONFIG_OF)
@@ -353,4 +299,3 @@ module_exit(at91sam_wdt_exit);
 MODULE_AUTHOR("Renaud CERRATO <r.cerrato@til-technologies.fr>");
 MODULE_DESCRIPTION("Watchdog driver for Atmel AT91SAM9x processors");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
-- 
1.7.9.5

Re: [PATCH v4 3/8] watchdog/at91sam9_wdt: Convert to use the watchdog framework

[Wim Van Sebroeck]

Hi Wenyou,

> According to Documentation/watchdog/convert_drivers_to_kernel_api.txt,
> remove the file_operations struct, miscdevice, and obsolete includes
> 
> Since the at91sam watchdog inherent characteristics, add the watchdog
> operations: at91wdt_start, at91wdt_stop and at91wdt_ping.
> 

This code not only converts the watchdog to the new framework,
but it also adds the is_enable related code changes which should
be a seperate patch.

So I took your original patch and changed it to the below at91sam9_wdt
watchdog conversion patch. Note: this is also without Fabio's timeout-sec
patch, this one needs to come after the conversion.

Please test this patch and let me know if this works (the watchdog should
behave the same before as after this patch).

Kind regards,
Wim.
---
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 5e8a034..c36fcb0 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -125,6 +125,7 @@ config AT91RM9200_WATCHDOG
 config AT91SAM9X_WATCHDOG
 	tristate "AT91SAM9X / AT91CAP9 watchdog"
 	depends on ARCH_AT91 && !ARCH_AT91RM9200
+	select WATCHDOG_CORE
 	help
 	  Watchdog timer embedded into AT91SAM9X and AT91CAP9 chips. This will
 	  reboot your system when the timeout is reached.
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 6dad954..53fa325 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -18,11 +18,9 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/errno.h>
-#include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
-#include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/platform_device.h>
@@ -68,19 +66,17 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
+static struct watchdog_device at91_wdt_dev;
 static void at91_ping(unsigned long data);
 
 static struct {
 	void __iomem *base;
 	unsigned long next_heartbeat;	/* the next_heartbeat for the timer */
-	unsigned long open;
-	char expect_close;
 	struct timer_list timer;	/* The timer that pings the watchdog */
 } at91wdt_private;
 
 /* ......................................................................... */
 
-
 /*
  * Reload the watchdog timer.  (ie, pat the watchdog)
  */
@@ -95,39 +91,37 @@ static inline void at91_wdt_reset(void)
 static void at91_ping(unsigned long data)
 {
 	if (time_before(jiffies, at91wdt_private.next_heartbeat) ||
-			(!nowayout && !at91wdt_private.open)) {
+	    (!watchdog_active(&at91_wdt_dev))) {
 		at91_wdt_reset();
 		mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
 	} else
 		pr_crit("I will reset your machine !\n");
 }
 
-/*
- * Watchdog device is opened, and watchdog starts running.
- */
-static int at91_wdt_open(struct inode *inode, struct file *file)
+static int at91_wdt_ping(struct watchdog_device *wdd)
 {
-	if (test_and_set_bit(0, &at91wdt_private.open))
-		return -EBUSY;
+	/* calculate when the next userspace timeout will be */
+	at91wdt_private.next_heartbeat = jiffies + wdd->timeout * HZ;
+	return 0;
+}
 
-	at91wdt_private.next_heartbeat = jiffies + heartbeat * HZ;
+static int at91_wdt_start(struct watchdog_device *wdd)
+{
+	/* calculate the next userspace timeout and modify the timer */
+	at91_wdt_ping(wdd);
 	mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
-
-	return nonseekable_open(inode, file);
+	return 0;
 }
 
-/*
- * Close the watchdog device.
- */
-static int at91_wdt_close(struct inode *inode, struct file *file)
+static int at91_wdt_stop(struct watchdog_device *wdd)
 {
-	clear_bit(0, &at91wdt_private.open);
-
-	/* stop internal ping */
-	if (!at91wdt_private.expect_close)
-		del_timer(&at91wdt_private.timer);
+	/* The watchdog timer hardware can not be stopped... */
+	return 0;
+}
 
-	at91wdt_private.expect_close = 0;
+static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
+{
+	wdd->timeout = new_timeout;
 	return 0;
 }
 
@@ -163,96 +157,27 @@ static int at91_wdt_settimeout(unsigned int timeout)
 	return 0;
 }
 
+/* ......................................................................... */
+
 static const struct watchdog_info at91_wdt_info = {
 	.identity	= DRV_NAME,
 	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
 						WDIOF_MAGICCLOSE,
 };
 
-/*
- * Handle commands from user-space.
- */
-static long at91_wdt_ioctl(struct file *file,
-		unsigned int cmd, unsigned long arg)
-{
-	void __user *argp = (void __user *)arg;
-	int __user *p = argp;
-	int new_value;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(argp, &at91_wdt_info,
-				    sizeof(at91_wdt_info)) ? -EFAULT : 0;
-
-	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(0, p);
-
-	case WDIOC_KEEPALIVE:
-		at91wdt_private.next_heartbeat = jiffies + heartbeat * HZ;
-		return 0;
-
-	case WDIOC_SETTIMEOUT:
-		if (get_user(new_value, p))
-			return -EFAULT;
-
-		heartbeat = new_value;
-		at91wdt_private.next_heartbeat = jiffies + heartbeat * HZ;
-
-		return put_user(new_value, p);  /* return current value */
-
-	case WDIOC_GETTIMEOUT:
-		return put_user(heartbeat, p);
-	}
-	return -ENOTTY;
-}
-
-/*
- * Pat the watchdog whenever device is written to.
- */
-static ssize_t at91_wdt_write(struct file *file, const char *data, size_t len,
-								loff_t *ppos)
-{
-	if (!len)
-		return 0;
-
-	/* Scan for magic character */
-	if (!nowayout) {
-		size_t i;
-
-		at91wdt_private.expect_close = 0;
-
-		for (i = 0; i < len; i++) {
-			char c;
-			if (get_user(c, data + i))
-				return -EFAULT;
-			if (c == 'V') {
-				at91wdt_private.expect_close = 42;
-				break;
-			}
-		}
-	}
-
-	at91wdt_private.next_heartbeat = jiffies + heartbeat * HZ;
-
-	return len;
-}
-
-/* ......................................................................... */
-
-static const struct file_operations at91wdt_fops = {
-	.owner			= THIS_MODULE,
-	.llseek			= no_llseek,
-	.unlocked_ioctl	= at91_wdt_ioctl,
-	.open			= at91_wdt_open,
-	.release		= at91_wdt_close,
-	.write			= at91_wdt_write,
+static const struct watchdog_ops at91_wdt_ops = {
+	.owner =	THIS_MODULE,
+	.start =	at91_wdt_start,
+	.stop =		at91_wdt_stop,
+	.ping =		at91_wdt_ping,
+	.set_timeout =	at91_wdt_set_timeout,
 };
 
-static struct miscdevice at91wdt_miscdev = {
-	.minor		= WATCHDOG_MINOR,
-	.name		= "watchdog",
-	.fops		= &at91wdt_fops,
+static struct watchdog_device at91_wdt_dev = {
+	.info =		&at91_wdt_info,
+	.ops =		&at91_wdt_ops,
+	.min_timeout =	1,
+	.max_timeout =	0xFFFF,
 };
 
 static int __init at91wdt_probe(struct platform_device *pdev)
@@ -260,10 +185,6 @@ static int __init at91wdt_probe(struct platform_device *pdev)
 	struct resource	*r;
 	int res;
 
-	if (at91wdt_miscdev.parent)
-		return -EBUSY;
-	at91wdt_miscdev.parent = &pdev->dev;
-
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!r)
 		return -ENODEV;
@@ -273,16 +194,20 @@ static int __init at91wdt_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	at91_wdt_dev.timeout = heartbeat;
+	at91_wdt_dev.parent = &pdev->dev;
+	watchdog_set_nowayout(&at91_wdt_dev, nowayout);
+
 	/* Set watchdog */
 	res = at91_wdt_settimeout(ms_to_ticks(WDT_HW_TIMEOUT * 1000));
 	if (res)
 		return res;
 
-	res = misc_register(&at91wdt_miscdev);
+	res = watchdog_register_device(&at91_wdt_dev);
 	if (res)
 		return res;
 
-	at91wdt_private.next_heartbeat = jiffies + heartbeat * HZ;
+	at91wdt_private.next_heartbeat = jiffies + wdd->timeout * HZ;
 	setup_timer(&at91wdt_private.timer, at91_ping, 0);
 	mod_timer(&at91wdt_private.timer, jiffies + WDT_TIMEOUT);
 
@@ -294,13 +219,12 @@ static int __init at91wdt_probe(struct platform_device *pdev)
 
 static int __exit at91wdt_remove(struct platform_device *pdev)
 {
-	int res;
+	watchdog_unregister_device(&at91_wdt_dev);
 
-	res = misc_deregister(&at91wdt_miscdev);
-	if (!res)
-		at91wdt_miscdev.parent = NULL;
+	pr_warn("I quit now, hardware will probably reboot!\n");
+	del_timer(&at91wdt_private.timer);
 
-	return res;
+	return 0;
 }
 
 #if defined(CONFIG_OF)
@@ -337,4 +261,3 @@ module_exit(at91sam_wdt_exit);
 MODULE_AUTHOR("Renaud CERRATO <r.cerrato@til-technologies.fr>");
 MODULE_DESCRIPTION("Watchdog driver for Atmel AT91SAM9x processors");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

RE: [PATCH v4 3/8] watchdog/at91sam9_wdt: Convert to use the watchdog framework

[Yang, Wenyou]

Hi Wim,

> This code not only converts the watchdog to the new framework,
> but it also adds the is_enable related code changes which should
> be a seperate patch.
> 
> So I took your original patch and changed it to the below at91sam9_wdt
> watchdog conversion patch. Note: this is also without Fabio's timeout-sec
> patch, this one needs to come after the conversion.
> 
> Please test this patch and let me know if this works (the watchdog should
> behave the same before as after this patch).

Sorry for so late reply due to Chinese New Year's holiday.

I applied it on the linux-watchdog trees, 
and tested it on at91sam9m10g45ek both DT and non-DT support, at91sam9g25ek with DT support.

It works OK after one compilation error fixed.

> +	at91wdt_private.next_heartbeat = jiffies + wdd->timeout * HZ;
 +  at91wdt_private.next_heartbeat = jiffies + at91_wdt_dev.timeout * HZ;

Thank you very much.

Best Regards,
Wenyou Yang

[PATCH v4 4/8] watchdog/at91sam9_wdt: Adjust the options of watchdog_info

[Wenyou Yang]

Since the Watchdog Timer Mode Register can be only written only once,
so the watchdog_info shall not support WDIOF_SETTIMEOUT
and WDIOF_MAGICCLOSE options, remove them.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
Cc: wim@iguana.be
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/at91sam9_wdt.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index ce7930b..c6d9f1f 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -152,8 +152,7 @@ static int at91wdt_enable(struct watchdog_device *wddev, unsigned int timeout)
 
 static const struct watchdog_info at91_wdt_info = {
 	.identity	= DRV_NAME,
-	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
-						WDIOF_MAGICCLOSE,
+	.options	= WDIOF_KEEPALIVEPING,
 };
 
 static int at91wdt_start(struct watchdog_device *wddev)
-- 
1.7.9.5

Re: [PATCH v4 4/8] watchdog/at91sam9_wdt: Adjust the options of watchdog_info

[Wim Van Sebroeck]

Hi Wenyou,

> Since the Watchdog Timer Mode Register can be only written only once,
> so the watchdog_info shall not support WDIOF_SETTIMEOUT
> and WDIOF_MAGICCLOSE options, remove them.

Ik you keep using the timer, then you don't have to remove this.

Kind regards,
Wim.

Re: [PATCH v4 4/8] watchdog/at91sam9_wdt: Adjust the options of watchdog_info

[Alexander Stein]

Hello,

On Thursday 14 February 2013 00:15:22, Wim Van Sebroeck wrote:
> Hi Wenyou,
> 
> > Since the Watchdog Timer Mode Register can be only written only once,
> > so the watchdog_info shall not support WDIOF_SETTIMEOUT
> > and WDIOF_MAGICCLOSE options, remove them.
> 
> Ik you keep using the timer, then you don't have to remove this.

Why removing especially WDIOF_MAGICCLOSE (and the timer needed for that)? This is a feature removal. Without there isn't even a need for nowayout option as it is implicitly set anyway.
Nevertheless I would prefere keeping the timer to allow a user to let the kernel triggering the timer upon MAGICCLOSE.

Alexander

[PATCH v4 5/8] watchdog/at91sam9_wdt: Add nowayout helpers to Watchdog Timer Driver Kernel API

[Wenyou Yang]

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
Cc: wim@iguana.be
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/at91sam9_wdt.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index c6d9f1f..e60a718 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -238,6 +238,8 @@ static int __init at91wdt_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	watchdog_set_nowayout(&at91wdt_wdd, nowayout);
+
 	watchdog_init_timeout(&at91wdt_wdd, heartbeat, pdev->dev.of_node);
 
 	ret = at91wdt_enable(&at91wdt_wdd, ms_to_ticks(WDT_HW_TIMEOUT * 1000));
-- 
1.7.9.5

Re: [PATCH v4 5/8] watchdog/at91sam9_wdt: Add nowayout helpers to Watchdog Timer Driver Kernel API

[Wim Van Sebroeck]

Hi Wenyou

> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> index c6d9f1f..e60a718 100644
> --- a/drivers/watchdog/at91sam9_wdt.c
> +++ b/drivers/watchdog/at91sam9_wdt.c
> @@ -238,6 +238,8 @@ static int __init at91wdt_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	watchdog_set_nowayout(&at91wdt_wdd, nowayout);
> +
>  	watchdog_init_timeout(&at91wdt_wdd, heartbeat, pdev->dev.of_node);
>  
>  	ret = at91wdt_enable(&at91wdt_wdd, ms_to_ticks(WDT_HW_TIMEOUT * 1000));

allready part of the patch I just sent you.

Kind regards,
Wim.

[PATCH v4 6/8] watchdog/at91sam9_wdt: Remove the __initdata of at91wdt_wdd

[Wenyou Yang]

For this variable will be used in the timer handler.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
Cc: wim@iguana.be
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/at91sam9_wdt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index e60a718..3fc90ba 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -199,7 +199,7 @@ static struct watchdog_ops at91wdt_ops = {
 	.ping = at91wdt_ping,
 };
 
-static struct watchdog_device at91wdt_wdd __initdata = {
+static struct watchdog_device at91wdt_wdd = {
 	.timeout = WDT_HEARTBEAT,
 	.min_timeout = MIN_HEARTBEAT,
 	.max_timeout = MAX_HEARTBEAT,
-- 
1.7.9.5

Re: [PATCH v4 6/8] watchdog/at91sam9_wdt: Remove the __initdata of at91wdt_wdd

[Wim Van Sebroeck]

Hi Wenyou,

> For this variable will be used in the timer handler.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> Cc: wim@iguana.be
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/watchdog/at91sam9_wdt.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> index e60a718..3fc90ba 100644
> --- a/drivers/watchdog/at91sam9_wdt.c
> +++ b/drivers/watchdog/at91sam9_wdt.c
> @@ -199,7 +199,7 @@ static struct watchdog_ops at91wdt_ops = {
>  	.ping = at91wdt_ping,
>  };
>  
> -static struct watchdog_device at91wdt_wdd __initdata = {
> +static struct watchdog_device at91wdt_wdd = {
>  	.timeout = WDT_HEARTBEAT,
>  	.min_timeout = MIN_HEARTBEAT,
>  	.max_timeout = MAX_HEARTBEAT,

should imho also have been part of your previous patches.
Anyway, it's also like this in the patch I just sent you.

Kind regards,
Wim.

[PATCH v4 7/8] ARM: dts: add the watchdog nodes for at91sam9x5 and at91sam9n12 SoC

[Wenyou Yang]

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
Cc: linux@arm.linux.org.uk
Cc: linux-kernel@vger.kernel.org
---
 arch/arm/boot/dts/at91sam9n12.dtsi |    6 ++++++
 arch/arm/boot/dts/at91sam9x5.dtsi  |    6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/at91sam9n12.dtsi b/arch/arm/boot/dts/at91sam9n12.dtsi
index 80e29c6..8fecdd1 100644
--- a/arch/arm/boot/dts/at91sam9n12.dtsi
+++ b/arch/arm/boot/dts/at91sam9n12.dtsi
@@ -381,6 +381,12 @@
 				#size-cells = <0>;
 				status = "disabled";
 			};
+
+			watchdog@fffffe40 {
+				compatible = "atmel,at91sam9260-wdt";
+				reg = <0xfffffe40 0x10>;
+				status = "disabled";
+			};
 		};
 
 		nand0: nand@40000000 {
diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi
index 8ecca69..eadba6d 100644
--- a/arch/arm/boot/dts/at91sam9x5.dtsi
+++ b/arch/arm/boot/dts/at91sam9x5.dtsi
@@ -511,6 +511,12 @@
 					trigger-value = <0x6>;
 				};
 			};
+
+			watchdog@fffffe40 {
+				compatible = "atmel,at91sam9260-wdt";
+				reg = <0xfffffe40 0x10>;
+				status = "disabled";
+			};
 		};
 
 		nand0: nand@40000000 {
-- 
1.7.9.5

[PATCH v4 8/8] ARM: dts: add the watchdog nodes for at91sam9g25ek and at91sam9m10g45ek boards

[Wenyou Yang]

Tested on the at91sam9g25ek and at91sam9m10g45ek boards

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
Cc: linux@arm.linux.org.uk
Cc: linux-kernel@vger.kernel.org
---
 arch/arm/boot/dts/at91sam9m10g45ek.dts |    4 ++++
 arch/arm/boot/dts/at91sam9x5ek.dtsi    |    4 ++++
 2 files changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/at91sam9m10g45ek.dts b/arch/arm/boot/dts/at91sam9m10g45ek.dts
index 20c3191..0832c7a 100644
--- a/arch/arm/boot/dts/at91sam9m10g45ek.dts
+++ b/arch/arm/boot/dts/at91sam9m10g45ek.dts
@@ -59,6 +59,10 @@
 				status = "okay";
 			};
 
+			watchdog@fffffd40 {
+				status = "okay";
+			};
+
 			mmc0: mmc@fff80000 {
 				pinctrl-0 = <
 					&pinctrl_board_mmc0
diff --git a/arch/arm/boot/dts/at91sam9x5ek.dtsi b/arch/arm/boot/dts/at91sam9x5ek.dtsi
index 8a7cf1d..afddf75 100644
--- a/arch/arm/boot/dts/at91sam9x5ek.dtsi
+++ b/arch/arm/boot/dts/at91sam9x5ek.dtsi
@@ -69,6 +69,10 @@
 				status = "okay";
 			};
 
+			watchdog@fffffe40 {
+				status = "okay";
+			};
+
 			pinctrl@fffff400 {
 				mmc0 {
 					pinctrl_board_mmc0: mmc0-board {
-- 
1.7.9.5