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

11 Oct 2007

      At Wed, 10 Oct 2007 23:57:16 +0900,
sh_okada@d4.dion.ne.jp wrote:
...
A new code was put on my HP.
http://www.d4.dion.ne.jp/~sh_okada/misc/alsa-SE200PCI.patch-2.tgz
Thanks!
...
The change is as follows.
1  The coding style
Well, the current code still doesn't follow properly.
Below are some nitpickings.
...
static void se200pci_WM8766_write(struct snd_ice1712 *ice,
   				unsigned int addr,unsigned int data)
You need a space after comma (,)
...
st = (((addr&0x7f)<<9)|(data&0x1ff));
Ditto for each operator.
...
for (i=0 ; i<16 ; i++) {
Spaces around '=' and '<', no space before ';'
...
if (st & 0x10000) bits |= DATA; else bits &= ~DATA;

Break lines for each statement, namely
...
if (st & 0x10000)
	bits |= DATA;
else
	bits &= ~DATA;

And,
...
if (ch == 0) {
   	se200pci_WM8766_write(ice, 0x000, vol1);
   	se200pci_WM8766_write(ice, 0x001, vol2|0x100);
   }
   else if (ch == 1) {
Put 'else if...' with the same line as the close brace,
...
} else if (ch == 1) {
But in this case, it's better to use switch, or a value integer array.
Ditto for se200pci_WM8776_set_input_selector().
...
if (rate > 96000)
   	se200pci_WM8766_write(ice, 0x0a, 0x000); /* MCLK=128fs */
   else	se200pci_WM8766_write(ice, 0x0a, 0x080); /* MCLK=256fs */
Break the line of 'else'.
...
struct {
   char *name;
   enum { WM8766,
   	WM8776in,
   	WM8776out,
   	WM8776sel,
   	WM8776agc,
   	WM8776afl
   } target;
   enum { VOLUME1,VOLUME2,BOOLEAN,ENUM } type;
   int ch;
   char **member;
   char *comment;
} se200pci_cont[]={
Missing static here.  Also the above definition still doesn't follow
the coding style well.  The enum should be defined outside the struct
as its scope is always global in C (unlike C++).
...
{"Front Playback Volume",     WM8776out,VOLUME1,0,NULL,"Front(green)"},
Use C99 initialization style, such as
...
{
   	.name = "Front Playback Volume",
   	.target = WM8776out,
   	.type = VOLUME1,
   	.ch = 0,
   	.comment = "Front(green)"
   }
Note that 0 and NULL can be omitted in initializations if you want (as
I did for member in the above example).
se200pci_cont_info() should use switch instead of if..else.
...
for (c=0 ; c<100 ; c++) {
	if (member[c] == NULL) break;
}

First, coding style error, and second, a magic number 100 appears here
suddenly.
In se200pci_WM8766_write(),
...
unsigned int DATA  = 0x010000;
   unsigned int CLOCK = 0x020000;
   unsigned int LOAD  = 0x040000;
   unsigned int ALL_MASK = (DATA|CLOCK|LOAD);
Apparently they should be defined as const (or use define).
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);
Well, let's stop here.
...
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.
thanks,
Takashi

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

Takashi Iwai