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

[PATCH v3 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.

Changes in v3:
1. replace bit index with bit mask in jack_kctl;
2. add exception for SND_JACK_HEADSET and SND_JACK_AVOUT, only create
one jack kctl for these two combo jacks, respectively.
3. add NULL check, mem kfree, and fix some potential risk.

Change in v2:
1. define jack_kctl struct, and put jack kctl related stuff there;
2. add a patch to remove the existing controls for HDA jack.

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        | 118 ++++++++++++++++++++++++++++++++++++++++++++++-
 sound/pci/hda/Kconfig    |  10 +---
 sound/pci/hda/hda_jack.c |  45 ++----------------
 sound/pci/hda/hda_jack.h |   3 --
 7 files changed, 129 insertions(+), 61 deletions(-)

-- 
1.9.1

[PATCH v3 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    | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 124 insertions(+), 2 deletions(-)

diff --git a/include/sound/jack.h b/include/sound/jack.h
index 2182350..cd5652d 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 mask_bit;	/* the corresponding bit of the jack type */
+};
+
 #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..82c8316 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_init(&jack_kctl->jack_list);
+		snd_ctl_remove(card, jack_kctl->kctl);
+	}
 	if (jack->private_free)
 		jack->private_free(jack);
 
@@ -100,6 +107,98 @@ static int snd_jack_dev_register(struct snd_device *device)
 	return err;
 }
 
+static int jack_ctl_name_found(struct snd_card *card, const char *name, int index)
+{
+	struct snd_kcontrol *kctl;
+	int len = strlen(name);
+
+	down_read(&card->controls_rwsem);
+
+	list_for_each_entry(kctl, &card->controls, list) {
+		if (!strncmp(name, kctl->id.name, len) &&
+		    !strcmp(" Jack", kctl->id.name + len) &&
+		    kctl->id.index == index) {
+			up_read(&card->controls_rwsem);
+			return 0;
+		}
+	}
+
+	up_read(&card->controls_rwsem);
+	return 1;
+}
+
+/* get the first unused/available index number for the given kctl name */
+static int get_available_index(struct snd_card *card, const char *name)
+{
+	int idx = 0;
+
+	while (!jack_ctl_name_found(card, name, idx))
+		idx++;
+
+	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(jack_kctl);
+}
+
+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;
+
+		/* we only new headphone kctl for headset jack */
+		if ((testbit == SND_JACK_MICROPHONE) &&
+				((type & SND_JACK_HEADSET) == SND_JACK_HEADSET))
+			continue;
+
+		/* we only new lineout kctl for avout jack */
+		if ((testbit == SND_JACK_VIDEOOUT) &&
+				((type & SND_JACK_AVOUT) == SND_JACK_AVOUT))
+			continue;
+
+		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);
+
+			if (!jack_kctl)
+				return -ENOMEM;
+
+			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->mask_bit = testbit;
+
+			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 +237,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 +249,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 +336,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 +352,20 @@ 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);
+
+	list_for_each_entry(jack_kctl, &jack->kctl_list, jack_list) {
+		snd_kctl_jack_report(jack->card, jack_kctl->kctl,
+					status & jack_kctl->mask_bit);
+	}
+
 }
 EXPORT_SYMBOL(snd_jack_report);
 
-- 
1.9.1

[PATCH v3 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 v3 1/2] ALSA: jack: create jack kcontrols for every jack input device

[Takashi Iwai]

At Fri, 20 Mar 2015 23:39:12 +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>

Thanks for a quick update.  It's getting better, but still some points
to be fixed.  Let's see...


