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

[Takashi Iwai]

At Wed, 10 Oct 2007 23:57:16 +0900,
sh_okada at 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