[PATCHv3] watchdog: add support for the Synopsys DesignWare WDT

The Synopsys DesignWare watchdog is found in several ARM based systems
and provides a choice of 16 timeout periods depending on the clock
input. The watchdog cannot be disabled once started.

v3:
	- convert pm to dev_pm_ops
	- use devres for resource allocation

v2:
	- constify fops
	- request_mem_region() before ioremap()
	- disable clk if misc_register() fails

Cc: Wim Van Sebroeck <redacted>
Signed-off-by: Jamie Iles <redacted>
---
 drivers/watchdog/Kconfig  |    9 ++
 drivers/watchdog/Makefile |    1 +
 drivers/watchdog/dw_wdt.c |  300 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 310 insertions(+), 0 deletions(-)
 create mode 100644 drivers/watchdog/dw_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 8a3aa2f..0981aae 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig

@@ -331,6 +331,15 @@ config IMX2_WDT
 	  To compile this driver as a module, choose M here: the
 	  module will be called imx2_wdt.
 
+config DW_WATCHDOG
+	tristate "Synopsys DesignWare watchdog"
+        select WATCHDOG_NOWAYOUT
+	help
+	  Say Y here if to include support for the Synopsys DesignWare
+	  watchdog timer found in many ARM chips.
+	  To compile this driver as a module, choose M here: the
+	  module will be called dw_wdt.
+
 # AVR32 Architecture
 
 config AT32AP700X_WDT

diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 4b0ef38..3b3da4a 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile

@@ -49,6 +49,7 @@ obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
 obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o
 obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
+obj-$(CONFIG_DW_WATCHDOG) += dw_wdt.o
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
new file mode 100644
index 0000000..e0d0e377
--- /dev/null
+++ b/drivers/watchdog/dw_wdt.c

