[PATCH 0/4] pnx4008: convert to watchdog framework

[PATCH 0/4] pnx4008: convert to watchdog framework

[Wolfram Sang]

Thanks to Roland's work of making lpc32xx boot again, I could finally brush up
those patches and test them on the latest version of the watchdog framework.

Patch 3/4 is of special interest, because it extends the core to allow to set
timeouts without having a dedicated callback. Since the core automatically sets
'timeout' in the watchdog device (and does limit checking, too), there is
nothing else left to do for this device since it uses that value every time in
start/ping.

Please review and consider for inclusion. Roland, if you'd like to test them,
too, please do and donate "Tested-by" tags. Thanks!

Regards,

   Wolfram

Wolfram Sang (4):
  watchdog: pnx4008: cleanup resource handling using managed devices
  watchdog: pnx4008: don't use __raw_-accessors
  watchdog: dev: don't enforce set_timeout()
  watchdog: pnx4008: convert driver to use the watchdog framework

 .../watchdog/convert_drivers_to_kernel_api.txt     |   10 +-
 Documentation/watchdog/watchdog-kernel-api.txt     |    4 +-
 drivers/watchdog/Kconfig                           |    1 +
 drivers/watchdog/pnx4008_wdt.c                     |  264 +++++---------------
 drivers/watchdog/watchdog_dev.c                    |   11 +-
 5 files changed, 79 insertions(+), 211 deletions(-)

-- 
1.7.8.3

[PATCH 1/4] watchdog: pnx4008: cleanup resource handling using managed devices

[Wolfram Sang]

The resource handling in this driver was flaky: IO_ADDRESS instead of
ioremap (and no unmapping), an unneeded static resource, no central exit
path for error cases. Fix this by converting the driver to use managed
resources. Also use dev_*-messages instead of printk while we are here.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/watchdog/pnx4008_wdt.c |   74 ++++++++++++++-------------------------
 1 files changed, 27 insertions(+), 47 deletions(-)

diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 8e210aa..322a392 100644
--- a/drivers/watchdog/pnx4008_wdt.c
+++ b/drivers/watchdog/pnx4008_wdt.c
@@ -89,7 +89,6 @@ static unsigned long wdt_status;
 
 static unsigned long boot_status;
 
-static struct resource	*wdt_mem;
 static void __iomem	*wdt_base;
 struct clk		*wdt_clk;
 
@@ -253,61 +252,46 @@ static struct miscdevice pnx4008_wdt_miscdev = {
 
 static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
 {
-	int ret = 0, size;
+	struct resource *r;
+	int ret = 0;
 
 	if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT)
 		heartbeat = DEFAULT_HEARTBEAT;
 
-	printk(KERN_INFO MODULE_NAME
-		"PNX4008 Watchdog Timer: heartbeat %d sec\n", heartbeat);
-
-	wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (wdt_mem == NULL) {
-		printk(KERN_INFO MODULE_NAME
-			"failed to get memory region resouce\n");
-		return -ENOENT;
-	}
-
-	size = resource_size(wdt_mem);
-
-	if (!request_mem_region(wdt_mem->start, size, pdev->name)) {
-		printk(KERN_INFO MODULE_NAME "failed to get memory region\n");
-		return -ENOENT;
-	}
-	wdt_base = (void __iomem *)IO_ADDRESS(wdt_mem->start);
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	wdt_base = devm_request_and_ioremap(&pdev->dev, r);
+	if (!wdt_base)
+		return -EADDRINUSE;
 
 	wdt_clk = clk_get(&pdev->dev, NULL);
-	if (IS_ERR(wdt_clk)) {
-		ret = PTR_ERR(wdt_clk);
-		release_mem_region(wdt_mem->start, size);
-		wdt_mem = NULL;
-		goto out;
-	}
+	if (IS_ERR(wdt_clk))
+		return PTR_ERR(wdt_clk);
 
 	ret = clk_enable(wdt_clk);
-	if (ret) {
-		release_mem_region(wdt_mem->start, size);
-		wdt_mem = NULL;
-		clk_put(wdt_clk);
-		goto out;
-	}
+	if (ret)
+		goto put_clk;
 
 	ret = misc_register(&pnx4008_wdt_miscdev);
 	if (ret < 0) {
-		printk(KERN_ERR MODULE_NAME "cannot register misc device\n");
-		release_mem_region(wdt_mem->start, size);
-		wdt_mem = NULL;
-		clk_disable(wdt_clk);
-		clk_put(wdt_clk);
-	} else {
-		boot_status = (__raw_readl(WDTIM_RES(wdt_base)) & WDOG_RESET) ?
-		    WDIOF_CARDRESET : 0;
-		wdt_disable();		/*disable for now */
-		clk_disable(wdt_clk);
-		set_bit(WDT_DEVICE_INITED, &wdt_status);
+		dev_err(&pdev->dev, "cannot register misc device\n");
+		goto disable_clk;
 	}
 
-out:
+	boot_status = (__raw_readl(WDTIM_RES(wdt_base)) &
+		WDOG_RESET) ? WDIOF_CARDRESET : 0;
+	wdt_disable();		/*disable for now */
+	clk_disable(wdt_clk);
+	set_bit(WDT_DEVICE_INITED, &wdt_status);
+
+	dev_info(&pdev->dev, "PNX4008 Watchdog Timer: heartbeat %d sec\n",
+			heartbeat);
+
+	return 0;
+
+ disable_clk:
+	clk_disable(wdt_clk);
+ put_clk:
+	clk_put(wdt_clk);
 	return ret;
 }
 
@@ -318,10 +302,6 @@ static int __devexit pnx4008_wdt_remove(struct platform_device *pdev)
 	clk_disable(wdt_clk);
 	clk_put(wdt_clk);
 
-	if (wdt_mem) {
-		release_mem_region(wdt_mem->start, resource_size(wdt_mem));
-		wdt_mem = NULL;
-	}
 	return 0;
 }
 
-- 
1.7.8.3

[PATCH 2/4] watchdog: pnx4008: don't use __raw_-accessors

[Wolfram Sang]

__raw_readl/__raw_writel are not meant for drivers [1].

