[PATCH 00/15] watchdog/mpcore: updates & fixes

[PATCH 00/15] watchdog/mpcore: updates & fixes

[Viresh Kumar]

Hello,

This patchset contains fixes and updates for ARM mpcore watchdog driver.
Changes are mentioned in following patch list:

Viresh Kumar (15):
  watchdog/mpcore_wdt: Fix multiline comments
  watchdog/mpcore_wdt: Add blank line after variable definitions in
    routines
  watchdog/mpcore_wdt: Arrange #includes in alphabetical order
  watchdog/mpcore_wdt: remove multiple 'ret = 0' statements from ioctl
    ops
  watchdog/mpcore_wdt: Rename dev to pdev for pointing to struct
    platform_device
  watchdog/mpcore_wdt: do request_irq before registering misc device
  watchdog/mpcore_wdt: Use devm routines
  watchdog/mpcore_wdt: handle module param mpcore_margin in probe
  watchdog/mpcore_wdt: convert to use module_platform_driver()
  watchdog/mpcore_wdt: Add support for dev_pm_ops interface
  watchdog/mpcore_wdt: disable wdt in suspend only if it is busy
  watchdog/mpcore_wdt: Add support for WDIOF_CARDRESET
  watchdog/mpcore_wdt: Allow platform_get_irq() to fail
  watchdog/mpcore_wdt: Add clock framework support
  watchdog/mpcore_wdt: use correct clk_rate to program timeout

 arch/arm/include/asm/smp_twd.h |   12 ++
 drivers/watchdog/mpcore_wdt.c  |  409 ++++++++++++++++++++++++----------------
 2 files changed, 255 insertions(+), 166 deletions(-)

-- 
1.7.8.110.g4cb5d

[PATCH 01/15] watchdog/mpcore_wdt: Fix multiline comments

[Viresh Kumar]

Following kind of multiline comments are fixed in this patch:
 - Add single line comment instead of multiline one, wherever possible
 - In case of few multiline comment a 'tab' is present instead of 'space' after
   the '*' in the second column. Replace tab with space here.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |   61 +++++++++++++++++------------------------
 1 files changed, 25 insertions(+), 36 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 82ccd36..64041ac 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -1,23 +1,21 @@
 /*
- *	Watchdog driver for the mpcore watchdog timer
+ * Watchdog driver for the mpcore watchdog timer
  *
- *	(c) Copyright 2004 ARM Limited
+ * (c) Copyright 2004 ARM Limited
  *
- *	Based on the SoftDog driver:
- *	(c) Copyright 1996 Alan Cox <alan@lxorguk.ukuu.org.uk>,
- *						All Rights Reserved.
+ * Based on the SoftDog driver:
+ * (c) Copyright 1996 Alan Cox <alan@lxorguk.ukuu.org.uk>, All Rights Reserved.
  *
- *	This program is free software; you can redistribute it and/or
- *	modify it under the terms of the GNU General Public License
- *	as published by the Free Software Foundation; either version
- *	2 of the License, or (at your option) any later version.
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
  *
- *	Neither Alan Cox nor CymruNet Ltd. admit liability nor provide
- *	warranty for any of this software. This material is provided
- *	"AS-IS" and at no charge.
- *
- *	(c) Copyright 1995    Alan Cox <alan@lxorguk.ukuu.org.uk>
+ * Neither Alan Cox nor CymruNet Ltd. admit liability nor provide
+ * warranty for any of this software. This material is provided
+ * "AS-IS" and at no charge.
  *
+ * (c) Copyright 1995    Alan Cox <alan@lxorguk.ukuu.org.uk>
  */
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -68,8 +66,8 @@ MODULE_PARM_DESC(mpcore_noboot, "MPcore watchdog action, "
 					__MODULE_STRING(ONLY_TESTING) ")");
 
 /*
- *	This is the interrupt handler.  Note that we only use this
- *	in testing mode, so don't actually do a reboot here.
+ * This is the interrupt handler.  Note that we only use this
+ * in testing mode, so don't actually do a reboot here.
  */
 static irqreturn_t mpcore_wdt_fire(int irq, void *arg)
 {
@@ -87,11 +85,11 @@ static irqreturn_t mpcore_wdt_fire(int irq, void *arg)
 }
 
 /*
- *	mpcore_wdt_keepalive - reload the timer
+ * mpcore_wdt_keepalive - reload the timer
  *
- *	Note that the spec says a DIFFERENT value must be written to the reload
- *	register each time.  The "perturb" variable deals with this by adding 1
- *	to the count every other time the function is called.
+ * Note that the spec says a DIFFERENT value must be written to the reload
+ * register each time.  The "perturb" variable deals with this by adding 1 to
+ * the count every other time the function is called.
  */
 static void mpcore_wdt_keepalive(struct mpcore_wdt *wdt)
 {
@@ -143,9 +141,7 @@ static int mpcore_wdt_set_heartbeat(int t)
 	return 0;
 }
 
