From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Henningsson Subject: Re: [RFC PATCH] HDA: Generic input jack handling Date: Sun, 09 Oct 2011 13:14:00 +0200 Message-ID: <4E9181F8.9050405@canonical.com> References: <4E8EE75A.3050809@canonical.com> <4E8EF48F.4000707@canonical.com> <4E8F16AA.7000104@canonical.com> <4E8F2315.5070309@canonical.com> <4E915DA1.5040405@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from youngberry.canonical.com (youngberry.canonical.com [91.189.89.112]) by alsa0.perex.cz (Postfix) with ESMTP id C581324608 for ; Sun, 9 Oct 2011 13:14:00 +0200 (CEST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: alsa-devel-bounces@alsa-project.org Errors-To: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: ALSA Development Mailing List List-Id: alsa-devel@alsa-project.org On 10/09/2011 12:32 PM, Takashi Iwai wrote: > At Sun, 09 Oct 2011 10:38:57 +0200, > David Henningsson wrote: >> >> On 10/08/2011 09:11 AM, Takashi Iwai wrote: >>> At Fri, 07 Oct 2011 18:04:37 +0200, >>> David Henningsson wrote: >>>> >>>> On 10/07/2011 05:27 PM, Takashi Iwai wrote: >>>>> That being said, I'm inclined to delay it for 3.3. Now is so close to >>>>> the merge window and we don't need to rush for a refactoring patch. >>>>> It can be done more safely in the early development cycle. >>>>> Sorry if it disappoints you. >>>> >>>> This is much more than a refactoring patch - Realtek has no input jacks >>>> for everything but headphones (and mics only if autoswitch is enabled), >>>> so this is a good new feature. And my planned next step was to add it to >>>> patch_via, which currently does not have input jacks at all. >>> >>> OK. >>> >>>> To give you some background here. As you might know, I've written >>>> patches for PulseAudio to use these input jacks, that's why they're >>>> suddenly so much more important. Also, it is not certain whether the >>>> next release of Ubuntu (12.04, released April next year) will use kernel >>>> 3.2 or 3.3. Should it be 3.3, this is not a problem, but if it is 3.2, >>>> it would be better if all this was working upstream. Should it not be >>>> working, I'll need to apply it for Ubuntu only instead. >>> >>> Well, then really you shouldn't be rushing too much. >>> As mentioned, the API function will be needed to be changed because >>> it's wrong to call with jack_type. So, if you merge it in 3.2, you'll >>> get anyway API/ABI incompatibility for further changes. >>> >>> That is, if it were a self-contained patch only in patch_realtek.c, I >>> wouldn't mind too much. But it's an addition of API in HD-audio code >>> code, so I do care about it. >> >> I don't like copy-pasting the same code into several vendor's patch >> files when the function should really be generic. Therefore I don't want >> to make several self-contained patches. > > I understand it well. But, when you think of the generic code, first > think of porting the existing portions to the generic API. Then > you'll notice that the code in patch_sigmatel.c doesn't fit with your > design as it is. It'll need modifications in both sides. > >> So if you feel uncomfortable with having the generic function in 3.2, >> queue it up for 3.3 (after I've changed the jack_type stuff to your >> preference) and I'll try to backport it into Ubuntu's 3.2 kernel, should >> it become necessary. > > Don't get me wrong. If the addition were a rock-solid API that can > cover all existing cases, I'm willing to add it for 3.2 even now. > But the API design and the implementation look still having some rooms > for reconsideration. That's why I hesitate for 3.2 merge. > > Actually, I like the idea to let the driver parsing autocfg for all > pins. But, as mentioned, for the proper implementation, we'll need a > certain mapping among the NID, the unsol tag and the event > (jack-reporting). > > Then, thinking of the fact that other unsol handlers also need more or > less such a mapping, an idea like a generic unsol-event dispatcher > comes to mind. For example, imagine a table of NID containing the > corresponding tag id, and the list of function pointers to call (and > closures). The normal unsol event would be registered here, and any > input-jack events would be additionally registered to the same > NID/tag. Then, upon unsol events, the driver simply calls the > dispatcher with the tag, and the dispatcher calls the functions > assigned to the tag. Hmm, this sounds flexible enough, but perhaps a little overkill for the purpose? Are there often needs for a lot of independent functions to be called at an unsol event? Otherwise, could be simpler just to call the snd_hda_input_jack_report in addition to the current handling. An additional complication to such a generic function seems to be a (buggy?) Realtek codec having only 4 bit unsol tag. > Of course, there can be a better implementation for such a purpose. > My point is that, when you look a bit forward, you'll see the > direction of the further implementations. If the current > implementation matches well with such a forecast, we can merge the > stuff ASAP, yes. But, if it isn't clear, better to stand and look > around first. Thanks for the explanation and for your thoughts about what you think could lie ahead. What would you think about the following instead: struct detectable_jack { hda_nid_t nid; int jack_type; /* SND_JACK_xxx */ }; #define MAX_DETECTABLE_JACKS 16 struct auto_pin_cfg { /* ... */ struct detectable_jack detectable_jacks[MAX_DETECTABLE_JACKS]; }; We will let snd_hda_parse_pin_def_config fill this in as well, and the unsol_tag will be index to this array. Tags over MAX_DETECTABLE_JACKS can be used by codec specific stuff, e g sigmatel's power events. -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic