Patchwork watchdog: booke_wdt: clean up status messages

login
register
mail settings
Submitter Timur Tabi
Date Jan. 17, 2011, 9:29 p.m.
Message ID <1295299780-27338-1-git-send-email-timur@freescale.com>
Download mbox | patch
Permalink /patch/79220/
State Changes Requested
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Timur Tabi - Jan. 17, 2011, 9:29 p.m.
Improve the status messages that are displayed during some operations of the
PowerPC watchdog timer driver.  When the watchdog is enabled, the timeout is
displayed as a number of seconds, instead of an obscure "period".  The "period"
is the position of a bit in a 64-bit timer register.  The higher the value,
the quicker the watchdog timeout occurs.  Some people chose a high "period"
value for the timer and get confused as to why the board resets within a
few seconds.

Messages displayed during open and close are now debug messages, so that they
don't clutter the console by default.  Finally, printk() is replaced with the
pr_xxx() equivalent.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 drivers/watchdog/booke_wdt.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)
Benjamin Herrenschmidt - Feb. 7, 2011, 1:06 a.m.
On Mon, 2011-01-17 at 15:29 -0600, Timur Tabi wrote:
> Improve the status messages that are displayed during some operations of the
> PowerPC watchdog timer driver.  When the watchdog is enabled, the timeout is
> displayed as a number of seconds, instead of an obscure "period".  The "period"
> is the position of a bit in a 64-bit timer register.  The higher the value,
> the quicker the watchdog timeout occurs.  Some people chose a high "period"
> value for the timer and get confused as to why the board resets within a
> few seconds.
> 
> Messages displayed during open and close are now debug messages, so that they
> don't clutter the console by default.  Finally, printk() is replaced with the
> pr_xxx() equivalent.

Minor nit bu
>  
> -	printk(KERN_INFO "PowerPC Book-E Watchdog Timer Loaded\n");
> +	pr_info("PowerPC Book-E Watchdog Timer Loaded\n");
>  	ident.firmware_version = cur_cpu_spec->pvr_value;
>  
>  	ret = misc_register(&booke_wdt_miscdev);
>  	if (ret) {
> -		printk(KERN_CRIT "Cannot register miscdev on minor=%d: %d\n",
> -				WATCHDOG_MINOR, ret);
> +		pr_err("booke_wdt: cannot register device (minor=%u, ret=%i)\n",
> +		       WATCHDOG_MINOR, ret);
>  		return ret;
>  	}
>  
>  	spin_lock(&booke_wdt_lock);
>  	if (booke_wdt_enabled == 1) {
> -		printk(KERN_INFO
> -		      "PowerPC Book-E Watchdog Timer Enabled (wdt_period=%d)\n",
> -				booke_wdt_period);
> +		pr_info("booke_wdt: watchdog enabled (timeout = %llu sec)\n",
> +			period_to_sec(booke_wdt_period));
>  		on_each_cpu(__booke_wdt_enable, NULL, 0);
>  	}

If you're going to cleanup the messages, then I think displaying:

PowerPC Book-E Watchdog Timer Loaded
booke_wdt: watchdog enabled (timeout = xxx sec)

Isn't very nice.

Lacks consistency ... you can prefix both lines the same way, or
maybe better print only one line with all the necessary info.

Ben.
Timur Tabi - Feb. 8, 2011, 4:33 p.m.
Benjamin Herrenschmidt wrote:
> If you're going to cleanup the messages, then I think displaying:
> 
> PowerPC Book-E Watchdog Timer Loaded
> booke_wdt: watchdog enabled (timeout = xxx sec)
> 
> Isn't very nice.
> 
> Lacks consistency ... you can prefix both lines the same way, or
> maybe better print only one line with all the necessary info.

Yeah, I see your point.  I'll post a v2.

Patch

diff --git a/drivers/watchdog/booke_wdt.c b/drivers/watchdog/booke_wdt.c
index 7e7ec9c..935d0dd 100644
--- a/drivers/watchdog/booke_wdt.c
+++ b/drivers/watchdog/booke_wdt.c
@@ -221,9 +221,8 @@  static int booke_wdt_open(struct inode *inode, struct file *file)
 	if (booke_wdt_enabled == 0) {
 		booke_wdt_enabled = 1;
 		on_each_cpu(__booke_wdt_enable, NULL, 0);
-		printk(KERN_INFO
-		      "PowerPC Book-E Watchdog Timer Enabled (wdt_period=%d)\n",
-				booke_wdt_period);
+		pr_debug("booke_wdt: watchdog enabled (timeout = %llu sec)\n",
+			period_to_sec(booke_wdt_period));
 	}
 	spin_unlock(&booke_wdt_lock);
 
@@ -240,6 +239,7 @@  static int booke_wdt_release(struct inode *inode, struct file *file)
 	 */
 	on_each_cpu(__booke_wdt_disable, NULL, 0);
 	booke_wdt_enabled = 0;
+	pr_debug("booke_wdt: watchdog disabled\n",
 #endif
 
 	clear_bit(0, &wdt_is_active);
@@ -271,21 +271,20 @@  static int __init booke_wdt_init(void)
 {
 	int ret = 0;
 
-	printk(KERN_INFO "PowerPC Book-E Watchdog Timer Loaded\n");
+	pr_info("PowerPC Book-E Watchdog Timer Loaded\n");
 	ident.firmware_version = cur_cpu_spec->pvr_value;
 
 	ret = misc_register(&booke_wdt_miscdev);
 	if (ret) {
-		printk(KERN_CRIT "Cannot register miscdev on minor=%d: %d\n",
-				WATCHDOG_MINOR, ret);
+		pr_err("booke_wdt: cannot register device (minor=%u, ret=%i)\n",
+		       WATCHDOG_MINOR, ret);
 		return ret;
 	}
 
 	spin_lock(&booke_wdt_lock);
 	if (booke_wdt_enabled == 1) {
-		printk(KERN_INFO
-		      "PowerPC Book-E Watchdog Timer Enabled (wdt_period=%d)\n",
-				booke_wdt_period);
+		pr_info("booke_wdt: watchdog enabled (timeout = %llu sec)\n",
+			period_to_sec(booke_wdt_period));
 		on_each_cpu(__booke_wdt_enable, NULL, 0);
 	}
 	spin_unlock(&booke_wdt_lock);