[PATCH v2 0/2] ALSA: jack: Refactoring for jack kctls

[PATCH v2 0/2] ALSA: jack: Refactoring for jack kctls

[Jie Yang]

Currently only hda will create kctls for hda jacks, for asoc, people
may need create jack kctls in specific driver.

Here we are introducing kctls for each jack, by creating kctls and add
them to jack controls list (considering exist of combo jack). At the
same time, we will report events for each control in the list.

With this implement, we are no longer need hda jack specific kctls, so
here also remove jack kctls for hda.

Any comments are welcome.

Jie Yang (2):
  ALSA: jack: create jack kcontrols for every jack input device
  ALSA: hda - Remove jack kctls

 include/sound/jack.h     |  8 ++++
 sound/core/Kconfig       |  3 --
 sound/core/Makefile      |  3 +-
 sound/core/jack.c        | 99 +++++++++++++++++++++++++++++++++++++++++++++++-
 sound/pci/hda/Kconfig    | 10 +----
 sound/pci/hda/hda_jack.c | 45 ++--------------------
 sound/pci/hda/hda_jack.h |  3 --
 7 files changed, 110 insertions(+), 61 deletions(-)

-- 
1.9.1

[PATCH v2 0/2] ALSA: jack: Refactoring for jack kctls

[Jie Yang]

Currently only hda will create kctls for hda jacks, for asoc, people
may need create jack kctls in specific driver.

Here we are introducing kctls for each jack, by creating kctls and add
them to jack controls list (considering exist of combo jack). At the
same time, we will report events for each control in the list.

With this implement, we are no longer need hda jack kctls, so here
also remove jack kctls for hda.

Any comments are welcome.

Jie Yang (2):
  ALSA: jack: create jack kcontrols for every jack input device
  ALSA: hda - Remove jack kctls

 include/sound/jack.h     |  8 ++++
 sound/core/Kconfig       |  3 --
 sound/core/Makefile      |  3 +-
 sound/core/jack.c        | 99 +++++++++++++++++++++++++++++++++++++++++++++++-
 sound/pci/hda/Kconfig    | 10 +----
 sound/pci/hda/hda_jack.c | 45 ++--------------------
 sound/pci/hda/hda_jack.h |  3 --
 7 files changed, 110 insertions(+), 61 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie Yang]

Currently the ALSA jack core registers only input devices for each jack
registered. These jack input devices are not readable by userspace devices
that run as non root.

This patch adds support for additionally registering jack kcontrol devices
for every input jack registered. This allows non root userspace to read
jack status.

Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Modified-by: Jie Yang <yang.jie@intel.com>
Signed-off-by: Jie Yang <yang.jie@intel.com>
Reveiwed-by: Mark Brown <broonie@kernel.org>
---
 include/sound/jack.h |  8 +++++
 sound/core/jack.c    | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/include/sound/jack.h b/include/sound/jack.h
index 2182350..ef0f0ed 100644
--- a/include/sound/jack.h
+++ b/include/sound/jack.h
@@ -73,6 +73,8 @@ enum snd_jack_types {
 
 struct snd_jack {
 	struct input_dev *input_dev;
+	struct list_head kctl_list;
+	struct snd_card *card;
 	int registered;
 	int type;
 	const char *id;
@@ -82,6 +84,12 @@ struct snd_jack {
 	void (*private_free)(struct snd_jack *);
 };
 
+struct snd_jack_kctl {
+	struct snd_kcontrol *kctl;
+	struct list_head jack_list;		/* list of controls belong to the same jack*/
+	unsigned int jack_bit_idx;	/* the corresponding jack type bit index */
+};
+
 #ifdef CONFIG_SND_JACK
 
 int snd_jack_new(struct snd_card *card, const char *id, int type,
diff --git a/sound/core/jack.c b/sound/core/jack.c
index 8658578..741924f 100644
--- a/sound/core/jack.c
+++ b/sound/core/jack.c
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <sound/jack.h>
 #include <sound/core.h>
+#include <sound/control.h>
 
 static int jack_switch_types[SND_JACK_SWITCH_TYPES] = {
 	SW_HEADPHONE_INSERT,
@@ -54,7 +55,13 @@ static int snd_jack_dev_disconnect(struct snd_device *device)
 static int snd_jack_dev_free(struct snd_device *device)
 {
 	struct snd_jack *jack = device->device_data;
+	struct snd_card *card = device->card;
+	struct snd_jack_kctl *jack_kctl, *tmp_jack_kctl;
 
+	list_for_each_entry_safe(jack_kctl, tmp_jack_kctl, &jack->kctl_list, jack_list) {
+		list_del(&jack_kctl->jack_list);
+		snd_ctl_remove(card, jack_kctl->kctl);
+	}
 	if (jack->private_free)
 		jack->private_free(jack);
 
@@ -100,6 +107,73 @@ static int snd_jack_dev_register(struct snd_device *device)
 	return err;
 }
 
+
+/* get the first unused/available index number for the given kctl name */
+static int get_available_index(struct snd_card *card, const char *name)
+{
+	struct snd_kcontrol *kctl;
+	int idx = 0;
+	int len = strlen(name);
+
+	down_write(&card->controls_rwsem);
+	next:
+	list_for_each_entry(kctl, &card->controls, list) {
+		if (!strncmp(name, kctl->id.name, len) &&
+		    !strcmp(" Jack", kctl->id.name + len) &&
+		    kctl->id.index == idx) {
+			idx++;
+			goto next;
+		}
+	}
+	up_write(&card->controls_rwsem);
+	return idx;
+}
+
+static void kctl_private_free(struct snd_kcontrol *kctl)
+{
+	struct snd_jack_kctl *jack_kctl = kctl->private_data;
+	list_del(&jack_kctl->jack_list);
+}
+
+static int snd_jack_new_kctl(struct snd_card *card, struct snd_jack *jack, int type)
+{
+	struct snd_kcontrol *kctl;
+	struct snd_jack_kctl *jack_kctl;
+	int i, err, index, state = 0 /* use 0 for default state ?? */;
+
+	INIT_LIST_HEAD(&jack->kctl_list);
+	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
+		int testbit = 1 << i;
+		if (type & testbit) {
+			index = get_available_index(card,jack->id);
+			kctl = snd_kctl_jack_new(jack->id, index, card);
+			if (!kctl)
+				return -ENOMEM;
+
+			err = snd_ctl_add(card, kctl);
+			if (err < 0)
+				return err;
+
+			jack_kctl = kzalloc(sizeof(*jack_kctl), GFP_KERNEL);
+
+			jack_kctl->kctl = kctl;
+
+			kctl->private_data = jack_kctl;
+			kctl->private_free = kctl_private_free;
+
+			/* use jack_bit_idx for the kctl type bit */
+			jack_kctl->jack_bit_idx = i;
+
+			list_add_tail(&jack_kctl->jack_list, &jack->kctl_list);
+
+			/* set initial jack state */
+			snd_kctl_jack_report(card, kctl, state & testbit);
+		}
+	}
+
+	return 0;
+}
+
 /**
  * snd_jack_new - Create a new jack
  * @card:  the card instance
@@ -138,7 +212,7 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
 	}
 
 	jack->input_dev->phys = "ALSA";
-
+	jack->card = card;
 	jack->type = type;
 
 	for (i = 0; i < SND_JACK_SWITCH_TYPES; i++)
@@ -150,10 +224,17 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
 	if (err < 0)
 		goto fail_input;
 
+	/* register jack kcontrols  */
+	err = snd_jack_new_kctl(card, jack, type);
+	if (err < 0)
+		goto fail_kctl;
+
 	*jjack = jack;
 
 	return 0;
 
+fail_kctl:
+	snd_device_free(card, jack);
 fail_input:
 	input_free_device(jack->input_dev);
 	kfree(jack->id);
@@ -230,6 +311,7 @@ EXPORT_SYMBOL(snd_jack_set_key);
  */
 void snd_jack_report(struct snd_jack *jack, int status)
 {
+	struct snd_jack_kctl *jack_kctl;
 	int i;
 
 	if (!jack)
@@ -245,13 +327,26 @@ void snd_jack_report(struct snd_jack *jack, int status)
 
 	for (i = 0; i < ARRAY_SIZE(jack_switch_types); i++) {
 		int testbit = 1 << i;
-		if (jack->type & testbit)
+		if (jack->type & testbit) {
 			input_report_switch(jack->input_dev,
 					    jack_switch_types[i],
 					    status & testbit);
+		}
 	}
 
 	input_sync(jack->input_dev);
+
+	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
+		int testbit = 1 << i;
+		if (jack->type & testbit) {
+			list_for_each_entry(jack_kctl, &jack->kctl_list, jack_list) {
+				if (jack_kctl->jack_bit_idx == i) {
+					snd_kctl_jack_report(jack->card, jack_kctl->kctl,
+								status & testbit);
+				}
+			}
+		}
+	}
 }
 EXPORT_SYMBOL(snd_jack_report);
 
-- 
1.9.1

[PATCH v2 2/2] ALSA: hda - Remove jack kctls

[Jie Yang]

We move kctls creating to core jack part, which means that
the kctls belonging to the jack will be created at jack
new stage. Then we can remove hda jack kctls here.

Signed-off-by: Jie Yang <yang.jie@intel.com>
---
 sound/core/Kconfig       |  3 ---
 sound/core/Makefile      |  3 +--
 sound/pci/hda/Kconfig    | 10 +---------
 sound/pci/hda/hda_jack.c | 45 +++------------------------------------------
 sound/pci/hda/hda_jack.h |  3 ---
 5 files changed, 5 insertions(+), 59 deletions(-)

diff --git a/sound/core/Kconfig b/sound/core/Kconfig
index 313f22e..63cc2e9 100644
--- a/sound/core/Kconfig
+++ b/sound/core/Kconfig
@@ -221,9 +221,6 @@ config SND_PCM_XRUN_DEBUG
 config SND_VMASTER
 	bool
 
-config SND_KCTL_JACK
-	bool
-
 config SND_DMA_SGBUF
 	def_bool y
 	depends on X86
diff --git a/sound/core/Makefile b/sound/core/Makefile
index 4daf2f5..e041dc2 100644
--- a/sound/core/Makefile
+++ b/sound/core/Makefile
@@ -7,8 +7,7 @@ snd-y     := sound.o init.o memory.o info.o control.o misc.o device.o
 snd-$(CONFIG_ISA_DMA_API) += isadma.o
 snd-$(CONFIG_SND_OSSEMUL) += sound_oss.o info_oss.o
 snd-$(CONFIG_SND_VMASTER) += vmaster.o
-snd-$(CONFIG_SND_KCTL_JACK) += ctljack.o
-snd-$(CONFIG_SND_JACK)	  += jack.o
+snd-$(CONFIG_SND_JACK)	  += ctljack.o jack.o
 
 snd-pcm-y := pcm.o pcm_native.o pcm_lib.o pcm_timer.o pcm_misc.o \
 		pcm_memory.o memalloc.o
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
index 7f0f2c5..7fe4a30 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -4,7 +4,7 @@ config SND_HDA
 	tristate
 	select SND_PCM
 	select SND_VMASTER
-	select SND_KCTL_JACK
+	select SND_JACK
 
 config SND_HDA_INTEL
 	tristate "HD Audio PCI"
@@ -86,14 +86,6 @@ config SND_HDA_INPUT_BEEP_MODE
 	  Set 1 to always enable the digital beep interface for HD-audio by
 	  default.
 
-config SND_HDA_INPUT_JACK
-	bool "Support jack plugging notification via input layer"
-	depends on INPUT=y || INPUT=SND
-	select SND_JACK
-	help
-	  Say Y here to enable the jack plugging notification via
-	  input layer.
-
 config SND_HDA_PATCH_LOADER
 	bool "Support initialization patch loading for HD-audio"
 	select FW_LOADER
diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
index e664307..5cbd1a7 100644
--- a/sound/pci/hda/hda_jack.c
+++ b/sound/pci/hda/hda_jack.c
@@ -132,11 +132,9 @@ void snd_hda_jack_tbl_clear(struct hda_codec *codec)
 
 	for (i = 0; i < codec->jacktbl.used; i++, jack++) {
 		struct hda_jack_callback *cb, *next;
-#ifdef CONFIG_SND_HDA_INPUT_JACK
 		/* free jack instances manually when clearing/reconfiguring */
 		if (!codec->bus->shutdown && jack->jack)
 			snd_device_free(codec->bus->card, jack->jack);
-#endif
 		for (cb = jack->callback; cb; cb = next) {
 			next = cb->next;
 			kfree(cb);
@@ -337,20 +335,16 @@ void snd_hda_jack_report_sync(struct hda_codec *codec)
 	jack = codec->jacktbl.list;
 	for (i = 0; i < codec->jacktbl.used; i++, jack++)
 		if (jack->nid) {
-			if (!jack->kctl || jack->block_report)
+			if (!jack->jack || jack->block_report)
 				continue;
 			state = get_jack_plug_state(jack->pin_sense);
-			snd_kctl_jack_report(codec->bus->card, jack->kctl, state);
-#ifdef CONFIG_SND_HDA_INPUT_JACK
 			if (jack->jack)
 				snd_jack_report(jack->jack,
 						state ? jack->type : 0);
-#endif
 		}
 }
 EXPORT_SYMBOL_GPL(snd_hda_jack_report_sync);
 
-#ifdef CONFIG_SND_HDA_INPUT_JACK
 /* guess the jack type from the pin-config */
 static int get_input_jack_type(struct hda_codec *codec, hda_nid_t nid)
 {
@@ -377,7 +371,6 @@ static void hda_free_jack_priv(struct snd_jack *jack)
 	jacks->nid = 0;
 	jacks->jack = NULL;
 }
-#endif
 
 /**
  * snd_hda_jack_add_kctl - Add a kctl for the given pin
@@ -394,26 +387,15 @@ static int __snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid,
 			  const char *name, int idx, bool phantom_jack)
 {
 	struct hda_jack_tbl *jack;
-	struct snd_kcontrol *kctl;
 	int err, state;
 
 	jack = snd_hda_jack_tbl_new(codec, nid);
 	if (!jack)
 		return 0;
-	if (jack->kctl)
+	if (jack->jack)
 		return 0; /* already created */
-	kctl = snd_kctl_jack_new(name, idx, codec);
-	if (!kctl)
-		return -ENOMEM;
-	err = snd_hda_ctl_add(codec, nid, kctl);
-	if (err < 0)
-		return err;
-	jack->kctl = kctl;
 	jack->phantom_jack = !!phantom_jack;
 
-	state = snd_hda_jack_detect(codec, nid);
-	snd_kctl_jack_report(codec->bus->card, kctl, state);
-#ifdef CONFIG_SND_HDA_INPUT_JACK
 	if (!phantom_jack) {
 		jack->type = get_input_jack_type(codec, nid);
 		err = snd_jack_new(codec->bus->card, name, jack->type,
@@ -422,9 +404,9 @@ static int __snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid,
 			return err;
 		jack->jack->private_data = jack;
 		jack->jack->private_free = hda_free_jack_priv;
+		state = snd_hda_jack_detect(codec, nid);
 		snd_jack_report(jack->jack, state ? jack->type : 0);
 	}
-#endif
 	return 0;
 }
 
@@ -444,26 +426,6 @@ int snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid,
 }
 EXPORT_SYMBOL_GPL(snd_hda_jack_add_kctl);
 
-/* get the unique index number for the given kctl name */
-static int get_unique_index(struct hda_codec *codec, const char *name, int idx)
-{
-	struct hda_jack_tbl *jack;
-	int i, len = strlen(name);
- again:
-	jack = codec->jacktbl.list;
-	for (i = 0; i < codec->jacktbl.used; i++, jack++) {
-		/* jack->kctl.id contains "XXX Jack" name string with index */
-		if (jack->kctl &&
-		    !strncmp(name, jack->kctl->id.name, len) &&
-		    !strcmp(" Jack", jack->kctl->id.name + len) &&
-		    jack->kctl->id.index == idx) {
-			idx++;
-			goto again;
-		}
-	}
-	return idx;
-}
-
 static int add_jack_kctl(struct hda_codec *codec, hda_nid_t nid,
 			 const struct auto_pin_cfg *cfg,
 			 const char *base_name)
@@ -490,7 +452,6 @@ static int add_jack_kctl(struct hda_codec *codec, hda_nid_t nid,
 	if (phantom_jack)
 		/* Example final name: "Internal Mic Phantom Jack" */
 		strncat(name, " Phantom", sizeof(name) - strlen(name) - 1);
-	idx = get_unique_index(codec, name, idx);
 	err = __snd_hda_jack_add_kctl(codec, nid, name, idx, phantom_jack);
 	if (err < 0)
 		return err;
diff --git a/sound/pci/hda/hda_jack.h b/sound/pci/hda/hda_jack.h
index b279e32..6530faf 100644
--- a/sound/pci/hda/hda_jack.h
+++ b/sound/pci/hda/hda_jack.h
@@ -39,11 +39,8 @@ struct hda_jack_tbl {
 	unsigned int block_report:1;    /* in a transitional state - do not report to userspace */
 	hda_nid_t gating_jack;		/* valid when gating jack plugged */
 	hda_nid_t gated_jack;		/* gated is dependent on this jack */
-	struct snd_kcontrol *kctl;	/* assigned kctl for jack-detection */
-#ifdef CONFIG_SND_HDA_INPUT_JACK
 	int type;
 	struct snd_jack *jack;
-#endif
 };
 
 struct hda_jack_tbl *
-- 
1.9.1

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Takashi Iwai]

At Fri, 20 Mar 2015 16:55:18 +0800,
Jie Yang wrote:
> 
> Currently the ALSA jack core registers only input devices for each jack
> registered. These jack input devices are not readable by userspace devices
> that run as non root.
> 
> This patch adds support for additionally registering jack kcontrol devices
> for every input jack registered. This allows non root userspace to read
> jack status.
> 
> Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> Modified-by: Jie Yang <yang.jie@intel.com>
> Signed-off-by: Jie Yang <yang.jie@intel.com>
> Reveiwed-by: Mark Brown <broonie@kernel.org>
> ---
>  include/sound/jack.h |  8 +++++
>  sound/core/jack.c    | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sound/jack.h b/include/sound/jack.h
> index 2182350..ef0f0ed 100644
> --- a/include/sound/jack.h
> +++ b/include/sound/jack.h
> @@ -73,6 +73,8 @@ enum snd_jack_types {
>  
>  struct snd_jack {
>  	struct input_dev *input_dev;
> +	struct list_head kctl_list;
> +	struct snd_card *card;
>  	int registered;
>  	int type;
>  	const char *id;
> @@ -82,6 +84,12 @@ struct snd_jack {
>  	void (*private_free)(struct snd_jack *);
>  };
>  
> +struct snd_jack_kctl {
> +	struct snd_kcontrol *kctl;
> +	struct list_head jack_list;		/* list of controls belong to the same jack*/
> +	unsigned int jack_bit_idx;	/* the corresponding jack type bit index */

You can omit jack_ prefix here.

> +};
> +
>  #ifdef CONFIG_SND_JACK

Remove CONFIG_SND_JACK, both from Kconfig and ifdefs, as now it's
always enabled together with CONFIG_SND_JACK.
(Or do it later in the patch series.)

>  int snd_jack_new(struct snd_card *card, const char *id, int type,
> diff --git a/sound/core/jack.c b/sound/core/jack.c
> index 8658578..741924f 100644
> --- a/sound/core/jack.c
> +++ b/sound/core/jack.c
> @@ -24,6 +24,7 @@
>  #include <linux/module.h>
>  #include <sound/jack.h>
>  #include <sound/core.h>
> +#include <sound/control.h>
>  
>  static int jack_switch_types[SND_JACK_SWITCH_TYPES] = {
>  	SW_HEADPHONE_INSERT,
> @@ -54,7 +55,13 @@ static int snd_jack_dev_disconnect(struct snd_device *device)
>  static int snd_jack_dev_free(struct snd_device *device)
>  {
>  	struct snd_jack *jack = device->device_data;
> +	struct snd_card *card = device->card;
> +	struct snd_jack_kctl *jack_kctl, *tmp_jack_kctl;
>  
> +	list_for_each_entry_safe(jack_kctl, tmp_jack_kctl, &jack->kctl_list, jack_list) {
> +		list_del(&jack_kctl->jack_list);

Use list_del_init().  Otherwise it'll strike back.
(list_del() will be called again in private_free callback from
 snd_ctl_remove(), and this can be broken.)

> +		snd_ctl_remove(card, jack_kctl->kctl);
> +	}
>  	if (jack->private_free)
>  		jack->private_free(jack);
>  
> @@ -100,6 +107,73 @@ static int snd_jack_dev_register(struct snd_device *device)
>  	return err;
>  }
>  
> +
> +/* get the first unused/available index number for the given kctl name */
> +static int get_available_index(struct snd_card *card, const char *name)
> +{
> +	struct snd_kcontrol *kctl;
> +	int idx = 0;
> +	int len = strlen(name);
> +
> +	down_write(&card->controls_rwsem);

Use down_read(), as Liam already suggested.

> +	next:
> +	list_for_each_entry(kctl, &card->controls, list) {
> +		if (!strncmp(name, kctl->id.name, len) &&
> +		    !strcmp(" Jack", kctl->id.name + len) &&
> +		    kctl->id.index == idx) {
> +			idx++;
> +			goto next;
> +		}
> +	}

Better to split a small function, e.g.

	while (!jack_ctl_name_found(card, kctl))
		idx++;

than using ugly goto.

> +	up_write(&card->controls_rwsem);
> +	return idx;
> +}
> +
> +static void kctl_private_free(struct snd_kcontrol *kctl)
> +{
> +	struct snd_jack_kctl *jack_kctl = kctl->private_data;
> +	list_del(&jack_kctl->jack_list);

kfree() is forgotten.

> +}
> +
> +static int snd_jack_new_kctl(struct snd_card *card, struct snd_jack *jack, int type)
> +{
> +	struct snd_kcontrol *kctl;
> +	struct snd_jack_kctl *jack_kctl;
> +	int i, err, index, state = 0 /* use 0 for default state ?? */;
> +
> +	INIT_LIST_HEAD(&jack->kctl_list);
> +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> +		int testbit = 1 << i;
> +		if (type & testbit) {

With this implementation, you'll get multiple boolean kctls for a
headset.  I thought we agreed with creating a single boolean for
headset?

In the case of input device, the situation is a bit different, so we
shouldn't mix up.


> +			index = get_available_index(card,jack->id);
> +			kctl = snd_kctl_jack_new(jack->id, index, card);
> +			if (!kctl)
> +				return -ENOMEM;
> +
> +			err = snd_ctl_add(card, kctl);
> +			if (err < 0)
> +				return err;
> +
> +			jack_kctl = kzalloc(sizeof(*jack_kctl), GFP_KERNEL);

Missing NULL check.

> +			jack_kctl->kctl = kctl;
> +
> +			kctl->private_data = jack_kctl;
> +			kctl->private_free = kctl_private_free;
> +
> +			/* use jack_bit_idx for the kctl type bit */
> +			jack_kctl->jack_bit_idx = i;

This can be better to hold bit mask instead of bit index.
Then in the below...

> @@ -245,13 +327,26 @@ void snd_jack_report(struct snd_jack *jack, int status)
....
>  	}
>  
>  	input_sync(jack->input_dev);
> +
> +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> +		int testbit = 1 << i;
> +		if (jack->type & testbit) {
> +			list_for_each_entry(jack_kctl, &jack->kctl_list, jack_list) {
> +				if (jack_kctl->jack_bit_idx == i) {
> +					snd_kctl_jack_report(jack->card, jack_kctl->kctl,
> +								status & testbit);
> +				}

.... you can reduce to a single loop.


thanks,

Takashi

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Takashi Iwai]

At Fri, 20 Mar 2015 10:27:37 +0100,
Takashi Iwai wrote:
> 
> > +};
> > +
> >  #ifdef CONFIG_SND_JACK
> 
> Remove CONFIG_SND_JACK, both from Kconfig and ifdefs, as now it's
> always enabled together with CONFIG_SND_JACK.
> (Or do it later in the patch series.)

Disregard this comment, I was confused about CONFIG_SND_KCTL_JACK.


Takashi

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Raymond Yau]

>
> Currently the ALSA jack core registers only input devices for each jack
> registered. These jack input devices are not readable by userspace devices
> that run as non root.
>
> This patch adds support for additionally registering jack kcontrol devices
> for every input jack registered. This allows non root userspace to read
> jack status.
>
> Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> Modified-by: Jie Yang <yang.jie@intel.com>
> Signed-off-by: Jie Yang <yang.jie@intel.com>
> Reveiwed-by: Mark Brown <broonie@kernel.org>
> ---
>  include/sound/jack.h |  8 +++++
>  sound/core/jack.c    | 99
++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 105 insertions(+), 2 deletions(-)
>
> diff --git a/include/sound/jack.h b/include/sound/jack.h
> index 2182350..ef0f0ed 100644
> --- a/include/sound/jack.h
> +++ b/include/sound/jack.h
> @@ -73,6 +73,8 @@ enum snd_jack_types {
>
>  struct snd_jack {
>         struct input_dev *input_dev;
> +       struct list_head kctl_list;
> +       struct snd_card *card;
>         int registered;
>         int type;
>         const char *id;
> @@ -82,6 +84,12 @@ struct snd_jack {
>         void (*private_free)(struct snd_jack *);
>  };
>
> +struct snd_jack_kctl {
> +       struct snd_kcontrol *kctl;
> +       struct list_head jack_list;             /* list of controls
belong to the same jack*/

Does it mean that those multi io jacks (line in and mic jacks of desktop)
and those headphone/mic combo jack of netbook have all the input and output
kctls in this list ?

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, March 20, 2015 5:28 PM
> To: Jie, Yang
> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> Girdwood, Liam R; Liam Girdwood
> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> input device
> 
> At Fri, 20 Mar 2015 16:55:18 +0800,
> Jie Yang wrote:
> >
> > Currently the ALSA jack core registers only input devices for each
> > jack registered. These jack input devices are not readable by
> > userspace devices that run as non root.
> >
> > This patch adds support for additionally registering jack kcontrol
> > devices for every input jack registered. This allows non root
> > userspace to read jack status.
> >
> > Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> > Modified-by: Jie Yang <yang.jie@intel.com>
> > Signed-off-by: Jie Yang <yang.jie@intel.com>
> > Reveiwed-by: Mark Brown <broonie@kernel.org>
> > ---
> >  include/sound/jack.h |  8 +++++
> >  sound/core/jack.c    | 99
> ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 105 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/sound/jack.h b/include/sound/jack.h index
> > 2182350..ef0f0ed 100644
> > --- a/include/sound/jack.h
> > +++ b/include/sound/jack.h
> > @@ -73,6 +73,8 @@ enum snd_jack_types {
> >
> >  struct snd_jack {
> >  	struct input_dev *input_dev;
> > +	struct list_head kctl_list;
> > +	struct snd_card *card;
> >  	int registered;
> >  	int type;
> >  	const char *id;
> > @@ -82,6 +84,12 @@ struct snd_jack {
> >  	void (*private_free)(struct snd_jack *);  };
> >
> > +struct snd_jack_kctl {
> > +	struct snd_kcontrol *kctl;
> > +	struct list_head jack_list;		/* list of controls belong to
> the same jack*/
> > +	unsigned int jack_bit_idx;	/* the corresponding jack type bit
> index */
> 
> You can omit jack_ prefix here.
[Keyon] OK.
> 
> > +};
> > +
> >  #ifdef CONFIG_SND_JACK
> 
> Remove CONFIG_SND_JACK, both from Kconfig and ifdefs, as now it's always
> enabled together with CONFIG_SND_JACK.
> (Or do it later in the patch series.)
> 
> >  int snd_jack_new(struct snd_card *card, const char *id, int type,
> > diff --git a/sound/core/jack.c b/sound/core/jack.c index
> > 8658578..741924f 100644
> > --- a/sound/core/jack.c
> > +++ b/sound/core/jack.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/module.h>
> >  #include <sound/jack.h>
> >  #include <sound/core.h>
> > +#include <sound/control.h>
> >
> >  static int jack_switch_types[SND_JACK_SWITCH_TYPES] = {
> >  	SW_HEADPHONE_INSERT,
> > @@ -54,7 +55,13 @@ static int snd_jack_dev_disconnect(struct
> > snd_device *device)  static int snd_jack_dev_free(struct snd_device
> > *device)  {
> >  	struct snd_jack *jack = device->device_data;
> > +	struct snd_card *card = device->card;
> > +	struct snd_jack_kctl *jack_kctl, *tmp_jack_kctl;
> >
> > +	list_for_each_entry_safe(jack_kctl, tmp_jack_kctl, &jack->kctl_list,
> jack_list) {
> > +		list_del(&jack_kctl->jack_list);
> 
> Use list_del_init().  Otherwise it'll strike back.
> (list_del() will be called again in private_free callback from  snd_ctl_remove(),
> and this can be broken.)
[Keyon] good! Thanks for pointing out this potential risk!
> 
> > +		snd_ctl_remove(card, jack_kctl->kctl);
> > +	}
> >  	if (jack->private_free)
> >  		jack->private_free(jack);
> >
> > @@ -100,6 +107,73 @@ static int snd_jack_dev_register(struct snd_device
> *device)
> >  	return err;
> >  }
> >
> > +
> > +/* get the first unused/available index number for the given kctl
> > +name */ static int get_available_index(struct snd_card *card, const
> > +char *name) {
> > +	struct snd_kcontrol *kctl;
> > +	int idx = 0;
> > +	int len = strlen(name);
> > +
> > +	down_write(&card->controls_rwsem);
> 
> Use down_read(), as Liam already suggested.
> 
> > +	next:
> > +	list_for_each_entry(kctl, &card->controls, list) {
> > +		if (!strncmp(name, kctl->id.name, len) &&
> > +		    !strcmp(" Jack", kctl->id.name + len) &&
> > +		    kctl->id.index == idx) {
> > +			idx++;
> > +			goto next;
> > +		}
> > +	}
> 
> Better to split a small function, e.g.
> 
> 	while (!jack_ctl_name_found(card, kctl))
> 		idx++;
> 
> than using ugly goto.
[Keyon] OK, will do like that.
> 
> > +	up_write(&card->controls_rwsem);
> > +	return idx;
> > +}
> > +
> > +static void kctl_private_free(struct snd_kcontrol *kctl) {
> > +	struct snd_jack_kctl *jack_kctl = kctl->private_data;
> > +	list_del(&jack_kctl->jack_list);
> 
> kfree() is forgotten.
[Keyon] right.
> 
> > +}
> > +
> > +static int snd_jack_new_kctl(struct snd_card *card, struct snd_jack
> > +*jack, int type) {
> > +	struct snd_kcontrol *kctl;
> > +	struct snd_jack_kctl *jack_kctl;
> > +	int i, err, index, state = 0 /* use 0 for default state ?? */;
> > +
> > +	INIT_LIST_HEAD(&jack->kctl_list);
> > +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> > +		int testbit = 1 << i;
> > +		if (type & testbit) {
> 
> With this implementation, you'll get multiple boolean kctls for a headset.  I
> thought we agreed with creating a single boolean for headset?
[Keyon] We agreed with creating multiple kctls for combo jack, e.g. headset.
Furthermore, e.g., imagine that type = SND_JACK_HEADSET  | SND_JACK_BTN_0,
we will create 3 kctls for the jack, when BTN_0 is pressed, we will report to
the 3rd kctl.
> 
> In the case of input device, the situation is a bit different, so we shouldn't
> mix up.
> 
> 
> > +			index = get_available_index(card,jack->id);
> > +			kctl = snd_kctl_jack_new(jack->id, index, card);
> > +			if (!kctl)
> > +				return -ENOMEM;
> > +
> > +			err = snd_ctl_add(card, kctl);
> > +			if (err < 0)
> > +				return err;
> > +
> > +			jack_kctl = kzalloc(sizeof(*jack_kctl), GFP_KERNEL);
> 
> Missing NULL check.
[Keyon] got it, add it soon.
> 
> > +			jack_kctl->kctl = kctl;
> > +
> > +			kctl->private_data = jack_kctl;
> > +			kctl->private_free = kctl_private_free;
> > +
> > +			/* use jack_bit_idx for the kctl type bit */
> > +			jack_kctl->jack_bit_idx = i;
> 
> This can be better to hold bit mask instead of bit index.
[Keyon] good idea, will change to bit mask.
> Then in the below...
> 
> > @@ -245,13 +327,26 @@ void snd_jack_report(struct snd_jack *jack, int
> > status)
> ....
> >  	}
> >
> >  	input_sync(jack->input_dev);
> > +
> > +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> > +		int testbit = 1 << i;
> > +		if (jack->type & testbit) {
> > +			list_for_each_entry(jack_kctl, &jack->kctl_list,
> jack_list) {
> > +				if (jack_kctl->jack_bit_idx == i) {
> > +					snd_kctl_jack_report(jack->card,
> jack_kctl->kctl,
> > +								status &
> testbit);
> > +				}
> 
> .... you can reduce to a single loop.
> 
> 
> thanks,
> 
> Takashi

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Takashi Iwai]

At Fri, 20 Mar 2015 12:22:10 +0000,
Jie, Yang wrote:
> 
> > > +}
> > > +
> > > +static int snd_jack_new_kctl(struct snd_card *card, struct snd_jack
> > > +*jack, int type) {
> > > +	struct snd_kcontrol *kctl;
> > > +	struct snd_jack_kctl *jack_kctl;
> > > +	int i, err, index, state = 0 /* use 0 for default state ?? */;
> > > +
> > > +	INIT_LIST_HEAD(&jack->kctl_list);
> > > +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> > > +		int testbit = 1 << i;
> > > +		if (type & testbit) {
> > 
> > With this implementation, you'll get multiple boolean kctls for a headset.  I
> > thought we agreed with creating a single boolean for headset?
> [Keyon] We agreed with creating multiple kctls for combo jack, e.g. headset.
> Furthermore, e.g., imagine that type = SND_JACK_HEADSET  | SND_JACK_BTN_0,
> we will create 3 kctls for the jack, when BTN_0 is pressed, we will report to
> the 3rd kctl.

Hm, I don't remember that I agreed with multiple kctls...

The multiple kctls have a significant drawback (multiple event calls
for a single headset) and its behavior is incompatible with the
current code (both the name change and the behavior change).  That is,
your patch will very likely break the existing applications.


Takashi

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, March 20, 2015 8:27 PM
> To: Jie, Yang
> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> Girdwood, Liam R; Liam Girdwood
> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> input device
> 
> At Fri, 20 Mar 2015 12:22:10 +0000,
> Jie, Yang wrote:
> >
> > > > +}
> > > > +
> > > > +static int snd_jack_new_kctl(struct snd_card *card, struct
> > > > +snd_jack *jack, int type) {
> > > > +	struct snd_kcontrol *kctl;
> > > > +	struct snd_jack_kctl *jack_kctl;
> > > > +	int i, err, index, state = 0 /* use 0 for default state ?? */;
> > > > +
> > > > +	INIT_LIST_HEAD(&jack->kctl_list);
> > > > +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> > > > +		int testbit = 1 << i;
> > > > +		if (type & testbit) {
> > >
> > > With this implementation, you'll get multiple boolean kctls for a
> > > headset.  I thought we agreed with creating a single boolean for headset?
> > [Keyon] We agreed with creating multiple kctls for combo jack, e.g.
> headset.
> > Furthermore, e.g., imagine that type = SND_JACK_HEADSET  |
> > SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is
> > pressed, we will report to the 3rd kctl.
> 
> Hm, I don't remember that I agreed with multiple kctls...
> 
> The multiple kctls have a significant drawback (multiple event calls for a single
> headset) and its behavior is incompatible with the current code (both the
> name change and the behavior change).  That is, your patch will very likely
> break the existing applications.
[Keyon] I am not very clear with the existing applications that using these 
kctl events(seems Android use input subsystem event? Which we didn't
Change here. If I understand correctly, Pulseaudio uses jack switch controls, 
via the name, then we can use different name for headphone and mic here.)

we will generate 2 event calls(one headphone, one mic) when Headset 
plug in/out, applications will receive these 2 events,  and they can do anything,
e.g. decide to switch on/off speaker/headphone.

BTW, I haven't implemented the generating of combo jack kctls' name yet,
currently, they looked like below:
numid=12,iface=CARD,name='Headset Jack'
numid=13,iface=CARD,name='Headset Jack',index=1
numid=14,iface=CARD,name='Headset Jack',index=2

once we have come to agreement, we can modify it in snd_jack_new_kctl(),
e.g., "Headset Jack Mic" and "Headset Jack Speakers".

> 
> 
> Takashi

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Takashi Iwai]

At Fri, 20 Mar 2015 12:50:33 +0000,
Jie, Yang wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Friday, March 20, 2015 8:27 PM
> > To: Jie, Yang
> > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> > Girdwood, Liam R; Liam Girdwood
> > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> > input device
> > 
> > At Fri, 20 Mar 2015 12:22:10 +0000,
> > Jie, Yang wrote:
> > >
> > > > > +}
> > > > > +
> > > > > +static int snd_jack_new_kctl(struct snd_card *card, struct
> > > > > +snd_jack *jack, int type) {
> > > > > +	struct snd_kcontrol *kctl;
> > > > > +	struct snd_jack_kctl *jack_kctl;
> > > > > +	int i, err, index, state = 0 /* use 0 for default state ?? */;
> > > > > +
> > > > > +	INIT_LIST_HEAD(&jack->kctl_list);
> > > > > +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> > > > > +		int testbit = 1 << i;
> > > > > +		if (type & testbit) {
> > > >
> > > > With this implementation, you'll get multiple boolean kctls for a
> > > > headset.  I thought we agreed with creating a single boolean for headset?
> > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g.
> > headset.
> > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET  |
> > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is
> > > pressed, we will report to the 3rd kctl.
> > 
> > Hm, I don't remember that I agreed with multiple kctls...
> > 
> > The multiple kctls have a significant drawback (multiple event calls for a single
> > headset) and its behavior is incompatible with the current code (both the
> > name change and the behavior change).  That is, your patch will very likely
> > break the existing applications.
> [Keyon] I am not very clear with the existing applications that using these 
> kctl events(seems Android use input subsystem event? Which we didn't
> Change here. If I understand correctly, Pulseaudio uses jack switch controls, 
> via the name, then we can use different name for headphone and mic here.)

PA uses jack kctls.

If you rename, how would you guarantee that the existing application
will work as expected?  PA doesn't have the definition of "Headset
Speaker Jack" or such.

And, no, we have no option "fix PA".  Other way round: we are not
allowed to break the current PA (or any user-space) behavior in
general.

> we will generate 2 event calls(one headphone, one mic) when Headset 
> plug in/out, applications will receive these 2 events,  and they can do anything,
> e.g. decide to switch on/off speaker/headphone.

Won't this break any user-space stuff?

> BTW, I haven't implemented the generating of combo jack kctls' name yet,
> currently, they looked like below:
> numid=12,iface=CARD,name='Headset Jack'
> numid=13,iface=CARD,name='Headset Jack',index=1
> numid=14,iface=CARD,name='Headset Jack',index=2
> 
> once we have come to agreement, we can modify it in snd_jack_new_kctl(),
> e.g., "Headset Jack Mic" and "Headset Jack Speakers".

.... and how the existing user-space works without changing its code?


Keyon, the most important point at this moment is to keep the
compatibility.  HD-audio is no new driver.  It's a driver that has
been present over a decade with (literally) thousands of variants.
Please keep this in mind, and reconsider whether your patch will
retain the compatibility, especially with PulseAudio.


thanks,

Takashi

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, March 20, 2015 9:21 PM
> To: Jie, Yang
> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> Girdwood, Liam R; Liam Girdwood; Tanu Kaskinen
> (tanu.kaskinen@linux.intel.com)
> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> input device
> 
> At Fri, 20 Mar 2015 12:50:33 +0000,
> Jie, Yang wrote:
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Friday, March 20, 2015 8:27 PM
> > > To: Jie, Yang
> > > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> > > Girdwood, Liam R; Liam Girdwood
> > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for
> > > every jack input device
> > >
> > > At Fri, 20 Mar 2015 12:22:10 +0000,
> > > Jie, Yang wrote:
> > > >
> > > > > > +}
> > > > > > +
> > > > > > +static int snd_jack_new_kctl(struct snd_card *card, struct
> > > > > > +snd_jack *jack, int type) {
> > > > > > +	struct snd_kcontrol *kctl;
> > > > > > +	struct snd_jack_kctl *jack_kctl;
> > > > > > +	int i, err, index, state = 0 /* use 0 for default state ??
> > > > > > +*/;
> > > > > > +
> > > > > > +	INIT_LIST_HEAD(&jack->kctl_list);
> > > > > > +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> > > > > > +		int testbit = 1 << i;
> > > > > > +		if (type & testbit) {
> > > > >
> > > > > With this implementation, you'll get multiple boolean kctls for
> > > > > a headset.  I thought we agreed with creating a single boolean for
> headset?
> > > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g.
> > > headset.
> > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET  |
> > > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is
> > > > pressed, we will report to the 3rd kctl.
> > >
> > > Hm, I don't remember that I agreed with multiple kctls...
> > >
> > > The multiple kctls have a significant drawback (multiple event calls
> > > for a single
> > > headset) and its behavior is incompatible with the current code
> > > (both the name change and the behavior change).  That is, your patch
> > > will very likely break the existing applications.
> > [Keyon] I am not very clear with the existing applications that using
> > these kctl events(seems Android use input subsystem event? Which we
> > didn't Change here. If I understand correctly, Pulseaudio uses jack
> > switch controls, via the name, then we can use different name for
> > headphone and mic here.)
> 
> PA uses jack kctls.
> 
> If you rename, how would you guarantee that the existing application will
> work as expected?  PA doesn't have the definition of "Headset Speaker Jack"
> or such.
> 
> And, no, we have no option "fix PA".  Other way round: we are not allowed
> to break the current PA (or any user-space) behavior in general.
> 
> > we will generate 2 event calls(one headphone, one mic) when Headset
> > plug in/out, applications will receive these 2 events,  and they can
> > do anything, e.g. decide to switch on/off speaker/headphone.
> 
> Won't this break any user-space stuff?
> 
> > BTW, I haven't implemented the generating of combo jack kctls' name
> > yet, currently, they looked like below:
> > numid=12,iface=CARD,name='Headset Jack'
> > numid=13,iface=CARD,name='Headset Jack',index=1
> > numid=14,iface=CARD,name='Headset Jack',index=2
> >
> > once we have come to agreement, we can modify it in
> > snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset Jack
> Speakers".
> 
> .... and how the existing user-space works without changing its code?
> 
> 
> Keyon, the most important point at this moment is to keep the compatibility.
> HD-audio is no new driver.  It's a driver that has been present over a decade
> with (literally) thousands of variants.
> Please keep this in mind, and reconsider whether your patch will retain the
> compatibility, especially with PulseAudio.
[Keyon] understood. Then we should follow the HD-audio style, So, what do 
you suggest here? Should we create only one single Boolean kctls for headset,
and report true when status in headphone bit it true? Then we need a tricky
exception mapping here?

Sorry, I am a little confusing here, because Mark suggest to create multiple controls
for multiple bits jack, and you also agreed with that.  :(

> 
> 
> thanks,
> 
> Takashi

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Takashi Iwai]

At Fri, 20 Mar 2015 13:49:24 +0000,
Jie, Yang wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Friday, March 20, 2015 9:21 PM
> > To: Jie, Yang
> > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> > Girdwood, Liam R; Liam Girdwood; Tanu Kaskinen
> > (tanu.kaskinen@linux.intel.com)
> > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> > input device
> > 
> > At Fri, 20 Mar 2015 12:50:33 +0000,
> > Jie, Yang wrote:
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Friday, March 20, 2015 8:27 PM
> > > > To: Jie, Yang
> > > > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> > > > Girdwood, Liam R; Liam Girdwood
> > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for
> > > > every jack input device
> > > >
> > > > At Fri, 20 Mar 2015 12:22:10 +0000,
> > > > Jie, Yang wrote:
> > > > >
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int snd_jack_new_kctl(struct snd_card *card, struct
> > > > > > > +snd_jack *jack, int type) {
> > > > > > > +	struct snd_kcontrol *kctl;
> > > > > > > +	struct snd_jack_kctl *jack_kctl;
> > > > > > > +	int i, err, index, state = 0 /* use 0 for default state ??
> > > > > > > +*/;
> > > > > > > +
> > > > > > > +	INIT_LIST_HEAD(&jack->kctl_list);
> > > > > > > +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> > > > > > > +		int testbit = 1 << i;
> > > > > > > +		if (type & testbit) {
> > > > > >
> > > > > > With this implementation, you'll get multiple boolean kctls for
> > > > > > a headset.  I thought we agreed with creating a single boolean for
> > headset?
> > > > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g.
> > > > headset.
> > > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET  |
> > > > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is
> > > > > pressed, we will report to the 3rd kctl.
> > > >
> > > > Hm, I don't remember that I agreed with multiple kctls...
> > > >
> > > > The multiple kctls have a significant drawback (multiple event calls
> > > > for a single
> > > > headset) and its behavior is incompatible with the current code
> > > > (both the name change and the behavior change).  That is, your patch
> > > > will very likely break the existing applications.
> > > [Keyon] I am not very clear with the existing applications that using
> > > these kctl events(seems Android use input subsystem event? Which we
> > > didn't Change here. If I understand correctly, Pulseaudio uses jack
> > > switch controls, via the name, then we can use different name for
> > > headphone and mic here.)
> > 
> > PA uses jack kctls.
> > 
> > If you rename, how would you guarantee that the existing application will
> > work as expected?  PA doesn't have the definition of "Headset Speaker Jack"
> > or such.
> > 
> > And, no, we have no option "fix PA".  Other way round: we are not allowed
> > to break the current PA (or any user-space) behavior in general.
> > 
> > > we will generate 2 event calls(one headphone, one mic) when Headset
> > > plug in/out, applications will receive these 2 events,  and they can
> > > do anything, e.g. decide to switch on/off speaker/headphone.
> > 
> > Won't this break any user-space stuff?
> > 
> > > BTW, I haven't implemented the generating of combo jack kctls' name
> > > yet, currently, they looked like below:
> > > numid=12,iface=CARD,name='Headset Jack'
> > > numid=13,iface=CARD,name='Headset Jack',index=1
> > > numid=14,iface=CARD,name='Headset Jack',index=2
> > >
> > > once we have come to agreement, we can modify it in
> > > snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset Jack
> > Speakers".
> > 
> > .... and how the existing user-space works without changing its code?
> > 
> > 
> > Keyon, the most important point at this moment is to keep the compatibility.
> > HD-audio is no new driver.  It's a driver that has been present over a decade
> > with (literally) thousands of variants.
> > Please keep this in mind, and reconsider whether your patch will retain the
> > compatibility, especially with PulseAudio.
> [Keyon] understood. Then we should follow the HD-audio style, So, what do 
> you suggest here? Should we create only one single Boolean kctls for headset,
> and report true when status in headphone bit it true? Then we need a tricky
> exception mapping here?
> 
> Sorry, I am a little confusing here, because Mark suggest to create multiple controls
> for multiple bits jack, and you also agreed with that.  :(

Just prepare two exceptions for SND_JACK_HEADSET and
SND_JACK_VIDEOUT.  These are already defined as single types in
sound/jack.h.  The code can't be so tricky if you write properly :)


Takashi

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Tanu Kaskinen]

On Fri, 2015-03-20 at 15:18 +0100, Takashi Iwai wrote:
> At Fri, 20 Mar 2015 13:49:24 +0000,
> Jie, Yang wrote:
> > 
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Friday, March 20, 2015 9:21 PM
> > > To: Jie, Yang
> > > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> > > Girdwood, Liam R; Liam Girdwood; Tanu Kaskinen
> > > (tanu.kaskinen@linux.intel.com)
> > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> > > input device
> > > 
> > > At Fri, 20 Mar 2015 12:50:33 +0000,
> > > Jie, Yang wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > Sent: Friday, March 20, 2015 8:27 PM
> > > > > To: Jie, Yang
> > > > > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> > > > > Girdwood, Liam R; Liam Girdwood
> > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for
> > > > > every jack input device
> > > > >
> > > > > At Fri, 20 Mar 2015 12:22:10 +0000,
> > > > > Jie, Yang wrote:
> > > > > >
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int snd_jack_new_kctl(struct snd_card *card, struct
> > > > > > > > +snd_jack *jack, int type) {
> > > > > > > > +	struct snd_kcontrol *kctl;
> > > > > > > > +	struct snd_jack_kctl *jack_kctl;
> > > > > > > > +	int i, err, index, state = 0 /* use 0 for default state ??
> > > > > > > > +*/;
> > > > > > > > +
> > > > > > > > +	INIT_LIST_HEAD(&jack->kctl_list);
> > > > > > > > +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> > > > > > > > +		int testbit = 1 << i;
> > > > > > > > +		if (type & testbit) {
> > > > > > >
> > > > > > > With this implementation, you'll get multiple boolean kctls for
> > > > > > > a headset.  I thought we agreed with creating a single boolean for
> > > headset?
> > > > > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g.
> > > > > headset.
> > > > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET  |
> > > > > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is
> > > > > > pressed, we will report to the 3rd kctl.
> > > > >
> > > > > Hm, I don't remember that I agreed with multiple kctls...
> > > > >
> > > > > The multiple kctls have a significant drawback (multiple event calls
> > > > > for a single
> > > > > headset) and its behavior is incompatible with the current code
> > > > > (both the name change and the behavior change).  That is, your patch
> > > > > will very likely break the existing applications.
> > > > [Keyon] I am not very clear with the existing applications that using
> > > > these kctl events(seems Android use input subsystem event? Which we
> > > > didn't Change here. If I understand correctly, Pulseaudio uses jack
> > > > switch controls, via the name, then we can use different name for
> > > > headphone and mic here.)
> > > 
> > > PA uses jack kctls.
> > > 
> > > If you rename, how would you guarantee that the existing application will
> > > work as expected?  PA doesn't have the definition of "Headset Speaker Jack"
> > > or such.
> > > 
> > > And, no, we have no option "fix PA".  Other way round: we are not allowed
> > > to break the current PA (or any user-space) behavior in general.
> > > 
> > > > we will generate 2 event calls(one headphone, one mic) when Headset
> > > > plug in/out, applications will receive these 2 events,  and they can
> > > > do anything, e.g. decide to switch on/off speaker/headphone.
> > > 
> > > Won't this break any user-space stuff?
> > > 
> > > > BTW, I haven't implemented the generating of combo jack kctls' name
> > > > yet, currently, they looked like below:
> > > > numid=12,iface=CARD,name='Headset Jack'
> > > > numid=13,iface=CARD,name='Headset Jack',index=1
> > > > numid=14,iface=CARD,name='Headset Jack',index=2
> > > >
> > > > once we have come to agreement, we can modify it in
> > > > snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset Jack
> > > Speakers".
> > > 
> > > .... and how the existing user-space works without changing its code?
> > > 
> > > 
> > > Keyon, the most important point at this moment is to keep the compatibility.
> > > HD-audio is no new driver.  It's a driver that has been present over a decade
> > > with (literally) thousands of variants.
> > > Please keep this in mind, and reconsider whether your patch will retain the
> > > compatibility, especially with PulseAudio.
> > [Keyon] understood. Then we should follow the HD-audio style, So, what do 
> > you suggest here? Should we create only one single Boolean kctls for headset,
> > and report true when status in headphone bit it true? Then we need a tricky
> > exception mapping here?
> > 
> > Sorry, I am a little confusing here, because Mark suggest to create multiple controls
> > for multiple bits jack, and you also agreed with that.  :(
> 
> Just prepare two exceptions for SND_JACK_HEADSET and
> SND_JACK_VIDEOUT.  These are already defined as single types in
> sound/jack.h.  The code can't be so tricky if you write properly :)

Can someone clarify what is the current plan? Will there be changes to
the control naming or behaviour for existing hardware?

PulseAudio's jack handling seems somewhat messy to me, so it may be
difficult to understand what kind of changes will break things and what
won't. I think these are the jack controls that PulseAudio currently
recognizes that are most important in this discussion:

 * Headset Mic Jack
 * Headphone Mic Jack
 * Mic Jack
 * Headphone Jack

"Headset Mic Jack" indicates the availability of a headset mic. In
PulseAudio it doesn't imply anything about the headset speaker
availability, even though logically, if there's a headset mic plugged
in, surely the headset speakers are available too.

"Headphone Mic Jack" indicates the availability of either headphones or
a microphone. There's no information about which of those is plugged in,
just that one of those devices is plugged in. This control is *not* for
headsets, according to the commit that added the support for that
control to PulseAudio[1].

"Mic Jack" indicates the availability of a standalone microphone. I
don't know if the kernel currently exposes both "Headset Mick Jack" and
"Mic Jack" if there's one physical jack that is able to distinguish
between the two device types, but I suppose it would be good to have
both controls in that case, so that applications know what kind of
device has been plugged in.

"Headphone Jack" indicates the availability of headphones. PulseAudio
doesn't currently have support for any kind of "Headset Speaker Jack"
control, so I guess the kernel currently uses "Headphone Jack" also with
physical jacks that support headsets.

One thing that is unclear for me is that how are those jacks represented
that support any of headsets/headphones/microphones, but don't provide
information about which device type has been plugged in. I know David
has made a UI for Ubuntu for selecting the device type once something
has been plugged in to such jack, but I don't remember how the UI can
know that the jack supports all of those three device types.

[1] http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=7369a53ab5f606e87a3cd1cd4eebd40226bab090

-- 
Tanu

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[David Henningsson]

On 2015-03-23 11:56, Tanu Kaskinen wrote:
> One thing that is unclear for me is that how are those jacks represented
> that support any of headsets/headphones/microphones, but don't provide
> information about which device type has been plugged in.

For headphone or headset, independent switches:

  * "Headphone Jack"
  * "Headset Mic Jack"

For headphone or headset, one hw switch only:

  * "Headphone Jack"
  * "Headset Mic Phantom Jack"

Headphone or mic, one hw switch:

  * "Headphone Mic Jack"

Headphone, headset, or mic, one hw switch only:

  * "Headphone Mic Jack"
  * "Headset Mic Phantom Jack"

Headphone, headset, or mic, one switch for hp/mic and the other for the 
headset mic:

  * "Headphone Mic Jack"
  * "Headset Mic Jack"

The first one is the most common one, but all of the other ones exist, 
especially on recent Dell machines.

> I know David
> has made a UI for Ubuntu for selecting the device type once something
> has been plugged in to such jack, but I don't remember how the UI can
> know that the jack supports all of those three device types.

For reference, the code is here: 
http://bazaar.launchpad.net/~unity-settings-daemon-team/unity-settings-daemon/trunk/files/head:/plugins/media-keys/what-did-you-plug-in/

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

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Takashi Iwai]

At Mon, 23 Mar 2015 12:51:07 +0100,
David Henningsson wrote:
> 
> 
> 
> On 2015-03-23 11:56, Tanu Kaskinen wrote:
> > One thing that is unclear for me is that how are those jacks represented
> > that support any of headsets/headphones/microphones, but don't provide
> > information about which device type has been plugged in.
> 
> For headphone or headset, independent switches:
> 
>   * "Headphone Jack"
>   * "Headset Mic Jack"
> 
> For headphone or headset, one hw switch only:
> 
>   * "Headphone Jack"
>   * "Headset Mic Phantom Jack"
> 
> Headphone or mic, one hw switch:
> 
>   * "Headphone Mic Jack"
> 
> Headphone, headset, or mic, one hw switch only:
> 
>   * "Headphone Mic Jack"
>   * "Headset Mic Phantom Jack"
> 
> Headphone, headset, or mic, one switch for hp/mic and the other for the 
> headset mic:
> 
>   * "Headphone Mic Jack"
>   * "Headset Mic Jack"
> 
> The first one is the most common one, but all of the other ones exist, 
> especially on recent Dell machines.

Are all these about a single headset/headphone jack?
If so, yeah, it's really messy.


Takashi

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 645 bytes --]

On Mon, Mar 23, 2015 at 12:51:07PM +0100, David Henningsson wrote:
> On 2015-03-23 11:56, Tanu Kaskinen wrote:

> >One thing that is unclear for me is that how are those jacks represented
> >that support any of headsets/headphones/microphones, but don't provide
> >information about which device type has been plugged in.

> For headphone or headset, independent switches:

>  * "Headphone Jack"
>  * "Headset Mic Jack"

> For headphone or headset, one hw switch only:

>  * "Headphone Jack"
>  * "Headset Mic Phantom Jack"

I'd have expected these to be the same thing, just with the second case
always having the same state for both switches.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[David Henningsson]

On 2015-03-23 17:41, Mark Brown wrote:
> On Mon, Mar 23, 2015 at 12:51:07PM +0100, David Henningsson wrote:
>> On 2015-03-23 11:56, Tanu Kaskinen wrote:
>
>>> One thing that is unclear for me is that how are those jacks represented
>>> that support any of headsets/headphones/microphones, but don't provide
>>> information about which device type has been plugged in.
>
>> For headphone or headset, independent switches:
>
>>   * "Headphone Jack"
>>   * "Headset Mic Jack"
>
>> For headphone or headset, one hw switch only:
>
>>   * "Headphone Jack"
>>   * "Headset Mic Phantom Jack"
>
> I'd have expected these to be the same thing, just with the second case
> always having the same state for both switches.

We need to distinguish the use cases: in case of independent switches, 
we can make the assumption that if the user plugged in a headset, the 
user wants to use the headset mic instead of the internal mic, so we 
switch to it.

In the other case, where we don't know if the user plugged in a 
headphone or a headset, we need to ask the user what (s)he plugged in, 
so we can determine whether to select headphone+internal mic or 
headphone+headset mic.

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

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1193 bytes --]

On Tue, Mar 24, 2015 at 07:50:39AM +0100, David Henningsson wrote:
> On 2015-03-23 17:41, Mark Brown wrote:

> >>For headphone or headset, one hw switch only:

> >>  * "Headphone Jack"
> >>  * "Headset Mic Phantom Jack"

> >I'd have expected these to be the same thing, just with the second case
> >always having the same state for both switches.

> We need to distinguish the use cases: in case of independent switches, we
> can make the assumption that if the user plugged in a headset, the user
> wants to use the headset mic instead of the internal mic, so we switch to
> it.

> In the other case, where we don't know if the user plugged in a headphone or
> a headset, we need to ask the user what (s)he plugged in, so we can
> determine whether to select headphone+internal mic or headphone+headset mic.

Hrm, I can see that.  However it feels wrong to have no state change at
all on the microphone part of the jack - I think I would have expected
this case to be flagged as a separate mode, say "Tied", so that simpler
applications have more chance to do the right thing.  It's a bit of a
separate issue though and it's a bit late now as we've already shipped
the ABI for the kcontrols.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Raymond Yau]

>
> > >>For headphone or headset, one hw switch only:
>
> > >>  * "Headphone Jack"
> > >>  * "Headset Mic Phantom Jack"
>
> > >I'd have expected these to be the same thing, just with the second case
> > >always having the same state for both switches.
>
> > We need to distinguish the use cases: in case of independent switches,
we
> > can make the assumption that if the user plugged in a headset, the user
> > wants to use the headset mic instead of the internal mic, so we switch
to
> > it.
>
> > In the other case, where we don't know if the user plugged in a
headphone or
> > a headset, we need to ask the user what (s)he plugged in, so we can
> > determine whether to select headphone+internal mic or headphone+headset
mic.::

Does the user maunal of those notebook explicitly state that the combo jack
support headset and headphone ( or mic) ?

if not, why do the driver need to support both headphone and headset ( or
mic ) especially when the icon near the jack is headset

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Raymond Yau]

>>
>>
>>>> One thing that is unclear for me is that how are those jacks
represented
>>>> that support any of headsets/headphones/microphones, but don't provide
>>>> information about which device type has been plugged in.
>>
>>
>>> For headphone or headset, independent switches:
>>
>>
>>>   * "Headphone Jack"
>>>   * "Headset Mic Jack"
>>
>>
>>> For headphone or headset, one hw switch only:
>>
>>
>>>   * "Headphone Jack"
>>>   * "Headset Mic Phantom Jack"
>>
>>
>> I'd have expected these to be the same thing, just with the second case
>> always having the same state for both switches.
>
>
> We need to distinguish the use cases: in case of independent switches, we
can make the assumption that if the user plugged in a headset, the user
wants to use the headset mic instead of the internal mic, so we switch to
it.
>

What will happen for those Dell Alienware 14, 15 and 17 which have three
jacks: headset, headphone and mic

http://www.dell.com/support/home/us/en/19/product-support/product/alienware-17/manuals

Alienware 17 Quick Start Guide

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie, Yang]

> -----Original Message-----
> From: Tanu Kaskinen [mailto:tanu.kaskinen@linux.intel.com]
> Sent: Monday, March 23, 2015 6:57 PM
> To: Takashi Iwai
> Cc: Jie, Yang; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-
> project.org; Girdwood, Liam R; Liam Girdwood; David Henningsson
> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> input device
> 
> On Fri, 2015-03-20 at 15:18 +0100, Takashi Iwai wrote:
> > At Fri, 20 Mar 2015 13:49:24 +0000,
> > Jie, Yang wrote:
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Friday, March 20, 2015 9:21 PM
> > > > To: Jie, Yang
> > > > Cc: perex@perex.cz; broonie@kernel.org;
> > > > alsa-devel@alsa-project.org; Girdwood, Liam R; Liam Girdwood; Tanu
> > > > Kaskinen
> > > > (tanu.kaskinen@linux.intel.com)
> > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for
> > > > every jack input device
> > > >
> > > > At Fri, 20 Mar 2015 12:50:33 +0000, Jie, Yang wrote:
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > Sent: Friday, March 20, 2015 8:27 PM
> > > > > > To: Jie, Yang
> > > > > > Cc: perex@perex.cz; broonie@kernel.org;
> > > > > > alsa-devel@alsa-project.org; Girdwood, Liam R; Liam Girdwood
> > > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols
> > > > > > for every jack input device
> > > > > >
> > > > > > At Fri, 20 Mar 2015 12:22:10 +0000, Jie, Yang wrote:
> > > > > > >
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static int snd_jack_new_kctl(struct snd_card *card,
> > > > > > > > > +struct snd_jack *jack, int type) {
> > > > > > > > > +	struct snd_kcontrol *kctl;
> > > > > > > > > +	struct snd_jack_kctl *jack_kctl;
> > > > > > > > > +	int i, err, index, state = 0 /* use 0 for default state ??
> > > > > > > > > +*/;
> > > > > > > > > +
> > > > > > > > > +	INIT_LIST_HEAD(&jack->kctl_list);
> > > > > > > > > +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> > > > > > > > > +		int testbit = 1 << i;
> > > > > > > > > +		if (type & testbit) {
> > > > > > > >
> > > > > > > > With this implementation, you'll get multiple boolean
> > > > > > > > kctls for a headset.  I thought we agreed with creating a
> > > > > > > > single boolean for
> > > > headset?
> > > > > > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g.
> > > > > > headset.
> > > > > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET  |
> > > > > > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when
> > > > > > > BTN_0 is pressed, we will report to the 3rd kctl.
> > > > > >
> > > > > > Hm, I don't remember that I agreed with multiple kctls...
> > > > > >
> > > > > > The multiple kctls have a significant drawback (multiple event
> > > > > > calls for a single
> > > > > > headset) and its behavior is incompatible with the current
> > > > > > code (both the name change and the behavior change).  That is,
> > > > > > your patch will very likely break the existing applications.
> > > > > [Keyon] I am not very clear with the existing applications that
> > > > > using these kctl events(seems Android use input subsystem event?
> > > > > Which we didn't Change here. If I understand correctly,
> > > > > Pulseaudio uses jack switch controls, via the name, then we can
> > > > > use different name for headphone and mic here.)
> > > >
> > > > PA uses jack kctls.
> > > >
> > > > If you rename, how would you guarantee that the existing
> > > > application will work as expected?  PA doesn't have the definition of
> "Headset Speaker Jack"
> > > > or such.
> > > >
> > > > And, no, we have no option "fix PA".  Other way round: we are not
> > > > allowed to break the current PA (or any user-space) behavior in general.
> > > >
> > > > > we will generate 2 event calls(one headphone, one mic) when
> > > > > Headset plug in/out, applications will receive these 2 events,
> > > > > and they can do anything, e.g. decide to switch on/off
> speaker/headphone.
> > > >
> > > > Won't this break any user-space stuff?
> > > >
> > > > > BTW, I haven't implemented the generating of combo jack kctls'
> > > > > name yet, currently, they looked like below:
> > > > > numid=12,iface=CARD,name='Headset Jack'
> > > > > numid=13,iface=CARD,name='Headset Jack',index=1
> > > > > numid=14,iface=CARD,name='Headset Jack',index=2
> > > > >
> > > > > once we have come to agreement, we can modify it in
> > > > > snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset Jack
> > > > Speakers".
> > > >
> > > > .... and how the existing user-space works without changing its code?
> > > >
> > > >
> > > > Keyon, the most important point at this moment is to keep the
> compatibility.
> > > > HD-audio is no new driver.  It's a driver that has been present
> > > > over a decade with (literally) thousands of variants.
> > > > Please keep this in mind, and reconsider whether your patch will
> > > > retain the compatibility, especially with PulseAudio.
> > > [Keyon] understood. Then we should follow the HD-audio style, So,
> > > what do you suggest here? Should we create only one single Boolean
> > > kctls for headset, and report true when status in headphone bit it
> > > true? Then we need a tricky exception mapping here?
> > >
> > > Sorry, I am a little confusing here, because Mark suggest to create
> > > multiple controls for multiple bits jack, and you also agreed with
> > > that.  :(
> >
> > Just prepare two exceptions for SND_JACK_HEADSET and
> SND_JACK_VIDEOUT.
> > These are already defined as single types in sound/jack.h.  The code
> > can't be so tricky if you write properly :)
> 
> Can someone clarify what is the current plan? Will there be changes to the
> control naming or behaviour for existing hardware?
[Keyon] Hi Tanu, currently we plan to create kctls/switches at jack creating
stage, and attached them to the jack.
So, we need to decide the naming of these kctls/switches and make sure
they will be understood by PA correctly. 
At the same time, we create jacks with snd_jack_types as parameter passed
in, so, we need decision about what kctls/switches do we need create, for
each separate or combo(bits) type jack:
enum snd_jack_types {
        SND_JACK_HEADPHONE      = 0x0001,
        SND_JACK_MICROPHONE     = 0x0002,
        SND_JACK_HEADSET        = SND_JACK_HEADPHONE | SND_JACK_MICROPHONE,
        SND_JACK_LINEOUT        = 0x0004,
        SND_JACK_MECHANICAL     = 0x0008, /* If detected separately */
        SND_JACK_VIDEOOUT       = 0x0010,
        SND_JACK_AVOUT          = SND_JACK_LINEOUT | SND_JACK_VIDEOOUT,
        SND_JACK_LINEIN         = 0x0020,

        /* Kept separate from switches to facilitate implementation */
        SND_JACK_BTN_0          = 0x4000,
        SND_JACK_BTN_1          = 0x2000,
        SND_JACK_BTN_2          = 0x1000,
        SND_JACK_BTN_3          = 0x0800,
        SND_JACK_BTN_4          = 0x0400,
        SND_JACK_BTN_5          = 0x0200,
};

So, 
1. we need these naming, what you summarized below, that's quite helpful.
2. I am a little concern if the exist snd_jack_types enum can indicate all diverse
existing hardware, e.g. " Headphone Mic Jack " as you mentioned below, can
we use any bits combination for this kind of jack?
SND_JACK_HEADPHONE | SND_JACK_MICROPHONE seems can't tell difference
With "Headset Mic Jack" + "Headphone Jack"?

> 
> PulseAudio's jack handling seems somewhat messy to me, so it may be
> difficult to understand what kind of changes will break things and what won't.
> I think these are the jack controls that PulseAudio currently recognizes that
> are most important in this discussion:
> 
>  * Headset Mic Jack
>  * Headphone Mic Jack
>  * Mic Jack
>  * Headphone Jack
> 
> "Headset Mic Jack" indicates the availability of a headset mic. In PulseAudio it
> doesn't imply anything about the headset speaker availability, even though
> logically, if there's a headset mic plugged in, surely the headset speakers are
> available too.
> 
> "Headphone Mic Jack" indicates the availability of either headphones or a
> microphone. There's no information about which of those is plugged in, just
> that one of those devices is plugged in. This control is *not* for headsets,
> according to the commit that added the support for that control to
> PulseAudio[1].
> 
> "Mic Jack" indicates the availability of a standalone microphone. I don't know
> if the kernel currently exposes both "Headset Mick Jack" and "Mic Jack" if
> there's one physical jack that is able to distinguish between the two device
> types, but I suppose it would be good to have both controls in that case, so
> that applications know what kind of device has been plugged in.
> 
> "Headphone Jack" indicates the availability of headphones. PulseAudio
> doesn't currently have support for any kind of "Headset Speaker Jack"
> control, so I guess the kernel currently uses "Headphone Jack" also with
> physical jacks that support headsets.
> 
> One thing that is unclear for me is that how are those jacks represented that
> support any of headsets/headphones/microphones, but don't provide
> information about which device type has been plugged in. I know David has
> made a UI for Ubuntu for selecting the device type once something has been
> plugged in to such jack, but I don't remember how the UI can know that the
> jack supports all of those three device types.
> 
> [1]
> http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=7369a53ab5
> f606e87a3cd1cd4eebd40226bab090
> 
> --
> Tanu

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Tanu Kaskinen]

On Wed, 2015-03-25 at 07:53 +0000, Jie, Yang wrote:
> > -----Original Message-----
> > From: Tanu Kaskinen [mailto:tanu.kaskinen@linux.intel.com]
> > Sent: Monday, March 23, 2015 6:57 PM
> > To: Takashi Iwai
> > Cc: Jie, Yang; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-
> > project.org; Girdwood, Liam R; Liam Girdwood; David Henningsson
> > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> > input device
> > 
> > On Fri, 2015-03-20 at 15:18 +0100, Takashi Iwai wrote:
> > > At Fri, 20 Mar 2015 13:49:24 +0000,
> > > Jie, Yang wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > Sent: Friday, March 20, 2015 9:21 PM
> > > > > To: Jie, Yang
> > > > > Cc: perex@perex.cz; broonie@kernel.org;
> > > > > alsa-devel@alsa-project.org; Girdwood, Liam R; Liam Girdwood; Tanu
> > > > > Kaskinen
> > > > > (tanu.kaskinen@linux.intel.com)
> > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for
> > > > > every jack input device
> > > > >
> > > > > At Fri, 20 Mar 2015 12:50:33 +0000, Jie, Yang wrote:
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > Sent: Friday, March 20, 2015 8:27 PM
> > > > > > > To: Jie, Yang
> > > > > > > Cc: perex@perex.cz; broonie@kernel.org;
> > > > > > > alsa-devel@alsa-project.org; Girdwood, Liam R; Liam Girdwood
> > > > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols
> > > > > > > for every jack input device
> > > > > > >
> > > > > > > At Fri, 20 Mar 2015 12:22:10 +0000, Jie, Yang wrote:
> > > > > > > >
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static int snd_jack_new_kctl(struct snd_card *card,
> > > > > > > > > > +struct snd_jack *jack, int type) {
> > > > > > > > > > +	struct snd_kcontrol *kctl;
> > > > > > > > > > +	struct snd_jack_kctl *jack_kctl;
> > > > > > > > > > +	int i, err, index, state = 0 /* use 0 for default state ??
> > > > > > > > > > +*/;
> > > > > > > > > > +
> > > > > > > > > > +	INIT_LIST_HEAD(&jack->kctl_list);
> > > > > > > > > > +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> > > > > > > > > > +		int testbit = 1 << i;
> > > > > > > > > > +		if (type & testbit) {
> > > > > > > > >
> > > > > > > > > With this implementation, you'll get multiple boolean
> > > > > > > > > kctls for a headset.  I thought we agreed with creating a
> > > > > > > > > single boolean for
> > > > > headset?
> > > > > > > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g.
> > > > > > > headset.
> > > > > > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET  |
> > > > > > > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when
> > > > > > > > BTN_0 is pressed, we will report to the 3rd kctl.
> > > > > > >
> > > > > > > Hm, I don't remember that I agreed with multiple kctls...
> > > > > > >
> > > > > > > The multiple kctls have a significant drawback (multiple event
> > > > > > > calls for a single
> > > > > > > headset) and its behavior is incompatible with the current
> > > > > > > code (both the name change and the behavior change).  That is,
> > > > > > > your patch will very likely break the existing applications.
> > > > > > [Keyon] I am not very clear with the existing applications that
> > > > > > using these kctl events(seems Android use input subsystem event?
> > > > > > Which we didn't Change here. If I understand correctly,
> > > > > > Pulseaudio uses jack switch controls, via the name, then we can
> > > > > > use different name for headphone and mic here.)
> > > > >
> > > > > PA uses jack kctls.
> > > > >
> > > > > If you rename, how would you guarantee that the existing
> > > > > application will work as expected?  PA doesn't have the definition of
> > "Headset Speaker Jack"
> > > > > or such.
> > > > >
> > > > > And, no, we have no option "fix PA".  Other way round: we are not
> > > > > allowed to break the current PA (or any user-space) behavior in general.
> > > > >
> > > > > > we will generate 2 event calls(one headphone, one mic) when
> > > > > > Headset plug in/out, applications will receive these 2 events,
> > > > > > and they can do anything, e.g. decide to switch on/off
> > speaker/headphone.
> > > > >
> > > > > Won't this break any user-space stuff?
> > > > >
> > > > > > BTW, I haven't implemented the generating of combo jack kctls'
> > > > > > name yet, currently, they looked like below:
> > > > > > numid=12,iface=CARD,name='Headset Jack'
> > > > > > numid=13,iface=CARD,name='Headset Jack',index=1
> > > > > > numid=14,iface=CARD,name='Headset Jack',index=2
> > > > > >
> > > > > > once we have come to agreement, we can modify it in
> > > > > > snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset Jack
> > > > > Speakers".
> > > > >
> > > > > .... and how the existing user-space works without changing its code?
> > > > >
> > > > >
> > > > > Keyon, the most important point at this moment is to keep the
> > compatibility.
> > > > > HD-audio is no new driver.  It's a driver that has been present
> > > > > over a decade with (literally) thousands of variants.
> > > > > Please keep this in mind, and reconsider whether your patch will
> > > > > retain the compatibility, especially with PulseAudio.
> > > > [Keyon] understood. Then we should follow the HD-audio style, So,
> > > > what do you suggest here? Should we create only one single Boolean
> > > > kctls for headset, and report true when status in headphone bit it
> > > > true? Then we need a tricky exception mapping here?
> > > >
> > > > Sorry, I am a little confusing here, because Mark suggest to create
> > > > multiple controls for multiple bits jack, and you also agreed with
> > > > that.  :(
> > >
> > > Just prepare two exceptions for SND_JACK_HEADSET and
> > SND_JACK_VIDEOUT.
> > > These are already defined as single types in sound/jack.h.  The code
> > > can't be so tricky if you write properly :)
> > 
> > Can someone clarify what is the current plan? Will there be changes to the
> > control naming or behaviour for existing hardware?
> [Keyon] Hi Tanu, currently we plan to create kctls/switches at jack creating
> stage, and attached them to the jack.
> So, we need to decide the naming of these kctls/switches and make sure
> they will be understood by PA correctly. 

So the plan is to avoid any changes to what kctls applications see?
That's very good.

> At the same time, we create jacks with snd_jack_types as parameter passed
> in, so, we need decision about what kctls/switches do we need create, for
> each separate or combo(bits) type jack:
> enum snd_jack_types {
>         SND_JACK_HEADPHONE      = 0x0001,
>         SND_JACK_MICROPHONE     = 0x0002,
>         SND_JACK_HEADSET        = SND_JACK_HEADPHONE | SND_JACK_MICROPHONE,
>         SND_JACK_LINEOUT        = 0x0004,
>         SND_JACK_MECHANICAL     = 0x0008, /* If detected separately */
>         SND_JACK_VIDEOOUT       = 0x0010,
>         SND_JACK_AVOUT          = SND_JACK_LINEOUT | SND_JACK_VIDEOOUT,
>         SND_JACK_LINEIN         = 0x0020,
> 
>         /* Kept separate from switches to facilitate implementation */
>         SND_JACK_BTN_0          = 0x4000,
>         SND_JACK_BTN_1          = 0x2000,
>         SND_JACK_BTN_2          = 0x1000,
>         SND_JACK_BTN_3          = 0x0800,
>         SND_JACK_BTN_4          = 0x0400,
>         SND_JACK_BTN_5          = 0x0200,
> };
> 
> So, 
> 1. we need these naming, what you summarized below, that's quite helpful.

David's summary is even more helpful!

> 2. I am a little concern if the exist snd_jack_types enum can indicate all diverse
> existing hardware, e.g. " Headphone Mic Jack " as you mentioned below, can
> we use any bits combination for this kind of jack?
> SND_JACK_HEADPHONE | SND_JACK_MICROPHONE seems can't tell difference
> With "Headset Mic Jack" + "Headphone Jack"?

Yes, it seems the existing enumeration isn't able to do that
distinction. Can the enumeration be extended?

Another thing that might be a problem is that this jack enumeration
doesn't make distinction between what device types can be used with the
jack, and what level of device type detection is available. That is, is
a headset jack only able to tell that "something was plugged in", or is
it also able to tell whether that something was headphones, a microphone
or a headset. (I suppose this might essentially be the same topic as
discussed in the "ALSA: jack: Refactoring for jack kctls" thread about
phantom jacks.)

-- 
Tanu

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie, Yang]

Thank you for comprehensive summarizing, David.
Can you help give more description as below?

~Keyon

> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Monday, March 23, 2015 7:51 PM
> To: Tanu Kaskinen; Takashi Iwai
> Cc: Jie, Yang; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-
> project.org; Girdwood, Liam R; Liam Girdwood
> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> input device
> 
> 
> 
> On 2015-03-23 11:56, Tanu Kaskinen wrote:
> > One thing that is unclear for me is that how are those jacks
> > represented that support any of headsets/headphones/microphones, but
> > don't provide information about which device type has been plugged in.
> 
> For headphone or headset, independent switches:
> 
>   * "Headphone Jack"
>   * "Headset Mic Jack"
[Keyon] here for the most common headset jack
(SND_JACK_HEADPHONE | SND_JACK_MICROPHONE), we should create
two independent kctls/switches--"Headphone Jack" + "Headset Mic Jack",
right?

so, is it possible that there is only "Mic Jack" in this case? I mean that
only input(no output, no physical headphone/speaker jack) jack exist.

If yes, then we may need change "Headset Mic Jack" to "Mic Jack"?

> 
> For headphone or headset, one hw switch only:
[Keyon] I am sorry I am not familiar with the jack HW circuit. What "one 
hw switch only" here means? Does it means that -- for headset Jack, the 
status(connected/disconnected) of HP pin is always exactly same with 
that of Mic pin?
> 
>   * "Headphone Jack"
>   * "Headset Mic Phantom Jack"
[Keyon] for Headset of this type, do we need create only "Headset Mic 
Phantom Jack" kctl, or "Headphone Jack" kctl is also needed?
> 
> Headphone or mic, one hw switch:
[Keyon]I am fresh about this kind of hw jack, it should use different 
segment of the plug, seems we don't need check the actual connected 
status at the jack creation stage, just creating "Headphone Mic Jack"
should works, right?
> 
>   * "Headphone Mic Jack"
> 
> Headphone, headset, or mic, one hw switch only:
[Keyon] how many kctls should we create for this?
> 
>   * "Headphone Mic Jack"
>   * "Headset Mic Phantom Jack"
> 
> Headphone, headset, or mic, one switch for hp/mic and the other for the
> headset mic:
[Keyon] I can't imagine how this works, it's too messy for me. :(
> 
>   * "Headphone Mic Jack"
>   * "Headset Mic Jack"
> 
> The first one is the most common one, but all of the other ones exist,
> especially on recent Dell machines.
> 
> > I know David
> > has made a UI for Ubuntu for selecting the device type once something
> > has been plugged in to such jack, but I don't remember how the UI can
> > know that the jack supports all of those three device types.
> 
> For reference, the code is here:
> http://bazaar.launchpad.net/~unity-settings-daemon-team/unity-settings-
> daemon/trunk/files/head:/plugins/media-keys/what-did-you-plug-in/
> 
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie, Yang]

> -----Original Message-----
> From: Tanu Kaskinen [mailto:tanu.kaskinen@linux.intel.com]
> Sent: Wednesday, March 25, 2015 5:28 PM
> To: Jie, Yang
> Cc: Takashi Iwai; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-
> project.org; Girdwood, Liam R; Liam Girdwood; David Henningsson
> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> input device
> 
> On Wed, 2015-03-25 at 07:53 +0000, Jie, Yang wrote:
> > > -----Original Message-----
> > > From: Tanu Kaskinen [mailto:tanu.kaskinen@linux.intel.com]
> > > Sent: Monday, March 23, 2015 6:57 PM
> > > To: Takashi Iwai
> > > Cc: Jie, Yang; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-
> > > project.org; Girdwood, Liam R; Liam Girdwood; David Henningsson
> > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for
> > > every jack input device
> > >
> > > On Fri, 2015-03-20 at 15:18 +0100, Takashi Iwai wrote:
> > > > At Fri, 20 Mar 2015 13:49:24 +0000, Jie, Yang wrote:
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > Sent: Friday, March 20, 2015 9:21 PM
> > > > > > To: Jie, Yang
> > > > > > Cc: perex@perex.cz; broonie@kernel.org;
> > > > > > alsa-devel@alsa-project.org; Girdwood, Liam R; Liam Girdwood;
> > > > > > Tanu Kaskinen
> > > > > > (tanu.kaskinen@linux.intel.com)
> > > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols
> > > > > > for every jack input device
> > > > > >
> > > > > > At Fri, 20 Mar 2015 12:50:33 +0000, Jie, Yang wrote:
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > > Sent: Friday, March 20, 2015 8:27 PM
> > > > > > > > To: Jie, Yang
> > > > > > > > Cc: perex@perex.cz; broonie@kernel.org;
> > > > > > > > alsa-devel@alsa-project.org; Girdwood, Liam R; Liam
> > > > > > > > Girdwood
> > > > > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack
> > > > > > > > kcontrols for every jack input device
> > > > > > > >
> > > > > > > > At Fri, 20 Mar 2015 12:22:10 +0000, Jie, Yang wrote:
> > > > > > > > >
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +static int snd_jack_new_kctl(struct snd_card *card,
> > > > > > > > > > > +struct snd_jack *jack, int type) {
> > > > > > > > > > > +	struct snd_kcontrol *kctl;
> > > > > > > > > > > +	struct snd_jack_kctl *jack_kctl;
> > > > > > > > > > > +	int i, err, index, state = 0 /* use 0 for default state ??
> > > > > > > > > > > +*/;
> > > > > > > > > > > +
> > > > > > > > > > > +	INIT_LIST_HEAD(&jack->kctl_list);
> > > > > > > > > > > +	for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> > > > > > > > > > > +		int testbit = 1 << i;
> > > > > > > > > > > +		if (type & testbit) {
> > > > > > > > > >
> > > > > > > > > > With this implementation, you'll get multiple boolean
> > > > > > > > > > kctls for a headset.  I thought we agreed with
> > > > > > > > > > creating a single boolean for
> > > > > > headset?
> > > > > > > > > [Keyon] We agreed with creating multiple kctls for combo jack,
> e.g.
> > > > > > > > headset.
> > > > > > > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET
> > > > > > > > > | SND_JACK_BTN_0, we will create 3 kctls for the jack,
> > > > > > > > > when
> > > > > > > > > BTN_0 is pressed, we will report to the 3rd kctl.
> > > > > > > >
> > > > > > > > Hm, I don't remember that I agreed with multiple kctls...
> > > > > > > >
> > > > > > > > The multiple kctls have a significant drawback (multiple
> > > > > > > > event calls for a single
> > > > > > > > headset) and its behavior is incompatible with the current
> > > > > > > > code (both the name change and the behavior change).  That
> > > > > > > > is, your patch will very likely break the existing applications.
> > > > > > > [Keyon] I am not very clear with the existing applications
> > > > > > > that using these kctl events(seems Android use input subsystem
> event?
> > > > > > > Which we didn't Change here. If I understand correctly,
> > > > > > > Pulseaudio uses jack switch controls, via the name, then we
> > > > > > > can use different name for headphone and mic here.)
> > > > > >
> > > > > > PA uses jack kctls.
> > > > > >
> > > > > > If you rename, how would you guarantee that the existing
> > > > > > application will work as expected?  PA doesn't have the
> > > > > > definition of
> > > "Headset Speaker Jack"
> > > > > > or such.
> > > > > >
> > > > > > And, no, we have no option "fix PA".  Other way round: we are
> > > > > > not allowed to break the current PA (or any user-space) behavior in
> general.
> > > > > >
> > > > > > > we will generate 2 event calls(one headphone, one mic) when
> > > > > > > Headset plug in/out, applications will receive these 2
> > > > > > > events, and they can do anything, e.g. decide to switch
> > > > > > > on/off
> > > speaker/headphone.
> > > > > >
> > > > > > Won't this break any user-space stuff?
> > > > > >
> > > > > > > BTW, I haven't implemented the generating of combo jack kctls'
> > > > > > > name yet, currently, they looked like below:
> > > > > > > numid=12,iface=CARD,name='Headset Jack'
> > > > > > > numid=13,iface=CARD,name='Headset Jack',index=1
> > > > > > > numid=14,iface=CARD,name='Headset Jack',index=2
> > > > > > >
> > > > > > > once we have come to agreement, we can modify it in
> > > > > > > snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset
> > > > > > > Jack
> > > > > > Speakers".
> > > > > >
> > > > > > .... and how the existing user-space works without changing its code?
> > > > > >
> > > > > >
> > > > > > Keyon, the most important point at this moment is to keep the
> > > compatibility.
> > > > > > HD-audio is no new driver.  It's a driver that has been
> > > > > > present over a decade with (literally) thousands of variants.
> > > > > > Please keep this in mind, and reconsider whether your patch
> > > > > > will retain the compatibility, especially with PulseAudio.
> > > > > [Keyon] understood. Then we should follow the HD-audio style,
> > > > > So, what do you suggest here? Should we create only one single
> > > > > Boolean kctls for headset, and report true when status in
> > > > > headphone bit it true? Then we need a tricky exception mapping here?
> > > > >
> > > > > Sorry, I am a little confusing here, because Mark suggest to
> > > > > create multiple controls for multiple bits jack, and you also
> > > > > agreed with that.  :(
> > > >
> > > > Just prepare two exceptions for SND_JACK_HEADSET and
> > > SND_JACK_VIDEOUT.
> > > > These are already defined as single types in sound/jack.h.  The
> > > > code can't be so tricky if you write properly :)
> > >
> > > Can someone clarify what is the current plan? Will there be changes
> > > to the control naming or behaviour for existing hardware?
> > [Keyon] Hi Tanu, currently we plan to create kctls/switches at jack
> > creating stage, and attached them to the jack.
> > So, we need to decide the naming of these kctls/switches and make sure
> > they will be understood by PA correctly.
> 
> So the plan is to avoid any changes to what kctls applications see?
> That's very good.
> 
> > At the same time, we create jacks with snd_jack_types as parameter
> > passed in, so, we need decision about what kctls/switches do we need
> > create, for each separate or combo(bits) type jack:
> > enum snd_jack_types {
> >         SND_JACK_HEADPHONE      = 0x0001,
> >         SND_JACK_MICROPHONE     = 0x0002,
> >         SND_JACK_HEADSET        = SND_JACK_HEADPHONE |
> SND_JACK_MICROPHONE,
> >         SND_JACK_LINEOUT        = 0x0004,
> >         SND_JACK_MECHANICAL     = 0x0008, /* If detected separately */
> >         SND_JACK_VIDEOOUT       = 0x0010,
> >         SND_JACK_AVOUT          = SND_JACK_LINEOUT | SND_JACK_VIDEOOUT,
> >         SND_JACK_LINEIN         = 0x0020,
> >
> >         /* Kept separate from switches to facilitate implementation */
> >         SND_JACK_BTN_0          = 0x4000,
> >         SND_JACK_BTN_1          = 0x2000,
> >         SND_JACK_BTN_2          = 0x1000,
> >         SND_JACK_BTN_3          = 0x0800,
> >         SND_JACK_BTN_4          = 0x0400,
> >         SND_JACK_BTN_5          = 0x0200,
> > };
> >
> > So,
> > 1. we need these naming, what you summarized below, that's quite helpful.
> 
> David's summary is even more helpful!
> 
> > 2. I am a little concern if the exist snd_jack_types enum can indicate
> > all diverse existing hardware, e.g. " Headphone Mic Jack " as you
> > mentioned below, can we use any bits combination for this kind of jack?
> > SND_JACK_HEADPHONE | SND_JACK_MICROPHONE seems can't tell
> difference
> > With "Headset Mic Jack" + "Headphone Jack"?
> 
> Yes, it seems the existing enumeration isn't able to do that distinction. Can
> the enumeration be extended?
[Keyon] I think we can extend it if needed.
> 
> Another thing that might be a problem is that this jack enumeration doesn't
> make distinction between what device types can be used with the jack, and
> what level of device type detection is available. That is, is a headset jack only
[Keyon] our goal for this patch set should be achieve this, IMO.
> able to tell that "something was plugged in", or is it also able to tell whether
> that something was headphones, a microphone or a headset. (I suppose this
> might essentially be the same topic as discussed in the "ALSA: jack:
> Refactoring for jack kctls" thread about phantom jacks.)
[Keyon] yes, we creating kctls and adding them to the kctl list which belong to
the jack, hopefully, it may help giving out these information.
> 
> --
> Tanu

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Raymond Yau]

> > Headphone or mic, one hw switch:
> [Keyon]I am fresh about this kind of hw jack, it should use different
> segment of the plug, seems we don't need check the actual connected
> status at the jack creation stage, just creating "Headphone Mic Jack"
> should works, right?
> >
> >   * "Headphone Mic Jack"
> >
>

https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/sound/pci/hda?id=5780b627e24113323427c102175296ae43dfb9d7

http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=7369a53ab5f606e87a3cd1cd4eebd40226bab090

http://www.asus.com/Notebooks_Ultrabooks/Eee_PC_1015PX/specifications/

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Raymond Yau]

>>
>> One thing that is unclear for me is that how are those jacks represented
>> that support any of headsets/headphones/microphones, but don't provide
>> information about which device type has been plugged in.
>
>
> For headphone or headset, independent switches:
>
>  * "Headphone Jack"
>  * "Headset Mic Jack"
>
> For headphone or headset, one hw switch only:
>
>  * "Headphone Jack"
>  * "Headset Mic Phantom Jack"

https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/log/sound/pci/hda?qt=grep&q=dell-headset-multi

I don't understand how can Dell OptiPlex  3030 All-in-One and XPS 15
laoptop can use the same pin config "dell-headset-multi" ?

There is line out jack in aio spec : Universal Headset (Side)1
Line-out(Rear)

While xps 15 notebook has internal speaker and headset

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[David Henningsson]

On 2015-03-25 14:53, Jie, Yang wrote:
>
> Thank you for comprehensive summarizing, David.
> Can you help give more description as below?
>
> ~Keyon
>
>> -----Original Message-----
>> From: David Henningsson [mailto:david.henningsson@canonical.com]
>> Sent: Monday, March 23, 2015 7:51 PM
>> To: Tanu Kaskinen; Takashi Iwai
>> Cc: Jie, Yang; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-
>> project.org; Girdwood, Liam R; Liam Girdwood
>> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
>> input device
>>
>>
>>
>> On 2015-03-23 11:56, Tanu Kaskinen wrote:
>>> One thing that is unclear for me is that how are those jacks
>>> represented that support any of headsets/headphones/microphones, but
>>> don't provide information about which device type has been plugged in.
>>
>> For headphone or headset, independent switches:
>>
>>    * "Headphone Jack"
>>    * "Headset Mic Jack"
> [Keyon] here for the most common headset jack
> (SND_JACK_HEADPHONE | SND_JACK_MICROPHONE), we should create
> two independent kctls/switches--"Headphone Jack" + "Headset Mic Jack",
> right?
>
> so, is it possible that there is only "Mic Jack" in this case? I mean that
> only input(no output, no physical headphone/speaker jack) jack exist.
>
> If yes, then we may need change "Headset Mic Jack" to "Mic Jack"?

I'm not sure I understand your question - of course there are input only 
jacks, but they would then be SND_JACK_MICROPHONE only, and also 
labelled "Mic Jack". But that's not a headset jack, that's a microphone 
jack.

>> For headphone or headset, one hw switch only:
> [Keyon] I am sorry I am not familiar with the jack HW circuit. What "one
> hw switch only" here means? Does it means that -- for headset Jack, the
> status(connected/disconnected) of HP pin is always exactly same with
> that of Mic pin?

There is only one jack detection input from HW. Regardless of whether 
you plug in headphone or a headset, the HW switch would detect "plugged 
in". When the jack is unplugged, the HW switch would detect "unplugged".

The HW cannot tell us whether you plugged in a headphone or headset.

>>    * "Headphone Jack"
>>    * "Headset Mic Phantom Jack"
> [Keyon] for Headset of this type, do we need create only "Headset Mic
> Phantom Jack" kctl, or "Headphone Jack" kctl is also needed?

We need both kctls.

>> Headphone or mic, one hw switch:
> [Keyon]I am fresh about this kind of hw jack, it should use different
> segment of the plug, seems we don't need check the actual connected
> status at the jack creation stage, just creating "Headphone Mic Jack"
> should works, right?

It is not very common. It was used on some Asus netbooks a while ago. 
But yes, we should just create a "Headphone Mic Jack".

>>
>>    * "Headphone Mic Jack"
>>
>> Headphone, headset, or mic, one hw switch only:
> [Keyon] how many kctls should we create for this?
>>
>>    * "Headphone Mic Jack"
>>    * "Headset Mic Phantom Jack"

Same as today, two: "Headphone Mic Jack" and "Headset Mic Phantom Jack".

>> Headphone, headset, or mic, one switch for hp/mic and the other for the
>> headset mic:
> [Keyon] I can't imagine how this works, it's too messy for me. :(
>>
>>    * "Headphone Mic Jack"
>>    * "Headset Mic Jack"

Nothing plugged in:
   "Headphone Mic Jack" = false, "Headset Mic Jack" = false
Headphones plugged in:
   "Headphone Mic Jack" = true, "Headset Mic Jack" = false
Headset plugged in:
   "Headphone Mic Jack" = true, "Headset Mic Jack" = true
Mic plugged in:
   "Headphone Mic Jack" = true, "Headset Mic Jack" = false

As you can see, "Headphones" and "Mic" results in the same output, hence 
userspace needs to ask the user if (s)he plugged in a headphone or mic.

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

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie, Yang]

Thank you so much, David!

So, I summarized as below table:

Jack Type					kctls/switches name					detection																description

Headphone					Headphone Jack					HP plugged in/unplugged	

Mic						Mic Jack						Mic plugged in/unplugged	

Headset					Headphone Jack					Nothing plugged in:   ""Headphone Jack"" = false, ""Headset Mic Jack"" = false
						Headset Mic Jack"					Headphones plugged in:   ""Headphone Jack"" = true, ""Headset Mic Jack"" = false 
													Headset plugged in:   ""Headphone Jack"" = true, ""Headset Mic Jack"" = true
													Mic plugged in:   ""Headphone Jack"" = true ""Headset Mic Jack"" = false(should not plugged in Mic, detect wrongly)"		independent switches

Headset Mic					Headphone Jack					Nothing plugged in:   ""Headphone Jack"" = false, ""Headset Mic Phantom Jack"" = false
						Headset Mic Phantom Jack				Headphones plugged in:   ""Headphone Jack"" = true, ""Headset Mic Phantom Jack"" = false? 
													Headset plugged in:   ""Headphone Jack"" = false, ""Headset Mic Phantom Jack"" = true ?
													Mic plugged in:   ""Headphone Jack"" = true, ""Headset Mic Phantom Jack"" = false(should not plugged in Mic, detect wrongly)"    one hw switch only

Headphone Mic				Headphone Mic Jack					Nothing plugged in:   ""Headphone Mic Jack"" = false
													Headphones plugged in:   ""Headphone Mic Jack"" = true 
													Mic plugged in:   ""Headphone Mic Jack"" = true
													Headset plugged in:   ""Headphone Mic Jack"" = true?"											one hw switch

Headset Headphone Mic			Headphone Mic Jack					Nothing plugged in:   ""Headphone Mic Jack"" = false, ""Headset Mic Phantom Jack"" = false
						Headset Mic Phantom Jack				Headphones plugged in:   ""Headphone Mic Jack"" = true, ""Headset Mic Phantom Jack"" = false 
													Headset plugged in:   ""Headphone Mic Jack"" = false, ""Headset Mic Phantom Jack"" = true ?
													Mic plugged in:   ""Headphone Mic Jack"" = true, ""Headset Mic Phantom Jack"" = false"					one hw switch only

Headphone Mic Headset			Headphone Mic Jack					Nothing plugged in:   ""Headphone Mic Jack"" = false, ""Headset Mic Jack"" = false
						Headset Mic Jack					Headphones plugged in:   ""Headphone Mic Jack"" = true, ""Headset Mic Jack"" = false 
													Headset plugged in:   ""Headphone Mic Jack"" = true, ""Headset Mic Jack"" = true 
													Mic plugged in:   ""Headphone Mic Jack"" = true, ""Headset Mic Jack"" = false
																									one switch for hp/mic and the other for the headset mic

so, we may need extend snd_jack_types enum, by adding types:  Headset Mic, Headphone Mic, Headset Headphone Mic, Headphone Mic Headset.

enum snd_jack_types {
	SND_JACK_HEADPHONE	= 0x0001,
	SND_JACK_MICROPHONE	= 0x0002,
	SND_JACK_HEADSET	= SND_JACK_HEADPHONE | SND_JACK_MICROPHONE,
	SND_JACK_LINEOUT	= 0x0004,
	SND_JACK_MECHANICAL	= 0x0008, /* If detected separately */
	SND_JACK_VIDEOOUT	= 0x0010,
	SND_JACK_AVOUT		= SND_JACK_LINEOUT | SND_JACK_VIDEOOUT,
	SND_JACK_LINEIN		= 0x0020,

	SND_JACK_HEADSET_MIC	= 0x0040, /* one hw switch only */
	SND_JACK_HEADPHONE_MIC	= 0x0080, /* one hw switch only, headphone, or mic */
	SND_JACK_HEADSET_ HEADPHONE_MIC	= 0x0100, /* one hw switch only,  headset, headphone, or mic*/
	SND_JACK_ HEADPHONE_HEADSET_ MIC	= 0x0200, /* headset, headphone, or mic; one switch for hp/mic and the other for the headset mic  */

	/* Kept separate from switches to facilitate implementation */
	SND_JACK_BTN_0		= 0x8000,
	SND_JACK_BTN_1		= 0x4000,
	SND_JACK_BTN_2		= 0x2000,
	SND_JACK_BTN_3		= 0x1000,
	SND_JACK_BTN_4		= 0x0800,
	SND_JACK_BTN_5		= 0x0400,
};

And then, we may create corresponding kctls during jack creating.

Any comment?

~Keyon


> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Thursday, March 26, 2015 2:42 PM
> To: Jie, Yang; Tanu Kaskinen; Takashi Iwai
> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> Girdwood, Liam R; Liam Girdwood
> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> input device
> 
> 
> 
> On 2015-03-25 14:53, Jie, Yang wrote:
> >
> > Thank you for comprehensive summarizing, David.
> > Can you help give more description as below?
> >
> > ~Keyon
> >
> >> -----Original Message-----
> >> From: David Henningsson [mailto:david.henningsson@canonical.com]
> >> Sent: Monday, March 23, 2015 7:51 PM
> >> To: Tanu Kaskinen; Takashi Iwai
> >> Cc: Jie, Yang; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-
> >> project.org; Girdwood, Liam R; Liam Girdwood
> >> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for
> >> every jack input device
> >>
> >>
> >>
> >> On 2015-03-23 11:56, Tanu Kaskinen wrote:
> >>> One thing that is unclear for me is that how are those jacks
> >>> represented that support any of headsets/headphones/microphones,
> but
> >>> don't provide information about which device type has been plugged in.
> >>
> >> For headphone or headset, independent switches:
> >>
> >>    * "Headphone Jack"
> >>    * "Headset Mic Jack"
> > [Keyon] here for the most common headset jack (SND_JACK_HEADPHONE
> |
> > SND_JACK_MICROPHONE), we should create two independent
> > kctls/switches--"Headphone Jack" + "Headset Mic Jack", right?
> >
> > so, is it possible that there is only "Mic Jack" in this case? I mean
> > that only input(no output, no physical headphone/speaker jack) jack exist.
> >
> > If yes, then we may need change "Headset Mic Jack" to "Mic Jack"?
> 
> I'm not sure I understand your question - of course there are input only jacks,
> but they would then be SND_JACK_MICROPHONE only, and also labelled
> "Mic Jack". But that's not a headset jack, that's a microphone jack.
> 
> >> For headphone or headset, one hw switch only:
> > [Keyon] I am sorry I am not familiar with the jack HW circuit. What
> > "one hw switch only" here means? Does it means that -- for headset
> > Jack, the
> > status(connected/disconnected) of HP pin is always exactly same with
> > that of Mic pin?
> 
> There is only one jack detection input from HW. Regardless of whether you
> plug in headphone or a headset, the HW switch would detect "plugged in".
> When the jack is unplugged, the HW switch would detect "unplugged".
> 
> The HW cannot tell us whether you plugged in a headphone or headset.
> 
> >>    * "Headphone Jack"
> >>    * "Headset Mic Phantom Jack"
> > [Keyon] for Headset of this type, do we need create only "Headset Mic
> > Phantom Jack" kctl, or "Headphone Jack" kctl is also needed?
> 
> We need both kctls.
> 
> >> Headphone or mic, one hw switch:
> > [Keyon]I am fresh about this kind of hw jack, it should use different
> > segment of the plug, seems we don't need check the actual connected
> > status at the jack creation stage, just creating "Headphone Mic Jack"
> > should works, right?
> 
> It is not very common. It was used on some Asus netbooks a while ago.
> But yes, we should just create a "Headphone Mic Jack".
> 
> >>
> >>    * "Headphone Mic Jack"
> >>
> >> Headphone, headset, or mic, one hw switch only:
> > [Keyon] how many kctls should we create for this?
> >>
> >>    * "Headphone Mic Jack"
> >>    * "Headset Mic Phantom Jack"
> 
> Same as today, two: "Headphone Mic Jack" and "Headset Mic Phantom Jack".
> 
> >> Headphone, headset, or mic, one switch for hp/mic and the other for
> >> the headset mic:
> > [Keyon] I can't imagine how this works, it's too messy for me. :(
> >>
> >>    * "Headphone Mic Jack"
> >>    * "Headset Mic Jack"
> 
> Nothing plugged in:
>    "Headphone Mic Jack" = false, "Headset Mic Jack" = false Headphones
> plugged in:
>    "Headphone Mic Jack" = true, "Headset Mic Jack" = false Headset plugged
> in:
>    "Headphone Mic Jack" = true, "Headset Mic Jack" = true Mic plugged in:
>    "Headphone Mic Jack" = true, "Headset Mic Jack" = false
> 
> As you can see, "Headphones" and "Mic" results in the same output, hence
> userspace needs to ask the user if (s)he plugged in a headphone or mic.
> 
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[David Henningsson]

On 2015-03-26 09:29, Jie, Yang wrote:
>
> Thank you so much, David!
>
> So, I summarized as below table:

I've added how I see them with the new API, see below for further comments:

>
> Jack Type					kctls/switches name					detection																description
>
> Headphone					Headphone Jack					HP plugged in/unplugged	

SND_JACK_HEADPHONE

>
> Mic						Mic Jack						Mic plugged in/unplugged	

SND_JACK_MICROPHONE

>
> Headset					Headphone Jack					Nothing plugged in:   ""Headphone Jack"" = false, ""Headset Mic Jack"" = false
> 						Headset Mic Jack"					Headphones plugged in:   ""Headphone Jack"" = true, ""Headset Mic Jack"" = false
> 													Headset plugged in:   ""Headphone Jack"" = true, ""Headset Mic Jack"" = true
> 													Mic plugged in:   ""Headphone Jack"" = true ""Headset Mic Jack"" = false(should not plugged in Mic, detect wrongly)"		independent switches

SND_JACK_HEADSET

"Mic plugged in" case is irrelevant - not supported by hw

>
> Headset Mic					Headphone Jack					Nothing plugged in:   ""Headphone Jack"" = false, ""Headset Mic Phantom Jack"" = false
> 						Headset Mic Phantom Jack				Headphones plugged in:   ""Headphone Jack"" = true, ""Headset Mic Phantom Jack"" = false?
> 													Headset plugged in:   ""Headphone Jack"" = false, ""Headset Mic Phantom Jack"" = true ?
> 													Mic plugged in:   ""Headphone Jack"" = true, ""Headset Mic Phantom Jack"" = false(should not plugged in Mic, detect wrongly)"    one hw switch only

SND_JACK_HEADPHONE, the "Headset Mic Phantom Jack" is created manually

> Headphone Mic				Headphone Mic Jack					Nothing plugged in:   ""Headphone Mic Jack"" = false
> 													Headphones plugged in:   ""Headphone Mic Jack"" = true
> 													Mic plugged in:   ""Headphone Mic Jack"" = true
> 													Headset plugged in:   ""Headphone Mic Jack"" = true?"											one hw switch

SND_JACK_HEADPHONE (probably)

> Headset Headphone Mic			Headphone Mic Jack					Nothing plugged in:   ""Headphone Mic Jack"" = false, ""Headset Mic Phantom Jack"" = false
> 						Headset Mic Phantom Jack				Headphones plugged in:   ""Headphone Mic Jack"" = true, ""Headset Mic Phantom Jack"" = false
> 													Headset plugged in:   ""Headphone Mic Jack"" = false, ""Headset Mic Phantom Jack"" = true ?
> 													Mic plugged in:   ""Headphone Mic Jack"" = true, ""Headset Mic Phantom Jack"" = false"					one hw switch only

SND_JACK_HEADPHONE (probably), the "Headset Mic Phantom Jack" is created 
manually

> Headphone Mic Headset			Headphone Mic Jack					Nothing plugged in:   ""Headphone Mic Jack"" = false, ""Headset Mic Jack"" = false
> 						Headset Mic Jack					Headphones plugged in:   ""Headphone Mic Jack"" = true, ""Headset Mic Jack"" = false
> 													Headset plugged in:   ""Headphone Mic Jack"" = true, ""Headset Mic Jack"" = true
> 													Mic plugged in:   ""Headphone Mic Jack"" = true, ""Headset Mic Jack"" = false
> 																									one switch for hp/mic and the other for the headset mic

SND_JACK_HEADSET (probably)

>
> so, we may need extend snd_jack_types enum, by adding types:  Headset Mic, Headphone Mic, Headset Headphone Mic, Headphone Mic Headset.
>
> enum snd_jack_types {
> 	SND_JACK_HEADPHONE	= 0x0001,
> 	SND_JACK_MICROPHONE	= 0x0002,
> 	SND_JACK_HEADSET	= SND_JACK_HEADPHONE | SND_JACK_MICROPHONE,
> 	SND_JACK_LINEOUT	= 0x0004,
> 	SND_JACK_MECHANICAL	= 0x0008, /* If detected separately */
> 	SND_JACK_VIDEOOUT	= 0x0010,
> 	SND_JACK_AVOUT		= SND_JACK_LINEOUT | SND_JACK_VIDEOOUT,
> 	SND_JACK_LINEIN		= 0x0020,
>
> 	SND_JACK_HEADSET_MIC	= 0x0040, /* one hw switch only */
> 	SND_JACK_HEADPHONE_MIC	= 0x0080, /* one hw switch only, headphone, or mic */
> 	SND_JACK_HEADSET_ HEADPHONE_MIC	= 0x0100, /* one hw switch only,  headset, headphone, or mic*/
> 	SND_JACK_ HEADPHONE_HEADSET_ MIC	= 0x0200, /* headset, headphone, or mic; one switch for hp/mic and the other for the headset mic  */

This seems overly complex. I don't think we really need to add all of that.

First, for the phantom jacks. We'll need phantom jacks not only for 
"Headset Mic Phantom Jack" but also for "Internal Speaker Phantom Jack" 
and "Internal Mic Phantom Jack", and probably some others as well. So 
that means that the HDA driver needs to still create some kctls using 
the current kctl API instead of your new unified API.

A phantom jack is merely a marker to userspace indicating what hardware 
is present. It always returns "plugged in" when trying to read it.

With the phantom jacks dealt with separately, the question remains about 
the Headphone Mic stuff. If the name remains the way it is, maybe all we 
need is a SND_JACK_HEADPHONE (or SND_JACK_HEADSET) jack with the 
"Headphone Mic Jack" name and then userspace would know that this jack 
could potentially accomodate a microphone too. Otherwise we might need 
to add a SND_JACK_HEADPHONE_OR_MIC constant, but I prefer that the names 
are kept properly, because there might be other relevant info added to 
the name too (e g "Front Headphone Jack" instead of "Headphone Jack").

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

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie, Yang]

OK, then I am thinking we can add jack kctls creating code like below: 

snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jjack) {
...
	Switch(type | SND_JACK_HEADSET)
	{
		case SND_JACK_MICROPHONE:
			create "Mic Jack" kctl;
			break;
		case SND_JACK_HEADSET:
			if (id == "Headphone Mic Headset") {
				create " Headphone Mic Jack " kctl;
				create " Headset Mic Jack " kctl;
			} else {
				create " Headphone Jack " kctl;
				create " Headset Mic Jack " kctl;
			}
			break;
		case SND_JACK_HEADPHONE:
			if (id == "Headset Mic") {
				create " Headphone Jack " kctl;
			//	create " Headset Mic Phantom Jack " kctl;
			} else if (id == "Headphone Mic")  {
				create " Headphone Mic Jack " kctl;
			} else if (id == "Headset Headphone Mic")  {
				create " Headphone Mic Jack " kctl;
			//	create " Headset Mic Phantom Jack " kctl;
			} else {
				create " Headphone Jack " kctl;
			}
			break;
		default:
			create "Mic Jack" kctl;
			break;
	}
...
}


Jack Type			Jack name			kctls/switches name
SND_JACK_HEADPHONE	Headphone			Headphone Jack
SND_JACK_MICROPHONE	Mic				Mic Jack
SND_JACK_HEADSET		Headset			Headphone Jack
								Headset Mic Jack
SND_JACK_HEADPHONE	Headset Mic			Headphone Jack
								Headset Mic Phantom Jack
SND_JACK_HEADPHONE 	Headphone Mic		Headphone Mic Jack
SND_JACK_HEADPHONE 	Headset Headphone Mic	Headphone Mic Jack
								Headset Mic Phantom Jack
SND_JACK_HEADSET 		Headphone Mic Headset	Headphone Mic Jack
								Headset Mic Jack

~Keyon

> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Thursday, March 26, 2015 5:06 PM
> To: Jie, Yang; Tanu Kaskinen; Takashi Iwai
> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> Girdwood, Liam R; Liam Girdwood
> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> input device
> 
> 
> 
> On 2015-03-26 09:29, Jie, Yang wrote:
> >
> > Thank you so much, David!
> >
> > So, I summarized as below table:
> 
> I've added how I see them with the new API, see below for further
> comments:
> 
> >
> > Jack Type					kctls/switches name
> 				detection
> 
> 			description
> >
> > Headphone					Headphone Jack
> 				HP plugged in/unplugged
> 
> SND_JACK_HEADPHONE
> 
> >
> > Mic						Mic Jack
> 				Mic plugged in/unplugged
> 
> SND_JACK_MICROPHONE
> 
> >
> > Headset					Headphone Jack
> 				Nothing plugged in:   ""Headphone Jack"" =
> false, ""Headset Mic Jack"" = false
> > 						Headset Mic Jack"
> 				Headphones plugged in:   ""Headphone
> Jack"" = true, ""Headset Mic Jack"" = false
> >
> 				Headset plugged in:   ""Headphone Jack"" =
> true, ""Headset Mic Jack"" = true
> >
> 				Mic plugged in:   ""Headphone Jack"" = true
> ""Headset Mic Jack"" = false(should not plugged in Mic, detect wrongly)"
> 		independent switches
> 
> SND_JACK_HEADSET
> 
> "Mic plugged in" case is irrelevant - not supported by hw
> 
> >
> > Headset Mic					Headphone Jack
> 				Nothing plugged in:   ""Headphone Jack"" =
> false, ""Headset Mic Phantom Jack"" = false
> > 						Headset Mic Phantom Jack
> 				Headphones plugged in:   ""Headphone
> Jack"" = true, ""Headset Mic Phantom Jack"" = false?
> >
> 				Headset plugged in:   ""Headphone Jack"" =
> false, ""Headset Mic Phantom Jack"" = true ?
> >
> 				Mic plugged in:   ""Headphone Jack"" = true,
> ""Headset Mic Phantom Jack"" = false(should not plugged in Mic, detect
> wrongly)"    one hw switch only
> 
> SND_JACK_HEADPHONE, the "Headset Mic Phantom Jack" is created
> manually
> 
> > Headphone Mic				Headphone Mic Jack
> 				Nothing plugged in:   ""Headphone Mic Jack""
> = false
> >
> 				Headphones plugged in:   ""Headphone Mic
> Jack"" = true
> >
> 				Mic plugged in:   ""Headphone Mic Jack"" =
> true
> >
> 				Headset plugged in:   ""Headphone Mic
> Jack"" = true?"
> 			one hw switch
> 
> SND_JACK_HEADPHONE (probably)
> 
> > Headset Headphone Mic			Headphone Mic Jack
> 				Nothing plugged in:   ""Headphone Mic Jack""
> = false, ""Headset Mic Phantom Jack"" = false
> > 						Headset Mic Phantom Jack
> 				Headphones plugged in:   ""Headphone Mic
> Jack"" = true, ""Headset Mic Phantom Jack"" = false
> >
> 				Headset plugged in:   ""Headphone Mic
> Jack"" = false, ""Headset Mic Phantom Jack"" = true ?
> >
> 				Mic plugged in:   ""Headphone Mic Jack"" =
> true, ""Headset Mic Phantom Jack"" = false"
> 	one hw switch only
> 
> SND_JACK_HEADPHONE (probably), the "Headset Mic Phantom Jack" is
> created manually
> 
> > Headphone Mic Headset			Headphone Mic Jack
> 				Nothing plugged in:   ""Headphone Mic Jack""
> = false, ""Headset Mic Jack"" = false
> > 						Headset Mic Jack
> 				Headphones plugged in:   ""Headphone Mic
> Jack"" = true, ""Headset Mic Jack"" = false
> >
> 				Headset plugged in:   ""Headphone Mic
> Jack"" = true, ""Headset Mic Jack"" = true
> >
> 				Mic plugged in:   ""Headphone Mic Jack"" =
> true, ""Headset Mic Jack"" = false
> >
> 
> 							one switch for hp/mic
> and the other for the
> > headset mic
> 
> SND_JACK_HEADSET (probably)
> 
> >
> > so, we may need extend snd_jack_types enum, by adding types:  Headset
> Mic, Headphone Mic, Headset Headphone Mic, Headphone Mic Headset.
> >
> > enum snd_jack_types {
> > 	SND_JACK_HEADPHONE	= 0x0001,
> > 	SND_JACK_MICROPHONE	= 0x0002,
> > 	SND_JACK_HEADSET	= SND_JACK_HEADPHONE |
> SND_JACK_MICROPHONE,
> > 	SND_JACK_LINEOUT	= 0x0004,
> > 	SND_JACK_MECHANICAL	= 0x0008, /* If detected separately */
> > 	SND_JACK_VIDEOOUT	= 0x0010,
> > 	SND_JACK_AVOUT		= SND_JACK_LINEOUT |
> SND_JACK_VIDEOOUT,
> > 	SND_JACK_LINEIN		= 0x0020,
> >
> > 	SND_JACK_HEADSET_MIC	= 0x0040, /* one hw switch only */
> > 	SND_JACK_HEADPHONE_MIC	= 0x0080, /* one hw switch only,
> headphone, or mic */
> > 	SND_JACK_HEADSET_ HEADPHONE_MIC	= 0x0100, /* one hw
> switch only,  headset, headphone, or mic*/
> > 	SND_JACK_ HEADPHONE_HEADSET_ MIC	= 0x0200, /* headset,
> headphone, or mic; one switch for hp/mic and the other for the headset mic
> */
> 
> This seems overly complex. I don't think we really need to add all of that.
> 
> First, for the phantom jacks. We'll need phantom jacks not only for "Headset
> Mic Phantom Jack" but also for "Internal Speaker Phantom Jack"
> and "Internal Mic Phantom Jack", and probably some others as well. So that
> means that the HDA driver needs to still create some kctls using the current
> kctl API instead of your new unified API.
> 
> A phantom jack is merely a marker to userspace indicating what hardware is
> present. It always returns "plugged in" when trying to read it.
> 
> With the phantom jacks dealt with separately, the question remains about
> the Headphone Mic stuff. If the name remains the way it is, maybe all we
> need is a SND_JACK_HEADPHONE (or SND_JACK_HEADSET) jack with the
> "Headphone Mic Jack" name and then userspace would know that this jack
> could potentially accomodate a microphone too. Otherwise we might need
> to add a SND_JACK_HEADPHONE_OR_MIC constant, but I prefer that the
> names are kept properly, because there might be other relevant info added
> to the name too (e g "Front Headphone Jack" instead of "Headphone Jack").
> 
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie, Yang]

Sorry, Correct a typo(removing line "create "Mic Jack" kctl;" in default case)

snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jjack) { ...
	Switch(type | SND_JACK_HEADSET)
	{
		case SND_JACK_MICROPHONE:
			create "Mic Jack" kctl;
			break;
		case SND_JACK_HEADSET:
			if (id == "Headphone Mic Headset") {
				create " Headphone Mic Jack " kctl;
				create " Headset Mic Jack " kctl;
			} else {
				create " Headphone Jack " kctl;
				create " Headset Mic Jack " kctl;
			}
			break;
		case SND_JACK_HEADPHONE:
			if (id == "Headset Mic") {
				create " Headphone Jack " kctl;
			//	create " Headset Mic Phantom Jack " kctl;
			} else if (id == "Headphone Mic")  {
				create " Headphone Mic Jack " kctl;
			} else if (id == "Headset Headphone Mic")  {
				create " Headphone Mic Jack " kctl;
			//	create " Headset Mic Phantom Jack " kctl;
			} else {
				create " Headphone Jack " kctl;
			}
			break;
		default:
			break;
	}
...
}


Jack Type			Jack name			kctls/switches name
SND_JACK_HEADPHONE	Headphone			Headphone Jack
SND_JACK_MICROPHONE	Mic				Mic Jack
SND_JACK_HEADSET		Headset			Headphone Jack
								Headset Mic Jack
SND_JACK_HEADPHONE	Headset Mic			Headphone Jack
								Headset Mic Phantom Jack
SND_JACK_HEADPHONE 	Headphone Mic		Headphone Mic Jack
SND_JACK_HEADPHONE 	Headset Headphone Mic	Headphone Mic Jack
								Headset Mic Phantom Jack
SND_JACK_HEADSET 		Headphone Mic Headset	Headphone Mic Jack
								Headset Mic Jack

~Keyon

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Raymond Yau]

l
>
>
> OK, then I am thinking we can add jack kctls creating code like below:
>
> snd_jack_new(struct snd_card *card, const char *id, int type, struct
snd_jack **jjack) {
> ...
>         Switch(type | SND_JACK_HEADSET)
>         {
>                 case SND_JACK_MICROPHONE:
>                         create "Mic Jack" kctl;
>                         break;
>                 case SND_JACK_HEADSET:
>                         if (id == "Headphone Mic Headset") {
>                                 create " Headphone Mic Jack " kctl;
>                                 create " Headset Mic Jack " kctl;
>                         } else {
>                                 create " Headphone Jack " kctl;
>                                 create " Headset Mic Jack " kctl;
>                         }
>                         break;
>                 case SND_JACK_HEADPHONE:
>                         if (id == "Headset Mic") {
>                                 create " Headphone Jack " kctl;
>                         //      create " Headset Mic Phantom Jack " kctl;
>                         } else if (id == "Headphone Mic")  {
>                                 create " Headphone Mic Jack " kctl;
>                         } else if (id == "Headset Headphone Mic")  {
>                                 create " Headphone Mic Jack " kctl;
>                         //      create " Headset Mic Phantom Jack " kctl;
>                         } else {
>                                 create " Headphone Jack " kctl;
>                         }
>                         break;
>                 default:
>                         create "Mic Jack" kctl;
>                         break;
>         }
> ...
> }

https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/pci/hda/hda_jack.c

If you look at snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t
nid, const char *name, int idx) which add kctl and call snd_jack_new if it
is not phantom

It is unlikely snd_jack_new() create those kctl

For mobile phone or tablet , there is one and only one input : headset

for notebook and desktop, there are dual headphone jacks, four line out
jacks for 7.1 surround, line in,

phantom jack for spdif, surround speakers , hp and mic for those desktop
using ac97 front audio panel

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie, Yang]

From: Raymond Yau [mailto:superquad.vortex2@gmail.com]
Sent: Friday, March 27, 2015 8:39 AM
To: Jie, Yang
Cc: ALSA Development Mailing List; Liam Girdwood; Takashi Iwai; David Henningsson; Girdwood, Liam R; Tanu Kaskinen; broonie@kernel.org
Subject: Re: [alsa-devel] [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device


l
>
>
> OK, then I am thinking we can add jack kctls creating code like below:
>
> snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jjack) {
> ...
>         Switch(type | SND_JACK_HEADSET)
>         {
>                 case SND_JACK_MICROPHONE:
>                         create "Mic Jack" kctl;
>                         break;
>                 case SND_JACK_HEADSET:
>                         if (id == "Headphone Mic Headset") {
>                                 create " Headphone Mic Jack " kctl;
>                                 create " Headset Mic Jack " kctl;
>                         } else {
>                                 create " Headphone Jack " kctl;
>                                 create " Headset Mic Jack " kctl;
>                         }
>                         break;
>                 case SND_JACK_HEADPHONE:
>                         if (id == "Headset Mic") {
>                                 create " Headphone Jack " kctl;
>                         //      create " Headset Mic Phantom Jack " kctl;
>                         } else if (id == "Headphone Mic")  {
>                                 create " Headphone Mic Jack " kctl;
>                         } else if (id == "Headset Headphone Mic")  {
>                                 create " Headphone Mic Jack " kctl;
>                         //      create " Headset Mic Phantom Jack " kctl;
>                         } else {
>                                 create " Headphone Jack " kctl;
>                         }
>                         break;
>                 default:
>                         create "Mic Jack" kctl;
>                         break;
>         }
> ...
> }

https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/pci/hda/hda_jack.c

If you look at snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid, const char *name, int idx) which add kctl and call snd_jack_new if it is not phantom

[Keyon] it’s good that we don’t need handle phantom jack in snd_jack_new().

It is unlikely snd_jack_new() create those kctl

For mobile phone or tablet , there is one and only one input : headset

[Keyon] that’s OK.

for notebook and desktop, there are dual headphone jacks, four line out jacks for 7.1 surround, line in,

phantom jack for spdif, surround speakers , hp and mic for those desktop using ac97 front audio panel

[Keyon] we will create kctls for each headphone/mic/headset jacks, but not for line in/out and phantom

jacks, is this OK?

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[David Henningsson]

On 2015-03-26 13:43, Jie, Yang wrote:
>
> Sorry, Correct a typo(removing line "create "Mic Jack" kctl;" in default case)

It looks like your code can be simplified as:

switch (type | SND_JACK_HEADSET) {
	case SND_JACK_HEADSET:
		create "Headset Mic Jack" kctl;
		/* fall through */
	case SND_JACK_MICROPHONE:
	case SND_JACK_HEADPHONE:
		create format("%s Jack", id) kctl;
}

...that way, prefixes such as "Front Headphone Jack" are preserved too, 
which is a good thing.

// David

>
> snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jjack) { ...
> 	Switch(type | SND_JACK_HEADSET)
> 	{
> 		case SND_JACK_MICROPHONE:
> 			create "Mic Jack" kctl;
> 			break;
> 		case SND_JACK_HEADSET:
> 			if (id == "Headphone Mic Headset") {
> 				create " Headphone Mic Jack " kctl;
> 				create " Headset Mic Jack " kctl;
> 			} else {
> 				create " Headphone Jack " kctl;
> 				create " Headset Mic Jack " kctl;
> 			}
> 			break;
> 		case SND_JACK_HEADPHONE:
> 			if (id == "Headset Mic") {
> 				create " Headphone Jack " kctl;
> 			//	create " Headset Mic Phantom Jack " kctl;
> 			} else if (id == "Headphone Mic")  {
> 				create " Headphone Mic Jack " kctl;
> 			} else if (id == "Headset Headphone Mic")  {
> 				create " Headphone Mic Jack " kctl;
> 			//	create " Headset Mic Phantom Jack " kctl;
> 			} else {
> 				create " Headphone Jack " kctl;
> 			}
> 			break;
> 		default:
> 			break;
> 	}
> ...
> }
>
>
> Jack Type			Jack name			kctls/switches name
> SND_JACK_HEADPHONE	Headphone			Headphone Jack
> SND_JACK_MICROPHONE	Mic				Mic Jack
> SND_JACK_HEADSET		Headset			Headphone Jack
> 								Headset Mic Jack
> SND_JACK_HEADPHONE	Headset Mic			Headphone Jack
> 								Headset Mic Phantom Jack
> SND_JACK_HEADPHONE 	Headphone Mic		Headphone Mic Jack
> SND_JACK_HEADPHONE 	Headset Headphone Mic	Headphone Mic Jack
> 								Headset Mic Phantom Jack
> SND_JACK_HEADSET 		Headphone Mic Headset	Headphone Mic Jack
> 								Headset Mic Jack
>
> ~Keyon
>

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

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie, Yang]

> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Friday, March 27, 2015 2:50 PM
> To: Jie, Yang; Tanu Kaskinen; Takashi Iwai
> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> Girdwood, Liam R; Liam Girdwood
> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> input device
> 
> 
> 
> On 2015-03-26 13:43, Jie, Yang wrote:
> >
> > Sorry, Correct a typo(removing line "create "Mic Jack" kctl;" in
> > default case)
> 
> It looks like your code can be simplified as:
[Keyon] thanks, I will try simplify them.
> 
> switch (type | SND_JACK_HEADSET) {
> 	case SND_JACK_HEADSET:
> 		create "Headset Mic Jack" kctl;
> 		/* fall through */
> 	case SND_JACK_MICROPHONE:
> 	case SND_JACK_HEADPHONE:
> 		create format("%s Jack", id) kctl;
[Keyon] I don't want to fill "id" to kctl name, because sometimes it can be
string which can't be understood by PA, such as:

soc/intel/mfld_machine.c:       ret_val = snd_soc_card_jack_new(runtime->card,
soc/intel/mfld_machine.c-                       "Intel(R) MID Audio Jack", SND_JACK_HEADSET |
soc/intel/mfld_machine.c-                       SND_JACK_BTN_0 | SND_JACK_BTN_1, &mfld_jack,

soc/omap/ams-delta.c:   ret = snd_soc_card_jack_new(card, "hook_switch", SND_JACK_HEADSET,
soc/omap/ams-delta.c-                               &ams_delta_hook_switch, NULL, 0);

> }
> 
> ...that way, prefixes such as "Front Headphone Jack" are preserved too,
[Keyon] if PA can recognize "Front Headphone Jack" and "Rear Headphone Jack", then I
prefer to add a determine here:

	if (!strcmp(id, "Front Headphone Jack") || !strcmp(id, "Rear Headphone Jack"))
		create format("%s Jack", id) kctl;

> which is a good thing.
> 
> // David
> 
> >
> > snd_jack_new(struct snd_card *card, const char *id, int type, struct
> snd_jack **jjack) { ...
> > 	Switch(type | SND_JACK_HEADSET)
> > 	{
> > 		case SND_JACK_MICROPHONE:
> > 			create "Mic Jack" kctl;
> > 			break;
> > 		case SND_JACK_HEADSET:
> > 			if (id == "Headphone Mic Headset") {
> > 				create " Headphone Mic Jack " kctl;
> > 				create " Headset Mic Jack " kctl;
> > 			} else {
> > 				create " Headphone Jack " kctl;
> > 				create " Headset Mic Jack " kctl;
> > 			}
> > 			break;
> > 		case SND_JACK_HEADPHONE:
> > 			if (id == "Headset Mic") {
> > 				create " Headphone Jack " kctl;
> > 			//	create " Headset Mic Phantom Jack " kctl;
> > 			} else if (id == "Headphone Mic")  {
> > 				create " Headphone Mic Jack " kctl;
> > 			} else if (id == "Headset Headphone Mic")  {
> > 				create " Headphone Mic Jack " kctl;
> > 			//	create " Headset Mic Phantom Jack " kctl;
> > 			} else {
> > 				create " Headphone Jack " kctl;
> > 			}
> > 			break;
> > 		default:
> > 			break;
> > 	}
> > ...
> > }
> >
> >
> > Jack Type			Jack name			kctls/switches
> name
> > SND_JACK_HEADPHONE	Headphone			Headphone
> Jack
> > SND_JACK_MICROPHONE	Mic				Mic Jack
> > SND_JACK_HEADSET		Headset			Headphone
> Jack
> > 								Headset Mic
> Jack
> > SND_JACK_HEADPHONE	Headset Mic			Headphone
> Jack
> > 								Headset Mic
> Phantom Jack
> > SND_JACK_HEADPHONE 	Headphone Mic		Headphone
> Mic Jack
> > SND_JACK_HEADPHONE 	Headset Headphone Mic	Headphone
> Mic Jack
> > 								Headset Mic
> Phantom Jack
> > SND_JACK_HEADSET 		Headphone Mic Headset	Headphone
> Mic Jack
> > 								Headset Mic
> Jack
> >
> > ~Keyon
> >
> 
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[David Henningsson]

On 2015-03-27 08:45, Jie, Yang wrote:
>> -----Original Message-----
>> From: David Henningsson [mailto:david.henningsson@canonical.com]
>> Sent: Friday, March 27, 2015 2:50 PM
>> To: Jie, Yang; Tanu Kaskinen; Takashi Iwai
>> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
>> Girdwood, Liam R; Liam Girdwood
>> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
>> input device
>>
>>
>>
>> On 2015-03-26 13:43, Jie, Yang wrote:
>>>
>>> Sorry, Correct a typo(removing line "create "Mic Jack" kctl;" in
>>> default case)
>>
>> It looks like your code can be simplified as:
> [Keyon] thanks, I will try simplify them.
>>
>> switch (type | SND_JACK_HEADSET) {
>> 	case SND_JACK_HEADSET:
>> 		create "Headset Mic Jack" kctl;
>> 		/* fall through */
>> 	case SND_JACK_MICROPHONE:
>> 	case SND_JACK_HEADPHONE:
>> 		create format("%s Jack", id) kctl;
> [Keyon] I don't want to fill "id" to kctl name, because sometimes it can be
> string which can't be understood by PA, such as:
>
> soc/intel/mfld_machine.c:       ret_val = snd_soc_card_jack_new(runtime->card,
> soc/intel/mfld_machine.c-                       "Intel(R) MID Audio Jack", SND_JACK_HEADSET |
> soc/intel/mfld_machine.c-                       SND_JACK_BTN_0 | SND_JACK_BTN_1, &mfld_jack,
>
> soc/omap/ams-delta.c:   ret = snd_soc_card_jack_new(card, "hook_switch", SND_JACK_HEADSET,
> soc/omap/ams-delta.c-                               &ams_delta_hook_switch, NULL, 0);

Ok, good point. It's a matter of different conventions between ASoC and 
HDA. In ASoC, naming is wild west. In HDA, naming is (mostly) consistent 
and we depend on the exact naming for userspace to know exactly how the 
jack functions.

Another example is HDMI/DisplayPort, where the hw often supports several 
ports and we need to know which jack belongs to which PCM device, and we 
use the name of the jack to determine that.

I would very much prefer that all the ASoC jack names (and mixer control 
names!) would be fixed up to be consistent to the extent that makes 
sense, but the ASoC people haven't really agreed to do that, but instead 
prefer to use a large database of UCM files. :-( And seen from a UCM 
context, that the ASoC jacks are labelled terribly inconsistent, isn't 
that big of a deal given that UCM will specify the jack name anyhow.

>> }
>>
>> ...that way, prefixes such as "Front Headphone Jack" are preserved too,
> [Keyon] if PA can recognize "Front Headphone Jack" and "Rear Headphone Jack", then I
> prefer to add a determine here:
>
> 	if (!strcmp(id, "Front Headphone Jack") || !strcmp(id, "Rear Headphone Jack"))
> 		create format("%s Jack", id) kctl;

This will not scale, there are too many variants.

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

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Jie, Yang]

> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Friday, March 27, 2015 4:01 PM
> To: Jie, Yang; Tanu Kaskinen; Takashi Iwai
> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> Girdwood, Liam R; Liam Girdwood
> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
> input device
> 
> 
> 
> On 2015-03-27 08:45, Jie, Yang wrote:
> >> -----Original Message-----
> >> From: David Henningsson [mailto:david.henningsson@canonical.com]
> >> Sent: Friday, March 27, 2015 2:50 PM
> >> To: Jie, Yang; Tanu Kaskinen; Takashi Iwai
> >> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
> >> Girdwood, Liam R; Liam Girdwood
> >> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for
> >> every jack input device
> >>
> >>
> >>
> >> On 2015-03-26 13:43, Jie, Yang wrote:
> >>>
> >>> Sorry, Correct a typo(removing line "create "Mic Jack" kctl;" in
> >>> default case)
> >>
> >> It looks like your code can be simplified as:
> > [Keyon] thanks, I will try simplify them.
> >>
> >> switch (type | SND_JACK_HEADSET) {
> >> 	case SND_JACK_HEADSET:
> >> 		create "Headset Mic Jack" kctl;
> >> 		/* fall through */
> >> 	case SND_JACK_MICROPHONE:
> >> 	case SND_JACK_HEADPHONE:
> >> 		create format("%s Jack", id) kctl;
> > [Keyon] I don't want to fill "id" to kctl name, because sometimes it
> > can be string which can't be understood by PA, such as:
> >
> > soc/intel/mfld_machine.c:       ret_val = snd_soc_card_jack_new(runtime-
> >card,
> > soc/intel/mfld_machine.c-                       "Intel(R) MID Audio Jack",
> SND_JACK_HEADSET |
> > soc/intel/mfld_machine.c-                       SND_JACK_BTN_0 |
> SND_JACK_BTN_1, &mfld_jack,
> >
> > soc/omap/ams-delta.c:   ret = snd_soc_card_jack_new(card,
> "hook_switch", SND_JACK_HEADSET,
> > soc/omap/ams-delta.c-                               &ams_delta_hook_switch, NULL, 0);
> 
> Ok, good point. It's a matter of different conventions between ASoC and
> HDA. In ASoC, naming is wild west. In HDA, naming is (mostly) consistent and
> we depend on the exact naming for userspace to know exactly how the jack
> functions.
> 
> Another example is HDMI/DisplayPort, where the hw often supports several
> ports and we need to know which jack belongs to which PCM device, and we
> use the name of the jack to determine that.
> 
> I would very much prefer that all the ASoC jack names (and mixer control
> names!) would be fixed up to be consistent to the extent that makes sense,
> but the ASoC people haven't really agreed to do that, but instead prefer to
> use a large database of UCM files. :-( And seen from a UCM context, that the
> ASoC jacks are labelled terribly inconsistent, isn't that big of a deal given that
> UCM will specify the jack name anyhow.
[Keyon] that's the bad status. :(
Maybe we can just make HDA jack kctls works and leave not standard name
soc jack kctls invisible for PA?

> 
> >> }
> >>
> >> ...that way, prefixes such as "Front Headphone Jack" are preserved too,
> > [Keyon] if PA can recognize "Front Headphone Jack" and "Rear Headphone
> Jack", then I
> > prefer to add a determine here:
> >
> > 	if (!strcmp(id, "Front Headphone Jack") || !strcmp(id, "Rear
> Headphone Jack"))
> > 		create format("%s Jack", id) kctl;
> 
> This will not scale, there are too many variants.
> 
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Raymond Yau]

> > >
> > > > > +}
> > > > > +
> > > > > +static int snd_jack_new_kctl(struct snd_card *card, struct
> > > > > +snd_jack *jack, int type) {
> > > > > +       struct snd_kcontrol *kctl;
> > > > > +       struct snd_jack_kctl *jack_kctl;
> > > > > +       int i, err, index, state = 0 /* use 0 for default state
?? */;
> > > > > +
> > > > > +       INIT_LIST_HEAD(&jack->kctl_list);
> > > > > +       for (i = 0; i < fls(SND_JACK_BTN_0); i++) {
> > > > > +               int testbit = 1 << i;
> > > > > +               if (type & testbit) {
> > > >
> > > > With this implementation, you'll get multiple boolean kctls for a
> > > > headset.  I thought we agreed with creating a single boolean for
headset?
> > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g.
> > headset.
> > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET  |
> > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is
> > > pressed, we will report to the 3rd kctl.
> >
> > Hm, I don't remember that I agreed with multiple kctls...
> >
> > The multiple kctls have a significant drawback (multiple event calls
for a single
> > headset) and its behavior is incompatible with the current code (both
the
> > name change and the behavior change).  That is, your patch will very
likely
> > break the existing applications.
> [Keyon] I am not very clear with the existing applications that using
these
> kctl events(seems Android use input subsystem event? Which we didn't
> Change here. If I understand correctly, Pulseaudio uses jack switch
controls,
> via the name, then we can use different name for headphone and mic here.)
>
> we will generate 2 event calls(one headphone, one mic) when Headset
> plug in/out, applications will receive these 2 events,  and they can do
anything,
> e.g. decide to switch on/off speaker/headphone.

Why do you need to generate two event (hp and mic) if soc jack only support
headset ?

For mobile phone and tablet which only support headset using TRRRS
connector  and most of them don't support  headphone using TRRS connector.

When the headset jack kctl is on, this implies headphone and headset mic
are used

Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

[David Henningsson]

On 2015-03-27 10:11, Jie, Yang wrote:
>> -----Original Message-----
>> From: David Henningsson [mailto:david.henningsson@canonical.com]
>> Sent: Friday, March 27, 2015 4:01 PM
>> To: Jie, Yang; Tanu Kaskinen; Takashi Iwai
>> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
>> Girdwood, Liam R; Liam Girdwood
>> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack
>> input device
>>
>>
>>
>> On 2015-03-27 08:45, Jie, Yang wrote:
>>>> -----Original Message-----
>>>> From: David Henningsson [mailto:david.henningsson@canonical.com]
>>>> Sent: Friday, March 27, 2015 2:50 PM
>>>> To: Jie, Yang; Tanu Kaskinen; Takashi Iwai
>>>> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org;
>>>> Girdwood, Liam R; Liam Girdwood
>>>> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for
>>>> every jack input device
>>>>
>>>>
>>>>
>>>> On 2015-03-26 13:43, Jie, Yang wrote:
>>>>>
>>>>> Sorry, Correct a typo(removing line "create "Mic Jack" kctl;" in
>>>>> default case)
>>>>
>>>> It looks like your code can be simplified as:
>>> [Keyon] thanks, I will try simplify them.
>>>>
>>>> switch (type | SND_JACK_HEADSET) {
>>>> 	case SND_JACK_HEADSET:
>>>> 		create "Headset Mic Jack" kctl;
>>>> 		/* fall through */
>>>> 	case SND_JACK_MICROPHONE:
>>>> 	case SND_JACK_HEADPHONE:
>>>> 		create format("%s Jack", id) kctl;
>>> [Keyon] I don't want to fill "id" to kctl name, because sometimes it
>>> can be string which can't be understood by PA, such as:
>>>
>>> soc/intel/mfld_machine.c:       ret_val = snd_soc_card_jack_new(runtime-
>>> card,
>>> soc/intel/mfld_machine.c-                       "Intel(R) MID Audio Jack",
>> SND_JACK_HEADSET |
>>> soc/intel/mfld_machine.c-                       SND_JACK_BTN_0 |
>> SND_JACK_BTN_1, &mfld_jack,
>>>
>>> soc/omap/ams-delta.c:   ret = snd_soc_card_jack_new(card,
>> "hook_switch", SND_JACK_HEADSET,
>>> soc/omap/ams-delta.c-                               &ams_delta_hook_switch, NULL, 0);
>>
>> Ok, good point. It's a matter of different conventions between ASoC and
>> HDA. In ASoC, naming is wild west. In HDA, naming is (mostly) consistent and
>> we depend on the exact naming for userspace to know exactly how the jack
>> functions.
>>
>> Another example is HDMI/DisplayPort, where the hw often supports several
>> ports and we need to know which jack belongs to which PCM device, and we
>> use the name of the jack to determine that.
>>
>> I would very much prefer that all the ASoC jack names (and mixer control
>> names!) would be fixed up to be consistent to the extent that makes sense,
>> but the ASoC people haven't really agreed to do that, but instead prefer to
>> use a large database of UCM files. :-( And seen from a UCM context, that the
>> ASoC jacks are labelled terribly inconsistent, isn't that big of a deal given that
>> UCM will specify the jack name anyhow.
> [Keyon] that's the bad status. :(
> Maybe we can just make HDA jack kctls works and leave not standard name
> soc jack kctls invisible for PA?

If with "invisible for PA" you mean that there are still kctls created, 
but their naming of the jacks are non-standard, yes, I think this is an 
okay outcome of your patch.
I still think ASoC should change their jack naming to be more 
consistent, but that can be addressed in some future patch set.


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