At Sat, 13 Oct 2007 00:05:43 +0900, sh_okada@xxxxxxxxxxxxx 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
