[PATCH 1/2] watchdog: add gpio watchdog driver

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

On 10/05/2021 18.28, Simon Glass wrote:
> Hi Rasmus,
> 
> On Mon, 10 May 2021 at 09:47, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> A rather common kind of external watchdog circuit is one that is kept
>> alive by toggling a gpio. Add a driver for handling such a watchdog.
>>
>> The compatible string is probably a little odd as it has nothing to do
>> with linux per se - however, I chose that to make .dts snippets
>> reusable between device trees used with U-Boot and linux, and this is
>> the (only) compatible string that linux' corresponding driver and DT
>> binding accepts. I have asked whether one should/could add "wdt-gpio"
>> to that binding, but the answer was no:
>>
>>   https://lore.kernel.org/lkml/CAL_JsqKEGaFpiFV_oAtE+S_bnHkg4qry+bhx2EDs=NSbVf_giA at mail.gmail.com/
>>
>> If someone feels strongly about this, I can certainly remove the
>> "linux," part from the string - it probably wouldn't the only place where
>> one can't reuse a DT snippet as-is between linux and U-Boot.
> 
> It seems fine to me. We share DT bindings with Linux anyway.
> 
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  .../watchdog/gpio-wdt.txt                     | 15 ++++++
>>  drivers/watchdog/Kconfig                      |  7 +++
>>  drivers/watchdog/Makefile                     |  1 +
>>  drivers/watchdog/gpio_wdt.c                   | 51 +++++++++++++++++++
>>  4 files changed, 74 insertions(+)
>>  create mode 100644 doc/device-tree-bindings/watchdog/gpio-wdt.txt
>>  create mode 100644 drivers/watchdog/gpio_wdt.c
>>
>> diff --git a/doc/device-tree-bindings/watchdog/gpio-wdt.txt b/doc/device-tree-bindings/watchdog/gpio-wdt.txt
>> new file mode 100644
>> index 0000000000..2283b7ba6e
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/watchdog/gpio-wdt.txt
>> @@ -0,0 +1,15 @@
>> +GPIO watchdog timer
>> +
>> +Describes a simple watchdog timer which is reset by toggling a gpio.
>> +
>> +Required properties:
>> +
>> +- compatible: must be "linux,wdt-gpio"
>> +- gpios: gpio to toggle when wdt driver reset method is called
>> +
>> +Example:
>> +
>> +       gpio-wdt {
>> +               gpios = <&gpio0 1 0>;
>> +               compatible = "linux,wdt-gpio";
>> +       };
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index f0ff2612a6..2cf378db29 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -147,6 +147,13 @@ config WDT_CORTINA
>>           This driver support all CPU ISAs supported by Cortina
>>           Access CAxxxx SoCs.
>>
>> +config WDT_GPIO
>> +       bool "External gpio watchdog support"
>> +       depends on WDT
>> +       depends on DM_GPIO
>> +       help
>> +          Support for external watchdog fed by toggling a gpio.
> 
> please add a bit more detail. How do you configure it? e.g. point to
> the binding file (which you should add to U-Boot if not there).

You mean add a reference to
doc/device-tree-bindings/watchdog/gpio-wdt.txt which is added in the
very same patch? I can do that.

No other config option in drivers/watchdog/Kconfig has that, but perhaps
that's because there's no mention anywhere of wdt under
doc/device-tree-bindings/. And more generally, very few Kconfig files
have any such reference.

>> +
>>  config WDT_MPC8xx
>>         bool "MPC8xx watchdog timer support"
>>         depends on WDT && MPC8xx
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 5c7ef593fe..f14415bb8e 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -25,6 +25,7 @@ obj-$(CONFIG_WDT_BOOKE) += booke_wdt.o
>>  obj-$(CONFIG_WDT_CORTINA) += cortina_wdt.o
>>  obj-$(CONFIG_WDT_ORION) += orion_wdt.o
>>  obj-$(CONFIG_WDT_CDNS) += cdns_wdt.o
>> +obj-$(CONFIG_WDT_GPIO) += gpio_wdt.o
>>  obj-$(CONFIG_WDT_MPC8xx) += mpc8xx_wdt.o
>>  obj-$(CONFIG_WDT_MT7620) += mt7620_wdt.o
>>  obj-$(CONFIG_WDT_MT7621) += mt7621_wdt.o
>> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
>> new file mode 100644
>> index 0000000000..9dba9c254e
>> --- /dev/null
>> +++ b/drivers/watchdog/gpio_wdt.c
>> @@ -0,0 +1,51 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <dm/device_compat.h>
>> +#include <wdt.h>
>> +#include <asm/gpio.h>
>> +
>> +struct gpio_wdt_priv {
>> +       struct gpio_desc gpio;
>> +       int state;
> 
> bool ?

Perhaps, though I don't tend to think of the two values a gpio can have
as true/false; it's more like an uint:1, so any integer type that can
represent 0 and 1 (i.e., any) would do. I don't really care either way.

>> +};
>> +
>> +static int gpio_wdt_reset(struct udevice *dev)
>> +{
>> +       struct gpio_wdt_priv *priv = dev_get_priv(dev);
>> +
>> +       priv->state = !priv->state;
> 
> blank line before final return (please fix below also)
> 

OK, but I don't think that improves readability in such a tiny function
- and checkpatch didn't complain.

Thanks,
Rasmus