[RFC 0/6] watchdog drivers converted to the new framework

[RFC 0/6] watchdog drivers converted to the new framework

[Wolfram Sang]

As promised, here is a RFC with two examples demonstrating how watchdog drivers
can be converted to use the new watchdog framework (using the current version
Wim posted two days ago). There is also conversion guide put to the
documentation folder. Being RFC, all this is not final yet, but presentable, I
hope.

Although there are a few more consolidation options left, there is already a
gain of ~100 lines per driver. Promising, but there are a few issues to be
sorted out, too, yet nothing which can be dealt with.

I have two other drivers in the making (stmp3xxx and imx2), but they need some
more preparation; the first one needs some internal cleanups (like a lot of
watchdog drivers); the latter one needs an addition to the framework
(installing a timer for non-stoppable devices). I will also prepare a new
driver (mx1) to show how small new drivers can be now :) The aim for all these
driver conversions is inclusion in Linux 3.2. I still hope we can get the basic
framework into Linux 3.1.

Many thanks to CELF/LF for supporting this work and to Wim and Alan for making
the framework!

Looking forward to comments,

   Wolfram

They are also available in the git repository at:
  git://git.pengutronix.de/git/wsa/linux-2.6.git generic-watchdog-with-drivers

Wolfram Sang (6):
  Documentation: watchdog: add guide how to convert drivers to new framework
  watchdog: s3c2410: convert to use the watchdog framework
  watchdog: pnx4008: cleanup resource handling using managed devices
  watchdog: pnx4008: don't use __raw_-accessors
  watchdog: pnx4008: convert driver to use the watchdog framework
  watchdog: pnx4008: WIP refactor disabling device

 .../watchdog/convert_drivers_to_kernel_api.txt     |  195 ++++++++++++++++
 drivers/watchdog/Kconfig                           |    2 +
 drivers/watchdog/pnx4008_wdt.c                     |  246 +++++++-------------
 drivers/watchdog/s3c2410_wdt.c                     |  176 +++-----------
 4 files changed, 313 insertions(+), 306 deletions(-)
 create mode 100644 Documentation/watchdog/convert_drivers_to_kernel_api.txt

-- 
1.7.2.5

[RFC 1/6] Documentation: watchdog: add guide how to convert drivers to new framework

[Wolfram Sang]

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 .../watchdog/convert_drivers_to_kernel_api.txt     |  195 ++++++++++++++++++++
 1 files changed, 195 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/watchdog/convert_drivers_to_kernel_api.txt

diff --git a/Documentation/watchdog/convert_drivers_to_kernel_api.txt b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
new file mode 100644
index 0000000..9c0cff9
--- /dev/null
+++ b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
@@ -0,0 +1,195 @@
+Converting old watchdog drivers to the watchdog framework
+by Wolfram Sang <w.sang@pengutronix.de>
+=========================================================
+
+Before the watchdog framework came into the kernel, every driver had to
+implement the API on its own. Now, as the framework factored out the common
+components, those drivers can be lightened making it a user of the framework.
+This document shall guide you for this task. The necessary steps are described
+as well as things to look out for.
+
+
+Remove the file_operations struct
+---------------------------------
+
+Old drivers define their own file_operations for actions like open(), write(),
+etc... These are now handled by the framework and just call the driver when
+needed. So, in general, the file operations-struct and assorted functions can
+go. Only very few driver-specific details have to be moved to other functions.
+Here is a overview of the functions and probably needed actions:
+
+- open: Everything dealing with resource management (file-open checks, magic
+  close preparations) can simply go. Device specific stuff needs to go to the
+  driver specific start-function. Note that for some drivers, the start-function
+  also serves as the ping-function. If that is the case and you need start/stop
+  to be balanced (clocks!), you are better off refactoring a separate start-function.
+
+- close: Same hints as for open apply.
+
+- write: Can simply go, all defined behaviour is taken care of by the framework,
+  i.e. ping on write and magic char ('V') handling.
+
+- ioctl: While the driver is allowed to have extensions to the IOCTL interface,
+  the most common ones are handled by the framework, supported by some assistance
+  from the driver:
+
+	WDIOC_GETSUPPORT:
+		Returns the mandatory watchdog_info struct from the driver
+
+	WDIOC_GETSTATUS:
+		Needs the status-callback defined, otherwise returns 0
+
+	WDIOC_GETBOOTSTATUS:
+		Needs the bootstatus member properly set. Make sure it is 0 if you
+		don't have further support!
+
+	WDIOC_SETOPTIONS:
+		No preparations needed
+
+	WDIOC_KEEPALIVE:
+		If wanted, options in watchdog_info need to have WDIOF_KEEPALIVEPING
+		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.
+
+	WDIOC_GETTIMEOUT:
+		No preparations needed
+
+  Other IOCTLs can be served using the ioctl-callback. Note that this is mainly
+  intended for porting old drivers, new drivers should not invent private IOCTLs.
+  Private IOCTLs are processed first. When the callback returns with
+  -ENOIOCTLCMD, the IOCTLs of the framework will be tried, too. Any other error
+  is directly given to the user.
+
+Example conversion:
+
+-static const struct file_operations s3c2410wdt_fops = {
+-       .owner          = THIS_MODULE,
+-       .llseek         = no_llseek,
+-       .write          = s3c2410wdt_write,
+-       .unlocked_ioctl = s3c2410wdt_ioctl,
+-       .open           = s3c2410wdt_open,
+-       .release        = s3c2410wdt_release,
+-};
+
+Check the functions for device-specific stuff and keep it for later
+refactoring. The rest can go.
+
+
+Remove the miscdevice
+---------------------
+
+Since the file_operations are gone now, you can also remove the 'struct
+miscdevice'. The framework will create it on watchdog_dev_register() called by
+watchdog_register_device().
+
+-static struct miscdevice s3c2410wdt_miscdev = {
+-       .minor          = WATCHDOG_MINOR,
+-       .name           = "watchdog",
+-       .fops           = &s3c2410wdt_fops,
+-};
+
+
+Remove obsolete includes and defines
+------------------------------------
+
+Because of the simplifications, a few defines are probably unused now. Remove
+them. Includes can be removed, too. For example:
+
+- #include <linux/fs.h>
+- #include <linux/miscdevice.h> (if MODULE_ALIAS_MISCDEV is not used)
+- #include <linux/uaccess.h> (if no custom IOCTLs are used)
+
+
+Add the watchdog operations
+---------------------------
+
+All possible callbacks are defined in 'struct watchdog_ops'. You can find it
+explained in 'watchdog-kernel-api.txt' in this directory. start(), stop() and
+owner must be set, the rest is optional. You will easily find corresponding
+functions in the old driver. Note that you will now get a pointer to the
+watchdog_device as a parameter to these functions, so you probably have to
+change the function header. Other changes are most likely not needed, because
+here simply happens the direct hardware access. If you have device-specific
+code left from the above steps, it should be refactored into these callbacks.
+
+Here is a simple example:
+
++static struct watchdog_ops s3c2410wdt_ops = {
++       .owner = THIS_MODULE,
++       .start = s3c2410wdt_start,
++       .stop = s3c2410wdt_stop,
++       .ping = s3c2410wdt_keepalive,
++       .set_timeout = s3c2410wdt_set_heartbeat,
++};
+
+A typical function-header change looks like:
+
+-static void s3c2410wdt_keepalive(void)
++static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
+ {
+...
++
++       return 0;
+ }
+
+...
+
+-       s3c2410wdt_keepalive();
++       s3c2410wdt_keepalive(&s3c2410_wdd);
+
+
+Add the watchdog device
+-----------------------
+
+Now we need to create a 'struct watchdog_device' and populate it with the
+necessary information for the framework. The struct is also explained in detail
+in 'watchdog-kernel-api.txt' in this directory. We pass it the mandatory
+watchdog_info struct and the newly created watchdog_ops. Often, old drivers
+have their own record-keeping for things like bootstatus and timeout using
+static variables. Those have to be converted to use the members in
+watchdog_device. Note that the timeout values are unsigned int. Some drivers
+use signed int, so this has to be converted, too.
+
+Here is a simple example for a watchdog device:
+
++static struct watchdog_device s3c2410_wdd = {
++       .info = &s3c2410_wdt_ident,
++       .ops = &s3c2410wdt_ops,
++};
+
+
+Register the watchdog device
+----------------------------
+
+Replace misc_register(&miscdev) with watchdog_register_device(&watchdog_dev).
+Make sure the return value gets checked and the error message, if present,
+still fits. Also convert the unregister case.
+
+-       ret = misc_register(&s3c2410wdt_miscdev);
++       ret = watchdog_register_device(&s3c2410_wdd);
+
+...
+
+-       misc_deregister(&s3c2410wdt_miscdev);
++       watchdog_unregister_device(&s3c2410_wdd);
+
+
+Update the Kconfig-entry
+------------------------
+
+The entry for the driver now needs to select WATCHDOG_CORE:
+
++       select WATCHDOG_CORE
+
+
+Create a patch and send it to upstream
+--------------------------------------
+
+Make sure you understood Documentation/SubmittingPatches and send your patch to
+linux-watchdog@vger.kernel.org. We are looking forward to it :)
+
-- 
1.7.2.5

Re: [RFC 1/6] Documentation: watchdog: add guide how to convert drivers to new framework

[Randy Dunlap]

On Wed, 13 Jul 2011 22:26:01 +0200 Wolfram Sang wrote:

> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  .../watchdog/convert_drivers_to_kernel_api.txt     |  195 ++++++++++++++++++++
>  1 files changed, 195 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/watchdog/convert_drivers_to_kernel_api.txt
> 
> diff --git a/Documentation/watchdog/convert_drivers_to_kernel_api.txt b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
> new file mode 100644
> index 0000000..9c0cff9
> --- /dev/null
> +++ b/Documentation/watchdog/convert_drivers_to_kernel_api.txt
> @@ -0,0 +1,195 @@
> +Converting old watchdog drivers to the watchdog framework
> +by Wolfram Sang <w.sang@pengutronix.de>
> +=========================================================
> +
> +Before the watchdog framework came into the kernel, every driver had to
> +implement the API on its own. Now, as the framework factored out the common
> +components, those drivers can be lightened making it a user of the framework.
> +This document shall guide you for this task. The necessary steps are described
> +as well as things to look out for.
> +
> +
> +Remove the file_operations struct
> +---------------------------------
> +
> +Old drivers define their own file_operations for actions like open(), write(),
> +etc... These are now handled by the framework and just call the driver when
> +needed. So, in general, the file operations-struct and assorted functions can

                               file operations struct

> +go. Only very few driver-specific details have to be moved to other functions.
> +Here is a overview of the functions and probably needed actions:
> +
> +- open: Everything dealing with resource management (file-open checks, magic
> +  close preparations) can simply go. Device specific stuff needs to go to the
> +  driver specific start-function. Note that for some drivers, the start-function
> +  also serves as the ping-function. If that is the case and you need start/stop
> +  to be balanced (clocks!), you are better off refactoring a separate start-function.
> +
> +- close: Same hints as for open apply.
> +
> +- write: Can simply go, all defined behaviour is taken care of by the framework,
> +  i.e. ping on write and magic char ('V') handling.
> +
> +- ioctl: While the driver is allowed to have extensions to the IOCTL interface,
> +  the most common ones are handled by the framework, supported by some assistance
> +  from the driver:
> +
[snip]
> +
> +  Other IOCTLs can be served using the ioctl-callback. Note that this is mainly
> +  intended for porting old drivers, new drivers should not invent private IOCTLs.

                          old drivers; new drivers

> +  Private IOCTLs are processed first. When the callback returns with
> +  -ENOIOCTLCMD, the IOCTLs of the framework will be tried, too. Any other error
> +  is directly given to the user.
> +
> +Example conversion:
> +
> +-static const struct file_operations s3c2410wdt_fops = {
> +-       .owner          = THIS_MODULE,
> +-       .llseek         = no_llseek,
> +-       .write          = s3c2410wdt_write,
> +-       .unlocked_ioctl = s3c2410wdt_ioctl,
> +-       .open           = s3c2410wdt_open,
> +-       .release        = s3c2410wdt_release,
> +-};
> +
> +Check the functions for device-specific stuff and keep it for later
> +refactoring. The rest can go.
> +
> +
> +Remove the miscdevice
> +---------------------
[snip]

> +Remove obsolete includes and defines
> +------------------------------------
[snip]
> +
> +Add the watchdog operations
> +---------------------------
> +
> +All possible callbacks are defined in 'struct watchdog_ops'. You can find it
> +explained in 'watchdog-kernel-api.txt' in this directory. start(), stop() and
> +owner must be set, the rest is optional. You will easily find corresponding

I would say:          the rest are optional.

> +functions in the old driver. Note that you will now get a pointer to the
> +watchdog_device as a parameter to these functions, so you probably have to
> +change the function header. Other changes are most likely not needed, because
> +here simply happens the direct hardware access. If you have device-specific
> +code left from the above steps, it should be refactored into these callbacks.
> +
> +Here is a simple example:
[snip]

> +Add the watchdog device
> +-----------------------
[snip]

> +Register the watchdog device
> +----------------------------
[snip]

> +Update the Kconfig-entry
> +------------------------
> +
> +The entry for the driver now needs to select WATCHDOG_CORE:
> +
> ++       select WATCHDOG_CORE
> +
> +
> +Create a patch and send it to upstream
> +--------------------------------------
> +
> +Make sure you understood Documentation/SubmittingPatches and send your patch to
> +linux-watchdog@vger.kernel.org. We are looking forward to it :)

Good job.  Thanks.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

[RFC 2/6] watchdog: s3c2410: convert to use the watchdog framework

[Wolfram Sang]

Make this driver a user of the watchdog framework and remove now
centrally handled parts. Tested on a mini2440.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/watchdog/Kconfig       |    1 +
 drivers/watchdog/s3c2410_wdt.c |  176 +++++++++-------------------------------
 2 files changed, 38 insertions(+), 139 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index edb0f1b..4f1379c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -173,6 +173,7 @@ config HAVE_S3C2410_WATCHDOG
 config S3C2410_WATCHDOG
 	tristate "S3C2410 Watchdog"
 	depends on ARCH_S3C2410 || HAVE_S3C2410_WATCHDOG
+	select WATCHDOG_CORE
 	help
 	  Watchdog timer block in the Samsung SoCs. This will reboot
 	  the system when the timer expires with the watchdog enabled.
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 30da88f..5de7e4f 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -27,9 +27,8 @@
 #include <linux/moduleparam.h>
 #include <linux/types.h>
 #include <linux/timer.h>
