[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Thierry Reding]

This patch adds support for the watchdog timer on Avionic Design Xanthos
boards.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

---
 drivers/watchdog/Kconfig   |    7 +
 drivers/watchdog/Makefile  |    1 +
 drivers/watchdog/adx_wdt.c |  380 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 388 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b1ccc04..378d518 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -266,6 +266,13 @@ config STMP3XXX_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called stmp3xxx_wdt.
 
+config ADX_WATCHDOG
+	tristate "Avionic Design Xanthos watchdog"
+	depends on ARCH_PXA_ADX
+	help
+	  Say Y here if you want support for the watchdog timer on Avionic
+	  Design Xanthos boards.
+
 # AVR32 Architecture
 
 config AT32AP700X_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 3d77429..f91719f 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o
 obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o
 obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o
 obj-$(CONFIG_STMP3XXX_WATCHDOG) += stmp3xxx_wdt.o
+obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/adx_wdt.c b/drivers/watchdog/adx_wdt.c
new file mode 100644
index 0000000..0abf7ca
--- /dev/null
+++ b/drivers/watchdog/adx_wdt.c
@@ -0,0 +1,380 @@
+/*
+ * linux/drivers/watchdog/adx_wdt.c
+ *
+ * Copyright (C) 2008-2009 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/fs.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+
+#define WATCHDOG_NAME "adx-wdt"
+
+/* register offsets */
+#define	ADX_WDT_CONTROL		0x00
+#define	ADX_WDT_CONTROL_ENABLE	(1 << 0)
+#define	ADX_WDT_CONTROL_nRESET	(1 << 1)
+#define	ADX_WDT_TIMEOUT		0x08
+
+struct adx_wdt_nb;
+struct adx_wdt;
+
+struct adx_wdt_nb {
+	struct adx_wdt *wdt;
+};
+
+static struct platform_device *adx_wdt_dev;
+static unsigned long driver_open;
+
+#define	WDT_STATE_STOP	0
+#define	WDT_STATE_START	1
+
+struct adx_wdt {
+	void __iomem *base;
+	unsigned int irq;
+	unsigned long timeout;
+	struct adx_wdt_nb *nb;
+	unsigned int state;
+	unsigned int wake;
+	spinlock_t lock;
+};
+
+static struct watchdog_info adx_wdt_info = {
+	.identity = "Avionic Design Xanthos Watchdog",
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+};
+
+static void adx_wdt_start_locked(struct adx_wdt *wdt)
+{
+	u32 ctrl;
+
+	ctrl = readl(wdt->base + ADX_WDT_CONTROL);
+	ctrl |= ADX_WDT_CONTROL_ENABLE;
+	writel(ctrl, wdt->base + ADX_WDT_CONTROL);
+	wdt->state = WDT_STATE_START;
+}
+
+static void adx_wdt_start(struct adx_wdt *wdt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	adx_wdt_start_locked(wdt);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+}
+
+static void adx_wdt_stop_locked(struct adx_wdt *wdt)
+{
+	u32 ctrl;
+
+	ctrl = readl(wdt->base + ADX_WDT_CONTROL);
+	ctrl &= ~ADX_WDT_CONTROL_ENABLE;
+	writel(ctrl, wdt->base + ADX_WDT_CONTROL);
+	wdt->state = WDT_STATE_STOP;
+}
+
+static void adx_wdt_stop(struct adx_wdt *wdt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	adx_wdt_stop_locked(wdt);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+}
+
+static void adx_wdt_set_timeout(struct adx_wdt *wdt, unsigned long timeout)
+{
+	unsigned long flags;
+	unsigned int state;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	state = wdt->state;
+	adx_wdt_stop_locked(wdt);
+	writel(timeout, wdt->base + ADX_WDT_TIMEOUT);
+
+	if (state == WDT_STATE_START)
+		adx_wdt_start_locked(wdt);
+
+	wdt->timeout = timeout;
+	spin_unlock_irqrestore(&wdt->lock, flags);
+}
+
+static void adx_wdt_keepalive(struct adx_wdt *wdt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	writel(wdt->timeout, wdt->base + ADX_WDT_TIMEOUT);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+}
+
+static int adx_wdt_open(struct inode *inode, struct file *file)
+{
+	struct adx_wdt *wdt = platform_get_drvdata(adx_wdt_dev);
+
+	if (test_and_set_bit(0, &driver_open))
+		return -EBUSY;
+
+	adx_wdt_set_timeout(wdt, 30 * 1000);
+	adx_wdt_start(wdt);
+	file->private_data = wdt;
+
+	return nonseekable_open(inode, file);
+}
+
+static int adx_wdt_release(struct inode *inode, struct file *file)
+{
+	struct adx_wdt *wdt = file->private_data;
+
+	adx_wdt_stop(wdt);
+	clear_bit(0, &driver_open);
+
+	return 0;
+}
+
+static int adx_wdt_ioctl(struct inode *inode, struct file *file,
+		unsigned int cmd, unsigned long arg)
+{
+	struct adx_wdt *wdt = file->private_data;
+	void __user *argp = (void __user *)arg;
+	unsigned long __user *p = argp;
+	unsigned long timeout = 0;
+	unsigned int options;
+	int ret = -EINVAL;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		if (copy_to_user(argp, &adx_wdt_info, sizeof(adx_wdt_info)))
+			return -EFAULT;
+		else
+			return 0;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(!!(0), p);
+
+	case WDIOC_KEEPALIVE:
+		adx_wdt_keepalive(wdt);
+		return 0;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(timeout, p))
+			return -EFAULT;
+
+		adx_wdt_set_timeout(wdt, timeout * 1000);
+
+		/* fallthrough */
+	case WDIOC_GETTIMEOUT:
+		return put_user(timeout, p);
+
+	case WDIOC_SETOPTIONS:
+		if (copy_from_user(&options, argp, sizeof(options)))
+			return -EFAULT;
+
+		if (options & WDIOS_DISABLECARD) {
+			adx_wdt_stop(wdt);
+			ret = 0;
+		}
+
+		if (options & WDIOS_ENABLECARD) {
+			adx_wdt_start(wdt);
+			ret = 0;
+		}
+
+		return ret;
+
+	default:
+		break;
+	}
+
+	return -ENOTTY;
+}
+
+static ssize_t adx_wdt_write(struct file *file, const char __user *data,
+		size_t len, loff_t *ppos)
+{
+	struct adx_wdt *wdt = file->private_data;
+
+	if (len)
+		adx_wdt_keepalive(wdt);
+
+	return len;
+}
+
+static const struct file_operations adx_wdt_fops = {
+	.owner = THIS_MODULE,
+	.llseek = no_llseek,
+	.open = adx_wdt_open,
+	.release = adx_wdt_release,
+	.ioctl = adx_wdt_ioctl,
+	.write = adx_wdt_write,
+};
+
+static struct miscdevice adx_wdt_miscdev = {
+	.minor = WATCHDOG_MINOR,
+	.name = "watchdog",
+	.fops = &adx_wdt_fops,
+};
+
+static irqreturn_t adx_wdt_interrupt(int irq, void *dev_id)
+{
+	struct adx_wdt *wdt = dev_id;
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	writel(wdt->timeout, wdt->base + ADX_WDT_TIMEOUT);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
+static int __devinit adx_wdt_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct adx_wdt *wdt;
+	int ret = 0;
+
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt) {
+		dev_err(&pdev->dev, "cannot allocate WDT structure\n");
+		return -ENOMEM;
+	}
+
+	spin_lock_init(&wdt->lock);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "cannot obtain I/O memory region\n");
+		return -ENXIO;
+	}
+
+	res = devm_request_mem_region(&pdev->dev, res->start,
+			res->end - res->start + 1, res->name);
+	if (!res) {
+		dev_err(&pdev->dev, "cannot request I/O memory region\n");
+		return -ENXIO;
+	}
+
+	wdt->base = devm_ioremap_nocache(&pdev->dev, res->start,
+			res->end - res->start + 1);
+	if (!wdt->base) {
+		dev_err(&pdev->dev, "cannot remap I/O memory region\n");
+		return -ENXIO;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "cannot obtain IRQ\n");
+		return -ENXIO;
+	}
+
+	wdt->irq = res->start;
+
+	ret = devm_request_irq(&pdev->dev, wdt->irq, adx_wdt_interrupt,
+			IRQF_SHARED, WATCHDOG_NAME, wdt);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "cannot request IRQ\n");
+		return -ENXIO;
+	}
+
+	ret = misc_register(&adx_wdt_miscdev);
+	if (ret) {
+		dev_err(&pdev->dev, "cannot register miscdev on minor %d "
+				"(err=%d)\n", WATCHDOG_MINOR, ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, wdt);
+	adx_wdt_dev = pdev;
+
+	return 0;
+}
+
+static int __devexit adx_wdt_remove(struct platform_device *pdev)
+{
+	struct adx_wdt *wdt = platform_get_drvdata(pdev);
+
+	platform_set_drvdata(pdev, NULL);
+	misc_deregister(&adx_wdt_miscdev);
+	adx_wdt_stop(wdt);
+
+	return 0;
+}
+
+static void adx_wdt_shutdown(struct platform_device *pdev)
+{
+	struct adx_wdt *wdt = platform_get_drvdata(pdev);
+	adx_wdt_stop(wdt);
+}
+
+#ifdef CONFIG_PM
+static int adx_wdt_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct adx_wdt *wdt = platform_get_drvdata(pdev);
+
+	wdt->wake = (wdt->state == WDT_STATE_START) ? 1 : 0;
+	adx_wdt_stop(wdt);
+
+	return 0;
+}
+
+static int adx_wdt_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct adx_wdt *wdt = platform_get_drvdata(pdev);
+
+	if (wdt->wake)
+		adx_wdt_start(wdt);
+
+	return 0;
+}
+
+static struct dev_pm_ops adx_wdt_pm_ops = {
+	.suspend = adx_wdt_suspend,
+	.resume = adx_wdt_resume,
+};
+
+#  define ADX_WDT_PM_OPS	(&adx_wdt_pm_ops)
+#else
+#  define ADX_WDT_PM_OPS	NULL
+#endif
+
+static struct platform_driver adx_wdt_driver = {
+	.probe = adx_wdt_probe,
+	.remove = __devexit_p(adx_wdt_remove),
+	.shutdown = adx_wdt_shutdown,
+	.driver = {
+		.name = WATCHDOG_NAME,
+		.owner = THIS_MODULE,
+		.pm = ADX_WDT_PM_OPS,
+	},
+};
+
+static int __init adx_wdt_init(void)
+{
+	return platform_driver_register(&adx_wdt_driver);
+}
+
+static void __exit adx_wdt_exit(void)
+{
+	platform_driver_unregister(&adx_wdt_driver);
+}
+
+module_init(adx_wdt_init);
+module_exit(adx_wdt_exit);
+
+MODULE_DESCRIPTION("Avionic Design Xanthos Watchdog Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Thierry Reding <thierry.reding@avionic-design.de>");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
-- 
tg: (64f1607..) adx/watchdog (depends on: adx/master)

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Andrew Morton]

On Fri, 21 Aug 2009 15:10:02 +0200
Thierry Reding <thierry.reding@avionic-design.de> wrote:

> This patch adds support for the watchdog timer on Avionic Design Xanthos
> boards.
> 
>
> ...
>
> +static struct watchdog_info adx_wdt_info = {
> +	.identity = "Avionic Design Xanthos Watchdog",

heh, that fits with zero bytes to spare.

>
> ...
>
> +static const struct file_operations adx_wdt_fops = {
> +	.owner = THIS_MODULE,
> +	.llseek = no_llseek,
> +	.open = adx_wdt_open,
> +	.release = adx_wdt_release,
> +	.ioctl = adx_wdt_ioctl,

Nope.  All other watchdog drivers use .unlocked_ioctl and this one
should as well, please.

> +	.write = adx_wdt_write,
> +};

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Wim Van Sebroeck]

Hi Thierry,

> + * linux/drivers/watchdog/adx_wdt.c

Please change this to adx_wdt.c . If the tree get's moved then we don't want to review every driver to get this "fixed".

> +struct adx_wdt_nb {
> +	struct adx_wdt *wdt;
> +};

Is this being used?

> +static void adx_wdt_set_timeout(struct adx_wdt *wdt, unsigned long timeout)
> +{
> +	unsigned long flags;
> +	unsigned int state;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +	state = wdt->state;
> +	adx_wdt_stop_locked(wdt);
> +	writel(timeout, wdt->base + ADX_WDT_TIMEOUT);
> +
> +	if (state == WDT_STATE_START)
> +		adx_wdt_start_locked(wdt);
> +
> +	wdt->timeout = timeout;
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +}

You implemented the timeout towards userspace indeed as seconds. Internally you are using milliseconds.
I would prefer to see the conversion from seconds to milliseconds taking place in the adx_wdt_set_timeout function.
This will make conversion to the new watchdog api easier.

I just thought about having the internal timeout routines being in milliseconds.
Most watchdogs use seconds anyway, so that would mean a lot of multiply and divide by 1000 or additional code that isn't going to be a benefit. So I prefer to stick to seconds.

> +static int adx_wdt_open(struct inode *inode, struct file *file)
> +{
> +	struct adx_wdt *wdt = platform_get_drvdata(adx_wdt_dev);
> +
> +	if (test_and_set_bit(0, &driver_open))
> +		return -EBUSY;
> +
> +	adx_wdt_set_timeout(wdt, 30 * 1000);
> +	adx_wdt_start(wdt);
> +	file->private_data = wdt;
> +
> +	return nonseekable_open(inode, file);
> +}

I prefer to have file->private_data = wdt; to be set before you start the watchdog.

> +static int adx_wdt_ioctl(struct inode *inode, struct file *file,
> +		unsigned int cmd, unsigned long arg)

we use unlocked_ioctl. Ths should thus be:
static long adx_wdt_ioctl(struct file *file, unsigned int cmd,
						unsigned long arg)

> +{
> +	struct adx_wdt *wdt = file->private_data;
> +	void __user *argp = (void __user *)arg;
> +	unsigned long __user *p = argp;
> +	unsigned long timeout = 0;
> +	unsigned int options;
> +	int ret = -EINVAL;
> +
> +	switch (cmd) {
> +	case WDIOC_GETSUPPORT:
> +		if (copy_to_user(argp, &adx_wdt_info, sizeof(adx_wdt_info)))
> +			return -EFAULT;
> +		else
> +			return 0;
> +
> +	case WDIOC_GETSTATUS:
> +	case WDIOC_GETBOOTSTATUS:
> +		return put_user(!!(0), p);

Why not just "return put_user(0, p);"?

> +static const struct file_operations adx_wdt_fops = {
> +	.owner = THIS_MODULE,
> +	.llseek = no_llseek,
> +	.open = adx_wdt_open,
> +	.release = adx_wdt_release,
> +	.ioctl = adx_wdt_ioctl,

And then this becomes: .unlocked_ioctl = adx_wdt_ioctl,

> +	.write = adx_wdt_write,
> +};
> +
> +static struct miscdevice adx_wdt_miscdev = {
> +	.minor = WATCHDOG_MINOR,
> +	.name = "watchdog",
> +	.fops = &adx_wdt_fops,
> +};
> +
> +static irqreturn_t adx_wdt_interrupt(int irq, void *dev_id)
> +{
> +	struct adx_wdt *wdt = dev_id;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +	writel(wdt->timeout, wdt->base + ADX_WDT_TIMEOUT);
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	return IRQ_HANDLED;
> +}

What's this for? a watchdog driver is controlled by userspace and not retriggered via an interrupt.
It's userspace that needs to control wether or not we are in a stable situation or not.
See discussion with Wan ZongShun 2 to 3 weeks ago.

> +
> +static int __devinit adx_wdt_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct adx_wdt *wdt;
> +	int ret = 0;
> +
> +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt) {
> +		dev_err(&pdev->dev, "cannot allocate WDT structure\n");
> +		return -ENOMEM;
> +	}
> +
> +	spin_lock_init(&wdt->lock);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "cannot obtain I/O memory region\n");
> +		return -ENXIO;
> +	}
> +
> +	res = devm_request_mem_region(&pdev->dev, res->start,
> +			res->end - res->start + 1, res->name);
> +	if (!res) {
> +		dev_err(&pdev->dev, "cannot request I/O memory region\n");
> +		return -ENXIO;
> +	}
> +
> +	wdt->base = devm_ioremap_nocache(&pdev->dev, res->start,
> +			res->end - res->start + 1);
> +	if (!wdt->base) {
> +		dev_err(&pdev->dev, "cannot remap I/O memory region\n");
> +		return -ENXIO;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "cannot obtain IRQ\n");
> +		return -ENXIO;
> +	}
> +
> +	wdt->irq = res->start;
> +
> +	ret = devm_request_irq(&pdev->dev, wdt->irq, adx_wdt_interrupt,
> +			IRQF_SHARED, WATCHDOG_NAME, wdt);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "cannot request IRQ\n");
> +		return -ENXIO;
> +	}
> +
> +	ret = misc_register(&adx_wdt_miscdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "cannot register miscdev on minor %d "
> +				"(err=%d)\n", WATCHDOG_MINOR, ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +	adx_wdt_dev = pdev;

Please put these 2 lines in front of the misc_register.

> +
> +	return 0;
> +}
> +
> +static int __devexit adx_wdt_remove(struct platform_device *pdev)
> +{
> +	struct adx_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	platform_set_drvdata(pdev, NULL);
> +	misc_deregister(&adx_wdt_miscdev);
> +	adx_wdt_stop(wdt);

and put the platform_set_drvdata after the adx_wdt_stop.

> +
> +	return 0;
> +}
> +

