diff mbox

[RFC] GPIO-Watchdog in devicetree

Message ID 20080922194357.GA32041@pengutronix.de (mailing list archive)
State RFC, archived
Headers show

Commit Message

Wolfram Sang Sept. 22, 2008, 7:43 p.m. UTC 
Hello all,

I understood that the device-tree is for describing hardware and should
not contain driver specific information. I have problems drawing this
line right now. I made a driver for watchdogs which are pinged by
toggling a gpio. Currently the device-tree entry looks like this:

	watchdog@gpio {
		compatible = "gpio-watchdog";
		gpios =	<&gpio_simple 19 0>;
	};

Then, there are two module parameters. One to define the initial state of
the gpio (0 or 1), one to define the length of the pulse when serving
the watchdog (default 1 us). Now my question is:

Is it plausible to say that the module parameters would also fit to the
device-tree as properties? Recently, I tend to say so as otherwise the
description of the watchdog is incomplete. Then again, one might argue
to develop a specific watchdog driver instead of a generic one, and use
something like compatible = "<myvendor>, <mywatchdog>" which would
result in lots of duplicated code per watchdog. So, which way to go? I
am really undecided and would be happy to hear opinions.

For completeness, I'll append the current version of my driver.

All the best,

   Wolfram

Comments

Grant Likely Sept. 23, 2008, 3:02 p.m. UTC | #1 
On Mon, Sep 22, 2008 at 09:43:57PM +0200, Wolfram Sang wrote:
> Hello all,
> 
> I understood that the device-tree is for describing hardware and should
> not contain driver specific information. I have problems drawing this
> line right now. I made a driver for watchdogs which are pinged by
> toggling a gpio. Currently the device-tree entry looks like this:
> 
> 	watchdog@gpio {
> 		compatible = "gpio-watchdog";
> 		gpios =	<&gpio_simple 19 0>;
> 	};
> 
> Then, there are two module parameters. One to define the initial state of
> the gpio (0 or 1), one to define the length of the pulse when serving
> the watchdog (default 1 us). Now my question is:
> 
> Is it plausible to say that the module parameters would also fit to the
> device-tree as properties? Recently, I tend to say so as otherwise the
> description of the watchdog is incomplete. Then again, one might argue
> to develop a specific watchdog driver instead of a generic one, and use
> something like compatible = "<myvendor>, <mywatchdog>" which would
> result in lots of duplicated code per watchdog. So, which way to go? I
> am really undecided and would be happy to hear opinions.

Since this is *very* board specific and there are a lot of parameters
that come into play that affect how the GPIO should be twiddled, I'd
consider leaving it out of the device tree entirely and coding it in the
platform code for your board (arch/powerpc/platforms/*/*).  Or, if you
still want to describe the gpio pin used the device tree, then add a
property to the base of the device tree that only your board platform
code looks at.

For example:
/ {
	model = "pengutronix,super-sexy-board";
	#address-cells = <1>;
	#size-cells = <1>;
	super-sexy-board,watchdog-gpio = <&gpio_simple 19 0>;
	...
}

Decoding a GPIO from the tree is quite simple so this should not result
in great swaths of duplicated code.

g.
Scott Wood Sept. 25, 2008, 5:59 p.m. UTC | #2 
On Tue, Sep 23, 2008 at 09:02:56AM -0600, Grant Likely wrote:
> For example:
> / {
> 	model = "pengutronix,super-sexy-board";
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 	super-sexy-board,watchdog-gpio = <&gpio_simple 19 0>;
> 	...
> }

Why as a property of the root node, and not as a node with a very
specific compatible property?

-Scott
Grant Likely Sept. 25, 2008, 6:10 p.m. UTC | #3 
On Thu, Sep 25, 2008 at 11:59 AM, Scott Wood <scottwood@freescale.com> wrote:
>
> On Tue, Sep 23, 2008 at 09:02:56AM -0600, Grant Likely wrote:
> > For example:
> > / {
> >       model = "pengutronix,super-sexy-board";
> >       #address-cells = <1>;
> >       #size-cells = <1>;
> >       super-sexy-board,watchdog-gpio = <&gpio_simple 19 0>;
> >       ...
> > }
>
> Why as a property of the root node, and not as a node with a very
> specific compatible property?

Because the root node is the only logical board-level node we have
right now.  However, I'm not deeply committed to this approach.  The
only question I have about putting it in another node is choosing the
parent node.  I don't think it fits to make it a child of the SoC node
or any other bus node.

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
Scott Wood Sept. 25, 2008, 6:21 p.m. UTC | #4 
Grant Likely wrote:
> On Thu, Sep 25, 2008 at 11:59 AM, Scott Wood <scottwood@freescale.com> wrote:
>> On Tue, Sep 23, 2008 at 09:02:56AM -0600, Grant Likely wrote:
>>> For example:
>>> / {
>>>       model = "pengutronix,super-sexy-board";
>>>       #address-cells = <1>;
>>>       #size-cells = <1>;
>>>       super-sexy-board,watchdog-gpio = <&gpio_simple 19 0>;
>>>       ...
>>> }
>> Why as a property of the root node, and not as a node with a very
>> specific compatible property?
> 
> Because the root node is the only logical board-level node we have
> right now.  However, I'm not deeply committed to this approach.  The
> only question I have about putting it in another node is choosing the
> parent node.  I don't think it fits to make it a child of the SoC node
> or any other bus node.

A child of the gpio controller node seems most logical, though a child 
of the root node would be OK.  It was the freefloating property that 
struck me as a little odd.

-Scott
David Gibson Sept. 26, 2008, 1:15 a.m. UTC | #5 
On Mon, Sep 22, 2008 at 09:43:57PM +0200, Wolfram Sang wrote:
> Hello all,
> 
> I understood that the device-tree is for describing hardware and should
> not contain driver specific information. I have problems drawing this
> line right now. I made a driver for watchdogs which are pinged by
> toggling a gpio. Currently the device-tree entry looks like this:
> 
> 	watchdog@gpio {
> 		compatible = "gpio-watchdog";
> 		gpios =	<&gpio_simple 19 0>;
> 	};

This certainly isn't right.  The unit address should be based on the
'reg' property.  If there's no 'reg' property, there should be no unit
address.
David Gibson Sept. 26, 2008, 1:15 a.m. UTC | #6 
On Thu, Sep 25, 2008 at 01:21:23PM -0500, Scott Wood wrote:
> Grant Likely wrote:
>> On Thu, Sep 25, 2008 at 11:59 AM, Scott Wood <scottwood@freescale.com> wrote:
>>> On Tue, Sep 23, 2008 at 09:02:56AM -0600, Grant Likely wrote:
>>>> For example:
>>>> / {
>>>>       model = "pengutronix,super-sexy-board";
>>>>       #address-cells = <1>;
>>>>       #size-cells = <1>;
>>>>       super-sexy-board,watchdog-gpio = <&gpio_simple 19 0>;
>>>>       ...
>>>> }
>>> Why as a property of the root node, and not as a node with a very
>>> specific compatible property?
>>
>> Because the root node is the only logical board-level node we have
>> right now.  However, I'm not deeply committed to this approach.  The
>> only question I have about putting it in another node is choosing the
>> parent node.  I don't think it fits to make it a child of the SoC node
>> or any other bus node.
>
> A child of the gpio controller node seems most logical, though a child  
> of the root node would be OK.  It was the freefloating property that  
> struck me as a little odd.

I concur.
diff mbox

Patch

===

From; Wolfram Sang <w.sang@pengutronix.de>
Subject; of-driver for external gpio-triggered watchdogs

Still needs tests and review before it can go mainline.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/watchdog/Kconfig       |    8 +
 drivers/watchdog/Makefile      |    1 
 drivers/watchdog/of_gpio_wdt.c |  188 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 197 insertions(+)

Index: drivers/watchdog/Kconfig
===================================================================
--- drivers/watchdog/Kconfig.orig
+++ drivers/watchdog/Kconfig
@@ -55,6 +55,14 @@ 
 	  To compile this driver as a module, choose M here: the
 	  module will be called softdog.
 
+config OF_GPIO_WDT
+	tristate "OF GPIO watchdog"
+	help
+	  This driver handles external watchdogs which are triggered by a gpio.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called of_gpio_wdt.
+
 # ALPHA Architecture
 
 # ARM Architecture
Index: drivers/watchdog/Makefile
===================================================================
--- drivers/watchdog/Makefile.orig
+++ drivers/watchdog/Makefile
@@ -124,4 +124,5 @@ 
 # XTENSA Architecture
 
 # Architecture Independant
+obj-$(CONFIG_OF_GPIO_WDT) += of_gpio_wdt.o
 obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o
Index: drivers/watchdog/of_gpio_wdt.c
===================================================================
--- /dev/null
+++ drivers/watchdog/of_gpio_wdt.c
@@ -0,0 +1,188 @@ 
+/*
+ * of_gpio_wdt.c - driver for gpio-driven watchdogs
+ *
+ * Copyright (C) 2008 Pengutronix e.K.
+ *
+ * Author: Wolfram Sang <w.sang@pengutronix.de>
+ *
+ * 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; version 2 of the License.
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/miscdevice.h>
+#include <linux/of_platform.h>
+#include <linux/of_gpio.h>
+#include <linux/fs.h>
+#include <linux/module.h>
+#include <linux/watchdog.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/uaccess.h>
+
+#define DRIVER_NAME "of-gpio-wdt"
+
+
+static int wdt_gpio;
+
+static int wdt_init_state;
+module_param(wdt_init_state, bool, 0);
+MODULE_PARM_DESC(wdt_init_state,
+	"Initial state of the gpio pin (0/1), default = 0");
+
+static int wdt_toggle_delay = 1;
+module_param(wdt_toggle_delay, int, 0);
+MODULE_PARM_DESC(wdt_toggle_delay,
+	"Delay in us to keep the gpio triggered, default = 1");
+
+static void of_gpio_wdt_ping(void)
+{
+	gpio_set_value(wdt_gpio, wdt_init_state ^ 1);
+	udelay(wdt_toggle_delay);
+	gpio_set_value(wdt_gpio, wdt_init_state);
+}
+
+static ssize_t of_gpio_wdt_write(struct file *file, const char __user *buf,
+				 size_t count, loff_t *ppos)
+{
+	if (count)
+		of_gpio_wdt_ping();
+	return count;
+}
+
+static int of_gpio_wdt_open(struct inode *inode, struct file *file)
+{
+	of_gpio_wdt_ping();
+	return nonseekable_open(inode, file);
+}
+
+static int of_gpio_wdt_release(struct inode *inode, struct file *file)
+{
+	printk(KERN_CRIT "Unexpected close on watchdog device. "
+			 "File is closed, but watchdog is still running!\n");
+	return 0;
+}
+
+static int of_gpio_wdt_ioctl(struct inode *inode, struct file *file,
+				unsigned int cmd, unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	static struct watchdog_info ident = {
+		.options = WDIOF_KEEPALIVEPING,
+		.firmware_version = 0,
+		.identity = DRIVER_NAME,
+	};
+
+	switch (cmd) {
+
+	case WDIOC_GETSUPPORT:
+		if (copy_to_user(argp, &ident, sizeof(ident)))
+			return -EFAULT;
+		break;
+
+	case WDIOC_KEEPALIVE:
+		of_gpio_wdt_ping();
+		break;
+
+	case WDIOC_GETTEMP:
+	case WDIOC_GETTIMEOUT:
+	case WDIOC_SETOPTIONS:
+	case WDIOC_SETTIMEOUT:
+		return -EOPNOTSUPP;
+
+	default:
+		return -ENOTTY;
+	}
+
+	return 0;
+}
+
+static const struct file_operations of_gpio_wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.write		= of_gpio_wdt_write,
+	.ioctl		= of_gpio_wdt_ioctl,
+	.open		= of_gpio_wdt_open,
+	.release	= of_gpio_wdt_release,
+};
+
+static struct miscdevice of_gpio_wdt_miscdev = {
+	.minor	= WATCHDOG_MINOR,
+	.name	= "watchdog",
+	.fops	= &of_gpio_wdt_fops,
+};
+
+static int __devinit of_gpio_wdt_probe(struct of_device *op,
+			const struct of_device_id *match)
+{
+	int ret;
+
+	wdt_gpio = of_get_gpio(op->node, 0);
+	if (wdt_gpio < 0) {
+		dev_err(&op->dev, "could not determine gpio! (err=%d)\n",
+			wdt_gpio);
+		return wdt_gpio;
+	}
+
+	ret = gpio_request(wdt_gpio, DRIVER_NAME);
+	if (ret) {
+		dev_err(&op->dev, "could not get gpio! (err=%d)\n", ret);
+		return ret;
+	}
+
+	gpio_direction_output(wdt_gpio, wdt_init_state);
+
+	ret = misc_register(&of_gpio_wdt_miscdev);
+	if (ret) {
+		dev_err(&op->dev, "cannot register miscdev on minor=%d "
+				"(err=%d)\n", WATCHDOG_MINOR, ret);
+		gpio_free(wdt_gpio);
+		return -ENODEV;
+	}
+
+	dev_info(&op->dev, "gpio-watchdog driver started using gpio %d.\n",
+		wdt_gpio);
+	return 0;
+}
+
+static int __devexit of_gpio_wdt_remove(struct of_device *op)
+{
+	misc_deregister(&of_gpio_wdt_miscdev);
+	gpio_free(wdt_gpio);
+	return 0;
+}
+
+static struct of_device_id of_gpio_wdt_match[] = {
+	{ .compatible = "gpio-watchdog", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_gpio_wdt_match);
+
+static struct of_platform_driver of_gpio_wdt_driver = {
+	.owner = THIS_MODULE,
+	.name = DRIVER_NAME,
+	.match_table = of_gpio_wdt_match,
+	.probe = of_gpio_wdt_probe,
+	.remove = __devexit_p(of_gpio_wdt_remove),
+};
+
+static int __init of_gpio_wdt_init(void)
+{
+	return of_register_platform_driver(&of_gpio_wdt_driver);
+}
+
+static void __exit of_gpio_wdt_exit(void)
+{
+	of_unregister_platform_driver(&of_gpio_wdt_driver);
+}
+
+module_init(of_gpio_wdt_init);
+module_exit(of_gpio_wdt_exit);
+
+MODULE_AUTHOR("Wolfram Sang <w.sang@pengutronix.de>");
+MODULE_DESCRIPTION("Driver for gpio-triggered watchdogs");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);