At Tue, 19 Aug 2008 17:17:44 -0700, John L. Utz III wrote:
Signed-off-by: John L. Utz III john.utz@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