[PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro

[PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro

[Jiri Slaby]

Despite the AZF_FMT_XLATE macro looks totally weird and useless, it's
heavily broken (apart it also broke my parser).

The 'break' in the macro belongs to the do-while loop and not to the
switch. So it always falls through the all cases till the end.

Remove the do-while from the macro.

Also people should terminate statements with semicolons. So force
people to do so by removing the last one in the macro.

I vote for removing that crap completely because it makes the code
weird anyway -- you have to specify manually both of freq and bits
which is I would expect to be avoided exactly by such a macro.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Andreas Mohr <andi@lisas.de>
Cc: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/azt3328.c |   32 +++++++++++++++-----------------
 1 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/sound/pci/azt3328.c b/sound/pci/azt3328.c
index 6117595..134f55f 100644
--- a/sound/pci/azt3328.c
+++ b/sound/pci/azt3328.c
@@ -980,29 +980,27 @@ snd_azf3328_codec_setfmt(struct snd_azf3328_codec_data *codec,
 	snd_azf3328_dbgcallenter();
 	switch (bitrate) {
 #define AZF_FMT_XLATE(in_freq, out_bits) \
-	do { \
 		case AZF_FREQ_ ## in_freq: \
 			freq = SOUNDFORMAT_FREQ_ ## out_bits; \
-			break; \
-	} while (0);
-	AZF_FMT_XLATE(4000, SUSPECTED_4000)
-	AZF_FMT_XLATE(4800, SUSPECTED_4800)
+			break
+	AZF_FMT_XLATE(4000, SUSPECTED_4000);
+	AZF_FMT_XLATE(4800, SUSPECTED_4800);
 	/* the AZF3328 names it "5510" for some strange reason: */
-	AZF_FMT_XLATE(5512, 5510)
-	AZF_FMT_XLATE(6620, 6620)
-	AZF_FMT_XLATE(8000, 8000)
-	AZF_FMT_XLATE(9600, 9600)
-	AZF_FMT_XLATE(11025, 11025)
-	AZF_FMT_XLATE(13240, SUSPECTED_13240)
-	AZF_FMT_XLATE(16000, 16000)
-	AZF_FMT_XLATE(22050, 22050)
-	AZF_FMT_XLATE(32000, 32000)
+	AZF_FMT_XLATE(5512, 5510);
+	AZF_FMT_XLATE(6620, 6620);
+	AZF_FMT_XLATE(8000, 8000);
+	AZF_FMT_XLATE(9600, 9600);
+	AZF_FMT_XLATE(11025, 11025);
+	AZF_FMT_XLATE(13240, SUSPECTED_13240);
+	AZF_FMT_XLATE(16000, 16000);
+	AZF_FMT_XLATE(22050, 22050);
+	AZF_FMT_XLATE(32000, 32000);
 	default:
 		snd_printk(KERN_WARNING "unknown bitrate %d, assuming 44.1kHz!\n", bitrate);
 		/* fall-through */
-	AZF_FMT_XLATE(44100, 44100)
-	AZF_FMT_XLATE(48000, 48000)
-	AZF_FMT_XLATE(66200, SUSPECTED_66200)
+	AZF_FMT_XLATE(44100, 44100);
+	AZF_FMT_XLATE(48000, 48000);
+	AZF_FMT_XLATE(66200, SUSPECTED_66200);
 #undef AZF_FMT_XLATE
 	}
 	/* val = 0xff07; 3m27.993s (65301Hz; -> 64000Hz???) hmm, 66120, 65967, 66123 */
-- 
1.7.3.2

Re: [PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro

[Takashi Iwai]

At Sat, 22 Jan 2011 14:56:10 +0100,
Jiri Slaby wrote:
> 
> Despite the AZF_FMT_XLATE macro looks totally weird and useless, it's
> heavily broken (apart it also broke my parser).
> 
> The 'break' in the macro belongs to the do-while loop and not to the
> switch. So it always falls through the all cases till the end.
> 
> Remove the do-while from the macro.
> 
> Also people should terminate statements with semicolons. So force
> people to do so by removing the last one in the macro.
> 
> I vote for removing that crap completely because it makes the code
> weird anyway -- you have to specify manually both of freq and bits
> which is I would expect to be avoided exactly by such a macro.

I agree.  Andreas, please either revert back to the plain code,
or use a simple look-up table if you don't want switch/case.


thanks,

Takashi

Re: [PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro

[Takashi Iwai]

At Sat, 22 Jan 2011 14:56:10 +0100,
Jiri Slaby wrote:
> 
> Despite the AZF_FMT_XLATE macro looks totally weird and useless, it's
> heavily broken (apart it also broke my parser).
> 
> The 'break' in the macro belongs to the do-while loop and not to the
> switch. So it always falls through the all cases till the end.
> 
> Remove the do-while from the macro.
> 
> Also people should terminate statements with semicolons. So force
> people to do so by removing the last one in the macro.
> 
> I vote for removing that crap completely because it makes the code
> weird anyway -- you have to specify manually both of freq and bits
> which is I would expect to be avoided exactly by such a macro.

I agree.  Andreas, please either revert back to the plain code,
or use a simple look-up table if you don't want switch/case.


thanks,

Takashi

[PATCH] azt3328: repair breakage (Re: [PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro)

[Andreas Mohr]

Cleanly revert to non-macro implementation of
snd_azf3328_codec_setfmt(), to fix last-minute functionality breakage
induced by following checkpatch.pl recommendations without giving them
their due full share of thought ("revolting computer, ensuing PEBKAC").

I would like to thank Jiri Slaby for his very timely (in -rc1 even)
and unexpected (uncommon hardware) "recognition of the dangerous situation"
due to his very commendable static parser use. :)

Reported-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Andreas Mohr <andi@lisas.de>
---
On Sat, Jan 22, 2011 at 05:26:39PM +0100, Takashi Iwai wrote:
> At Sat, 22 Jan 2011 14:56:10 +0100,
> Jiri Slaby wrote:
> > I vote for removing that crap completely because it makes the code
> > weird anyway -- you have to specify manually both of freq and bits
> > which is I would expect to be avoided exactly by such a macro.
> 
> I agree.  Andreas, please either revert back to the plain code,
> or use a simple look-up table if you don't want switch/case.

And indeed, the currently committed state IS broken since listening to audio
is strangely devoid of positive effects. :)
Sucks to be taking a larger tried and tested patch series into the holidays
and then making minor "improvements" to it before submission.


I decided that an additional lookup table isn't really worth it,
thus reverting is probably best.


The checkpatch.pl false positive (which triggered the horrid mistake of
additionally enclosing this per-case macro within do-while(0)) was:

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#20: FILE: azt3328.c:982:
+#define AZF_FMT_XLATE(in_freq, out_bits) \
+               case AZF_FREQ_ ## in_freq: \
+                       freq = SOUNDFORMAT_FREQ_ ## out_bits; \
+                       break;


It is also quite interesting to note that this rather uncommon language syntax
(do...while(0) within switch scope) compiled cleanly without issue
(no warnings).


Please note that this clean revert (exclusively created from existing
development steps) was compile-tested only and not runtime-tested on -rc2.
This is due to existing suspected modvers issues on -rc2 (it did work fine
in many previous versions, now I'm getting unresolved symbols issues
with my -dirty, extra/ modules, perhaps due to having switched to/enabled
CONFIG_KERNEL_LZO, CONFIG_DEBUG_SET_MODULE_RONX)
which I was unable to rectify sufficiently easily so far in my limited time.

The clean revert will be followed up by
corresponding comments and some cleanup within that function in the mid-future.

Thanks,

Andreas Mohr

diff --git a/sound/pci/azt3328.c b/sound/pci/azt3328.c
index 6117595..573594b 100644
--- a/sound/pci/azt3328.c
+++ b/sound/pci/azt3328.c
@@ -979,31 +979,25 @@ snd_azf3328_codec_setfmt(struct snd_azf3328_codec_data *codec,
 
 	snd_azf3328_dbgcallenter();
 	switch (bitrate) {
-#define AZF_FMT_XLATE(in_freq, out_bits) \
-	do { \
-		case AZF_FREQ_ ## in_freq: \
-			freq = SOUNDFORMAT_FREQ_ ## out_bits; \
-			break; \
-	} while (0);
-	AZF_FMT_XLATE(4000, SUSPECTED_4000)
-	AZF_FMT_XLATE(4800, SUSPECTED_4800)
-	/* the AZF3328 names it "5510" for some strange reason: */
-	AZF_FMT_XLATE(5512, 5510)
-	AZF_FMT_XLATE(6620, 6620)
-	AZF_FMT_XLATE(8000, 8000)
-	AZF_FMT_XLATE(9600, 9600)
-	AZF_FMT_XLATE(11025, 11025)
-	AZF_FMT_XLATE(13240, SUSPECTED_13240)
-	AZF_FMT_XLATE(16000, 16000)
-	AZF_FMT_XLATE(22050, 22050)
-	AZF_FMT_XLATE(32000, 32000)
+	case AZF_FREQ_4000:  freq = SOUNDFORMAT_FREQ_SUSPECTED_4000; break;
+	case AZF_FREQ_4800:  freq = SOUNDFORMAT_FREQ_SUSPECTED_4800; break;
+	case AZF_FREQ_5512:
+		/* the AZF3328 names it "5510" for some strange reason */
+			     freq = SOUNDFORMAT_FREQ_5510; break;
+	case AZF_FREQ_6620:  freq = SOUNDFORMAT_FREQ_6620; break;
+	case AZF_FREQ_8000:  freq = SOUNDFORMAT_FREQ_8000; break;
+	case AZF_FREQ_9600:  freq = SOUNDFORMAT_FREQ_9600; break;
+	case AZF_FREQ_11025: freq = SOUNDFORMAT_FREQ_11025; break;
+	case AZF_FREQ_13240: freq = SOUNDFORMAT_FREQ_SUSPECTED_13240; break;
+	case AZF_FREQ_16000: freq = SOUNDFORMAT_FREQ_16000; break;
+	case AZF_FREQ_22050: freq = SOUNDFORMAT_FREQ_22050; break;
+	case AZF_FREQ_32000: freq = SOUNDFORMAT_FREQ_32000; break;
 	default:
 		snd_printk(KERN_WARNING "unknown bitrate %d, assuming 44.1kHz!\n", bitrate);
 		/* fall-through */
-	AZF_FMT_XLATE(44100, 44100)
-	AZF_FMT_XLATE(48000, 48000)
-	AZF_FMT_XLATE(66200, SUSPECTED_66200)
-#undef AZF_FMT_XLATE
+	case AZF_FREQ_44100: freq = SOUNDFORMAT_FREQ_44100; break;
+	case AZF_FREQ_48000: freq = SOUNDFORMAT_FREQ_48000; break;
+	case AZF_FREQ_66200: freq = SOUNDFORMAT_FREQ_SUSPECTED_66200; break;
 	}
 	/* val = 0xff07; 3m27.993s (65301Hz; -> 64000Hz???) hmm, 66120, 65967, 66123 */
 	/* val = 0xff09; 17m15.098s (13123,478Hz; -> 12000Hz???) hmm, 13237.2Hz? */

[PATCH] azt3328: repair breakage (Re: [PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro)

[Andreas Mohr]

Cleanly revert to non-macro implementation of
snd_azf3328_codec_setfmt(), to fix last-minute functionality breakage
induced by following checkpatch.pl recommendations without giving them
their due full share of thought ("revolting computer, ensuing PEBKAC").

I would like to thank Jiri Slaby for his very timely (in -rc1 even)
and unexpected (uncommon hardware) "recognition of the dangerous situation"
due to his very commendable static parser use. :)

Reported-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Andreas Mohr <andi@lisas.de>
---
On Sat, Jan 22, 2011 at 05:26:39PM +0100, Takashi Iwai wrote:
> At Sat, 22 Jan 2011 14:56:10 +0100,
> Jiri Slaby wrote:
> > I vote for removing that crap completely because it makes the code
> > weird anyway -- you have to specify manually both of freq and bits
> > which is I would expect to be avoided exactly by such a macro.
> 
> I agree.  Andreas, please either revert back to the plain code,
> or use a simple look-up table if you don't want switch/case.

And indeed, the currently committed state IS broken since listening to audio
is strangely devoid of positive effects. :)
Sucks to be taking a larger tried and tested patch series into the holidays
and then making minor "improvements" to it before submission.


I decided that an additional lookup table isn't really worth it,
thus reverting is probably best.


The checkpatch.pl false positive (which triggered the horrid mistake of
additionally enclosing this per-case macro within do-while(0)) was:

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#20: FILE: azt3328.c:982:
+#define AZF_FMT_XLATE(in_freq, out_bits) \
+               case AZF_FREQ_ ## in_freq: \
+                       freq = SOUNDFORMAT_FREQ_ ## out_bits; \
+                       break;


It is also quite interesting to note that this rather uncommon language syntax
(do...while(0) within switch scope) compiled cleanly without issue
(no warnings).


Please note that this clean revert (exclusively created from existing
development steps) was compile-tested only and not runtime-tested on -rc2.
This is due to existing suspected modvers issues on -rc2 (it did work fine
in many previous versions, now I'm getting unresolved symbols issues
with my -dirty, extra/ modules, perhaps due to having switched to/enabled
CONFIG_KERNEL_LZO, CONFIG_DEBUG_SET_MODULE_RONX)
which I was unable to rectify sufficiently easily so far in my limited time.

The clean revert will be followed up by
corresponding comments and some cleanup within that function in the mid-future.

Thanks,

Andreas Mohr

diff --git a/sound/pci/azt3328.c b/sound/pci/azt3328.c
index 6117595..573594b 100644
--- a/sound/pci/azt3328.c
+++ b/sound/pci/azt3328.c
@@ -979,31 +979,25 @@ snd_azf3328_codec_setfmt(struct snd_azf3328_codec_data *codec,
 
 	snd_azf3328_dbgcallenter();
 	switch (bitrate) {
-#define AZF_FMT_XLATE(in_freq, out_bits) \
-	do { \
-		case AZF_FREQ_ ## in_freq: \
-			freq = SOUNDFORMAT_FREQ_ ## out_bits; \
-			break; \
-	} while (0);
-	AZF_FMT_XLATE(4000, SUSPECTED_4000)
-	AZF_FMT_XLATE(4800, SUSPECTED_4800)
-	/* the AZF3328 names it "5510" for some strange reason: */
-	AZF_FMT_XLATE(5512, 5510)
-	AZF_FMT_XLATE(6620, 6620)
-	AZF_FMT_XLATE(8000, 8000)
-	AZF_FMT_XLATE(9600, 9600)
-	AZF_FMT_XLATE(11025, 11025)
-	AZF_FMT_XLATE(13240, SUSPECTED_13240)
-	AZF_FMT_XLATE(16000, 16000)
-	AZF_FMT_XLATE(22050, 22050)
-	AZF_FMT_XLATE(32000, 32000)
+	case AZF_FREQ_4000:  freq = SOUNDFORMAT_FREQ_SUSPECTED_4000; break;
+	case AZF_FREQ_4800:  freq = SOUNDFORMAT_FREQ_SUSPECTED_4800; break;
+	case AZF_FREQ_5512:
+		/* the AZF3328 names it "5510" for some strange reason */
+			     freq = SOUNDFORMAT_FREQ_5510; break;
+	case AZF_FREQ_6620:  freq = SOUNDFORMAT_FREQ_6620; break;
+	case AZF_FREQ_8000:  freq = SOUNDFORMAT_FREQ_8000; break;
+	case AZF_FREQ_9600:  freq = SOUNDFORMAT_FREQ_9600; break;
+	case AZF_FREQ_11025: freq = SOUNDFORMAT_FREQ_11025; break;
+	case AZF_FREQ_13240: freq = SOUNDFORMAT_FREQ_SUSPECTED_13240; break;
+	case AZF_FREQ_16000: freq = SOUNDFORMAT_FREQ_16000; break;
+	case AZF_FREQ_22050: freq = SOUNDFORMAT_FREQ_22050; break;
+	case AZF_FREQ_32000: freq = SOUNDFORMAT_FREQ_32000; break;
 	default:
 		snd_printk(KERN_WARNING "unknown bitrate %d, assuming 44.1kHz!\n", bitrate);
 		/* fall-through */
-	AZF_FMT_XLATE(44100, 44100)
-	AZF_FMT_XLATE(48000, 48000)
-	AZF_FMT_XLATE(66200, SUSPECTED_66200)
-#undef AZF_FMT_XLATE
+	case AZF_FREQ_44100: freq = SOUNDFORMAT_FREQ_44100; break;
+	case AZF_FREQ_48000: freq = SOUNDFORMAT_FREQ_48000; break;
+	case AZF_FREQ_66200: freq = SOUNDFORMAT_FREQ_SUSPECTED_66200; break;
 	}
 	/* val = 0xff07; 3m27.993s (65301Hz; -> 64000Hz???) hmm, 66120, 65967, 66123 */
 	/* val = 0xff09; 17m15.098s (13123,478Hz; -> 12000Hz???) hmm, 13237.2Hz? */

Re: [PATCH] azt3328: repair breakage (Re: [PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro)

[Takashi Iwai]

At Tue, 25 Jan 2011 06:46:31 +0100,
Andreas Mohr wrote:
> 
> Cleanly revert to non-macro implementation of
> snd_azf3328_codec_setfmt(), to fix last-minute functionality breakage
> induced by following checkpatch.pl recommendations without giving them
> their due full share of thought ("revolting computer, ensuing PEBKAC").
> 
> I would like to thank Jiri Slaby for his very timely (in -rc1 even)
> and unexpected (uncommon hardware) "recognition of the dangerous situation"
> due to his very commendable static parser use. :)
> 
> Reported-by: Jiri Slaby <jslaby@suse.cz>
> Signed-off-by: Andreas Mohr <andi@lisas.de>

Applied now.  Thanks.


Takashi

> ---
> On Sat, Jan 22, 2011 at 05:26:39PM +0100, Takashi Iwai wrote:
> > At Sat, 22 Jan 2011 14:56:10 +0100,
> > Jiri Slaby wrote:
> > > I vote for removing that crap completely because it makes the code
> > > weird anyway -- you have to specify manually both of freq and bits
> > > which is I would expect to be avoided exactly by such a macro.
> > 
> > I agree.  Andreas, please either revert back to the plain code,
> > or use a simple look-up table if you don't want switch/case.
> 
> And indeed, the currently committed state IS broken since listening to audio
> is strangely devoid of positive effects. :)
> Sucks to be taking a larger tried and tested patch series into the holidays
> and then making minor "improvements" to it before submission.
> 
> 
> I decided that an additional lookup table isn't really worth it,
> thus reverting is probably best.
> 
> 
> The checkpatch.pl false positive (which triggered the horrid mistake of
> additionally enclosing this per-case macro within do-while(0)) was:
> 
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #20: FILE: azt3328.c:982:
> +#define AZF_FMT_XLATE(in_freq, out_bits) \
> +               case AZF_FREQ_ ## in_freq: \
> +                       freq = SOUNDFORMAT_FREQ_ ## out_bits; \
> +                       break;
> 
> 
> It is also quite interesting to note that this rather uncommon language syntax
> (do...while(0) within switch scope) compiled cleanly without issue
> (no warnings).
> 
> 
> Please note that this clean revert (exclusively created from existing
> development steps) was compile-tested only and not runtime-tested on -rc2.
> This is due to existing suspected modvers issues on -rc2 (it did work fine
> in many previous versions, now I'm getting unresolved symbols issues
> with my -dirty, extra/ modules, perhaps due to having switched to/enabled
> CONFIG_KERNEL_LZO, CONFIG_DEBUG_SET_MODULE_RONX)
> which I was unable to rectify sufficiently easily so far in my limited time.
> 
> The clean revert will be followed up by
> corresponding comments and some cleanup within that function in the mid-future.
> 
> Thanks,
> 
> Andreas Mohr
> 
> diff --git a/sound/pci/azt3328.c b/sound/pci/azt3328.c
> index 6117595..573594b 100644
> --- a/sound/pci/azt3328.c
> +++ b/sound/pci/azt3328.c
> @@ -979,31 +979,25 @@ snd_azf3328_codec_setfmt(struct snd_azf3328_codec_data *codec,
>  
>  	snd_azf3328_dbgcallenter();
>  	switch (bitrate) {
> -#define AZF_FMT_XLATE(in_freq, out_bits) \
> -	do { \
> -		case AZF_FREQ_ ## in_freq: \
> -			freq = SOUNDFORMAT_FREQ_ ## out_bits; \
> -			break; \
> -	} while (0);
> -	AZF_FMT_XLATE(4000, SUSPECTED_4000)
> -	AZF_FMT_XLATE(4800, SUSPECTED_4800)
> -	/* the AZF3328 names it "5510" for some strange reason: */
> -	AZF_FMT_XLATE(5512, 5510)
> -	AZF_FMT_XLATE(6620, 6620)
> -	AZF_FMT_XLATE(8000, 8000)
> -	AZF_FMT_XLATE(9600, 9600)
> -	AZF_FMT_XLATE(11025, 11025)
> -	AZF_FMT_XLATE(13240, SUSPECTED_13240)
> -	AZF_FMT_XLATE(16000, 16000)
> -	AZF_FMT_XLATE(22050, 22050)
> -	AZF_FMT_XLATE(32000, 32000)
> +	case AZF_FREQ_4000:  freq = SOUNDFORMAT_FREQ_SUSPECTED_4000; break;
> +	case AZF_FREQ_4800:  freq = SOUNDFORMAT_FREQ_SUSPECTED_4800; break;
> +	case AZF_FREQ_5512:
> +		/* the AZF3328 names it "5510" for some strange reason */
> +			     freq = SOUNDFORMAT_FREQ_5510; break;
> +	case AZF_FREQ_6620:  freq = SOUNDFORMAT_FREQ_6620; break;
> +	case AZF_FREQ_8000:  freq = SOUNDFORMAT_FREQ_8000; break;
> +	case AZF_FREQ_9600:  freq = SOUNDFORMAT_FREQ_9600; break;
> +	case AZF_FREQ_11025: freq = SOUNDFORMAT_FREQ_11025; break;
> +	case AZF_FREQ_13240: freq = SOUNDFORMAT_FREQ_SUSPECTED_13240; break;
> +	case AZF_FREQ_16000: freq = SOUNDFORMAT_FREQ_16000; break;
> +	case AZF_FREQ_22050: freq = SOUNDFORMAT_FREQ_22050; break;
> +	case AZF_FREQ_32000: freq = SOUNDFORMAT_FREQ_32000; break;
>  	default:
>  		snd_printk(KERN_WARNING "unknown bitrate %d, assuming 44.1kHz!\n", bitrate);
>  		/* fall-through */
> -	AZF_FMT_XLATE(44100, 44100)
> -	AZF_FMT_XLATE(48000, 48000)
> -	AZF_FMT_XLATE(66200, SUSPECTED_66200)
> -#undef AZF_FMT_XLATE
> +	case AZF_FREQ_44100: freq = SOUNDFORMAT_FREQ_44100; break;
> +	case AZF_FREQ_48000: freq = SOUNDFORMAT_FREQ_48000; break;
> +	case AZF_FREQ_66200: freq = SOUNDFORMAT_FREQ_SUSPECTED_66200; break;
>  	}
>  	/* val = 0xff07; 3m27.993s (65301Hz; -> 64000Hz???) hmm, 66120, 65967, 66123 */
>  	/* val = 0xff09; 17m15.098s (13123,478Hz; -> 12000Hz???) hmm, 13237.2Hz? */
> 

Re: [PATCH] azt3328: repair breakage (Re: [PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro)

[Takashi Iwai]

At Tue, 25 Jan 2011 06:46:31 +0100,
Andreas Mohr wrote:
> 
> Cleanly revert to non-macro implementation of
> snd_azf3328_codec_setfmt(), to fix last-minute functionality breakage
> induced by following checkpatch.pl recommendations without giving them
> their due full share of thought ("revolting computer, ensuing PEBKAC").
> 
> I would like to thank Jiri Slaby for his very timely (in -rc1 even)
> and unexpected (uncommon hardware) "recognition of the dangerous situation"
> due to his very commendable static parser use. :)
> 
> Reported-by: Jiri Slaby <jslaby@suse.cz>
> Signed-off-by: Andreas Mohr <andi@lisas.de>

Applied now.  Thanks.


Takashi

> ---
> On Sat, Jan 22, 2011 at 05:26:39PM +0100, Takashi Iwai wrote:
> > At Sat, 22 Jan 2011 14:56:10 +0100,
> > Jiri Slaby wrote:
> > > I vote for removing that crap completely because it makes the code
> > > weird anyway -- you have to specify manually both of freq and bits
> > > which is I would expect to be avoided exactly by such a macro.
> > 
> > I agree.  Andreas, please either revert back to the plain code,
> > or use a simple look-up table if you don't want switch/case.
> 
> And indeed, the currently committed state IS broken since listening to audio
> is strangely devoid of positive effects. :)
> Sucks to be taking a larger tried and tested patch series into the holidays
> and then making minor "improvements" to it before submission.
> 
> 
> I decided that an additional lookup table isn't really worth it,
> thus reverting is probably best.
> 
> 
> The checkpatch.pl false positive (which triggered the horrid mistake of
> additionally enclosing this per-case macro within do-while(0)) was:
> 
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #20: FILE: azt3328.c:982:
> +#define AZF_FMT_XLATE(in_freq, out_bits) \
> +               case AZF_FREQ_ ## in_freq: \
> +                       freq = SOUNDFORMAT_FREQ_ ## out_bits; \
> +                       break;
> 
> 
> It is also quite interesting to note that this rather uncommon language syntax
> (do...while(0) within switch scope) compiled cleanly without issue
> (no warnings).
> 
> 
> Please note that this clean revert (exclusively created from existing
> development steps) was compile-tested only and not runtime-tested on -rc2.
> This is due to existing suspected modvers issues on -rc2 (it did work fine
> in many previous versions, now I'm getting unresolved symbols issues
> with my -dirty, extra/ modules, perhaps due to having switched to/enabled
> CONFIG_KERNEL_LZO, CONFIG_DEBUG_SET_MODULE_RONX)
> which I was unable to rectify sufficiently easily so far in my limited time.
> 
> The clean revert will be followed up by
> corresponding comments and some cleanup within that function in the mid-future.
> 
> Thanks,
> 
> Andreas Mohr
> 
> diff --git a/sound/pci/azt3328.c b/sound/pci/azt3328.c
> index 6117595..573594b 100644
> --- a/sound/pci/azt3328.c
> +++ b/sound/pci/azt3328.c
> @@ -979,31 +979,25 @@ snd_azf3328_codec_setfmt(struct snd_azf3328_codec_data *codec,
>  
>  	snd_azf3328_dbgcallenter();
>  	switch (bitrate) {
> -#define AZF_FMT_XLATE(in_freq, out_bits) \
> -	do { \
> -		case AZF_FREQ_ ## in_freq: \
> -			freq = SOUNDFORMAT_FREQ_ ## out_bits; \
> -			break; \
> -	} while (0);
> -	AZF_FMT_XLATE(4000, SUSPECTED_4000)
> -	AZF_FMT_XLATE(4800, SUSPECTED_4800)
> -	/* the AZF3328 names it "5510" for some strange reason: */
> -	AZF_FMT_XLATE(5512, 5510)
> -	AZF_FMT_XLATE(6620, 6620)
> -	AZF_FMT_XLATE(8000, 8000)
> -	AZF_FMT_XLATE(9600, 9600)
> -	AZF_FMT_XLATE(11025, 11025)
> -	AZF_FMT_XLATE(13240, SUSPECTED_13240)
> -	AZF_FMT_XLATE(16000, 16000)
> -	AZF_FMT_XLATE(22050, 22050)
> -	AZF_FMT_XLATE(32000, 32000)
> +	case AZF_FREQ_4000:  freq = SOUNDFORMAT_FREQ_SUSPECTED_4000; break;
> +	case AZF_FREQ_4800:  freq = SOUNDFORMAT_FREQ_SUSPECTED_4800; break;
> +	case AZF_FREQ_5512:
> +		/* the AZF3328 names it "5510" for some strange reason */
> +			     freq = SOUNDFORMAT_FREQ_5510; break;
> +	case AZF_FREQ_6620:  freq = SOUNDFORMAT_FREQ_6620; break;
> +	case AZF_FREQ_8000:  freq = SOUNDFORMAT_FREQ_8000; break;
> +	case AZF_FREQ_9600:  freq = SOUNDFORMAT_FREQ_9600; break;
> +	case AZF_FREQ_11025: freq = SOUNDFORMAT_FREQ_11025; break;
> +	case AZF_FREQ_13240: freq = SOUNDFORMAT_FREQ_SUSPECTED_13240; break;
> +	case AZF_FREQ_16000: freq = SOUNDFORMAT_FREQ_16000; break;
> +	case AZF_FREQ_22050: freq = SOUNDFORMAT_FREQ_22050; break;
> +	case AZF_FREQ_32000: freq = SOUNDFORMAT_FREQ_32000; break;
>  	default:
>  		snd_printk(KERN_WARNING "unknown bitrate %d, assuming 44.1kHz!\n", bitrate);
>  		/* fall-through */
> -	AZF_FMT_XLATE(44100, 44100)
> -	AZF_FMT_XLATE(48000, 48000)
> -	AZF_FMT_XLATE(66200, SUSPECTED_66200)
> -#undef AZF_FMT_XLATE
> +	case AZF_FREQ_44100: freq = SOUNDFORMAT_FREQ_44100; break;
> +	case AZF_FREQ_48000: freq = SOUNDFORMAT_FREQ_48000; break;
> +	case AZF_FREQ_66200: freq = SOUNDFORMAT_FREQ_SUSPECTED_66200; break;
>  	}
>  	/* val = 0xff07; 3m27.993s (65301Hz; -> 64000Hz???) hmm, 66120, 65967, 66123 */
>  	/* val = 0xff09; 17m15.098s (13123,478Hz; -> 12000Hz???) hmm, 13237.2Hz? */
> 

Re: [PATCH] azt3328: repair breakage (Re: [PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro)

[Jiri Slaby]

On 01/25/2011 06:46 AM, Andreas Mohr wrote:
> It is also quite interesting to note that this rather uncommon language syntax
> (do...while(0) within switch scope) compiled cleanly without issue
> (no warnings).

Note that the switch block is the same like the others and case and
default and very similar to goto labels. You can even do:
switch (x) {
int abc;
{ case 5: abc = 7; break; }
do { case 3: abc = 5; break; } while (0);
while (1) {
        printf("%d\n", abc);
default:
        abc = 1;
}
}
And check what it will do for x == 3, 5, and others :).

regards,
-- 
js
suse labs

Re: [PATCH] azt3328: repair breakage (Re: [PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro)

[Takashi Iwai]

At Tue, 25 Jan 2011 09:23:41 +0100,
Jiri Slaby wrote:
> 
> On 01/25/2011 06:46 AM, Andreas Mohr wrote:
> > It is also quite interesting to note that this rather uncommon language syntax
> > (do...while(0) within switch scope) compiled cleanly without issue
> > (no warnings).
> 
> Note that the switch block is the same like the others and case and
> default and very similar to goto labels. You can even do:
> switch (x) {
> int abc;
> { case 5: abc = 7; break; }
> do { case 3: abc = 5; break; } while (0);
> while (1) {
>         printf("%d\n", abc);
> default:
>         abc = 1;
> }
> }
> And check what it will do for x == 3, 5, and others :).

Oh, don't let him bring to the dark side of C again :)


Takashi

Re: [PATCH] azt3328: repair breakage (Re: [PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro)

[Takashi Iwai]

At Tue, 25 Jan 2011 09:23:41 +0100,
Jiri Slaby wrote:
> 
> On 01/25/2011 06:46 AM, Andreas Mohr wrote:
> > It is also quite interesting to note that this rather uncommon language syntax
> > (do...while(0) within switch scope) compiled cleanly without issue
> > (no warnings).
> 
> Note that the switch block is the same like the others and case and
> default and very similar to goto labels. You can even do:
> switch (x) {
> int abc;
> { case 5: abc = 7; break; }
> do { case 3: abc = 5; break; } while (0);
> while (1) {
>         printf("%d\n", abc);
> default:
>         abc = 1;
> }
> }
> And check what it will do for x == 3, 5, and others :).

Oh, don't let him bring to the dark side of C again :)


Takashi

Re: [PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro

[Andreas Mohr]

On Sat, Jan 22, 2011 at 02:56:10PM +0100, Jiri Slaby wrote:
> Despite the AZF_FMT_XLATE macro looks totally weird and useless, it's
> heavily broken (apart it also broke my parser).
> The 'break' in the macro belongs to the do-while loop and not to the
> switch. So it always falls through the all cases till the end.
> 
> Remove the do-while from the macro.

Hmm, there doesn't remain much to say other than "Double ouch. Mea culpa.".

However I'd like to add one bit, that the usual macro-side
do ... while(0) loop has been added by me due to a checkpatch.pl warning
which called for attention. IOW, it could be argued that checkpatch.pl
led people into ""improving"" formely "working" (whatever one may think of it)
code.
Something to be investigated, methinks.

> Also people should terminate statements with semicolons. So force
> people to do so by removing the last one in the macro.

Oh dammit of course, I'm normally implementing it this way as well.
Unfortunately I failed to submit it that way in this particular case.

> I vote for removing that crap completely because it makes the code
> weird anyway -- you have to specify manually both of freq and bits
> which is I would expect to be avoided exactly by such a macro.

The reason that it broke your parser is certainly a very valid one.
Macro syntax is known to be arcane and quite variable between compilers
(e.g. I read that the gcc people said that they were the only one who
_really_ grokked macro evaluation order and token pasting the way
it was meant to be implemented - go figure...).

As such I support a change here.
I'll try to come up with a translation table instead, 
or else probably best just revert this wetware fart.

Thanks,

Andreas Mohr

Re: [PATCH 1/1] SOUND: azt3328, fix broken AZF_FMT_XLATE macro

[Andreas Mohr]

On Sat, Jan 22, 2011 at 02:56:10PM +0100, Jiri Slaby wrote:
> Despite the AZF_FMT_XLATE macro looks totally weird and useless, it's
> heavily broken (apart it also broke my parser).
> The 'break' in the macro belongs to the do-while loop and not to the
> switch. So it always falls through the all cases till the end.
> 
> Remove the do-while from the macro.

Hmm, there doesn't remain much to say other than "Double ouch. Mea culpa.".

However I'd like to add one bit, that the usual macro-side
do ... while(0) loop has been added by me due to a checkpatch.pl warning
which called for attention. IOW, it could be argued that checkpatch.pl
led people into ""improving"" formely "working" (whatever one may think of it)
code.
Something to be investigated, methinks.

> Also people should terminate statements with semicolons. So force
> people to do so by removing the last one in the macro.

Oh dammit of course, I'm normally implementing it this way as well.
Unfortunately I failed to submit it that way in this particular case.

> I vote for removing that crap completely because it makes the code
> weird anyway -- you have to specify manually both of freq and bits
> which is I would expect to be avoided exactly by such a macro.

The reason that it broke your parser is certainly a very valid one.
Macro syntax is known to be arcane and quite variable between compilers
(e.g. I read that the gcc people said that they were the only one who
_really_ grokked macro evaluation order and token pasting the way
it was meant to be implemented - go figure...).

As such I support a change here.
I'll try to come up with a translation table instead, 
or else probably best just revert this wetware fart.

Thanks,

Andreas Mohr