-#include <linux/miscdevice.h>
+#include <linux/miscdevice.h> /* for MODULE_ALIAS_MISCDEV */
 #include <linux/watchdog.h>
-#include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
@@ -38,6 +37,7 @@
 #include <linux/io.h>
 #include <linux/cpufreq.h>
 #include <linux/slab.h>
+#include <linux/err.h>
 
 #include <mach/map.h>
 
@@ -74,14 +74,12 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, "
 			"0 to reboot (default 0)");
 MODULE_PARM_DESC(debug, "Watchdog debug, set to >1 for debug (default 0)");
 
-static unsigned long open_lock;
 static struct device    *wdt_dev;	/* platform device attached to */
 static struct resource	*wdt_mem;
 static struct resource	*wdt_irq;
 static struct clk	*wdt_clock;
 static void __iomem	*wdt_base;
 static unsigned int	 wdt_count;
-static char		 expect_close;
 static DEFINE_SPINLOCK(wdt_lock);
 
 /* watchdog control routines */
@@ -93,11 +91,13 @@ static DEFINE_SPINLOCK(wdt_lock);
 
 /* functions */
 
-static void s3c2410wdt_keepalive(void)
+static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
 {
 	spin_lock(&wdt_lock);
 	writel(wdt_count, wdt_base + S3C2410_WTCNT);
 	spin_unlock(&wdt_lock);
+
+	return 0;
 }
 
 static void __s3c2410wdt_stop(void)
@@ -109,14 +109,16 @@ static void __s3c2410wdt_stop(void)
 	writel(wtcon, wdt_base + S3C2410_WTCON);
 }
 
-static void s3c2410wdt_stop(void)
+static int s3c2410wdt_stop(struct watchdog_device *wdd)
 {
 	spin_lock(&wdt_lock);
 	__s3c2410wdt_stop();
 	spin_unlock(&wdt_lock);
+
+	return 0;
 }
 
-static void s3c2410wdt_start(void)
+static int s3c2410wdt_start(struct watchdog_device *wdd)
 {
 	unsigned long wtcon;
 
@@ -142,6 +144,8 @@ static void s3c2410wdt_start(void)
 	writel(wdt_count, wdt_base + S3C2410_WTCNT);
 	writel(wtcon, wdt_base + S3C2410_WTCON);
 	spin_unlock(&wdt_lock);
+
+	return 0;
 }
 
 static inline int s3c2410wdt_is_running(void)
@@ -149,7 +153,7 @@ static inline int s3c2410wdt_is_running(void)
 	return readl(wdt_base + S3C2410_WTCON) & S3C2410_WTCON_ENABLE;
 }
 
-static int s3c2410wdt_set_heartbeat(int timeout)
+static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, unsigned timeout)
 {
 	unsigned long freq = clk_get_rate(wdt_clock);
 	unsigned int count;
@@ -182,8 +186,6 @@ static int s3c2410wdt_set_heartbeat(int timeout)
 		}
 	}
 
-	tmr_margin = timeout;
-
 	DBG("%s: timeout=%d, divisor=%d, count=%d (%08x)\n",
 	    __func__, timeout, divisor, count, count/divisor);
 
@@ -201,70 +203,6 @@ static int s3c2410wdt_set_heartbeat(int timeout)
 	return 0;
 }
 
-/*
- *	/dev/watchdog handling
- */
-
-static int s3c2410wdt_open(struct inode *inode, struct file *file)
-{
-	if (test_and_set_bit(0, &open_lock))
-		return -EBUSY;
-
-	if (nowayout)
-		__module_get(THIS_MODULE);
-
-	expect_close = 0;
-
-	/* start the timer */
-	s3c2410wdt_start();
-	return nonseekable_open(inode, file);
-}
-
-static int s3c2410wdt_release(struct inode *inode, struct file *file)
-{
-	/*
-	 *	Shut off the timer.
-	 *	Lock it in if it's a module and we set nowayout
-	 */
-
-	if (expect_close == 42)
-		s3c2410wdt_stop();
-	else {
-		dev_err(wdt_dev, "Unexpected close, not stopping watchdog\n");
-		s3c2410wdt_keepalive();
-	}
-	expect_close = 0;
-	clear_bit(0, &open_lock);
-	return 0;
-}
-
-static ssize_t s3c2410wdt_write(struct file *file, const char __user *data,
-				size_t len, loff_t *ppos)
-{
-	/*
-	 *	Refresh the timer.
-	 */
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			/* In case it was set long ago */
-			expect_close = 0;
-
-			for (i = 0; i != len; i++) {
-				char c;
-
-				if (get_user(c, data + i))
-					return -EFAULT;
-				if (c == 'V')
-					expect_close = 42;
-			}
-		}
-		s3c2410wdt_keepalive();
-	}
-	return len;
-}
-
 #define OPTIONS (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
 
 static const struct watchdog_info s3c2410_wdt_ident = {
@@ -273,53 +211,17 @@ static const struct watchdog_info s3c2410_wdt_ident = {
 	.identity         =	"S3C2410 Watchdog",
 };
 
-
-static long s3c2410wdt_ioctl(struct file *file,	unsigned int cmd,
-							unsigned long arg)
-{
-	void __user *argp = (void __user *)arg;
-	int __user *p = argp;
-	int new_margin;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(argp, &s3c2410_wdt_ident,
-			sizeof(s3c2410_wdt_ident)) ? -EFAULT : 0;
-	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(0, p);
-	case WDIOC_KEEPALIVE:
-		s3c2410wdt_keepalive();
-		return 0;
-	case WDIOC_SETTIMEOUT:
-		if (get_user(new_margin, p))
-			return -EFAULT;
-		if (s3c2410wdt_set_heartbeat(new_margin))
-			return -EINVAL;
-		s3c2410wdt_keepalive();
-		return put_user(tmr_margin, p);
-	case WDIOC_GETTIMEOUT:
-		return put_user(tmr_margin, p);
-	default:
-		return -ENOTTY;
-	}
-}
-
-/* kernel interface */
-
-static const struct file_operations s3c2410wdt_fops = {
-	.owner		= THIS_MODULE,
-	.llseek		= no_llseek,
-	.write		= s3c2410wdt_write,
-	.unlocked_ioctl	= s3c2410wdt_ioctl,
-	.open		= s3c2410wdt_open,
-	.release	= s3c2410wdt_release,
+static struct watchdog_ops s3c2410wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = s3c2410wdt_start,
+	.stop = s3c2410wdt_stop,
+	.ping = s3c2410wdt_keepalive,
+	.set_timeout = s3c2410wdt_set_heartbeat,
 };
 
-static struct miscdevice s3c2410wdt_miscdev = {
-	.minor		= WATCHDOG_MINOR,
-	.name		= "watchdog",
-	.fops		= &s3c2410wdt_fops,
+static struct watchdog_device s3c2410_wdd = {
+	.info = &s3c2410_wdt_ident,
+	.ops = &s3c2410wdt_ops,
 };
 
 /* interrupt handler code */
@@ -328,7 +230,7 @@ static irqreturn_t s3c2410wdt_irq(int irqno, void *param)
 {
 	dev_info(wdt_dev, "watchdog timer expired (irq)\n");
 
-	s3c2410wdt_keepalive();
+	s3c2410wdt_keepalive(&s3c2410_wdd);
 	return IRQ_HANDLED;
 }
 
@@ -349,14 +251,14 @@ static int s3c2410wdt_cpufreq_transition(struct notifier_block *nb,
 		 * the watchdog is running.
 		 */
 
-		s3c2410wdt_keepalive();
+		s3c2410wdt_keepalive(&s3c2410_wdd);
 	} else if (val == CPUFREQ_POSTCHANGE) {
-		s3c2410wdt_stop();
+		s3c2410wdt_stop(&s3c2410_wdd);
 
-		ret = s3c2410wdt_set_heartbeat(tmr_margin);
+		ret = s3c2410wdt_set_heartbeat(&s3c2410_wdd, s3c2410_wdd.timeout);
 
 		if (ret >= 0)
-			s3c2410wdt_start();
+			s3c2410wdt_start(&s3c2410_wdd);
 		else
 			goto err;
 	}
@@ -365,7 +267,8 @@ done:
 	return 0;
 
  err:
-	dev_err(wdt_dev, "cannot set new value for timeout %d\n", tmr_margin);
+	dev_err(wdt_dev, "cannot set new value for timeout %d\n",
+				s3c2410_wdd.timeout);
 	return ret;
 }
 
@@ -396,10 +299,6 @@ static inline void s3c2410wdt_cpufreq_deregister(void)
 }
 #endif
 
-
-
-/* device interface */
-
 static int __devinit s3c2410wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev;
@@ -466,8 +365,8 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev)
 	/* see if we can actually set the requested timer margin, and if
 	 * not, try the default value */
 
-	if (s3c2410wdt_set_heartbeat(tmr_margin)) {
-		started = s3c2410wdt_set_heartbeat(
+	if (s3c2410wdt_set_heartbeat(&s3c2410_wdd, tmr_margin)) {
+		started = s3c2410wdt_set_heartbeat(&s3c2410_wdd,
 					CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME);
 
 		if (started == 0)
@@ -479,22 +378,21 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev)
 							"cannot start\n");
 	}
 
-	ret = misc_register(&s3c2410wdt_miscdev);
+	ret = watchdog_register_device(&s3c2410_wdd);
 	if (ret) {
-		dev_err(dev, "cannot register miscdev on minor=%d (%d)\n",
-			WATCHDOG_MINOR, ret);
+		dev_err(dev, "cannot register watchdog (%d)\n", ret);
 		goto err_cpufreq;
 	}
 
 	if (tmr_atboot && started == 0) {
 		dev_info(dev, "starting watchdog timer\n");
-		s3c2410wdt_start();
+		s3c2410wdt_start(&s3c2410_wdd);
 	} else if (!tmr_atboot) {
 		/* if we're not enabling the watchdog, then ensure it is
 		 * disabled if it has been left running from the bootloader
 		 * or other source */
 
-		s3c2410wdt_stop();
+		s3c2410wdt_stop(&s3c2410_wdd);
 	}
 
 	/* print out a statement of readiness */
@@ -530,7 +428,7 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev)
 
 static int __devexit s3c2410wdt_remove(struct platform_device *dev)
 {
-	misc_deregister(&s3c2410wdt_miscdev);
+	watchdog_unregister_device(&s3c2410_wdd);
 
 	s3c2410wdt_cpufreq_deregister();
 
@@ -550,7 +448,7 @@ static int __devexit s3c2410wdt_remove(struct platform_device *dev)
 
 static void s3c2410wdt_shutdown(struct platform_device *dev)
 {
-	s3c2410wdt_stop();
+	s3c2410wdt_stop(&s3c2410_wdd);
 }
 
 #ifdef CONFIG_PM
@@ -565,7 +463,7 @@ static int s3c2410wdt_suspend(struct platform_device *dev, pm_message_t state)
 	wtdat_save = readl(wdt_base + S3C2410_WTDAT);
 
 	/* Note that WTCNT doesn't need to be saved. */
-	s3c2410wdt_stop();
+	s3c2410wdt_stop(&s3c2410_wdd);
 
 	return 0;
 }
-- 
1.7.2.5

[RFC 3/6] 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 used 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 |   70 +++++++++++++++++++---------------------
 1 files changed, 33 insertions(+), 37 deletions(-)

diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 6149332..8e34691 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,62 @@ static struct miscdevice pnx4008_wdt_miscdev = {
 
 static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
 {
+	struct resource *r;
 	int ret = 0, size;
 
 	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");
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r) {
+		dev_err(&pdev->dev, "failed to get memory region resouce\n");
 		return -ENOENT;
 	}
 
-	size = resource_size(wdt_mem);
+	size = resource_size(r);
 
-	if (!request_mem_region(wdt_mem->start, size, pdev->name)) {
-		printk(KERN_INFO MODULE_NAME "failed to get memory region\n");
-		return -ENOENT;
+	if (!devm_request_mem_region(&pdev->dev, r->start, size, pdev->name)) {
+		dev_err(&pdev->dev, "failed to get memory region\n");
+		return -ENOMEM;
+	}
+
+	wdt_base = devm_ioremap_nocache(&pdev->dev, r->start, size);
+	if (!wdt_base) {
+		dev_err(&pdev->dev, "ioremap failed\n");
+		return -ENOMEM;
 	}
-	wdt_base = (void __iomem *)IO_ADDRESS(wdt_mem->start);
 
 	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;
 	}
 
 	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:
+	pnx4008_wdd.bootstatus = (__raw_readl(WDTIM_RES(wdt_base)) &
+		WDOG_RESET) ? WDIOF_CARDRESET : 0;
+	wdt_disable();		/*disable for now */
+	clk_disable(wdt_clk);
+
+	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);
+ out:
 	return ret;
 }
 
@@ -318,10 +318,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.2.5

[RFC 4/6] 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 |   23 +++++++++++------------
 1 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 8e34691..4cc9c27 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);
 }
@@ -293,8 +292,8 @@ static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
 		goto disable_clk;
 	}
 
-	pnx4008_wdd.bootstatus = (__raw_readl(WDTIM_RES(wdt_base)) &
-		WDOG_RESET) ? WDIOF_CARDRESET : 0;
+	pnx4008_wdd.bootstatus = (readl(WDTIM_RES(wdt_base)) & WDOG_RESET) ?
+				WDIOF_CARDRESET : 0;
 	wdt_disable();		/*disable for now */
 	clk_disable(wdt_clk);
 
-- 
1.7.2.5

[RFC 5/6] watchdog: pnx4008: convert driver to use the watchdog framework

[Wolfram Sang]

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

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

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4f1379c..4ef6ba9 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -236,6 +236,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 4cc9c27..1ccb49a 100644
--- a/drivers/watchdog/pnx4008_wdt.c
+++ b/drivers/watchdog/pnx4008_wdt.c
@@ -18,7 +18,6 @@
 #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>
@@ -32,6 +31,7 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <mach/hardware.h>
+#include <linux/err.h>
 
 #define MODULE_NAME "PNX4008-WDT: "
 
@@ -81,18 +81,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_ping(struct watchdog_device *wdd)
 {
 	spin_lock(&io_lock);
 
@@ -109,11 +102,13 @@ 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)
@@ -125,128 +120,53 @@ static void wdt_disable(void)
 	spin_unlock(&io_lock);
 }
 
-static int pnx4008_wdt_open(struct inode *inode, struct file *file)
+static int pnx4008_wdt_start(struct watchdog_device *wdd)
 {
 	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);
+	if (ret)
 		return ret;
-	}
 
-	wdt_enable();
+	pnx4008_wdt_ping(wdd);
 
