[alsa-devel] [RFC PATCH] HDA: Generic input jack handling
So, this is what I had in mind for 3.2. Assuming positive feedback from Takashi I'll go ahead and make a real patch out of this, and to clean up the Realtek implementation, as well as probably add this method for more codecs.
Thoughts:
1) The unsol event tags vary wildly between different vendors. How about standardising that as well?
2) If alc_init_jacks would call the new method, that might regress model-based (non auto) parsers. Is that a big deal these days?
3) Todo for realtek parser is to clean up all other calls to snd_input_jack_report so that jacks are not reported twice.
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index e9b039c..69390fd 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -5284,6 +5284,142 @@ int snd_hda_input_jack_add(struct hda_codec *codec, hda_nid_t nid, int type, } EXPORT_SYMBOL_HDA(snd_hda_input_jack_add);
+/** + * snd_hda_input_auto_jack_add - Add relevant input jacks based on + * auto pin configuration. + * @param unsol_tags lists of jack types to enable, terminate with + * list entry with jack_type set to 0. If unsol_tag is set to 0, + * unsol events will not be enabled for that jack type. + * @return 0 or error code + */ +int snd_hda_input_auto_jack_add(struct hda_codec *codec, + const struct auto_pin_cfg *cfg, + const struct hda_unsol_jack_tag* unsol_tags) +{ + for (; unsol_tags->jack_type; unsol_tags++) { + hda_nid_t nid_list_storage[AUTO_CFG_MAX_INS]; + const hda_nid_t *nid_list; + int nid_count = 0; + int i; + int input_match = AUTO_PIN_MIC; + + switch (unsol_tags->jack_type) { + case SND_JACK_LINEIN: + input_match = AUTO_PIN_LINE_IN; + /* Fall through */ + case SND_JACK_MICROPHONE: + for (i = 0; i < cfg->num_inputs; i++) { + if (cfg->inputs[i].type == input_match) + nid_list_storage[nid_count++] = cfg->inputs[i].pin; + } + nid_list = nid_list_storage; + break; + case SND_JACK_HEADPHONE: + if (cfg->line_out_type == AUTO_PIN_HP_OUT) { + nid_list = cfg->line_out_pins; + nid_count = cfg->line_outs; + } else { + nid_list = cfg->hp_pins; + nid_count = cfg->hp_outs; + } + break; + case SND_JACK_LINEOUT: + if (cfg->line_out_type == AUTO_PIN_LINE_OUT) { + nid_list = cfg->line_out_pins; + nid_count = cfg->line_outs; + } + break; + } + + for (i = 0; i < nid_count; i++) { + int pin = nid_list[i]; + int err; + if (!is_jack_detectable(codec, pin)) + continue; + + if (unsol_tags->unsol_tag) { + err = snd_hda_codec_write(codec, pin, 0, + AC_VERB_SET_UNSOLICITED_ENABLE, + AC_USRSP_EN | unsol_tags->unsol_tag); + if (err) + return err; + } + + err = snd_hda_input_jack_add(codec, pin, + unsol_tags->jack_type, NULL); + if (err) + return err; + snd_hda_input_jack_report(codec, pin); + } + } + return 0; +} +EXPORT_SYMBOL_HDA(snd_hda_input_auto_jack_add); + +void snd_hda_input_jack_report_type(struct hda_codec *codec, int jack_type) +{ + struct hda_jack_item *jacks = codec->jacks.list; + int i; + + if (!jacks) + return; + + for (i = 0; i < codec->jacks.used; i++, jacks++) + if (jacks->type == jack_type) + snd_hda_input_jack_report(codec, jacks->nid); +} +EXPORT_SYMBOL_HDA(snd_hda_input_jack_report_type); + + void snd_hda_input_jack_report(struct hda_codec *codec, hda_nid_t nid) { struct hda_jack_item *jacks = codec->jacks.list; diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 46c581c..bb59a3f 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -678,11 +678,21 @@ void snd_print_channel_allocation(int spk_alloc, char *buf, int buflen); /* * Input-jack notification support */ + +struct hda_unsol_jack_tag { + int jack_type; /* SND_JACK_xxx constant */ + int unsol_tag; /* event tag */ +}; + #ifdef CONFIG_SND_HDA_INPUT_JACK int snd_hda_input_jack_add(struct hda_codec *codec, hda_nid_t nid, int type, const char *name); void snd_hda_input_jack_report(struct hda_codec *codec, hda_nid_t nid); +void snd_hda_input_jack_report_type(struct hda_codec *codec, int jack_type); void snd_hda_input_jack_free(struct hda_codec *codec); +int snd_hda_input_auto_jack_add(struct hda_codec *codec, + const struct auto_pin_cfg *cfg, + const struct hda_unsol_jack_tag *unsol_tags); #else /* CONFIG_SND_HDA_INPUT_JACK */ static inline int snd_hda_input_jack_add(struct hda_codec *codec, hda_nid_t nid, int type, @@ -694,9 +704,19 @@ static inline void snd_hda_input_jack_report(struct hda_codec *codec, hda_nid_t nid) { } +static inline void snd_hda_input_jack_report_type(struct hda_codec *codec, + int jack_type) +{ +} static inline void snd_hda_input_jack_free(struct hda_codec *codec) { } +static inline int snd_hda_input_auto_jack_add(struct hda_codec *codec, + const struct auto_pin_cfg *cfg, + const struct hda_unsol_jack_tag *unsol_tags) +{ + return 0; +} #endif /* CONFIG_SND_HDA_INPUT_JACK */
#endif /* __SOUND_HDA_LOCAL_H */ diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index bf53663..90cdd6c 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -38,6 +38,7 @@ #define ALC_DCVOL_EVENT 0x02 #define ALC_HP_EVENT 0x04 #define ALC_MIC_EVENT 0x08 +#define ALC_LINEIN_EVENT 0x10
/* for GPIO Poll */ #define GPIO_MASK 0x03 @@ -441,11 +442,21 @@ static void alc_fix_pll_init(struct hda_codec *codec, hda_nid_t nid, * Jack-reporting via input-jack layer */
+static const struct hda_unsol_jack_tag unsol_tags[] = { + {.jack_type = SND_JACK_HEADPHONE, .unsol_tag = ALC_HP_EVENT }, + {.jack_type = SND_JACK_LINEOUT, .unsol_tag = ALC_FRONT_EVENT }, + {.jack_type = SND_JACK_MICROPHONE, .unsol_tag = ALC_MIC_EVENT }, + {.jack_type = SND_JACK_LINEIN, .unsol_tag = ALC_LINEIN_EVENT }, + {} /* Zero terminator */ +}; + /* initialization of jacks; currently checks only a few known pins */ static int alc_init_jacks(struct hda_codec *codec) { #ifdef CONFIG_SND_HDA_INPUT_JACK struct alc_spec *spec = codec->spec; + snd_hda_input_auto_jack_add(codec, &spec->autocfg, unsol_tags); +/* ; int err; unsigned int hp_nid = spec->autocfg.hp_pins[0]; unsigned int mic_nid = spec->ext_mic_pin; @@ -472,7 +483,7 @@ static int alc_init_jacks(struct hda_codec *codec) if (err < 0) return err; snd_hda_input_jack_report(codec, dock_nid); - } + }*/ #endif /* CONFIG_SND_HDA_INPUT_JACK */ return 0; } @@ -645,12 +656,18 @@ static void alc_sku_unsol_event(struct hda_codec *codec, unsigned int res) switch (res) { case ALC_HP_EVENT: alc_hp_automute(codec); + snd_hda_input_jack_report_type(codec, SND_JACK_HEADPHONE); break; case ALC_FRONT_EVENT: alc_line_automute(codec); + snd_hda_input_jack_report_type(codec, SND_JACK_LINEOUT); break; case ALC_MIC_EVENT: alc_mic_automute(codec); + snd_hda_input_jack_report_type(codec, SND_JACK_MICROPHONE); + break; + case ALC_LINEIN_EVENT: + snd_hda_input_jack_report_type(codec, SND_JACK_LINEIN); break; } }
On Fri, Oct 07, 2011 at 01:49:46PM +0200, David Henningsson wrote:
- Todo for realtek parser is to clean up all other calls to
snd_input_jack_report so that jacks are not reported twice.
FWIW this is relatively minor as duplicate reports are silently eaten by the core (it makes drivers a lot simpler).
At Fri, 07 Oct 2011 13:49:46 +0200, David Henningsson wrote:
So, this is what I had in mind for 3.2. Assuming positive feedback from Takashi I'll go ahead and make a real patch out of this, and to clean up the Realtek implementation, as well as probably add this method for more codecs.
Thoughts:
- The unsol event tags vary wildly between different vendors. How about
standardising that as well?
Generalization is good. But tags aren't always constant. It'd be better to assign each tag dynamically like in patch_sigmatel.c. The reason is that you'd need to know the pin NID from the unsol event, so the tag has to be unique even for the same purpose. E.g. if a machine has two headphones, both are the same type but they should issue unsol events with different tags.
- If alc_init_jacks would call the new method, that might regress
model-based (non auto) parsers. Is that a big deal these days?
I don't think so. And most of all model-based entries will vanish in 3.3. In 3.2, already a half of realtek quirks are gone.
- Todo for realtek parser is to clean up all other calls to
snd_input_jack_report so that jacks are not reported twice.
thanks,
Takashi
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index e9b039c..69390fd 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -5284,6 +5284,142 @@ int snd_hda_input_jack_add(struct hda_codec *codec, hda_nid_t nid, int type, } EXPORT_SYMBOL_HDA(snd_hda_input_jack_add);
+/**
- snd_hda_input_auto_jack_add - Add relevant input jacks based on
- auto pin configuration.
- @param unsol_tags lists of jack types to enable, terminate with
- list entry with jack_type set to 0. If unsol_tag is set to 0,
- unsol events will not be enabled for that jack type.
- @return 0 or error code
- */
+int snd_hda_input_auto_jack_add(struct hda_codec *codec,
const struct auto_pin_cfg *cfg,const struct hda_unsol_jack_tag* unsol_tags)+{
- for (; unsol_tags->jack_type; unsol_tags++) {
hda_nid_t nid_list_storage[AUTO_CFG_MAX_INS];const hda_nid_t *nid_list;int nid_count = 0;int i;int input_match = AUTO_PIN_MIC;switch (unsol_tags->jack_type) {case SND_JACK_LINEIN:input_match = AUTO_PIN_LINE_IN;/* Fall through */case SND_JACK_MICROPHONE:for (i = 0; i < cfg->num_inputs; i++) {if (cfg->inputs[i].type == input_match)nid_list_storage[nid_count++] = cfg->inputs[i].pin;}nid_list = nid_list_storage;break;case SND_JACK_HEADPHONE:if (cfg->line_out_type == AUTO_PIN_HP_OUT) {nid_list = cfg->line_out_pins;nid_count = cfg->line_outs;} else {nid_list = cfg->hp_pins;nid_count = cfg->hp_outs;}break;case SND_JACK_LINEOUT:if (cfg->line_out_type == AUTO_PIN_LINE_OUT) {nid_list = cfg->line_out_pins;nid_count = cfg->line_outs;}break;}for (i = 0; i < nid_count; i++) {int pin = nid_list[i];int err;if (!is_jack_detectable(codec, pin))continue;if (unsol_tags->unsol_tag) {err = snd_hda_codec_write(codec, pin, 0,AC_VERB_SET_UNSOLICITED_ENABLE,AC_USRSP_EN | unsol_tags->unsol_tag);if (err)return err;}err = snd_hda_input_jack_add(codec, pin,unsol_tags->jack_type, NULL);if (err)return err;snd_hda_input_jack_report(codec, pin);}- }
- return 0;
+} +EXPORT_SYMBOL_HDA(snd_hda_input_auto_jack_add);
+void snd_hda_input_jack_report_type(struct hda_codec *codec, int jack_type) +{
- struct hda_jack_item *jacks = codec->jacks.list;
- int i;
- if (!jacks)
return;- for (i = 0; i < codec->jacks.used; i++, jacks++)
if (jacks->type == jack_type)snd_hda_input_jack_report(codec, jacks->nid);+} +EXPORT_SYMBOL_HDA(snd_hda_input_jack_report_type);
- void snd_hda_input_jack_report(struct hda_codec *codec, hda_nid_t nid) { struct hda_jack_item *jacks = codec->jacks.list;
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 46c581c..bb59a3f 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -678,11 +678,21 @@ void snd_print_channel_allocation(int spk_alloc, char *buf, int buflen); /*
- Input-jack notification support
*/
+struct hda_unsol_jack_tag {
- int jack_type; /* SND_JACK_xxx constant */
- int unsol_tag; /* event tag */
+};
- #ifdef CONFIG_SND_HDA_INPUT_JACK int snd_hda_input_jack_add(struct hda_codec *codec, hda_nid_t nid, int
type, const char *name); void snd_hda_input_jack_report(struct hda_codec *codec, hda_nid_t nid); +void snd_hda_input_jack_report_type(struct hda_codec *codec, int jack_type); void snd_hda_input_jack_free(struct hda_codec *codec); +int snd_hda_input_auto_jack_add(struct hda_codec *codec,
const struct auto_pin_cfg *cfg,const struct hda_unsol_jack_tag *unsol_tags);@@ -694,9 +704,19 @@ static inline void snd_hda_input_jack_report(struct hda_codec *codec, hda_nid_t nid) { } +static inline void snd_hda_input_jack_report_type(struct hda_codec *codec,
int jack_type)+{ +} static inline void snd_hda_input_jack_free(struct hda_codec *codec) { } +static inline int snd_hda_input_auto_jack_add(struct hda_codec *codec,
const struct auto_pin_cfg *cfg,const struct hda_unsol_jack_tag *unsol_tags)+{
- return 0;
+} #endif /* CONFIG_SND_HDA_INPUT_JACK */
#endif /* __SOUND_HDA_LOCAL_H */ diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index bf53663..90cdd6c 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -38,6 +38,7 @@ #define ALC_DCVOL_EVENT 0x02 #define ALC_HP_EVENT 0x04 #define ALC_MIC_EVENT 0x08 +#define ALC_LINEIN_EVENT 0x10
/* for GPIO Poll */ #define GPIO_MASK 0x03 @@ -441,11 +442,21 @@ static void alc_fix_pll_init(struct hda_codec *codec, hda_nid_t nid,
- Jack-reporting via input-jack layer
*/
+static const struct hda_unsol_jack_tag unsol_tags[] = {
- {.jack_type = SND_JACK_HEADPHONE, .unsol_tag = ALC_HP_EVENT },
- {.jack_type = SND_JACK_LINEOUT, .unsol_tag = ALC_FRONT_EVENT },
- {.jack_type = SND_JACK_MICROPHONE, .unsol_tag = ALC_MIC_EVENT },
- {.jack_type = SND_JACK_LINEIN, .unsol_tag = ALC_LINEIN_EVENT },
- {} /* Zero terminator */
+};
- /* initialization of jacks; currently checks only a few known pins */ static int alc_init_jacks(struct hda_codec *codec) { #ifdef CONFIG_SND_HDA_INPUT_JACK struct alc_spec *spec = codec->spec;
- snd_hda_input_auto_jack_add(codec, &spec->autocfg, unsol_tags);
+/* ; int err; unsigned int hp_nid = spec->autocfg.hp_pins[0]; unsigned int mic_nid = spec->ext_mic_pin; @@ -472,7 +483,7 @@ static int alc_init_jacks(struct hda_codec *codec) if (err < 0) return err; snd_hda_input_jack_report(codec, dock_nid);
- }
- }*/ #endif /* CONFIG_SND_HDA_INPUT_JACK */ return 0; }
@@ -645,12 +656,18 @@ static void alc_sku_unsol_event(struct hda_codec *codec, unsigned int res) switch (res) { case ALC_HP_EVENT: alc_hp_automute(codec);
snd_hda_input_jack_report_type(codec, SND_JACK_HEADPHONE);snd_hda_input_jack_report_type(codec, SND_JACK_LINEOUT);snd_hda_input_jack_report_type(codec, SND_JACK_MICROPHONE);break;- case ALC_LINEIN_EVENT:
snd_hda_input_jack_report_type(codec, SND_JACK_LINEIN);-- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic
On 10/07/2011 02:08 PM, Takashi Iwai wrote:
At Fri, 07 Oct 2011 13:49:46 +0200, David Henningsson wrote:
So, this is what I had in mind for 3.2. Assuming positive feedback from Takashi I'll go ahead and make a real patch out of this, and to clean up the Realtek implementation, as well as probably add this method for more codecs.
Thoughts:
- The unsol event tags vary wildly between different vendors. How about
standardising that as well?
Generalization is good. But tags aren't always constant. It'd be better to assign each tag dynamically like in patch_sigmatel.c. The reason is that you'd need to know the pin NID from the unsol event, so the tag has to be unique even for the same purpose. E.g. if a machine has two headphones, both are the same type but they should issue unsol events with different tags.
One would think that this is an area where it shouldn't differ between vendors (after all, they need to do the same things, so this is just different implementations), but we can clean that up later, and when that is done we could consider standardising on having the nid as the unsol tag value. Anyway, as the patch below stands, sigmatel would call the function with unsol_tags->unsol_tag = 0, and then enable the jack itself.
Did you think the patch looked good otherwise?
- If alc_init_jacks would call the new method, that might regress
model-based (non auto) parsers. Is that a big deal these days?
I don't think so. And most of all model-based entries will vanish in 3.3. In 3.2, already a half of realtek quirks are gone.
Ok.
At Fri, 07 Oct 2011 14:46:07 +0200, David Henningsson wrote:
On 10/07/2011 02:08 PM, Takashi Iwai wrote:
At Fri, 07 Oct 2011 13:49:46 +0200, David Henningsson wrote:
So, this is what I had in mind for 3.2. Assuming positive feedback from Takashi I'll go ahead and make a real patch out of this, and to clean up the Realtek implementation, as well as probably add this method for more codecs.
Thoughts:
- The unsol event tags vary wildly between different vendors. How about
standardising that as well?
Generalization is good. But tags aren't always constant. It'd be better to assign each tag dynamically like in patch_sigmatel.c. The reason is that you'd need to know the pin NID from the unsol event, so the tag has to be unique even for the same purpose. E.g. if a machine has two headphones, both are the same type but they should issue unsol events with different tags.
One would think that this is an area where it shouldn't differ between vendors (after all, they need to do the same things, so this is just different implementations), but we can clean that up later, and when that is done we could consider standardising on having the nid as the unsol tag value. Anyway, as the patch below stands, sigmatel would call the function with unsol_tags->unsol_tag = 0, and then enable the jack itself.
Did you think the patch looked good otherwise?
Reporting per jack type isn't necessarily correct, e.g. when multiple pins for the same type are present. In that case, only the changed pin should be reported. So, in patch_realtek.c, the tag should be also individual for each pin like in patch_sigmatel.c. Currently it's using constants because of the model quirks. Once when these are removed, we can move to the dynamic allocation.
thanks,
Takashi
On 10/07/2011 03:03 PM, Takashi Iwai wrote:
At Fri, 07 Oct 2011 14:46:07 +0200, David Henningsson wrote:
On 10/07/2011 02:08 PM, Takashi Iwai wrote:
At Fri, 07 Oct 2011 13:49:46 +0200, David Henningsson wrote:
So, this is what I had in mind for 3.2. Assuming positive feedback from Takashi I'll go ahead and make a real patch out of this, and to clean up the Realtek implementation, as well as probably add this method for more codecs.
Thoughts:
- The unsol event tags vary wildly between different vendors. How about
standardising that as well?
Generalization is good. But tags aren't always constant. It'd be better to assign each tag dynamically like in patch_sigmatel.c. The reason is that you'd need to know the pin NID from the unsol event, so the tag has to be unique even for the same purpose. E.g. if a machine has two headphones, both are the same type but they should issue unsol events with different tags.
One would think that this is an area where it shouldn't differ between vendors (after all, they need to do the same things, so this is just different implementations), but we can clean that up later, and when that is done we could consider standardising on having the nid as the unsol tag value. Anyway, as the patch below stands, sigmatel would call the function with unsol_tags->unsol_tag = 0, and then enable the jack itself.
Did you think the patch looked good otherwise?
Reporting per jack type isn't necessarily correct, e.g. when multiple pins for the same type are present. In that case, only the changed pin should be reported. So, in patch_realtek.c, the tag should be also individual for each pin like in patch_sigmatel.c. Currently it's using constants because of the model quirks. Once when these are removed, we can move to the dynamic allocation.
Ok. I was afraid you would consider such a change too big to reach 3.2, and current handling does not make things worse, really - it's just slightly inoptimal to detect one more jack, but does not hurt much. Would you like me to add an associate tag -> nid array?
At Fri, 07 Oct 2011 17:11:38 +0200, David Henningsson wrote:
On 10/07/2011 03:03 PM, Takashi Iwai wrote:
At Fri, 07 Oct 2011 14:46:07 +0200, David Henningsson wrote:
On 10/07/2011 02:08 PM, Takashi Iwai wrote:
At Fri, 07 Oct 2011 13:49:46 +0200, David Henningsson wrote:
So, this is what I had in mind for 3.2. Assuming positive feedback from Takashi I'll go ahead and make a real patch out of this, and to clean up the Realtek implementation, as well as probably add this method for more codecs.
Thoughts:
- The unsol event tags vary wildly between different vendors. How about
standardising that as well?
Generalization is good. But tags aren't always constant. It'd be better to assign each tag dynamically like in patch_sigmatel.c. The reason is that you'd need to know the pin NID from the unsol event, so the tag has to be unique even for the same purpose. E.g. if a machine has two headphones, both are the same type but they should issue unsol events with different tags.
One would think that this is an area where it shouldn't differ between vendors (after all, they need to do the same things, so this is just different implementations), but we can clean that up later, and when that is done we could consider standardising on having the nid as the unsol tag value. Anyway, as the patch below stands, sigmatel would call the function with unsol_tags->unsol_tag = 0, and then enable the jack itself.
Did you think the patch looked good otherwise?
Reporting per jack type isn't necessarily correct, e.g. when multiple pins for the same type are present. In that case, only the changed pin should be reported. So, in patch_realtek.c, the tag should be also individual for each pin like in patch_sigmatel.c. Currently it's using constants because of the model quirks. Once when these are removed, we can move to the dynamic allocation.
Ok. I was afraid you would consider such a change too big to reach 3.2, and current handling does not make things worse, really - it's just slightly inoptimal to detect one more jack, but does not hurt much. Would you like me to add an associate tag -> nid array?
Well, the point is that this is a try to move a function into the core code although you know it'll need a fix. And this is no bug fix, but a code clean-up. It's very nice, but still it's to be done not to break / worsen things.
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.
And, yes, it'd be nice if you can give a patch for tag->nid association, too ;)
thanks,
Takashi
On 10/07/2011 05:27 PM, Takashi Iwai wrote:
At Fri, 07 Oct 2011 17:11:38 +0200, David Henningsson wrote:
On 10/07/2011 03:03 PM, Takashi Iwai wrote:
At Fri, 07 Oct 2011 14:46:07 +0200, David Henningsson wrote:
On 10/07/2011 02:08 PM, Takashi Iwai wrote:
At Fri, 07 Oct 2011 13:49:46 +0200, David Henningsson wrote:
So, this is what I had in mind for 3.2. Assuming positive feedback from Takashi I'll go ahead and make a real patch out of this, and to clean up the Realtek implementation, as well as probably add this method for more codecs.
Thoughts:
- The unsol event tags vary wildly between different vendors. How about
standardising that as well?
Generalization is good. But tags aren't always constant. It'd be better to assign each tag dynamically like in patch_sigmatel.c. The reason is that you'd need to know the pin NID from the unsol event, so the tag has to be unique even for the same purpose. E.g. if a machine has two headphones, both are the same type but they should issue unsol events with different tags.
One would think that this is an area where it shouldn't differ between vendors (after all, they need to do the same things, so this is just different implementations), but we can clean that up later, and when that is done we could consider standardising on having the nid as the unsol tag value. Anyway, as the patch below stands, sigmatel would call the function with unsol_tags->unsol_tag = 0, and then enable the jack itself.
Did you think the patch looked good otherwise?
Reporting per jack type isn't necessarily correct, e.g. when multiple pins for the same type are present. In that case, only the changed pin should be reported. So, in patch_realtek.c, the tag should be also individual for each pin like in patch_sigmatel.c. Currently it's using constants because of the model quirks. Once when these are removed, we can move to the dynamic allocation.
Ok. I was afraid you would consider such a change too big to reach 3.2, and current handling does not make things worse, really - it's just slightly inoptimal to detect one more jack, but does not hurt much. Would you like me to add an associate tag -> nid array?
Well, the point is that this is a try to move a function into the core code although you know it'll need a fix. And this is no bug fix, but a code clean-up. It's very nice, but still it's to be done not to break / worsen things.
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.
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.
And, yes, it'd be nice if you can give a patch for tag->nid association, too ;)
Ok, I can do that.
At Fri, 07 Oct 2011 18:04:37 +0200, David Henningsson wrote:
On 10/07/2011 05:27 PM, Takashi Iwai wrote:
At Fri, 07 Oct 2011 17:11:38 +0200, David Henningsson wrote:
On 10/07/2011 03:03 PM, Takashi Iwai wrote:
At Fri, 07 Oct 2011 14:46:07 +0200, David Henningsson wrote:
On 10/07/2011 02:08 PM, Takashi Iwai wrote:
At Fri, 07 Oct 2011 13:49:46 +0200, David Henningsson wrote: > > So, this is what I had in mind for 3.2. Assuming positive feedback from > Takashi I'll go ahead and make a real patch out of this, and to clean up > the Realtek implementation, as well as probably add this method for more > codecs. > > Thoughts: > > 1) The unsol event tags vary wildly between different vendors. How about > standardising that as well?
Generalization is good. But tags aren't always constant. It'd be better to assign each tag dynamically like in patch_sigmatel.c. The reason is that you'd need to know the pin NID from the unsol event, so the tag has to be unique even for the same purpose. E.g. if a machine has two headphones, both are the same type but they should issue unsol events with different tags.
One would think that this is an area where it shouldn't differ between vendors (after all, they need to do the same things, so this is just different implementations), but we can clean that up later, and when that is done we could consider standardising on having the nid as the unsol tag value. Anyway, as the patch below stands, sigmatel would call the function with unsol_tags->unsol_tag = 0, and then enable the jack itself.
Did you think the patch looked good otherwise?
Reporting per jack type isn't necessarily correct, e.g. when multiple pins for the same type are present. In that case, only the changed pin should be reported. So, in patch_realtek.c, the tag should be also individual for each pin like in patch_sigmatel.c. Currently it's using constants because of the model quirks. Once when these are removed, we can move to the dynamic allocation.
Ok. I was afraid you would consider such a change too big to reach 3.2, and current handling does not make things worse, really - it's just slightly inoptimal to detect one more jack, but does not hurt much. Would you like me to add an associate tag -> nid array?
Well, the point is that this is a try to move a function into the core code although you know it'll need a fix. And this is no bug fix, but a code clean-up. It's very nice, but still it's to be done not to break / worsen things.
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.
thanks,
Takashi
2011/10/8 Takashi Iwai tiwai@suse.de:
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 have doubt about this since the biggest problem is in patch_sigmatel.c
Those dell xps notebook with idt codec have dual headphone and mic jacks and there is still no way for the user to support surround51 with those "use headphone as line" and "mic jack mode" switch
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.
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.
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.
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,
Takashi
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.
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
On 10/13/2011 08:40 AM, Takashi Iwai wrote:
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.
Just for the record - I think we can to talk about this next week to see if we can merge our thoughts and come to a conclusion we're both happy with.
At Tue, 18 Oct 2011 15:13:15 +0200, David Henningsson wrote:
On 10/13/2011 08:40 AM, Takashi Iwai wrote:
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.
Just for the record - I think we can to talk about this next week to see if we can merge our thoughts and come to a conclusion we're both happy with.
Sounds OK :)
thanks,
Takashi
participants (4)
-
David Henningsson -
Mark Brown -
Raymond Yau -
Takashi Iwai