[RFC PATCH] HDA: Generic input jack handling

[RFC PATCH] HDA: Generic input jack handling

[David Henningsson]

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;
  	}
  }

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

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

[Mark Brown]

On Fri, Oct 07, 2011 at 01:49:46PM +0200, David Henningsson wrote:

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

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

[Takashi Iwai]

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.

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

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.

> 3) 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);
>   #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;
>   	}
>   }
> 
> -- 
> David Henningsson, Canonical Ltd.
> http://launchpad.net/~diwic
> 

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

[David Henningsson]

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?

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

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

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

[Takashi Iwai]

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.


thanks,

Takashi

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

[David Henningsson]

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?

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

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

[Takashi Iwai]

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.

And, yes, it'd be nice if you can give a patch for tag->nid
association, too ;)


thanks,

Takashi

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

[David Henningsson]

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.

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.

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

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

[Takashi Iwai]

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

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

[Raymond Yau]

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

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

[David Henningsson]

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.

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

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

[Takashi Iwai]

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

Re: [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

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

[Takashi Iwai]

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

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

[David Henningsson]

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.

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

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

[Takashi Iwai]

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