Re: [RFC] GPIO-Watchdog in devicetree
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 <[EMAIL PROTECTED]> 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.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] GPIO-Watchdog in devicetree
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:
>
> [EMAIL PROTECTED] {
> 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| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] GPIO-Watchdog in devicetree
Grant Likely wrote:
On Thu, Sep 25, 2008 at 11:59 AM, Scott Wood <[EMAIL PROTECTED]> 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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] GPIO-Watchdog in devicetree
On Thu, Sep 25, 2008 at 11:59 AM, Scott Wood <[EMAIL PROTECTED]> 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.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] GPIO-Watchdog in devicetree
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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] GPIO-Watchdog in devicetree
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:
>
> [EMAIL PROTECTED] {
> 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 = ", " 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.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
[RFC] GPIO-Watchdog in devicetree
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:
[EMAIL PROTECTED] {
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 = ", " 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
===
From; Wolfram Sang <[EMAIL PROTECTED]>
Subject; of-driver for external gpio-triggered watchdogs
Still needs tests and review before it can go mainline.
Signed-off-by: Wolfram Sang <[EMAIL PROTECTED]>
---
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 <[EMAIL PROTECTED]>
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#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();
+
