dt-bindings: watchdog: max63xx: Add GPIO binding

PATCH Superseded Pali Rohár 4 participants 8 messages Apr 29, 2022 – Jul 5, 2022

Subsystems (2)

Device tree bindings Watchdog device drivers

lore

Share & tools

lore

Pali Rohár

author

2022-04-29 13:20 UTC #0 lore raw

PATCH v2 1/2 dt-bindings: watchdog: max63xx: Add GPIO binding

GPIO is optional and used for WDI logic.

Signed-off-by Pali Rohár <[email protected]>

---
 Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml | 4 ++++
 1 file changed, 4 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

Signature · 1 line

2.20.1

Pali Rohár

author

2022-04-29 13:20 UTC #1 lore raw

PATCH v2 2/2 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 to allow specifying WDI logic via GPIO.

Signed-off-by Pali Rohár <[email protected]>

---
Changes in v2:
* Usage of dev_err_probe()
* Fixing assignment of wdt->ping
* Remove clearing of wdt->gpio_wdi
* Move YAML change to separate patch

---
 drivers/watchdog/max63xx_wdt.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

drivers/watchdog/max63xx_wdt.c +24 -0

index 9e1541cfae0d..6e43f9e6d7eb 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,10 +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)
+		return dev_err_probe(dev, PTR_ERR(wdt->gpio_wdi),
+				     "unable to request gpio: %ld\n",
+				     PTR_ERR(wdt->gpio_wdi));
+
 	err = max63xx_mmap_init(pdev, wdt);
 	if (err)
 		return err;
 
+	if (!IS_ERR(wdt->gpio_wdi))
+		wdt->ping = max63xx_gpio_ping;
+
 	platform_set_drvdata(pdev, &wdt->wdd);
 	watchdog_set_drvdata(&wdt->wdd, wdt);

Signature · 1 line

2.20.1

Tzung-Bi Shih

2022-05-03 03:58 UTC #2 lore raw

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

drivers/watchdog/max63xx_wdt.c line 30

Quoted 5 lines · Pali Rohár

drivers/watchdog/max63xx_wdt.c +1 -0

@@ -27,6 +27,7 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/property.h>
+#include <linux/gpio/consumer.h>

It would be better to keep them alphabetically. Anyway, they aren't sorted originally...