@@ -0,0 +1,300 @@
+/*
+ * Copyright 2010 Picochip Ltd., Jamie Iles
+ * http://www.picochip.com
+ *
+ * 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 file implements a driver for the Synopsys DesignWare watchdog device
+ * in the many ARM subsystems. The watchdog has 16 different timeout periods
+ * and these are a function of the input clock frequency.
+ */
+#define pr_fmt(fmt) "dw_wdt: " fmt
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/fs.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+
+#define WDOG_CONTROL_REG_OFFSET             0x00
+#define WDOG_TIMEOUT_RANGE_REG_OFFSET       0x04
+#define WDOG_CURRENT_COUNT_REG_OFFSET       0x08
+#define WDOG_COUNTER_RESTART_REG_OFFSET     0x0c
+
+/* The maximum TOP (timeout period) value that can be set in the watchdog. */
+#define DW_WDT_MAX_TOP		15
+
+static struct {
+	spinlock_t		lock;
+	void __iomem		*regs;
+	struct clk		*clk;
+} dw_wdt;
+
+static inline int dw_wdt_is_enabled(void)
+{
+#define WDOG_CONTROL_REG_WDT_EN_MASK		0x01
+	return readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET) &
+		WDOG_CONTROL_REG_WDT_EN_MASK;
+}
+
+static inline int dw_wdt_top_in_seconds(unsigned top)
+{
+	/*
+	 * There are 16 possible timeout values in 0..15 where the number of
+	 * cycles is 2 ^ (16 + i) and the watchdog counts down.
+	 */
+	return (1 << (16 + top)) / clk_get_rate(dw_wdt.clk);
+}
+
+static int dw_wdt_set_top(unsigned top_s)
+{
+	int i, top_val = -1;
+
+	/*
+	 * Iterate over the timeout values until we find the closest match. We
+	 * always look for >=.
+	 */
+	for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
+		if (dw_wdt_top_in_seconds(i) >= top_s) {
+			top_val = i;
+			break;
+		}
+
+	/*
+	 * If we didn't find a suitable value, it must have been too large. Go
+	 * with the biggest that we can.
+	 */
+	if (top_val < 0)
+		top_val = DW_WDT_MAX_TOP;
+
+	/* Set the new value in the watchdog. */
+	writel(top_val, dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
+
+	return dw_wdt_top_in_seconds(top_val);
+}
+
+static int dw_wdt_get_top(void)
+{
+	int top = readl(dw_wdt.regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
+
+	return dw_wdt_top_in_seconds(top);
+}
+
+static void dw_wdt_keepalive(void)
+{
+#define WDOG_COUNTER_RESTART_KICK_VALUE	    0x76
+	writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
+	       WDOG_COUNTER_RESTART_REG_OFFSET);
+}
+
+static int dw_wdt_open(struct inode *inode, struct file *filp)
+{
+	/* Make sure we don't get unloaded. */
+	__module_get(THIS_MODULE);
+
+	spin_lock(&dw_wdt.lock);
+	if (!dw_wdt_is_enabled()) {
+		/*
+		 * The watchdog is not currently enabled. Set the timeout to
+		 * the maximum and then start it.
+		 */
+		dw_wdt_set_top(DW_WDT_MAX_TOP);
+		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
+		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+	}
+	spin_unlock(&dw_wdt.lock);
+
+	return nonseekable_open(inode, filp);
+}
+
+ssize_t dw_wdt_write(struct file *filp, const char __user *buf, size_t len,
+		     loff_t *offset)
+{
+	dw_wdt_keepalive();
+
+	return len;
+}
+
+static u32 dw_wdt_time_left(void)
+{
+	return readl(dw_wdt.regs + WDOG_CURRENT_COUNT_REG_OFFSET) /
+		clk_get_rate(dw_wdt.clk);
+}
+
+static const struct watchdog_info dw_wdt_ident = {
+	.options	= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+	.identity	= "Synopsys DesignWare Watchdog",
+};
+
+static long dw_wdt_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	unsigned long val;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		return copy_to_user((struct watchdog_info *)arg, &dw_wdt_ident,
+				    sizeof(dw_wdt_ident)) ? -EFAULT : 0;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, (int *)arg);
+
+	case WDIOC_KEEPALIVE:
+		dw_wdt_keepalive();
+		return 0;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(val, (int __user *)arg))
+			return -EFAULT;
+		return put_user(dw_wdt_set_top(val), (int __user *)arg);
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(dw_wdt_get_top(), (int __user *)arg);
+
+	case WDIOC_GETTIMELEFT:
+		/* Get the time left until expiry. */
+		if (get_user(val, (int __user *)arg))
+			return -EFAULT;
+		return put_user(dw_wdt_time_left(), (int __user *)arg);
+
+	default:
+		return -ENOTTY;
+	}
+}
+
+static int dw_wdt_release(struct inode *inode, struct file *filp)
+{
+	pr_crit("WATCHDOG: device closed - timer will not stop\n");
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int dw_wdt_suspend(struct device *dev)
+{
+	clk_disable(dw_wdt.clk);
+
+	return 0;
+}
+
+static int dw_wdt_resume(struct device *dev)
+{
+	int err = clk_enable(dw_wdt.clk);
+
+	if (err)
+		return err;
+
+	dw_wdt_keepalive();
+
+	return 0;
+}
+
+static const struct dev_pm_ops dw_wdt_pm_ops = {
+	.suspend	= dw_wdt_suspend,
+	.resume		= dw_wdt_resume,
+};
+
+#define DW_WDT_PM_OPS	(&dw_wdt_pm_ops)
+#else /* CONFIG_PM */
+#define DW_WDT_PM_OPS	NULL
+#endif /* CONFIG_PM */
+
+static const struct file_operations wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.open		= dw_wdt_open,
+	.write		= dw_wdt_write,
+	.unlocked_ioctl	= dw_wdt_ioctl,
+	.release	= dw_wdt_release
+};
+
+static struct miscdevice dw_wdt_miscdev = {
+	.fops		= &wdt_fops,
+	.name		= "watchdog",
+	.minor		= WATCHDOG_MINOR,
+};
+
+static int __devinit dw_wdt_drv_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	if (!mem)
+		return -EINVAL;
+
+	if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
+				     "iomem"))
+		return -ENOMEM;
+
+	dw_wdt.regs = devm_ioremap(&pdev->dev, mem->start,
+					     resource_size(mem));
+	if (!dw_wdt.regs)
+		return -ENOMEM;
+
+	dw_wdt.clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR_OR_NULL(dw_wdt.clk))
+		return -ENODEV;
+	clk_enable(dw_wdt.clk);
+
+	ret = misc_register(&dw_wdt_miscdev);
+	if (ret)
+		goto register_failed;
+
+	return 0;
+
+register_failed:
+	clk_disable(dw_wdt.clk);
+	clk_put(dw_wdt.clk);
+
+	return ret;
+}
+
+static int __devexit dw_wdt_drv_remove(struct platform_device *pdev)
+{
+	clk_disable(dw_wdt.clk);
+	clk_put(dw_wdt.clk);
+
+	misc_deregister(&dw_wdt_miscdev);
+
+	return 0;
+}
+
+static struct platform_driver dw_wdt_driver = {
+	.probe		= dw_wdt_drv_probe,
+	.remove		= __devexit_p(dw_wdt_drv_remove),
+	.driver		= {
+		.name	= "dw_wdt",
+		.owner	= THIS_MODULE,
+		.pm	= DW_WDT_PM_OPS,
+	},
+};
+
+static int __init dw_wdt_watchdog_init(void)
+{
+	spin_lock_init(&dw_wdt.lock);
+
+	return platform_driver_register(&dw_wdt_driver);
+}
+
+static void __exit dw_wdt_watchdog_exit(void)
+{
+	platform_driver_unregister(&dw_wdt_driver);
+}
+
+module_init(dw_wdt_watchdog_init);
+module_exit(dw_wdt_watchdog_exit);
+
+MODULE_AUTHOR("Jamie Iles");
+MODULE_DESCRIPTION("Synopsys DesignWare Watchdog Driver");
+MODULE_LICENSE("GPL");