Thanks in advance,
Wim.

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Thierry Reding]

* Wim Van Sebroeck wrote:
> Hi Thierry,
> 
> > + * linux/drivers/watchdog/adx_wdt.c
> 
> Please change this to adx_wdt.c . If the tree get's moved then we don't want to review every driver to get this "fixed".
> 
> > +struct adx_wdt_nb {
> > +	struct adx_wdt *wdt;
> > +};
> 
> Is this being used?

No. I missed it when replacing the reboot/shutdown notifier code.

> > +static void adx_wdt_set_timeout(struct adx_wdt *wdt, unsigned long timeout)
> > +{
> > +	unsigned long flags;
> > +	unsigned int state;
> > +
> > +	spin_lock_irqsave(&wdt->lock, flags);
> > +	state = wdt->state;
> > +	adx_wdt_stop_locked(wdt);
> > +	writel(timeout, wdt->base + ADX_WDT_TIMEOUT);
> > +
> > +	if (state == WDT_STATE_START)
> > +		adx_wdt_start_locked(wdt);
> > +
> > +	wdt->timeout = timeout;
> > +	spin_unlock_irqrestore(&wdt->lock, flags);
> > +}
> 
> You implemented the timeout towards userspace indeed as seconds. Internally you are using milliseconds.
> I would prefer to see the conversion from seconds to milliseconds taking place in the adx_wdt_set_timeout function.
> This will make conversion to the new watchdog api easier.
> 
> I just thought about having the internal timeout routines being in milliseconds.
> Most watchdogs use seconds anyway, so that would mean a lot of multiply and divide by 1000 or additional code that isn't going to be a benefit. So I prefer to stick to seconds.

Okay. I've moved the conversion to the adx_wdt_set_timeout() function and all
other code now passes seconds to that function (I also renamed the parameter
to "seconds" to clarify this).

> > +static int adx_wdt_open(struct inode *inode, struct file *file)
> > +{
> > +	struct adx_wdt *wdt = platform_get_drvdata(adx_wdt_dev);
> > +
> > +	if (test_and_set_bit(0, &driver_open))
> > +		return -EBUSY;
> > +
> > +	adx_wdt_set_timeout(wdt, 30 * 1000);
> > +	adx_wdt_start(wdt);
> > +	file->private_data = wdt;
> > +
> > +	return nonseekable_open(inode, file);
> > +}
> 
> I prefer to have file->private_data = wdt; to be set before you start the watchdog.

Done.

> > +static int adx_wdt_ioctl(struct inode *inode, struct file *file,
> > +		unsigned int cmd, unsigned long arg)
> 
> we use unlocked_ioctl. Ths should thus be:
> static long adx_wdt_ioctl(struct file *file, unsigned int cmd,
> 						unsigned long arg)

Done.

> > +{
> > +	struct adx_wdt *wdt = file->private_data;
> > +	void __user *argp = (void __user *)arg;
> > +	unsigned long __user *p = argp;
> > +	unsigned long timeout = 0;
> > +	unsigned int options;
> > +	int ret = -EINVAL;
> > +
> > +	switch (cmd) {
> > +	case WDIOC_GETSUPPORT:
> > +		if (copy_to_user(argp, &adx_wdt_info, sizeof(adx_wdt_info)))
> > +			return -EFAULT;
> > +		else
> > +			return 0;
> > +
> > +	case WDIOC_GETSTATUS:
> > +	case WDIOC_GETBOOTSTATUS:
> > +		return put_user(!!(0), p);
> 
> Why not just "return put_user(0, p);"?

Done.

> > +static const struct file_operations adx_wdt_fops = {
> > +	.owner = THIS_MODULE,
> > +	.llseek = no_llseek,
> > +	.open = adx_wdt_open,
> > +	.release = adx_wdt_release,
> > +	.ioctl = adx_wdt_ioctl,
> 
> And then this becomes: .unlocked_ioctl = adx_wdt_ioctl,

Done.

> > +	.write = adx_wdt_write,
> > +};
> > +
> > +static struct miscdevice adx_wdt_miscdev = {
> > +	.minor = WATCHDOG_MINOR,
> > +	.name = "watchdog",
> > +	.fops = &adx_wdt_fops,
> > +};
> > +
> > +static irqreturn_t adx_wdt_interrupt(int irq, void *dev_id)
> > +{
> > +	struct adx_wdt *wdt = dev_id;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&wdt->lock, flags);
> > +	writel(wdt->timeout, wdt->base + ADX_WDT_TIMEOUT);
> > +	spin_unlock_irqrestore(&wdt->lock, flags);
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> What's this for? a watchdog driver is controlled by userspace and not retriggered via an interrupt.
> It's userspace that needs to control wether or not we are in a stable situation or not.
> See discussion with Wan ZongShun 2 to 3 weeks ago.

The watchdog can also be programmed to only trigger an interrupt instead of
resetting the hardware on timeout. In that case the timeout register needns
to be written to clear the interrupt.

> > +
> > +static int __devinit adx_wdt_probe(struct platform_device *pdev)
> > +{
> > +	struct resource *res;
> > +	struct adx_wdt *wdt;
> > +	int ret = 0;
> > +
> > +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> > +	if (!wdt) {
> > +		dev_err(&pdev->dev, "cannot allocate WDT structure\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	spin_lock_init(&wdt->lock);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "cannot obtain I/O memory region\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	res = devm_request_mem_region(&pdev->dev, res->start,
> > +			res->end - res->start + 1, res->name);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "cannot request I/O memory region\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	wdt->base = devm_ioremap_nocache(&pdev->dev, res->start,
> > +			res->end - res->start + 1);
> > +	if (!wdt->base) {
> > +		dev_err(&pdev->dev, "cannot remap I/O memory region\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "cannot obtain IRQ\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	wdt->irq = res->start;
> > +
> > +	ret = devm_request_irq(&pdev->dev, wdt->irq, adx_wdt_interrupt,
> > +			IRQF_SHARED, WATCHDOG_NAME, wdt);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "cannot request IRQ\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	ret = misc_register(&adx_wdt_miscdev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "cannot register miscdev on minor %d "
> > +				"(err=%d)\n", WATCHDOG_MINOR, ret);
> > +		return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, wdt);
> > +	adx_wdt_dev = pdev;
> 
> Please put these 2 lines in front of the misc_register.

Done.

> > +
> > +	return 0;
> > +}
> > +
> > +static int __devexit adx_wdt_remove(struct platform_device *pdev)
> > +{
> > +	struct adx_wdt *wdt = platform_get_drvdata(pdev);
> > +
> > +	platform_set_drvdata(pdev, NULL);
> > +	misc_deregister(&adx_wdt_miscdev);
> > +	adx_wdt_stop(wdt);
> 
> and put the platform_set_drvdata after the adx_wdt_stop.

Done.

> > +
> > +	return 0;
> > +}
> > +
> 
> Thanks in advance,
> Wim.

Thanks for the review.
Thierry

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Thierry Reding]

This patch adds support for the watchdog timer on Avionic Design Xanthos
boards.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

---
 drivers/watchdog/Kconfig   |    7 +
 drivers/watchdog/Makefile  |    1 +
 drivers/watchdog/adx_wdt.c |  376 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 384 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b1ccc04..378d518 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -266,6 +266,13 @@ config STMP3XXX_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called stmp3xxx_wdt.
 
+config ADX_WATCHDOG
+	tristate "Avionic Design Xanthos watchdog"
+	depends on ARCH_PXA_ADX
+	help
+	  Say Y here if you want support for the watchdog timer on Avionic
+	  Design Xanthos boards.
+
 # AVR32 Architecture
 
 config AT32AP700X_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 3d77429..f91719f 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o
 obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o
 obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o
 obj-$(CONFIG_STMP3XXX_WATCHDOG) += stmp3xxx_wdt.o
+obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/adx_wdt.c b/drivers/watchdog/adx_wdt.c
new file mode 100644
index 0000000..2a43fd7
--- /dev/null
+++ b/drivers/watchdog/adx_wdt.c
@@ -0,0 +1,376 @@
+/*
+ * Copyright (C) 2008-2009 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/fs.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+
+#define WATCHDOG_NAME "adx-wdt"
+
+/* register offsets */
+#define	ADX_WDT_CONTROL		0x00
+#define	ADX_WDT_CONTROL_ENABLE	(1 << 0)
+#define	ADX_WDT_CONTROL_nRESET	(1 << 1)
+#define	ADX_WDT_TIMEOUT		0x08
+
+static struct platform_device *adx_wdt_dev;
+static unsigned long driver_open;
+
+#define	WDT_STATE_STOP	0
+#define	WDT_STATE_START	1
+
+struct adx_wdt {
+	void __iomem *base;
+	unsigned int irq;
+	unsigned long timeout;
+	unsigned int state;
+	unsigned int wake;
+	spinlock_t lock;
+};
+
+static struct watchdog_info adx_wdt_info = {
+	.identity = "Avionic Design Xanthos Watchdog",
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+};
+
+static void adx_wdt_start_locked(struct adx_wdt *wdt)
+{
+	u32 ctrl;
+
+	ctrl = readl(wdt->base + ADX_WDT_CONTROL);
+	ctrl |= ADX_WDT_CONTROL_ENABLE;
+	writel(ctrl, wdt->base + ADX_WDT_CONTROL);
+	wdt->state = WDT_STATE_START;
+}
+
+static void adx_wdt_start(struct adx_wdt *wdt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	adx_wdt_start_locked(wdt);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+}
+
+static void adx_wdt_stop_locked(struct adx_wdt *wdt)
+{
+	u32 ctrl;
+
+	ctrl = readl(wdt->base + ADX_WDT_CONTROL);
+	ctrl &= ~ADX_WDT_CONTROL_ENABLE;
+	writel(ctrl, wdt->base + ADX_WDT_CONTROL);
+	wdt->state = WDT_STATE_STOP;
+}
+
+static void adx_wdt_stop(struct adx_wdt *wdt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	adx_wdt_stop_locked(wdt);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+}
+
+static void adx_wdt_set_timeout(struct adx_wdt *wdt, unsigned long seconds)
+{
+	unsigned long timeout = seconds * 1000;
+	unsigned long flags;
+	unsigned int state;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	state = wdt->state;
+	adx_wdt_stop_locked(wdt);
+	writel(timeout, wdt->base + ADX_WDT_TIMEOUT);
+
+	if (state == WDT_STATE_START)
+		adx_wdt_start_locked(wdt);
+
+	wdt->timeout = timeout;
+	spin_unlock_irqrestore(&wdt->lock, flags);
+}
+
+static void adx_wdt_get_timeout(struct adx_wdt *wdt, unsigned long *seconds)
+{
+	*seconds = wdt->timeout / 1000;
+}
+
+static void adx_wdt_keepalive(struct adx_wdt *wdt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	writel(wdt->timeout, wdt->base + ADX_WDT_TIMEOUT);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+}
+
+static int adx_wdt_open(struct inode *inode, struct file *file)
+{
+	struct adx_wdt *wdt = platform_get_drvdata(adx_wdt_dev);
+
+	if (test_and_set_bit(0, &driver_open))
+		return -EBUSY;
+
+	file->private_data = wdt;
+	adx_wdt_set_timeout(wdt, 30);
+	adx_wdt_start(wdt);
+
+	return nonseekable_open(inode, file);
+}
+
+static int adx_wdt_release(struct inode *inode, struct file *file)
+{
+	struct adx_wdt *wdt = file->private_data;
+
+	adx_wdt_stop(wdt);
+	clear_bit(0, &driver_open);
+
+	return 0;
+}
+
+static long adx_wdt_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct adx_wdt *wdt = file->private_data;
+	void __user *argp = (void __user *)arg;
+	unsigned long __user *p = argp;
+	unsigned long seconds = 0;
+	unsigned int options;
+	long ret = -EINVAL;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		if (copy_to_user(argp, &adx_wdt_info, sizeof(adx_wdt_info)))
+			return -EFAULT;
+		else
+			return 0;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, p);
+
+	case WDIOC_KEEPALIVE:
+		adx_wdt_keepalive(wdt);
+		return 0;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(seconds, p))
+			return -EFAULT;
+
+		adx_wdt_set_timeout(wdt, seconds);
+
+		/* fallthrough */
+	case WDIOC_GETTIMEOUT:
+		adx_wdt_get_timeout(wdt, &seconds);
+		return put_user(seconds, p);
+
+	case WDIOC_SETOPTIONS:
+		if (copy_from_user(&options, argp, sizeof(options)))
+			return -EFAULT;
+
+		if (options & WDIOS_DISABLECARD) {
+			adx_wdt_stop(wdt);
+			ret = 0;
+		}
+
+		if (options & WDIOS_ENABLECARD) {
+			adx_wdt_start(wdt);
+			ret = 0;
+		}
+
+		return ret;
+
+	default:
+		break;
+	}
+
+	return -ENOTTY;
+}
+
+static ssize_t adx_wdt_write(struct file *file, const char __user *data,
+		size_t len, loff_t *ppos)
+{
+	struct adx_wdt *wdt = file->private_data;
+
+	if (len)
+		adx_wdt_keepalive(wdt);
+
+	return len;
+}
+
+static const struct file_operations adx_wdt_fops = {
+	.owner = THIS_MODULE,
+	.llseek = no_llseek,
+	.open = adx_wdt_open,
+	.release = adx_wdt_release,
+	.unlocked_ioctl = adx_wdt_ioctl,
+	.write = adx_wdt_write,
+};
+
+static struct miscdevice adx_wdt_miscdev = {
+	.minor = WATCHDOG_MINOR,
+	.name = "watchdog",
+	.fops = &adx_wdt_fops,
+};
+
+static irqreturn_t adx_wdt_interrupt(int irq, void *dev_id)
+{
+	struct adx_wdt *wdt = dev_id;
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	writel(wdt->timeout, wdt->base + ADX_WDT_TIMEOUT);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
+static int __devinit adx_wdt_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct adx_wdt *wdt;
+	int ret = 0;
+
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt) {
+		dev_err(&pdev->dev, "cannot allocate WDT structure\n");
+		return -ENOMEM;
+	}
+
+	spin_lock_init(&wdt->lock);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "cannot obtain I/O memory region\n");
+		return -ENXIO;
+	}
+
+	res = devm_request_mem_region(&pdev->dev, res->start,
+			res->end - res->start + 1, res->name);
+	if (!res) {
+		dev_err(&pdev->dev, "cannot request I/O memory region\n");
+		return -ENXIO;
+	}
+
+	wdt->base = devm_ioremap_nocache(&pdev->dev, res->start,
+			res->end - res->start + 1);
+	if (!wdt->base) {
+		dev_err(&pdev->dev, "cannot remap I/O memory region\n");
+		return -ENXIO;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "cannot obtain IRQ\n");
+		return -ENXIO;
+	}
+
+	wdt->irq = res->start;
+
+	ret = devm_request_irq(&pdev->dev, wdt->irq, adx_wdt_interrupt,
+			IRQF_SHARED, WATCHDOG_NAME, wdt);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "cannot request IRQ\n");
+		return -ENXIO;
+	}
+
+	platform_set_drvdata(pdev, wdt);
+	adx_wdt_dev = pdev;
+
+	ret = misc_register(&adx_wdt_miscdev);
+	if (ret) {
+		dev_err(&pdev->dev, "cannot register miscdev on minor %d "
+				"(err=%d)\n", WATCHDOG_MINOR, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __devexit adx_wdt_remove(struct platform_device *pdev)
+{
+	struct adx_wdt *wdt = platform_get_drvdata(pdev);
+
+	misc_deregister(&adx_wdt_miscdev);
+	adx_wdt_stop(wdt);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static void adx_wdt_shutdown(struct platform_device *pdev)
+{
+	struct adx_wdt *wdt = platform_get_drvdata(pdev);
+	adx_wdt_stop(wdt);
+}
+
+#ifdef CONFIG_PM
+static int adx_wdt_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct adx_wdt *wdt = platform_get_drvdata(pdev);
+
+	wdt->wake = (wdt->state == WDT_STATE_START) ? 1 : 0;
+	adx_wdt_stop(wdt);
+
+	return 0;
+}
+
+static int adx_wdt_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct adx_wdt *wdt = platform_get_drvdata(pdev);
+
+	if (wdt->wake)
+		adx_wdt_start(wdt);
+
+	return 0;
+}
+
+static struct dev_pm_ops adx_wdt_pm_ops = {
+	.suspend = adx_wdt_suspend,
+	.resume = adx_wdt_resume,
+};
+
+#  define ADX_WDT_PM_OPS	(&adx_wdt_pm_ops)
+#else
+#  define ADX_WDT_PM_OPS	NULL
+#endif
+
+static struct platform_driver adx_wdt_driver = {
+	.probe = adx_wdt_probe,
+	.remove = __devexit_p(adx_wdt_remove),
+	.shutdown = adx_wdt_shutdown,
+	.driver = {
+		.name = WATCHDOG_NAME,
+		.owner = THIS_MODULE,
+		.pm = ADX_WDT_PM_OPS,
+	},
+};
+
+static int __init adx_wdt_init(void)
+{
+	return platform_driver_register(&adx_wdt_driver);
+}
+
+static void __exit adx_wdt_exit(void)
+{
+	platform_driver_unregister(&adx_wdt_driver);
+}
+
+module_init(adx_wdt_init);
+module_exit(adx_wdt_exit);
+
+MODULE_DESCRIPTION("Avionic Design Xanthos Watchdog Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Thierry Reding <thierry.reding@avionic-design.de>");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
-- 
tg: (422bef8..) adx/watchdog (depends on: adx/master)

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Wim Van Sebroeck]

Hi Thierry,

> > > +static irqreturn_t adx_wdt_interrupt(int irq, void *dev_id)
> > > +{
> > > +	struct adx_wdt *wdt = dev_id;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&wdt->lock, flags);
> > > +	writel(wdt->timeout, wdt->base + ADX_WDT_TIMEOUT);
> > > +	spin_unlock_irqrestore(&wdt->lock, flags);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > 
> > What's this for? a watchdog driver is controlled by userspace and not retriggered via an interrupt.
> > It's userspace that needs to control wether or not we are in a stable situation or not.
> > See discussion with Wan ZongShun 2 to 3 weeks ago.
> 
> The watchdog can also be programmed to only trigger an interrupt instead of
> resetting the hardware on timeout. In that case the timeout register needns
> to be written to clear the interrupt.

This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?

Thanks in advance,
Wim.

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Bill Gatliff]

Wim Van Sebroeck wrote:
> This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
>   

... because a hard reboot might not be the way he wants to resolve the 
situation.


b.g.

-- 
Bill Gatliff
bgat at billgatliff.com

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Wim Van Sebroeck]

Hi Bill,

>> This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
>>   
>
> ... because a hard reboot might not be the way he wants to resolve the  
> situation.

If your system is unstable then sending a keepalive to the watchdog during this interrupt is not going to change anything neither... Or am I missing something?

Kind regards,
Wim.

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Bill Gatliff]

Wim Van Sebroeck wrote:
> Hi Bill,
>
>   
>>> This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
>>>   
>>>       
>> ... because a hard reboot might not be the way he wants to resolve the  
>> situation.
>>     
>
> If your system is unstable then sending a keepalive to the watchdog during this interrupt is not going to change anything neither... Or am I missing something?
>   

All I'm saying is that it isn't always a given that a watchdog timeout 
should universally issue a hardware reset.  I've worked with systems 
where doing so would be a Really Bad Idea, and what we wanted instead 
was the ability to know when the watchdog timer had expired so that we 
could wrest control of the system away from the application.

Put another way, it isn't always the case that the _system_ has become 
unstable.  It's more likely that the _application_ has, and there are 
other ways to fix that than by issuing a hardware reset.

In the bigger picture, getting all the benefits out of a watchdog timer 
while at the same time avoiding their limitations can be a tricky 
business.  They're misused far more often than used properly, 
unfortunately.  I sense that you and I both recognize that.


b.g.

-- 
Bill Gatliff
bgat at billgatliff.com

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Wim Van Sebroeck]

Hi Bill,

>>>> This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
>>>>         
>>> ... because a hard reboot might not be the way he wants to resolve 
>>> the  situation.
>>>     
>>
>> If your system is unstable then sending a keepalive to the watchdog during this interrupt is not going to change anything neither... Or am I missing something?
>>   
>
> All I'm saying is that it isn't always a given that a watchdog timeout  
> should universally issue a hardware reset.  I've worked with systems  
> where doing so would be a Really Bad Idea, and what we wanted instead  
> was the ability to know when the watchdog timer had expired so that we  
> could wrest control of the system away from the application.
>
> Put another way, it isn't always the case that the _system_ has become  
> unstable.  It's more likely that the _application_ has, and there are  
> other ways to fix that than by issuing a hardware reset.
>
> In the bigger picture, getting all the benefits out of a watchdog timer  
> while at the same time avoiding their limitations can be a tricky  
> business.  They're misused far more often than used properly,  
> unfortunately.  I sense that you and I both recognize that.

The original watchdog API was designed to make sure that unstable systems, systems that are going in a deadlock situation, can be recovered by a reboot.
And this is still how we use the watchdog API. The goal is to keep the system running. 

So if (and I can imagine that certain embedded applications might need this) a system is wanted to restart unstable applications, then (for me) this means that you want another functionality then what we have now. This can be an extension of the current watchdog API, but it's not said that this is the best solution.

For me the core function of a linux watchdog device driver is and remains to make sure that a system get's rebooted if it crashes.
Monitoring applications and other pheripherals looks to me additional finetuning of your system (and can indeed be usefull or needed). For applications monitoring there are imho other tools allready available (example: we used a heavily modified version of spong in the late 90's to test mail, dns, radius, ...). 

Kind regards,
Wim.

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Mark Brown]

On Mon, Aug 31, 2009 at 02:35:00PM +0200, Wim Van Sebroeck wrote:

> So if (and I can imagine that certain embedded applications might need
> this) a system is wanted to restart unstable applications, then (for me)
> this means that you want another functionality then what we have now.
> This can be an extension of the current watchdog API, but it's not said
> that this is the best solution.

Given the presentation of the hardware there's a fairly strong
expectation from the user side that the functionality would appear via
the watchdog API.  In common configurations there's a very strong
integration between the less serious watchdog actions and the shutdowns
or resets that they generate so trying to run them via different APIs
would cause issues - each API would want some idea what the other was
doing.

Indeed, one common use of the options that don't cause a hard reset or
shutdown is to extend the length of time that the watchdog can run for -
watchdogs that support this functionality normally take configurable
actions on both of the first two expiries of the watchdog (sometimes
more).  A system might configure the first expiry to generate an
interrupt and only reset the system if the watchdog still hasn't been
updated after another period.  Sometimes the first period will be used
to try to give the system a chance to recover more gracefully than a
hard reset allows but a lot of the time it'll just be ignored so the
watchdog can run for longer before it resets.

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Thierry Reding]

* Wim Van Sebroeck wrote:
> Hi Bill,
> 
> >> This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
> >>   
> >
> > ... because a hard reboot might not be the way he wants to resolve the  
> > situation.
> 
> If your system is unstable then sending a keepalive to the watchdog during
> this interrupt is not going to change anything neither... Or am I missing
> something?

In this case, writing the timeout value to the watchdog is not actually a
keepalive. This is in fact the mechanism used to reset/acknowledge the
watchdog's interrupt.

Thierry

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Wim Van Sebroeck]

Hi Thierry,

> > >> This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
> > >>   
> > >
> > > ... because a hard reboot might not be the way he wants to resolve the  
> > > situation.
> > 
> > If your system is unstable then sending a keepalive to the watchdog during
> > this interrupt is not going to change anything neither... Or am I missing
> > something?
> 
> In this case, writing the timeout value to the watchdog is not actually a
> keepalive. This is in fact the mechanism used to reset/acknowledge the
> watchdog's interrupt.

I'm still puzzled. I have seen a number of watchdog devices that have the option to either reboot the system or generate an interrupt.
They can be set via toggling the correct bits of some configuration register.

How does this hardware works? Is there a datasheet available somewhere?

Kind regards,
Wim.

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Thierry Reding]