-	return nonseekable_open(inode, file);
+	return 0;
 }
 
-static ssize_t pnx4008_wdt_write(struct file *file, const char *data,
-					size_t len, loff_t *ppos)
+static int pnx4008_wdt_stop(struct watchdog_device *wdd)
 {
-	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;
-}
-
-static const struct watchdog_info ident = {
-	.options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE |
-	    WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
-	.identity = "PNX4008 Watchdog",
-};
+	wdt_disable();
+	clk_disable(wdt_clk);
 
-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;
+	return 0;
 }
 
-static int pnx4008_wdt_release(struct inode *inode, struct file *file)
+static int pnx4008_set_timeout(struct watchdog_device *wdd, unsigned timeout)
 {
-	if (!test_bit(WDT_OK_TO_CLOSE, &wdt_status))
-		printk(KERN_WARNING "WATCHDOG: Device closed unexpectdly\n");
-
-	wdt_disable();
-	clk_disable(wdt_clk);
-	clear_bit(WDT_IN_USE, &wdt_status);
-	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
+	pnx4008_wdt_ping(wdd);
 
 	return 0;
 }
 
-static const struct file_operations pnx4008_wdt_fops = {
+static const struct watchdog_info pnx4008_wdt_ident = {
+	.options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE |
+	    WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = "PNX4008 Watchdog",
+};
+
+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,
+	.ping = pnx4008_wdt_ping,
+	.set_timeout = pnx4008_set_timeout,
 };
 
-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)
@@ -286,14 +206,16 @@ 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;
+
+	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;
 	}
 
-	pnx4008_wdd.bootstatus = (readl(WDTIM_RES(wdt_base)) & WDOG_RESET) ?
-				WDIOF_CARDRESET : 0;
 	wdt_disable();		/*disable for now */
 	clk_disable(wdt_clk);
 
@@ -312,7 +234,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);
-- 
1.7.2.5

[RFC 6/6] WIP watchdog: pnx4008: refactor disabling device

[Wolfram Sang]

wdt_disable and clk_disable must always be used in combination in stop()
to match the initialization in start(), so group them together. Also,
fix disabling clock in remove, which should have been done before.

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

diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 1ccb49a..9f049d7 100644
--- a/drivers/watchdog/pnx4008_wdt.c
+++ b/drivers/watchdog/pnx4008_wdt.c
@@ -111,19 +111,11 @@ static int pnx4008_wdt_ping(struct watchdog_device *wdd)
 	return 0;
 }
 
-static void wdt_disable(void)
-{
-	spin_lock(&io_lock);
-
-	writel(0, WDTIM_CTRL(wdt_base));	/*stop counter */
-
-	spin_unlock(&io_lock);
-}
-
 static int pnx4008_wdt_start(struct watchdog_device *wdd)
 {
 	int ret;
 
+	//FIXME: better in an open-callback?
 	ret = clk_enable(wdt_clk);
 	if (ret)
 		return ret;
@@ -135,7 +127,11 @@ static int pnx4008_wdt_start(struct watchdog_device *wdd)
 
 static int pnx4008_wdt_stop(struct watchdog_device *wdd)
 {
-	wdt_disable();
+	spin_lock(&io_lock);
+	writel(0, WDTIM_CTRL(wdt_base));
+	spin_unlock(&io_lock);
+
+	//FIXME: better in a close-callback?
 	clk_disable(wdt_clk);
 
 	return 0;
@@ -216,8 +212,8 @@ static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
 		goto disable_clk;
 	}
 
-	wdt_disable();		/*disable for now */
-	clk_disable(wdt_clk);
+	/* disable watchdog and its clock until opened */
+	pnx4008_wdt_stop(&pnx4008_wdd);
 
 	dev_info(&pdev->dev, "PNX4008 Watchdog Timer: heartbeat %d sec\n",
 			heartbeat);
@@ -235,8 +231,7 @@ static int __devinit pnx4008_wdt_probe(struct platform_device *pdev)
 static int __devexit pnx4008_wdt_remove(struct platform_device *pdev)
 {
 	watchdog_unregister_device(&pnx4008_wdd);
-
-	clk_disable(wdt_clk);
+	/* clk has been disabled on close. If not, we'll reboot anyhow */
 	clk_put(wdt_clk);
 
 	return 0;
-- 
1.7.2.5

RE: [RFC 0/6] watchdog drivers converted to the new framework

[H Hartley Sweeten]

On Wednesday, July 13, 2011 1:26 PM, Wolfram Sang wrote:
> As promised, here is a RFC with two examples demonstrating how watchdog drivers
> can be converted to use the new watchdog framework (using the current version
> Wim posted two days ago). There is also conversion guide put to the
> documentation folder. Being RFC, all this is not final yet, but presentable, I
> hope.
> 
> Although there are a few more consolidation options left, there is already a
> gain of ~100 lines per driver. Promising, but there are a few issues to be
> sorted out, too, yet nothing which can be dealt with.
> 
> I have two other drivers in the making (stmp3xxx and imx2), but they need some
> more preparation; the first one needs some internal cleanups (like a lot of
> watchdog drivers); the latter one needs an addition to the framework
> (installing a timer for non-stoppable devices). I will also prepare a new
> driver (mx1) to show how small new drivers can be now :) The aim for all these
> driver conversions is inclusion in Linux 3.2. I still hope we can get the basic
> framework into Linux 3.1.
> 
> Many thanks to CELF/LF for supporting this work and to Wim and Alan for making
> the framework!
> 
> Looking forward to comments,

What's the status on Wim's watchdog framework patches?  It would be nice to have
them re-posted and CC the linux-arm-kernel list.  Not everyone follows lkml
regularly.

I found Wim's patches on lkml and converted the ep93xx watchdog driver and get:

 drivers/watchdog/ep93xx_wdt.c |  174 +++++++++-------------------------------
 1 files changed, 39 insertions(+), 135 deletions(-)

Overall I like the results.  It also makes the driver a lot easier to follow.

I would also like to convert this driver into a proper platform_driver using
ioremap'ed addresses instead of the static mappings.  Converting the driver
to the new watchdog framework would make this a bit cleaner.

Regards,
Hartley

Re: [RFC 0/6] watchdog drivers converted to the new framework

[Wolfram Sang]

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


> I would also like to convert this driver into a proper platform_driver using
> ioremap'ed addresses instead of the static mappings.  Converting the driver
> to the new watchdog framework would make this a bit cleaner.

Try managed devices (devm_* and friends) for an extra cleanup bonus.

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

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

RE: [RFC 0/6] watchdog drivers converted to the new framework

[H Hartley Sweeten]

On Thursday, July 14, 2011 11:28 AM, Wolfram Sang wrote:
>> I would also like to convert this driver into a proper platform_driver using
>> ioremap'ed addresses instead of the static mappings.  Converting the driver
>> to the new watchdog framework would make this a bit cleaner.
>
> Try managed devices (devm_* and friends) for an extra cleanup bonus.

Already done, just waiting for the framework... ;-)

Regards,
Hartley

Re: [RFC 0/6] watchdog drivers converted to the new framework

[Wolfram Sang]

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

On Thu, Jul 14, 2011 at 01:42:02PM -0500, H Hartley Sweeten wrote:
> On Thursday, July 14, 2011 11:28 AM, Wolfram Sang wrote:
> >> I would also like to convert this driver into a proper platform_driver using
> >> ioremap'ed addresses instead of the static mappings.  Converting the driver
> >> to the new watchdog framework would make this a bit cleaner.
> >
> > Try managed devices (devm_* and friends) for an extra cleanup bonus.
> 
> Already done, just waiting for the framework... ;-)

Nice :) Did you use my conversion-guide as a reference? Any comments if so?

Regards,

   Wolfram

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

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

RE: [RFC 0/6] watchdog drivers converted to the new framework

[H Hartley Sweeten]

On Thursday, July 14, 2011 1:01 PM, Wolfram Sang wrote:
> On Thu, Jul 14, 2011 at 01:42:02PM -0500, H Hartley Sweeten wrote:
>> On Thursday, July 14, 2011 11:28 AM, Wolfram Sang wrote:
>>>> I would also like to convert this driver into a proper platform_driver using
>>>> ioremap'ed addresses instead of the static mappings.  Converting the driver
>>>> to the new watchdog framework would make this a bit cleaner.
>>>
>>> Try managed devices (devm_* and friends) for an extra cleanup bonus.
>> 
>> Already done, just waiting for the framework... ;-)
>
> Nice :) Did you use my conversion-guide as a reference? Any comments if so?

Sorry, I didn't.  I just worked it out from the framework core and looking
at your examples.

When I found the framework patches on lkml I couldn't get them into a state
to apply to my tree.  I ended up manually applying all the pieces so I skipped
the Documentation stuff to save some time.

The conversion is pretty straight forward after examining the watchdog
framework files.

Regards,
Hartley

Re: [RFC 0/6] watchdog drivers converted to the new framework

[Wim Van Sebroeck]

Hi Hartley,

> >>>> I would also like to convert this driver into a proper platform_driver using
> >>>> ioremap'ed addresses instead of the static mappings.  Converting the driver
> >>>> to the new watchdog framework would make this a bit cleaner.
> >>>
> >>> Try managed devices (devm_* and friends) for an extra cleanup bonus.
> >> 
> >> Already done, just waiting for the framework... ;-)
> >
> > Nice :) Did you use my conversion-guide as a reference? Any comments if so?
> 
> Sorry, I didn't.  I just worked it out from the framework core and looking
> at your examples.
> 
> When I found the framework patches on lkml I couldn't get them into a state
> to apply to my tree.  I ended up manually applying all the pieces so I skipped
> the Documentation stuff to save some time.
> 
> The conversion is pretty straight forward after examining the watchdog
> framework files.

Maybe you could allready post your patches to the linux watchdog mailing list.

Kind regards,
Wim.