-- 
1.7.3.4

On Fri, Jan 7, 2011 at 5:33 PM, Jamie Iles [off-list ref] wrote:

(...)

(...)

+static inline int dw_wdt_is_enabled(void)
+{
+#define WDOG_CONTROL_REG_WDT_EN_MASK ? ? ? ? ? 0x01

can be moved to top, with other macros.

+ ? ? ? return readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET) &
+ ? ? ? ? ? ? ? WDOG_CONTROL_REG_WDT_EN_MASK;
+}
+

(...)

+static void dw_wdt_keepalive(void)
+{
+#define WDOG_COUNTER_RESTART_KICK_VALUE ? ? ? ? ? ?0x76

ditto...

+ ? ? ? writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
+ ? ? ? ? ? ? ?WDOG_COUNTER_RESTART_REG_OFFSET);
+}
+
+static int dw_wdt_open(struct inode *inode, struct file *filp)
+{
+ ? ? ? /* Make sure we don't get unloaded. */
+ ? ? ? __module_get(THIS_MODULE);
+
+ ? ? ? spin_lock(&dw_wdt.lock);
+ ? ? ? if (!dw_wdt_is_enabled()) {
+ ? ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ? ?* The watchdog is not currently enabled. Set the timeout to
+ ? ? ? ? ? ? ? ?* the maximum and then start it.
+ ? ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? ? dw_wdt_set_top(DW_WDT_MAX_TOP);

shouldn't we check return value here??

+ ? ? ? ? ? ? ? writel(WDOG_CONTROL_REG_WDT_EN_MASK,
+ ? ? ? ? ? ? ? ? ? ? ?dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+ ? ? ? }
+ ? ? ? spin_unlock(&dw_wdt.lock);
+
+ ? ? ? return nonseekable_open(inode, filp);
+}
+

(...)

+static const struct dev_pm_ops dw_wdt_pm_ops = {
+ ? ? ? .suspend ? ? ? ?= dw_wdt_suspend,
+ ? ? ? .resume ? ? ? ? = dw_wdt_resume,
+};
+
+#define DW_WDT_PM_OPS ?(&dw_wdt_pm_ops)
+#else /* CONFIG_PM */
+#define DW_WDT_PM_OPS ?NULL
+#endif /* CONFIG_PM */

This can be rewritten as:
static struct platform_driver dw_wdt_driver = {
...
+#ifdef CONFIG_PM
 ? ? ? ? ? ? ? .pm ? ? = dw_wdt_pm_ops,
#endif
}

+static int __devinit dw_wdt_drv_probe(struct platform_device *pdev)
+{
+ ? ? ? int ret;
+ ? ? ? struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ ? ? ? if (!mem)
+ ? ? ? ? ? ? ? return -EINVAL;
+
+ ? ? ? if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"iomem"))
+ ? ? ? ? ? ? ? return -ENOMEM;
+
+ ? ? ? dw_wdt.regs = devm_ioremap(&pdev->dev, mem->start,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?resource_size(mem));
+ ? ? ? if (!dw_wdt.regs)
+ ? ? ? ? ? ? ? return -ENOMEM;