* Wim Van Sebroeck wrote:
> Hi Thierry,
> 
> > > >> This is the only thing I'm still struggling with: the goal of a watchdog device driver is to make sure that the system reboots when the system isn't stable anymore. So why should you then trigger the interrupt?
> > > >>   
> > > >
> > > > ... because a hard reboot might not be the way he wants to resolve the  
> > > > situation.
> > > 
> > > If your system is unstable then sending a keepalive to the watchdog during
> > > this interrupt is not going to change anything neither... Or am I missing
> > > something?
> > 
> > In this case, writing the timeout value to the watchdog is not actually a
> > keepalive. This is in fact the mechanism used to reset/acknowledge the
> > watchdog's interrupt.
> 
> I'm still puzzled. I have seen a number of watchdog devices that have the option to either reboot the system or generate an interrupt.
> They can be set via toggling the correct bits of some configuration register.
> 
> How does this hardware works? Is there a datasheet available somewhere?

It works exactly the same way. The bit defined by ADX_WDT_CONTROL_nRESET
controls this behaviour. If set, the watchdog will only generate an
interrupt, if cleared (the default) it will reboot the system.

There's currently no real datasheet available. The register definitions at
the top of adx_wdt.c are pretty much all of the documentation we currently
have, I'm afraid.

> Kind regards,
> Wim.

Thierry

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Wim Van Sebroeck]

Hi Thierry,

> > I'm still puzzled. I have seen a number of watchdog devices that have the option to either reboot the system or generate an interrupt.
> > They can be set via toggling the correct bits of some configuration register.
> > 
> > How does this hardware works? Is there a datasheet available somewhere?
> 
> It works exactly the same way. The bit defined by ADX_WDT_CONTROL_nRESET
> controls this behaviour. If set, the watchdog will only generate an
> interrupt, if cleared (the default) it will reboot the system.
> 
> There's currently no real datasheet available. The register definitions at
> the top of adx_wdt.c are pretty much all of the documentation we currently
> have, I'm afraid.

I looked at the register definitions and then searched in your code where
the ADX_WDT_CONTROL_nRESET bit of the watchdog device was initialized.
I didn't find it.

So to get this driver in line with how the other drivers work, we should still
do the following:
1) initialize the device at startup so that the ADX_WDT_CONTROL_nRESET is set correctly
(and the device should also be disabled if it isn't done yet).
2) we can get rid of the interrupt handling because we will use the reboot behaviour.

Can you adapt your code?

Thanks in advance,
Wim.

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Thierry Reding]

This patch adds support for the watchdog timer on Avionic Design Xanthos
boards.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

---
 drivers/watchdog/Kconfig   |    7 +
 drivers/watchdog/Makefile  |    1 +
 drivers/watchdog/adx_wdt.c |  354 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 362 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b1ccc04..378d518 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -266,6 +266,13 @@ config STMP3XXX_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called stmp3xxx_wdt.
 
+config ADX_WATCHDOG
+	tristate "Avionic Design Xanthos watchdog"
+	depends on ARCH_PXA_ADX
+	help
+	  Say Y here if you want support for the watchdog timer on Avionic
+	  Design Xanthos boards.
+
 # AVR32 Architecture
 
 config AT32AP700X_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 3d77429..f91719f 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o
 obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o
 obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o
 obj-$(CONFIG_STMP3XXX_WATCHDOG) += stmp3xxx_wdt.o
+obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/adx_wdt.c b/drivers/watchdog/adx_wdt.c
new file mode 100644
index 0000000..77afb0a
--- /dev/null
+++ b/drivers/watchdog/adx_wdt.c
@@ -0,0 +1,354 @@
+/*
+ * Copyright (C) 2008-2009 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/fs.h>
+#include <linux/io.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+
+#define WATCHDOG_NAME "adx-wdt"
+
+/* register offsets */
+#define	ADX_WDT_CONTROL		0x00
+#define	ADX_WDT_CONTROL_ENABLE	(1 << 0)
+#define	ADX_WDT_CONTROL_nRESET	(1 << 1)
+#define	ADX_WDT_TIMEOUT		0x08
+
+static struct platform_device *adx_wdt_dev;
+static unsigned long driver_open;
+
+#define	WDT_STATE_STOP	0
+#define	WDT_STATE_START	1
+
+struct adx_wdt {
+	void __iomem *base;
+	unsigned long timeout;
+	unsigned int state;
+	unsigned int wake;
+	spinlock_t lock;
+};
+
+static struct watchdog_info adx_wdt_info = {
+	.identity = "Avionic Design Xanthos Watchdog",
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+};
+
+static void adx_wdt_start_locked(struct adx_wdt *wdt)
+{
+	u32 ctrl;
+
+	ctrl = readl(wdt->base + ADX_WDT_CONTROL);
+	ctrl |= ADX_WDT_CONTROL_ENABLE;
+	writel(ctrl, wdt->base + ADX_WDT_CONTROL);
+	wdt->state = WDT_STATE_START;
+}
+
+static void adx_wdt_start(struct adx_wdt *wdt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	adx_wdt_start_locked(wdt);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+}
+
+static void adx_wdt_stop_locked(struct adx_wdt *wdt)
+{
+	u32 ctrl;
+
+	ctrl = readl(wdt->base + ADX_WDT_CONTROL);
+	ctrl &= ~ADX_WDT_CONTROL_ENABLE;
+	writel(ctrl, wdt->base + ADX_WDT_CONTROL);
+	wdt->state = WDT_STATE_STOP;
+}
+
+static void adx_wdt_stop(struct adx_wdt *wdt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	adx_wdt_stop_locked(wdt);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+}
+
+static void adx_wdt_set_timeout(struct adx_wdt *wdt, unsigned long seconds)
+{
+	unsigned long timeout = seconds * 1000;
+	unsigned long flags;
+	unsigned int state;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	state = wdt->state;
+	adx_wdt_stop_locked(wdt);
+	writel(timeout, wdt->base + ADX_WDT_TIMEOUT);
+
+	if (state == WDT_STATE_START)
+		adx_wdt_start_locked(wdt);
+
+	wdt->timeout = timeout;
+	spin_unlock_irqrestore(&wdt->lock, flags);
+}
+
+static void adx_wdt_get_timeout(struct adx_wdt *wdt, unsigned long *seconds)
+{
+	*seconds = wdt->timeout / 1000;
+}
+
+static void adx_wdt_keepalive(struct adx_wdt *wdt)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	writel(wdt->timeout, wdt->base + ADX_WDT_TIMEOUT);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+}
+
+static int adx_wdt_open(struct inode *inode, struct file *file)
+{
+	struct adx_wdt *wdt = platform_get_drvdata(adx_wdt_dev);
+
+	if (test_and_set_bit(0, &driver_open))
+		return -EBUSY;
+
+	file->private_data = wdt;
+	adx_wdt_set_timeout(wdt, 30);
+	adx_wdt_start(wdt);
+
+	return nonseekable_open(inode, file);
+}
+
+static int adx_wdt_release(struct inode *inode, struct file *file)
+{
+	struct adx_wdt *wdt = file->private_data;
+
+	adx_wdt_stop(wdt);
+	clear_bit(0, &driver_open);
+
+	return 0;
+}
+
+static long adx_wdt_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct adx_wdt *wdt = file->private_data;
+	void __user *argp = (void __user *)arg;
+	unsigned long __user *p = argp;
+	unsigned long seconds = 0;
+	unsigned int options;
+	long ret = -EINVAL;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		if (copy_to_user(argp, &adx_wdt_info, sizeof(adx_wdt_info)))
+			return -EFAULT;
+		else
+			return 0;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, p);
+
+	case WDIOC_KEEPALIVE:
+		adx_wdt_keepalive(wdt);
+		return 0;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(seconds, p))
+			return -EFAULT;
+
+		adx_wdt_set_timeout(wdt, seconds);
+
+		/* fallthrough */
+	case WDIOC_GETTIMEOUT:
+		adx_wdt_get_timeout(wdt, &seconds);
+		return put_user(seconds, p);
+
+	case WDIOC_SETOPTIONS:
+		if (copy_from_user(&options, argp, sizeof(options)))
+			return -EFAULT;
+
+		if (options & WDIOS_DISABLECARD) {
+			adx_wdt_stop(wdt);
+			ret = 0;
+		}
+
+		if (options & WDIOS_ENABLECARD) {
+			adx_wdt_start(wdt);
+			ret = 0;
+		}
+
+		return ret;
+
+	default:
+		break;
+	}
+
+	return -ENOTTY;
+}
+
+static ssize_t adx_wdt_write(struct file *file, const char __user *data,
+		size_t len, loff_t *ppos)
+{
+	struct adx_wdt *wdt = file->private_data;
+
+	if (len)
+		adx_wdt_keepalive(wdt);
+
+	return len;
+}
+
+static const struct file_operations adx_wdt_fops = {
+	.owner = THIS_MODULE,
+	.llseek = no_llseek,
+	.open = adx_wdt_open,
+	.release = adx_wdt_release,
+	.unlocked_ioctl = adx_wdt_ioctl,
+	.write = adx_wdt_write,
+};
+
+static struct miscdevice adx_wdt_miscdev = {
+	.minor = WATCHDOG_MINOR,
+	.name = "watchdog",
+	.fops = &adx_wdt_fops,
+};
+
+static int __devinit adx_wdt_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct adx_wdt *wdt;
+	int ret = 0;
+	u32 ctrl;
+
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt) {
+		dev_err(&pdev->dev, "cannot allocate WDT structure\n");
+		return -ENOMEM;
+	}
+
+	spin_lock_init(&wdt->lock);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "cannot obtain I/O memory region\n");
+		return -ENXIO;
+	}
+
+	res = devm_request_mem_region(&pdev->dev, res->start,
+			res->end - res->start + 1, res->name);
+	if (!res) {
+		dev_err(&pdev->dev, "cannot request I/O memory region\n");
+		return -ENXIO;
+	}
+
+	wdt->base = devm_ioremap_nocache(&pdev->dev, res->start,
+			res->end - res->start + 1);
+	if (!wdt->base) {
+		dev_err(&pdev->dev, "cannot remap I/O memory region\n");
+		return -ENXIO;
+	}
+
+	/* disable watchdog and reboot on timeout */
+	ctrl = readl(wdt->base + ADX_WDT_CONTROL);
+	ctrl &= ~ADX_WDT_CONTROL_ENABLE;
+	ctrl &= ~ADX_WDT_CONTROL_nRESET;
+	writel(ctrl, wdt->base + ADX_WDT_CONTROL);
+
+	platform_set_drvdata(pdev, wdt);
+	adx_wdt_dev = pdev;
+
+	ret = misc_register(&adx_wdt_miscdev);
+	if (ret) {
+		dev_err(&pdev->dev, "cannot register miscdev on minor %d "
+				"(err=%d)\n", WATCHDOG_MINOR, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __devexit adx_wdt_remove(struct platform_device *pdev)
+{
+	struct adx_wdt *wdt = platform_get_drvdata(pdev);
+
+	misc_deregister(&adx_wdt_miscdev);
+	adx_wdt_stop(wdt);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static void adx_wdt_shutdown(struct platform_device *pdev)
+{
+	struct adx_wdt *wdt = platform_get_drvdata(pdev);
+	adx_wdt_stop(wdt);
+}
+
+#ifdef CONFIG_PM
+static int adx_wdt_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct adx_wdt *wdt = platform_get_drvdata(pdev);
+
+	wdt->wake = (wdt->state == WDT_STATE_START) ? 1 : 0;
+	adx_wdt_stop(wdt);
+
+	return 0;
+}
+
+static int adx_wdt_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct adx_wdt *wdt = platform_get_drvdata(pdev);
+
+	if (wdt->wake)
+		adx_wdt_start(wdt);
+
+	return 0;
+}
+
+static struct dev_pm_ops adx_wdt_pm_ops = {
+	.suspend = adx_wdt_suspend,
+	.resume = adx_wdt_resume,
+};
+
+#  define ADX_WDT_PM_OPS	(&adx_wdt_pm_ops)
+#else
+#  define ADX_WDT_PM_OPS	NULL
+#endif
+
+static struct platform_driver adx_wdt_driver = {
+	.probe = adx_wdt_probe,
+	.remove = __devexit_p(adx_wdt_remove),
+	.shutdown = adx_wdt_shutdown,
+	.driver = {
+		.name = WATCHDOG_NAME,
+		.owner = THIS_MODULE,
+		.pm = ADX_WDT_PM_OPS,
+	},
+};
+
+static int __init adx_wdt_init(void)
+{
+	return platform_driver_register(&adx_wdt_driver);
+}
+
+static void __exit adx_wdt_exit(void)
+{
+	platform_driver_unregister(&adx_wdt_driver);
+}
+
+module_init(adx_wdt_init);
+module_exit(adx_wdt_exit);
+
+MODULE_DESCRIPTION("Avionic Design Xanthos Watchdog Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Thierry Reding <thierry.reding@avionic-design.de>");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
-- 
tg: (74fca6a..) adx/watchdog (depends on: adx/master)

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Wim Van Sebroeck]

Hi Thierry,

it's in the linux-watchdog-next tree now.

Kind regards,
Wim.

> This patch adds support for the watchdog timer on Avionic Design Xanthos
> boards.
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> ---
>  drivers/watchdog/Kconfig   |    7 +
>  drivers/watchdog/Makefile  |    1 +
>  drivers/watchdog/adx_wdt.c |  354 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 362 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index b1ccc04..378d518 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -266,6 +266,13 @@ config STMP3XXX_WATCHDOG
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called stmp3xxx_wdt.
>  
> +config ADX_WATCHDOG
> +	tristate "Avionic Design Xanthos watchdog"
> +	depends on ARCH_PXA_ADX
> +	help
> +	  Say Y here if you want support for the watchdog timer on Avionic
> +	  Design Xanthos boards.
> +
>  # AVR32 Architecture
>  
>  config AT32AP700X_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 3d77429..f91719f 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -44,6 +44,7 @@ obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o
>  obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o
>  obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o
>  obj-$(CONFIG_STMP3XXX_WATCHDOG) += stmp3xxx_wdt.o
> +obj-$(CONFIG_ADX_WATCHDOG) += adx_wdt.o
>  
>  # AVR32 Architecture
>  obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/adx_wdt.c b/drivers/watchdog/adx_wdt.c
> new file mode 100644
> index 0000000..77afb0a
> --- /dev/null
> +++ b/drivers/watchdog/adx_wdt.c
> @@ -0,0 +1,354 @@
> +/*
> + * Copyright (C) 2008-2009 Avionic Design GmbH
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/io.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +#include <linux/watchdog.h>
> +
> +#define WATCHDOG_NAME "adx-wdt"
> +
> +/* register offsets */
> +#define	ADX_WDT_CONTROL		0x00
> +#define	ADX_WDT_CONTROL_ENABLE	(1 << 0)
> +#define	ADX_WDT_CONTROL_nRESET	(1 << 1)
> +#define	ADX_WDT_TIMEOUT		0x08
> +
> +static struct platform_device *adx_wdt_dev;
> +static unsigned long driver_open;
> +
> +#define	WDT_STATE_STOP	0
> +#define	WDT_STATE_START	1
> +
> +struct adx_wdt {
> +	void __iomem *base;
> +	unsigned long timeout;
> +	unsigned int state;
> +	unsigned int wake;
> +	spinlock_t lock;
> +};
> +
> +static struct watchdog_info adx_wdt_info = {
> +	.identity = "Avionic Design Xanthos Watchdog",
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> +};
> +
> +static void adx_wdt_start_locked(struct adx_wdt *wdt)
> +{
> +	u32 ctrl;
> +
> +	ctrl = readl(wdt->base + ADX_WDT_CONTROL);
> +	ctrl |= ADX_WDT_CONTROL_ENABLE;
> +	writel(ctrl, wdt->base + ADX_WDT_CONTROL);
> +	wdt->state = WDT_STATE_START;
> +}
> +
> +static void adx_wdt_start(struct adx_wdt *wdt)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +	adx_wdt_start_locked(wdt);
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +}
> +
> +static void adx_wdt_stop_locked(struct adx_wdt *wdt)
> +{
> +	u32 ctrl;
> +
> +	ctrl = readl(wdt->base + ADX_WDT_CONTROL);
> +	ctrl &= ~ADX_WDT_CONTROL_ENABLE;
> +	writel(ctrl, wdt->base + ADX_WDT_CONTROL);
> +	wdt->state = WDT_STATE_STOP;
> +}
> +
> +static void adx_wdt_stop(struct adx_wdt *wdt)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +	adx_wdt_stop_locked(wdt);
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +}
> +
> +static void adx_wdt_set_timeout(struct adx_wdt *wdt, unsigned long seconds)
> +{
> +	unsigned long timeout = seconds * 1000;
> +	unsigned long flags;
> +	unsigned int state;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +	state = wdt->state;
> +	adx_wdt_stop_locked(wdt);
> +	writel(timeout, wdt->base + ADX_WDT_TIMEOUT);
> +
> +	if (state == WDT_STATE_START)
> +		adx_wdt_start_locked(wdt);
> +
> +	wdt->timeout = timeout;
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +}
> +
> +static void adx_wdt_get_timeout(struct adx_wdt *wdt, unsigned long *seconds)
> +{
> +	*seconds = wdt->timeout / 1000;
> +}
> +
> +static void adx_wdt_keepalive(struct adx_wdt *wdt)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +	writel(wdt->timeout, wdt->base + ADX_WDT_TIMEOUT);
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +}
> +
> +static int adx_wdt_open(struct inode *inode, struct file *file)
> +{
> +	struct adx_wdt *wdt = platform_get_drvdata(adx_wdt_dev);
> +
> +	if (test_and_set_bit(0, &driver_open))
> +		return -EBUSY;
> +
> +	file->private_data = wdt;
> +	adx_wdt_set_timeout(wdt, 30);
> +	adx_wdt_start(wdt);
> +
> +	return nonseekable_open(inode, file);
> +}
> +
> +static int adx_wdt_release(struct inode *inode, struct file *file)
> +{
> +	struct adx_wdt *wdt = file->private_data;
> +
> +	adx_wdt_stop(wdt);
> +	clear_bit(0, &driver_open);
> +
> +	return 0;
> +}
> +
> +static long adx_wdt_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	struct adx_wdt *wdt = file->private_data;
> +	void __user *argp = (void __user *)arg;
> +	unsigned long __user *p = argp;
> +	unsigned long seconds = 0;
> +	unsigned int options;
> +	long ret = -EINVAL;
> +
> +	switch (cmd) {
> +	case WDIOC_GETSUPPORT:
> +		if (copy_to_user(argp, &adx_wdt_info, sizeof(adx_wdt_info)))
> +			return -EFAULT;
> +		else
> +			return 0;
> +
> +	case WDIOC_GETSTATUS:
> +	case WDIOC_GETBOOTSTATUS:
> +		return put_user(0, p);
> +
> +	case WDIOC_KEEPALIVE:
> +		adx_wdt_keepalive(wdt);
> +		return 0;
> +
> +	case WDIOC_SETTIMEOUT:
> +		if (get_user(seconds, p))
> +			return -EFAULT;
> +
> +		adx_wdt_set_timeout(wdt, seconds);
> +
> +		/* fallthrough */
> +	case WDIOC_GETTIMEOUT:
> +		adx_wdt_get_timeout(wdt, &seconds);
> +		return put_user(seconds, p);
> +
> +	case WDIOC_SETOPTIONS:
> +		if (copy_from_user(&options, argp, sizeof(options)))
> +			return -EFAULT;
> +
> +		if (options & WDIOS_DISABLECARD) {
> +			adx_wdt_stop(wdt);
> +			ret = 0;
> +		}
> +
> +		if (options & WDIOS_ENABLECARD) {
> +			adx_wdt_start(wdt);
> +			ret = 0;
> +		}
> +
> +		return ret;
> +
> +	default:
> +		break;
> +	}
> +
> +	return -ENOTTY;
> +}
> +
> +static ssize_t adx_wdt_write(struct file *file, const char __user *data,
> +		size_t len, loff_t *ppos)
> +{
> +	struct adx_wdt *wdt = file->private_data;
> +
> +	if (len)
> +		adx_wdt_keepalive(wdt);
> +
> +	return len;
> +}
> +
> +static const struct file_operations adx_wdt_fops = {
> +	.owner = THIS_MODULE,
> +	.llseek = no_llseek,
> +	.open = adx_wdt_open,
> +	.release = adx_wdt_release,
> +	.unlocked_ioctl = adx_wdt_ioctl,
> +	.write = adx_wdt_write,
> +};
> +
> +static struct miscdevice adx_wdt_miscdev = {
> +	.minor = WATCHDOG_MINOR,
> +	.name = "watchdog",
> +	.fops = &adx_wdt_fops,
> +};
> +
> +static int __devinit adx_wdt_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct adx_wdt *wdt;
> +	int ret = 0;
> +	u32 ctrl;
> +
> +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt) {
> +		dev_err(&pdev->dev, "cannot allocate WDT structure\n");
> +		return -ENOMEM;
> +	}
> +
> +	spin_lock_init(&wdt->lock);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "cannot obtain I/O memory region\n");
> +		return -ENXIO;
> +	}
> +
> +	res = devm_request_mem_region(&pdev->dev, res->start,
> +			res->end - res->start + 1, res->name);
> +	if (!res) {
> +		dev_err(&pdev->dev, "cannot request I/O memory region\n");
> +		return -ENXIO;
> +	}
> +
> +	wdt->base = devm_ioremap_nocache(&pdev->dev, res->start,
> +			res->end - res->start + 1);
> +	if (!wdt->base) {
> +		dev_err(&pdev->dev, "cannot remap I/O memory region\n");
> +		return -ENXIO;
> +	}
> +
> +	/* disable watchdog and reboot on timeout */
> +	ctrl = readl(wdt->base + ADX_WDT_CONTROL);
> +	ctrl &= ~ADX_WDT_CONTROL_ENABLE;
> +	ctrl &= ~ADX_WDT_CONTROL_nRESET;
> +	writel(ctrl, wdt->base + ADX_WDT_CONTROL);
> +
> +	platform_set_drvdata(pdev, wdt);
> +	adx_wdt_dev = pdev;
> +
> +	ret = misc_register(&adx_wdt_miscdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "cannot register miscdev on minor %d "
> +				"(err=%d)\n", WATCHDOG_MINOR, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __devexit adx_wdt_remove(struct platform_device *pdev)
> +{
> +	struct adx_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	misc_deregister(&adx_wdt_miscdev);
> +	adx_wdt_stop(wdt);
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static void adx_wdt_shutdown(struct platform_device *pdev)
> +{
> +	struct adx_wdt *wdt = platform_get_drvdata(pdev);
> +	adx_wdt_stop(wdt);
> +}
> +
> +#ifdef CONFIG_PM
> +static int adx_wdt_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct adx_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	wdt->wake = (wdt->state == WDT_STATE_START) ? 1 : 0;
> +	adx_wdt_stop(wdt);
> +
> +	return 0;
> +}
> +
> +static int adx_wdt_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct adx_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	if (wdt->wake)
> +		adx_wdt_start(wdt);
> +
> +	return 0;
> +}
> +
> +static struct dev_pm_ops adx_wdt_pm_ops = {
> +	.suspend = adx_wdt_suspend,
> +	.resume = adx_wdt_resume,
> +};
> +
> +#  define ADX_WDT_PM_OPS	(&adx_wdt_pm_ops)
> +#else
> +#  define ADX_WDT_PM_OPS	NULL
> +#endif
> +
> +static struct platform_driver adx_wdt_driver = {
> +	.probe = adx_wdt_probe,
> +	.remove = __devexit_p(adx_wdt_remove),
> +	.shutdown = adx_wdt_shutdown,
> +	.driver = {
> +		.name = WATCHDOG_NAME,
> +		.owner = THIS_MODULE,
> +		.pm = ADX_WDT_PM_OPS,
> +	},
> +};
> +
> +static int __init adx_wdt_init(void)
> +{
> +	return platform_driver_register(&adx_wdt_driver);
> +}
> +
> +static void __exit adx_wdt_exit(void)
> +{
> +	platform_driver_unregister(&adx_wdt_driver);
> +}
> +
> +module_init(adx_wdt_init);
> +module_exit(adx_wdt_exit);
> +
> +MODULE_DESCRIPTION("Avionic Design Xanthos Watchdog Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Thierry Reding <thierry.reding@avionic-design.de>");
> +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
> -- 
> tg: (74fca6a..) adx/watchdog (depends on: adx/master)

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Thierry Reding]

* Wim Van Sebroeck wrote:
> Hi Thierry,
> 
> it's in the linux-watchdog-next tree now.
> 
> Kind regards,
> Wim.

Great, thanks!
Thierry

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Thierry Reding]

* Wim Van Sebroeck wrote:
> Hi Thierry,
> 
> > + * linux/drivers/watchdog/adx_wdt.c
> 
> Please change this to adx_wdt.c . If the tree get's moved then we don't want to review every driver to get this "fixed".

Is it okay with you if I just drop this line altogether? It really just noise
anyway.

Thierry

[PATCH] watchdog: Add support for the Avionic Design Xanthos watchdog timer.

[Wim Van Sebroeck]

Hi Thierry,

> > > + * linux/drivers/watchdog/adx_wdt.c
> > 
> > Please change this to adx_wdt.c . If the tree get's moved then we don't want to review every driver to get this "fixed".
> 
> Is it okay with you if I just drop this line altogether? It really just noise
> anyway.

No problem :-).

Kind regards,
Wim.