Re: [PATCH] media: staging: zoran: refactor printk debugging function

[Ian Cowan <ian@linux.cowan.aero>]

On Thu, Apr 21, 2022 at 05:21:54PM +0300, Dan Carpenter wrote:
> On Wed, Apr 20, 2022 at 08:23:16PM -0400, Ian Cowan wrote:
> > This is a patch to refactor the zoran debugging function. This function
> > existed in all of the changed files and they also all import the
> > videocodec header file. This patch moves the dprintk function into the
> > videocodec header file and out of each of the individual files.
> > 
> > -#define dprintk(num, format, args...) \
> > -	do { \
> > -		if (videocodec_debug >= num) \
> > -			printk(format, ##args); \
> > -	} while (0)
> > -
[ snip ]
> 
> > +/* Print debug information based on the specified level of debugging */
> > +#define dprintk(debug, num, format, args...) \
> > +do { \
> > +	if (debug > num) \
> > +		printk(format, ##args); \
> > +} while (0)
> > +
> 
> I don't like the new format.  It needs to have a KERN_DEBUG at the
> start.  Passing the param_module is awkward.  No one knows what the
> magic "num" things are.

I was going to put KERN_DEBUG at the beginning, but it appears that some
of the calls already include KERN_ERR, KERN_WARNING, or KERN_INFO, but
most of them do not. Would it be better to split these into multiple
macros or use printk in those specific spots that difer?

> Using a module parameter is old school badness.  The standard dev_dbg()
> macros are better and more flexible.  Just use them instead.
> 
> regards,
> dan carpenter
> 

For using the dev_dbg() macro, do you define this in the header file
(i.e. for this it would be videocodec.h), or where should this be
included from?

---
Ian Cowan