should release mem_region in case of error.

+
+ ? ? ? dw_wdt.clk = clk_get(&pdev->dev, NULL);
+ ? ? ? if (IS_ERR_OR_NULL(dw_wdt.clk))
+ ? ? ? ? ? ? ? return -ENODEV;

should release mem_region and free ioremaped space.
Also, may be we can continue here in case of error too.
Some platforms might nor support clock framework. You can enable and
disable clk's
if dw_wdt.clk is !NULL

+ ? ? ? clk_enable(dw_wdt.clk);
+
+ ? ? ? ret = misc_register(&dw_wdt_miscdev);
+ ? ? ? if (ret)
+ ? ? ? ? ? ? ? goto register_failed;
+
+ ? ? ? return 0;
+
+register_failed:
+ ? ? ? clk_disable(dw_wdt.clk);
+ ? ? ? clk_put(dw_wdt.clk);

also iounmap and release_mem_region...

+
+ ? ? ? return ret;
+}
+
+static int __devexit dw_wdt_drv_remove(struct platform_device *pdev)
+{
+ ? ? ? clk_disable(dw_wdt.clk);
+ ? ? ? clk_put(dw_wdt.clk);
+
+ ? ? ? misc_deregister(&dw_wdt_miscdev);

also iounmap and release_mem_region...

+
+ ? ? ? return 0;
+}
+
+static struct platform_driver dw_wdt_driver = {
+ ? ? ? .probe ? ? ? ? ?= dw_wdt_drv_probe,
+ ? ? ? .remove ? ? ? ? = __devexit_p(dw_wdt_drv_remove),
+ ? ? ? .driver ? ? ? ? = {
+ ? ? ? ? ? ? ? .name ? = "dw_wdt",
+ ? ? ? ? ? ? ? .owner ?= THIS_MODULE,
+ ? ? ? ? ? ? ? .pm ? ? = DW_WDT_PM_OPS,
+ ? ? ? },
+};
+
+static int __init dw_wdt_watchdog_init(void)
+{
+ ? ? ? spin_lock_init(&dw_wdt.lock);

This can be moved to probe. spin_lock_init should be only called when we have
some device for this driver.

+
+ ? ? ? return platform_driver_register(&dw_wdt_driver);
+}
+
+static void __exit dw_wdt_watchdog_exit(void)
+{
+ ? ? ? platform_driver_unregister(&dw_wdt_driver);
+}
+
+module_init(dw_wdt_watchdog_init);
+module_exit(dw_wdt_watchdog_exit);

should be moved after init & exit routines without any blank lines.

--
viresh
ST Microelectronics

On Fri, Jan 07, 2011 at 10:13:50PM +0530, viresh kumar wrote:

quoted

+
+ ? ? ? dw_wdt.clk = clk_get(&pdev->dev, NULL);
+ ? ? ? if (IS_ERR_OR_NULL(dw_wdt.clk))
+ ? ? ? ? ? ? ? return -ENODEV;

should release mem_region and free ioremaped space.
Also, may be we can continue here in case of error too.
Some platforms might nor support clock framework. You can enable and
disable clk's
if dw_wdt.clk is !NULL

And some platforms have been known to return NULL for clk_get() as they
don't support more than one clock.

arch/arm/mach-aaec2000/core.c:struct clk *clk_get(struct device *dev, const char *id)
arch/arm/mach-aaec2000/core.c-{
arch/arm/mach-aaec2000/core.c-  return dev && strcmp(dev_name(dev), "mb:16") ==
0 ? NULL : ERR_PTR(-ENOENT);
--
arch/arm/mach-at91/at91x40.c:struct clk *clk_get(struct device *dev, const char
*id)
arch/arm/mach-at91/at91x40.c-{
arch/arm/mach-at91/at91x40.c-   return NULL;
--
arch/arm/mach-netx/fb.c:struct clk *clk_get(struct device *dev, const char *id)
arch/arm/mach-netx/fb.c-{
arch/arm/mach-netx/fb.c-        return dev && strcmp(dev_name(dev), "fb") == 0 ? NULL : ERR_PTR(-ENOENT);

Please stick to the conventions of the API in the driver.  IS_ERR() values
mean we failed.  Anything else must be considered success.  Don't assume
NULL means we failed.

Hi Viresh,

Thanks for the feedback, comments inline.

Jamie

On Fri, Jan 07, 2011 at 10:13:50PM +0530, viresh kumar wrote:

On Fri, Jan 7, 2011 at 5:33 PM, Jamie Iles [off-list ref] wrote:

(...)

quoted

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
new file mode 100644
index 0000000..e0d0e377
--- /dev/null
+++ b/drivers/watchdog/dw_wdt.c

@@ -0,0 +1,300 @@

(...)

quoted

+static inline int dw_wdt_is_enabled(void)
+{
+#define WDOG_CONTROL_REG_WDT_EN_MASK ? ? ? ? ? 0x01

can be moved to top, with other macros.

quoted

+ ? ? ? return readl(dw_wdt.regs + WDOG_CONTROL_REG_OFFSET) &
+ ? ? ? ? ? ? ? WDOG_CONTROL_REG_WDT_EN_MASK;
+}
+

(...)

quoted

+static void dw_wdt_keepalive(void)
+{
+#define WDOG_COUNTER_RESTART_KICK_VALUE ? ? ? ? ? ?0x76

ditto...

Ok, agreed.

quoted

+ ? ? ? writel(WDOG_COUNTER_RESTART_KICK_VALUE, dw_wdt.regs +
+ ? ? ? ? ? ? ?WDOG_COUNTER_RESTART_REG_OFFSET);
+}
+
+static int dw_wdt_open(struct inode *inode, struct file *filp)
+{
+ ? ? ? /* Make sure we don't get unloaded. */
+ ? ? ? __module_get(THIS_MODULE);
+
+ ? ? ? spin_lock(&dw_wdt.lock);
+ ? ? ? if (!dw_wdt_is_enabled()) {
+ ? ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ? ?* The watchdog is not currently enabled. Set the timeout to
+ ? ? ? ? ? ? ? ?* the maximum and then start it.
+ ? ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? ? dw_wdt_set_top(DW_WDT_MAX_TOP);

shouldn't we check return value here??

No, dw_wdt_set_top() can't fail, it just returns the timeout period that 
it set in seconds.  We use this when the user changes the timeout period 
as we may not be able to select the exact timeout period they chose, but 
here we just select the maximum timeout.

quoted

+ ? ? ? ? ? ? ? writel(WDOG_CONTROL_REG_WDT_EN_MASK,
+ ? ? ? ? ? ? ? ? ? ? ?dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+ ? ? ? }
+ ? ? ? spin_unlock(&dw_wdt.lock);
+
+ ? ? ? return nonseekable_open(inode, filp);
+}
+

(...)

quoted

+static const struct dev_pm_ops dw_wdt_pm_ops = {
+ ? ? ? .suspend ? ? ? ?= dw_wdt_suspend,
+ ? ? ? .resume ? ? ? ? = dw_wdt_resume,
+};
+
+#define DW_WDT_PM_OPS ?(&dw_wdt_pm_ops)
+#else /* CONFIG_PM */
+#define DW_WDT_PM_OPS ?NULL
+#endif /* CONFIG_PM */

This can be rewritten as:
static struct platform_driver dw_wdt_driver = {
...
+#ifdef CONFIG_PM
 ? ? ? ? ? ? ? .pm ? ? = dw_wdt_pm_ops,
#endif
}

Ok, that's clearer.

quoted

+static int __devinit dw_wdt_drv_probe(struct platform_device *pdev)
+{
+ ? ? ? int ret;
+ ? ? ? struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ ? ? ? if (!mem)
+ ? ? ? ? ? ? ? return -EINVAL;
+
+ ? ? ? if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"iomem"))
+ ? ? ? ? ? ? ? return -ENOMEM;
+
+ ? ? ? dw_wdt.regs = devm_ioremap(&pdev->dev, mem->start,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?resource_size(mem));
+ ? ? ? if (!dw_wdt.regs)
+ ? ? ? ? ? ? ? return -ENOMEM;

should release mem_region in case of error.

quoted

+
+ ? ? ? dw_wdt.clk = clk_get(&pdev->dev, NULL);
+ ? ? ? if (IS_ERR_OR_NULL(dw_wdt.clk))
+ ? ? ? ? ? ? ? return -ENODEV;

should release mem_region and free ioremaped space.

We're using devres for the ioremap and region request so that takes care 
of the cleanup for us.  This also means we don't need to iounmap and 
release in the release method.

Also, may be we can continue here in case of error too.
Some platforms might nor support clock framework. You can enable and
disable clk's if dw_wdt.clk is !NULL

The DesignWare watchdog has 16 timeout periods and these are derived 
from the clock frequency input to the WDT.  If we don't have a clk then 
we don't know how long the timeout periods.  The only alternative would 
be to have a 'struct dw_wdt_platform_data' that includes the input clock 
frequency which we use if we can't get a clk and get the rate.

quoted

+ ? ? ? clk_enable(dw_wdt.clk);
+
+ ? ? ? ret = misc_register(&dw_wdt_miscdev);
+ ? ? ? if (ret)
+ ? ? ? ? ? ? ? goto register_failed;
+
+ ? ? ? return 0;
+
+register_failed:
+ ? ? ? clk_disable(dw_wdt.clk);
+ ? ? ? clk_put(dw_wdt.clk);

also iounmap and release_mem_region...

quoted

+
+ ? ? ? return ret;
+}
+
+static int __devexit dw_wdt_drv_remove(struct platform_device *pdev)
+{
+ ? ? ? clk_disable(dw_wdt.clk);
+ ? ? ? clk_put(dw_wdt.clk);
+
+ ? ? ? misc_deregister(&dw_wdt_miscdev);

also iounmap and release_mem_region...

quoted

+
+ ? ? ? return 0;
+}
+
+static struct platform_driver dw_wdt_driver = {
+ ? ? ? .probe ? ? ? ? ?= dw_wdt_drv_probe,
+ ? ? ? .remove ? ? ? ? = __devexit_p(dw_wdt_drv_remove),
+ ? ? ? .driver ? ? ? ? = {
+ ? ? ? ? ? ? ? .name ? = "dw_wdt",
+ ? ? ? ? ? ? ? .owner ?= THIS_MODULE,
+ ? ? ? ? ? ? ? .pm ? ? = DW_WDT_PM_OPS,
+ ? ? ? },
+};
+
+static int __init dw_wdt_watchdog_init(void)
+{
+ ? ? ? spin_lock_init(&dw_wdt.lock);

This can be moved to probe. spin_lock_init should be only called when we have
some device for this driver.

Agreed.

quoted

+
+ ? ? ? return platform_driver_register(&dw_wdt_driver);
+}
+
+static void __exit dw_wdt_watchdog_exit(void)
+{
+ ? ? ? platform_driver_unregister(&dw_wdt_driver);
+}
+
+module_init(dw_wdt_watchdog_init);
+module_exit(dw_wdt_watchdog_exit);

should be moved after init & exit routines without any blank lines.

Ok, thanks.

On Fri, Jan 7, 2011 at 10:38 PM, Jamie Iles [off-list ref] wrote:

quoted

quoted

+static int dw_wdt_open(struct inode *inode, struct file *filp)
+{
+ ? ? ? /* Make sure we don't get unloaded. */
+ ? ? ? __module_get(THIS_MODULE);
+
+ ? ? ? spin_lock(&dw_wdt.lock);
+ ? ? ? if (!dw_wdt_is_enabled()) {
+ ? ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ? ?* The watchdog is not currently enabled. Set the timeout to
+ ? ? ? ? ? ? ? ?* the maximum and then start it.
+ ? ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? ? dw_wdt_set_top(DW_WDT_MAX_TOP);

shouldn't we check return value here??

No, dw_wdt_set_top() can't fail, it just returns the timeout period that
it set in seconds. ?We use this when the user changes the timeout period
as we may not be able to select the exact timeout period they chose, but
here we just select the maximum timeout.

Sorry, I missed that.

quoted

quoted

+static int __devinit dw_wdt_drv_probe(struct platform_device *pdev)
+{
+ ? ? ? int ret;
+ ? ? ? struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ ? ? ? if (!mem)
+ ? ? ? ? ? ? ? return -EINVAL;
+
+ ? ? ? if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"iomem"))
+ ? ? ? ? ? ? ? return -ENOMEM;
+
+ ? ? ? dw_wdt.regs = devm_ioremap(&pdev->dev, mem->start,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?resource_size(mem));
+ ? ? ? if (!dw_wdt.regs)
+ ? ? ? ? ? ? ? return -ENOMEM;

should release mem_region in case of error.

quoted

+
+ ? ? ? dw_wdt.clk = clk_get(&pdev->dev, NULL);
+ ? ? ? if (IS_ERR_OR_NULL(dw_wdt.clk))
+ ? ? ? ? ? ? ? return -ENODEV;

should release mem_region and free ioremaped space.

We're using devres for the ioremap and region request so that takes care
of the cleanup for us. ?This also means we don't need to iounmap and
release in the release method.

ok

quoted

Also, may be we can continue here in case of error too.
Some platforms might nor support clock framework. You can enable and
disable clk's if dw_wdt.clk is !NULL

The DesignWare watchdog has 16 timeout periods and these are derived
from the clock frequency input to the WDT. ?If we don't have a clk then
we don't know how long the timeout periods. ?The only alternative would
be to have a 'struct dw_wdt_platform_data' that includes the input clock
frequency which we use if we can't get a clk and get the rate.

That will be fine.

--
viresh

On Fri, Jan 07, 2011 at 04:52:50PM +0000, Russell King - ARM Linux wrote:

On Fri, Jan 07, 2011 at 10:13:50PM +0530, viresh kumar wrote:

quoted

quoted

+
+ ? ? ? dw_wdt.clk = clk_get(&pdev->dev, NULL);
+ ? ? ? if (IS_ERR_OR_NULL(dw_wdt.clk))
+ ? ? ? ? ? ? ? return -ENODEV;

should release mem_region and free ioremaped space.
Also, may be we can continue here in case of error too.
Some platforms might nor support clock framework. You can enable and
disable clk's
if dw_wdt.clk is !NULL

And some platforms have been known to return NULL for clk_get() as they
don't support more than one clock.

arch/arm/mach-aaec2000/core.c:struct clk *clk_get(struct device *dev, const char *id)
arch/arm/mach-aaec2000/core.c-{
arch/arm/mach-aaec2000/core.c-  return dev && strcmp(dev_name(dev), "mb:16") ==
0 ? NULL : ERR_PTR(-ENOENT);
--
arch/arm/mach-at91/at91x40.c:struct clk *clk_get(struct device *dev, const char
*id)
arch/arm/mach-at91/at91x40.c-{
arch/arm/mach-at91/at91x40.c-   return NULL;
--
arch/arm/mach-netx/fb.c:struct clk *clk_get(struct device *dev, const char *id)
arch/arm/mach-netx/fb.c-{
arch/arm/mach-netx/fb.c-        return dev && strcmp(dev_name(dev), "fb") == 0 ? NULL : ERR_PTR(-ENOENT);

Please stick to the conventions of the API in the driver.  IS_ERR() values
mean we failed.  Anything else must be considered success.  Don't assume
NULL means we failed.

Ok, so I'll change the driver to get the clock rate of the watchdog 
through platform data for those that don't provide a struct clk and 
clk_get_rate() returns 0.

Looking at mach-aaec2000 and mach-netx they don't provide a 
clk_get_rate() which from linux/clk.h doesn't look like it's optional.  
Should these platforms have some kind of stub clk_get_rate()?

Jamie

On Fri, Jan 07, 2011 at 05:56:13PM +0000, Jamie Iles wrote:

On Fri, Jan 07, 2011 at 04:52:50PM +0000, Russell King - ARM Linux wrote:

quoted

On Fri, Jan 07, 2011 at 10:13:50PM +0530, viresh kumar wrote:

quoted

quoted

+
+ ? ? ? dw_wdt.clk = clk_get(&pdev->dev, NULL);
+ ? ? ? if (IS_ERR_OR_NULL(dw_wdt.clk))
+ ? ? ? ? ? ? ? return -ENODEV;

should release mem_region and free ioremaped space.
Also, may be we can continue here in case of error too.
Some platforms might nor support clock framework. You can enable and
disable clk's
if dw_wdt.clk is !NULL

And some platforms have been known to return NULL for clk_get() as they
don't support more than one clock.

arch/arm/mach-aaec2000/core.c:struct clk *clk_get(struct device *dev, const char *id)
arch/arm/mach-aaec2000/core.c-{
arch/arm/mach-aaec2000/core.c-  return dev && strcmp(dev_name(dev), "mb:16") ==
0 ? NULL : ERR_PTR(-ENOENT);
--
arch/arm/mach-at91/at91x40.c:struct clk *clk_get(struct device *dev, const char
*id)
arch/arm/mach-at91/at91x40.c-{
arch/arm/mach-at91/at91x40.c-   return NULL;
--
arch/arm/mach-netx/fb.c:struct clk *clk_get(struct device *dev, const char *id)
arch/arm/mach-netx/fb.c-{
arch/arm/mach-netx/fb.c-        return dev && strcmp(dev_name(dev), "fb") == 0 ? NULL : ERR_PTR(-ENOENT);

Please stick to the conventions of the API in the driver.  IS_ERR() values
mean we failed.  Anything else must be considered success.  Don't assume
NULL means we failed.

Ok, so I'll change the driver to get the clock rate of the watchdog 
through platform data for those that don't provide a struct clk and 
clk_get_rate() returns 0.

Looking at mach-aaec2000 and mach-netx they don't provide a 
clk_get_rate() which from linux/clk.h doesn't look like it's optional.  
Should these platforms have some kind of stub clk_get_rate()?

Probably, though I don't think aaec2000 is maintained anymore (I think it
was a dump-and-run thing) so should probably be deleted from the kernel
tree.

netx - since December 2008, it's only received updates when other stuff
has changed (eg, on my clocksource sweep, or when I've noticed it no
longer building.)  So I think that's also a candidate for deletion
unless someone speaks up.

Wonder if anyone wants to volunteer to delete them... ;)

On Fri, Jan 07, 2011 at 06:09:24PM +0000, Russell King - ARM Linux wrote:

Probably, though I don't think aaec2000 is maintained anymore (I think it
was a dump-and-run thing) so should probably be deleted from the kernel
tree.

netx - since December 2008, it's only received updates when other stuff
has changed (eg, on my clocksource sweep, or when I've noticed it no
longer building.)  So I think that's also a candidate for deletion
unless someone speaks up.

Wonder if anyone wants to volunteer to delete them... ;)

I'm happy to do that for aaec2000, patch to follow.  I'm willing to do
the same for netx too or would you like to give people chance to shout
first?

Jamie

Hi Jamie,

On top of other comments from Viresh:

+static int dw_wdt_open(struct inode *inode, struct file *filp)
+{
+	/* Make sure we don't get unloaded. */
+	__module_get(THIS_MODULE);
+
+	spin_lock(&dw_wdt.lock);
+	if (!dw_wdt_is_enabled()) {
+		/*
+		 * The watchdog is not currently enabled. Set the timeout to
+		 * the maximum and then start it.
+		 */
+		dw_wdt_set_top(DW_WDT_MAX_TOP);
+		writel(WDOG_CONTROL_REG_WDT_EN_MASK,
+		       dw_wdt.regs + WDOG_CONTROL_REG_OFFSET);
+	}
+	spin_unlock(&dw_wdt.lock);
+
+	return nonseekable_open(inode, filp);
+}

Would be nice to have the open /dev/watchdog once protection here also.

+static int __devexit dw_wdt_drv_remove(struct platform_device *pdev)
+{
+	clk_disable(dw_wdt.clk);
+	clk_put(dw_wdt.clk);
+
+	misc_deregister(&dw_wdt_miscdev);
+
+	return 0;
+}

misc_deregister (=link to user-space) should be called before the clk_disabel calls.

Kind regards,
Wim.