From: Takashi Iwai Subject: Re: [PATCH] Adds dB gain to alsa-driver, alsa-lib. Date: Fri, 16 Dec 2005 13:05:51 +0100 Message-ID: References: <43A1B744.2050703@superbug.co.uk> <43A1C7F5.6070307@superbug.co.uk> <43A29B2A.1000406@superbug.co.uk> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Return-path: In-Reply-To: <43A29B2A.1000406@superbug.co.uk> Sender: alsa-devel-admin@lists.sourceforge.net Errors-To: alsa-devel-admin@lists.sourceforge.net List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , List-Archive: To: James Courtier-Dutton Cc: Jaroslav Kysela , alsa-devel List-Id: alsa-devel@alsa-project.org At Fri, 16 Dec 2005 10:47:06 +0000, James Courtier-Dutton wrote: > > Takashi Iwai wrote: > > At Thu, 15 Dec 2005 19:45:57 +0000, > > James Courtier-Dutton wrote: > > > >>>2) The ca0106 driver is not candidate for such info. The info should > >>> be static somewhere in alsa-lib, probably in the src/control/control.c > >>> and added only when TLV stuff is requested. > >> > >>What "info" are you talking about? If you are talking about the db_scale > >>information, then I disagree with you on this point and I explained why > >>some time ago in a previous thread. I think Takashi agreed with me in > >>the end, once I managed to fully explain the reasons. > > > > > > Err, then it's a misunderstanding. I'm also against inclusion of > > unnecessary code in the kernel. That is, if the dB information for > > each control is static and unchanged, it shouldn't be embedded in the > > driver. > > > > I agree to add a new ioctl for drivers with dynamic controls or > > the drivers taking the dB information from the codec chip. > > The former case is emu10k1, which mixer can be changed via ld10k1. > > The latter case is usb-audio and hda-intel. > > > > Since the number of drivers requring this ioctl should be relatively > > small, I suppose that the implementation would be hardware-dependent. > > So, a generic "hint" and a dedicated parser in alsa-lib would suffice, > > rather than defining a too complicated struct to cover everyhing about > > dB. > > > > The case of ac97 needs a further investigation. I believe it can be > > implemented well in alsa-lib, but the implementation on the driver > > side _might_ be more efficient. Let's see the code. > > > > > > Takashi > > > > > > Ok, somone else can write this. I give up. > > Your solution is going to break down in the edge cases, and mine adds > just 4 bytes to each mixer control structure in the kernel. No extra > code at all to each driver. Just some lines of extra core code to handle > the new ioctl. > > Your suggestions will add considerably to code bloat in the alsa-lib, > and probably not work very well due to the edge cases. The code won't be compliex nor bload so much as you thought. Imagine that you just move the table of dB information into alsa-lib like static struct db_info_lookup table[] = { { "ca0106", db_info_ca0106 }, ... }; The static table could be implemented as an external file, too, of course. OTOH, the lookup-table in alsa-lib has a drawback that the table must includes the whole snd_ctl_elem_id, thus the memory usage about the control name will be doubled. So the actual size might be bigger. ... OK, now I've been slowly convinced to add minimal dB information to the kernel side. The further question is the implementation. IMO, your method, everything-about-parser-is-in-alsa-lib, is a bit exaggerated. In that way, you'll have to add a different type for each driver and chip and end up with endless bloat of new types. As Liam suggested, most cases are linear. They can be embedded in 4 bytes, including min, max, step and 0dB-offset. Take a look at HD-audio codec or USB-audio specification. (BTW, the floating point expression is (still) prohibitied in the kernel code.) One exception is the volume muting at the lowest value as often seen in ac97 codecs. This needs a different type. More than 90% cases could be coverted by these two types (linear and linear+mute), I guess. In addition, some suggestions/questions about your patch: - The name snd_ctl_misc sounds too ambiguous - (in kernel) Move the dB-specific stuff out of snd_ctl_elem_misc_user() - TLV parser (e.g. uint32_to_message) should be more optimized :) - As Jaroslav pointed, the parsing of dB information should be in the mixer layer instead of [h]control. - How to implement the reverse conversion, from dB to raw? Takashi ------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Do you grep through log files for problems? Stop! Download the new AJAX search engine that makes searching your log files as easy as surfing the web. DOWNLOAD SPLUNK! http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click