Re: [PATCH] zr36067: Debugging cleanups

Hi Trent,

On Wed, 23 May 2007 02:34:25 -0700 (PDT), Trent Piepho wrote:
> On Wed, 23 May 2007, Jean Delvare wrote:
> > 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.
> 
> Maybe it was my message from two weeks ago where I suggested doing exactly
> that which jogged your memory?
> 
> http://thread.gmane.org/gmane.comp.video.mjpeg.devel/2382/focus=2383

Oops, I'm sorry about this. I didn't read your post. I am subscribed to
the mjpeg-developer list in order to possibly post to it, but I am
otherwise not reading it (I disabled message delivery) - I am busy
enough as is. I expected people replying to me to Cc me.

> zr_debug is probably a poor choice of names for a global variable, since
> there are also currently zr36060, zr36120, and zr364xx drivers which could
> easily use the same name.

The variable was already named zr_debug, my patch simply kept the
original name. But I can rename it to zr36067_debug if Ronald and you
agree it's better, no problem.

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

OK, I'll post a merged patch in a minute.

> I was going to ask if you indended to finish this
> patch, since I've been fixing bugs in this driver to make it work with
> mplayer and tvtime, and fixing the debug printing will conflict.

As far as I am concerned the patch is finished - I was waiting for
Ronald to ack my previous patch but I guess he was waiting for an
update after the exchange you had that I missed :/

-- 
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/