watchdog: orion_wdt & at91sam9_wdt: improve dt support

Tested on an at91sam9260 board (evk-pro3)

Signed-off-by Fabio Porcedda <[email protected]>

---
 .../devicetree/bindings/watchdog/atmel-wdt.txt      | 19 +++++++++++++++++++
 drivers/watchdog/at91sam9_wdt.c                     | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/atmel-wdt.txt

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt new file +19 -0

new file mode 100644
index 0000000..65c1755
@@ -0,0 +1,19 @@
+* Atmel Watchdog Timers
+
+** at91sam9-wdt
+
+Required properties:
+- compatible: must be "atmel,at91sam9260-wdt".
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+Optional properties:
+- timeout: contains the watchdog timeout in seconds.
+
+Example:
+
+	watchdog at fffffd40 {
+		compatible = "atmel,at91sam9260-wdt";
+		reg = <0xfffffd40 0x10>;
+		timeout = <10>;
+	};

drivers/watchdog/at91sam9_wdt.c +1 -0

index 05e1be8..c9e6bfa 100644
@@ -32,6 +32,7 @@
 #include <linux/timer.h>
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
+#include <linux/of.h>
 
 #include "at91sam9_wdt.h"
 

@@ -254,6 +255,14 @@ static struct miscdevice at91wdt_miscdev = {
 	.fops		= &at91wdt_fops,

Quoted 48 lines · Fabio Porcedda

Tested on an at91sam9260 board (evk-pro3)

Signed-off-by Fabio Porcedda <[email protected]>

---
 .../devicetree/bindings/watchdog/atmel-wdt.txt      | 19 +++++++++++++++++++
 drivers/watchdog/at91sam9_wdt.c                     | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/atmel-wdt.txt

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt new file +19 -0

new file mode 100644
index 0000000..65c1755
@@ -0,0 +1,19 @@
+* Atmel Watchdog Timers
+
+** at91sam9-wdt
+
+Required properties:
+- compatible: must be "atmel,at91sam9260-wdt".
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+Optional properties:
+- timeout: contains the watchdog timeout in seconds.
+
+Example:
+
+     watchdog at fffffd40 {
+             compatible = "atmel,at91sam9260-wdt";
+             reg = <0xfffffd40 0x10>;
+             timeout = <10>;
+     };

drivers/watchdog/at91sam9_wdt.c +1 -0

index 05e1be8..c9e6bfa 100644
@@ -32,6 +32,7 @@
 #include <linux/timer.h>
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
+#include <linux/of.h>
 
 #include "at91sam9_wdt.h"
 

@@ -254,6 +255,14 @@ static struct miscdevice at91wdt_miscdev = {
      .fops           = &at91wdt_fops,

drivers/watchdog/at91sam9_wdt.c static struct miscdevice at91wdt_miscdev = { · line 265

Quoted 10 lines

drivers/watchdog/at91sam9_wdt.c +8 -0

 };
 
+static inline void __init at91wdt_probe_dt(struct device_node *node)
+{
+     if (!node)
+             return;
+
+     of_property_read_u32(node, "timeout", &heartbeat);
+}
+

In patch #1 you add a function to do this, and then you don't make use of it here ?

Or am i missing something?

Quoted 63 lines · Andrew Lunn

Quoted 48 lines · Fabio Porcedda

Tested on an at91sam9260 board (evk-pro3)

Signed-off-by Fabio Porcedda <[email protected]>

---
 .../devicetree/bindings/watchdog/atmel-wdt.txt      | 19 +++++++++++++++++++
 drivers/watchdog/at91sam9_wdt.c                     | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/atmel-wdt.txt

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt new file +19 -0

new file mode 100644
index 0000000..65c1755
@@ -0,0 +1,19 @@
+* Atmel Watchdog Timers
+
+** at91sam9-wdt
+
+Required properties:
+- compatible: must be "atmel,at91sam9260-wdt".
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+Optional properties:
+- timeout: contains the watchdog timeout in seconds.
+
+Example:
+
+     watchdog at fffffd40 {
+             compatible = "atmel,at91sam9260-wdt";
+             reg = <0xfffffd40 0x10>;
+             timeout = <10>;
+     };

drivers/watchdog/at91sam9_wdt.c +1 -0

index 05e1be8..c9e6bfa 100644
@@ -32,6 +32,7 @@
 #include <linux/timer.h>
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
+#include <linux/of.h>
 
 #include "at91sam9_wdt.h"
 

@@ -254,6 +255,14 @@ static struct miscdevice at91wdt_miscdev = {
      .fops           = &at91wdt_fops,

drivers/watchdog/at91sam9_wdt.c static struct miscdevice at91wdt_miscdev = { · line 265

Quoted 10 lines

drivers/watchdog/at91sam9_wdt.c +8 -0

 };
 
+static inline void __init at91wdt_probe_dt(struct device_node *node)
+{
+     if (!node)
+             return;
+
+     of_property_read_u32(node, "timeout", &heartbeat);
+}
+

In patch #1 you add a function to do this, and then you don't make use of it here ?

Quoted 1 line

Or am i missing something?

I'm using it on the patch #2 for the orion_wdt driver. Do you think it's better to join the #1 and the #2 patch?

Best regards
--
Fabio Porcedda

Quoted 64 lines · Fabio Porcedda

Quoted 63 lines · Andrew Lunn

Quoted 48 lines · Fabio Porcedda

Tested on an at91sam9260 board (evk-pro3)

Signed-off-by Fabio Porcedda <[email protected]>

---
 .../devicetree/bindings/watchdog/atmel-wdt.txt      | 19 +++++++++++++++++++
 drivers/watchdog/at91sam9_wdt.c                     | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/atmel-wdt.txt

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt new file +19 -0

new file mode 100644
index 0000000..65c1755
@@ -0,0 +1,19 @@
+* Atmel Watchdog Timers
+
+** at91sam9-wdt
+
+Required properties:
+- compatible: must be "atmel,at91sam9260-wdt".
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+Optional properties:
+- timeout: contains the watchdog timeout in seconds.
+
+Example:
+
+     watchdog at fffffd40 {
+             compatible = "atmel,at91sam9260-wdt";
+             reg = <0xfffffd40 0x10>;
+             timeout = <10>;
+     };

drivers/watchdog/at91sam9_wdt.c +1 -0

index 05e1be8..c9e6bfa 100644
@@ -32,6 +32,7 @@
 #include <linux/timer.h>
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
+#include <linux/of.h>
 
 #include "at91sam9_wdt.h"
 

@@ -254,6 +255,14 @@ static struct miscdevice at91wdt_miscdev = {
      .fops           = &at91wdt_fops,

drivers/watchdog/at91sam9_wdt.c static struct miscdevice at91wdt_miscdev = { · line 265

Quoted 10 lines

drivers/watchdog/at91sam9_wdt.c +8 -0

 };
 
+static inline void __init at91wdt_probe_dt(struct device_node *node)
+{
+     if (!node)
+             return;
+
+     of_property_read_u32(node, "timeout", &heartbeat);
+}
+

In patch #1 you add a function to do this, and then you don't make use of it here ?

Quoted 8 lines

Quoted 1 line

Or am i missing something?

I'm using it on the patch #2 for the orion_wdt driver. Do you think it's better to join the #1 and the #2 patch?

Best regards
--
Fabio Porcedda

I'm sorry, only now i understand your question.
The at91sam9_wdt driver don't use the watchdog core framework si i can't use that function cleanly.

The patch #1 and #2 are for introducing the same property as the at91sam9_wdt driver.

Quoted 74 lines · Fabio Porcedda

Quoted 64 lines · Fabio Porcedda

Quoted 63 lines · Andrew Lunn

Quoted 48 lines · Fabio Porcedda

Tested on an at91sam9260 board (evk-pro3)

Signed-off-by Fabio Porcedda <[email protected]>

---
 .../devicetree/bindings/watchdog/atmel-wdt.txt      | 19 +++++++++++++++++++
 drivers/watchdog/at91sam9_wdt.c                     | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/atmel-wdt.txt

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt new file +19 -0

new file mode 100644
index 0000000..65c1755
@@ -0,0 +1,19 @@
+* Atmel Watchdog Timers
+
+** at91sam9-wdt
+
+Required properties:
+- compatible: must be "atmel,at91sam9260-wdt".
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+Optional properties:
+- timeout: contains the watchdog timeout in seconds.
+
+Example:
+
+     watchdog at fffffd40 {
+             compatible = "atmel,at91sam9260-wdt";
+             reg = <0xfffffd40 0x10>;
+             timeout = <10>;
+     };

drivers/watchdog/at91sam9_wdt.c +1 -0

index 05e1be8..c9e6bfa 100644
@@ -32,6 +32,7 @@
 #include <linux/timer.h>
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
+#include <linux/of.h>
 
 #include "at91sam9_wdt.h"
 

@@ -254,6 +255,14 @@ static struct miscdevice at91wdt_miscdev = {
      .fops           = &at91wdt_fops,

drivers/watchdog/at91sam9_wdt.c static struct miscdevice at91wdt_miscdev = { · line 265

Quoted 10 lines

drivers/watchdog/at91sam9_wdt.c +8 -0

 };
 
+static inline void __init at91wdt_probe_dt(struct device_node *node)
+{
+     if (!node)
+             return;
+
+     of_property_read_u32(node, "timeout", &heartbeat);
+}
+

In patch #1 you add a function to do this, and then you don't make use of it here ?

Quoted 8 lines

Quoted 1 line

Or am i missing something?

I'm using it on the patch #2 for the orion_wdt driver. Do you think it's better to join the #1 and the #2 patch?

Best regards
--
Fabio Porcedda

Quoted 6 lines

I'm sorry, only now i understand your question.
The at91sam9_wdt driver don't use the watchdog core framework si i can't use that function cleanly.

The patch #1 and #2 are for introducing the same property as the at91sam9_wdt driver.

So maybe split this up into two different patchsets. One patchset to add the helper function, and the use of this helper to all watchdog divers that can use it. I think the following drivers should be modified:

orion_wdt.c
pnx4008_wdt.c
s3c2410_wdt.c

In a second patchset, convert the AT91SAM9 driver over to the watchdog core framework, and then use the helper function.

Quoted 87 lines · Andrew Lunn

Quoted 74 lines · Fabio Porcedda

Quoted 64 lines · Fabio Porcedda

Quoted 63 lines · Andrew Lunn

Quoted 48 lines · Fabio Porcedda

Tested on an at91sam9260 board (evk-pro3)

Signed-off-by Fabio Porcedda <[email protected]>

---
 .../devicetree/bindings/watchdog/atmel-wdt.txt      | 19 +++++++++++++++++++
 drivers/watchdog/at91sam9_wdt.c                     | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/atmel-wdt.txt

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt new file +19 -0

new file mode 100644
index 0000000..65c1755
@@ -0,0 +1,19 @@
+* Atmel Watchdog Timers
+
+** at91sam9-wdt
+
+Required properties:
+- compatible: must be "atmel,at91sam9260-wdt".
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+Optional properties:
+- timeout: contains the watchdog timeout in seconds.
+
+Example:
+
+     watchdog at fffffd40 {
+             compatible = "atmel,at91sam9260-wdt";
+             reg = <0xfffffd40 0x10>;
+             timeout = <10>;
+     };

drivers/watchdog/at91sam9_wdt.c +1 -0

index 05e1be8..c9e6bfa 100644
@@ -32,6 +32,7 @@
 #include <linux/timer.h>
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
+#include <linux/of.h>
 
 #include "at91sam9_wdt.h"
 

@@ -254,6 +255,14 @@ static struct miscdevice at91wdt_miscdev = {
      .fops           = &at91wdt_fops,

drivers/watchdog/at91sam9_wdt.c static struct miscdevice at91wdt_miscdev = { · line 265

Quoted 10 lines

drivers/watchdog/at91sam9_wdt.c +8 -0

 };
 
+static inline void __init at91wdt_probe_dt(struct device_node *node)
+{
+     if (!node)
+             return;
+
+     of_property_read_u32(node, "timeout", &heartbeat);
+}
+

In patch #1 you add a function to do this, and then you don't make use of it here ?

Quoted 8 lines

Quoted 1 line

Or am i missing something?

I'm using it on the patch #2 for the orion_wdt driver. Do you think it's better to join the #1 and the #2 patch?

Best regards
--
Fabio Porcedda

Quoted 6 lines

I'm sorry, only now i understand your question.
The at91sam9_wdt driver don't use the watchdog core framework si i can't use that function cleanly.

The patch #1 and #2 are for introducing the same property as the at91sam9_wdt driver.

So maybe split this up into two different patchsets. One patchset to add the helper function, and the use of this helper to all watchdog divers that can use it. I think the following drivers should be modified:

Quoted 6 lines

orion_wdt.c
pnx4008_wdt.c
s3c2410_wdt.c

In a second patchset, convert the AT91SAM9 driver over to the watchdog core framework, and then use the helper function.

I was thinking to add a more generic helper function like this:

static inline void watchdog_get_dttimeout(struct device_node *node, u32 *timeout)

{
	if (node)
		of_property_read_u32(node, "timeout", &wdd->timeout);
}

This way i can use this helper function in the at91sam9_wdt driver too. What do you think?

Quoted 54 lines · Fabio Porcedda

Quoted 37 lines · Andrew Lunn

Quoted 24 lines · Fabio Porcedda

Quoted 14 lines · Fabio Porcedda

Quoted 13 lines · Andrew Lunn

Quoted 8 lines · Fabio Porcedda

Tested on an at91sam9260 board (evk-pro3)

Signed-off-by Fabio Porcedda <[email protected]>

---
 .../devicetree/bindings/watchdog/atmel-wdt.txt      | 19 +++++++++++++++++++
 drivers/watchdog/at91sam9_wdt.c                     | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)

...

In patch #1 you add a function to do this, and then you don't make use of it here ?

Quoted 8 lines

Quoted 1 line

Or am i missing something?

I'm using it on the patch #2 for the orion_wdt driver. Do you think it's better to join the #1 and the #2 patch?

Best regards
--
Fabio Porcedda

Quoted 6 lines

I'm sorry, only now i understand your question.
The at91sam9_wdt driver don't use the watchdog core framework si i can't use that function cleanly.

The patch #1 and #2 are for introducing the same property as the at91sam9_wdt driver.

So maybe split this up into two different patchsets. One patchset to add the helper function, and the use of this helper to all watchdog divers that can use it. I think the following drivers should be modified:

Quoted 6 lines

orion_wdt.c
pnx4008_wdt.c
s3c2410_wdt.c

In a second patchset, convert the AT91SAM9 driver over to the watchdog core framework, and then use the helper function.

I was thinking to add a more generic helper function like this:

static inline void watchdog_get_dttimeout(struct device_node *node, u32 *timeout)

{
      if (node)
              of_property_read_u32(node, "timeout", &wdd->timeout);
}

Quoted 2 lines

This way i can use this helper function in the at91sam9_wdt driver too. What do you think?

timeout_sec and this can be move at of.h level

as this is not watchdog framework secific

Quoted 59 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 49 lines · Fabio Porcedda

Quoted 37 lines · Andrew Lunn

Quoted 24 lines · Fabio Porcedda

Quoted 14 lines · Fabio Porcedda

Quoted 13 lines · Andrew Lunn

Quoted 8 lines · Fabio Porcedda

Tested on an at91sam9260 board (evk-pro3)

Signed-off-by Fabio Porcedda <[email protected]>

---
 .../devicetree/bindings/watchdog/atmel-wdt.txt      | 19 +++++++++++++++++++
 drivers/watchdog/at91sam9_wdt.c                     | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)

...

In patch #1 you add a function to do this, and then you don't make use of it here ?

Quoted 8 lines

Quoted 1 line

Or am i missing something?

I'm using it on the patch #2 for the orion_wdt driver. Do you think it's better to join the #1 and the #2 patch?

Best regards
--
Fabio Porcedda

Quoted 6 lines

I'm sorry, only now i understand your question.
The at91sam9_wdt driver don't use the watchdog core framework si i can't use that function cleanly.

The patch #1 and #2 are for introducing the same property as the at91sam9_wdt driver.

So maybe split this up into two different patchsets. One patchset to add the helper function, and the use of this helper to all watchdog divers that can use it. I think the following drivers should be modified:

Quoted 6 lines

orion_wdt.c
pnx4008_wdt.c
s3c2410_wdt.c

In a second patchset, convert the AT91SAM9 driver over to the watchdog core framework, and then use the helper function.

I was thinking to add a more generic helper function like this:

static inline void watchdog_get_dttimeout(struct device_node *node, u32 *timeout)

Quoted 7 lines

{
      if (node)
              of_property_read_u32(node, "timeout", &wdd->timeout);
}

This way i can use this helper function in the at91sam9_wdt driver too. What do you think?

timeout_sec and this can be move at of.h level

Quoted 1 line

as this is not watchdog framework secific

I can not find any property with the "_sec" suffix, you think it's still fine to use that suffix?

You are speaking about a of_watchdog.h header with a of_watchdog_get_timeout function?

I was thinking to add a more generic helper function like this:

static inline void watchdog_get_dttimeout(struct device_node *node, u32 *timeout)

{
	if (node)
		of_property_read_u32(node, "timeout", &wdd->timeout);
}

Quoted 8 lines

I was thinking to add a more generic helper function like this:

static inline void watchdog_get_dttimeout(struct device_node *node, u32 *timeout)

{
      if (node)
              of_property_read_u32(node, "timeout", &wdd->timeout);
}

You forgot to change the function signature.

Also, if you are adding a generic function, it should be a generic function for the framework. All drivers should be slowly moving towards the framework, so adding functions which help you not move towards the framework are wrong.

Quoted 63 lines · Andrew Lunn

Quoted 48 lines · Fabio Porcedda

Tested on an at91sam9260 board (evk-pro3)

Signed-off-by Fabio Porcedda <[email protected]>

---
 .../devicetree/bindings/watchdog/atmel-wdt.txt      | 19 +++++++++++++++++++
 drivers/watchdog/at91sam9_wdt.c                     | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/atmel-wdt.txt

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt new file +19 -0

new file mode 100644
index 0000000..65c1755
@@ -0,0 +1,19 @@
+* Atmel Watchdog Timers
+
+** at91sam9-wdt
+
+Required properties:
+- compatible: must be "atmel,at91sam9260-wdt".
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+Optional properties:
+- timeout: contains the watchdog timeout in seconds.
+
+Example:
+
+     watchdog at fffffd40 {
+             compatible = "atmel,at91sam9260-wdt";
+             reg = <0xfffffd40 0x10>;
+             timeout = <10>;
+     };

drivers/watchdog/at91sam9_wdt.c +1 -0

index 05e1be8..c9e6bfa 100644
@@ -32,6 +32,7 @@
 #include <linux/timer.h>
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
+#include <linux/of.h>
 
 #include "at91sam9_wdt.h"
 

@@ -254,6 +255,14 @@ static struct miscdevice at91wdt_miscdev = {
      .fops           = &at91wdt_fops,

drivers/watchdog/at91sam9_wdt.c static struct miscdevice at91wdt_miscdev = { · line 265

Quoted 10 lines

drivers/watchdog/at91sam9_wdt.c +8 -0

 };
 
+static inline void __init at91wdt_probe_dt(struct device_node *node)
+{
+     if (!node)
+             return;
+
+     of_property_read_u32(node, "timeout", &heartbeat);
+}
+

In patch #1 you add a function to do this, and then you don't make use of it here ?

Quoted 1 line

Or am i missing something?

I'm using it on the patch #2 for the orion_wdt driver. Do you think it's better to join the #1 and the #2 patch?