Re: [PATCH] zr36067: Debugging cleanups

Hi Ronald,

On Mon, 7 May 2007 17:43:45 +0200, Jean Delvare wrote:
> On Mon, 7 May 2007 08:10:31 -0400, Ronald S. Bultje wrote:
> > On May 7, 2007, at 5:05 AM, Jean Delvare wrote:
> > > * Use a single global variable to handle the debug level. This makes
> > >   the driver a bit smaller (by about 800 bytes on x86_64), thanks to
> > >   one less level of indirection on every use.
> > [..]
> > > +int debug = 1;
> > 
> > This is not good, imagine another driver or kernel particle using a  
> > (global) variable called debug. Last time I did this, there were 3 of  
> > them. :-). Please call it something else.
> 
> I guess it really only matters when the conflicting drivers are all
> built-in (as opposed to modular) and I would hope nobody does this,
> but... You're totally right.
> 
> > The reason other drivers use debug is b/c their debug is static. The  
> > reason I did this zr_debug + debug hack is b/c it allows the debug  
> > name without having a debug variable.
> 
> I should have known there was a good reason for this strange construct.
> Thanks for pointing out the obvious.
> 
> So I would need to rename the variable to something else, but that
> breaks backwards compatibility and is also not really aesthetical so...
> I'd rather not do it and leave things as is, sorry for the noise.

I just remembered that we can use module_param_named() to solve that
particular problem. This gives us the best of both world: nice and
unchanged module parameter name, and non-conflicting global variable
name.

The following patch applies on top of the previous one (although I can
send a merged patch with all the cleanups if you prefer):

* * * * *

More debugging cleanups to the zoran (zr36067) driver:

* Use module_param_named() to declare the debug parameter, so we can
  use a single global variable to handle the debug level. This makes
  the driver a bit smaller (by 648 bytes on x86_64), thanks to one
  less level of indirection on every use.

Signed-off-by: Jean Delvare <[email protected]>
---
 drivers/media/video/zoran_card.c   |    9 ++++-----
 drivers/media/video/zoran_card.h   |    4 ++--
 drivers/media/video/zoran_device.c |    8 ++++----
 drivers/media/video/zoran_driver.c |    2 +-
 4 files changed, 11 insertions(+), 12 deletions(-)

--- linux-2.6.22-rc2.orig/drivers/media/video/zoran_card.c	2007-05-23 09:51:29.000000000 +0200
+++ linux-2.6.22-rc2/drivers/media/video/zoran_card.c	2007-05-23 10:10:32.000000000 +0200
@@ -145,9 +145,8 @@ module_param(pass_through, int, 0);
 MODULE_PARM_DESC(pass_through,
 		 "Pass TV signal through to TV-out when idling");
 
-static int debug = 1;
-int *zr_debug = &debug;
-module_param(debug, int, 0644);
+int zr_debug = 1;
+module_param_named(debug, zr_debug, int, 0644);
 MODULE_PARM_DESC(debug, "Debug level (0-5)");
 
 MODULE_DESCRIPTION("Zoran-36057/36067 JPEG codec driver");
@@ -1069,7 +1068,7 @@ test_interrupts (struct zoran *zr)
 	if (timeout) {
 		dprintk(1, ": time spent: %d\n", 1 * HZ - timeout);
 	}
-	if (*zr_debug > 1)
+	if (zr_debug > 1)
 		print_interrupts(zr);
 	btwrite(icr, ZR36057_ICR);
 }
@@ -1152,7 +1151,7 @@ zr36057_init (struct zoran *zr)
 		goto exit_unregister;
 
 	zoran_init_hardware(zr);
-	if (*zr_debug > 2)
+	if (zr_debug > 2)
 		detect_guest_activity(zr);
 	test_interrupts(zr);
 	if (!pass_through) {
--- linux-2.6.22-rc2.orig/drivers/media/video/zoran_card.h	2007-05-23 09:51:29.000000000 +0200
+++ linux-2.6.22-rc2/drivers/media/video/zoran_card.h	2007-05-23 10:09:37.000000000 +0200
@@ -30,11 +30,11 @@
 #ifndef __ZORAN_CARD_H__
 #define __ZORAN_CARD_H__
 
-extern int *zr_debug;
+extern int zr_debug;
 
 #define dprintk(num, format, args...) \
 	do { \
-		if (*zr_debug >= num) \
+		if (zr_debug >= num) \
 			printk(format, ##args); \
 	} while (0)
 
--- linux-2.6.22-rc2.orig/drivers/media/video/zoran_device.c	2007-05-23 09:51:29.000000000 +0200
+++ linux-2.6.22-rc2/drivers/media/video/zoran_device.c	2007-05-23 10:09:49.000000000 +0200
@@ -167,7 +167,7 @@ post_office_read (struct zoran *zr,
 static void
 dump_guests (struct zoran *zr)
 {
-	if (*zr_debug > 2) {
+	if (zr_debug > 2) {
 		int i, guest[8];
 
 		for (i = 1; i < 8; i++) {	// Don't read jpeg codec here
@@ -1288,7 +1288,7 @@ error_handler (struct zoran *zr,
 		zr->num_errors++;
 
 		/* Report error */
-		if (*zr_debug > 1 && zr->num_errors <= 8) {
+		if (zr_debug > 1 && zr->num_errors <= 8) {
 			long frame;
 			frame =
 			    zr->jpg_pend[zr->jpg_dma_tail & BUZ_MASK_FRAME];
@@ -1548,7 +1548,7 @@ zoran_irq (int             irq,
 
 			if (zr->codec_mode == BUZ_MODE_MOTION_DECOMPRESS ||
 			    zr->codec_mode == BUZ_MODE_MOTION_COMPRESS) {
-				if (*zr_debug > 1 &&
+				if (zr_debug > 1 &&
 				    (!zr->frame_num || zr->JPEG_error)) {
 					printk(KERN_INFO
 					       "%s: first frame ready: state=0x%08x odd_even=%d field_per_buff=%d delay=%d\n",
@@ -1585,7 +1585,7 @@ zoran_irq (int             irq,
 						    zr->JPEG_missed;
 				}
 
-				if (*zr_debug > 2 && zr->frame_num < 6) {
+				if (zr_debug > 2 && zr->frame_num < 6) {
 					int i;
 					printk("%s: seq=%ld stat_com:",
 					       ZR_DEVNAME(zr), zr->jpg_seq_num);
--- linux-2.6.22-rc2.orig/drivers/media/video/zoran_driver.c	2007-05-23 09:51:29.000000000 +0200
+++ linux-2.6.22-rc2/drivers/media/video/zoran_driver.c	2007-05-23 10:09:55.000000000 +0200
@@ -1423,7 +1423,7 @@ zoran_close (struct inode *inode,
 		/* disable interrupts */
 		btand(~ZR36057_ICR_IntPinEn, ZR36057_ICR);
 
-		if (*zr_debug > 1)
+		if (zr_debug > 1)
 			print_interrupts(zr);
 
 		/* Overlay off */

Thanks,
-- 
Jean Delvare

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/