Skip to contentSkip to conversation

watchdog/mpcore: updates & fixes

PATCH New Viresh Kumar 6 participants 30 messages Mar 7, 2012

Subsystems (2)
Arm (32-bit) Watchdog device drivers
lore
Share & tools
lore

Viresh Kumar

author
#0 lore raw

cover watchdog/mpcore: updates & fixes

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(-)
Signature · 1 line
1.7.8.110.g4cb5d
author
#1 lore raw

PATCH 1/15 watchdog/mpcore_wdt: Fix multiline comments

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 <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |   61 +++++++++++++++++------------------------
 1 files changed, 25 insertions(+), 36 deletions(-)
drivers/watchdog/mpcore_wdt.c +25 -36
index 82ccd36..64041ac 100644
@@ -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 <[email protected]>,
- *						All Rights Reserved.
+ * Based on the SoftDog driver:
+ * (c) Copyright 1996 Alan Cox <[email protected]>, 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 <[email protected]>
+ * 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 <[email protected]>
  */
 #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,
Signature · 1 line
1.7.8.110.g4cb5d
author
#2 lore raw

PATCH 2/15 watchdog/mpcore_wdt: Add blank line after variable definitions in routines

In reply to 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 <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/watchdog/mpcore_wdt.c +2 -0
index 64041ac..25f6e4a 100644
@@ -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);
Signature · 1 line
1.7.8.110.g4cb5d
author
#3 lore raw

PATCH 3/15 watchdog/mpcore_wdt: Arrange #includes in alphabetical order

In reply to Viresh Kumar
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)
drivers/watchdog/mpcore_wdt.c +8 -8
index 25f6e4a..daa6658 100644
@@ -17,19 +17,19 @@
  *
  * (c) Copyright 1995    Alan Cox <[email protected]>
  */
-#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>
Signature · 1 line
1.7.8.110.g4cb5d
author
#4 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |   17 +++++------------
 1 files changed, 5 insertions(+), 12 deletions(-)
drivers/watchdog/mpcore_wdt.c +5 -12
index daa6658..89e8fae 100644
@@ -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:
Signature · 1 line
1.7.8.110.g4cb5d
author
#5 lore raw

PATCH 5/15 watchdog/mpcore_wdt: Rename dev to pdev for pointing to struct platform_device

In reply to 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 <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |   40 ++++++++++++++++++++--------------------
 1 files changed, 20 insertions(+), 20 deletions(-)
drivers/watchdog/mpcore_wdt.c +20 -20
index 89e8fae..1e9c3d3 100644
@@ -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))
Signature · 1 line
1.7.8.110.g4cb5d
author
#6 lore raw

PATCH 6/15 watchdog/mpcore_wdt: do request_irq before registering misc device

In reply to 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 <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)
drivers/watchdog/mpcore_wdt.c +9 -9
index 1e9c3d3..db379f7 100644
@@ -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);
Signature · 1 line
1.7.8.110.g4cb5d
author
#7 lore raw

PATCH 7/15 watchdog/mpcore_wdt: Use devm routines

In reply to 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 <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |   50 ++++++++++++----------------------------
 1 files changed, 15 insertions(+), 35 deletions(-)
drivers/watchdog/mpcore_wdt.c +15 -35
index db379f7..209c81d 100644
@@ -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;
 }
Signature · 1 line
1.7.8.110.g4cb5d
author
#8 lore raw

PATCH 8/15 watchdog/mpcore_wdt: handle module param mpcore_margin in probe

In reply to 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 <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)
drivers/watchdog/mpcore_wdt.c +15 -15
index 209c81d..9c58c63 100644
@@ -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);
 }
Signature · 1 line
1.7.8.110.g4cb5d
author
#9 lore raw

PATCH 9/15 watchdog/mpcore_wdt: convert to use module_platform_driver()

In reply to 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 <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |   13 +------------
 1 files changed, 1 insertions(+), 12 deletions(-)