> +struct snd_jack_kctl {
> +	struct snd_kcontrol *kctl;
> +	struct list_head jack_list;		/* list of controls belong to the same jack*/

jack_ prefix is superfluous.

> +static int jack_ctl_name_found(struct snd_card *card, const char *name, int index)

Use bool.

> +{
> +	struct snd_kcontrol *kctl;
> +	int len = strlen(name);
> +
> +	down_read(&card->controls_rwsem);
> +
> +	list_for_each_entry(kctl, &card->controls, list) {
> +		if (!strncmp(name, kctl->id.name, len) &&
> +		    !strcmp(" Jack", kctl->id.name + len) &&
> +		    kctl->id.index == index) {

This doesn't work when ctl elements are with multiple counts.  A
single kctl object may contain multiple snd_kcontrol_volatile entries.
That's why snd_ctl_find_id() checks like:

	list_for_each_entry(kctl, &card->controls, list) {
		....
		if (kctl->id.index > id->index)
			continue;
		if (kctl->id.index + kctl->count <= id->index)
			continue;

Also, more importantly, your code checks only the name string.  But
the same name string can be assigned for different iface, device and
subdevice entries.  You need to compare them as well.

That said, it'd be even simpler just to call snd_ctl_find_id() after
creating a ctl id instance instead of writing an own funciton.  It
ends up with a bit more stack usage, but it should be still
acceptable.


> +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;
> +
> +		/* we only new headphone kctl for headset jack */
> +		if ((testbit == SND_JACK_MICROPHONE) &&
> +				((type & SND_JACK_HEADSET) == SND_JACK_HEADSET))
> +			continue;

The parentheses are superfluous.

> +		/* we only new lineout kctl for avout jack */
> +		if ((testbit == SND_JACK_VIDEOOUT) &&
> +				((type & SND_JACK_AVOUT) == SND_JACK_AVOUT))
> +			continue;
> +
> +		if (type & testbit) {

This check can be at the beginning of the loop, i.e.

		if (!(type & testbit))
			continue;

then you'll reduce one indent level.


Now, a bigger question:

> +			index = get_available_index(card,jack->id);
> +			kctl = snd_kctl_jack_new(jack->id, index, card);

So, here you are creating multiple kctl elements with the very same
name string.  Did you take a look at the actual usages of
snd_jack_new(), especially snd_soc_card_jack_new()?

Try like

% git grep -A2 snd_soc_card_jack_new sound/soc/

sound/soc/fsl/imx-es8328.c:             ret = snd_soc_card_jack_new(rtd->card, "Headphone",
sound/soc/fsl/imx-es8328.c-                                         SND_JACK_HEADPHONE | SND_JACK_BTN_0,
sound/soc/fsl/imx-es8328.c-                                         &headset_jack, NULL, 0);
--
sound/soc/fsl/wm1133-ev1.c:     snd_soc_card_jack_new(rtd->card, "Headphone", SND_JACK_HEADPHONE,
sound/soc/fsl/wm1133-ev1.c-                           &hp_jack, hp_jack_pins, ARRAY_SIZE(hp_jack_pins));
sound/soc/fsl/wm1133-ev1.c-     wm8350_hp_jack_detect(codec, WM8350_JDR, &hp_jack, SND_JACK_HEADPHONE);
--
sound/soc/fsl/wm1133-ev1.c:     snd_soc_card_jack_new(rtd->card, "Microphone",
sound/soc/fsl/wm1133-ev1.c-                           SND_JACK_MICROPHONE | SND_JACK_BTN_0, &mic_jack,
sound/soc/fsl/wm1133-ev1.c-                           mic_jack_pins, ARRAY_SIZE(mic_jack_pins));
.....

As you'll find through the whole output, many calls are combined with
SND_JACK_BTN_* bits.  And think again how this will result with your
patch.  A call like

	snd_soc_card_jack_new(card, "Headphone",
		SND_JACK_HEADPHONE | SND_JACK_BTN_0 | SND_JACK_BTN_1, ...)

would give three kctls, all "Headphone Jack" with indices 0, 1 and 2.
How user-space knows what is for what?

Also, when you look through the grep above, you'll find that many
calls already contain "Jack" name suffix.  So, they will result in
a name like "Headset Jack Jack".

One last bit:
> +			/* set initial jack state */
> +			snd_kctl_jack_report(card, kctl, state & testbit);

This can be dropped.  The value is zero as default, so unless the
actual value is different, you don't need to call it. 

Or, yet another last bit: remove EXPORT_SYMBOL_GPL() from ctljack.c,
as they are merely internal functions.  But this can be done another
patch later.


Takashi

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

[Takashi Iwai]

At Fri, 20 Mar 2015 23:39:13 +0800,
Jie Yang wrote:
> 
> @@ -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)

The check here is superfluous, as you already added in the above.


thanks,

Takashi

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

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Saturday, March 21, 2015 12:22 AM
> To: Jie, Yang
> Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood, Liam R
> Subject: Re: [alsa-devel] [PATCH v3 2/2] ALSA: hda - Remove jack kctls
> 
> At Fri, 20 Mar 2015 23:39:13 +0800,
> Jie Yang wrote:
> >
> > @@ -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)
> 
> The check here is superfluous, as you already added in the above.
[Keyon] got it.
> 
> 
> thanks,
> 
> Takashi

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

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Saturday, March 21, 2015 12:17 AM
> To: Jie, Yang
> Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood, Liam R;
> Liam Girdwood
> Subject: Re: [PATCH v3 1/2] ALSA: jack: create jack kcontrols for every jack
> input device
> 
> At Fri, 20 Mar 2015 23:39:12 +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>
> 
> Thanks for a quick update.  It's getting better, but still some points to be fixed.
> Let's see...
> 
> 
> > +struct snd_jack_kctl {
> > +	struct snd_kcontrol *kctl;
> > +	struct list_head jack_list;		/* list of controls belong to
> the same jack*/
> 
> jack_ prefix is superfluous.
[Keyon] OK.
> 
> > +static int jack_ctl_name_found(struct snd_card *card, const char
> > +*name, int index)
> 
> Use bool.
[Keyon] OK, will change it.
> 
> > +{
> > +	struct snd_kcontrol *kctl;
> > +	int len = strlen(name);
> > +
> > +	down_read(&card->controls_rwsem);
> > +
> > +	list_for_each_entry(kctl, &card->controls, list) {
> > +		if (!strncmp(name, kctl->id.name, len) &&
> > +		    !strcmp(" Jack", kctl->id.name + len) &&
> > +		    kctl->id.index == index) {
> 
> This doesn't work when ctl elements are with multiple counts.  A single kctl
> object may contain multiple snd_kcontrol_volatile entries.
> That's why snd_ctl_find_id() checks like:
> 
> 	list_for_each_entry(kctl, &card->controls, list) {
> 		....
> 		if (kctl->id.index > id->index)
> 			continue;
> 		if (kctl->id.index + kctl->count <= id->index)
> 			continue;
> 
> Also, more importantly, your code checks only the name string.  But the
> same name string can be assigned for different iface, device and subdevice
> entries.  You need to compare them as well.
> 
> That said, it'd be even simpler just to call snd_ctl_find_id() after creating a ctl
> id instance instead of writing an own funciton.  It ends up with a bit more
> stack usage, but it should be still acceptable.
[Keyon] Here I did the getting index work by imitating function from
had_jack.c:get_unique_index(), and I also noticed and concerned about
not checking other snd_ctl_elem_id items.

Thank you for explaining this detail, Takashi, will change to use snd_ctl_find_id(). 

> 
> 
> > +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;
> > +
> > +		/* we only new headphone kctl for headset jack */
> > +		if ((testbit == SND_JACK_MICROPHONE) &&
> > +				((type & SND_JACK_HEADSET) ==
> SND_JACK_HEADSET))
> > +			continue;
> 
> The parentheses are superfluous.
[Keyon] OK.
> 
> > +		/* we only new lineout kctl for avout jack */
> > +		if ((testbit == SND_JACK_VIDEOOUT) &&
> > +				((type & SND_JACK_AVOUT) ==
> SND_JACK_AVOUT))
> > +			continue;
> > +
> > +		if (type & testbit) {
> 
> This check can be at the beginning of the loop, i.e.
> 
> 		if (!(type & testbit))
> 			continue;
> 
> then you'll reduce one indent level.
[Keyon] good idea. Will change it.
> 
> 
> Now, a bigger question:
> 
> > +			index = get_available_index(card,jack->id);
> > +			kctl = snd_kctl_jack_new(jack->id, index, card);
> 
> So, here you are creating multiple kctl elements with the very same name
> string.  Did you take a look at the actual usages of snd_jack_new(), especially
> snd_soc_card_jack_new()?
[Keyon] yes, I noticed this.  I can also find it at soc/intel/broadwell.c:

        ret = snd_soc_card_jack_new(rtd->card, "Headset",
                SND_JACK_HEADSET | SND_JACK_BTN_0, &broadwell_headset,
                broadwell_headset_pins, ARRAY_SIZE(broadwell_headset_pins));

it will create 2 kctl elements with the same name string "Headset":

numid=12,iface=CARD,name='Headset Jack'
numid=13,iface=CARD,name='Headset Jack',index=1

> 
> Try like
> 
> % git grep -A2 snd_soc_card_jack_new sound/soc/
> 
> sound/soc/fsl/imx-es8328.c:             ret = snd_soc_card_jack_new(rtd->card,
> "Headphone",
> sound/soc/fsl/imx-es8328.c-                                         SND_JACK_HEADPHONE |
> SND_JACK_BTN_0,
> sound/soc/fsl/imx-es8328.c-                                         &headset_jack, NULL, 0);
> --
> sound/soc/fsl/wm1133-ev1.c:     snd_soc_card_jack_new(rtd->card,
> "Headphone", SND_JACK_HEADPHONE,
> sound/soc/fsl/wm1133-ev1.c-                           &hp_jack, hp_jack_pins,
> ARRAY_SIZE(hp_jack_pins));
> sound/soc/fsl/wm1133-ev1.c-     wm8350_hp_jack_detect(codec,
> WM8350_JDR, &hp_jack, SND_JACK_HEADPHONE);
> --
> sound/soc/fsl/wm1133-ev1.c:     snd_soc_card_jack_new(rtd->card,
> "Microphone",
> sound/soc/fsl/wm1133-ev1.c-                           SND_JACK_MICROPHONE |
> SND_JACK_BTN_0, &mic_jack,
> sound/soc/fsl/wm1133-ev1.c-                           mic_jack_pins,
> ARRAY_SIZE(mic_jack_pins));
> .....
> 
> As you'll find through the whole output, many calls are combined with
> SND_JACK_BTN_* bits.  And think again how this will result with your patch.
> A call like
> 
> 	snd_soc_card_jack_new(card, "Headphone",
> 		SND_JACK_HEADPHONE | SND_JACK_BTN_0 |
> SND_JACK_BTN_1, ...)
> 
> would give three kctls, all "Headphone Jack" with indices 0, 1 and 2.
> How user-space knows what is for what?
[Keyon] yes, this is an issue I was thinking about, we need discussing and conclusion
about that, including comments from user-space, here adding Tanu.
Here I am thinking about
1. do we really need kctls for buttons(SND_JACK_BTN_n)? if no, that is simplest, 
we can filter out them at this kctl creating stage.
2. if yes, then we need an name regenerator here, to generate understandable
(for user-space) kctl names for buttons.

> 
> Also, when you look through the grep above, you'll find that many calls
> already contain "Jack" name suffix.  So, they will result in a name like
> "Headset Jack Jack".
[Keyon] do you think we should implement intelligent name regenerator here
to remove this suffix? 

And furthermore, we can check more here, e.g. if the name ("Headset") is 
matched with the type(type should have SND_JACK_HEADSET bits), or 
something like that. 

> 
> One last bit:
> > +			/* set initial jack state */
> > +			snd_kctl_jack_report(card, kctl, state & testbit);
> 
> This can be dropped.  The value is zero as default, so unless the actual value
> is different, you don't need to call it.
[Keyon] OK.
> 
> Or, yet another last bit: remove EXPORT_SYMBOL_GPL() from ctljack.c, as
> they are merely internal functions.  But this can be done another patch later.
[Keyon] OK, I will add this to another patch.
> 
> 
> Takashi

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

[Takashi Iwai]

At Sat, 21 Mar 2015 02:22:47 +0000,
Jie, Yang wrote:
> 
> > > +{
> > > +	struct snd_kcontrol *kctl;
> > > +	int len = strlen(name);
> > > +
> > > +	down_read(&card->controls_rwsem);
> > > +
> > > +	list_for_each_entry(kctl, &card->controls, list) {
> > > +		if (!strncmp(name, kctl->id.name, len) &&
> > > +		    !strcmp(" Jack", kctl->id.name + len) &&
> > > +		    kctl->id.index == index) {
> > 
> > This doesn't work when ctl elements are with multiple counts.  A single kctl
> > object may contain multiple snd_kcontrol_volatile entries.
> > That's why snd_ctl_find_id() checks like:
> > 
> > 	list_for_each_entry(kctl, &card->controls, list) {
> > 		....
> > 		if (kctl->id.index > id->index)
> > 			continue;
> > 		if (kctl->id.index + kctl->count <= id->index)
> > 			continue;
> > 
> > Also, more importantly, your code checks only the name string.  But the
> > same name string can be assigned for different iface, device and subdevice
> > entries.  You need to compare them as well.
> > 
> > That said, it'd be even simpler just to call snd_ctl_find_id() after creating a ctl
> > id instance instead of writing an own funciton.  It ends up with a bit more
> > stack usage, but it should be still acceptable.
> [Keyon] Here I did the getting index work by imitating function from
> had_jack.c:get_unique_index(), and I also noticed and concerned about
> not checking other snd_ctl_elem_id items.

In the case of hda_jack.c it works like that because it checks the own
list of ctls, not the global list.  The driver knows that there will
be no other "XXX Jack" controls.

> > 
> > Now, a bigger question:
> > 
> > > +			index = get_available_index(card,jack->id);
> > > +			kctl = snd_kctl_jack_new(jack->id, index, card);
> > 
> > So, here you are creating multiple kctl elements with the very same name
> > string.  Did you take a look at the actual usages of snd_jack_new(), especially
> > snd_soc_card_jack_new()?
> [Keyon] yes, I noticed this.  I can also find it at soc/intel/broadwell.c:
> 
>         ret = snd_soc_card_jack_new(rtd->card, "Headset",
>                 SND_JACK_HEADSET | SND_JACK_BTN_0, &broadwell_headset,
>                 broadwell_headset_pins, ARRAY_SIZE(broadwell_headset_pins));
> 
> it will create 2 kctl elements with the same name string "Headset":
> 
> numid=12,iface=CARD,name='Headset Jack'
> numid=13,iface=CARD,name='Headset Jack',index=1
> 
> > 
> > Try like
> > 
> > % git grep -A2 snd_soc_card_jack_new sound/soc/
> > 
> > sound/soc/fsl/imx-es8328.c:             ret = snd_soc_card_jack_new(rtd->card,
> > "Headphone",
> > sound/soc/fsl/imx-es8328.c-                                         SND_JACK_HEADPHONE |
> > SND_JACK_BTN_0,
> > sound/soc/fsl/imx-es8328.c-                                         &headset_jack, NULL, 0);
> > --
> > sound/soc/fsl/wm1133-ev1.c:     snd_soc_card_jack_new(rtd->card,
> > "Headphone", SND_JACK_HEADPHONE,
> > sound/soc/fsl/wm1133-ev1.c-                           &hp_jack, hp_jack_pins,
> > ARRAY_SIZE(hp_jack_pins));
> > sound/soc/fsl/wm1133-ev1.c-     wm8350_hp_jack_detect(codec,
> > WM8350_JDR, &hp_jack, SND_JACK_HEADPHONE);
> > --
> > sound/soc/fsl/wm1133-ev1.c:     snd_soc_card_jack_new(rtd->card,
> > "Microphone",
> > sound/soc/fsl/wm1133-ev1.c-                           SND_JACK_MICROPHONE |
> > SND_JACK_BTN_0, &mic_jack,
> > sound/soc/fsl/wm1133-ev1.c-                           mic_jack_pins,
> > ARRAY_SIZE(mic_jack_pins));
> > .....
> > 
> > As you'll find through the whole output, many calls are combined with
> > SND_JACK_BTN_* bits.  And think again how this will result with your patch.
> > A call like
> > 
> > 	snd_soc_card_jack_new(card, "Headphone",
> > 		SND_JACK_HEADPHONE | SND_JACK_BTN_0 |
> > SND_JACK_BTN_1, ...)
> > 
> > would give three kctls, all "Headphone Jack" with indices 0, 1 and 2.
> > How user-space knows what is for what?
> [Keyon] yes, this is an issue I was thinking about, we need discussing and conclusion
> about that, including comments from user-space, here adding Tanu.
> Here I am thinking about
> 1. do we really need kctls for buttons(SND_JACK_BTN_n)? if no, that is simplest, 
> we can filter out them at this kctl creating stage.
> 2. if yes, then we need an name regenerator here, to generate understandable
> (for user-space) kctl names for buttons.

IMO, we should create kctls with proper unique names.

> > Also, when you look through the grep above, you'll find that many calls
> > already contain "Jack" name suffix.  So, they will result in a name like
> > "Headset Jack Jack".
> [Keyon] do you think we should implement intelligent name regenerator here
> to remove this suffix? 

Yes, "XXX Jack" is easy to check.

> And furthermore, we can check more here, e.g. if the name ("Headset") is 
> matched with the type(type should have SND_JACK_HEADSET bits), or 
> something like that. 

I don't think we need a naming police in the core code.


Takashi

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

[David Henningsson]

On 2015-03-20 16:39, Jie Yang wrote:
> 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.

Hmm, I must have missed this. It's great that you try to implement kctl 
jacks for ASoC, but your actual implementation seems to regress HDA, 
unless I'm missing something.

In particular, the phantom jacks currently show up in the kctl jack 
layer only, not in the /dev/input layer, and I prefer to keep it that 
way in order not to pollute the /dev/input layer with phantom jacks.
But it looks like these will just disappear (?!) with your patch, but 
I'm not entirely sure. Could you elaborate?

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

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

[Tanu Kaskinen]

On Sat, 2015-03-21 at 02:22 +0000, Jie, Yang wrote:
> > As you'll find through the whole output, many calls are combined with
> > SND_JACK_BTN_* bits.  And think again how this will result with your patch.
> > A call like
> > 
> > 	snd_soc_card_jack_new(card, "Headphone",
> > 		SND_JACK_HEADPHONE | SND_JACK_BTN_0 |
> > SND_JACK_BTN_1, ...)
> > 
> > would give three kctls, all "Headphone Jack" with indices 0, 1 and 2.
> > How user-space knows what is for what?
> [Keyon] yes, this is an issue I was thinking about, we need discussing and conclusion
> about that, including comments from user-space, here adding Tanu.
> Here I am thinking about
> 1. do we really need kctls for buttons(SND_JACK_BTN_n)? if no, that is simplest, 
> we can filter out them at this kctl creating stage.
> 2. if yes, then we need an name regenerator here, to generate understandable
> (for user-space) kctl names for buttons.

>From PulseAudio point of view, you must make sure that "Headphone Jack"
with index 0 doesn't refer to a button, because the assumption is that
"Headphone Jack" at index 0 refers to a jack and not a button (indexes
other than 0 are currently ignored). Other than that, PulseAudio doesn't
currently care, because there's no support for buttons.

When thinking about how to implement button support in PulseAudio, the
first question is that are kcontrols a realiable interface for getting
button events? What happens when the button is pressed? Does one press
cause only one on->off or off->on state transition, or is the state "on"
only for the duration of the press? If the former, then kcontrols should
be fine, but if the latter, is it guaranteed that the application sees
the event? If the button press is a short one, causing a quick
off->on->off transition, is it possible that the application gets a
notification of a mixer change, but when the application then inspects
the mixer state, the button state is already "off", so the application
doesn't get enough information about what happened?

If buttons are exposed as kcontrols, I suppose "Headphone Button N"
would be a fine name.

-- 
Tanu

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

[Takashi Iwai]

At Mon, 23 Mar 2015 12:08:17 +0200,
Tanu Kaskinen wrote:
> 
> On Sat, 2015-03-21 at 02:22 +0000, Jie, Yang wrote:
> > > As you'll find through the whole output, many calls are combined with
> > > SND_JACK_BTN_* bits.  And think again how this will result with your patch.
> > > A call like
> > > 
> > > 	snd_soc_card_jack_new(card, "Headphone",
> > > 		SND_JACK_HEADPHONE | SND_JACK_BTN_0 |
> > > SND_JACK_BTN_1, ...)
> > > 
> > > would give three kctls, all "Headphone Jack" with indices 0, 1 and 2.
> > > How user-space knows what is for what?
> > [Keyon] yes, this is an issue I was thinking about, we need discussing and conclusion
> > about that, including comments from user-space, here adding Tanu.
> > Here I am thinking about
> > 1. do we really need kctls for buttons(SND_JACK_BTN_n)? if no, that is simplest, 
> > we can filter out them at this kctl creating stage.
> > 2. if yes, then we need an name regenerator here, to generate understandable
> > (for user-space) kctl names for buttons.
> 
> >From PulseAudio point of view, you must make sure that "Headphone Jack"
> with index 0 doesn't refer to a button, because the assumption is that
> "Headphone Jack" at index 0 refers to a jack and not a button (indexes
> other than 0 are currently ignored). Other than that, PulseAudio doesn't
> currently care, because there's no support for buttons.
> 
> When thinking about how to implement button support in PulseAudio, the
> first question is that are kcontrols a realiable interface for getting
> button events? What happens when the button is pressed? Does one press
> cause only one on->off or off->on state transition, or is the state "on"
> only for the duration of the press? If the former, then kcontrols should
> be fine, but if the latter, is it guaranteed that the application sees
> the event? If the button press is a short one, causing a quick
> off->on->off transition, is it possible that the application gets a
> notification of a mixer change, but when the application then inspects
> the mixer state, the button state is already "off", so the application
> doesn't get enough information about what happened?

It's a good point.  The notification via kctl can't follow the
complete state changes since it's nothing but a notification telling
"something changed".  So, events like buttons that need the complete
state change history, the current implementation isn't appropriate.
For things like jacks, we usually care only about the final state, and
the state change is much less frequent, so kctl notification works
well.


Takashi

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

[Mark Brown]

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

On Mon, Mar 23, 2015 at 12:57:02PM +0100, Takashi Iwai wrote:

> It's a good point.  The notification via kctl can't follow the
> complete state changes since it's nothing but a notification telling
> "something changed".  So, events like buttons that need the complete
> state change history, the current implementation isn't appropriate.
> For things like jacks, we usually care only about the final state, and
> the state change is much less frequent, so kctl notification works
> well.

I'd expect the buttons to be going through only as input devices, not as
kcontrols at all.

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

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

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

[Mark Brown]

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

On Mon, Mar 23, 2015 at 10:18:19AM +0100, David Henningsson wrote:

> Hmm, I must have missed this. It's great that you try to implement kctl
> jacks for ASoC, but your actual implementation seems to regress HDA, unless
> I'm missing something.

> In particular, the phantom jacks currently show up in the kctl jack layer
> only, not in the /dev/input layer, and I prefer to keep it that way in order
> not to pollute the /dev/input layer with phantom jacks.
> But it looks like these will just disappear (?!) with your patch, but I'm
> not entirely sure. Could you elaborate?

We shouldn't be hacking individual drivers to support whatever random
subset of userspace interfaces some system decides it wants to use -
that isn't making anyone happy.  If we want to have the ability to
customize which userspace interfaces appear it seems better to put that
in the core code so individual drivers don't need to worry about it,
from that point of view unifying the interfaces should be progress.

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

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

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

[Takashi Iwai]

At Mon, 23 Mar 2015 08:00:31 -0700,
Mark Brown wrote:
> 
> On Mon, Mar 23, 2015 at 10:18:19AM +0100, David Henningsson wrote:
> 
> > Hmm, I must have missed this. It's great that you try to implement kctl
> > jacks for ASoC, but your actual implementation seems to regress HDA, unless
> > I'm missing something.
> 
> > In particular, the phantom jacks currently show up in the kctl jack layer
> > only, not in the /dev/input layer, and I prefer to keep it that way in order
> > not to pollute the /dev/input layer with phantom jacks.
> > But it looks like these will just disappear (?!) with your patch, but I'm
> > not entirely sure. Could you elaborate?
> 
> We shouldn't be hacking individual drivers to support whatever random
> subset of userspace interfaces some system decides it wants to use -
> that isn't making anyone happy.  If we want to have the ability to
> customize which userspace interfaces appear it seems better to put that
> in the core code so individual drivers don't need to worry about it,
> from that point of view unifying the interfaces should be progress.

Right, but it's a bit irrelevant.  We do want to have a common core
code, indeed.  However, as David suggested, the latest patchset still
doesn't care about "phantom" jack that is a mandatory feature.
HD-audio create kctl items even for fixed pins without jack detection.
This is needed for having consistent pin mapping.

Now the question is whether we need representing the same item for
input jack, even though it's nothing but a place holder.  If we want
to have consistency between input and kctl jack items, then yes.
OTOH, we might want to drop buttons from kctls in anyway, so such a
consistency has no importance.  Then we may just ignore phantom jacks
for input jacks but create only phatom jack kctls, too, either by
adding a new flag to indicate that, or let type=0 behaving like that.


Takashi

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

[Mark Brown]

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

On Mon, Mar 23, 2015 at 04:09:33PM +0100, Takashi Iwai wrote:

> Right, but it's a bit irrelevant.  We do want to have a common core
> code, indeed.  However, as David suggested, the latest patchset still
> doesn't care about "phantom" jack that is a mandatory feature.
> HD-audio create kctl items even for fixed pins without jack detection.
> This is needed for having consistent pin mapping.

OK, right - phantom jacks aren't something I was aware of.

> Now the question is whether we need representing the same item for
> input jack, even though it's nothing but a place holder.  If we want
> to have consistency between input and kctl jack items, then yes.
> OTOH, we might want to drop buttons from kctls in anyway, so such a
> consistency has no importance.  Then we may just ignore phantom jacks
> for input jacks but create only phatom jack kctls, too, either by
> adding a new flag to indicate that, or let type=0 behaving like that.

I'm not sure I see the relevancy of buttons here?

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

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

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

[Takashi Iwai]

At Mon, 23 Mar 2015 09:35:43 -0700,
Mark Brown wrote:
> 
> > Now the question is whether we need representing the same item for
> > input jack, even though it's nothing but a place holder.  If we want
> > to have consistency between input and kctl jack items, then yes.
> > OTOH, we might want to drop buttons from kctls in anyway, so such a
> > consistency has no importance.  Then we may just ignore phantom jacks
> > for input jacks but create only phatom jack kctls, too, either by
> > adding a new flag to indicate that, or let type=0 behaving like that.
> 
> I'm not sure I see the relevancy of buttons here?

Well, you pointed that buttons are needed only for input devices, that
implicitly means that we don't need to create kctls for buttons.
That is, it's not guaranteed that all jack items have to be present in
as both input and kctl.  Following this logic, we may make phantom
jacks only appearing in kctls but not in input.


Takashi

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

[Mark Brown]

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

On Mon, Mar 23, 2015 at 05:38:22PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > > Now the question is whether we need representing the same item for
> > > input jack, even though it's nothing but a place holder.  If we want
> > > to have consistency between input and kctl jack items, then yes.
> > > OTOH, we might want to drop buttons from kctls in anyway, so such a
> > > consistency has no importance.  Then we may just ignore phantom jacks
> > > for input jacks but create only phatom jack kctls, too, either by
> > > adding a new flag to indicate that, or let type=0 behaving like that.

> > I'm not sure I see the relevancy of buttons here?

> Well, you pointed that buttons are needed only for input devices, that
> implicitly means that we don't need to create kctls for buttons.

OK, right - if we had a jack that was button only and didn't have any
capability for detecting accessories.  That'd be a bit odd though.

> That is, it's not guaranteed that all jack items have to be present in
> as both input and kctl.  Following this logic, we may make phantom
> jacks only appearing in kctls but not in input.

Right, I understood that bit - I was just confused about the buttons.

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

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

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

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Monday, March 23, 2015 7:57 PM
> To: Tanu Kaskinen
> Cc: Jie, Yang; broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood,
> Liam R; Liam Girdwood
> Subject: Re: [PATCH v3 1/2] ALSA: jack: create jack kcontrols for every jack
> input device
> 
> At Mon, 23 Mar 2015 12:08:17 +0200,
> Tanu Kaskinen wrote:
> >
> > On Sat, 2015-03-21 at 02:22 +0000, Jie, Yang wrote:
> > > > As you'll find through the whole output, many calls are combined
> > > > with
> > > > SND_JACK_BTN_* bits.  And think again how this will result with your
> patch.
> > > > A call like
> > > >
> > > > 	snd_soc_card_jack_new(card, "Headphone",
> > > > 		SND_JACK_HEADPHONE | SND_JACK_BTN_0 |
> SND_JACK_BTN_1, ...)
> > > >
> > > > would give three kctls, all "Headphone Jack" with indices 0, 1 and 2.
> > > > How user-space knows what is for what?
> > > [Keyon] yes, this is an issue I was thinking about, we need
> > > discussing and conclusion about that, including comments from user-
> space, here adding Tanu.
> > > Here I am thinking about
> > > 1. do we really need kctls for buttons(SND_JACK_BTN_n)? if no, that
> > > is simplest, we can filter out them at this kctl creating stage.
> > > 2. if yes, then we need an name regenerator here, to generate
> > > understandable (for user-space) kctl names for buttons.
> >
> > >From PulseAudio point of view, you must make sure that "Headphone
> Jack"
> > with index 0 doesn't refer to a button, because the assumption is that
> > "Headphone Jack" at index 0 refers to a jack and not a button (indexes
> > other than 0 are currently ignored). Other than that, PulseAudio
> > doesn't currently care, because there's no support for buttons.
> >
> > When thinking about how to implement button support in PulseAudio, the
> > first question is that are kcontrols a realiable interface for getting
> > button events? What happens when the button is pressed? Does one
> press
> > cause only one on->off or off->on state transition, or is the state "on"
> > only for the duration of the press? If the former, then kcontrols
> > should be fine, but if the latter, is it guaranteed that the
> > application sees the event? If the button press is a short one,
> > causing a quick
> > off->on->off transition, is it possible that the application gets a
> > notification of a mixer change, but when the application then inspects
> > the mixer state, the button state is already "off", so the application
> > doesn't get enough information about what happened?
> 
> It's a good point.  The notification via kctl can't follow the complete state
> changes since it's nothing but a notification telling "something changed".  So,
> events like buttons that need the complete state change history, the current
> implementation isn't appropriate.
> For things like jacks, we usually care only about the final state, and the state
> change is much less frequent, so kctl notification works well.
[Keyon] Agree that we don't need create kctl for button(just leave it to input
event notification/handler), then I can update patch to filter out them at kctl
creating stage, which may make life easier IMO. :)

BTW, Takashi, according to what Tanu mentioned, kctls indexes other than 0 
are currently ignored by PA, so creating and exporting kctls with non-zero 
indices for the same name in HD-Audio Jack before, seems no use for upper
layer?

> 
> 
> Takashi

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

[Raymond Yau]

> BTW, Takashi, according to what Tanu mentioned, kctls indexes other than 0
> are currently ignored by PA, so creating and exporting kctls with non-zero
> indices for the same name in HD-Audio Jack before, seems no use for upper
> layer?

Does this mean that pulseaudio don't support notebook with dual headphone
jacks since you need to use  jack states of both headphone jacks to
determine the speaker is muted or not even you assign different name to the
headphone jacks

For notebook with dock station ,  headphone jack and dock headphone jack
usually share volume

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

[Takashi Iwai]

At Wed, 25 Mar 2015 04:11:28 +0000,
Jie, Yang wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Monday, March 23, 2015 7:57 PM
> > To: Tanu Kaskinen
> > Cc: Jie, Yang; broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood,
> > Liam R; Liam Girdwood
> > Subject: Re: [PATCH v3 1/2] ALSA: jack: create jack kcontrols for every jack
> > input device
> > 
> > At Mon, 23 Mar 2015 12:08:17 +0200,
> > Tanu Kaskinen wrote:
> > >
> > > On Sat, 2015-03-21 at 02:22 +0000, Jie, Yang wrote:
> > > > > As you'll find through the whole output, many calls are combined
> > > > > with
> > > > > SND_JACK_BTN_* bits.  And think again how this will result with your
> > patch.
> > > > > A call like
> > > > >
> > > > > 	snd_soc_card_jack_new(card, "Headphone",
> > > > > 		SND_JACK_HEADPHONE | SND_JACK_BTN_0 |
> > SND_JACK_BTN_1, ...)
> > > > >
> > > > > would give three kctls, all "Headphone Jack" with indices 0, 1 and 2.
> > > > > How user-space knows what is for what?
> > > > [Keyon] yes, this is an issue I was thinking about, we need
> > > > discussing and conclusion about that, including comments from user-
> > space, here adding Tanu.
> > > > Here I am thinking about
> > > > 1. do we really need kctls for buttons(SND_JACK_BTN_n)? if no, that
> > > > is simplest, we can filter out them at this kctl creating stage.
> > > > 2. if yes, then we need an name regenerator here, to generate
> > > > understandable (for user-space) kctl names for buttons.
> > >
> > > >From PulseAudio point of view, you must make sure that "Headphone
> > Jack"
> > > with index 0 doesn't refer to a button, because the assumption is that
> > > "Headphone Jack" at index 0 refers to a jack and not a button (indexes
> > > other than 0 are currently ignored). Other than that, PulseAudio
> > > doesn't currently care, because there's no support for buttons.
> > >
> > > When thinking about how to implement button support in PulseAudio, the
> > > first question is that are kcontrols a realiable interface for getting
> > > button events? What happens when the button is pressed? Does one
> > press
> > > cause only one on->off or off->on state transition, or is the state "on"
> > > only for the duration of the press? If the former, then kcontrols
> > > should be fine, but if the latter, is it guaranteed that the
> > > application sees the event? If the button press is a short one,
> > > causing a quick
> > > off->on->off transition, is it possible that the application gets a
> > > notification of a mixer change, but when the application then inspects
> > > the mixer state, the button state is already "off", so the application
> > > doesn't get enough information about what happened?
> > 
> > It's a good point.  The notification via kctl can't follow the complete state
> > changes since it's nothing but a notification telling "something changed".  So,
> > events like buttons that need the complete state change history, the current
> > implementation isn't appropriate.
> > For things like jacks, we usually care only about the final state, and the state
> > change is much less frequent, so kctl notification works well.
> [Keyon] Agree that we don't need create kctl for button(just leave it to input
> event notification/handler), then I can update patch to filter out them at kctl
> creating stage, which may make life easier IMO. :)
> 
> BTW, Takashi, according to what Tanu mentioned, kctls indexes other than 0 
> are currently ignored by PA, so creating and exporting kctls with non-zero 
> indices for the same name in HD-Audio Jack before, seems no use for upper
> layer?

No "big" use.  Who knows.

But I don't mind to convert the usage of index number in hda_jack.c to
a unique string instead so that the name can be used for both input
and kctl jacks.


Takashi