Re: [PATCH] watchdog: max63xx_wdt: Add support for specifying WDI logic via GPIO

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/28/22 07:32, Pali Rohár wrote:
On Thursday 28 April 2022 06:10:56 Guenter Roeck wrote:
On 4/28/22 02:16, Pali Rohár wrote:
On some boards is WDI logic of max6370 chip connected via GPIO. So extend
max63xx_wdt driver and DTS schema to allow specifying WDI logic via GPIO.

Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>

How is that different to just using the gpio watchdog driver ?

GPIO watchdog driver does not support max6370 memory mapped
configuration.

With this change, max6370 can use memory mapped space for watchdog
configuration and GPIO WDI for pinging.


Ok, that makes sense. Comments below.

Thanks,
Guenter

Guenter

---
   .../bindings/watchdog/maxim,max63xx.yaml      |  4 +++
   drivers/watchdog/max63xx_wdt.c                | 28 +++++++++++++++++++
   2 files changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml b/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml
index ab9641e845db..a97aa0135ef9 100644
--- a/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml
+++ b/Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml
@@ -27,6 +27,10 @@ properties:
       description: This is a 1-byte memory-mapped address
       maxItems: 1
+  gpios:
+    description: Optional GPIO used for controlling WDI when WDI bit is not mapped to memory
+    maxItems: 1
+
   required:
     - compatible
     - reg

Devicetree patches needs to be separate.

diff --git a/drivers/watchdog/max63xx_wdt.c b/drivers/watchdog/max63xx_wdt.c
index 9e1541cfae0d..eaf00c3f06a5 100644
--- a/drivers/watchdog/max63xx_wdt.c
+++ b/drivers/watchdog/max63xx_wdt.c
@@ -27,6 +27,7 @@
   #include <linux/io.h>
   #include <linux/slab.h>
   #include <linux/property.h>
+#include <linux/gpio/consumer.h>
   #define DEFAULT_HEARTBEAT 60
   #define MAX_HEARTBEAT     60
@@ -53,6 +54,9 @@ struct max63xx_wdt {
   	void __iomem *base;
   	spinlock_t lock;
+	/* GPIOs */
+	struct gpio_desc *gpio_wdi;
+
   	/* WDI and WSET bits write access routines */
   	void (*ping)(struct max63xx_wdt *wdt);
   	void (*set)(struct max63xx_wdt *wdt, u8 set);
@@ -158,6 +162,17 @@ static const struct watchdog_info max63xx_wdt_info = {
   	.identity = "max63xx Watchdog",
   };
+static void max63xx_gpio_ping(struct max63xx_wdt *wdt)
+{
+	spin_lock(&wdt->lock);
+
+	gpiod_set_value_cansleep(wdt->gpio_wdi, 1);
+	udelay(1);
+	gpiod_set_value_cansleep(wdt->gpio_wdi, 0);
+
+	spin_unlock(&wdt->lock);
+}
+
   static void max63xx_mmap_ping(struct max63xx_wdt *wdt)
   {
   	u8 val;
@@ -225,6 +240,19 @@ static int max63xx_wdt_probe(struct platform_device *pdev)
   		return -EINVAL;
   	}
+	wdt->gpio_wdi = devm_gpiod_get(dev, NULL, GPIOD_FLAGS_BIT_DIR_OUT);
+	if (IS_ERR(wdt->gpio_wdi) && PTR_ERR(wdt->gpio_wdi) != -ENOENT) {
+		if (PTR_ERR(wdt->gpio_wdi) != -EPROBE_DEFER)
+			dev_err(dev, "unable to request gpio: %ld\n",
+				PTR_ERR(wdt->gpio_wdi));

Please use dev_err_probe().

+		return PTR_ERR(wdt->gpio_wdi);
+	}
+
+	if (!IS_ERR(wdt->gpio_wdi))
+		wdt->ping = max63xx_gpio_ping;
+	else
+		wdt->gpio_wdi = NULL;

Why set gpio_wdi to NULL? It isn't used if the ping function is not set.

+
   	err = max63xx_mmap_init(pdev, wdt);

Doesn't this override the gpio ping function set above ?

   	if (err)
   		return err;





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux