[alsa-devel] PATCH - vt1618 7.1 Audio

[Takashi Iwai]

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.

> +
> +/* 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.

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).

> +{
> +	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.

> +	if(0x000C == mask) usShift = 2;
> +	if(0x0030 == mask) usShift = 4;

Fix coding style please.

> +
> +	pac97 = snd_kcontrol_chip(kcontrol);
> +        mutex_lock(&pac97->page_mutex);

Keep to use tabs instead of space letters (found in other places).

> +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.

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.


Takashi