drivers/watchdog/mpcore_wdt.c +1 -12
index 9c58c63..e02c02b 100644
@@ -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");
Signature · 1 line
1.7.8.110.g4cb5d
author
#10 lore raw

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

In reply to Viresh Kumar

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

Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)
drivers/watchdog/mpcore_wdt.c +9 -9
index e02c02b..a099418 100644
@@ -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,
 	},
 };
Signature · 1 line
1.7.8.110.g4cb5d
#11 lore raw

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

Hello.

Quoted 11 lines · Viresh Kumar

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

Signed-off-by Viresh Kumar<[email protected]>
---
  drivers/watchdog/mpcore_wdt.c |   18 +++++++++---------
  1 files changed, 9 insertions(+), 9 deletions(-)
drivers/watchdog/mpcore_wdt.c +0 -0
index e02c02b..a099418 100644

[...]

Quoted 1 line
drivers/watchdog/mpcore_wdt.c +0 -0
@@ -384,40 +385,39 @@ static int __devexit mpcore_wdt_remove(struct platform_device *pdev)

[...]

drivers/watchdog/mpcore_wdt.c static int __devexit mpcore_wdt_remove(struct platform_device *pdev) · line 420

Quoted 13 lines

/* 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),
drivers/watchdog/mpcore_wdt.c +1 -2
-	.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

#12 lore raw

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

Quoted 4 lines · Sergei Shtylyov
Quoted 1 line · Viresh Kumar
  static struct platform_driver mpcore_wdt_driver = {
Quoted 9 lines

drivers/watchdog/mpcore_wdt.c static int __devexit mpcore_wdt_remove(struct platform_device *pdev) · line 420

Quoted 4 lines
  	.driver		= {
  		.owner	= THIS_MODULE,
  		.name	= "mpcore_wdt",
drivers/watchdog/mpcore_wdt.c +1 -0
+		.pm = &mpcore_wdt_dev_pm_ops,

Please align the right part of initializer like all above.

WBR, Sergei

Sure.

--
viresh

author
#13 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/watchdog/mpcore_wdt.c +2 -1
index a099418..27a5069 100644
@@ -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;
 }
Signature · 1 line
1.7.8.110.g4cb5d
author
#14 lore raw

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

In reply to 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 <[email protected]>
---
 arch/arm/include/asm/smp_twd.h |    2 ++
 drivers/watchdog/mpcore_wdt.c  |   10 +++++++++-
 2 files changed, 11 insertions(+), 1 deletions(-)
arch/arm/include/asm/smp_twd.h +2 -0
index ef9ffba9..a13b536 100644
@@ -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;
drivers/watchdog/mpcore_wdt.c +9 -1
index 27a5069..6a04e31 100644
@@ -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;
Signature · 1 line
1.7.8.110.g4cb5d
#15 lore raw

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

Hello.

Quoted 3 lines · 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.

In the subject you say about another ioclt.

Quoted 5 lines
Signed-off-by Viresh Kumar<[email protected]>
---
  arch/arm/include/asm/smp_twd.h |    2 ++
  drivers/watchdog/mpcore_wdt.c  |   10 +++++++++-
  2 files changed, 11 insertions(+), 1 deletions(-)

WBR, Sergei

#16 lore raw

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

Quoted 10 lines · Sergei Shtylyov
Quoted 5 lines · 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.

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

author
#17 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)
drivers/watchdog/mpcore_wdt.c +9 -9
index 6a04e31..ebd964a 100644
@@ -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;
+		}
 	}
 
 	/*
Signature · 1 line
1.7.8.110.g4cb5d
author
#18 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |   71 ++++++++++++++++++++++++++++++++++++-----
 1 files changed, 63 insertions(+), 8 deletions(-)
drivers/watchdog/mpcore_wdt.c +63 -8
index ebd964a..9747193 100644
@@ -17,6 +17,7 @@
  *
  * (c) Copyright 1995    Alan Cox <[email protected]>
  */
+#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
Signature · 1 line
1.7.8.110.g4cb5d
#19 lore raw

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

Quoted 16 lines · 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 <[email protected]>
---
 drivers/watchdog/mpcore_wdt.c |   71 ++++++++++++++++++++++++++++++++++++-----
 1 files changed, 63 insertions(+), 8 deletions(-)
drivers/watchdog/mpcore_wdt.c +0 -0
index ebd964a..9747193 100644
@@ -17,6 +17,7 @@
  *
  * (c) Copyright 1995    Alan Cox <[email protected]>
  */

[..]

Quoted 11 lines
drivers/watchdog/mpcore_wdt.c +7 -0
@@ -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) {

drivers/watchdog/mpcore_wdt.c static int __devinit mpcore_wdt_probe(struct platform_device *pdev) · line 387

Quoted 10 lines
drivers/watchdog/mpcore_wdt.c +10 -0
+		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?

Quoted 9 lines
drivers/watchdog/mpcore_wdt.c +5 -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

drivers/watchdog/mpcore_wdt.c static int __devinit mpcore_wdt_probe(struct platform_device *pdev) · line 376

Quoted 10 lines
drivers/watchdog/mpcore_wdt.c +1 -4
@@ -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

#20 lore raw

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

Quoted 10 lines · Srinidhi Kasagar

drivers/watchdog/mpcore_wdt.c static int __devinit mpcore_wdt_probe(struct platform_device *pdev) · line 387

Quoted 3 lines · Viresh KUMAR
drivers/watchdog/mpcore_wdt.c +3 -0
+	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?

Quoted 1 line
Quoted 7 lines

drivers/watchdog/mpcore_wdt.c static int __devinit mpcore_wdt_probe(struct platform_device *pdev) · line 376

Quoted 3 lines
drivers/watchdog/mpcore_wdt.c +0 -3
-	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

#21 lore raw

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

In reply to Viresh Kumar

drivers/watchdog/mpcore_wdt.c static int mpcore_wdt_open(struct inode *inode, struct file *file) · line 162

Quoted 5 lines · Viresh Kumar
drivers/watchdog/mpcore_wdt.c +1 -0
@@ -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)) {

Review static int mpcore_wdt_open(struct inode *inode, struct file *file) · line 163

Quoted 1 line
(quoted diff) +1 -0
+		ret = clk_enable(wdt->clk);

What about preparing the clock?

Quoted 18 lines
drivers/watchdog/mpcore_wdt.c +10 -0
+		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)

drivers/watchdog/mpcore_wdt.c static int mpcore_wdt_release(struct inode *inode, struct file *file) · line 190

Quoted 9 lines
drivers/watchdog/mpcore_wdt.c +3 -2
@@ -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))

Review static int mpcore_wdt_release(struct inode *inode, struct file *file) · line 191

Quoted 1 line
(quoted diff) +1 -0
+			clk_disable(wdt->clk);

What about unpreparing the clock?

drivers/watchdog/mpcore_wdt.c static int __devinit mpcore_wdt_probe(struct platform_device *pdev) · line 374

Quoted 12 lines
drivers/watchdog/mpcore_wdt.c +5 -0
+	} 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.

drivers/watchdog/mpcore_wdt.c static int __devinit mpcore_wdt_probe(struct platform_device *pdev) · line 377

