Ioctl WDIOC_GETBOOTSTATUS is supposed to return some information
on why the system did (re)boot recently, value WDIOF_CARDRESET
being used to indicate watchdog induced reboot.
Up to now, imx2_wdt did not provide a value here, always returning
zero to indicate normal boot.
Do evaluate the IMX Watchdog Reset Status Register and
produce WDIOF_CARDRESET with WDIOC_GETBOOTSTATUS in case
of a watchdog induced reset.
Signed-off-by: Oskar Schirmer <[email protected]>
---
drivers/watchdog/imx2_wdt.c | 12 +++++++++++-
1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index c44c333..940fd43 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -46,6 +46,9 @@
#define IMX2_WDT_SEQ1 0x5555 /* -> service sequence 1 */
#define IMX2_WDT_SEQ2 0xAAAA /* -> service sequence 2 */
+#define IMX2_WDT_WRSR 0x04 /* Reset Status Register */
+#define IMX2_WDT_WRSR_TOUT (1 << 1) /* -> Reset due to Timeout */
+
#define IMX2_WDT_MAX_TIME 128
#define IMX2_WDT_DEFAULT_TIME 60 /* in seconds */
@@ -175,6 +178,7 @@ static long imx2_wdt_ioctl(struct file *file, unsigned int cmd,
void __user *argp = (void __user *)arg;
int __user *p = argp;
int new_value;
+ u16 val;
switch (cmd) {
case WDIOC_GETSUPPORT:
@@ -182,9 +186,15 @@ static long imx2_wdt_ioctl(struct file *file, unsigned int cmd,
sizeof(struct watchdog_info)) ? -EFAULT : 0;
case WDIOC_GETSTATUS:
- case WDIOC_GETBOOTSTATUS:
return put_user(0, p);
+ case WDIOC_GETBOOTSTATUS:
+ val = __raw_readw(imx2_wdt.base + IMX2_WDT_WRSR);
+ new_value = 0;
+ if (val & IMX2_WDT_WRSR_TOUT)
+ new_value = WDIOF_CARDRESET;
+ return put_user(new_value, p);
+
case WDIOC_KEEPALIVE:
imx2_wdt_ping();
return 0;
--
1.7.5.4
Hi Oskar,
besides this minor thing (which may be just personal taste)...
> + case WDIOC_GETBOOTSTATUS:
> + val = __raw_readw(imx2_wdt.base + IMX2_WDT_WRSR);
> + new_value = 0;
> + if (val & IMX2_WDT_WRSR_TOUT)
> + new_value = WDIOF_CARDRESET;
I'd go for this to save some lines:
new_value = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;
But in general:
Acked-by: Wolfram Sang <[email protected]>
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Wolfram,
On Thu, Feb 16, 2012 at 14:35:10 +0100, Wolfram Sang wrote:
> besides this minor thing (which may be just personal taste)...
>
> > + case WDIOC_GETBOOTSTATUS:
> > + val = __raw_readw(imx2_wdt.base + IMX2_WDT_WRSR);
> > + new_value = 0;
> > + if (val & IMX2_WDT_WRSR_TOUT)
> > + new_value = WDIOF_CARDRESET;
>
> I'd go for this to save some lines:
> new_value = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;
A good alternative for sure.
Seen from the perspective of code compactness ("save some lines"),
one could try to be consequent here, and save the "val" variable
altogether, ending up with some "new_value = __raw_readw ... & ... ? ... : 0;"
But eventually there might be code readability issues, so it's
a good idea to keep the balance.
An argument for the original, longer version might be it is easier
to extend, when more flags need to be handled, like
"else if (val & ...) new_value = ...", while nested conditional
expressions would most likely become quite complex soon.
Actually, my personal taste doesn't show a preference for
one version or the other, so I'm ok with Your proposal, too.
Oskar
> Seen from the perspective of code compactness ("save some lines"),
> one could try to be consequent here, and save the "val" variable
> altogether, ending up with some "new_value = __raw_readw ... & ... ? ... : 0;"
> But eventually there might be code readability issues, so it's
> a good idea to keep the balance.
Yes.
> An argument for the original, longer version might be it is easier
> to extend, when more flags need to be handled, like
> "else if (val & ...) new_value = ...", while nested conditional
> expressions would most likely become quite complex soon.
Once we have support to handle non-stoppable watchdogs in the the new
watchdog framework, the code will be greatly refactored anyhow :)
Thanks,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |