[PATCH] watchdog: booke_wdt: clean up status messages

[PATCH] watchdog: booke_wdt: clean up status messages

[Timur Tabi]

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

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);
-- 
1.7.3.4

[PATCH] watchdog: booke_wdt: clean up status messages

[Timur Tabi]

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

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);
-- 
1.7.3.4

Re: [PATCH] watchdog: booke_wdt: clean up status messages

[Benjamin Herrenschmidt]

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.

Re: [PATCH] watchdog: booke_wdt: clean up status messages

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale