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

sh_okada at d4.dion.ne.jp sh_okada at d4.dion.ne.jp
Fri Oct 12 17:05:43 CEST 2007


>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


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


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


Thanks


More information about the Alsa-devel mailing list