[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/117626

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/watchdog/pnx4008_wdt.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 322a392..6010602 100644
--- a/drivers/watchdog/pnx4008_wdt.c
+++ b/drivers/watchdog/pnx4008_wdt.c
@@ -97,22 +97,21 @@ static void wdt_enable(void)
 	spin_lock(&io_lock);
 
 	/* stop counter, initiate counter reset */
-	__raw_writel(RESET_COUNT, WDTIM_CTRL(wdt_base));
+	writel(RESET_COUNT, WDTIM_CTRL(wdt_base));
 	/*wait for reset to complete. 100% guarantee event */
-	while (__raw_readl(WDTIM_COUNTER(wdt_base)))
+	while (readl(WDTIM_COUNTER(wdt_base)))
 		cpu_relax();
 	/* internal and external reset, stop after that */
-	__raw_writel(M_RES2 | STOP_COUNT0 | RESET_COUNT0,
-		WDTIM_MCTRL(wdt_base));
+	writel(M_RES2 | STOP_COUNT0 | RESET_COUNT0, WDTIM_MCTRL(wdt_base));
 	/* configure match output */
-	__raw_writel(MATCH_OUTPUT_HIGH, WDTIM_EMR(wdt_base));
+	writel(MATCH_OUTPUT_HIGH, WDTIM_EMR(wdt_base));
 	/* clear interrupt, just in case */
-	__raw_writel(MATCH_INT, WDTIM_INT(wdt_base));
+	writel(MATCH_INT, WDTIM_INT(wdt_base));
 	/* the longest pulse period 65541/(13*10^6) seconds ~ 5 ms. */
-	__raw_writel(0xFFFF, WDTIM_PULSE(wdt_base));
-	__raw_writel(heartbeat * WDOG_COUNTER_RATE, WDTIM_MATCH0(wdt_base));
+	writel(0xFFFF, WDTIM_PULSE(wdt_base));
+	writel(heartbeat * WDOG_COUNTER_RATE, WDTIM_MATCH0(wdt_base));
 	/*enable counter, stop when debugger active */
-	__raw_writel(COUNT_ENAB | DEBUG_EN, WDTIM_CTRL(wdt_base));
+	writel(COUNT_ENAB | DEBUG_EN, WDTIM_CTRL(wdt_base));
 
 	spin_unlock(&io_lock);
 }
@@ -121,7 +120,7 @@ static void wdt_disable(void)
 {
 	spin_lock(&io_lock);
 
-	__raw_writel(0, WDTIM_CTRL(wdt_base));	/*stop counter */
+	writel(0, WDTIM_CTRL(wdt_base));	/*stop counter */
 
 	spin_unlock(&io_lock);
 }
@@ -277,7 +276,7 @@ static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
 		goto disable_clk;
 	}
 
