At Wed, 10 Oct 2007 23:57:16 +0900, sh_okada@xxxxxxxxxxxxx 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
