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. "return" is used by "tristate" switch. ... > > +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? ... Thanks. --- ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f