-	boot_status = (__raw_readl(WDTIM_RES(wdt_base)) &
+	boot_status = (readl(WDTIM_RES(wdt_base)) &
 		WDOG_RESET) ? WDIOF_CARDRESET : 0;
 	wdt_disable();		/*disable for now */
 	clk_disable(wdt_clk);
-- 
1.7.8.3

[PATCH 3/4] watchdog: dev: don't enforce set_timeout()

[Wolfram Sang]

Some watchdogs rewrite the timer on every ping, so they don't need a specific
set_timeout callback. Allow the callback to be empty for such devices.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 .../watchdog/convert_drivers_to_kernel_api.txt     |   10 ++++++----
 Documentation/watchdog/watchdog-kernel-api.txt     |    4 +++-
 drivers/watchdog/watchdog_dev.c                    |   11 ++++++-----
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/Documentation/watchdog/convert_drivers_to_kernel_api.txt b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
index be8119b..25647a3 100644
--- a/Documentation/watchdog/convert_drivers_to_kernel_api.txt
+++ b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
@@ -51,10 +51,12 @@ Here is a overview of the functions and probably needed actions:
 		set
 
 	WDIOC_SETTIMEOUT:
-		Options in watchdog_info need to have WDIOF_SETTIMEOUT set
-		and a set_timeout-callback has to be defined. The core will also
-		do limit-checking, if min_timeout and max_timeout in the watchdog
-		device are set. All is optional.
+		Options in watchdog_info need to have WDIOF_SETTIMEOUT set. The core
+		will also do limit-checking, if min_timeout and max_timeout in the
+		watchdog device are set. By default, the timeout variable of the
+		watchdog device will simply get updated. Additionally, a
+		set_timeout-callback can be defined if further actions are needed
+		(e.g. hardware setup, more advanced checks). All is optional.
 
 	WDIOC_GETTIMEOUT:
 		No preparations needed
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 4b93c28..dcf1cfe 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -121,7 +121,9 @@ they are supported. These optional routines/operations are:
   value of the watchdog_device will be changed to the value that was just used
   to re-program the watchdog timer device.
   (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
-  watchdog's info structure).
+  watchdog's info structure. If it is set, and no set_timeout function is
+  provided, only the update of the timeout value will happen. This is enough
+  if the ping of the watchdog will rewrite the timer anyway.)
 * ioctl: if this routine is present then it will be called first before we do
   our own internal ioctl call handling. This routine should return -ENOIOCTLCMD
   if a command is not supported. The parameters that are passed to the ioctl
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 1199da0..ec224b4 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -215,17 +215,18 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 		watchdog_ping(wdd);
 		return 0;
 	case WDIOC_SETTIMEOUT:
-		if ((wdd->ops->set_timeout == NULL) ||
-		    !(wdd->info->options & WDIOF_SETTIMEOUT))
+		if (!(wdd->info->options & WDIOF_SETTIMEOUT))
 			return -EOPNOTSUPP;
 		if (get_user(val, p))
 			return -EFAULT;
 		if ((wdd->max_timeout != 0) &&
 		    (val < wdd->min_timeout || val > wdd->max_timeout))
 				return -EINVAL;
-		err = wdd->ops->set_timeout(wdd, val);
-		if (err < 0)
-			return err;
+		if (wdd->ops->set_timeout) {
+			err = wdd->ops->set_timeout(wdd, val);
+			if (err < 0)
+				return err;
+		}
 		wdd->timeout = val;
 		/* If the watchdog is active then we send a keepalive ping
 		 * to make sure that the watchdog keep's running (and if
-- 
1.7.8.3

[PATCH 4/4] watchdog: pnx4008: convert driver to use the watchdog framework

[Wolfram Sang]

Make this driver a user of the watchdog framework and remove parts now handled
by the core. Tested on a custom lpc32xx-board.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/watchdog/Kconfig       |    1 +
 drivers/watchdog/pnx4008_wdt.c |  185 +++++++--------------------------------
 2 files changed, 35 insertions(+), 151 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 877b107..f6827e4 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -234,6 +234,7 @@ config OMAP_WATCHDOG
 config PNX4008_WATCHDOG
 	tristate "PNX4008 and LPC32XX Watchdog"
 	depends on ARCH_PNX4008 || ARCH_LPC32XX
+	select WATCHDOG_CORE
 	help
 	  Say Y here if to include support for the watchdog timer
 	  in the PNX4008 or LPC32XX processor.
diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 6010602..eb86e3e 100644
--- a/drivers/watchdog/pnx4008_wdt.c
+++ b/drivers/watchdog/pnx4008_wdt.c
@@ -8,29 +8,27 @@
  * Based on sa1100 driver,
  * Copyright (C) 2000 Oleg Drokin <green@crimea.edu>
  *
- * 2005-2006 (c) MontaVista Software, Inc. This file is licensed under
- * the terms of the GNU General Public License version 2. This program
- * is licensed "as is" without any warranty of any kind, whether express
- * or implied.
+ * 2005-2006 (c) MontaVista Software, Inc.
+ *
+ * (C) 2012 Wolfram Sang, Pengutronix
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
  */
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
-#include <linux/fs.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>
 #include <linux/init.h>
-#include <linux/bitops.h>
-#include <linux/ioport.h>
-#include <linux/device.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
-#include <linux/spinlock.h>
-#include <linux/uaccess.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/err.h>
 #include <mach/hardware.h>
 
 #define MODULE_NAME "PNX4008-WDT: "
@@ -80,22 +78,11 @@
 static int nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = DEFAULT_HEARTBEAT;
 
-static DEFINE_SPINLOCK(io_lock);
-static unsigned long wdt_status;
-#define WDT_IN_USE        0
-#define WDT_OK_TO_CLOSE   1
-#define WDT_REGION_INITED 2
-#define WDT_DEVICE_INITED 3
-
-static unsigned long boot_status;
-
 static void __iomem	*wdt_base;
 struct clk		*wdt_clk;
 
-static void wdt_enable(void)
+static int pnx4008_wdt_start(struct watchdog_device *wdd)
 {
-	spin_lock(&io_lock);
-
 	/* stop counter, initiate counter reset */
 	writel(RESET_COUNT, WDTIM_CTRL(wdt_base));
 	/*wait for reset to complete. 100% guarantee event */
@@ -109,144 +96,37 @@ static void wdt_enable(void)
 	writel(MATCH_INT, WDTIM_INT(wdt_base));
 	/* the longest pulse period 65541/(13*10^6) seconds ~ 5 ms. */
 	writel(0xFFFF, WDTIM_PULSE(wdt_base));
-	writel(heartbeat * WDOG_COUNTER_RATE, WDTIM_MATCH0(wdt_base));
+	writel(wdd->timeout * WDOG_COUNTER_RATE, WDTIM_MATCH0(wdt_base));
 	/*enable counter, stop when debugger active */
 	writel(COUNT_ENAB | DEBUG_EN, WDTIM_CTRL(wdt_base));
 
-	spin_unlock(&io_lock);
+	return 0;
 }
 
-static void wdt_disable(void)
+static int pnx4008_wdt_stop(struct watchdog_device *wdd)
 {
-	spin_lock(&io_lock);
-
 	writel(0, WDTIM_CTRL(wdt_base));	/*stop counter */
 
-	spin_unlock(&io_lock);
-}
-
-static int pnx4008_wdt_open(struct inode *inode, struct file *file)
-{
-	int ret;
-
-	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
-		return -EBUSY;
-
-	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-
-	ret = clk_enable(wdt_clk);
-	if (ret) {
-		clear_bit(WDT_IN_USE, &wdt_status);
-		return ret;
-	}
-
-	wdt_enable();
-
-	return nonseekable_open(inode, file);
-}
-
-static ssize_t pnx4008_wdt_write(struct file *file, const char *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);
-			}
-		}
-		wdt_enable();
-	}
-
-	return len;
+	return 0;
 }
 
-static const struct watchdog_info ident = {
+static const struct watchdog_info pnx4008_wdt_ident = {
 	.options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE |
 	    WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
 	.identity = "PNX4008 Watchdog",
 };
 
-static long pnx4008_wdt_ioctl(struct file *file, unsigned int cmd,
-				unsigned long arg)
-{
-	int ret = -ENOTTY;
-	int time;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		ret = copy_to_user((struct watchdog_info *)arg, &ident,
-				   sizeof(ident)) ? -EFAULT : 0;
-		break;
-
-	case WDIOC_GETSTATUS:
-		ret = put_user(0, (int *)arg);
-		break;
-
-	case WDIOC_GETBOOTSTATUS:
-		ret = put_user(boot_status, (int *)arg);
-		break;
-
-	case WDIOC_KEEPALIVE:
-		wdt_enable();
-		ret = 0;
-		break;
-
-	case WDIOC_SETTIMEOUT:
-		ret = get_user(time, (int *)arg);
-		if (ret)
-			break;
-
-		if (time <= 0 || time > MAX_HEARTBEAT) {
-			ret = -EINVAL;
-			break;
-		}
-
-		heartbeat = time;
-		wdt_enable();
-		/* Fall through */
-
-	case WDIOC_GETTIMEOUT:
-		ret = put_user(heartbeat, (int *)arg);
-		break;
-	}
-	return ret;
-}
-
-static int pnx4008_wdt_release(struct inode *inode, struct file *file)
-{
-	if (!test_bit(WDT_OK_TO_CLOSE, &wdt_status))
-		printk(KERN_WARNING "WATCHDOG: Device closed unexpectedly\n");
-
-	wdt_disable();
-	clk_disable(wdt_clk);
-	clear_bit(WDT_IN_USE, &wdt_status);
-	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
-
-	return 0;
-}
-
-static const struct file_operations pnx4008_wdt_fops = {
+static struct watchdog_ops pnx4008_wdt_ops = {
 	.owner = THIS_MODULE,
-	.llseek = no_llseek,
-	.write = pnx4008_wdt_write,
-	.unlocked_ioctl = pnx4008_wdt_ioctl,
-	.open = pnx4008_wdt_open,
-	.release = pnx4008_wdt_release,
+	.start = pnx4008_wdt_start,
+	.stop = pnx4008_wdt_stop,
 };
 
-static struct miscdevice pnx4008_wdt_miscdev = {
-	.minor = WATCHDOG_MINOR,
-	.name = "watchdog",
-	.fops = &pnx4008_wdt_fops,
+static struct watchdog_device pnx4008_wdd = {
+	.info = &pnx4008_wdt_ident,
+	.ops = &pnx4008_wdt_ops,
+	.min_timeout = 1,
+	.max_timeout = MAX_HEARTBEAT,
 };
 
 static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
@@ -270,17 +150,19 @@ static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
 	if (ret)
 		goto put_clk;
 
-	ret = misc_register(&pnx4008_wdt_miscdev);
+	pnx4008_wdd.timeout = heartbeat;
+	pnx4008_wdd.bootstatus = (readl(WDTIM_RES(wdt_base)) & WDOG_RESET) ?
+				WDIOF_CARDRESET : 0;
+
+	watchdog_set_nowayout(&pnx4008_wdd, nowayout);
+
+	ret = watchdog_register_device(&pnx4008_wdd);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "cannot register misc device\n");
+		dev_err(&pdev->dev, "cannot register watchdog device\n");
 		goto disable_clk;
 	}
 