-/*
- *	/dev/watchdog handling
- */
+/* /dev/watchdog handling */
 static int mpcore_wdt_open(struct inode *inode, struct file *file)
 {
 	struct mpcore_wdt *wdt = platform_get_drvdata(mpcore_wdt_dev);
@@ -158,9 +154,7 @@ static int mpcore_wdt_open(struct inode *inode, struct file *file)
 
 	file->private_data = wdt;
 
-	/*
-	 *	Activate timer
-	 */
+	/* Activate timer */
 	mpcore_wdt_start(wdt);
 
 	return nonseekable_open(inode, file);
@@ -171,8 +165,7 @@ static int mpcore_wdt_release(struct inode *inode, struct file *file)
 	struct mpcore_wdt *wdt = file->private_data;
 
 	/*
-	 *	Shut off the timer.
-	 *	Lock it in if it's a module and we set nowayout
+	 * Shut off the timer. Lock it in if it's a module and we set nowayout
 	 */
 	if (wdt->expect_close == 42)
 		mpcore_wdt_stop(wdt);
@@ -191,9 +184,7 @@ static ssize_t mpcore_wdt_write(struct file *file, const char *data,
 {
 	struct mpcore_wdt *wdt = file->private_data;
 
-	/*
-	 *	Refresh the timer.
-	 */
+	/* Refresh the timer. */
 	if (len) {
 		if (!nowayout) {
 			size_t i;
@@ -295,8 +286,8 @@ static long mpcore_wdt_ioctl(struct file *file, unsigned int cmd,
 }
 
 /*
- *	System shutdown handler.  Turn off the watchdog if we're
- *	restarting or halting the system.
+ * System shutdown handler.  Turn off the watchdog if we're restarting or
+ * halting the system.
  */
 static void mpcore_wdt_shutdown(struct platform_device *dev)
 {
@@ -306,9 +297,7 @@ static void mpcore_wdt_shutdown(struct platform_device *dev)
 		mpcore_wdt_stop(wdt);
 }
 
-/*
- *	Kernel Interfaces
- */
+/* Kernel Interfaces */
 static const struct file_operations mpcore_wdt_fops = {
 	.owner		= THIS_MODULE,
 	.llseek		= no_llseek,
-- 
1.7.8.110.g4cb5d

[PATCH 02/15] watchdog/mpcore_wdt: Add blank line after variable definitions in routines

[Viresh Kumar]

This patch adds blank line between local variable definitions and normal code
inside routine to make code more clean.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 64041ac..25f6e4a 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -399,6 +399,7 @@ static int __devexit mpcore_wdt_remove(struct platform_device *dev)
 static int mpcore_wdt_suspend(struct platform_device *dev, pm_message_t msg)
 {
 	struct mpcore_wdt *wdt = platform_get_drvdata(dev);
+
 	mpcore_wdt_stop(wdt);		/* Turn the WDT off */
 	return 0;
 }
@@ -406,6 +407,7 @@ static int mpcore_wdt_suspend(struct platform_device *dev, pm_message_t msg)
 static int mpcore_wdt_resume(struct platform_device *dev)
 {
 	struct mpcore_wdt *wdt = platform_get_drvdata(dev);
+
 	/* re-activate timer */
 	if (test_bit(0, &wdt->timer_alive))
 		mpcore_wdt_start(wdt);
-- 
1.7.8.110.g4cb5d

[PATCH 03/15] watchdog/mpcore_wdt: Arrange #includes in alphabetical order

[Viresh Kumar]

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 25f6e4a..daa6658 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -17,19 +17,19 @@
  *
  * (c) Copyright 1995    Alan Cox <alan@lxorguk.ukuu.org.uk>
  */
-#include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/types.h>
-#include <linux/miscdevice.h>
-#include <linux/watchdog.h>
 #include <linux/fs.h>
-#include <linux/reboot.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/platform_device.h>
-#include <linux/uaccess.h>
+#include <linux/reboot.h>
 #include <linux/slab.h>
-#include <linux/io.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
 
 #include <asm/smp_twd.h>
 
-- 
1.7.8.110.g4cb5d

[PATCH 04/15] watchdog/mpcore_wdt: remove multiple 'ret = 0' statements from ioctl ops

[Viresh Kumar]

mpcore_wdt_ioctl() has many lines where we do
	ret = 0;

Rewrite this routine to optimize these writes.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |   17 +++++------------
 1 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index daa6658..89e8fae 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -217,7 +217,7 @@ static long mpcore_wdt_ioctl(struct file *file, unsigned int cmd,
 							unsigned long arg)
 {
 	struct mpcore_wdt *wdt = file->private_data;
-	int ret;
+	int ret = 0;
 	union {
 		struct watchdog_info ident;
 		int i;
@@ -235,30 +235,24 @@ static long mpcore_wdt_ioctl(struct file *file, unsigned int cmd,
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
 		uarg.ident = ident;
-		ret = 0;
 		break;
 
 	case WDIOC_GETSTATUS:
 	case WDIOC_GETBOOTSTATUS:
 		uarg.i = 0;
-		ret = 0;
 		break;
 
 	case WDIOC_SETOPTIONS:
-		ret = -EINVAL;
-		if (uarg.i & WDIOS_DISABLECARD) {
+		if (uarg.i & WDIOS_DISABLECARD)
 			mpcore_wdt_stop(wdt);
-			ret = 0;
-		}
-		if (uarg.i & WDIOS_ENABLECARD) {
+		else if (uarg.i & WDIOS_ENABLECARD)
 			mpcore_wdt_start(wdt);
-			ret = 0;
-		}
+		else
+			ret = -EINVAL;
 		break;
 
 	case WDIOC_KEEPALIVE:
 		mpcore_wdt_keepalive(wdt);
-		ret = 0;
 		break;
 
 	case WDIOC_SETTIMEOUT:
@@ -270,7 +264,6 @@ static long mpcore_wdt_ioctl(struct file *file, unsigned int cmd,
 		/* Fall */
 	case WDIOC_GETTIMEOUT:
 		uarg.i = mpcore_margin;
-		ret = 0;
 		break;
 
 	default:
-- 
1.7.8.110.g4cb5d

[PATCH 05/15] watchdog/mpcore_wdt: Rename dev to pdev for pointing to struct platform_device

[Viresh Kumar]

Pointer to struct platform_device is named as dev, which makes it confusing when
we write statements like dev->dev to access struct device within it.

This patch renames such names to pdev.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |   40 ++++++++++++++++++++--------------------
 1 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 89e8fae..1e9c3d3 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -42,7 +42,7 @@ struct mpcore_wdt {
 	char		expect_close;
 };
 
-static struct platform_device *mpcore_wdt_dev;
+static struct platform_device *mpcore_wdt_pdev;
 static DEFINE_SPINLOCK(wdt_lock);
 
 #define TIMER_MARGIN	60
@@ -144,7 +144,7 @@ static int mpcore_wdt_set_heartbeat(int t)
 /* /dev/watchdog handling */
 static int mpcore_wdt_open(struct inode *inode, struct file *file)
 {
-	struct mpcore_wdt *wdt = platform_get_drvdata(mpcore_wdt_dev);
+	struct mpcore_wdt *wdt = platform_get_drvdata(mpcore_wdt_pdev);
 
 	if (test_and_set_bit(0, &wdt->timer_alive))
 		return -EBUSY;
@@ -282,9 +282,9 @@ static long mpcore_wdt_ioctl(struct file *file, unsigned int cmd,
  * System shutdown handler.  Turn off the watchdog if we're restarting or
  * halting the system.
  */
-static void mpcore_wdt_shutdown(struct platform_device *dev)
+static void mpcore_wdt_shutdown(struct platform_device *pdev)
 {
-	struct mpcore_wdt *wdt = platform_get_drvdata(dev);
+	struct mpcore_wdt *wdt = platform_get_drvdata(pdev);
 
 	if (system_state == SYSTEM_RESTART || system_state == SYSTEM_HALT)
 		mpcore_wdt_stop(wdt);
@@ -306,17 +306,17 @@ static struct miscdevice mpcore_wdt_miscdev = {
 	.fops		= &mpcore_wdt_fops,
 };
 
-static int __devinit mpcore_wdt_probe(struct platform_device *dev)
+static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 {
 	struct mpcore_wdt *wdt;
 	struct resource *res;
 	int ret;
 
 	/* We only accept one device, and it must have an id of -1 */
-	if (dev->id != -1)
+	if (pdev->id != -1)
 		return -ENODEV;
 
-	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		ret = -ENODEV;
 		goto err_out;
@@ -328,8 +328,8 @@ static int __devinit mpcore_wdt_probe(struct platform_device *dev)
 		goto err_out;
 	}
 
-	wdt->dev = &dev->dev;
-	wdt->irq = platform_get_irq(dev, 0);
+	wdt->dev = &pdev->dev;
+	wdt->irq = platform_get_irq(pdev, 0);
 	if (wdt->irq < 0) {
 		ret = -ENXIO;
 		goto err_free;
@@ -340,7 +340,7 @@ static int __devinit mpcore_wdt_probe(struct platform_device *dev)
 		goto err_free;
 	}
 
-	mpcore_wdt_miscdev.parent = &dev->dev;
+	mpcore_wdt_miscdev.parent = &pdev->dev;
 	ret = misc_register(&mpcore_wdt_miscdev);
 	if (ret) {
 		dev_printk(KERN_ERR, wdt->dev,
@@ -357,8 +357,8 @@ static int __devinit mpcore_wdt_probe(struct platform_device *dev)
 	}
 
 	mpcore_wdt_stop(wdt);
-	platform_set_drvdata(dev, wdt);
-	mpcore_wdt_dev = dev;
+	platform_set_drvdata(pdev, wdt);
+	mpcore_wdt_pdev = pdev;
 
 	return 0;
 
@@ -372,15 +372,15 @@ err_out:
 	return ret;
 }
 
-static int __devexit mpcore_wdt_remove(struct platform_device *dev)
+static int __devexit mpcore_wdt_remove(struct platform_device *pdev)
 {
-	struct mpcore_wdt *wdt = platform_get_drvdata(dev);
+	struct mpcore_wdt *wdt = platform_get_drvdata(pdev);
 
-	platform_set_drvdata(dev, NULL);
+	platform_set_drvdata(pdev, NULL);
 
 	misc_deregister(&mpcore_wdt_miscdev);
 
-	mpcore_wdt_dev = NULL;
+	mpcore_wdt_pdev = NULL;
 
 	free_irq(wdt->irq, wdt);
 	iounmap(wdt->base);
@@ -389,17 +389,17 @@ static int __devexit mpcore_wdt_remove(struct platform_device *dev)
 }
 
 #ifdef CONFIG_PM
-static int mpcore_wdt_suspend(struct platform_device *dev, pm_message_t msg)
+static int mpcore_wdt_suspend(struct platform_device *pdev, pm_message_t msg)
 {
-	struct mpcore_wdt *wdt = platform_get_drvdata(dev);
+	struct mpcore_wdt *wdt = platform_get_drvdata(pdev);
 
 	mpcore_wdt_stop(wdt);		/* Turn the WDT off */
 	return 0;
 }
 
-static int mpcore_wdt_resume(struct platform_device *dev)
+static int mpcore_wdt_resume(struct platform_device *pdev)
 {
-	struct mpcore_wdt *wdt = platform_get_drvdata(dev);
+	struct mpcore_wdt *wdt = platform_get_drvdata(pdev);
 
 	/* re-activate timer */
 	if (test_bit(0, &wdt->timer_alive))
-- 
1.7.8.110.g4cb5d

[PATCH 06/15] watchdog/mpcore_wdt: do request_irq before registering misc device

[Viresh Kumar]

Ideally, device/driver must be completely ready while we register it with any
framework. This includes allocating resources, registering irqs, etc. Currently
for mpcore_wdt irq is requested after it is registered as a misc device.

So, this patch moves request_irq before registration of misc device.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 1e9c3d3..db379f7 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -340,6 +340,13 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 		goto err_free;
 	}
 
+	ret = request_irq(wdt->irq, mpcore_wdt_fire, 0, "mpcore_wdt", wdt);
+	if (ret) {
+		dev_printk(KERN_ERR, wdt->dev,
+			"cannot register IRQ%d for watchdog\n", wdt->irq);
+		goto err_irq;
+	}
+
 	mpcore_wdt_miscdev.parent = &pdev->dev;
 	ret = misc_register(&mpcore_wdt_miscdev);
 	if (ret) {
@@ -349,22 +356,15 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 		goto err_misc;
 	}
 
-	ret = request_irq(wdt->irq, mpcore_wdt_fire, 0, "mpcore_wdt", wdt);
-	if (ret) {
-		dev_printk(KERN_ERR, wdt->dev,
-			"cannot register IRQ%d for watchdog\n", wdt->irq);
-		goto err_irq;
-	}
-
 	mpcore_wdt_stop(wdt);
 	platform_set_drvdata(pdev, wdt);
 	mpcore_wdt_pdev = pdev;
 
 	return 0;
 
-err_irq:
-	misc_deregister(&mpcore_wdt_miscdev);
 err_misc:
+	free_irq(wdt->irq, wdt);
+err_irq:
 	iounmap(wdt->base);
 err_free:
 	kfree(wdt);
-- 
1.7.8.110.g4cb5d

[PATCH 07/15] watchdog/mpcore_wdt: Use devm routines

[Viresh Kumar]

mpcore_wdt driver currently uses normal kzalloc, request_irq, ioremap, etc
routines. This patch replaces these routines with devm_kzalloc and
devm_request_mem_region etc, so that we don't need to handle freeing of
resources for error cases and module removal routine.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |   50 ++++++++++++----------------------------
 1 files changed, 15 insertions(+), 35 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index db379f7..209c81d 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -317,34 +317,28 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		ret = -ENODEV;
-		goto err_out;
-	}
+	if (!res)
+		return -ENODEV;
 
-	wdt = kzalloc(sizeof(struct mpcore_wdt), GFP_KERNEL);
-	if (!wdt) {
-		ret = -ENOMEM;
-		goto err_out;
-	}
+	wdt = devm_kzalloc(&pdev->dev, sizeof(struct mpcore_wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
 
 	wdt->dev = &pdev->dev;
 	wdt->irq = platform_get_irq(pdev, 0);
-	if (wdt->irq < 0) {
-		ret = -ENXIO;
-		goto err_free;
-	}
-	wdt->base = ioremap(res->start, resource_size(res));
-	if (!wdt->base) {
-		ret = -ENOMEM;
-		goto err_free;
-	}
+	if (wdt->irq < 0)
+		return -ENXIO;
 
-	ret = request_irq(wdt->irq, mpcore_wdt_fire, 0, "mpcore_wdt", wdt);
+	wdt->base = devm_ioremap(wdt->dev, res->start, resource_size(res));
+	if (!wdt->base)
+		return -ENOMEM;
+
+	ret = devm_request_irq(wdt->dev, wdt->irq, mpcore_wdt_fire, 0,
+			"mpcore_wdt", wdt);
 	if (ret) {
 		dev_printk(KERN_ERR, wdt->dev,
 			"cannot register IRQ%d for watchdog\n", wdt->irq);
-		goto err_irq;
+		return ret;
 	}
 
 	mpcore_wdt_miscdev.parent = &pdev->dev;
@@ -353,7 +347,7 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 		dev_printk(KERN_ERR, wdt->dev,
 			"cannot register miscdev on minor=%d (err=%d)\n",
 							WATCHDOG_MINOR, ret);
-		goto err_misc;
+		return ret;
 	}
 
 	mpcore_wdt_stop(wdt);
@@ -361,30 +355,16 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 	mpcore_wdt_pdev = pdev;
 
 	return 0;
-
-err_misc:
-	free_irq(wdt->irq, wdt);
-err_irq:
-	iounmap(wdt->base);
-err_free:
-	kfree(wdt);
-err_out:
-	return ret;
 }
 
 static int __devexit mpcore_wdt_remove(struct platform_device *pdev)
 {
-	struct mpcore_wdt *wdt = platform_get_drvdata(pdev);
-
 	platform_set_drvdata(pdev, NULL);
 
 	misc_deregister(&mpcore_wdt_miscdev);
 
 	mpcore_wdt_pdev = NULL;
 
-	free_irq(wdt->irq, wdt);
-	iounmap(wdt->base);
-	kfree(wdt);
 	return 0;
 }
 
-- 
1.7.8.110.g4cb5d

[PATCH 08/15] watchdog/mpcore_wdt: handle module param mpcore_margin in probe

[Viresh Kumar]

Currently mpcore_margin is handled in mpcore_wdt_init routine, which will be
called even if we don't have added any device for watchdog. Probably this is not
we want.

This patch moves this code into probe routine, so that it only gets hit once we
add device for this driver.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 209c81d..9c58c63 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -306,6 +306,9 @@ static struct miscdevice mpcore_wdt_miscdev = {
 	.fops		= &mpcore_wdt_fops,
 };
 
+static char banner[] __initdata = KERN_INFO "MPcore Watchdog Timer: 0.1. "
+		"mpcore_noboot=%d mpcore_margin=%d sec (nowayout= %d)\n";
+
 static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 {
 	struct mpcore_wdt *wdt;
@@ -341,6 +344,18 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/*
+	 * Check that the margin value is within it's range;
+	 * if not reset to the default
+	 */
+	if (mpcore_wdt_set_heartbeat(mpcore_margin)) {
+		mpcore_wdt_set_heartbeat(TIMER_MARGIN);
+		printk(KERN_INFO "mpcore_margin value must be 0 < mpcore_margin < 65536, using %d\n",
+			TIMER_MARGIN);
+	}
+
+	printk(banner, mpcore_noboot, mpcore_margin, nowayout);
+
 	mpcore_wdt_miscdev.parent = &pdev->dev;
 	ret = misc_register(&mpcore_wdt_miscdev);
 	if (ret) {
@@ -406,23 +421,8 @@ static struct platform_driver mpcore_wdt_driver = {
 	},
 };
 
-static char banner[] __initdata = KERN_INFO "MPcore Watchdog Timer: 0.1. "
-		"mpcore_noboot=%d mpcore_margin=%d sec (nowayout= %d)\n";
-
 static int __init mpcore_wdt_init(void)
 {
-	/*
-	 * Check that the margin value is within it's range;
-	 * if not reset to the default
-	 */
-	if (mpcore_wdt_set_heartbeat(mpcore_margin)) {
-		mpcore_wdt_set_heartbeat(TIMER_MARGIN);
-		printk(KERN_INFO "mpcore_margin value must be 0 < mpcore_margin < 65536, using %d\n",
-			TIMER_MARGIN);
-	}
-
-	printk(banner, mpcore_noboot, mpcore_margin, nowayout);
-
 	return platform_driver_register(&mpcore_wdt_driver);
 }
 
-- 
1.7.8.110.g4cb5d

[PATCH 09/15] watchdog/mpcore_wdt: convert to use module_platform_driver()

[Viresh Kumar]

This patch converts the mpcore_wdt driver to use the module_platform_driver()
macro which makes the code smaller and a bit simpler.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |   13 +------------
 1 files changed, 1 insertions(+), 12 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 9c58c63..e02c02b 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -421,18 +421,7 @@ static struct platform_driver mpcore_wdt_driver = {
 	},
 };
 
-static int __init mpcore_wdt_init(void)
-{
-	return platform_driver_register(&mpcore_wdt_driver);
-}
-
-static void __exit mpcore_wdt_exit(void)
-{
-	platform_driver_unregister(&mpcore_wdt_driver);
-}
-
-module_init(mpcore_wdt_init);
-module_exit(mpcore_wdt_exit);
+module_platform_driver(mpcore_wdt_driver);
 
 MODULE_AUTHOR("ARM Limited");
 MODULE_DESCRIPTION("MPcore Watchdog Device Driver");
-- 
1.7.8.110.g4cb5d

[PATCH 10/15] watchdog/mpcore_wdt: Add support for dev_pm_ops interface

[Viresh Kumar]

This patch adds dev_pm_ops support for standby/slepp/hibernate.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index e02c02b..a099418 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -25,6 +25,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/platform_device.h>
+#include <linux/pm.h>
 #include <linux/reboot.h>
 #include <linux/slab.h>
 #include <linux/types.h>
@@ -384,40 +385,39 @@ static int __devexit mpcore_wdt_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-static int mpcore_wdt_suspend(struct platform_device *pdev, pm_message_t msg)
+static int mpcore_wdt_suspend(struct device *dev)
 {
-	struct mpcore_wdt *wdt = platform_get_drvdata(pdev);
+	struct mpcore_wdt *wdt = dev_get_drvdata(dev);
 
 	mpcore_wdt_stop(wdt);		/* Turn the WDT off */
 	return 0;
 }
 
-static int mpcore_wdt_resume(struct platform_device *pdev)
+static int mpcore_wdt_resume(struct device *dev)
 {
-	struct mpcore_wdt *wdt = platform_get_drvdata(pdev);
+	struct mpcore_wdt *wdt = dev_get_drvdata(dev);
 
 	/* re-activate timer */
 	if (test_bit(0, &wdt->timer_alive))
 		mpcore_wdt_start(wdt);
 	return 0;
 }
-#else
-#define mpcore_wdt_suspend	NULL
-#define mpcore_wdt_resume	NULL
 #endif
 
+static SIMPLE_DEV_PM_OPS(mpcore_wdt_dev_pm_ops, mpcore_wdt_suspend,
+		mpcore_wdt_resume);
+
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:mpcore_wdt");
 
 static struct platform_driver mpcore_wdt_driver = {
 	.probe		= mpcore_wdt_probe,
 	.remove		= __devexit_p(mpcore_wdt_remove),
-	.suspend	= mpcore_wdt_suspend,
-	.resume		= mpcore_wdt_resume,
 	.shutdown	= mpcore_wdt_shutdown,
 	.driver		= {
 		.owner	= THIS_MODULE,
 		.name	= "mpcore_wdt",
+		.pm = &mpcore_wdt_dev_pm_ops,
 	},
 };
 
-- 
1.7.8.110.g4cb5d

[PATCH 10/15] watchdog/mpcore_wdt: Add support for dev_pm_ops interface

[Sergei Shtylyov]

Hello.

On 07-03-2012 14:27, Viresh Kumar wrote:

> This patch adds dev_pm_ops support for standby/slepp/hibernate.

> Signed-off-by: Viresh Kumar<viresh.kumar@st.com>
> ---
>   drivers/watchdog/mpcore_wdt.c |   18 +++++++++---------
>   1 files changed, 9 insertions(+), 9 deletions(-)

> diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
> index e02c02b..a099418 100644
> --- a/drivers/watchdog/mpcore_wdt.c
> +++ b/drivers/watchdog/mpcore_wdt.c
[...]
> @@ -384,40 +385,39 @@ static int __devexit mpcore_wdt_remove(struct platform_device *pdev)
[...]
>   /* work with hotplug and coldplug */
>   MODULE_ALIAS("platform:mpcore_wdt");
>
>   static struct platform_driver mpcore_wdt_driver = {
>   	.probe		= mpcore_wdt_probe,
>   	.remove		= __devexit_p(mpcore_wdt_remove),
> -	.suspend	= mpcore_wdt_suspend,
> -	.resume		= mpcore_wdt_resume,
>   	.shutdown	= mpcore_wdt_shutdown,
>   	.driver		= {
>   		.owner	= THIS_MODULE,
>   		.name	= "mpcore_wdt",
> +		.pm = &mpcore_wdt_dev_pm_ops,

    Please align the right part of initializer like all above.

WBR, Sergei

[PATCH 10/15] watchdog/mpcore_wdt: Add support for dev_pm_ops interface

[viresh kumar]

On 3/7/12, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> On 07-03-2012 14:27, Viresh Kumar wrote:

>>   static struct platform_driver mpcore_wdt_driver = {

>>   	.driver		= {
>>   		.owner	= THIS_MODULE,
>>   		.name	= "mpcore_wdt",
>> +		.pm = &mpcore_wdt_dev_pm_ops,
>
>     Please align the right part of initializer like all above.
>
> WBR, Sergei
>

Sure.

--
viresh

[PATCH 11/15] watchdog/mpcore_wdt: disable wdt in suspend only if it is busy

[Viresh Kumar]

We don't need to call mpcore_wdt_stop() in suspend, if wdt is not BUSY. So, call
mpcore_wdt_stop() conditionally in suspend.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index a099418..27a5069 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -389,7 +389,8 @@ static int mpcore_wdt_suspend(struct device *dev)
 {
 	struct mpcore_wdt *wdt = dev_get_drvdata(dev);
 
-	mpcore_wdt_stop(wdt);		/* Turn the WDT off */
+	if (test_bit(0, &wdt->timer_alive))
+		mpcore_wdt_stop(wdt);		/* Turn the WDT off */
 	return 0;
 }
 
-- 
1.7.8.110.g4cb5d

[PATCH 12/15] watchdog/mpcore_wdt: Add support for WDIOF_CARDRESET

[Viresh Kumar]

mpcore watchdog can give last boot status, whether we got a watchdog reset or
not, via bit 0 of TWD_WDOG_RESETSTAT register. This patch adds support to return
correct status if WDIOC_GETBOOTSTATUS cmd is called with ioctl.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 arch/arm/include/asm/smp_twd.h |    2 ++
 drivers/watchdog/mpcore_wdt.c  |   10 +++++++++-
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/smp_twd.h b/arch/arm/include/asm/smp_twd.h
index ef9ffba9..a13b536 100644
--- a/arch/arm/include/asm/smp_twd.h
+++ b/arch/arm/include/asm/smp_twd.h
@@ -18,6 +18,8 @@
 #define TWD_TIMER_CONTROL_PERIODIC	(1 << 1)
 #define TWD_TIMER_CONTROL_IT_ENABLE	(1 << 2)
 
+#define TWD_WDOG_RESETSTAT_MASK		0x1
+
 struct clock_event_device;
 
 extern void __iomem *twd_base;
diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 27a5069..6a04e31 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -36,6 +36,7 @@
 
 struct mpcore_wdt {
 	unsigned long	timer_alive;
+	unsigned long	boot_status;
 	struct device	*dev;
 	void __iomem	*base;
 	int		irq;
@@ -210,6 +211,7 @@ static ssize_t mpcore_wdt_write(struct file *file, const char *data,
 static const struct watchdog_info ident = {
 	.options		= WDIOF_SETTIMEOUT |
 				  WDIOF_KEEPALIVEPING |
+				  WDIOF_CARDRESET |
 				  WDIOF_MAGICCLOSE,
 	.identity		= "MPcore Watchdog",
 };
@@ -239,10 +241,13 @@ static long mpcore_wdt_ioctl(struct file *file, unsigned int cmd,
 		break;
 
 	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
 		uarg.i = 0;
 		break;
 
+	case WDIOC_GETBOOTSTATUS:
+		uarg.i = wdt->boot_status;
+		break;
+
 	case WDIOC_SETOPTIONS:
 		if (uarg.i & WDIOS_DISABLECARD)
 			mpcore_wdt_stop(wdt);
@@ -366,6 +371,9 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	wdt->boot_status = (readl(wdt->base + TWD_WDOG_RESETSTAT) &
+			TWD_WDOG_RESETSTAT_MASK) ? WDIOF_CARDRESET : 0;
+
 	mpcore_wdt_stop(wdt);
 	platform_set_drvdata(pdev, wdt);
 	mpcore_wdt_pdev = pdev;
-- 
1.7.8.110.g4cb5d

[PATCH 12/15] watchdog/mpcore_wdt: Add support for WDIOF_CARDRESET

[Sergei Shtylyov]

Hello.

On 07-03-2012 14:27, Viresh Kumar wrote:

> mpcore watchdog can give last boot status, whether we got a watchdog reset or
> not, via bit 0 of TWD_WDOG_RESETSTAT register. This patch adds support to return
> correct status if WDIOC_GETBOOTSTATUS cmd is called with ioctl.

    In the subject you say about another ioclt.

> Signed-off-by: Viresh Kumar<viresh.kumar@st.com>
> ---
>   arch/arm/include/asm/smp_twd.h |    2 ++
>   drivers/watchdog/mpcore_wdt.c  |   10 +++++++++-
>   2 files changed, 11 insertions(+), 1 deletions(-)

WBR, Sergei

[PATCH 12/15] watchdog/mpcore_wdt: Add support for WDIOF_CARDRESET

[viresh kumar]

On 3/7/12, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> On 07-03-2012 14:27, Viresh Kumar wrote:
>
>> mpcore watchdog can give last boot status, whether we got a watchdog reset
>> or
>> not, via bit 0 of TWD_WDOG_RESETSTAT register. This patch adds support to
>> return
>> correct status if WDIOC_GETBOOTSTATUS cmd is called with ioctl.
>
>     In the subject you say about another ioclt.
>

Actually i mentioned what we add in watchdog_info (WDIOF_CARDRESET) in subject,
and mentioned IOCTL in body. Both are right, but yes they must be
consistent. Will
fix subject in V2.

--
viresh

[PATCH 13/15] watchdog/mpcore_wdt: Allow platform_get_irq() to fail

[Viresh Kumar]

irq is not necessary for mpcore wdt. Don't return error if it is not passed. But
if it is passed, then request_irq must pass.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 6a04e31..ebd964a 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -334,20 +334,20 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	wdt->dev = &pdev->dev;
-	wdt->irq = platform_get_irq(pdev, 0);
-	if (wdt->irq < 0)
-		return -ENXIO;
 
 	wdt->base = devm_ioremap(wdt->dev, res->start, resource_size(res));
 	if (!wdt->base)
 		return -ENOMEM;
 
-	ret = devm_request_irq(wdt->dev, wdt->irq, mpcore_wdt_fire, 0,
-			"mpcore_wdt", wdt);
-	if (ret) {
-		dev_printk(KERN_ERR, wdt->dev,
-			"cannot register IRQ%d for watchdog\n", wdt->irq);
-		return ret;
+	wdt->irq = platform_get_irq(pdev, 0);
+	if (wdt->irq >= 0) {
+		ret = devm_request_irq(wdt->dev, wdt->irq, mpcore_wdt_fire, 0,
+				"mpcore_wdt", wdt);
+		if (ret) {
+			dev_printk(KERN_ERR, wdt->dev,
+					"cannot register IRQ%d for watchdog\n", wdt->irq);
+			return ret;
+		}
 	}
 
 	/*
-- 
1.7.8.110.g4cb5d

[PATCH 14/15] watchdog/mpcore_wdt: Add clock framework support

[Viresh Kumar]

This patch adds in clk framework support for wdt driver. If clk_get fails for
some platform, then we continue without clk_* API's.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/watchdog/mpcore_wdt.c |   71 ++++++++++++++++++++++++++++++++++++-----
 1 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index ebd964a..9747193 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -17,6 +17,7 @@
  *
  * (c) Copyright 1995    Alan Cox <alan@lxorguk.ukuu.org.uk>
  */
+#include <linux/clk.h>
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -39,6 +40,7 @@ struct mpcore_wdt {
 	unsigned long	boot_status;
 	struct device	*dev;
 	void __iomem	*base;
+	struct clk	*clk;
 	int		irq;
 	unsigned int	perturb;
 	char		expect_close;
@@ -147,6 +149,7 @@ static int mpcore_wdt_set_heartbeat(int t)
 static int mpcore_wdt_open(struct inode *inode, struct file *file)
 {
 	struct mpcore_wdt *wdt = platform_get_drvdata(mpcore_wdt_pdev);
+	int ret;
 
 	if (test_and_set_bit(0, &wdt->timer_alive))
 		return -EBUSY;
@@ -156,10 +159,22 @@ static int mpcore_wdt_open(struct inode *inode, struct file *file)
 
 	file->private_data = wdt;
 
+	if (wdt->clk) {
+		ret = clk_enable(wdt->clk);
+		if (ret) {
+			dev_err(wdt->dev, "clock enable fail");
+			goto err;
+		}
+	}
+
 	/* Activate timer */
 	mpcore_wdt_start(wdt);
 
 	return nonseekable_open(inode, file);
+
+err:
+	clear_bit(0, &wdt->timer_alive);
+	return ret;
 }
 
 static int mpcore_wdt_release(struct inode *inode, struct file *file)
@@ -169,9 +184,12 @@ static int mpcore_wdt_release(struct inode *inode, struct file *file)
 	/*
 	 * Shut off the timer. Lock it in if it's a module and we set nowayout
 	 */
-	if (wdt->expect_close == 42)
+	if (wdt->expect_close == 42) {
 		mpcore_wdt_stop(wdt);
-	else {
+
+		if (wdt->clk)
+			clk_disable(wdt->clk);
+	} else {
 		dev_printk(KERN_CRIT, wdt->dev,
 				"unexpected close, not stopping watchdog!\n");
 		mpcore_wdt_keepalive(wdt);
@@ -350,6 +368,28 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 		}
 	}
 
+	wdt->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(wdt->clk)) {
+		dev_warn(&pdev->dev, "Clock not found\n");
+		wdt->clk = NULL;
+	}
+
+	if (wdt->clk) {
+		ret = clk_enable(wdt->clk);
+		if (ret) {
+			dev_err(&pdev->dev, "Clock enable failed\n");
+			goto err_put_clk;
+		}
+	}
+
+	wdt->boot_status = (readl(wdt->base + TWD_WDOG_RESETSTAT) &
+			TWD_WDOG_RESETSTAT_MASK) ? WDIOF_CARDRESET : 0;
+
+	mpcore_wdt_stop(wdt);
+
+	if (wdt->clk)
+		clk_disable(wdt->clk);
+
 	/*
 	 * Check that the margin value is within it's range;
 	 * if not reset to the default
@@ -368,25 +408,32 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 		dev_printk(KERN_ERR, wdt->dev,
 			"cannot register miscdev on minor=%d (err=%d)\n",
 							WATCHDOG_MINOR, ret);
-		return ret;
+		goto err_put_clk;
 	}
 
-	wdt->boot_status = (readl(wdt->base + TWD_WDOG_RESETSTAT) &
-			TWD_WDOG_RESETSTAT_MASK) ? WDIOF_CARDRESET : 0;
-
-	mpcore_wdt_stop(wdt);
 	platform_set_drvdata(pdev, wdt);
 	mpcore_wdt_pdev = pdev;
 
 	return 0;
+
+err_put_clk:
+	if (wdt->clk)
+		clk_put(wdt->clk);
+
+	return ret;
 }
 
 static int __devexit mpcore_wdt_remove(struct platform_device *pdev)
 {
+	struct mpcore_wdt *wdt = platform_get_drvdata(pdev);
+
 	platform_set_drvdata(pdev, NULL);
 
 	misc_deregister(&mpcore_wdt_miscdev);
 
+	if (wdt->clk)
+		clk_put(wdt->clk);
+
 	mpcore_wdt_pdev = NULL;
 
 	return 0;
@@ -399,6 +446,9 @@ static int mpcore_wdt_suspend(struct device *dev)
 
 	if (test_bit(0, &wdt->timer_alive))
 		mpcore_wdt_stop(wdt);		/* Turn the WDT off */
+
+	if (wdt->clk)
+		clk_disable(wdt->clk);
 	return 0;
 }
 
@@ -407,8 +457,13 @@ static int mpcore_wdt_resume(struct device *dev)
 	struct mpcore_wdt *wdt = dev_get_drvdata(dev);
 
 	/* re-activate timer */
-	if (test_bit(0, &wdt->timer_alive))
+	if (test_bit(0, &wdt->timer_alive)) {
+		if (wdt->clk)
+			clk_enable(wdt->clk);
+
 		mpcore_wdt_start(wdt);
+	}
+
 	return 0;
 }
 #endif
-- 
1.7.8.110.g4cb5d

[PATCH 14/15] watchdog/mpcore_wdt: Add clock framework support

[Srinidhi Kasagar]

On Wed, Mar 07, 2012 at 11:27:55 +0100, Viresh KUMAR wrote:
> This patch adds in clk framework support for wdt driver. If clk_get fails for
> some platform, then we continue without clk_* API's.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  drivers/watchdog/mpcore_wdt.c |   71 ++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 63 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
> index ebd964a..9747193 100644
> --- a/drivers/watchdog/mpcore_wdt.c
> +++ b/drivers/watchdog/mpcore_wdt.c
> @@ -17,6 +17,7 @@
>   *
>   * (c) Copyright 1995    Alan Cox <alan@lxorguk.ukuu.org.uk>
>   */

[..]

> @@ -350,6 +368,28 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	wdt->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(wdt->clk)) {
> +		dev_warn(&pdev->dev, "Clock not found\n");
> +		wdt->clk = NULL;
> +	}
> +
> +	if (wdt->clk) {
> +		ret = clk_enable(wdt->clk);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Clock enable failed\n");
> +			goto err_put_clk;
> +		}
> +	}
> +
> +	wdt->boot_status = (readl(wdt->base + TWD_WDOG_RESETSTAT) &
> +			TWD_WDOG_RESETSTAT_MASK) ? WDIOF_CARDRESET : 0;
> +

I see that patch 12/15 does this, why are you doing it here again in clock patch?

> +	mpcore_wdt_stop(wdt);
> +
> +	if (wdt->clk)
> +		clk_disable(wdt->clk);
> +
>  	/*
>  	 * Check that the margin value is within it's range;
>  	 * if not reset to the default
> @@ -368,25 +408,32 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
>  		dev_printk(KERN_ERR, wdt->dev,
>  			"cannot register miscdev on minor=%d (err=%d)\n",
>  							WATCHDOG_MINOR, ret);
> -		return ret;
> +		goto err_put_clk;
>  	}
>  
> -	wdt->boot_status = (readl(wdt->base + TWD_WDOG_RESETSTAT) &
> -			TWD_WDOG_RESETSTAT_MASK) ? WDIOF_CARDRESET : 0;
> -

oh, you delete this stuff here again.

something messy in patch creation?

Srinidhi

[PATCH 14/15] watchdog/mpcore_wdt: Add clock framework support

[viresh kumar]

On 3/7/12, Srinidhi Kasagar <srinidhi.kasagar@stericsson.com> wrote:
> On Wed, Mar 07, 2012 at 11:27:55 +0100, Viresh KUMAR wrote:
>> +	wdt->boot_status = (readl(wdt->base + TWD_WDOG_RESETSTAT) &
>> +			TWD_WDOG_RESETSTAT_MASK) ? WDIOF_CARDRESET : 0;
>> +
>
> I see that patch 12/15 does this, why are you doing it here again in clock
> patch?
>

>>
>> -	wdt->boot_status = (readl(wdt->base + TWD_WDOG_RESETSTAT) &
>> -			TWD_WDOG_RESETSTAT_MASK) ? WDIOF_CARDRESET : 0;
>> -
>
> oh, you delete this stuff here again.
>
> something messy in patch creation?

Yes. It does look messy. But it was intentional to move this code up
in the function.
Will move clk patch before IOCTL addition patch to make it clean.

--
viresh

[PATCH 14/15] watchdog/mpcore_wdt: Add clock framework support

[Russell King - ARM Linux]

On Wed, Mar 07, 2012 at 03:57:55PM +0530, Viresh Kumar wrote:
> @@ -156,10 +159,22 @@ static int mpcore_wdt_open(struct inode *inode, struct file *file)
>  
>  	file->private_data = wdt;
>  
> +	if (wdt->clk) {

	if (!IS_ERR(wdt->clk)) {

> +		ret = clk_enable(wdt->clk);

What about preparing the clock?

> +		if (ret) {
> +			dev_err(wdt->dev, "clock enable fail");
> +			goto err;
> +		}
> +	}
> +
>  	/* Activate timer */
>  	mpcore_wdt_start(wdt);
>  
>  	return nonseekable_open(inode, file);
> +
> +err:
> +	clear_bit(0, &wdt->timer_alive);
> +	return ret;
>  }
>  
>  static int mpcore_wdt_release(struct inode *inode, struct file *file)
> @@ -169,9 +184,12 @@ static int mpcore_wdt_release(struct inode *inode, struct file *file)
>  	/*
>  	 * Shut off the timer. Lock it in if it's a module and we set nowayout
>  	 */
> -	if (wdt->expect_close == 42)
> +	if (wdt->expect_close == 42) {
>  		mpcore_wdt_stop(wdt);
> -	else {
> +
> +		if (wdt->clk)

		if (!IS_ERR(wdt->clk))

> +			clk_disable(wdt->clk);

What about unpreparing the clock?

> +	} else {
>  		dev_printk(KERN_CRIT, wdt->dev,
>  				"unexpected close, not stopping watchdog!\n");
>  		mpcore_wdt_keepalive(wdt);
> @@ -350,6 +368,28 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	wdt->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(wdt->clk)) {
> +		dev_warn(&pdev->dev, "Clock not found\n");
> +		wdt->clk = NULL;

Remove this line.

> +	}
> +
> +	if (wdt->clk) {

	if (!IS_ERR(wdt->clk)) {

> +		ret = clk_enable(wdt->clk);

What about preparing the clock?

> +		if (ret) {
> +			dev_err(&pdev->dev, "Clock enable failed\n");
> +			goto err_put_clk;
> +		}
> +	}
> +
> +	wdt->boot_status = (readl(wdt->base + TWD_WDOG_RESETSTAT) &
> +			TWD_WDOG_RESETSTAT_MASK) ? WDIOF_CARDRESET : 0;
> +
> +	mpcore_wdt_stop(wdt);
> +
> +	if (wdt->clk)
> +		clk_disable(wdt->clk);

Same two comments...

> +
>  	/*
>  	 * Check that the margin value is within it's range;
>  	 * if not reset to the default
> @@ -368,25 +408,32 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
>  		dev_printk(KERN_ERR, wdt->dev,
>  			"cannot register miscdev on minor=%d (err=%d)\n",
>  							WATCHDOG_MINOR, ret);
> -		return ret;
> +		goto err_put_clk;
>  	}
>  
> -	wdt->boot_status = (readl(wdt->base + TWD_WDOG_RESETSTAT) &
> -			TWD_WDOG_RESETSTAT_MASK) ? WDIOF_CARDRESET : 0;
> -
> -	mpcore_wdt_stop(wdt);
>  	platform_set_drvdata(pdev, wdt);
>  	mpcore_wdt_pdev = pdev;
>  
>  	return 0;
> +
> +err_put_clk:
> +	if (wdt->clk)
> +		clk_put(wdt->clk);
> +
> +	return ret;
>  }
>  
>  static int __devexit mpcore_wdt_remove(struct platform_device *pdev)
>  {
> +	struct mpcore_wdt *wdt = platform_get_drvdata(pdev);
> +
>  	platform_set_drvdata(pdev, NULL);
>  
>  	misc_deregister(&mpcore_wdt_miscdev);
>  
> +	if (wdt->clk)
> +		clk_put(wdt->clk);

Same comments.

> +
>  	mpcore_wdt_pdev = NULL;
>  
>  	return 0;
> @@ -399,6 +446,9 @@ static int mpcore_wdt_suspend(struct device *dev)
>  
>  	if (test_bit(0, &wdt->timer_alive))
>  		mpcore_wdt_stop(wdt);		/* Turn the WDT off */
> +
> +	if (wdt->clk)
> +		clk_disable(wdt->clk);

Same comments...  And what if the watchdog is not open?  Doesn't this
disable an already disabled clock?

>  	return 0;
>  }
>  
> @@ -407,8 +457,13 @@ static int mpcore_wdt_resume(struct device *dev)
>  	struct mpcore_wdt *wdt = dev_get_drvdata(dev);
>  
>  	/* re-activate timer */
> -	if (test_bit(0, &wdt->timer_alive))
> +	if (test_bit(0, &wdt->timer_alive)) {
> +		if (wdt->clk)
> +			clk_enable(wdt->clk);

Same two comments.

> +
>  		mpcore_wdt_start(wdt);
> +	}
> +
>  	return 0;
>  }
>  #endif
> -- 
> 1.7.8.110.g4cb5d
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 14/15] watchdog/mpcore_wdt: Add clock framework support

[viresh kumar]

On 3/7/12, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Wed, Mar 07, 2012 at 03:57:55PM +0530, Viresh Kumar wrote:
>> @@ -156,10 +159,22 @@ static int mpcore_wdt_open(struct inode *inode,
>> struct file *file)

>> +		ret = clk_enable(wdt->clk);
>
> What about preparing the clock?
>

I missed that. Will update prepare/unprepare of clk across driver.

>> +	wdt->clk = clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(wdt->clk)) {
>> +		dev_warn(&pdev->dev, "Clock not found\n");
>> +		wdt->clk = NULL;
>
> Remove this line.

Actually this was very much intentional, so that i can do
    if (wdt->clk).

Also, this makes more sense as some platform doesn't support clk,
so it is not an error for them if clk_get() fails. And thus the check

    if (wdt->clk)                /* i.e. clock is supported or not */

make more sense than

    if (!IS_ERR(wdt->clk))             /* i.e. there is an error while
getting clock */

from readability point of view. So, i would like to keep it as it is.

>> @@ -399,6 +446,9 @@ static int mpcore_wdt_suspend(struct device *dev)
>>
>>  	if (test_bit(0, &wdt->timer_alive))
>>  		mpcore_wdt_stop(wdt);		/* Turn the WDT off */
>> +
>> +	if (wdt->clk)
>> +		clk_disable(wdt->clk);
>
> Same comments...  And what if the watchdog is not open?  Doesn't this
> disable an already disabled clock?
>

Yes, that'a a BUG. Will fix it.

--
viresh

[PATCH 14/15] watchdog/mpcore_wdt: Add clock framework support

[Russell King - ARM Linux]

On Wed, Mar 07, 2012 at 04:20:58AM -0800, viresh kumar wrote:
> On 3/7/12, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > On Wed, Mar 07, 2012 at 03:57:55PM +0530, Viresh Kumar wrote:
> >> @@ -156,10 +159,22 @@ static int mpcore_wdt_open(struct inode *inode,
> >> struct file *file)
> 
> >> +		ret = clk_enable(wdt->clk);
> >
> > What about preparing the clock?
> >
> 
> I missed that. Will update prepare/unprepare of clk across driver.
> 
> >> +	wdt->clk = clk_get(&pdev->dev, NULL);
> >> +	if (IS_ERR(wdt->clk)) {
> >> +		dev_warn(&pdev->dev, "Clock not found\n");
> >> +		wdt->clk = NULL;
> >
> > Remove this line.
> 
> Actually this was very much intentional, so that i can do
>     if (wdt->clk).

Drivers have no business interpreting anything but an IS_ERR() return from
clk_get() as invalid.  Everything else is the CLK APIs business, not the
driver's business.  So, to start using NULL as a special case is wrong.

[PATCH 14/15] watchdog/mpcore_wdt: Add clock framework support

[viresh kumar]

On 3/7/12, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> Drivers have no business interpreting anything but an IS_ERR() return from
> clk_get() as invalid.  Everything else is the CLK APIs business, not the
> driver's business.  So, to start using NULL as a special case is wrong.

But driver isn't comparing return value of clk_get with NULL (that would have
been wrong) or interpreting its return value in a wrong way.

Instead it is using the same pointer/variable to check if clk_* APIs are
supported or not by making it NULL. And that's a driver convention.
I still believe it is fine.

Thanks for your quick comments.

--
viresh

[PATCH 14/15] watchdog/mpcore_wdt: Add clock framework support

[Russell King - ARM Linux]

On Wed, Mar 07, 2012 at 04:37:12AM -0800, viresh kumar wrote:
> On 3/7/12, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > Drivers have no business interpreting anything but an IS_ERR() return from
> > clk_get() as invalid.  Everything else is the CLK APIs business, not the
> > driver's business.  So, to start using NULL as a special case is wrong.
> 
> But driver isn't comparing return value of clk_get with NULL (that would have
> been wrong) or interpreting its return value in a wrong way.

What if clk_get() returned NULL?

Hint: as far as _you_ are concerned as a driver writer, a struct clk is
an opaque cookie.  IS_ERR(clk) means the cookie is invalid.  Everything
else is potentially valid.  You must not interpret anything which is
potentially valid as some kind of special case.

> Instead it is using the same pointer/variable to check if clk_* APIs are
> supported or not by making it NULL. And that's a driver convention.

Bollocks it is.

Plain and simple NAK to this patch until clue is forthcoming.

[PATCH 15/15] watchdog/mpcore_wdt: use correct clk_rate to program timeout

[Viresh Kumar]

Currently, mpcore wdt driver doesn't use exact clk_rate to program timeout in
hardware. This patch provides two ways of doing so:
	- either use clk_get_rate() if clk_get passes
	- use clk_rate passed via module param

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 arch/arm/include/asm/smp_twd.h |   10 ++++
 drivers/watchdog/mpcore_wdt.c  |  104 +++++++++++++++++++++++++++++++--------
 2 files changed, 92 insertions(+), 22 deletions(-)

diff --git a/arch/arm/include/asm/smp_twd.h b/arch/arm/include/asm/smp_twd.h
index a13b536..389ca02 100644
--- a/arch/arm/include/asm/smp_twd.h
+++ b/arch/arm/include/asm/smp_twd.h
@@ -13,11 +13,21 @@
 #define TWD_WDOG_RESETSTAT		0x30
 #define TWD_WDOG_DISABLE		0x34
 
+#define TWD_WDOG_LOAD_MIN		0x00000000
+#define TWD_WDOG_LOAD_MAX		0xFFFFFFFF
+
 #define TWD_TIMER_CONTROL_ENABLE	(1 << 0)
 #define TWD_TIMER_CONTROL_ONESHOT	(0 << 1)
 #define TWD_TIMER_CONTROL_PERIODIC	(1 << 1)
 #define TWD_TIMER_CONTROL_IT_ENABLE	(1 << 2)
 
+#define TWD_WDOG_CONTROL_ENABLE		(1 << 0)
+#define TWD_WDOG_CONTROL_IRQ_ENABLE	(1 << 2)
+#define TWD_WDOG_CONTROL_WDT_MODE	(1 << 3)
+#define TWD_WDOG_CONTROL_WDT_PRESCALE(x)	((x) << 8)
+#define TWD_WDOG_CONTROL_PRESCALE_MIN	0x00
+#define TWD_WDOG_CONTROL_PRESCALE_MAX	0xFF
+
 #define TWD_WDOG_RESETSTAT_MASK		0x1
 
 struct clock_event_device;
diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 9747193..fc14709 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -41,7 +41,10 @@ struct mpcore_wdt {
 	struct device	*dev;
 	void __iomem	*base;
 	struct clk	*clk;
+	unsigned long	clk_rate;	/* In Hz */
 	int		irq;
+	unsigned int	prescale;
+	unsigned int	load_val;
 	unsigned int	perturb;
 	char		expect_close;
 };
@@ -62,6 +65,11 @@ MODULE_PARM_DESC(nowayout,
 	"Watchdog cannot be stopped once started (default="
 				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
+static int clk_rate;
+module_param(clk_rate, int, 0);
+MODULE_PARM_DESC(clk_rate,
+	"Watchdog clock rate in Hz, required if clk_get() fails");
+
 #define ONLY_TESTING	0
 static int mpcore_noboot = ONLY_TESTING;
 module_param(mpcore_noboot, int, 0);
@@ -97,17 +105,9 @@ static irqreturn_t mpcore_wdt_fire(int irq, void *arg)
  */
 static void mpcore_wdt_keepalive(struct mpcore_wdt *wdt)
 {
-	unsigned long count;
-
 	spin_lock(&wdt_lock);
-	/* Assume prescale is set to 256 */
-	count =  __raw_readl(wdt->base + TWD_WDOG_COUNTER);
-	count = (0xFFFFFFFFU - count) * (HZ / 5);
-	count = (count / 256) * mpcore_margin;
-
-	/* Reload the counter */
-	writel(count + wdt->perturb, wdt->base + TWD_WDOG_LOAD);
-	wdt->perturb = wdt->perturb ? 0 : 1;
+	writel(wdt->load_val + wdt->perturb, wdt->base + TWD_WDOG_LOAD);
+	wdt->perturb = !wdt->perturb;
 	spin_unlock(&wdt_lock);
 }
 
@@ -122,26 +122,76 @@ static void mpcore_wdt_stop(struct mpcore_wdt *wdt)
 
 static void mpcore_wdt_start(struct mpcore_wdt *wdt)
 {
+	u32 val, mode;
+
 	dev_printk(KERN_INFO, wdt->dev, "enabling watchdog.\n");
 
 	/* This loads the count register but does NOT start the count yet */
 	mpcore_wdt_keepalive(wdt);
 
-	if (mpcore_noboot) {
-		/* Enable watchdog - prescale=256, watchdog mode=0, enable=1 */
-		writel(0x0000FF01, wdt->base + TWD_WDOG_CONTROL);
-	} else {
-		/* Enable watchdog - prescale=256, watchdog mode=1, enable=1 */
-		writel(0x0000FF09, wdt->base + TWD_WDOG_CONTROL);
+	if (mpcore_noboot)
+		mode = 0;
+	else
+		mode = TWD_WDOG_CONTROL_WDT_MODE;
+
+	spin_lock(&wdt_lock);
+
+	val = TWD_WDOG_CONTROL_WDT_PRESCALE(wdt->prescale) |
+		TWD_WDOG_CONTROL_ENABLE | mode;
+	writel(val, wdt->base + TWD_WDOG_CONTROL);
+
+	spin_unlock(&wdt_lock);
+}
+
+/* binary search */
+static inline void bsearch(u32 *var, u32 var_start, u32 var_end,
+		const u64 param, const u32 timeout, u32 rate)
+{
+	u64 tmp = 0;
+
+	/* get the lowest var value that can satisfy our requirement */
+	while (var_start < var_end) {
+		tmp = var_start;
+		tmp += var_end;
+		tmp = div_u64(tmp, 2);
+		if (timeout > div_u64((param + 1) * (tmp + 1), rate)) {
+			if (var_start == tmp)
+				break;
+			else
+				var_start = tmp;
+		} else {
+			if (var_end == tmp)
+				break;
+			else
+				var_end = tmp;
+		}
 	}
+	*var = tmp;
 }
 
-static int mpcore_wdt_set_heartbeat(int t)
+static int mpcore_wdt_set_heartbeat(struct mpcore_wdt *wdt, int timeout)
 {
-	if (t < 0x0001 || t > 0xFFFF)
+	unsigned int psc, rate = wdt->clk_rate;
+	u64 load = 0;
+
+	if (timeout < 0x0001 || timeout > 0xFFFF)
 		return -EINVAL;
 
-	mpcore_margin = t;
+	/* get appropriate value of psc and load */
+	bsearch(&psc, TWD_WDOG_CONTROL_PRESCALE_MIN,
+			TWD_WDOG_CONTROL_PRESCALE_MAX, TWD_WDOG_LOAD_MAX,
+			timeout, rate);
+	bsearch((u32 *)&load, TWD_WDOG_LOAD_MIN, TWD_WDOG_LOAD_MAX, psc,
+			timeout, rate);
+
+	spin_lock(&wdt_lock);
+	wdt->prescale = psc;
+	wdt->load_val = load;
+
+	/* roundup timeout to closest positive integer value */
+	mpcore_margin = div_u64((psc + 1) * (load + 1) + (rate / 2), rate);
+	spin_unlock(&wdt_lock);
+
 	return 0;
 }
 
@@ -280,7 +330,7 @@ static long mpcore_wdt_ioctl(struct file *file, unsigned int cmd,
 		break;
 
 	case WDIOC_SETTIMEOUT:
-		ret = mpcore_wdt_set_heartbeat(uarg.i);
+		ret = mpcore_wdt_set_heartbeat(wdt, uarg.i);
 		if (ret)
 			break;
 
@@ -372,6 +422,16 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 	if (IS_ERR(wdt->clk)) {
 		dev_warn(&pdev->dev, "Clock not found\n");
 		wdt->clk = NULL;
+
+		wdt->clk_rate = clk_rate;
+	} else {
+		wdt->clk_rate = clk_get_rate(wdt->clk);
+	}
+
+	if (!wdt->clk_rate) {
+		dev_err(&pdev->dev, "Clock rate can't be zero\n");
+		ret = -EINVAL;
+		goto err_put_clk;
 	}
 
 	if (wdt->clk) {
@@ -394,8 +454,8 @@ static int __devinit mpcore_wdt_probe(struct platform_device *pdev)
 	 * Check that the margin value is within it's range;
 	 * if not reset to the default
 	 */
-	if (mpcore_wdt_set_heartbeat(mpcore_margin)) {
-		mpcore_wdt_set_heartbeat(TIMER_MARGIN);
+	if (mpcore_wdt_set_heartbeat(wdt, mpcore_margin)) {
+		mpcore_wdt_set_heartbeat(wdt, TIMER_MARGIN);
 		printk(KERN_INFO "mpcore_margin value must be 0 < mpcore_margin < 65536, using %d\n",
 			TIMER_MARGIN);
 	}
-- 
1.7.8.110.g4cb5d

[PATCH 00/15] watchdog/mpcore: updates & fixes

[Wolfram Sang]

> This patchset contains fixes and updates for ARM mpcore watchdog driver.

It probably makes sense to convert this to use the watchdog-core. See
Documentation/watchdog/convert_drivers_to_kernel_api.txt for some
guidance.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120307/92387fc2/attachment-0001.sig>

[PATCH 00/15] watchdog/mpcore: updates & fixes

[viresh kumar]

On 3/7/12, Wolfram Sang <w.sang@pengutronix.de> wrote:
>
>> This patchset contains fixes and updates for ARM mpcore watchdog driver.
>
> It probably makes sense to convert this to use the watchdog-core. See
> Documentation/watchdog/convert_drivers_to_kernel_api.txt for some
> guidance.
>

Sorry for not checking that earlier.
Will align my patchset to that.

--
viresh

[PATCH 00/15] watchdog/mpcore: updates & fixes

[Wolfram Sang]

> > It probably makes sense to convert this to use the watchdog-core. See
> > Documentation/watchdog/convert_drivers_to_kernel_api.txt for some
> > guidance.
> >
> 
> Sorry for not checking that earlier.
> Will align my patchset to that.

Cool, thanks!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120307/b6d68136/attachment.sig>