watchdog/coh901327: convert to use watchdog core

PATCH Superseded Linus Walleij 5 participants 8 messages Mar 7, 2012 – Mar 22, 2012 Watchdog device drivers

lore

Share & tools

lore

Linus Walleij

author

2012-03-07 14:52 UTC #0 lore raw

PATCH 1/1 watchdog/coh901327: convert to use watchdog core

From: Linus Walleij <[email protected]>

This converts the COH901327 watchdog to use the watchdog core. I followed Wolframs document, looked at some other drivers and tested it on the U300.

Cc Wolfram Sang <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

---
 drivers/watchdog/Kconfig         |    1 +
 drivers/watchdog/coh901327_wdt.c |  148 ++++++++++----------------------------
 2 files changed, 40 insertions(+), 109 deletions(-)

drivers/watchdog/Kconfig +1 -0

index 877b107..74c5f66 100644
@@ -283,6 +283,7 @@ config COH901327_WATCHDOG
 	bool "ST-Ericsson COH 901 327 watchdog"
 	depends on ARCH_U300
 	default y if MACH_U300
+	select WATCHDOG_CORE
 	help
 	  Say Y here to include Watchdog timer support for the
 	  watchdog embedded into the ST-Ericsson U300 series platforms.

drivers/watchdog/coh901327_wdt.c +39 ~~-109~~

229 lines not on this page · raw message

Signature · 1 line

1.7.8

Wolfram Sang

2012-03-07 16:42 UTC #1 lore raw

PATCH 1/1 watchdog/coh901327: convert to use watchdog core

Hi Linus,

drivers/watchdog/coh901327_wdt.c static irqreturn_t coh901327_interrupt(int irq, void *data) · line 227

Quoted 12 lines

drivers/watchdog/coh901327_wdt.c +1 -3