drivers/watchdog/max63xx_wdt.c static const struct watchdog_info max63xx_wdt_info = { · line 167

Quoted 3 lines

drivers/watchdog/max63xx_wdt.c +3 -0

+static void max63xx_gpio_ping(struct max63xx_wdt *wdt)
+{
+	spin_lock(&wdt->lock);

Does it really need to acquire the lock? It looks like the lock is to prevent concurrent accesses to the mmap in max63xx_mmap_ping() and max63xx_mmap_set().

drivers/watchdog/max63xx_wdt.c static const struct watchdog_info max63xx_wdt_info = { · line 170

Quoted 2 lines

drivers/watchdog/max63xx_wdt.c +2 -0
+	gpiod_set_value_cansleep(wdt->gpio_wdi, 1);
+	udelay(1);

Doesn't it need to include <linux/delay.h> for udelay()?

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

Quoted 6 lines

drivers/watchdog/max63xx_wdt.c +2 -0

@@ -225,10 +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)

Use devm_gpiod_get_optional() to make the intent clear. Also, it gets rid of the check for -ENOENT.

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

Quoted 3 lines

drivers/watchdog/max63xx_wdt.c +3 -0

+		return dev_err_probe(dev, PTR_ERR(wdt->gpio_wdi),
+				     "unable to request gpio: %ld\n",
+				     PTR_ERR(wdt->gpio_wdi));

It doesn't need to again print for PTR_ERR(wdt->gpio_wdi). dev_err_probe() prints the error.

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

Quoted 6 lines

drivers/watchdog/max63xx_wdt.c +2 -0

 	err = max63xx_mmap_init(pdev, wdt);
 	if (err)
 		return err;
 
+	if (!IS_ERR(wdt->gpio_wdi))
+		wdt->ping = max63xx_gpio_ping;

Thus, the max63xx_gpio_ping() overrides max63xx_mmap_ping() if the GPIO was provided? It would be better to mention the behavior in the commit message.

Also, could both the assignments of wdt->gpio_wdi and wdt->ping happen after max63xx_mmap_init()?

Guenter Roeck

maintainer

2022-05-03 04:37 UTC #3 lore raw

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

Quoted 10 lines · Tzung-Bi Shih

drivers/watchdog/max63xx_wdt.c line 30
Quoted 5 lines · Pali Rohár
drivers/watchdog/max63xx_wdt.c +1 -0
@@ -27,6 +27,7 @@
  #include <linux/io.h>
  #include <linux/slab.h>
  #include <linux/property.h>
+#include <linux/gpio/consumer.h>
It would be better to keep them alphabetically. Anyway, they aren't sorted originally...

Quoted 7 lines

drivers/watchdog/max63xx_wdt.c static const struct watchdog_info max63xx_wdt_info = { · line 167
Quoted 3 lines
drivers/watchdog/max63xx_wdt.c +3 -0
+static void max63xx_gpio_ping(struct max63xx_wdt *wdt)
+{
+	spin_lock(&wdt->lock);
Does it really need to acquire the lock? It looks like the lock is to prevent concurrent accesses to the mmap in max63xx_mmap_ping() and max63xx_mmap_set().

Actually, that doesn't work at all. spin_lock() directly contradicts with gpiod_set_value_cansleep().

Quoted 34 lines

drivers/watchdog/max63xx_wdt.c static const struct watchdog_info max63xx_wdt_info = { · line 170
Quoted 2 lines
drivers/watchdog/max63xx_wdt.c +2 -0
+	gpiod_set_value_cansleep(wdt->gpio_wdi, 1);
+	udelay(1);
Doesn't it need to include <linux/delay.h> for udelay()?
drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 244
Quoted 6 lines
drivers/watchdog/max63xx_wdt.c +2 -0
@@ -225,10 +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)
Use devm_gpiod_get_optional() to make the intent clear. Also, it gets rid of the check for -ENOENT.
drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 247
Quoted 3 lines
drivers/watchdog/max63xx_wdt.c +3 -0
+		return dev_err_probe(dev, PTR_ERR(wdt->gpio_wdi),
+				     "unable to request gpio: %ld\n",
+				     PTR_ERR(wdt->gpio_wdi));
It doesn't need to again print for PTR_ERR(wdt->gpio_wdi). dev_err_probe() prints the error.
drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 254
Quoted 6 lines
drivers/watchdog/max63xx_wdt.c +2 -0
  	err = max63xx_mmap_init(pdev, wdt);
  	if (err)
  		return err;
 
+	if (!IS_ERR(wdt->gpio_wdi))
+		wdt->ping = max63xx_gpio_ping;
Thus, the max63xx_gpio_ping() overrides max63xx_mmap_ping() if the GPIO was provided? It would be better to mention the behavior in the commit message.
Also, could both the assignments of wdt->gpio_wdi and wdt->ping happen after max63xx_mmap_init()?

Pali Rohár

author

2022-05-03 22:06 UTC #4 lore raw

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

Quoted 19 lines · On Monday 02 May 2022 21:37:16 Guenter Roeck wrote:

Quoted 10 lines · Tzung-Bi Shih
drivers/watchdog/max63xx_wdt.c line 30
Quoted 5 lines · Pali Rohár
drivers/watchdog/max63xx_wdt.c +1 -0
@@ -27,6 +27,7 @@
  #include <linux/io.h>
  #include <linux/slab.h>
  #include <linux/property.h>
+#include <linux/gpio/consumer.h>
It would be better to keep them alphabetically. Anyway, they aren't sorted originally...
Quoted 7 lines
drivers/watchdog/max63xx_wdt.c static const struct watchdog_info max63xx_wdt_info = { · line 167
Quoted 3 lines
drivers/watchdog/max63xx_wdt.c +3 -0
+static void max63xx_gpio_ping(struct max63xx_wdt *wdt)
+{
+	spin_lock(&wdt->lock);
Does it really need to acquire the lock? It looks like the lock is to prevent concurrent accesses to the mmap in max63xx_mmap_ping() and max63xx_mmap_set().

Quoted 36 lines

Actually, that doesn't work at all. spin_lock() directly contradicts with gpiod_set_value_cansleep().
Quoted 32 lines
drivers/watchdog/max63xx_wdt.c static const struct watchdog_info max63xx_wdt_info = { · line 170
Quoted 2 lines
drivers/watchdog/max63xx_wdt.c +2 -0
+	gpiod_set_value_cansleep(wdt->gpio_wdi, 1);
+	udelay(1);
Doesn't it need to include <linux/delay.h> for udelay()?
drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 243
Quoted 5 lines
drivers/watchdog/max63xx_wdt.c +2 -0
@@ -225,10 +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)
Use devm_gpiod_get_optional() to make the intent clear. Also, it gets rid of the check for -ENOENT.
drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 247
Quoted 3 lines
drivers/watchdog/max63xx_wdt.c +3 -0
+		return dev_err_probe(dev, PTR_ERR(wdt->gpio_wdi),
+				     "unable to request gpio: %ld\n",
+				     PTR_ERR(wdt->gpio_wdi));
It doesn't need to again print for PTR_ERR(wdt->gpio_wdi). dev_err_probe() prints the error.
drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 254
Quoted 5 lines
drivers/watchdog/max63xx_wdt.c +2 -0
  	err = max63xx_mmap_init(pdev, wdt);
  	if (err)
  		return err;
+	if (!IS_ERR(wdt->gpio_wdi))
+		wdt->ping = max63xx_gpio_ping;
Thus, the max63xx_gpio_ping() overrides max63xx_mmap_ping() if the GPIO was provided? It would be better to mention the behavior in the commit message.
Also, could both the assignments of wdt->gpio_wdi and wdt->ping happen after max63xx_mmap_init()?

Hello! I'm going to look at all these issues. Recently I sent max63 watchdog driver also into U-Boot and seems that I mixed DTS and driver code between U-Boot and Kernel... and tested something mixed.

I will do new testing again, and will check that I'm testing correct code.

Pali Rohár

author

2022-07-05 00:12 UTC #5 lore raw

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

Quoted 57 lines · On Wednesday 04 May 2022 00:05:50 Pali Rohár wrote:

Quoted 19 lines · On Monday 02 May 2022 21:37:16 Guenter Roeck wrote:
Quoted 10 lines · Tzung-Bi Shih
drivers/watchdog/max63xx_wdt.c line 30
Quoted 5 lines · Pali Rohár
drivers/watchdog/max63xx_wdt.c +1 -0
@@ -27,6 +27,7 @@
  #include <linux/io.h>
  #include <linux/slab.h>
  #include <linux/property.h>
+#include <linux/gpio/consumer.h>
It would be better to keep them alphabetically. Anyway, they aren't sorted originally...
Quoted 7 lines
drivers/watchdog/max63xx_wdt.c static const struct watchdog_info max63xx_wdt_info = { · line 167
Quoted 3 lines
drivers/watchdog/max63xx_wdt.c +3 -0
+static void max63xx_gpio_ping(struct max63xx_wdt *wdt)
+{
+	spin_lock(&wdt->lock);
Does it really need to acquire the lock? It looks like the lock is to prevent concurrent accesses to the mmap in max63xx_mmap_ping() and max63xx_mmap_set().
Quoted 36 lines
Actually, that doesn't work at all. spin_lock() directly contradicts with gpiod_set_value_cansleep().
Quoted 32 lines
drivers/watchdog/max63xx_wdt.c static const struct watchdog_info max63xx_wdt_info = { · line 170
Quoted 2 lines
drivers/watchdog/max63xx_wdt.c +2 -0
+	gpiod_set_value_cansleep(wdt->gpio_wdi, 1);
+	udelay(1);
Doesn't it need to include <linux/delay.h> for udelay()?
drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 243
Quoted 5 lines
drivers/watchdog/max63xx_wdt.c +2 -0
@@ -225,10 +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)
Use devm_gpiod_get_optional() to make the intent clear. Also, it gets rid of the check for -ENOENT.
drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 247
Quoted 3 lines
drivers/watchdog/max63xx_wdt.c +3 -0
+		return dev_err_probe(dev, PTR_ERR(wdt->gpio_wdi),
+				     "unable to request gpio: %ld\n",
+				     PTR_ERR(wdt->gpio_wdi));
It doesn't need to again print for PTR_ERR(wdt->gpio_wdi). dev_err_probe() prints the error.
drivers/watchdog/max63xx_wdt.c static int max63xx_wdt_probe(struct platform_device *pdev) · line 254
Quoted 5 lines
drivers/watchdog/max63xx_wdt.c +2 -0
  	err = max63xx_mmap_init(pdev, wdt);
  	if (err)
  		return err;
+	if (!IS_ERR(wdt->gpio_wdi))
+		wdt->ping = max63xx_gpio_ping;
Thus, the max63xx_gpio_ping() overrides max63xx_mmap_ping() if the GPIO was provided? It would be better to mention the behavior in the commit message.
Also, could both the assignments of wdt->gpio_wdi and wdt->ping happen after max63xx_mmap_init()?

Quoted 6 lines

Hello! I'm going to look at all these issues. Recently I sent max63 watchdog driver also into U-Boot and seems that I mixed DTS and driver code between U-Boot and Kernel... and tested something mixed.
I will do new testing again, and will check that I'm testing correct code.

Hello! Now I sent a new version V3. I have tested it on PowerPC P2020 based board where watchdog registers are exported via CPLD and new V3 version is working fine.

Rob Herring

maintainer

2022-05-03 21:51 UTC #6 lore raw

In reply to Pali Rohár

Quoted 1 line · Pali Rohár

GPIO is optional and used for WDI logic.

Nowhere is WDI defined.

Quoted 11 lines

Signed-off-by Pali Rohár <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml | 4 ++++
 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +0 -0
index ab9641e845db..a97aa0135ef9 100644

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

Quoted 4 lines

Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +1 -0

@@ -27,6 +27,10 @@ properties:
     description: This is a 1-byte memory-mapped address
     maxItems: 1
 
+  gpios:

Usually, we want a name here. Maybe wdi-gpios, but I don't know what WDI is nor have I read the pin name in the datasheet for inspiration.

Quoted 10 lines

Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +3 -0
+    description: Optional GPIO used for controlling WDI when WDI bit is not mapped to memory
+    maxItems: 1
+
 required:
   - compatible
   - reg
Signature · 1 line
2.20.1

Pali Rohár

author

2022-05-03 22:02 UTC #7 lore raw

Quoted 19 lines · On Tuesday 03 May 2022 16:51:37 Rob Herring wrote:

Quoted 1 line · Pali Rohár
GPIO is optional and used for WDI logic.
Nowhere is WDI defined.
Quoted 14 lines
Signed-off-by Pali Rohár <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml | 4 ++++
 1 file changed, 4 insertions(+)
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
     maxItems: 1
 

Quoted 4 lines

Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml properties: · line 30
Quoted 1 line
Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +1 -0
+  gpios:
Usually, we want a name here. Maybe wdi-gpios, but I don't know what WDI is nor have I read the pin name in the datasheet for inspiration.

WDI is name of logic used in the datasheet, it is abbreviation of WatchDog Input (meaning that from watchdog chip this GPIO has input direction).

I'm not sure if we need to put gpio direction into the property name or also word watchdog (or its some abbrev) into name. As node is already named "watchdog" and direction depends on point of view (chip vs CPU), which can be in DTS misleading (because DTS describe direction from CPU point of view).

What for sure makes sense is extending description by explaining WDI abbreviation.

Quoted 10 lines

Quoted 10 lines
Documentation/devicetree/bindings/watchdog/maxim,max63xx.yaml +3 -0
+    description: Optional GPIO used for controlling WDI when WDI bit is not mapped to memory
+    maxItems: 1
+
 required:
   - compatible
   - reg
Signature · 1 line
2.20.1