At Sat, 13 Oct 2007 00:05:43 +0900, sh_okada@d4.dion.ne.jp wrote:
Well, the current code still doesn't follow properly. Below are some nitpickings.
Thank you for a detailed check. A new one put my HP http://www.d4.dion.ne.jp/~sh_okada/misc/alsa-SE200PCI.patch-3.tgz
Thanks, I'll take a look later.
You don't need braces around a single-line if, for example,
if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE90PCI) { /* nothing to do */ } else if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE200PCI) { se200pci_add_controls(ice); }
should be
if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE90PCI) /* nothing to do */ else if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE200PCI) se200pci_add_controls(ice);
This becomes a compile error, Because when omitting it
if (A) else if (B) func();
It needs ';' or '{}' like if (A) ; else if (B) func(); or if (A) {} else if (B) func();
Therefore, "/* nothing to do */" cannot be written by this style. When forcibly writing It becomes tricky.
if (A) /* nothing to do */; else if (B) func();
or
if (A) { /* nothing to do */ } else if (B) func();
So I wrote as
/* nothing to do for VT1724_SUBDEVICE_SE90PCI */ if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE200PCI) err = se200pci_add_controls(ice);
Yeah, that's better.
2 The name of the control
Now looks better. One remaining thing is you're using an enum control for "ACG Capture Switch". If it's a switch, you should use a boolean type.
AGC has some modes, but now coded one mode. It makes it to such a style, Because it is likely to be going to increase it in the future.
now OFF, ON(type=1) future OFF, ON(type=1), ON(type=2),...
Then let's rename it to another one, such as, 'AGC Capture Mode'. Also, a value like "ON(xxx)" look ugly. I'd suggest items like 'Off', 'Type1', 'Type2'.
Takashi