watchdog: cleanup ep93xx platform drivers

PATCH Superseded H Hartley Sweeten 3 participants 8 messages Jan 30, 2017 Watchdog device drivers

Share & tools

H Hartley Sweeten

author

2017-01-30 18:19 UTC #0 lore raw

cover watchdog: cleanup ep93xx platform drivers

The ep93xx_wdt driver is used by EP93xx based platforms for the internal watchdog of the EP93xx processor. The TS-72xx platforms have an additional watchdog provided by the CPLD on those boards.

Cleanup both drivers.

H Hartley Sweeten (2):
watchdog: ep93xx_wdt: cleanup and let the core handle the heartbeat
watchdog: ts72xx_wdt: convert driver to watchdog core

 drivers/watchdog/ep93xx_wdt.c | 115 +++++------
 drivers/watchdog/ts72xx_wdt.c | 447 +++++++++---------------------------------
 2 files changed, 139 insertions(+), 423 deletions(-)

Signature · 1 line

2.10.0

H Hartley Sweeten

author

2017-01-30 18:19 UTC #1 lore raw

PATCH 1/2 watchdog: ep93xx_wdt: cleanup and let the core handle the heartbeat

Cleanup this driver and remove the 200ms heartbeat timer. The core now has the ability to handle the heartbeat.

Signed-off-by H Hartley Sweeten <[email protected]>

Cc Wim Van Sebroeck <[email protected]>

Cc Guenter Roeck <[email protected]>

---
 drivers/watchdog/ep93xx_wdt.c | 115 +++++++++++++++++-------------------------
 1 file changed, 46 insertions(+), 69 deletions(-)

drivers/watchdog/ep93xx_wdt.c +46 ~~-69~~

index 0a4d7cc..756ca47 100644
@@ -19,81 +19,55 @@
  * for us to rely on the user space daemon alone. So we ping the
  * wdt each ~200msec and eventually stop doing it if the user space
  * daemon dies.
- *
- * TODO:
- *
- *	- Test last reset from watchdog status
- *	- Add a few missing ioctls
  */
 
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <linux/watchdog.h>
-#include <linux/timer.h>
 #include <linux/io.h>
 
-#define WDT_VERSION	"0.4"
-
-/* default timeout (secs) */
-#define WDT_TIMEOUT 30
-
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started");
 
-static unsigned int timeout = WDT_TIMEOUT;
-module_param(timeout, uint, 0);
-MODULE_PARM_DESC(timeout,
-	"Watchdog timeout in seconds. (1<=timeout<=3600, default="
-				__MODULE_STRING(WDT_TIMEOUT) ")");
-
-static void __iomem *mmio_base;
-static struct timer_list timer;
-static unsigned long next_heartbeat;
-
 #define EP93XX_WATCHDOG		0x00
 #define EP93XX_WDSTATUS		0x04
 
-/* reset the wdt every ~200ms - the heartbeat of the device is 0.250 seconds*/
-#define WDT_INTERVAL (HZ/5)
-
-static void ep93xx_wdt_timer_ping(unsigned long data)
-{
-	if (time_before(jiffies, next_heartbeat))
-		writel(0x5555, mmio_base + EP93XX_WATCHDOG);
-
-	/* Re-set the timer interval */
-	mod_timer(&timer, jiffies + WDT_INTERVAL);
-}
+struct ep93xx_wdt_priv {
+	void __iomem *mmio;
+	struct watchdog_device wdd;
+};
 
 static int ep93xx_wdt_start(struct watchdog_device *wdd)
 {
-	next_heartbeat = jiffies + (timeout * HZ);
+	struct ep93xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-	writel(0xaaaa, mmio_base + EP93XX_WATCHDOG);
-	mod_timer(&timer, jiffies + WDT_INTERVAL);
+	writel(0xaaaa, priv->mmio + EP93XX_WATCHDOG);
 
 	return 0;
 }
 
 static int ep93xx_wdt_stop(struct watchdog_device *wdd)
 {
-	del_timer_sync(&timer);
-	writel(0xaa55, mmio_base + EP93XX_WATCHDOG);
+	struct ep93xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
+
+	writel(0xaa55, priv->mmio + EP93XX_WATCHDOG);
 
 	return 0;
 }
 
-static int ep93xx_wdt_keepalive(struct watchdog_device *wdd)
+static int ep93xx_wdt_ping(struct watchdog_device *wdd)
 {
-	/* user land ping */
-	next_heartbeat = jiffies + (timeout * HZ);
+	struct ep93xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
+
+	writel(0x5555, priv->mmio + EP93XX_WATCHDOG);
 
 	return 0;
 }
 
 static const struct watchdog_info ep93xx_wdt_ident = {
 	.options	= WDIOF_CARDRESET |
+			  WDIOF_SETTIMEOUT |
 			  WDIOF_MAGICCLOSE |
 			  WDIOF_KEEPALIVEPING,
 	.identity	= "EP93xx Watchdog",

@@ -103,47 +77,48 @@ static struct watchdog_ops ep93xx_wdt_ops = {
 	.owner		= THIS_MODULE,
 	.start		= ep93xx_wdt_start,
 	.stop		= ep93xx_wdt_stop,
-	.ping		= ep93xx_wdt_keepalive,
-};
-
-static struct watchdog_device ep93xx_wdt_wdd = {
-	.info		= &ep93xx_wdt_ident,
-	.ops		= &ep93xx_wdt_ops,
+	.ping		= ep93xx_wdt_ping,
 };
 
 static int ep93xx_wdt_probe(struct platform_device *pdev)
 {
+	struct ep93xx_wdt_priv *priv;
+	struct watchdog_device *wdd;
 	struct resource *res;
 	unsigned long val;
-	int err;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	mmio_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(mmio_base))
-		return PTR_ERR(mmio_base);
+	priv->mmio = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->mmio))
+		return PTR_ERR(priv->mmio);
+
+	val = readl(priv->mmio + EP93XX_WATCHDOG);
 
-	if (timeout < 1 || timeout > 3600) {
-		timeout = WDT_TIMEOUT;
-		dev_warn(&pdev->dev,
-			"timeout value must be 1<=x<=3600, using %d\n",
-			timeout);
-	}
+	wdd = &priv->wdd;
+	wdd->bootstatus = (val & 0x01) ? WDIOF_CARDRESET : 0;
+	wdd->info = &ep93xx_wdt_ident;
+	wdd->ops = &ep93xx_wdt_ops;
+	wdd->timeout = 30;
+	wdd->min_timeout = 1;
+	wdd->max_hw_heartbeat_ms = 200;
+	wdd->parent = &pdev->dev;
 
-	val = readl(mmio_base + EP93XX_WATCHDOG);
-	ep93xx_wdt_wdd.bootstatus = (val & 0x01) ? WDIOF_CARDRESET : 0;
-	ep93xx_wdt_wdd.timeout = timeout;
-	ep93xx_wdt_wdd.parent = &pdev->dev;
+	watchdog_set_nowayout(wdd, nowayout);
 
-	watchdog_set_nowayout(&ep93xx_wdt_wdd, nowayout);
+	watchdog_set_drvdata(wdd, priv);
 
-	setup_timer(&timer, ep93xx_wdt_timer_ping, 1);
+	ret = watchdog_register_device(wdd);
+	if (ret)
+		return ret;
 
-	err = watchdog_register_device(&ep93xx_wdt_wdd);
-	if (err)
-		return err;
+	platform_set_drvdata(pdev, priv);
 
-	dev_info(&pdev->dev,
-		"EP93XX watchdog, driver version " WDT_VERSION "%s\n",
+	dev_info(&pdev->dev, "EP93XX watchdog driver %s\n",
 		(val & 0x08) ? " (nCS1 disable detected)" : "");
 
 	return 0;

@@ -151,7 +126,10 @@ static int ep93xx_wdt_probe(struct platform_device *pdev)
 
 static int ep93xx_wdt_remove(struct platform_device *pdev)
 {
-	watchdog_unregister_device(&ep93xx_wdt_wdd);
+	struct ep93xx_wdt_priv *priv = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&priv->wdd);
+
 	return 0;
 }

@@ -170,4 +148,3 @@ MODULE_AUTHOR("Alessandro Zummo <[email protected]>");
 MODULE_AUTHOR("H Hartley Sweeten <[email protected]>");
 MODULE_DESCRIPTION("EP93xx Watchdog");
 MODULE_LICENSE("GPL");
-MODULE_VERSION(WDT_VERSION);

Signature · 1 line

2.10.0

Guenter Roeck

maintainer

2017-01-30 18:57 UTC #2 lore raw

Re: [PATCH 1/2] watchdog: ep93xx_wdt: cleanup and let the core handle the heartbeat

Quoted 26 lines · H Hartley Sweeten

Cleanup this driver and remove the 200ms heartbeat timer. The core now has the ability to handle the heartbeat.
Signed-off-by H Hartley Sweeten <[email protected]>
Cc Wim Van Sebroeck <[email protected]>
Cc Guenter Roeck <[email protected]>
---
 drivers/watchdog/ep93xx_wdt.c | 115 +++++++++++++++++-------------------------
 1 file changed, 46 insertions(+), 69 deletions(-)
drivers/watchdog/ep93xx_wdt.c +0 -5
index 0a4d7cc..756ca47 100644
@@ -19,81 +19,55 @@
  * for us to rely on the user space daemon alone. So we ping the
  * wdt each ~200msec and eventually stop doing it if the user space
  * daemon dies.
- *
- * TODO:
- *
- *	- Test last reset from watchdog status
- *	- Add a few missing ioctls
  */
 
 #include <linux/platform_device.h>

drivers/watchdog/ep93xx_wdt.c line 39

Quoted 10 lines

drivers/watchdog/ep93xx_wdt.c +0 -6

 #include <linux/module.h>
 #include <linux/watchdog.h>
-#include <linux/timer.h>
 #include <linux/io.h>
 
-#define WDT_VERSION	"0.4"
-
-/* default timeout (secs) */
-#define WDT_TIMEOUT 30
-

Personally I like those constants, even if used only once (I know, it is a good candidate for bikeshedding). Two reasons: 1) It is already there, and 2) It helps seeing the default without having to dig into the code.

drivers/watchdog/ep93xx_wdt.c line 49

Quoted 10 lines

drivers/watchdog/ep93xx_wdt.c +0 -6

 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started");
 
-static unsigned int timeout = WDT_TIMEOUT;
-module_param(timeout, uint, 0);
-MODULE_PARM_DESC(timeout,
-	"Watchdog timeout in seconds. (1<=timeout<=3600, default="
-				__MODULE_STRING(WDT_TIMEOUT) ")");
-

Are you sure you want to take away the means to set the timeout with a module parameter ? You could easily retain the module parameter and call watchdog_init_timeout(wdd, timeout, dev). The parameter should then be initialized with 0, though, to have the watchdog core take the timeout from devicetree if provided.

Quoted 116 lines

drivers/watchdog/ep93xx_wdt.c +35 ~~-40~~

-static void __iomem *mmio_base;
-static struct timer_list timer;
-static unsigned long next_heartbeat;
-
 #define EP93XX_WATCHDOG		0x00
 #define EP93XX_WDSTATUS		0x04
 
-/* reset the wdt every ~200ms - the heartbeat of the device is 0.250 seconds*/
-#define WDT_INTERVAL (HZ/5)
-
-static void ep93xx_wdt_timer_ping(unsigned long data)
-{
-	if (time_before(jiffies, next_heartbeat))
-		writel(0x5555, mmio_base + EP93XX_WATCHDOG);
-
-	/* Re-set the timer interval */
-	mod_timer(&timer, jiffies + WDT_INTERVAL);
-}
+struct ep93xx_wdt_priv {
+	void __iomem *mmio;
+	struct watchdog_device wdd;
+};
 
 static int ep93xx_wdt_start(struct watchdog_device *wdd)
 {
-	next_heartbeat = jiffies + (timeout * HZ);
+	struct ep93xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-	writel(0xaaaa, mmio_base + EP93XX_WATCHDOG);
-	mod_timer(&timer, jiffies + WDT_INTERVAL);
+	writel(0xaaaa, priv->mmio + EP93XX_WATCHDOG);
 
 	return 0;
 }
 
 static int ep93xx_wdt_stop(struct watchdog_device *wdd)
 {
-	del_timer_sync(&timer);
-	writel(0xaa55, mmio_base + EP93XX_WATCHDOG);
+	struct ep93xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
+
+	writel(0xaa55, priv->mmio + EP93XX_WATCHDOG);
 
 	return 0;
 }
 
-static int ep93xx_wdt_keepalive(struct watchdog_device *wdd)
+static int ep93xx_wdt_ping(struct watchdog_device *wdd)
 {
-	/* user land ping */
-	next_heartbeat = jiffies + (timeout * HZ);
+	struct ep93xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
+
+	writel(0x5555, priv->mmio + EP93XX_WATCHDOG);
 
 	return 0;
 }
 
 static const struct watchdog_info ep93xx_wdt_ident = {
 	.options	= WDIOF_CARDRESET |
+			  WDIOF_SETTIMEOUT |
 			  WDIOF_MAGICCLOSE |
 			  WDIOF_KEEPALIVEPING,
 	.identity	= "EP93xx Watchdog",

@@ -103,47 +77,48 @@ static struct watchdog_ops ep93xx_wdt_ops = {
 	.owner		= THIS_MODULE,
 	.start		= ep93xx_wdt_start,
 	.stop		= ep93xx_wdt_stop,
-	.ping		= ep93xx_wdt_keepalive,
-};
-
-static struct watchdog_device ep93xx_wdt_wdd = {
-	.info		= &ep93xx_wdt_ident,
-	.ops		= &ep93xx_wdt_ops,
+	.ping		= ep93xx_wdt_ping,
 };
 
 static int ep93xx_wdt_probe(struct platform_device *pdev)
 {
+	struct ep93xx_wdt_priv *priv;
+	struct watchdog_device *wdd;
 	struct resource *res;
 	unsigned long val;
-	int err;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	mmio_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(mmio_base))
-		return PTR_ERR(mmio_base);
+	priv->mmio = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->mmio))
+		return PTR_ERR(priv->mmio);
+
+	val = readl(priv->mmio + EP93XX_WATCHDOG);
 
-	if (timeout < 1 || timeout > 3600) {
-		timeout = WDT_TIMEOUT;
-		dev_warn(&pdev->dev,
-			"timeout value must be 1<=x<=3600, using %d\n",
-			timeout);
-	}
+	wdd = &priv->wdd;
+	wdd->bootstatus = (val & 0x01) ? WDIOF_CARDRESET : 0;
+	wdd->info = &ep93xx_wdt_ident;
+	wdd->ops = &ep93xx_wdt_ops;
+	wdd->timeout = 30;
+	wdd->min_timeout = 1;
+	wdd->max_hw_heartbeat_ms = 200;
+	wdd->parent = &pdev->dev;
 
-	val = readl(mmio_base + EP93XX_WATCHDOG);

drivers/watchdog/ep93xx_wdt.c static struct watchdog_ops ep93xx_wdt_ops = { · line 115

Quoted 10 lines

drivers/watchdog/ep93xx_wdt.c +3 -5

-	ep93xx_wdt_wdd.bootstatus = (val & 0x01) ? WDIOF_CARDRESET : 0;
-	ep93xx_wdt_wdd.timeout = timeout;
-	ep93xx_wdt_wdd.parent = &pdev->dev;
+	watchdog_set_nowayout(wdd, nowayout);
 
-	watchdog_set_nowayout(&ep93xx_wdt_wdd, nowayout);
+	watchdog_set_drvdata(wdd, priv);
 
-	setup_timer(&timer, ep93xx_wdt_timer_ping, 1);
+	ret = watchdog_register_device(wdd);

Looks like a perfect candidate for devm_watchdog_register_device().

Quoted 38 lines

drivers/watchdog/ep93xx_wdt.c +8 -7

+	if (ret)
+		return ret;
 
-	err = watchdog_register_device(&ep93xx_wdt_wdd);
-	if (err)
-		return err;
+	platform_set_drvdata(pdev, priv);
 
-	dev_info(&pdev->dev,
-		"EP93XX watchdog, driver version " WDT_VERSION "%s\n",
+	dev_info(&pdev->dev, "EP93XX watchdog driver %s\n",
 		(val & 0x08) ? " (nCS1 disable detected)" : "");
 
 	return 0;

@@ -151,7 +126,10 @@ static int ep93xx_wdt_probe(struct platform_device *pdev)
 
 static int ep93xx_wdt_remove(struct platform_device *pdev)
 {
-	watchdog_unregister_device(&ep93xx_wdt_wdd);
+	struct ep93xx_wdt_priv *priv = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&priv->wdd);
+
 	return 0;
 }

@@ -170,4 +148,3 @@ MODULE_AUTHOR("Alessandro Zummo <[email protected]>");
 MODULE_AUTHOR("H Hartley Sweeten <[email protected]>");
 MODULE_DESCRIPTION("EP93xx Watchdog");
 MODULE_LICENSE("GPL");
-MODULE_VERSION(WDT_VERSION);

Signature · 6 lines

2.10.0

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hartley Sweeten

2017-01-30 19:09 UTC #3 lore raw

RE: [PATCH 1/2] watchdog: ep93xx_wdt: cleanup and let the core handle the heartbeat

Quoted 7 lines · Guenter Roeck

Quoted 6 lines · H Hartley Sweeten
Cleanup this driver and remove the 200ms heartbeat timer. The core now has the ability to handle the heartbeat.
Signed-off-by H Hartley Sweeten <[email protected]>
Cc Wim Van Sebroeck <[email protected]>
Cc Guenter Roeck <[email protected]>

Hi Guenter,

I wasn't sure this patch was going to get delivered correctly. I got an "Undeliverable" bounce due to possible spoofing. I am trying to figure out why right now.

Anyway...

<snip>

Quoted 9 lines

drivers/watchdog/ep93xx_wdt.c line 39
Quoted 5 lines
drivers/watchdog/ep93xx_wdt.c +0 -5
-#define WDT_VERSION	"0.4"
-
-/* default timeout (secs) */
-#define WDT_TIMEOUT 30
-
Personally I like those constants, even if used only once (I know, it is a good candidate for bikeshedding). Two reasons: 1) It is already there, and 2) It helps seeing the default without having to dig into the code.

I assume the WDT_VERSION can go away...

As far as the WDT_TIMEOUT, I have no problem leaving it. I was just trying to remove some cruft.

Quoted 4 lines

Quoted 4 lines
static bool nowayout = WATCHDOG_NOWAYOUT; module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started");

Quoted 12 lines

drivers/watchdog/ep93xx_wdt.c line 49
Quoted 6 lines
drivers/watchdog/ep93xx_wdt.c +0 -6
-static unsigned int timeout = WDT_TIMEOUT;
-module_param(timeout, uint, 0);
-MODULE_PARM_DESC(timeout,
-	"Watchdog timeout in seconds. (1<=timeout<=3600, default="
-				__MODULE_STRING(WDT_TIMEOUT) ")");
-
Are you sure you want to take away the means to set the timeout with a module parameter ? You could easily retain the module parameter and call watchdog_init_timeout(wdd, timeout, dev). The parameter should then be initialized with 0, though, to have the watchdog core take the timeout from devicetree if provided.

Again, I have no problem leaving this. I personally don't use it but someone else might. I'm not sure if the ep93xx will ever get converted to devicetree but I might figure it one eventually.

<snip>

Quoted 6 lines

drivers/watchdog/ep93xx_wdt.c static struct watchdog_ops ep93xx_wdt_ops = { · line 115
Quoted 4 lines
drivers/watchdog/ep93xx_wdt.c +2 -1
+	watchdog_set_drvdata(wdd, priv);
 
-	setup_timer(&timer, ep93xx_wdt_timer_ping, 1);
+	ret = watchdog_register_device(wdd);
Looks like a perfect candidate for devm_watchdog_register_device().

I just saw your patches that do this. I will update this patch to use it.

Thanks,
Hartley

Guenter Roeck

maintainer

2017-01-30 19:33 UTC #4 lore raw

Re: [PATCH 1/2] watchdog: ep93xx_wdt: cleanup and let the core handle the heartbeat

Quoted 5 lines · Hartley Sweeten

Quoted 4 lines · Guenter Roeck
Quoted 3 lines · H Hartley Sweeten
Cleanup this driver and remove the 200ms heartbeat timer. The core now has the ability to handle the heartbeat.

Quoted 9 lines

Quoted 3 lines
Quoted 3 lines
Signed-off-by H Hartley Sweeten <[email protected]>
Cc Wim Van Sebroeck <[email protected]>
Cc Guenter Roeck <[email protected]>
Hi Guenter,
I wasn't sure this patch was going to get delivered correctly. I got an "Undeliverable" bounce due to possible spoofing. I am trying to figure out why right now.

Interesting. If you find out, please let me know. Either case, I am subscribed to the watchdog mailing list, so should get all e-mail sent to it. I also have a patchwork instance running, so I see it there as well if the watchdog mailing list is copied.

Quoted 14 lines

Anyway...
<snip>
Quoted 9 lines
drivers/watchdog/ep93xx_wdt.c line 39
Quoted 5 lines
drivers/watchdog/ep93xx_wdt.c +0 -5
-#define WDT_VERSION	"0.4"
-
-/* default timeout (secs) */
-#define WDT_TIMEOUT 30
-
Personally I like those constants, even if used only once (I know, it is a good candidate for bikeshedding). Two reasons: 1) It is already there, and 2) It helps seeing the default without having to dig into the code.

Quoted 2 lines

I assume the WDT_VERSION can go away...

Yes, that is pretty much useless.

Quoted 20 lines

As far as the WDT_TIMEOUT, I have no problem leaving it. I was just trying to remove some cruft.
Quoted 4 lines
Quoted 4 lines
static bool nowayout = WATCHDOG_NOWAYOUT; module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started");
Quoted 12 lines
drivers/watchdog/ep93xx_wdt.c line 49
Quoted 6 lines
drivers/watchdog/ep93xx_wdt.c +0 -6
-static unsigned int timeout = WDT_TIMEOUT;
-module_param(timeout, uint, 0);
-MODULE_PARM_DESC(timeout,
-	"Watchdog timeout in seconds. (1<=timeout<=3600, default="
-				__MODULE_STRING(WDT_TIMEOUT) ")");
-
Are you sure you want to take away the means to set the timeout with a module parameter ? You could easily retain the module parameter and call watchdog_init_timeout(wdd, timeout, dev). The parameter should then be initialized with 0, though, to have the watchdog core take the timeout from devicetree if provided.

Quoted 4 lines

Again, I have no problem leaving this. I personally don't use it but someone else might. I'm not sure if the ep93xx will ever get converted to devicetree but I might figure it one eventually.

Thinking more about it, we should really leave the module parameter in. As you say, someone else may be using it, and we should not change the interface to user space. Using watchdog_init_timeout() is useful to check the range (not that it really matters here); that it reads a value from devicetree is an additional benefit.

Thanks,
Guenter