-	boot_status = (readl(WDTIM_RES(wdt_base)) &
-		WDOG_RESET) ? WDIOF_CARDRESET : 0;
-	wdt_disable();		/*disable for now */
-	clk_disable(wdt_clk);
-	set_bit(WDT_DEVICE_INITED, &wdt_status);
+	pnx4008_wdt_stop(&pnx4008_wdd);	/* disable for now */
 
 	dev_info(&pdev->dev, "PNX4008 Watchdog Timer: heartbeat %d sec\n",
 			heartbeat);
@@ -296,7 +178,7 @@ static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
 
 static int __devexit pnx4008_wdt_remove(struct platform_device *pdev)
 {
-	misc_deregister(&pnx4008_wdt_miscdev);
+	watchdog_unregister_device(&pnx4008_wdd);
 
 	clk_disable(wdt_clk);
 	clk_put(wdt_clk);
@@ -315,7 +197,8 @@ static struct platform_driver platform_wdt_driver = {
 
 module_platform_driver(platform_wdt_driver);
 
-MODULE_AUTHOR("MontaVista Software, Inc. <source@mvista.com>");
+MODULE_AUTHOR("MontaVista Software, Inc. <source@mvista.com> and "
+	"Wolfram Sang <w.sang@pengutronix.de>");
 MODULE_DESCRIPTION("PNX4008 Watchdog Driver");
 
 module_param(heartbeat, int, 0);
-- 
1.7.8.3

Re: [PATCH 0/4] pnx4008: convert to watchdog framework

[Roland Stigge]

On 02/02/2012 06:48 PM, Wolfram Sang wrote:
> Patch 3/4 is of special interest, because it extends the core to allow to set
> timeouts without having a dedicated callback. Since the core automatically sets
> 'timeout' in the watchdog device (and does limit checking, too), there is
> nothing else left to do for this device since it uses that value every time in
> start/ping.
> 
> Please review and consider for inclusion. Roland, if you'd like to test them,
> too, please do and donate "Tested-by" tags. Thanks!

Works for me: Applies cleanly, compiles, boots, watchdog barks
(rebooting appropriately when using /dev/watchdog).

Roland

Re: [PATCH 1/4] watchdog: pnx4008: cleanup resource handling using managed devices

[Roland Stigge]

On 02/02/2012 06:48 PM, Wolfram Sang wrote:
> The resource handling in this driver was flaky: IO_ADDRESS instead of
> ioremap (and no unmapping), an unneeded static resource, no central exit
> path for error cases. Fix this by converting the driver to use managed
> resources. Also use dev_*-messages instead of printk while we are here.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  drivers/watchdog/pnx4008_wdt.c |   74 ++++++++++++++-------------------------
>  1 files changed, 27 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
> index 8e210aa..322a392 100644
> --- a/drivers/watchdog/pnx4008_wdt.c
> +++ b/drivers/watchdog/pnx4008_wdt.c
> @@ -89,7 +89,6 @@ static unsigned long wdt_status;
>  
>  static unsigned long boot_status;
>  
> -static struct resource	*wdt_mem;
>  static void __iomem	*wdt_base;
>  struct clk		*wdt_clk;
>  
> @@ -253,61 +252,46 @@ static struct miscdevice pnx4008_wdt_miscdev = {
>  
>  static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
>  {
> -	int ret = 0, size;
> +	struct resource *r;
> +	int ret = 0;
>  
>  	if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT)
>  		heartbeat = DEFAULT_HEARTBEAT;
>  
> -	printk(KERN_INFO MODULE_NAME
> -		"PNX4008 Watchdog Timer: heartbeat %d sec\n", heartbeat);
> -
> -	wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (wdt_mem == NULL) {
> -		printk(KERN_INFO MODULE_NAME
> -			"failed to get memory region resouce\n");
> -		return -ENOENT;
> -	}
> -
> -	size = resource_size(wdt_mem);
> -
> -	if (!request_mem_region(wdt_mem->start, size, pdev->name)) {
> -		printk(KERN_INFO MODULE_NAME "failed to get memory region\n");
> -		return -ENOENT;
> -	}
> -	wdt_base = (void __iomem *)IO_ADDRESS(wdt_mem->start);
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	wdt_base = devm_request_and_ioremap(&pdev->dev, r);
> +	if (!wdt_base)
> +		return -EADDRINUSE;
>  
>  	wdt_clk = clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(wdt_clk)) {
> -		ret = PTR_ERR(wdt_clk);
> -		release_mem_region(wdt_mem->start, size);
> -		wdt_mem = NULL;
> -		goto out;
> -	}
> +	if (IS_ERR(wdt_clk))
> +		return PTR_ERR(wdt_clk);
>  
>  	ret = clk_enable(wdt_clk);
> -	if (ret) {
> -		release_mem_region(wdt_mem->start, size);
> -		wdt_mem = NULL;
> -		clk_put(wdt_clk);
> -		goto out;
> -	}
> +	if (ret)
> +		goto put_clk;
>  
>  	ret = misc_register(&pnx4008_wdt_miscdev);
>  	if (ret < 0) {
> -		printk(KERN_ERR MODULE_NAME "cannot register misc device\n");
> -		release_mem_region(wdt_mem->start, size);
> -		wdt_mem = NULL;
> -		clk_disable(wdt_clk);
> -		clk_put(wdt_clk);
> -	} else {
> -		boot_status = (__raw_readl(WDTIM_RES(wdt_base)) & WDOG_RESET) ?
> -		    WDIOF_CARDRESET : 0;
> -		wdt_disable();		/*disable for now */
> -		clk_disable(wdt_clk);
> -		set_bit(WDT_DEVICE_INITED, &wdt_status);
> +		dev_err(&pdev->dev, "cannot register misc device\n");
> +		goto disable_clk;
>  	}
>  
> -out:
> +	boot_status = (__raw_readl(WDTIM_RES(wdt_base)) &
> +		WDOG_RESET) ? WDIOF_CARDRESET : 0;
> +	wdt_disable();		/*disable for now */
> +	clk_disable(wdt_clk);
> +	set_bit(WDT_DEVICE_INITED, &wdt_status);
> +
> +	dev_info(&pdev->dev, "PNX4008 Watchdog Timer: heartbeat %d sec\n",
> +			heartbeat);
> +
> +	return 0;
> +
> + disable_clk:
> +	clk_disable(wdt_clk);
> + put_clk:
> +	clk_put(wdt_clk);
>  	return ret;
>  }
>  
> @@ -318,10 +302,6 @@ static int __devexit pnx4008_wdt_remove(struct platform_device *pdev)
>  	clk_disable(wdt_clk);
>  	clk_put(wdt_clk);
>  
> -	if (wdt_mem) {
> -		release_mem_region(wdt_mem->start, resource_size(wdt_mem));
> -		wdt_mem = NULL;
> -	}
>  	return 0;
>  }
>  

