Re: [RFC PATCH] HDA: Generic input jack handling

[Takashi Iwai <tiwai@suse.de>]

At Sun, 09 Oct 2011 13:14:00 +0200,
David Henningsson wrote:
> 
> 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.

Maybe true.  In that case, it can be a function like

	snd_hda_input_jack_report_tag(codec, tag)

instead of passing jack_type so that the unsol handler does a single
call.

> An additional complication to such a generic function seems to be a 
> (buggy?) Realtek codec having only 4 bit unsol tag.

If the whole handler goes to hda_codec.c, then we'd need a bit-flag
in struct hda_codec to indicate this.  It's no big issue, I think.


> > 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.

Hmm... This sounds trickier.  Can't it be simply a list of nid,
event_type and jack_type?  Or just add jack_type to the event struct
in patch_sigmatel.c.

When we track all unsol events in the table, the hardware
initialization can be simplified.  Create a function to call
SET_UNSOL_ENABLE verb for the all entries, and call it from the
codec_patch init callback.


thanks,

Takashi