H Hartley Sweeten

author

2017-01-30 18:17 UTC #5 lore raw

PATCH 2/2 watchdog: ts72xx_wdt: convert driver to watchdog core

In reply to H Hartley Sweeten

Cleanup this driver and convert it to use the watchdog framework API.

Signed-off-by H Hartley Sweeten <[email protected]>

Cc Mika Westerberg <[email protected]>

Cc Wim Van Sebroeck <[email protected]>

Cc Guenter Roeck <[email protected]>

---
 drivers/watchdog/ts72xx_wdt.c | 447 +++++++++---------------------------------
 1 file changed, 93 insertions(+), 354 deletions(-)

drivers/watchdog/ts72xx_wdt.c +93 ~~-354~~

index 4b54193..857ab71 100644
@@ -13,413 +13,149 @@
  * warranty of any kind, whether express or implied.
  */
 
-#include <linux/fs.h>
-#include <linux/io.h>
-#include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/miscdevice.h>
-#include <linux/mutex.h>
 #include <linux/platform_device.h>
-#include <linux/slab.h>
+#include <linux/module.h>
 #include <linux/watchdog.h>
-#include <linux/uaccess.h>
-
-#define TS72XX_WDT_FEED_VAL		0x05
-#define TS72XX_WDT_DEFAULT_TIMEOUT	8
-
-static int timeout = TS72XX_WDT_DEFAULT_TIMEOUT;
-module_param(timeout, int, 0);
-MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
-			  "(1 <= timeout <= 8, default="
-			  __MODULE_STRING(TS72XX_WDT_DEFAULT_TIMEOUT)
-			  ")");
+#include <linux/io.h>
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Disable watchdog shutdown on close");
 
-/**
- * struct ts72xx_wdt - watchdog control structure
- * @lock: lock that protects this structure
- * @regval: watchdog timeout value suitable for control register
- * @flags: flags controlling watchdog device state
- * @control_reg: watchdog control register
- * @feed_reg: watchdog feed register
- * @pdev: back pointer to platform dev
- */
-struct ts72xx_wdt {
-	struct mutex	lock;
-	int		regval;
-
-#define TS72XX_WDT_BUSY_FLAG		1
-#define TS72XX_WDT_EXPECT_CLOSE_FLAG	2
-	int		flags;
+/* priv->control_reg */
+#define TS72XX_WDT_CTRL_DISABLE		0x00
+#define TS72XX_WDT_CTRL_250MS		0x01
+#define TS72XX_WDT_CTRL_500MS		0x02
+#define TS72XX_WDT_CTRL_1SEC		0x03
+#define TS72XX_WDT_CTRL_RESERVED	0x04
+#define TS72XX_WDT_CTRL_2SEC		0x05
+#define TS72XX_WDT_CTRL_4SEC		0x06
+#define TS72XX_WDT_CTRL_8SEC		0x07
+
+/* priv->feed_reg */
+#define TS72XX_WDT_FEED_VAL		0x05
 
+struct ts72xx_wdt_priv {
 	void __iomem	*control_reg;
 	void __iomem	*feed_reg;
-
-	struct platform_device *pdev;
+	struct watchdog_device wdd;
+	unsigned char regval;
 };
 
-static struct platform_device *ts72xx_wdt_pdev;
-
-/*
- * TS-72xx Watchdog supports following timeouts (value written
- * to control register):
- *	value	description
- *	-------------------------
- *	0x00	watchdog disabled
- *	0x01	250ms
- *	0x02	500ms
- *	0x03	1s
- *	0x04	reserved
- *	0x05	2s
- *	0x06	4s
- *	0x07	8s
- *
- * Timeouts below 1s are not very usable so we don't
- * allow them at all.
- *
- * We provide two functions that convert between these:
- * timeout_to_regval() and regval_to_timeout().
- */
-static const struct {
-	int	timeout;
-	int	regval;
-} ts72xx_wdt_map[] = {
-	{ 1, 3 },
-	{ 2, 5 },
-	{ 4, 6 },
-	{ 8, 7 },
-};
-
-/**
- * timeout_to_regval() - converts given timeout to control register value
- * @new_timeout: timeout in seconds to be converted
- *
- * Function converts given @new_timeout into valid value that can
- * be programmed into watchdog control register. When conversion is
- * not possible, function returns %-EINVAL.
- */
-static int timeout_to_regval(int new_timeout)
+static int ts72xx_wdt_start(struct watchdog_device *wdd)
 {
-	int i;
+	struct ts72xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-	/* first limit it to 1 - 8 seconds */
-	new_timeout = clamp_val(new_timeout, 1, 8);
+	writeb(TS72XX_WDT_FEED_VAL, priv->feed_reg);
+	writeb(priv->regval, priv->control_reg);
 
-	for (i = 0; i < ARRAY_SIZE(ts72xx_wdt_map); i++) {
-		if (ts72xx_wdt_map[i].timeout >= new_timeout)
-			return ts72xx_wdt_map[i].regval;
-	}
-
-	return -EINVAL;
-}
-
-/**
- * regval_to_timeout() - converts control register value to timeout
- * @regval: control register value to be converted
- *
- * Function converts given @regval to timeout in seconds (1, 2, 4 or 8).
- * If @regval cannot be converted, function returns %-EINVAL.
- */
-static int regval_to_timeout(int regval)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(ts72xx_wdt_map); i++) {
-		if (ts72xx_wdt_map[i].regval == regval)
-			return ts72xx_wdt_map[i].timeout;
-	}
-
-	return -EINVAL;
-}
-
-/**
- * ts72xx_wdt_kick() - kick the watchdog
- * @wdt: watchdog to be kicked
- *
- * Called with @wdt->lock held.
- */
-static inline void ts72xx_wdt_kick(struct ts72xx_wdt *wdt)
-{
-	__raw_writeb(TS72XX_WDT_FEED_VAL, wdt->feed_reg);
-}
-
-/**
- * ts72xx_wdt_start() - starts the watchdog timer
- * @wdt: watchdog to be started
- *
- * This function programs timeout to watchdog timer
- * and starts it.
- *
- * Called with @wdt->lock held.
- */
-static void ts72xx_wdt_start(struct ts72xx_wdt *wdt)
-{
-	/*
-	 * To program the wdt, it first must be "fed" and
-	 * only after that (within 30 usecs) the configuration
-	 * can be changed.
-	 */
-	ts72xx_wdt_kick(wdt);
-	__raw_writeb((u8)wdt->regval, wdt->control_reg);
-}
-
-/**
- * ts72xx_wdt_stop() - stops the watchdog timer
- * @wdt: watchdog to be stopped
- *
- * Called with @wdt->lock held.
- */
-static void ts72xx_wdt_stop(struct ts72xx_wdt *wdt)
-{
-	ts72xx_wdt_kick(wdt);
-	__raw_writeb(0, wdt->control_reg);
+	return 0;
 }
 
-static int ts72xx_wdt_open(struct inode *inode, struct file *file)
+static int ts72xx_wdt_stop(struct watchdog_device *wdd)
 {
-	struct ts72xx_wdt *wdt = platform_get_drvdata(ts72xx_wdt_pdev);
-	int regval;
-
-	/*
-	 * Try to convert default timeout to valid register
-	 * value first.
-	 */
-	regval = timeout_to_regval(timeout);
-	if (regval < 0) {
-		dev_err(&wdt->pdev->dev,
-			"failed to convert timeout (%d) to register value\n",
-			timeout);
-		return regval;
-	}
-
-	if (mutex_lock_interruptible(&wdt->lock))
-		return -ERESTARTSYS;
-
-	if ((wdt->flags & TS72XX_WDT_BUSY_FLAG) != 0) {
-		mutex_unlock(&wdt->lock);
-		return -EBUSY;
-	}
+	struct ts72xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-	wdt->flags = TS72XX_WDT_BUSY_FLAG;
-	wdt->regval = regval;
-	file->private_data = wdt;
+	writeb(TS72XX_WDT_FEED_VAL, priv->feed_reg);
+	writeb(TS72XX_WDT_CTRL_DISABLE, priv->control_reg);
 
-	ts72xx_wdt_start(wdt);
-
-	mutex_unlock(&wdt->lock);
-	return nonseekable_open(inode, file);
+	return 0;
 }
 
-static int ts72xx_wdt_release(struct inode *inode, struct file *file)
+static int ts72xx_wdt_ping(struct watchdog_device *wdd)
 {
-	struct ts72xx_wdt *wdt = file->private_data;
-
-	if (mutex_lock_interruptible(&wdt->lock))
-		return -ERESTARTSYS;
-
-	if ((wdt->flags & TS72XX_WDT_EXPECT_CLOSE_FLAG) != 0) {
-		ts72xx_wdt_stop(wdt);
-	} else {
-		dev_warn(&wdt->pdev->dev,
-			 "TS-72XX WDT device closed unexpectly. "
-			 "Watchdog timer will not stop!\n");
-		/*
-		 * Kick it one more time, to give userland some time
-		 * to recover (for example, respawning the kicker
-		 * daemon).
-		 */
-		ts72xx_wdt_kick(wdt);
-	}
+	struct ts72xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-	wdt->flags = 0;
+	writeb(TS72XX_WDT_FEED_VAL, priv->feed_reg);
 
-	mutex_unlock(&wdt->lock);
 	return 0;
 }
 
-static ssize_t ts72xx_wdt_write(struct file *file,
-				const char __user *data,
-				size_t len,
-				loff_t *ppos)
+static int ts72xx_wdt_settimeout(struct watchdog_device *wdd, unsigned int to)
 {
-	struct ts72xx_wdt *wdt = file->private_data;
-
-	if (!len)
-		return 0;
-
-	if (mutex_lock_interruptible(&wdt->lock))
-		return -ERESTARTSYS;
-
-	ts72xx_wdt_kick(wdt);
-
-	/*
-	 * Support for magic character closing. User process
-	 * writes 'V' into the device, just before it is closed.
-	 * This means that we know that the wdt timer can be
-	 * stopped after user closes the device.
-	 */
-	if (!nowayout) {
-		int i;
-
-		for (i = 0; i < len; i++) {
-			char c;
-
-			/* In case it was set long ago */
-			wdt->flags &= ~TS72XX_WDT_EXPECT_CLOSE_FLAG;
-
-			if (get_user(c, data + i)) {
-				mutex_unlock(&wdt->lock);
-				return -EFAULT;
-			}
-			if (c == 'V') {
-				wdt->flags |= TS72XX_WDT_EXPECT_CLOSE_FLAG;
-				break;
-			}
-		}
-	}
-
-	mutex_unlock(&wdt->lock);
-	return len;
-}
+	struct ts72xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-static const struct watchdog_info winfo = {
-	.options		= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
-				  WDIOF_MAGICCLOSE,
-	.firmware_version	= 1,
-	.identity		= "TS-72XX WDT",
-};
-
-static long ts72xx_wdt_ioctl(struct file *file, unsigned int cmd,
-			     unsigned long arg)
-{
-	struct ts72xx_wdt *wdt = file->private_data;
-	void __user *argp = (void __user *)arg;
-	int __user *p = (int __user *)argp;
-	int error = 0;
-
-	if (mutex_lock_interruptible(&wdt->lock))
-		return -ERESTARTSYS;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		if (copy_to_user(argp, &winfo, sizeof(winfo)))
-			error = -EFAULT;
+	switch (to) {
+	case 1:
+		priv->regval = TS72XX_WDT_CTRL_1SEC;
 		break;
-
-	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
-		error = put_user(0, p);
+	case 2:
+		priv->regval = TS72XX_WDT_CTRL_2SEC;
 		break;
-
-	case WDIOC_KEEPALIVE:
-		ts72xx_wdt_kick(wdt);
+	case 4:
+		priv->regval = TS72XX_WDT_CTRL_4SEC;
 		break;
-
-	case WDIOC_SETOPTIONS: {
-		int options;
-
-		error = get_user(options, p);
-		if (error)
-			break;
-
-		error = -EINVAL;
-
-		if ((options & WDIOS_DISABLECARD) != 0) {
-			ts72xx_wdt_stop(wdt);
-			error = 0;
-		}
-		if ((options & WDIOS_ENABLECARD) != 0) {
-			ts72xx_wdt_start(wdt);
-			error = 0;
-		}
-
+	case 8:
+		priv->regval = TS72XX_WDT_CTRL_8SEC;
 		break;
+	default:
+		return -EINVAL;
 	}
 
-	case WDIOC_SETTIMEOUT: {
-		int new_timeout;
-		int regval;
-
-		error = get_user(new_timeout, p);
-		if (error)
-			break;
-
-		regval = timeout_to_regval(new_timeout);
-		if (regval < 0) {
-			error = regval;
-			break;
-		}
-		ts72xx_wdt_stop(wdt);
-		wdt->regval = regval;
-		ts72xx_wdt_start(wdt);
-
-		/*FALLTHROUGH*/
-	}
-
-	case WDIOC_GETTIMEOUT:
-		error = put_user(regval_to_timeout(wdt->regval), p);
-		break;
+	wdd->timeout = to;
 
-	default:
-		error = -ENOTTY;
-		break;
+	if (watchdog_active(wdd)) {
+		ts72xx_wdt_stop(wdd);
+		ts72xx_wdt_start(wdd);
 	}
 
-	mutex_unlock(&wdt->lock);
-	return error;
+	return 0;
 }
 
-static const struct file_operations ts72xx_wdt_fops = {
-	.owner		= THIS_MODULE,
-	.llseek		= no_llseek,
-	.open		= ts72xx_wdt_open,
-	.release	= ts72xx_wdt_release,
-	.write		= ts72xx_wdt_write,
-	.unlocked_ioctl	= ts72xx_wdt_ioctl,
+static const struct watchdog_info ts72xx_wdt_ident = {
+	.options		= WDIOF_KEEPALIVEPING |
+				  WDIOF_SETTIMEOUT |
+				  WDIOF_MAGICCLOSE,
+	.firmware_version	= 1,
+	.identity		= "TS-72XX WDT",
 };
 
-static struct miscdevice ts72xx_wdt_miscdev = {
-	.minor		= WATCHDOG_MINOR,
-	.name		= "watchdog",
-	.fops		= &ts72xx_wdt_fops,
+static struct watchdog_ops ts72xx_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= ts72xx_wdt_start,
+	.stop		= ts72xx_wdt_stop,
+	.ping		= ts72xx_wdt_ping,
+	.set_timeout	= ts72xx_wdt_settimeout,
 };
 
 static int ts72xx_wdt_probe(struct platform_device *pdev)
 {
-	struct ts72xx_wdt *wdt;
-	struct resource *r1, *r2;
-	int error = 0;
+	struct ts72xx_wdt_priv *priv;
+	struct watchdog_device *wdd;
+	struct resource *res;
+	int ret;
 
-	wdt = devm_kzalloc(&pdev->dev, sizeof(struct ts72xx_wdt), GFP_KERNEL);
-	if (!wdt)
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
 		return -ENOMEM;
 
-	r1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	wdt->control_reg = devm_ioremap_resource(&pdev->dev, r1);
-	if (IS_ERR(wdt->control_reg))
-		return PTR_ERR(wdt->control_reg);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->control_reg = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->control_reg))
+		return PTR_ERR(priv->control_reg);
 
-	r2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	wdt->feed_reg = devm_ioremap_resource(&pdev->dev, r2);
-	if (IS_ERR(wdt->feed_reg))
-		return PTR_ERR(wdt->feed_reg);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	priv->feed_reg = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->feed_reg))
+		return PTR_ERR(priv->feed_reg);
 
-	platform_set_drvdata(pdev, wdt);
-	ts72xx_wdt_pdev = pdev;
-	wdt->pdev = pdev;
-	mutex_init(&wdt->lock);
+	wdd = &priv->wdd;
+	wdd->info = &ts72xx_wdt_ident;
+	wdd->ops = &ts72xx_wdt_ops;
+	wdd->min_timeout = 1;
+	wdd->max_timeout = 8;
+	wdd->timeout = 8;
+	wdd->parent = &pdev->dev;
 
-	/* make sure that the watchdog is disabled */
-	ts72xx_wdt_stop(wdt);
+	watchdog_set_nowayout(wdd, nowayout);
 
-	error = misc_register(&ts72xx_wdt_miscdev);
-	if (error) {
-		dev_err(&pdev->dev, "failed to register miscdev\n");
-		return error;
-	}
+	watchdog_set_drvdata(wdd, priv);
+
+	ret = watchdog_register_device(wdd);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, priv);
 
 	dev_info(&pdev->dev, "TS-72xx Watchdog driver\n");

@@ -428,7 +164,10 @@ static int ts72xx_wdt_probe(struct platform_device *pdev)
 
 static int ts72xx_wdt_remove(struct platform_device *pdev)
 {
-	misc_deregister(&ts72xx_wdt_miscdev);
+	struct ts72xx_wdt_priv *priv = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&priv->wdd);
+
 	return 0;
 }

Signature · 1 line

2.10.0

Guenter Roeck

maintainer

2017-01-30 19:04 UTC #6 lore raw

Re: [PATCH 2/2] watchdog: ts72xx_wdt: convert driver to watchdog core

Quoted 29 lines · H Hartley Sweeten

Cleanup this driver and convert it to use the watchdog framework API.

Signed-off-by H Hartley Sweeten <[email protected]>

Cc Mika Westerberg <[email protected]>

Cc Wim Van Sebroeck <[email protected]>

Cc Guenter Roeck <[email protected]>

---
 drivers/watchdog/ts72xx_wdt.c | 447 +++++++++---------------------------------
 1 file changed, 93 insertions(+), 354 deletions(-)

drivers/watchdog/ts72xx_wdt.c +1 -8

index 4b54193..857ab71 100644
@@ -13,413 +13,149 @@
  * warranty of any kind, whether express or implied.
  */
 
-#include <linux/fs.h>
-#include <linux/io.h>
-#include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/miscdevice.h>
-#include <linux/mutex.h>
 #include <linux/platform_device.h>
-#include <linux/slab.h>
+#include <linux/module.h>
 #include <linux/watchdog.h>
-#include <linux/uaccess.h>

drivers/watchdog/ts72xx_wdt.c line 35

Quoted 10 lines

drivers/watchdog/ts72xx_wdt.c +0 ~~-10~~

-
-#define TS72XX_WDT_FEED_VAL		0x05
-#define TS72XX_WDT_DEFAULT_TIMEOUT	8
-
-static int timeout = TS72XX_WDT_DEFAULT_TIMEOUT;
-module_param(timeout, int, 0);
-MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
-			  "(1 <= timeout <= 8, default="
-			  __MODULE_STRING(TS72XX_WDT_DEFAULT_TIMEOUT)
-			  ")");

Same question as with patch #1 - are you sure you want to take this away ? You might just drop the limits instead (also see below).

Quoted 446 lines

drivers/watchdog/ts72xx_wdt.c +73 ~~-324~~

+#include <linux/io.h>
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Disable watchdog shutdown on close");
 
-/**
- * struct ts72xx_wdt - watchdog control structure
- * @lock: lock that protects this structure
- * @regval: watchdog timeout value suitable for control register
- * @flags: flags controlling watchdog device state
- * @control_reg: watchdog control register
- * @feed_reg: watchdog feed register
- * @pdev: back pointer to platform dev
- */
-struct ts72xx_wdt {
-	struct mutex	lock;
-	int		regval;
-
-#define TS72XX_WDT_BUSY_FLAG		1
-#define TS72XX_WDT_EXPECT_CLOSE_FLAG	2
-	int		flags;
+/* priv->control_reg */
+#define TS72XX_WDT_CTRL_DISABLE		0x00
+#define TS72XX_WDT_CTRL_250MS		0x01
+#define TS72XX_WDT_CTRL_500MS		0x02
+#define TS72XX_WDT_CTRL_1SEC		0x03
+#define TS72XX_WDT_CTRL_RESERVED	0x04
+#define TS72XX_WDT_CTRL_2SEC		0x05
+#define TS72XX_WDT_CTRL_4SEC		0x06
+#define TS72XX_WDT_CTRL_8SEC		0x07
+
+/* priv->feed_reg */
+#define TS72XX_WDT_FEED_VAL		0x05
 
+struct ts72xx_wdt_priv {
 	void __iomem	*control_reg;
 	void __iomem	*feed_reg;
-
-	struct platform_device *pdev;
+	struct watchdog_device wdd;
+	unsigned char regval;
 };
 
-static struct platform_device *ts72xx_wdt_pdev;
-
-/*
- * TS-72xx Watchdog supports following timeouts (value written
- * to control register):
- *	value	description
- *	-------------------------
- *	0x00	watchdog disabled
- *	0x01	250ms
- *	0x02	500ms
- *	0x03	1s
- *	0x04	reserved
- *	0x05	2s
- *	0x06	4s
- *	0x07	8s
- *
- * Timeouts below 1s are not very usable so we don't
- * allow them at all.
- *
- * We provide two functions that convert between these:
- * timeout_to_regval() and regval_to_timeout().
- */
-static const struct {
-	int	timeout;
-	int	regval;
-} ts72xx_wdt_map[] = {
-	{ 1, 3 },
-	{ 2, 5 },
-	{ 4, 6 },
-	{ 8, 7 },
-};
-
-/**
- * timeout_to_regval() - converts given timeout to control register value
- * @new_timeout: timeout in seconds to be converted
- *
- * Function converts given @new_timeout into valid value that can
- * be programmed into watchdog control register. When conversion is
- * not possible, function returns %-EINVAL.
- */
-static int timeout_to_regval(int new_timeout)
+static int ts72xx_wdt_start(struct watchdog_device *wdd)
 {
-	int i;
+	struct ts72xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-	/* first limit it to 1 - 8 seconds */
-	new_timeout = clamp_val(new_timeout, 1, 8);
+	writeb(TS72XX_WDT_FEED_VAL, priv->feed_reg);
+	writeb(priv->regval, priv->control_reg);
 
-	for (i = 0; i < ARRAY_SIZE(ts72xx_wdt_map); i++) {
-		if (ts72xx_wdt_map[i].timeout >= new_timeout)
-			return ts72xx_wdt_map[i].regval;
-	}
-
-	return -EINVAL;
-}
-
-/**
- * regval_to_timeout() - converts control register value to timeout
- * @regval: control register value to be converted
- *
- * Function converts given @regval to timeout in seconds (1, 2, 4 or 8).
- * If @regval cannot be converted, function returns %-EINVAL.
- */
-static int regval_to_timeout(int regval)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(ts72xx_wdt_map); i++) {
-		if (ts72xx_wdt_map[i].regval == regval)
-			return ts72xx_wdt_map[i].timeout;
-	}
-
-	return -EINVAL;
-}
-
-/**
- * ts72xx_wdt_kick() - kick the watchdog
- * @wdt: watchdog to be kicked
- *
- * Called with @wdt->lock held.
- */
-static inline void ts72xx_wdt_kick(struct ts72xx_wdt *wdt)
-{
-	__raw_writeb(TS72XX_WDT_FEED_VAL, wdt->feed_reg);
-}
-
-/**
- * ts72xx_wdt_start() - starts the watchdog timer
- * @wdt: watchdog to be started
- *
- * This function programs timeout to watchdog timer
- * and starts it.
- *
- * Called with @wdt->lock held.
- */
-static void ts72xx_wdt_start(struct ts72xx_wdt *wdt)
-{
-	/*
-	 * To program the wdt, it first must be "fed" and
-	 * only after that (within 30 usecs) the configuration
-	 * can be changed.
-	 */
-	ts72xx_wdt_kick(wdt);
-	__raw_writeb((u8)wdt->regval, wdt->control_reg);
-}
-
-/**
- * ts72xx_wdt_stop() - stops the watchdog timer
- * @wdt: watchdog to be stopped
- *
- * Called with @wdt->lock held.
- */
-static void ts72xx_wdt_stop(struct ts72xx_wdt *wdt)
-{
-	ts72xx_wdt_kick(wdt);
-	__raw_writeb(0, wdt->control_reg);
+	return 0;
 }
 
