2016-12-05 22:06 GMT+01:00 Marcel Hasler <mahasler@xxxxxxxxx>: > Hello > > 2016-12-05 16:38 GMT+01:00 Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>: >> On 5 December 2016 at 09:12, Mauro Carvalho Chehab >> <mchehab@xxxxxxxxxxxxxxxx> wrote: >>> Em Sun, 4 Dec 2016 15:25:25 -0300 >>> Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx> escreveu: >>> >>>> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@xxxxxxxxx> wrote: >>>> > Hello >>>> > >>>> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>: >>>> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab >>>> >> <mchehab@xxxxxxxxxxxxxxxx> wrote: >>>> >>> Em Sun, 27 Nov 2016 12:11:48 +0100 >>>> >>> Marcel Hasler <mahasler@xxxxxxxxx> escreveu: >>>> >>> >>>> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be >>>> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows >>>> >>>> driver also sets this to 8 without any possibility for changing it. >>>> >>> >>>> >>> The problem of removing the mixer is that you need this kind of >>>> >>> crap to setup the volumes on a non-standard way. >>>> >>> >>>> >> >>>> >> Right, that's a good point. >>>> >> >>>> >>> NACK. >>>> >>> >>>> >>> Instead, keep the alsa mixer. The way other drivers do (for example, >>>> >>> em28xx) is that they configure the mixer when an input is selected, >>>> >>> increasing the volume of the active audio channel to 100% and muting >>>> >>> the other audio channels. Yet, as the alsa mixer is exported, users >>>> >>> can change the mixer settings in runtime using some alsa (or pa) >>>> >>> mixer application. >>>> >>> >>>> >> >>>> >> Yeah, the AC97 mixer we are currently leveraging >>>> >> exposes many controls that have no meaning in this device, >>>> >> so removing that still looks like an improvement. >>>> >> >>>> >> I guess the proper way is creating our own mixer >>>> >> (not using snd_ac97_mixer) exposing only the record >>>> >> gain knob. >>>> >> >>>> >> Marcel, what do you think? >>>> >> -- >>>> >> Ezequiel García, VanguardiaSur >>>> >> www.vanguardiasur.com.ar >>>> > >>>> > As I have written before, the recording gain isn't actually meant to >>>> > be changed by the user. In the official Windows driver this value is >>>> > hard-coded to 8 and cannot be changed in any way. And there really is >>>> > no good reason why anyone should need to mess with it in the first >>>> > place. The default value will give the best results in pretty much all >>>> > cases and produces approximately the same volume as the internal 8-bit >>>> > ADC whose gain cannot be changed at all, not even by a driver. >>>> > >>>> > I had considered writing a mixer but chose not to. If the gain setting >>>> > is openly exposed to mixer applications, how do you tell the users >>>> > that the value set by the driver already is the optimal and >>>> > recommended value and that they shouldn't mess with the controls >>>> > unless they really have to? By having a module parameter, this setting >>>> > is practically hidden from the normal user but still is available to >>>> > power-users if they think they really need it. In the end it's really >>>> > just a compromise between hiding it completely and exposing it openly. >>>> > Also, this way the driver guarantees reproducible results, since >>>> > there's no need to remember the positions of any volume sliders. >>>> > >>>> >>>> Hm, right. I've never changed the record gain, and it's true that it >>>> doens't really improve the volume. So, I would be OK with having >>>> a module parameter. >>>> >>>> On the other side, we are exposing it currently, through the "Capture" >>>> mixer control: >>>> >>>> Simple mixer control 'Capture',0 >>>> Capabilities: cvolume cswitch cswitch-joined >>>> Capture channels: Front Left - Front Right >>>> Limits: Capture 0 - 15 >>>> Front Left: Capture 10 [67%] [15.00dB] [on] >>>> Front Right: Capture 8 [53%] [12.00dB] [on] >>>> >>>> So, it would be user-friendly to keep the user interface and continue >>>> to expose the same knob - even if the default is the optimal, etc. >>>> >>>> To be completely honest, I don't think any user is really relying >>>> on any REC_GAIN / Capture setting, and I'm completely OK >>>> with having a mixer control or a module parameter. It doesn't matter. >>> >>> If you're positive that *all* stk1160 use the ac97 mixer the >>> same way, and that there's no sense on having a mixer for it, >>> then it would be ok to remove it. >>> >> >> Let's remove it then! >> >>> In such case, then why you need a modprobe parameter to allow >>> setting the record level? If this mixer entry is not used, >>> just set it to zero and be happy with that. >>> >> >> Let's remove the module param too, then. > > I'm okay with that. > >> >> Thanks, >> -- >> Ezequiel García, VanguardiaSur >> www.vanguardiasur.com.ar > > I'm willing to prepare one final patchset, provided we can agree on > and resolve all issues beforehand. > > So far the changes would be to remove the module param and to poll > STK1160_AC97CTL_0 instead of using a fixed delay. It's probably better > to also poll it before writing, although that never caused problems. > > I'll post some code for review before actually submitting patches. > Mauro, is there anything else that you think should be changed? If so, > please tell me now. Thanks. > > Best regards > Marcel One more thing... The driver currently uses a lot of "magic numbers", both for the AC97 register addresses as well as the STK1160 register contents. That makes it a bit difficult to read unless you happen to have the datasheet open. Would it maybe be better to add defines for those, especially if we're going to poll individual bits? I usually prefer that approach myself. Would you put the defines for the AC97 chip registers into stk1160-reg.h or keep them in stk1160-ac97.c since they're only used there? Best regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
