Skip to contentSkip to conversation

watchdog: max63xx_wdt: Add support for specifying WDI logic via GPIO

PATCH In review Pali Rohár 2 participants 5 messages Apr 28, 2022

Subsystems (2)
Device tree bindings Watchdog device drivers
lore
Share & tools
lore

Pali Rohár

author
#0 lore raw

PATCH 1/1 watchdog: max63xx_wdt: Add support for specifying WDI logic via GPIO

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 <[email protected]>
---
 .../bindings/watchdog/maxim,max63xx.yaml      |  4 +++
 drivers/watchdog/max63xx_wdt.c                | 28 +++++++++++++++++++
 2 files changed, 32 insertions(+)
Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +4 -0
index ab9641e845db..a97aa0135ef9 100644
@@ -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
drivers/watchdog/max63xx_wdt.c +28 -0
index 9e1541cfae0d..eaf00c3f06a5 100644
@@ -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));
+		return PTR_ERR(wdt->gpio_wdi);
+	}
+
+	if (!IS_ERR(wdt->gpio_wdi))
+		wdt->ping = max63xx_gpio_ping;
+	else
+		wdt->gpio_wdi = NULL;
+
 	err = max63xx_mmap_init(pdev, wdt);
 	if (err)
 		return err;
Signature · 1 line
2.20.1
maintainer
#1 lore raw
Quoted 4 lines · Pali Rohár

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 <[email protected]>

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

Guenter

Quoted 80 lines
---
  .../bindings/watchdog/maxim,max63xx.yaml      |  4 +++
  drivers/watchdog/max63xx_wdt.c                | 28 +++++++++++++++++++
  2 files changed, 32 insertions(+)
Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +4 -0
index ab9641e845db..a97aa0135ef9 100644
@@ -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
drivers/watchdog/max63xx_wdt.c +28 -0
index 9e1541cfae0d..eaf00c3f06a5 100644
@@ -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));
+		return PTR_ERR(wdt->gpio_wdi);
+	}
+
+	if (!IS_ERR(wdt->gpio_wdi))
+		wdt->ping = max63xx_gpio_ping;
+	else
+		wdt->gpio_wdi = NULL;
+
  	err = max63xx_mmap_init(pdev, wdt);
  	if (err)
  		return err;
author
#2 lore raw
Quoted 7 lines · On Thursday 28 April 2022 06:10:56 Guenter Roeck wrote:
Quoted 4 lines · Pali Rohár

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 <[email protected]>

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.

Quoted 78 lines

Guenter

Quoted 75 lines
---
  .../bindings/watchdog/maxim,max63xx.yaml      |  4 +++
  drivers/watchdog/max63xx_wdt.c                | 28 +++++++++++++++++++
  2 files changed, 32 insertions(+)
Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +4 -0
index ab9641e845db..a97aa0135ef9 100644
@@ -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
drivers/watchdog/max63xx_wdt.c +28 -0
index 9e1541cfae0d..eaf00c3f06a5 100644
@@ -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));
+		return PTR_ERR(wdt->gpio_wdi);
+	}
+
+	if (!IS_ERR(wdt->gpio_wdi))
+		wdt->ping = max63xx_gpio_ping;
+	else
+		wdt->gpio_wdi = NULL;
+
  	err = max63xx_mmap_init(pdev, wdt);
  	if (err)
  		return err;
maintainer
#3 lore raw
Quoted 7 lines · Pali Rohár
Quoted 6 lines · On Thursday 28 April 2022 06:10:56 Guenter Roeck wrote:
Quoted 4 lines · Pali Rohár

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 <[email protected]>
Quoted 8 lines
Quoted 1 line

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

Quoted 7 lines
Quoted 7 lines

Guenter

Quoted 5 lines
---
   .../bindings/watchdog/maxim,max63xx.yaml      |  4 +++
   drivers/watchdog/max63xx_wdt.c                | 28 +++++++++++++++++++
   2 files changed, 32 insertions(+)
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +0 -0
index ab9641e845db..a97aa0135ef9 100644
@@ -27,6 +27,10 @@ properties:
       description: This is a 1-byte memory-mapped address
Quoted 8 lines

Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml properties: · line 36

Quoted 8 lines
Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +4 -0
       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.

Quoted 37 lines
Quoted 37 lines
Quoted 37 lines
drivers/watchdog/max63xx_wdt.c +15 -0
index 9e1541cfae0d..eaf00c3f06a5 100644
@@ -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;
Quoted 8 lines
Quoted 8 lines

drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 246

Quoted 8 lines
drivers/watchdog/max63xx_wdt.c +5 -0
@@ -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().

Quoted 7 lines
Quoted 7 lines

drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 254

Quoted 7 lines
drivers/watchdog/max63xx_wdt.c +7 -0
+		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.

Quoted 2 lines
Quoted 2 lines

drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 255

Quoted 2 lines
drivers/watchdog/max63xx_wdt.c +1 -0
+
   	err = max63xx_mmap_init(pdev, wdt);

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

Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
drivers/watchdog/max63xx_wdt.c +0 -0
   	if (err)
   		return err;
author
#4 lore raw
Quoted 29 lines · On Thursday 28 April 2022 08:29:52 Guenter Roeck wrote:
Quoted 7 lines · Pali Rohár
Quoted 6 lines · On Thursday 28 April 2022 06:10:56 Guenter Roeck wrote:
Quoted 4 lines · Pali Rohár

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 <[email protected]>
Quoted 8 lines
Quoted 1 line

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

Quoted 7 lines
Quoted 7 lines

Guenter

Quoted 5 lines
---
   .../bindings/watchdog/maxim,max63xx.yaml      |  4 +++
   drivers/watchdog/max63xx_wdt.c                | 28 +++++++++++++++++++
   2 files changed, 32 insertions(+)
Quoted 16 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +0 -0
index ab9641e845db..a97aa0135ef9 100644
@@ -27,6 +27,10 @@ properties:
       description: This is a 1-byte memory-mapped address
Quoted 8 lines
Quoted 8 lines

Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml properties: · line 36

Quoted 8 lines
Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +4 -0
       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.

Ok.

Quoted 39 lines
Quoted 39 lines
Quoted 39 lines
Quoted 39 lines
drivers/watchdog/max63xx_wdt.c +15 -0
index 9e1541cfae0d..eaf00c3f06a5 100644
@@ -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;
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines

drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 247

Quoted 6 lines
drivers/watchdog/max63xx_wdt.c +5 -0
   	}
+	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().

Ok.

Quoted 9 lines
Quoted 7 lines
Quoted 7 lines

drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 254

Quoted 7 lines
drivers/watchdog/max63xx_wdt.c +7 -0
+		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.

I thought that it would be more safer to have NULL value stored in pointer variable as opposite of some non-NULL value used for the error reporting. But I can remove this part of the code.

Quoted 4 lines
Quoted 2 lines
Quoted 2 lines

drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 255

Quoted 2 lines
drivers/watchdog/max63xx_wdt.c +1 -0
+
   	err = max63xx_mmap_init(pdev, wdt);

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

Yes, it does. I forgot to amend local change to this patch. wdt->ping has to be changed to gpio_ping _after_ max63xx_mmap_init call.

I will fix all those issues in v2.

Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
drivers/watchdog/max63xx_wdt.c +0 -0
   	if (err)
   		return err;