[alsa-devel] [RFC PATCH] HDA: Generic input jack handling

[David Henningsson]

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