[alsa-devel] wrong print debugging message code
Hello,
I written ASoC Audio driver for our company chip in linux kernel 2.6.24.1. New kernel 2.6.25.7 released before several days. So, I merge my ASoC Audio dirver to kernel 2.6.25.7. then I found wrong code that print debuuging message code in several files. These file location is kernel_src/sound/soc/codecs for SoC audio codec drvier files
These files include follow define code for print debug message.
------------------------------------------------------------------------------------------------------------------------ #define XXXX_DEBUG 0
#ifdef XXXX_DEBUG #define dbg(format, arg...) \ printk(KERN_DEBUG AUDIO_NAME ": " format "\n" , ## arg) #else #define dbg(format, arg...) do {} while (0) #endif ------------------------------------------------------------------------------------------------------------------------
I think "#ifdef XXXX_DEBUG" line is wrong. Because, XXXX_DEBUG was already defined 0. I known that "#ifdef" is only check defined or not defined, 0 or 1(false or true) are not check. "#if XXXX_DEBUG" code is more correct than "#ifdef XXXX_DEBUG".
thanks
At Sat, 21 Jun 2008 14:44:47 +0900, Jin-Young Park wrote:
Hello,
I written ASoC Audio driver for our company chip in linux kernel 2.6.24.1. New kernel 2.6.25.7 released before several days. So, I merge my ASoC Audio dirver to kernel 2.6.25.7. then I found wrong code that print debuuging message code in several files. These file location is kernel_src/sound/soc/codecs for SoC audio codec drvier files
These files include follow define code for print debug message.
#define XXXX_DEBUG 0
#ifdef XXXX_DEBUG #define dbg(format, arg...) \ printk(KERN_DEBUG AUDIO_NAME ": " format "\n" , ## arg) #else #define dbg(format, arg...) do {} while (0)
#endif
I think "#ifdef XXXX_DEBUG" line is wrong. Because, XXXX_DEBUG was already defined 0. I known that "#ifdef" is only check defined or not defined, 0 or 1(false or true) are not check. "#if XXXX_DEBUG" code is more correct than "#ifdef XXXX_DEBUG".
#ifdef XXX_DEBUG is more common than #if XXX_DEBUG in the kernel tree. So, I'd suggest to keep #ifdef style.
The confusing thing is that it defines to '0' (and I'm not sure the debug is activated as default in the current code -- Mark, Liam, is it intentional?).
IMO, the debug should be basically off as default, and if enabled, it should be defined to '1'.
Takashi
On Mon, Jun 23, 2008 at 12:55:57PM +0200, Takashi Iwai wrote:
The confusing thing is that it defines to '0' (and I'm not sure the debug is activated as default in the current code -- Mark, Liam, is it intentional?).
I doubt it.
IMO, the debug should be basically off as default, and if enabled, it should be defined to '1'.
Really they should all just be deleted and replaced by the standard pr_() macros but if we wait for v2 then we'd be able to use the dev_() variants instead.
At Mon, 23 Jun 2008 12:12:28 +0100, Mark Brown wrote:
On Mon, Jun 23, 2008 at 12:55:57PM +0200, Takashi Iwai wrote:
The confusing thing is that it defines to '0' (and I'm not sure the debug is activated as default in the current code -- Mark, Liam, is it intentional?).
I doubt it.
IMO, the debug should be basically off as default, and if enabled, it should be defined to '1'.
Really they should all just be deleted and replaced by the standard pr_() macros but if we wait for v2 then we'd be able to use the dev_() variants instead.
It depends on your plan. Definitely nice to fix it for 2.6.27, though.
Takashi
On Mon, Jun 23, 2008 at 02:13:19PM +0200, Takashi Iwai wrote:
It depends on your plan. Definitely nice to fix it for 2.6.27, though.
I've actually just written a patch ripping them all out (I'd probably have submitted it already if I hadn't been reviewing Ben's changes) - turns out that none of the drivers were actually using their dbg() macros anyway so it won't have any runtime effect, though obviously a fix would still be good.
Hello.
I tested the code that "#define XXXX_DEBUG 0" on our company EVB with WM8960. I defined "#define XXXX_DEBUG 0" and dbg() put into mute function(wm8960_digital_mute()). After linux boot, played audio file. My test dbg() message was not print terminal, but I find dbg() code was working exactly. Excuted dmesg command, then it was print that logged dbg() messages. I think, it maybe potential that reduce performance before one knows.
Thanks. Jinyoung Park.
On Tue, Jun 24, 2008 at 03:18:49AM +0900, Jin-Young Park wrote:
I defined "#define XXXX_DEBUG 0" and dbg() put into mute function(wm8960_digital_mute()).
The WM8960 driver isn't in the standard Linux kernel yet, it is currently distributed via the Wolfson web site. None of the drivers included in the standard Linux kernel were using their dbg() macros, though some had such macros present.
I think, it maybe potential that reduce performance before one knows.
The performance impact should be very minor - the digital mute is not used in any hot paths and the output will only go do the dmesg buffer by default so won't have the overhead of writing to the console.
Out of interest, which platform are you working with?
I get the point your explanation. Thanks.
Out of interest, which platform are you working with?
I working at Mtekvision in Korea. Mtekvision is fabless company, development MMP and ISP. you know? Anyway, my development platform is MV86XX series(MMP) with Wolfson Codecs(WM8960, 8973, 8983, 9713 ...) and Linux kernel 2.6.25.7 <tr_1214286468096>.
participants (3)
-
Jin-Young Park -
Mark Brown -
Takashi Iwai