-static long coh901327_ioctl(struct file *file, unsigned int cmd,
+static long coh901327_ioctl(struct watchdog_device *wdt_dev, unsigned int cmd,
 			    unsigned long arg)
 {
 	int ret = -ENOTTY;
 	u16 val;
-	int time;
-	int new_options;
 	union {
 		struct watchdog_info __user *ident;
 		int __user *i;
 	} uarg;

'uarg' is nice name for the union ;) You probably don't need that anymore. If we keep the ioctl, that is, because...

...

drivers/watchdog/coh901327_wdt.c static irqreturn_t coh901327_interrupt(int irq, void *data) · line 233

Quoted 3 lines

drivers/watchdog/coh901327_wdt.c +0 -0

 	case WDIOC_GETTIMELEFT:
 		clk_enable(clk);
 		/* Read repeatedly until the value is stable! */

I was wondering if it pays to put this IOCTL to watchdog_dev and add another callback to watchdog_ops? I'd think so. Wim, Linus, what do you think? We don't save much (yet?), but since this is part of the kernel-API it might be nice to have the common part centralized, even if it is mainly put_user (which might be nice, too, since most users of GETTIMELEFT cast the result to various types).

drivers/watchdog/coh901327_wdt.c static long coh901327_ioctl(struct file *file, unsigned int cmd, · line 268

Quoted 8 lines

drivers/watchdog/coh901327_wdt.c +8 -0

+static struct watchdog_device coh901327_wdt = {
+	.info = &coh901327_ident,
+	.ops = &coh901327_ops,
+	/*
+	 * Max margin is 327 since the 10ms
+	 * timeout register is max
+	 * 0x7FFF = 327670ms ~= 327s.
+	 */

I'd drop that comment, but well...

drivers/watchdog/coh901327_wdt.c static long coh901327_ioctl(struct file *file, unsigned int cmd, · line 270

Quoted 2 lines

drivers/watchdog/coh901327_wdt.c +2 -0
+	.min_timeout = 0,
+	.max_timeout = 327,

Thanks!

Wolfram

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120307/60dc17ea/attachment.sig>

Linus Walleij

2012-03-08 06:16 UTC #2 lore raw

PATCH 1/1 watchdog/coh901327: convert to use watchdog core

Quoted 6 lines · Wolfram Sang

Quoted 3 lines
? ? ? case WDIOC_GETTIMELEFT:
? ? ? ? ? ? ? clk_enable(clk);
? ? ? ? ? ? ? /* Read repeatedly until the value is stable! */
I was wondering if it pays to put this IOCTL to watchdog_dev and add another callback to watchdog_ops? I'd think so. Wim, Linus, what do you think?

OK with me, shall I hack a patch?

Yours,
Linus Walleij

Wim Van Sebroeck

2012-03-15 21:27 UTC #3 lore raw

Hi Linus,

Quoted 10 lines · Wolfram Sang

Quoted 7 lines
Quoted 4 lines
case WDIOC_GETTIMELEFT:
clk_enable(clk);
/* Read repeatedly until the value is stable! */
I was wondering if it pays to put this IOCTL to watchdog_dev and add another callback to watchdog_ops? I'd think so. Wim, Linus, what do you think?
OK with me, shall I hack a patch?

I think Viresh did something allready.

Kind regards,
Wim.

--
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

Viresh Kumar

2012-03-16 03:47 UTC #4 lore raw

PATCH 1/1 watchdog/coh901327: convert to use watchdog core

Quoted 8 lines · Wim Van Sebroeck

Hi Linus,
Quoted 6 lines · Wolfram Sang
Quoted 5 lines
Quoted 4 lines
      case WDIOC_GETTIMELEFT:
              clk_enable(clk);
              /* Read repeatedly until the value is stable! */

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
I was wondering if it pays to put this IOCTL to watchdog_dev and add another callback to watchdog_ops? I'd think so. Wim, Linus, what do you think?
OK with me, shall I hack a patch?
I think Viresh did something allready.

You can find it here:

http://www.spinics.net/lists/arm-kernel/msg164285.html

Signature · 1 line

viresh

Linus Walleij

2012-03-16 08:15 UTC #5 lore raw

PATCH 1/1 watchdog/coh901327: convert to use watchdog core

In reply to Wim Van Sebroeck

Quoted 8 lines · Wim Van Sebroeck

Hi Linus,
Quoted 6 lines · Wolfram Sang
Quoted 5 lines
Quoted 4 lines
? ? ? case WDIOC_GETTIMELEFT:
? ? ? ? ? ? ? clk_enable(clk);
? ? ? ? ? ? ? /* Read repeatedly until the value is stable! */

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
I was wondering if it pays to put this IOCTL to watchdog_dev and add another callback to watchdog_ops? I'd think so. Wim, Linus, what do you think?
OK with me, shall I hack a patch?
I think Viresh did something allready.

OK I rebased on top of Viresh's patch and sent them both out since I don't know the status of his patch.

If you already have Viresh's patch in your tree just applying 2/2 should work as fine.

Yours,
Linus Walleij

Wim Van Sebroeck

2012-03-22 14:49 UTC #6 lore raw

Hi Linus,

Quoted 2 lines

OK I rebased on top of Viresh's patch and sent them both out since I don't know the status of his patch.

I added it. Only issue left is that you don't set the timeout value properly. This will result in faulty values for the WDIOC_GETTIMEOUT iotcl. Furthermore margin should be unsigned int. And there is also no check to see if margin is a valid parameter after it is loaded as a module.

Could you test attached patch?

Kind regards,
Wim.
---------------------------------------------------------------------

drivers/watchdog/coh901327_wdt.c +10 -6

index 7f0cbeb..3175767 100644
@@ -67,7 +67,7 @@
 #define U300_WDOG_IFR_WILL_BARK_IRQ_FORCE_ENABLE			0x0001U
 
 /* Default timeout in seconds = 1 minute */
-static int margin = 60;
+static unsigned int margin = 60;
 static resource_size_t phybase;
 static resource_size_t physize;
 static int irq;

@@ -153,7 +153,7 @@ static void coh901327_disable(void)
 
 static int coh901327_start(struct watchdog_device *wdt_dev)
 {
-	coh901327_enable(margin * 100);
+	coh901327_enable(wdt_dev->timeout * 100);
 	return 0;
 }

@@ -176,10 +176,10 @@ static int coh901327_ping(struct watchdog_device *wdd)
 static int coh901327_settimeout(struct watchdog_device *wdt_dev,
 				unsigned int time)
 {
-	margin = time;
+	wdt_dev->timeout = time;
 	clk_enable(clk);
 	/* Set new timeout value */
-	writew(margin * 100, virtbase + U300_WDOG_TR);
+	writew(time * 100, virtbase + U300_WDOG_TR);
 	/* Feed the dog */
 	writew(U300_WDOG_FR_FEED_RESTART_TIMER,
 	       virtbase + U300_WDOG_FR);

@@ -250,7 +250,7 @@ static struct watchdog_device coh901327_wdt = {
 	.info = &coh901327_ident,
 	.ops = &coh901327_ops,
 	/*
-	 * Max margin is 327 since the 10ms
+	 * Max timout is 327 since the 10ms
 	 * timeout register is max
 	 * 0x7FFF = 327670ms ~= 327s.
 	 */

@@ -353,6 +353,10 @@ static int __init coh901327_probe(struct platform_device *pdev)
 
 	clk_disable(clk);
 
+	if (margin < 1 || margin > 327)
+		margin = 60;
+	coh901327_wdt.timout = margin;
+
 	ret = watchdog_register_device(&coh901327_wdt);
 	if (ret == 0)
 		dev_info(&pdev->dev,

@@ -461,7 +465,7 @@ module_exit(coh901327_exit);
 MODULE_AUTHOR("Linus Walleij <[email protected]>");
 MODULE_DESCRIPTION("COH 901 327 Watchdog");
 
-module_param(margin, int, 0);
+module_param(margin, uint, 0);
 MODULE_PARM_DESC(margin, "Watchdog margin in seconds (default 60s)");
 
 MODULE_LICENSE("GPL");

Linus Walleij

2012-03-22 18:09 UTC #7 lore raw

PATCH 1/1 watchdog/coh901327: convert to use watchdog core

Quoted 6 lines · Wim Van Sebroeck

I added it. Only issue left is that you don't set the timeout value properly. This will result in faulty values for the WDIOC_GETTIMEOUT iotcl. Furthermore margin should be unsigned int. And there is also no check to see if margin is a valid parameter after it is loaded as a module.
Could you test attached patch?

I cannot test it right now because I'm out of reach for the hardware, but it looks good to me so:

Acked-by Linus Walleij <[email protected]>

If I find any problems later I can surely fix them.

Yours,
Linus Walleij