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