-static int ts72xx_wdt_open(struct inode *inode, struct file *file)
+static int ts72xx_wdt_stop(struct watchdog_device *wdd)
 {
-	struct ts72xx_wdt *wdt = platform_get_drvdata(ts72xx_wdt_pdev);
-	int regval;
-
-	/*
-	 * Try to convert default timeout to valid register
-	 * value first.
-	 */
-	regval = timeout_to_regval(timeout);
-	if (regval < 0) {
-		dev_err(&wdt->pdev->dev,
-			"failed to convert timeout (%d) to register value\n",
-			timeout);
-		return regval;
-	}
-
-	if (mutex_lock_interruptible(&wdt->lock))
-		return -ERESTARTSYS;
-
-	if ((wdt->flags & TS72XX_WDT_BUSY_FLAG) != 0) {
-		mutex_unlock(&wdt->lock);
-		return -EBUSY;
-	}
+	struct ts72xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-	wdt->flags = TS72XX_WDT_BUSY_FLAG;
-	wdt->regval = regval;
-	file->private_data = wdt;
+	writeb(TS72XX_WDT_FEED_VAL, priv->feed_reg);
+	writeb(TS72XX_WDT_CTRL_DISABLE, priv->control_reg);
 
-	ts72xx_wdt_start(wdt);
-
-	mutex_unlock(&wdt->lock);
-	return nonseekable_open(inode, file);
+	return 0;
 }
 
-static int ts72xx_wdt_release(struct inode *inode, struct file *file)
+static int ts72xx_wdt_ping(struct watchdog_device *wdd)
 {
-	struct ts72xx_wdt *wdt = file->private_data;
-
-	if (mutex_lock_interruptible(&wdt->lock))
-		return -ERESTARTSYS;
-
-	if ((wdt->flags & TS72XX_WDT_EXPECT_CLOSE_FLAG) != 0) {
-		ts72xx_wdt_stop(wdt);
-	} else {
-		dev_warn(&wdt->pdev->dev,
-			 "TS-72XX WDT device closed unexpectly. "
-			 "Watchdog timer will not stop!\n");
-		/*
-		 * Kick it one more time, to give userland some time
-		 * to recover (for example, respawning the kicker
-		 * daemon).
-		 */
-		ts72xx_wdt_kick(wdt);
-	}
+	struct ts72xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-	wdt->flags = 0;
+	writeb(TS72XX_WDT_FEED_VAL, priv->feed_reg);
 
-	mutex_unlock(&wdt->lock);
 	return 0;
 }
 
-static ssize_t ts72xx_wdt_write(struct file *file,
-				const char __user *data,
-				size_t len,
-				loff_t *ppos)
+static int ts72xx_wdt_settimeout(struct watchdog_device *wdd, unsigned int to)
 {
-	struct ts72xx_wdt *wdt = file->private_data;
-
-	if (!len)
-		return 0;
-
-	if (mutex_lock_interruptible(&wdt->lock))
-		return -ERESTARTSYS;
-
-	ts72xx_wdt_kick(wdt);
-
-	/*
-	 * Support for magic character closing. User process
-	 * writes 'V' into the device, just before it is closed.
-	 * This means that we know that the wdt timer can be
-	 * stopped after user closes the device.
-	 */
-	if (!nowayout) {
-		int i;
-
-		for (i = 0; i < len; i++) {
-			char c;
-
-			/* In case it was set long ago */
-			wdt->flags &= ~TS72XX_WDT_EXPECT_CLOSE_FLAG;
-
-			if (get_user(c, data + i)) {
-				mutex_unlock(&wdt->lock);
-				return -EFAULT;
-			}
-			if (c == 'V') {
-				wdt->flags |= TS72XX_WDT_EXPECT_CLOSE_FLAG;
-				break;
-			}
-		}
-	}
-
-	mutex_unlock(&wdt->lock);
-	return len;
-}
+	struct ts72xx_wdt_priv *priv = watchdog_get_drvdata(wdd);
 
-static const struct watchdog_info winfo = {
-	.options		= WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
-				  WDIOF_MAGICCLOSE,
-	.firmware_version	= 1,
-	.identity		= "TS-72XX WDT",
-};
-
-static long ts72xx_wdt_ioctl(struct file *file, unsigned int cmd,
-			     unsigned long arg)
-{
-	struct ts72xx_wdt *wdt = file->private_data;
-	void __user *argp = (void __user *)arg;
-	int __user *p = (int __user *)argp;
-	int error = 0;
-
-	if (mutex_lock_interruptible(&wdt->lock))
-		return -ERESTARTSYS;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		if (copy_to_user(argp, &winfo, sizeof(winfo)))
-			error = -EFAULT;
+	switch (to) {
+	case 1:
+		priv->regval = TS72XX_WDT_CTRL_1SEC;
 		break;
-
-	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
-		error = put_user(0, p);
+	case 2:
+		priv->regval = TS72XX_WDT_CTRL_2SEC;
 		break;
-
-	case WDIOC_KEEPALIVE:
-		ts72xx_wdt_kick(wdt);
+	case 4:
+		priv->regval = TS72XX_WDT_CTRL_4SEC;
 		break;
-
-	case WDIOC_SETOPTIONS: {
-		int options;
-
-		error = get_user(options, p);
-		if (error)
-			break;
-
-		error = -EINVAL;
-
-		if ((options & WDIOS_DISABLECARD) != 0) {
-			ts72xx_wdt_stop(wdt);
-			error = 0;
-		}
-		if ((options & WDIOS_ENABLECARD) != 0) {
-			ts72xx_wdt_start(wdt);
-			error = 0;
-		}
-
+	case 8:
+		priv->regval = TS72XX_WDT_CTRL_8SEC;
 		break;
+	default:
+		return -EINVAL;
 	}
 
-	case WDIOC_SETTIMEOUT: {
-		int new_timeout;
-		int regval;
-
-		error = get_user(new_timeout, p);
-		if (error)
-			break;
-
-		regval = timeout_to_regval(new_timeout);
-		if (regval < 0) {
-			error = regval;
-			break;
-		}
-		ts72xx_wdt_stop(wdt);
-		wdt->regval = regval;
-		ts72xx_wdt_start(wdt);
-
-		/*FALLTHROUGH*/
-	}
-
-	case WDIOC_GETTIMEOUT:
-		error = put_user(regval_to_timeout(wdt->regval), p);
-		break;
+	wdd->timeout = to;
 
-	default:
-		error = -ENOTTY;
-		break;
+	if (watchdog_active(wdd)) {
+		ts72xx_wdt_stop(wdd);
+		ts72xx_wdt_start(wdd);
 	}
 
-	mutex_unlock(&wdt->lock);
-	return error;
+	return 0;
 }
 
-static const struct file_operations ts72xx_wdt_fops = {
-	.owner		= THIS_MODULE,
-	.llseek		= no_llseek,
-	.open		= ts72xx_wdt_open,
-	.release	= ts72xx_wdt_release,
-	.write		= ts72xx_wdt_write,
-	.unlocked_ioctl	= ts72xx_wdt_ioctl,
+static const struct watchdog_info ts72xx_wdt_ident = {
+	.options		= WDIOF_KEEPALIVEPING |
+				  WDIOF_SETTIMEOUT |
+				  WDIOF_MAGICCLOSE,
+	.firmware_version	= 1,
+	.identity		= "TS-72XX WDT",
 };
 
-static struct miscdevice ts72xx_wdt_miscdev = {
-	.minor		= WATCHDOG_MINOR,
-	.name		= "watchdog",
-	.fops		= &ts72xx_wdt_fops,
+static struct watchdog_ops ts72xx_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= ts72xx_wdt_start,
+	.stop		= ts72xx_wdt_stop,
+	.ping		= ts72xx_wdt_ping,
+	.set_timeout	= ts72xx_wdt_settimeout,
 };
 
 static int ts72xx_wdt_probe(struct platform_device *pdev)
 {
-	struct ts72xx_wdt *wdt;
-	struct resource *r1, *r2;
-	int error = 0;
+	struct ts72xx_wdt_priv *priv;
+	struct watchdog_device *wdd;
+	struct resource *res;
+	int ret;
 
-	wdt = devm_kzalloc(&pdev->dev, sizeof(struct ts72xx_wdt), GFP_KERNEL);
-	if (!wdt)
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
 		return -ENOMEM;
 
-	r1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	wdt->control_reg = devm_ioremap_resource(&pdev->dev, r1);
-	if (IS_ERR(wdt->control_reg))
-		return PTR_ERR(wdt->control_reg);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->control_reg = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->control_reg))
+		return PTR_ERR(priv->control_reg);
 
-	r2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	wdt->feed_reg = devm_ioremap_resource(&pdev->dev, r2);
-	if (IS_ERR(wdt->feed_reg))
-		return PTR_ERR(wdt->feed_reg);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	priv->feed_reg = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->feed_reg))
+		return PTR_ERR(priv->feed_reg);

