[alsa-devel] PATCH - vt1618 7.1 Audio

[John L. Utz III]

Hi Takashi;

tnx for the review, a few questions inline.

On Wed, 20 Aug 2008 01:28:39 -0700, Takashi Iwai <tiwai at suse.de> wrote:

> At Tue, 19 Aug 2008 17:17:44 -0700,
> John L. Utz III wrote:
>>
>> Signed-off-by: John L. Utz III john.utz at dmx.com
>
> Thanks for the patch.
>
> First off, could you give a brief description what this patch
> does/fixes exactly?
>
> Below is a quick review.
>
>> diff --git a/pci/ac97/ac97_codec.c b/pci/ac97/ac97_codec.c
>> index d0023e9..a34f1ea 100644
>> --- a/pci/ac97/ac97_codec.c
>> +++ b/pci/ac97/ac97_codec.c
>> @@ -168,7 +168,7 @@ static const struct ac97_codec_id  
>> snd_ac97_codec_ids[]
>> = {
>>   { 0x54584e20, 0xffffffff, "TLC320AD9xC",	NULL,		NULL },
>>   { 0x56494161, 0xffffffff, "VIA1612A",		NULL,		NULL }, // modified  
>> ICE1232
>> with S/PDIF
>>   { 0x56494170, 0xffffffff, "VIA1617A",		patch_vt1617a,	NULL }, //  
>> modified
>> VT1616 with S/PDIF
>> -{ 0x56494182, 0xffffffff, "VIA1618",		NULL,		NULL },
>> +{ 0x56494182, 0xffffffff, "VIA1618",		patch_vt1618,   NULL }, // clean
>
> Please avoid C++ comments.  Yeah, we see it in other places here and
> there, but you don't need to introduce more in the newly added code.
>

OK. takes more space tho.

>> +
>> +/* use these alot in the 1618 code but i cant find a better place to  
>> put
>> them */
>> +
>> +static const char* std_enable[]  = {"Enabled", "Disabled"};
>> +static const char* std_disable[] = {"Disabled","Enabled"};
>
> They must be consistent -- usually 0 = disable, 1 = enable.
> In general, the controls like "XXX Disable" is bad.  A switch should
> be to turn on, not to turn off.  If the register bit is to disable
> something, implement the control element in a reverse way.

OK.

note that it makes the code more complex in some places because the chip  
is inconsistent.

> Also, you can consider to implement it as a switch, not as an enum,
> depending on the role.  Then it'll be often better handled in many
> mixer apps.
>
> The rest are mostly about coding styles.
>
>> +static int snd_ac97_vt1618_UAJ0_info(struct snd_kcontrol *kcontrol,
>> +				     struct snd_ctl_elem_info *uinfo)
>> +{
>> +	static const char* texts[] = { "Speaker Out", "DAC Unmixed Out", "Line
>> In", "Mic In"};
>
> Avoid too long lines (also in other places).

OK. 80 column?

>> +{
>> +	struct snd_ac97 *pac97;
>> +	ushort usDatPag, usUAJ, usShift = 0; // 0 is a possible value
>
> ushort isn't a standard type.  Use either unsigned short or u16 if it
> must be 16bit.
> Also, a Windows style variable name should be avoided.  People tend to
> hate it.

Why would you call this 'Windows Style?' Is that supposed to be a  
perjorative comment?

The code compiled with gcc and creates a linux kernel module and I am  
using Gentoo. Wouldnt that then make it reasonable to call it 'Linux  
Style' or 'GPL Style' or 'Gentoo Style? or 'John Utz Style' ?

Having variable type decorations provides valuable context and (IMHO)  
makes the code more maintainable.

>> +	if(0x000C == mask) usShift = 2;
>> +	if(0x0030 == mask) usShift = 4;
>
> Fix coding style please.

specifics please. wait. nevermind, i'll run checkpatch against it. i  
forgot to do that last nite. :-)

>> +
>> +	pac97 = snd_kcontrol_chip(kcontrol);
>> +        mutex_lock(&pac97->page_mutex);
>
> Keep to use tabs instead of space letters (found in other places).

OK

>> +static int snd_ac97_vt1618_UAJ0_get(struct snd_kcontrol *kcontrol,
>> +				    struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	return snd_ac97_vt1618_UAJ_get(kcontrol, ucontrol, 0x0003);
>> +}
>> +
>> +static int snd_ac97_vt1618_UAJ1_get(struct snd_kcontrol *kcontrol,
>> +				    struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	return snd_ac97_vt1618_UAJ_get(kcontrol, ucontrol, 0x000C);
>> +}
>
> You can use private_value field in each definition of the control,
> so that you don't have to create each different function.

OH, that would be AWESOME. Can you elaborate? I'll go grep for private  
value in the existing code and see if i can find an example whilst i wait  
for your explanation.

> About the coding style, I recommend to run checkpatch.pl script once
> before reposting your patch.  The script is found in scripts directory
> of the recent linux-kernel source tree.

yes. i realized as i was walking to the bus last nite that i had forgotten  
to do so.

johnu

> Takashi
>