[PATCH 1/2] watchdog: sama5d4: Cache MR instead of a partial config

[PATCH 1/2] watchdog: sama5d4: Cache MR instead of a partial config

[Alexandre Belloni]

.config is used to cache a part of WDT_MR at probe time and is not used
afterwards. Instead of doing that, actually cache MR and avoid reading it
every time it is modified.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/watchdog/sama5d4_wdt.c | 45 ++++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 26 deletions(-)

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index a49634cdc1cc..6dd07bef515a 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -28,7 +28,7 @@
 struct sama5d4_wdt {
 	struct watchdog_device	wdd;
 	void __iomem		*reg_base;
-	u32	config;
+	u32			mr;
 };
 
 static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
@@ -53,11 +53,9 @@ MODULE_PARM_DESC(nowayout,
 static int sama5d4_wdt_start(struct watchdog_device *wdd)
 {
 	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
-	u32 reg;
 
-	reg = wdt_read(wdt, AT91_WDT_MR);
-	reg &= ~AT91_WDT_WDDIS;
-	wdt_write(wdt, AT91_WDT_MR, reg);
+	wdt->mr &= ~AT91_WDT_WDDIS;
+	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
 
 	return 0;
 }
@@ -65,11 +63,9 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
 static int sama5d4_wdt_stop(struct watchdog_device *wdd)
 {
 	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
-	u32 reg;
 
-	reg = wdt_read(wdt, AT91_WDT_MR);
-	reg |= AT91_WDT_WDDIS;
-	wdt_write(wdt, AT91_WDT_MR, reg);
+	wdt->mr |= AT91_WDT_WDDIS;
+	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
 
 	return 0;
 }
@@ -88,14 +84,12 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
 {
 	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
 	u32 value = WDT_SEC2TICKS(timeout);
-	u32 reg;
 
-	reg = wdt_read(wdt, AT91_WDT_MR);
-	reg &= ~AT91_WDT_WDV;
-	reg &= ~AT91_WDT_WDD;
-	reg |= AT91_WDT_SET_WDV(value);
-	reg |= AT91_WDT_SET_WDD(value);
-	wdt_write(wdt, AT91_WDT_MR, reg);
+	wdt->mr &= ~AT91_WDT_WDV;
+	wdt->mr &= ~AT91_WDT_WDD;
+	wdt->mr |= AT91_WDT_SET_WDV(value);
+	wdt->mr |= AT91_WDT_SET_WDD(value);
+	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
 
 	wdd->timeout = timeout;
 
@@ -132,19 +126,19 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
 {
 	const char *tmp;
 
-	wdt->config = AT91_WDT_WDDIS;
+	wdt->mr = AT91_WDT_WDDIS;
 
 	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
 	    !strcmp(tmp, "software"))
-		wdt->config |= AT91_WDT_WDFIEN;
+		wdt->mr |= AT91_WDT_WDFIEN;
 	else
-		wdt->config |= AT91_WDT_WDRSTEN;
+		wdt->mr |= AT91_WDT_WDRSTEN;
 
 	if (of_property_read_bool(np, "atmel,idle-halt"))
-		wdt->config |= AT91_WDT_WDIDLEHLT;
+		wdt->mr |= AT91_WDT_WDIDLEHLT;
 
 	if (of_property_read_bool(np, "atmel,dbg-halt"))
-		wdt->config |= AT91_WDT_WDDBGHLT;
+		wdt->mr |= AT91_WDT_WDDBGHLT;
 
 	return 0;
 }
@@ -163,11 +157,10 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
 	reg &= ~AT91_WDT_WDDIS;
 	wdt_write(wdt, AT91_WDT_MR, reg);
 
-	reg = wdt->config;
-	reg |= AT91_WDT_SET_WDD(value);
-	reg |= AT91_WDT_SET_WDV(value);
+	wdt->mr |= AT91_WDT_SET_WDD(value);
+	wdt->mr |= AT91_WDT_SET_WDV(value);
 
-	wdt_write(wdt, AT91_WDT_MR, reg);
+	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
 
 	return 0;
 }
@@ -211,7 +204,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
 			return ret;
 	}
 
-	if ((wdt->config & AT91_WDT_WDFIEN) && irq) {
+	if ((wdt->mr & AT91_WDT_WDFIEN) && irq) {
 		ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler,
 				       IRQF_SHARED | IRQF_IRQPOLL |
 				       IRQF_NO_SUSPEND, pdev->name, pdev);
-- 
2.11.0

[PATCH 2/2] watchdog: sama5d4: Implement resume hook

[Alexandre Belloni]

When resuming for the deepest state on sama5d2, it is necessary to restore
MR as the registers are lost.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/watchdog/sama5d4_wdt.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 6dd07bef515a..de8ff10a032e 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -258,11 +258,28 @@ static const struct of_device_id sama5d4_wdt_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
 
+#ifdef CONFIG_PM_SLEEP
+static int sama5d4_wdt_resume(struct device *dev)
+{
+	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
+
+	wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
+	if (wdt->mr & AT91_WDT_WDDIS)
+		wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(sama5d4_wdt_pm_ops, NULL,
+			 sama5d4_wdt_resume);
+
 static struct platform_driver sama5d4_wdt_driver = {
 	.probe		= sama5d4_wdt_probe,
 	.remove		= sama5d4_wdt_remove,
 	.driver		= {
 		.name	= "sama5d4_wdt",
+		.pm	= &sama5d4_wdt_pm_ops,
 		.of_match_table = sama5d4_wdt_of_match,
 	}
 };
-- 
2.11.0

Re: [PATCH 1/2] watchdog: sama5d4: Cache MR instead of a partial config

[Guenter Roeck]

On Mon, Jan 30, 2017 at 06:18:47PM +0100, Alexandre Belloni wrote:
> .config is used to cache a part of WDT_MR at probe time and is not used
> afterwards. Instead of doing that, actually cache MR and avoid reading it
> every time it is modified.
> 
The semantic change here is that the old code leaves "other" bits in the
mr register alone. I assume you have verified that clearing those bits
does not have negative impact.

Also, I am not really sure if replacing a read from a register is more
costly than a read from memory. Is that change really worth it ?
I mean, sure, the way the 'config' variable is used is a bit odd,
but I don't really see the improvement in its replacement either.

> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  drivers/watchdog/sama5d4_wdt.c | 45 ++++++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index a49634cdc1cc..6dd07bef515a 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -28,7 +28,7 @@
>  struct sama5d4_wdt {
>  	struct watchdog_device	wdd;
>  	void __iomem		*reg_base;
> -	u32	config;
> +	u32			mr;
>  };
>  
>  static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> @@ -53,11 +53,9 @@ MODULE_PARM_DESC(nowayout,
>  static int sama5d4_wdt_start(struct watchdog_device *wdd)
>  {
>  	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> -	u32 reg;
>  
> -	reg = wdt_read(wdt, AT91_WDT_MR);
> -	reg &= ~AT91_WDT_WDDIS;
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> +	wdt->mr &= ~AT91_WDT_WDDIS;
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>  
>  	return 0;
>  }
> @@ -65,11 +63,9 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
>  static int sama5d4_wdt_stop(struct watchdog_device *wdd)
>  {
>  	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> -	u32 reg;
>  
> -	reg = wdt_read(wdt, AT91_WDT_MR);
> -	reg |= AT91_WDT_WDDIS;
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> +	wdt->mr |= AT91_WDT_WDDIS;
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>  
>  	return 0;
>  }
> @@ -88,14 +84,12 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
>  {
>  	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
>  	u32 value = WDT_SEC2TICKS(timeout);
> -	u32 reg;
>  
> -	reg = wdt_read(wdt, AT91_WDT_MR);
> -	reg &= ~AT91_WDT_WDV;
> -	reg &= ~AT91_WDT_WDD;
> -	reg |= AT91_WDT_SET_WDV(value);
> -	reg |= AT91_WDT_SET_WDD(value);
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> +	wdt->mr &= ~AT91_WDT_WDV;
> +	wdt->mr &= ~AT91_WDT_WDD;
> +	wdt->mr |= AT91_WDT_SET_WDV(value);
> +	wdt->mr |= AT91_WDT_SET_WDD(value);
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>  
>  	wdd->timeout = timeout;
>  
> @@ -132,19 +126,19 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
>  {
>  	const char *tmp;
>  
> -	wdt->config = AT91_WDT_WDDIS;
> +	wdt->mr = AT91_WDT_WDDIS;
>  
>  	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
>  	    !strcmp(tmp, "software"))
> -		wdt->config |= AT91_WDT_WDFIEN;
> +		wdt->mr |= AT91_WDT_WDFIEN;
>  	else
> -		wdt->config |= AT91_WDT_WDRSTEN;
> +		wdt->mr |= AT91_WDT_WDRSTEN;
>  
>  	if (of_property_read_bool(np, "atmel,idle-halt"))
> -		wdt->config |= AT91_WDT_WDIDLEHLT;
> +		wdt->mr |= AT91_WDT_WDIDLEHLT;
>  
>  	if (of_property_read_bool(np, "atmel,dbg-halt"))
> -		wdt->config |= AT91_WDT_WDDBGHLT;
> +		wdt->mr |= AT91_WDT_WDDBGHLT;
>  
>  	return 0;
>  }
> @@ -163,11 +157,10 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
>  	reg &= ~AT91_WDT_WDDIS;
>  	wdt_write(wdt, AT91_WDT_MR, reg);
>  
There is (at least) one read of the mr register left above this code.
It is not entirely obvious to me why it would make sense to retain
this single read instead of just clearing the AT91_WDT_WDDIS bit
from the cached value and writing the rest. Maybe this is to make
sure that WDV or WDD isn't changed with the bit set, but ....
the explanation associated with clearing the bit is a bit odd either
case. It states that AT91_WDT_WDDIS must not be set (meaning the watchdog
must be running ? Shouldn't it be the opposite ?) when changing WDV or WDD,
then violates this rule when updating the timeout (which can happen with
the watchdog running or not).

> -	reg = wdt->config;
> -	reg |= AT91_WDT_SET_WDD(value);
> -	reg |= AT91_WDT_SET_WDV(value);
> +	wdt->mr |= AT91_WDT_SET_WDD(value);
> +	wdt->mr |= AT91_WDT_SET_WDV(value);
>  
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>  
>  	return 0;
>  }
> @@ -211,7 +204,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>  			return ret;
>  	}
>  
> -	if ((wdt->config & AT91_WDT_WDFIEN) && irq) {
> +	if ((wdt->mr & AT91_WDT_WDFIEN) && irq) {
>  		ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler,
>  				       IRQF_SHARED | IRQF_IRQPOLL |
>  				       IRQF_NO_SUSPEND, pdev->name, pdev);
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] watchdog: sama5d4: Cache MR instead of a partial config

[Alexandre Belloni]

On 30/01/2017 at 09:58:13 -0800, Guenter Roeck wrote:
> On Mon, Jan 30, 2017 at 06:18:47PM +0100, Alexandre Belloni wrote:
> > .config is used to cache a part of WDT_MR at probe time and is not used
> > afterwards. Instead of doing that, actually cache MR and avoid reading it
> > every time it is modified.
> > 
> The semantic change here is that the old code leaves "other" bits in the
> mr register alone. I assume you have verified that clearing those bits
> does not have negative impact.
> 

It doesn't have any impact unless you expect to restart a watchdog
exactly were you stopped it.

> Also, I am not really sure if replacing a read from a register is more
> costly than a read from memory. Is that change really worth it ?
> I mean, sure, the way the 'config' variable is used is a bit odd,
> but I don't really see the improvement in its replacement either.
> 

It is difficult to say performance wise (I doubt the sama5d4_wdt
structure stays long enough in the cache). But it allows to avoid
reading mr in the suspend function in 2/2.

> > @@ -132,19 +126,19 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
> >  {
> >  	const char *tmp;
> >  
> > -	wdt->config = AT91_WDT_WDDIS;
> > +	wdt->mr = AT91_WDT_WDDIS;
> >  
> >  	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> >  	    !strcmp(tmp, "software"))
> > -		wdt->config |= AT91_WDT_WDFIEN;
> > +		wdt->mr |= AT91_WDT_WDFIEN;
> >  	else
> > -		wdt->config |= AT91_WDT_WDRSTEN;
> > +		wdt->mr |= AT91_WDT_WDRSTEN;
> >  
> >  	if (of_property_read_bool(np, "atmel,idle-halt"))
> > -		wdt->config |= AT91_WDT_WDIDLEHLT;
> > +		wdt->mr |= AT91_WDT_WDIDLEHLT;
> >  
> >  	if (of_property_read_bool(np, "atmel,dbg-halt"))
> > -		wdt->config |= AT91_WDT_WDDBGHLT;
> > +		wdt->mr |= AT91_WDT_WDDBGHLT;
> >  
> >  	return 0;
> >  }
> > @@ -163,11 +157,10 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> >  	reg &= ~AT91_WDT_WDDIS;
> >  	wdt_write(wdt, AT91_WDT_MR, reg);
> >  
> There is (at least) one read of the mr register left above this code.
> It is not entirely obvious to me why it would make sense to retain
> this single read instead of just clearing the AT91_WDT_WDDIS bit
> from the cached value and writing the rest. Maybe this is to make
> sure that WDV or WDD isn't changed with the bit set, but ....
> the explanation associated with clearing the bit is a bit odd either
> case. It states that AT91_WDT_WDDIS must not be set (meaning the watchdog
> must be running ? Shouldn't it be the opposite ?) when changing WDV or WDD,
> then violates this rule when updating the timeout (which can happen with
> the watchdog running or not).

The datasheet states:
Note: When setting the WDDIS bit, and while it is set, the fields WDV
and WDD must not be modified.

And indeed, this may be an issue in sama5d4_wdt_set_timeout(). That is
something I needed to clarify and forgot about but I think that can be
solved in a separate patch.

If you prefer I can simply remove the weird wdt->config usage, keeping
the register accesses and then rework 2/2 in consequence.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/2] watchdog: sama5d4: Cache MR instead of a partial config

[Guenter Roeck]

On 01/30/2017 09:18 AM, Alexandre Belloni wrote:
> .config is used to cache a part of WDT_MR at probe time and is not used
> afterwards. Instead of doing that, actually cache MR and avoid reading it
> every time it is modified.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

[ hoping that you'll remember to fix the problem in the set_timeout function ]

> ---
>  drivers/watchdog/sama5d4_wdt.c | 45 ++++++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index a49634cdc1cc..6dd07bef515a 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -28,7 +28,7 @@
>  struct sama5d4_wdt {
>  	struct watchdog_device	wdd;
>  	void __iomem		*reg_base;
> -	u32	config;
> +	u32			mr;
>  };
>
>  static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> @@ -53,11 +53,9 @@ MODULE_PARM_DESC(nowayout,
>  static int sama5d4_wdt_start(struct watchdog_device *wdd)
>  {
>  	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> -	u32 reg;
>
> -	reg = wdt_read(wdt, AT91_WDT_MR);
> -	reg &= ~AT91_WDT_WDDIS;
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> +	wdt->mr &= ~AT91_WDT_WDDIS;
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>
>  	return 0;
>  }
> @@ -65,11 +63,9 @@ static int sama5d4_wdt_start(struct watchdog_device *wdd)
>  static int sama5d4_wdt_stop(struct watchdog_device *wdd)
>  {
>  	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> -	u32 reg;
>
> -	reg = wdt_read(wdt, AT91_WDT_MR);
> -	reg |= AT91_WDT_WDDIS;
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> +	wdt->mr |= AT91_WDT_WDDIS;
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>
>  	return 0;
>  }
> @@ -88,14 +84,12 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
>  {
>  	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
>  	u32 value = WDT_SEC2TICKS(timeout);
> -	u32 reg;
>
> -	reg = wdt_read(wdt, AT91_WDT_MR);
> -	reg &= ~AT91_WDT_WDV;
> -	reg &= ~AT91_WDT_WDD;
> -	reg |= AT91_WDT_SET_WDV(value);
> -	reg |= AT91_WDT_SET_WDD(value);
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> +	wdt->mr &= ~AT91_WDT_WDV;
> +	wdt->mr &= ~AT91_WDT_WDD;
> +	wdt->mr |= AT91_WDT_SET_WDV(value);
> +	wdt->mr |= AT91_WDT_SET_WDD(value);
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>
>  	wdd->timeout = timeout;
>
> @@ -132,19 +126,19 @@ static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
>  {
>  	const char *tmp;
>
> -	wdt->config = AT91_WDT_WDDIS;
> +	wdt->mr = AT91_WDT_WDDIS;
>
>  	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
>  	    !strcmp(tmp, "software"))
> -		wdt->config |= AT91_WDT_WDFIEN;
> +		wdt->mr |= AT91_WDT_WDFIEN;
>  	else
> -		wdt->config |= AT91_WDT_WDRSTEN;
> +		wdt->mr |= AT91_WDT_WDRSTEN;
>
>  	if (of_property_read_bool(np, "atmel,idle-halt"))
> -		wdt->config |= AT91_WDT_WDIDLEHLT;
> +		wdt->mr |= AT91_WDT_WDIDLEHLT;
>
>  	if (of_property_read_bool(np, "atmel,dbg-halt"))
> -		wdt->config |= AT91_WDT_WDDBGHLT;
> +		wdt->mr |= AT91_WDT_WDDBGHLT;
>
>  	return 0;
>  }
> @@ -163,11 +157,10 @@ static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
>  	reg &= ~AT91_WDT_WDDIS;
>  	wdt_write(wdt, AT91_WDT_MR, reg);
>
> -	reg = wdt->config;
> -	reg |= AT91_WDT_SET_WDD(value);
> -	reg |= AT91_WDT_SET_WDV(value);
> +	wdt->mr |= AT91_WDT_SET_WDD(value);
> +	wdt->mr |= AT91_WDT_SET_WDV(value);
>
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>
>  	return 0;
>  }
> @@ -211,7 +204,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>  			return ret;
>  	}
>
> -	if ((wdt->config & AT91_WDT_WDFIEN) && irq) {
> +	if ((wdt->mr & AT91_WDT_WDFIEN) && irq) {
>  		ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler,
>  				       IRQF_SHARED | IRQF_IRQPOLL |
>  				       IRQF_NO_SUSPEND, pdev->name, pdev);
>

Re: [PATCH 2/2] watchdog: sama5d4: Implement resume hook

[Guenter Roeck]

On 01/30/2017 09:18 AM, Alexandre Belloni wrote:
> When resuming for the deepest state on sama5d2, it is necessary to restore
> MR as the registers are lost.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  drivers/watchdog/sama5d4_wdt.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 6dd07bef515a..de8ff10a032e 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -258,11 +258,28 @@ static const struct of_device_id sama5d4_wdt_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
>
> +#ifdef CONFIG_PM_SLEEP
> +static int sama5d4_wdt_resume(struct device *dev)
> +{
> +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> +
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
> +	if (wdt->mr & AT91_WDT_WDDIS)
> +		wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> +
> +	return 0;
> +}
> +#endif
> +

First time I ever see a system which doesn't need a matching suspend function.
I assume that isn't needed. With that,

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Thanks,
Guenter

> +static SIMPLE_DEV_PM_OPS(sama5d4_wdt_pm_ops, NULL,
> +			 sama5d4_wdt_resume);
> +
>  static struct platform_driver sama5d4_wdt_driver = {
>  	.probe		= sama5d4_wdt_probe,
>  	.remove		= sama5d4_wdt_remove,
>  	.driver		= {
>  		.name	= "sama5d4_wdt",
> +		.pm	= &sama5d4_wdt_pm_ops,
>  		.of_match_table = sama5d4_wdt_of_match,
>  	}
>  };
>