drivers/watchdog/ts72xx_wdt.c line 146

Quoted 10 lines

drivers/watchdog/ts72xx_wdt.c +5 -4

 
-	platform_set_drvdata(pdev, wdt);
-	ts72xx_wdt_pdev = pdev;
-	wdt->pdev = pdev;
-	mutex_init(&wdt->lock);
+	wdd = &priv->wdd;
+	wdd->info = &ts72xx_wdt_ident;
+	wdd->ops = &ts72xx_wdt_ops;
+	wdd->min_timeout = 1;
+	wdd->max_timeout = 8;

With such a low maximum timeout, it might make sense to use the core to be able to support larger timeouts.

drivers/watchdog/ts72xx_wdt.c line 416

Quoted 5 lines

drivers/watchdog/ts72xx_wdt.c +2 -2

+	wdd->timeout = 8;
+	wdd->parent = &pdev->dev;
 
-	/* make sure that the watchdog is disabled */
-	ts72xx_wdt_stop(wdt);

Are you sure this is no longer needed ? If there is a means to detect if the watchdog is running, it might make sense to set the WDOG_HW_RUNNING flag instead if it is running and let the core handle the ping until the watchdog device is opened.

drivers/watchdog/ts72xx_wdt.c line 154

Quoted 10 lines

drivers/watchdog/ts72xx_wdt.c +4 -5

+	watchdog_set_nowayout(wdd, nowayout);
 
-	error = misc_register(&ts72xx_wdt_miscdev);
-	if (error) {
-		dev_err(&pdev->dev, "failed to register miscdev\n");
-		return error;
-	}
+	watchdog_set_drvdata(wdd, priv);
+
+	ret = watchdog_register_device(wdd);

devm_watchdog_register_device() ?

Quoted 26 lines

drivers/watchdog/ts72xx_wdt.c +8 -1

+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, priv);
 
 	dev_info(&pdev->dev, "TS-72xx Watchdog driver\n");

@@ -428,7 +164,10 @@ static int ts72xx_wdt_probe(struct platform_device *pdev)
 
 static int ts72xx_wdt_remove(struct platform_device *pdev)
 {
-	misc_deregister(&ts72xx_wdt_miscdev);
+	struct ts72xx_wdt_priv *priv = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&priv->wdd);
+
 	return 0;
 }

Signature · 6 lines

2.10.0

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hartley Sweeten

2017-01-30 19:17 UTC #7 lore raw

RE: [PATCH 2/2] watchdog: ts72xx_wdt: convert driver to watchdog core

Quoted 10 lines · Guenter Roeck

Quoted 9 lines · H Hartley Sweeten
Cleanup this driver and convert it to use the watchdog framework API.
Signed-off-by H Hartley Sweeten <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Wim Van Sebroeck <[email protected]>
Cc Guenter Roeck <[email protected]>
---
 drivers/watchdog/ts72xx_wdt.c | 447 +++++++++---------------------------------
 1 file changed, 93 insertions(+), 354 deletions(-)

<snip>

Quoted 10 lines

drivers/watchdog/ts72xx_wdt.c line 35
Quoted 8 lines
drivers/watchdog/ts72xx_wdt.c +0 -8
-#define TS72XX_WDT_DEFAULT_TIMEOUT	8
-
-static int timeout = TS72XX_WDT_DEFAULT_TIMEOUT;
-module_param(timeout, int, 0);
-MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
-			  "(1 <= timeout <= 8, default="
-			  __MODULE_STRING(TS72XX_WDT_DEFAULT_TIMEOUT)
-			  ")");
Same question as with patch #1 - are you sure you want to take this away ?

Again, not a problem leaving it in.

Quoted 1 line

You might just drop the limits instead (also see below).

<snip>

Quoted 5 lines

drivers/watchdog/ts72xx_wdt.c line 146
Quoted 2 lines
drivers/watchdog/ts72xx_wdt.c +2 -0
+	wdd->min_timeout = 1;
+	wdd->max_timeout = 8;
With such a low maximum timeout, it might make sense to use the core to be able to support larger timeouts.

Agree. I'll update and test this for the next version.

Quoted 10 lines

drivers/watchdog/ts72xx_wdt.c line 416
Quoted 5 lines
drivers/watchdog/ts72xx_wdt.c +2 -2
+	wdd->timeout = 8;
+	wdd->parent = &pdev->dev;
 
-	/* make sure that the watchdog is disabled */
-	ts72xx_wdt_stop(wdt);
Are you sure this is no longer needed ? If there is a means to detect if the watchdog is running, it might make sense to set the WDOG_HW_RUNNING flag instead if it is running and let the core handle the ping until the watchdog device is opened.

A patch to make sure this watchdog is disabled early during the kernel uncompress will soon be applied. Arnd Bergmann has it in his todo folder:

[PATCH] ARM: ep93xx: Disable TS-72xx watchdog before uncompressing

The bootloader currently enables the watchdog for 8 seconds and it needs to be disabled just in case the uncompress takes longer.

I don't have a problem leaving it in here it's just not necessary.

Quoted 12 lines

drivers/watchdog/ts72xx_wdt.c line 154

Quoted 10 lines

drivers/watchdog/ts72xx_wdt.c +4 -5

+	watchdog_set_nowayout(wdd, nowayout);
 
-	error = misc_register(&ts72xx_wdt_miscdev);
-	if (error) {
-		dev_err(&pdev->dev, "failed to register miscdev\n");
-		return error;
-	}
+	watchdog_set_drvdata(wdd, priv);
+
+	ret = watchdog_register_device(wdd);

devm_watchdog_register_device() ?

I'll update this.

Thanks,
Hartley