Quoted 3 lines
drivers/watchdog/mpcore_wdt.c +3 -0
+	}
+
+	if (wdt->clk) {

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

Review static int __devinit mpcore_wdt_probe(struct platform_device *pdev) · line 378

Quoted 1 line
(quoted diff) +1 -0
+		ret = clk_enable(wdt->clk);

What about preparing the clock?

drivers/watchdog/mpcore_wdt.c static int __devinit mpcore_wdt_probe(struct platform_device *pdev) · line 391

Quoted 13 lines
drivers/watchdog/mpcore_wdt.c +13 -0
+		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...

Quoted 28 lines
drivers/watchdog/mpcore_wdt.c +8 -5
+
 	/*
 	 * 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;
 }

drivers/watchdog/mpcore_wdt.c static int __devinit mpcore_wdt_probe(struct platform_device *pdev) · line 435

Quoted 10 lines
drivers/watchdog/mpcore_wdt.c +4 -0
 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.

drivers/watchdog/mpcore_wdt.c static int mpcore_wdt_suspend(struct device *dev) · line 451

Quoted 11 lines
drivers/watchdog/mpcore_wdt.c +4 -0
+
 	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?

drivers/watchdog/mpcore_wdt.c static int mpcore_wdt_resume(struct device *dev) · line 462

Quoted 11 lines
drivers/watchdog/mpcore_wdt.c +3 -1
 	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.

Quoted 15 lines
drivers/watchdog/mpcore_wdt.c +3 -0
+
 		mpcore_wdt_start(wdt);
+	}
+
 	return 0;
 }
 #endif
Signature · 7 lines
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
#22 lore raw

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

Quoted 8 lines · Russell King - ARM Linux

drivers/watchdog/mpcore_wdt.c static int mpcore_wdt_open(struct inode *inode, · line 161

Quoted 4 lines · Viresh Kumar
drivers/watchdog/mpcore_wdt.c +1 -0
@@ -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.

Quoted 6 lines

drivers/watchdog/mpcore_wdt.c static int __devinit mpcore_wdt_probe(struct platform_device *pdev) · line 374

Quoted 4 lines
drivers/watchdog/mpcore_wdt.c +4 -0
+	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.

Quoted 11 lines

drivers/watchdog/mpcore_wdt.c static int mpcore_wdt_suspend(struct device *dev) · line 451

Quoted 7 lines
drivers/watchdog/mpcore_wdt.c +3 -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?

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

--
viresh

#23 lore raw

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

Quoted 19 lines · viresh kumar
Quoted 8 lines · Russell King - ARM Linux

drivers/watchdog/mpcore_wdt.c static int mpcore_wdt_open(struct inode *inode, · line 161

Quoted 4 lines · Viresh Kumar
drivers/watchdog/mpcore_wdt.c +1 -0
@@ -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.

Quoted 6 lines

drivers/watchdog/mpcore_wdt.c static int __devinit mpcore_wdt_probe(struct platform_device *pdev) · line 374

Quoted 4 lines
drivers/watchdog/mpcore_wdt.c +4 -0
+	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.

Quoted 2 lines

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.

#24 lore raw

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

Quoted 3 lines · Russell King - ARM Linux

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

#25 lore raw

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

Quoted 7 lines · viresh kumar
Quoted 3 lines · Russell King - ARM Linux

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.

Quoted 2 lines

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.

author
#26 lore raw

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

In reply to 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 <[email protected]>
---
 arch/arm/include/asm/smp_twd.h |   10 ++++
 drivers/watchdog/mpcore_wdt.c  |  104 +++++++++++++++++++++++++++++++--------
 2 files changed, 92 insertions(+), 22 deletions(-)
arch/arm/include/asm/smp_twd.h +10 -0
index a13b536..389ca02 100644
@@ -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;
drivers/watchdog/mpcore_wdt.c +82 -22

159 lines not on this page · raw message

Signature · 1 line
1.7.8.110.g4cb5d
#27 lore raw

cover watchdog/mpcore: updates & fixes

In reply to Viresh Kumar
Quoted 2 lines

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

Signature · 9 lines
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>
#28 lore raw

cover watchdog/mpcore: updates & fixes

Quoted 7 lines · Wolfram Sang
Quoted 2 lines

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

#29 lore raw

cover watchdog/mpcore: updates & fixes

Quoted 7 lines
Quoted 4 lines

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!

Signature · 9 lines
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>