Tested-by: Roland Stigge <stigge@antcom.de>

Re: [PATCH 2/4] watchdog: pnx4008: don't use __raw_-accessors

[Roland Stigge]

On 02/02/2012 06:48 PM, Wolfram Sang wrote:
> __raw_readl/__raw_writel are not meant for drivers [1].
> 
> [1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/117626
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  drivers/watchdog/pnx4008_wdt.c |   21 ++++++++++-----------
>  1 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
> index 322a392..6010602 100644
> --- a/drivers/watchdog/pnx4008_wdt.c
> +++ b/drivers/watchdog/pnx4008_wdt.c
> @@ -97,22 +97,21 @@ static void wdt_enable(void)
>  	spin_lock(&io_lock);
>  
>  	/* stop counter, initiate counter reset */
> -	__raw_writel(RESET_COUNT, WDTIM_CTRL(wdt_base));
> +	writel(RESET_COUNT, WDTIM_CTRL(wdt_base));
>  	/*wait for reset to complete. 100% guarantee event */
> -	while (__raw_readl(WDTIM_COUNTER(wdt_base)))
> +	while (readl(WDTIM_COUNTER(wdt_base)))
>  		cpu_relax();
>  	/* internal and external reset, stop after that */
> -	__raw_writel(M_RES2 | STOP_COUNT0 | RESET_COUNT0,
> -		WDTIM_MCTRL(wdt_base));
> +	writel(M_RES2 | STOP_COUNT0 | RESET_COUNT0, WDTIM_MCTRL(wdt_base));
>  	/* configure match output */
> -	__raw_writel(MATCH_OUTPUT_HIGH, WDTIM_EMR(wdt_base));
> +	writel(MATCH_OUTPUT_HIGH, WDTIM_EMR(wdt_base));
>  	/* clear interrupt, just in case */
> -	__raw_writel(MATCH_INT, WDTIM_INT(wdt_base));
> +	writel(MATCH_INT, WDTIM_INT(wdt_base));
>  	/* the longest pulse period 65541/(13*10^6) seconds ~ 5 ms. */
> -	__raw_writel(0xFFFF, WDTIM_PULSE(wdt_base));
> -	__raw_writel(heartbeat * WDOG_COUNTER_RATE, WDTIM_MATCH0(wdt_base));
> +	writel(0xFFFF, WDTIM_PULSE(wdt_base));
> +	writel(heartbeat * WDOG_COUNTER_RATE, WDTIM_MATCH0(wdt_base));
>  	/*enable counter, stop when debugger active */
> -	__raw_writel(COUNT_ENAB | DEBUG_EN, WDTIM_CTRL(wdt_base));
> +	writel(COUNT_ENAB | DEBUG_EN, WDTIM_CTRL(wdt_base));
>  
>  	spin_unlock(&io_lock);
>  }
> @@ -121,7 +120,7 @@ static void wdt_disable(void)
>  {
>  	spin_lock(&io_lock);
>  
> -	__raw_writel(0, WDTIM_CTRL(wdt_base));	/*stop counter */
> +	writel(0, WDTIM_CTRL(wdt_base));	/*stop counter */
>  
>  	spin_unlock(&io_lock);
>  }
> @@ -277,7 +276,7 @@ static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
>  		goto disable_clk;
>  	}
>  
> -	boot_status = (__raw_readl(WDTIM_RES(wdt_base)) &
> +	boot_status = (readl(WDTIM_RES(wdt_base)) &
>  		WDOG_RESET) ? WDIOF_CARDRESET : 0;
>  	wdt_disable();		/*disable for now */
>  	clk_disable(wdt_clk);

Tested-by: Roland Stigge <stigge@antcom.de>

Re: [PATCH 3/4] watchdog: dev: don't enforce set_timeout()

[Roland Stigge]

On 02/02/2012 06:48 PM, Wolfram Sang wrote:
> Some watchdogs rewrite the timer on every ping, so they don't need a specific
> set_timeout callback. Allow the callback to be empty for such devices.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  .../watchdog/convert_drivers_to_kernel_api.txt     |   10 ++++++----
>  Documentation/watchdog/watchdog-kernel-api.txt     |    4 +++-
>  drivers/watchdog/watchdog_dev.c                    |   11 ++++++-----
>  3 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/watchdog/convert_drivers_to_kernel_api.txt b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
> index be8119b..25647a3 100644
> --- a/Documentation/watchdog/convert_drivers_to_kernel_api.txt
> +++ b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
> @@ -51,10 +51,12 @@ Here is a overview of the functions and probably needed actions:
>  		set
>  
>  	WDIOC_SETTIMEOUT:
> -		Options in watchdog_info need to have WDIOF_SETTIMEOUT set
> -		and a set_timeout-callback has to be defined. The core will also
> -		do limit-checking, if min_timeout and max_timeout in the watchdog
> -		device are set. All is optional.
> +		Options in watchdog_info need to have WDIOF_SETTIMEOUT set. The core
> +		will also do limit-checking, if min_timeout and max_timeout in the
> +		watchdog device are set. By default, the timeout variable of the
> +		watchdog device will simply get updated. Additionally, a
> +		set_timeout-callback can be defined if further actions are needed
> +		(e.g. hardware setup, more advanced checks). All is optional.
>  
>  	WDIOC_GETTIMEOUT:
>  		No preparations needed
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index 4b93c28..dcf1cfe 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -121,7 +121,9 @@ they are supported. These optional routines/operations are:
>    value of the watchdog_device will be changed to the value that was just used
>    to re-program the watchdog timer device.
>    (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
> -  watchdog's info structure).
> +  watchdog's info structure. If it is set, and no set_timeout function is
> +  provided, only the update of the timeout value will happen. This is enough
> +  if the ping of the watchdog will rewrite the timer anyway.)
>  * ioctl: if this routine is present then it will be called first before we do
>    our own internal ioctl call handling. This routine should return -ENOIOCTLCMD
>    if a command is not supported. The parameters that are passed to the ioctl
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 1199da0..ec224b4 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -215,17 +215,18 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>  		watchdog_ping(wdd);
>  		return 0;
>  	case WDIOC_SETTIMEOUT:
> -		if ((wdd->ops->set_timeout == NULL) ||
> -		    !(wdd->info->options & WDIOF_SETTIMEOUT))
> +		if (!(wdd->info->options & WDIOF_SETTIMEOUT))
>  			return -EOPNOTSUPP;
>  		if (get_user(val, p))
>  			return -EFAULT;
>  		if ((wdd->max_timeout != 0) &&
>  		    (val < wdd->min_timeout || val > wdd->max_timeout))
>  				return -EINVAL;
> -		err = wdd->ops->set_timeout(wdd, val);
> -		if (err < 0)
> -			return err;
> +		if (wdd->ops->set_timeout) {
> +			err = wdd->ops->set_timeout(wdd, val);
> +			if (err < 0)
> +				return err;
> +		}
>  		wdd->timeout = val;
>  		/* If the watchdog is active then we send a keepalive ping
>  		 * to make sure that the watchdog keep's running (and if

Tested-by: Roland Stigge <stigge@antcom.de>

Re: [PATCH 4/4] watchdog: pnx4008: convert driver to use the watchdog framework

[Roland Stigge]

On 02/02/2012 06:48 PM, Wolfram Sang wrote:
> Make this driver a user of the watchdog framework and remove parts now handled
> by the core. Tested on a custom lpc32xx-board.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  drivers/watchdog/Kconfig       |    1 +
>  drivers/watchdog/pnx4008_wdt.c |  185 +++++++--------------------------------
>  2 files changed, 35 insertions(+), 151 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 877b107..f6827e4 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -234,6 +234,7 @@ config OMAP_WATCHDOG
>  config PNX4008_WATCHDOG
>  	tristate "PNX4008 and LPC32XX Watchdog"
>  	depends on ARCH_PNX4008 || ARCH_LPC32XX
> +	select WATCHDOG_CORE
>  	help
>  	  Say Y here if to include support for the watchdog timer
>  	  in the PNX4008 or LPC32XX processor.
> diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
> index 6010602..eb86e3e 100644
> --- a/drivers/watchdog/pnx4008_wdt.c
> +++ b/drivers/watchdog/pnx4008_wdt.c
> @@ -8,29 +8,27 @@
>   * Based on sa1100 driver,
>   * Copyright (C) 2000 Oleg Drokin <green@crimea.edu>
>   *
> - * 2005-2006 (c) MontaVista Software, Inc. This file is licensed under
> - * the terms of the GNU General Public License version 2. This program
> - * is licensed "as is" without any warranty of any kind, whether express
> - * or implied.
> + * 2005-2006 (c) MontaVista Software, Inc.
> + *
> + * (C) 2012 Wolfram Sang, Pengutronix
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
>   */
>  
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
>  #include <linux/types.h>
>  #include <linux/kernel.h>
> -#include <linux/fs.h>
>  #include <linux/miscdevice.h>
>  #include <linux/watchdog.h>
>  #include <linux/init.h>
> -#include <linux/bitops.h>
> -#include <linux/ioport.h>
> -#include <linux/device.h>
>  #include <linux/platform_device.h>
>  #include <linux/clk.h>
> -#include <linux/spinlock.h>
> -#include <linux/uaccess.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
> +#include <linux/err.h>
>  #include <mach/hardware.h>
>  
>  #define MODULE_NAME "PNX4008-WDT: "
> @@ -80,22 +78,11 @@
>  static int nowayout = WATCHDOG_NOWAYOUT;
>  static int heartbeat = DEFAULT_HEARTBEAT;
>  
> -static DEFINE_SPINLOCK(io_lock);
> -static unsigned long wdt_status;
> -#define WDT_IN_USE        0
> -#define WDT_OK_TO_CLOSE   1
> -#define WDT_REGION_INITED 2
> -#define WDT_DEVICE_INITED 3
> -
> -static unsigned long boot_status;
> -
>  static void __iomem	*wdt_base;
>  struct clk		*wdt_clk;
>  
> -static void wdt_enable(void)
> +static int pnx4008_wdt_start(struct watchdog_device *wdd)
>  {
> -	spin_lock(&io_lock);
> -
>  	/* stop counter, initiate counter reset */
>  	writel(RESET_COUNT, WDTIM_CTRL(wdt_base));
>  	/*wait for reset to complete. 100% guarantee event */
> @@ -109,144 +96,37 @@ static void wdt_enable(void)
>  	writel(MATCH_INT, WDTIM_INT(wdt_base));
>  	/* the longest pulse period 65541/(13*10^6) seconds ~ 5 ms. */
>  	writel(0xFFFF, WDTIM_PULSE(wdt_base));
> -	writel(heartbeat * WDOG_COUNTER_RATE, WDTIM_MATCH0(wdt_base));
> +	writel(wdd->timeout * WDOG_COUNTER_RATE, WDTIM_MATCH0(wdt_base));
>  	/*enable counter, stop when debugger active */
>  	writel(COUNT_ENAB | DEBUG_EN, WDTIM_CTRL(wdt_base));
>  
> -	spin_unlock(&io_lock);
> +	return 0;
>  }
>  
> -static void wdt_disable(void)
> +static int pnx4008_wdt_stop(struct watchdog_device *wdd)
>  {
> -	spin_lock(&io_lock);
> -
>  	writel(0, WDTIM_CTRL(wdt_base));	/*stop counter */
>  
> -	spin_unlock(&io_lock);
> -}
> -
> -static int pnx4008_wdt_open(struct inode *inode, struct file *file)
> -{
> -	int ret;
> -
> -	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
> -		return -EBUSY;
> -
> -	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
> -
> -	ret = clk_enable(wdt_clk);
> -	if (ret) {
> -		clear_bit(WDT_IN_USE, &wdt_status);
> -		return ret;
> -	}
> -
> -	wdt_enable();
> -
> -	return nonseekable_open(inode, file);
> -}
> -
> -static ssize_t pnx4008_wdt_write(struct file *file, const char *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);
> -			}
> -		}
> -		wdt_enable();
> -	}
> -
> -	return len;
> +	return 0;
>  }
>  
> -static const struct watchdog_info ident = {
> +static const struct watchdog_info pnx4008_wdt_ident = {
>  	.options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE |
>  	    WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
>  	.identity = "PNX4008 Watchdog",
>  };
>  
> -static long pnx4008_wdt_ioctl(struct file *file, unsigned int cmd,
> -				unsigned long arg)
> -{
> -	int ret = -ENOTTY;
> -	int time;
> -
> -	switch (cmd) {
> -	case WDIOC_GETSUPPORT:
> -		ret = copy_to_user((struct watchdog_info *)arg, &ident,
> -				   sizeof(ident)) ? -EFAULT : 0;
> -		break;
> -
> -	case WDIOC_GETSTATUS:
> -		ret = put_user(0, (int *)arg);
> -		break;
> -
> -	case WDIOC_GETBOOTSTATUS:
> -		ret = put_user(boot_status, (int *)arg);
> -		break;
> -
> -	case WDIOC_KEEPALIVE:
> -		wdt_enable();
> -		ret = 0;
> -		break;
> -
> -	case WDIOC_SETTIMEOUT:
> -		ret = get_user(time, (int *)arg);
> -		if (ret)
> -			break;
> -
> -		if (time <= 0 || time > MAX_HEARTBEAT) {
> -			ret = -EINVAL;
> -			break;
> -		}
> -
> -		heartbeat = time;
> -		wdt_enable();
> -		/* Fall through */
> -
> -	case WDIOC_GETTIMEOUT:
> -		ret = put_user(heartbeat, (int *)arg);
> -		break;
> -	}
> -	return ret;
> -}
> -
> -static int pnx4008_wdt_release(struct inode *inode, struct file *file)
> -{
> -	if (!test_bit(WDT_OK_TO_CLOSE, &wdt_status))
> -		printk(KERN_WARNING "WATCHDOG: Device closed unexpectedly\n");
> -
> -	wdt_disable();
> -	clk_disable(wdt_clk);
> -	clear_bit(WDT_IN_USE, &wdt_status);
> -	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
> -
> -	return 0;
> -}
> -
> -static const struct file_operations pnx4008_wdt_fops = {
> +static struct watchdog_ops pnx4008_wdt_ops = {
>  	.owner = THIS_MODULE,
> -	.llseek = no_llseek,
> -	.write = pnx4008_wdt_write,
> -	.unlocked_ioctl = pnx4008_wdt_ioctl,
> -	.open = pnx4008_wdt_open,
> -	.release = pnx4008_wdt_release,
> +	.start = pnx4008_wdt_start,
> +	.stop = pnx4008_wdt_stop,
>  };
>  
> -static struct miscdevice pnx4008_wdt_miscdev = {
> -	.minor = WATCHDOG_MINOR,
> -	.name = "watchdog",
> -	.fops = &pnx4008_wdt_fops,
> +static struct watchdog_device pnx4008_wdd = {
> +	.info = &pnx4008_wdt_ident,
> +	.ops = &pnx4008_wdt_ops,
> +	.min_timeout = 1,
> +	.max_timeout = MAX_HEARTBEAT,
>  };
>  
>  static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
> @@ -270,17 +150,19 @@ static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto put_clk;
>  
> -	ret = misc_register(&pnx4008_wdt_miscdev);
> +	pnx4008_wdd.timeout = heartbeat;
> +	pnx4008_wdd.bootstatus = (readl(WDTIM_RES(wdt_base)) & WDOG_RESET) ?
> +				WDIOF_CARDRESET : 0;
> +
> +	watchdog_set_nowayout(&pnx4008_wdd, nowayout);
> +
> +	ret = watchdog_register_device(&pnx4008_wdd);
>  	if (ret < 0) {
> -		dev_err(&pdev->dev, "cannot register misc device\n");
> +		dev_err(&pdev->dev, "cannot register watchdog device\n");
>  		goto disable_clk;
>  	}
>  
> -	boot_status = (readl(WDTIM_RES(wdt_base)) &
> -		WDOG_RESET) ? WDIOF_CARDRESET : 0;
> -	wdt_disable();		/*disable for now */
> -	clk_disable(wdt_clk);
> -	set_bit(WDT_DEVICE_INITED, &wdt_status);
> +	pnx4008_wdt_stop(&pnx4008_wdd);	/* disable for now */
>  
>  	dev_info(&pdev->dev, "PNX4008 Watchdog Timer: heartbeat %d sec\n",
>  			heartbeat);
> @@ -296,7 +178,7 @@ static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
>  
>  static int __devexit pnx4008_wdt_remove(struct platform_device *pdev)
>  {
> -	misc_deregister(&pnx4008_wdt_miscdev);
> +	watchdog_unregister_device(&pnx4008_wdd);
>  
>  	clk_disable(wdt_clk);
>  	clk_put(wdt_clk);
> @@ -315,7 +197,8 @@ static struct platform_driver platform_wdt_driver = {
>  
>  module_platform_driver(platform_wdt_driver);
>  
> -MODULE_AUTHOR("MontaVista Software, Inc. <source@mvista.com>");
> +MODULE_AUTHOR("MontaVista Software, Inc. <source@mvista.com> and "
> +	"Wolfram Sang <w.sang@pengutronix.de>");
>  MODULE_DESCRIPTION("PNX4008 Watchdog Driver");
>  
>  module_param(heartbeat, int, 0);

Tested-by: Roland Stigge <stigge@antcom.de>

Re: [PATCH 0/4] pnx4008: convert to watchdog framework

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

On Thu, Feb 02, 2012 at 09:02:13PM +0100, Roland Stigge wrote:
> On 02/02/2012 06:48 PM, Wolfram Sang wrote:
> > Patch 3/4 is of special interest, because it extends the core to allow to set
> > timeouts without having a dedicated callback. Since the core automatically sets
> > 'timeout' in the watchdog device (and does limit checking, too), there is
> > nothing else left to do for this device since it uses that value every time in
> > start/ping.
> > 
> > Please review and consider for inclusion. Roland, if you'd like to test them,
> > too, please do and donate "Tested-by" tags. Thanks!
> 
> Works for me: Applies cleanly, compiles, boots, watchdog barks
> (rebooting appropriately when using /dev/watchdog).

Great, thanks!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/4] watchdog: dev: don't enforce set_timeout()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 4152 bytes --]

On Thu, Feb 02, 2012 at 06:48:10PM +0100, Wolfram Sang wrote:
> Some watchdogs rewrite the timer on every ping, so they don't need a specific
> set_timeout callback. Allow the callback to be empty for such devices.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>

Wim, I saw that the first two patches are in linux-next, but not this and the
following one? Any remarks?

Thanks,

   Wolfram


> ---
>  .../watchdog/convert_drivers_to_kernel_api.txt     |   10 ++++++----
>  Documentation/watchdog/watchdog-kernel-api.txt     |    4 +++-
>  drivers/watchdog/watchdog_dev.c                    |   11 ++++++-----
>  3 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/watchdog/convert_drivers_to_kernel_api.txt b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
> index be8119b..25647a3 100644
> --- a/Documentation/watchdog/convert_drivers_to_kernel_api.txt
> +++ b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
> @@ -51,10 +51,12 @@ Here is a overview of the functions and probably needed actions:
>  		set
>  
>  	WDIOC_SETTIMEOUT:
> -		Options in watchdog_info need to have WDIOF_SETTIMEOUT set
> -		and a set_timeout-callback has to be defined. The core will also
> -		do limit-checking, if min_timeout and max_timeout in the watchdog
> -		device are set. All is optional.
> +		Options in watchdog_info need to have WDIOF_SETTIMEOUT set. The core
> +		will also do limit-checking, if min_timeout and max_timeout in the
> +		watchdog device are set. By default, the timeout variable of the
> +		watchdog device will simply get updated. Additionally, a
> +		set_timeout-callback can be defined if further actions are needed
> +		(e.g. hardware setup, more advanced checks). All is optional.
>  
>  	WDIOC_GETTIMEOUT:
>  		No preparations needed
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index 4b93c28..dcf1cfe 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -121,7 +121,9 @@ they are supported. These optional routines/operations are:
>    value of the watchdog_device will be changed to the value that was just used
>    to re-program the watchdog timer device.
>    (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
> -  watchdog's info structure).
> +  watchdog's info structure. If it is set, and no set_timeout function is
> +  provided, only the update of the timeout value will happen. This is enough
> +  if the ping of the watchdog will rewrite the timer anyway.)
>  * ioctl: if this routine is present then it will be called first before we do
>    our own internal ioctl call handling. This routine should return -ENOIOCTLCMD
>    if a command is not supported. The parameters that are passed to the ioctl
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 1199da0..ec224b4 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -215,17 +215,18 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>  		watchdog_ping(wdd);
>  		return 0;
>  	case WDIOC_SETTIMEOUT:
> -		if ((wdd->ops->set_timeout == NULL) ||
> -		    !(wdd->info->options & WDIOF_SETTIMEOUT))
> +		if (!(wdd->info->options & WDIOF_SETTIMEOUT))
>  			return -EOPNOTSUPP;
>  		if (get_user(val, p))
>  			return -EFAULT;
>  		if ((wdd->max_timeout != 0) &&
>  		    (val < wdd->min_timeout || val > wdd->max_timeout))
>  				return -EINVAL;
> -		err = wdd->ops->set_timeout(wdd, val);
> -		if (err < 0)
> -			return err;
> +		if (wdd->ops->set_timeout) {
> +			err = wdd->ops->set_timeout(wdd, val);
> +			if (err < 0)
> +				return err;
> +		}
>  		wdd->timeout = val;
>  		/* If the watchdog is active then we send a keepalive ping
>  		 * to make sure that the watchdog keep's running (and if
> -- 
> 1.7.8.3
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/4] watchdog: dev: don't enforce set_timeout()

[Wim Van Sebroeck]

Hi Wolfram,

> On Thu, Feb 02, 2012 at 06:48:10PM +0100, Wolfram Sang wrote:
> > Some watchdogs rewrite the timer on every ping, so they don't need a specific
> > set_timeout callback. Allow the callback to be empty for such devices.
> > 
> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> 
> Wim, I saw that the first two patches are in linux-next, but not this and the
> following one? Any remarks?

first 2 patches are in indeed, but the 4th one (with some adoptions because I left
3 out) is also in. I have to look at patch 3 again, but I gave priority to Hans his
patch nr 4: watchdog_dev: Let the driver update the timeout field on set_timeout
success. Some device indeed don't have a 1 second resolution. That's why it should
be the drivers responsibility to update the code.

But on the other hand, you also are right to say that a lot of drivers just will do:

static int wdt_set_timeout(struct watchdog_device *wdd, unsigned timeout)
{
	wdd->timeout = timeout;
	retun 0;
}

For this we can add a helper or adjust the watchdog code to do this as default.
I have to check out what the best solution is.

Kind regards,
Wim.