At Tue, 19 Aug 2008 17:17:44 -0700,
John L. Utz III wrote:
>
> Signed-off-by: John L. Utz III john.utz@xxxxxxx
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
