Re: [PATCH] zr36067: Debugging cleanups

Hi Ronald,

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.

Here are the remaining parts of my patch, as I think they still are
valuable:

* * * * *

Debugging cleanups to the zoran (zr36067) driver:

* Change the debug parameter sysfs permissions, so that the debug
  level can be adjusted at runtime, as is done in many other
  media/video drivers.

* The debug level is between 0 and 5, not 0 and 4.

* Move the zr_debug export and dprintk macro definition to a header
  file so that we don't have to define them in each source file.

* Simplify a duplicate test on zr_debug.

Signed-off-by: Jean Delvare <[email protected]>
---
 drivers/media/video/zoran_card.c   |   10 ++--------
 drivers/media/video/zoran_card.h   |    8 ++++++++
 drivers/media/video/zoran_device.c |    9 +--------
 drivers/media/video/zoran_driver.c |   18 ++++--------------
 drivers/media/video/zoran_procfs.c |    9 +--------
 5 files changed, 16 insertions(+), 38 deletions(-)

--- linux-2.6.21-git.orig/drivers/media/video/zoran_card.c	2007-05-07 17:14:25.000000000 +0200
+++ linux-2.6.21-git/drivers/media/video/zoran_card.c	2007-05-07 17:24:26.000000000 +0200
@@ -147,8 +147,8 @@ MODULE_PARM_DESC(pass_through,
 
 static int debug = 1;
 int *zr_debug = &debug;
-module_param(debug, int, 0);
-MODULE_PARM_DESC(debug, "Debug level (0-4)");
+module_param(debug, int, 0644);
+MODULE_PARM_DESC(debug, "Debug level (0-5)");
 
 MODULE_DESCRIPTION("Zoran-36057/36067 JPEG codec driver");
 MODULE_AUTHOR("Serguei Miridonov");
@@ -161,12 +161,6 @@ static struct pci_device_id zr36067_pci_
 };
 MODULE_DEVICE_TABLE(pci, zr36067_pci_tbl);
 
-#define dprintk(num, format, args...) \
-	do { \
-		if (*zr_debug >= num) \
-			printk(format, ##args); \
-	} while (0)
-
 int zoran_num;			/* number of Buzs in use */
 struct zoran zoran[BUZ_MAX];
 
--- linux-2.6.21-git.orig/drivers/media/video/zoran_card.h	2007-05-07 17:14:25.000000000 +0200
+++ linux-2.6.21-git/drivers/media/video/zoran_card.h	2007-05-07 17:36:18.000000000 +0200
@@ -30,6 +30,14 @@
 #ifndef __ZORAN_CARD_H__
 #define __ZORAN_CARD_H__
 
+extern int *zr_debug;
+
+#define dprintk(num, format, args...) \
+	do { \
+		if (*zr_debug >= num) \
+			printk(format, ##args); \
+	} while (0)
+
 /* Anybody who uses more than four? */
 #define BUZ_MAX 4
 extern int zoran_num;
--- linux-2.6.21-git.orig/drivers/media/video/zoran_device.c	2007-05-07 17:14:25.000000000 +0200
+++ linux-2.6.21-git/drivers/media/video/zoran_device.c	2007-05-07 17:24:26.000000000 +0200
@@ -52,6 +52,7 @@
 #include "videocodec.h"
 #include "zoran.h"
 #include "zoran_device.h"
+#include "zoran_card.h"
 
 #define IRQ_MASK ( ZR36057_ISR_GIRQ0 | \
 		   ZR36057_ISR_GIRQ1 | \
@@ -59,14 +60,6 @@
 
 extern const struct zoran_format zoran_formats[];
 
-extern int *zr_debug;
-
-#define dprintk(num, format, args...) \
-	do { \
-		if (*zr_debug >= num) \
-			printk(format, ##args); \
-	} while (0)
-
 static int lml33dpath = 0;	/* 1 will use digital path in capture
 				 * mode instead of analog. It can be
 				 * used for picture adjustments using
--- linux-2.6.21-git.orig/drivers/media/video/zoran_procfs.c	2007-05-07 17:14:25.000000000 +0200
+++ linux-2.6.21-git/drivers/media/video/zoran_procfs.c	2007-05-07 17:24:26.000000000 +0200
@@ -48,14 +48,7 @@
 #include "videocodec.h"
 #include "zoran.h"
 #include "zoran_procfs.h"
-
-extern int *zr_debug;
-
-#define dprintk(num, format, args...) \
-	do { \
-		if (*zr_debug >= num) \
-			printk(format, ##args); \
-	} while (0)
+#include "zoran_card.h"
 
 #ifdef CONFIG_PROC_FS
 struct procfs_params_zr36067 {
--- linux-2.6.21-git.orig/drivers/media/video/zoran_driver.c	2007-05-07 17:14:25.000000000 +0200
+++ linux-2.6.21-git/drivers/media/video/zoran_driver.c	2007-05-07 17:24:26.000000000 +0200
@@ -192,14 +192,6 @@ static const int zoran_num_formats =
 #   include <linux/bigphysarea.h>
 #endif
 
-extern int *zr_debug;
-
-#define dprintk(num, format, args...) \
-	do { \
-		if (*zr_debug >= num) \
-			printk(format, ##args);	\
-	} while (0)
-
 extern int v4l_nbufs;
 extern int v4l_bufsize;
 extern int jpg_nbufs;
@@ -1154,12 +1146,10 @@ jpg_sync (struct file       *file,
 		frame = zr->jpg_pend[zr->jpg_que_tail & BUZ_MASK_FRAME];
 
 	/* buffer should now be in BUZ_STATE_DONE */
-	if (*zr_debug > 0)
-		if (zr->jpg_buffers.buffer[frame].state != BUZ_STATE_DONE)
-			dprintk(2,
-				KERN_ERR
-				"%s: jpg_sync() - internal state error\n",
-				ZR_DEVNAME(zr));
+	if (zr->jpg_buffers.buffer[frame].state != BUZ_STATE_DONE)
+		dprintk(2,
+			KERN_ERR "%s: jpg_sync() - internal state error\n",
+			ZR_DEVNAME(zr));
 
 	*bs = zr->jpg_buffers.buffer[frame].bs;
 	bs->frame = frame;

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/