On Mon, 2014-11-24 at 18:08 +0100, Takashi Iwai wrote:
At Sun, 23 Nov 2014 13:40:51 +0530, Sudip Mukherjee wrote:
[]
replaced all references of the debug messages via printk with dev_* macro (mostly dev_dbg). one reference was changed to pr_err as there the card might have been uninitialized.
this patch will generate warning from checkpatch about broken quoted strings. but that was not fixed intentionally to improve the readability.
I think it'd be easier to read and grep coalesced.
[]
in your review of v1, you said about some lines which are not ending with \n. but i was not able to find them. did i miss them somewhere?
[]
The problem is the one with multiple "\n", for example:
dev_dbg(korg1212->card->dev, "dspMemPhy = %08x U[%08x], " "PlayDataPhy = %08x L[%08x]\n" "korg1212: RecDataPhy = %08x L[%08x], " "VolumeTablePhy = %08x L[%08x]\n" "korg1212: RoutingTablePhy = %08x L[%08x], " "AdatTimeCodePhy = %08x L[%08x]\n",
I think these should be individual dev_dbg calls
dev_dbg(korg1212->card->dev, "dspMemPhy = %08x U[%08x]\n", val, val2) dev_dbg(korg1212->card->dev, "PhyDataPhy = %08x L[%08x]\n", val, val2); dev_dbg(korg1212->card->dev, "RecDataPhy = %08x L[%08x]\n", val, val2); dev_dbg(korg1212->card->dev, "VolumeTablePhy = %08x L[%08x]\n", val, val2);
etc..
Another possibility is to use another macro like:
#define k1212_dbg(k1212, fmt, ...) \ dev_dbg((k)->card->dev, fmt, ##__VA_ARGS__)
and change all these to
k1212_dbg(korg1212, "dspMemPhy = %08x U[%08x]\n", val, val2) k1212_dbg(korg1212, "PhyDataPhy = %08x L[%08x]\n", val, val2); k1212_dbg(korg1212, "RecDataPhy = %08x L[%08x]\n", val, val2); k1212_dbg(korg1212, "VolumeTablePhy = %08x L[%08x]\n", val, val2);
etc.
My biggest concern right now is, however, about the unnecessary code increase by this patch. Currently, most of debug prints were simply not built, because of:
// ---------------------------------------------------------------------------- -// Debug Stuff -// ---------------------------------------------------------------------------- -#define K1212_DEBUG_LEVEL 0 -#if K1212_DEBUG_LEVEL > 0 -#define K1212_DEBUG_PRINTK(fmt,args...) printk(KERN_DEBUG fmt,##args) -#else -#define K1212_DEBUG_PRINTK(fmt,...) -#endif -#if K1212_DEBUG_LEVEL > 1 -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,args...) printk(KERN_DEBUG fmt,##args) -#else -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,...) -#endif
With your patch, now all these codes are compiled.
Not really.
dev_dbg is a no-op unless DEBUG is #defined or CONFIG_DYNAMIC_DEBUG is set.
I have no clear answer what would be the best in such a case. I'd say it really depends. If they are just silly messages that can be covered in a better way (like ftrace), just get rid of them. If they are intended for some good register dumps, then dev_dbg() might make sense.
very true.