On 11/26/2013 11:34 PM, Alexander Shiyan wrote:
Hello.
On 11/26/2013 08:26 AM, Alexander Shiyan wrote:
This patch adds a watchdog driver for devices controlled through GPIO,
(Analog Devices ADM706, IC 555 etc).
Signed-off-by: Alexander Shiyan <shc_work@xxxxxxx>
...
+++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
@@ -0,0 +1,23 @@
+* GPIO-controlled Watchdog
+
+Required Properties:
+- compatible: Should contain "linux,wdt-gpio".
Should ?
+- gpios: From common gpio binding; gpio connection to WDT reset pin.
+- wdt-gpio,hw_algo: The algorithm used by the driver. Should be one
+ of the following values:
Should ?
What wrong here?
...
+Example:
+ watchdog: watchdog {
+ /* ADM706 */
+ compatible = "linux,wdt-gpio";
Oddly enough, the bindings could be used by non-Linux operating systems,
but who am I to argue. Fine if this is what the DT maintainers want to see.
On my opinion this mean that this is not a real hardware, but hardware emulation,
like some other driver does.
...
diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
new file mode 100644
index 0000000..c7166be
--- /dev/null
+++ b/drivers/watchdog/gpio_wdt.c
@@ -0,0 +1,246 @@
+/*
+ * Driver for watchdog device controlled through GPIO-line
+ *
+ * Author: 2013, Alexander Shiyan <shc_work@xxxxxxx>
+ *
+ * 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.
+ */
+
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/watchdog.h>
+
+#define SOFT_TIMEOUT_MIN 1
+#define SOFT_TIMEOUT_DEF 60
+#define SOFT_TIMEOUT_MAX 0xffff
+
+enum {
+ HW_ALGO_TOGGLE,
+ HW_ALGO_LEVEL,
+};
+
+struct gpio_wdt_priv {
+ int gpio;
+ bool active_low;
+ bool state;
+ unsigned int hw_algo;
+ unsigned int hw_margin;
+ unsigned long last_jiffies;
+ struct notifier_block notifier;
+ struct timer_list timer;
+ struct watchdog_device wdd;
+};
+
+static int gpio_wdt_disable(struct gpio_wdt_priv *priv)
+{
+ gpio_set_value_cansleep(priv->gpio, !priv->active_low);
+
+ /* Put GPIO back to tristate */
+ if (priv->hw_algo == HW_ALGO_TOGGLE)
+ return gpio_direction_input(priv->gpio);
+
No disable for 'level' watchdogs ? Shouldn't it be set to the opposite
of priv->state ?
+ return 0;
Since this is an internal function which never returns anything but 0,
you might as well make it void.
"level" version will be disabled by
"gpio_set_value_cansleep(priv->gpio, !priv->active_low);" line above.
Ok.
"return" is used by "tristate" switch.
Just to return 0. That isn't really necessary. The caller could just return 0 by itself.
...
+static void gpio_wdt_hwping(unsigned long data)
+{
+ struct watchdog_device *wdd = (struct watchdog_device *)data;
+ struct gpio_wdt_priv *priv = watchdog_get_drvdata(wdd);
+
+ if (time_before(jiffies, priv->last_jiffies +
+ msecs_to_jiffies(wdd->timeout * 1000))) {
+ /* Restart timer */
+ mod_timer(&priv->timer, jiffies + priv->hw_margin);
+
+ switch (priv->hw_algo) {
+ case HW_ALGO_TOGGLE:
+ /* Toggle output pin */
+ priv->state = !priv->state;
+ gpio_set_value_cansleep(priv->gpio, priv->state);
+ break;
+ case HW_ALGO_LEVEL:
+ /* Pulse */
+ gpio_set_value_cansleep(priv->gpio, !priv->active_low);
+ udelay(1);
Pretty arbitrary toggle time. Should this be a property ?
What about 1/10 of hardware timeout?
Even worse, as it is an active wait and not sleep.
Not mandatory from my perspective, though; guess the property can be added later
if/when needed.
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
