[alsa-devel] New alsa lowlevel driver for the SE-200PCI and SE-90PCI

[Takashi Iwai]

At Sat, 13 Oct 2007 00:05:43 +0900,
sh_okada at 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