[PATCH v6 0/6] ALSA: jack: Refactoring for jack kctls

[PATCH v6 0/6] 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 new implementation, both HDA and soc jack kctls works fine,
they are compatible for old HDA jack kctls, too.

For soc,
a. snd_jack_new() with NULL jack_kctl;
b. for each pin, call snd_jack_add_kctls() to add one kctl(it will
call snd_ctl_add() to add kctl to card);

For HDA,
With snd_jack_new(), we can use phantom_jack = true for phantom jack
creating, and the jack kctl pointer will be filled with the new
created kcontrol.

Changes in v6:
1. refine kctl name generating function;
2. integrate HDA jack kctl more deeply, support phantom jack in
snd_jack_new();
3. add documentation for Jack kcontrols to explain how to use it.

Changes in v5:
1. remove SND_KCTL_JACK config item, we need jack kctl once
CONFIG_SND_JACK is selected. 

Changes in v4:
1. use snd_ctl_find_id() to get avaliable index;
2. add initial kctl for snd_jack_new(compatible for HDA);
3. add struct snd_jack_kctl * field to struct hda_jack_tbl;
4. new kctls for soc jack during jack pins creating.
5. add a patch to remove exporting snd_kctl_jack_new().

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 (6):
  ALSA: jack: create jack kcontrols for every jack input
  ALSA: jack: extend snd_jack_new to support phantom jack
  ALSA: hda - Update to use the new jack kctls method
  ASoC: jack: create kctls according to jack pins info
  ALSA: jack: remove exporting ctljack functions
  ALSA: Docs: Add documentation for Jack kcontrols

 Documentation/sound/alsa/Jack-Controls.txt |  48 +++++++
 include/sound/jack.h                       |  21 +++-
 sound/core/Kconfig                         |   3 -
 sound/core/Makefile                        |   3 +-
 sound/core/ctljack.c                       |   2 -
 sound/core/jack.c                          | 193 +++++++++++++++++++++++++----
 sound/pci/hda/Kconfig                      |   1 -
 sound/pci/hda/hda_jack.c                   |  57 ++++-----
 sound/pci/hda/hda_jack.h                   |   4 +-
 sound/pci/oxygen/xonar_wm87x6.c            |   2 +-
 sound/soc/soc-jack.c                       |   3 +-
 11 files changed, 264 insertions(+), 73 deletions(-)
 create mode 100644 Documentation/sound/alsa/Jack-Controls.txt

-- 
1.9.1

[PATCH v6 1/6] ALSA: jack: create jack kcontrols for every jack input

[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  |  17 +++++-
 sound/core/Kconfig    |   3 --
 sound/core/Makefile   |   3 +-
 sound/core/jack.c     | 141 +++++++++++++++++++++++++++++++++++++++++++++++++-
 sound/pci/hda/Kconfig |   1 -
 5 files changed, 157 insertions(+), 8 deletions(-)

diff --git a/include/sound/jack.h b/include/sound/jack.h
index 2182350..8337000 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,10 +84,19 @@ struct snd_jack {
 	void (*private_free)(struct snd_jack *);
 };
 
+struct snd_jack_kctl {
+	struct snd_kcontrol *kctl;
+	struct list_head list;		/* list of controls belong to the same jack */
+	unsigned int mask_bits;	/* one of the corresponding bits of status change will report to this kctl */
+	void *private_data;
+	void (*private_free)(struct snd_jack_kctl *jack_kctl);
+};
+
 #ifdef CONFIG_SND_JACK
 
 int snd_jack_new(struct snd_card *card, const char *id, int type,
 		 struct snd_jack **jack);
+int snd_jack_add_new_kctls(struct snd_jack *jack, const char * name, int mask);
 void snd_jack_set_parent(struct snd_jack *jack, struct device *parent);
 int snd_jack_set_key(struct snd_jack *jack, enum snd_jack_types type,
 		     int keytype);
@@ -93,13 +104,17 @@ int snd_jack_set_key(struct snd_jack *jack, enum snd_jack_types type,
 void snd_jack_report(struct snd_jack *jack, int status);
 
 #else
-
 static inline int snd_jack_new(struct snd_card *card, const char *id, int type,
 			       struct snd_jack **jack)
 {
 	return 0;
 }
 
+static inline int snd_jack_add_new_kctls(struct snd_jack *jack, const char * name, int mask)
+{
+	return 0;
+}
+
 static inline void snd_jack_set_parent(struct snd_jack *jack,
 				       struct device *parent)
 {
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/core/jack.c b/sound/core/jack.c
index 8658578..a03fc93 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, list) {
+		list_del_init(&jack_kctl->list);
+		snd_ctl_remove(card, jack_kctl->kctl);
+	}
 	if (jack->private_free)
 		jack->private_free(jack);
 
@@ -100,6 +107,127 @@ static int snd_jack_dev_register(struct snd_device *device)
 	return err;
 }
 
+static int snd_jack_get_available_index(struct snd_card *card, const char *name)
+{
+	struct snd_ctl_elem_id sid;
+
+	memset(&sid, 0, sizeof(sid));
+
+	sid.index = 0;
+	sid.iface = SNDRV_CTL_ELEM_IFACE_CARD;
+	strlcpy(sid.name, name, sizeof(sid.name));
+
+	while (snd_ctl_find_id(card, &sid)) {
+		sid.index++;
+	}
+	return sid.index;
+}
+
+static void snd_jack_kctl_private_free(struct snd_kcontrol *kctl)
+{
+	struct snd_jack_kctl *jack_kctl;
+
+	if (kctl) {
+		jack_kctl = kctl->private_data;
+		if (jack_kctl) {
+			if (jack_kctl->private_free)
+				jack_kctl->private_free(jack_kctl);
+			list_del(&jack_kctl->list);
+			kfree(jack_kctl);
+		}
+	}
+
+}
+
+static void snd_jack_kctl_name_gen(char *name, const char *jack_id, int size)
+{
+	size_t count = strlen(jack_id);
+
+	/* remove redundant " Jack" from jack_id */
+	if (count >= 5)
+		count = strncmp(&jack_id[count - 5], " Jack", 5) ? count : count - 5;
+
+	count = (size > count) ? count + 1 : size;
+	/* name[count] will be filled to '\0' */
+	strlcpy(name, jack_id, count);
+
+}
+
+static void snd_jack_kctl_add(struct snd_jack *jack, struct snd_jack_kctl *jack_kctl)
+{
+	list_add_tail(&jack_kctl->list, &jack->kctl_list);
+}
+
+/**
+ * snd_jack_kctl_new - Create a new snd_jack_kctl and return it
+ * @card:  the card instance
+ * @kctl_name:  the name for the snd_kcontrol object
+ * @mask:  a bitmask of enum snd_jack_type values that can be detected
+ *         by this snd_jack_kctl object.
+ *
+ * Creates a new snd_kcontrol object, and assign it to the new created
+ *	     snd_jack_kctl object.
+ *
+ * Return: The new created snd_jack_kctl object, or NULL if failed.
+ */
+static struct snd_jack_kctl * snd_jack_kctl_new(struct snd_card *card, const char *name, unsigned int mask)
+{
+	struct snd_kcontrol *kctl;
+	struct snd_jack_kctl *jack_kctl;
+	int index, err;
+	char jack_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
+
+	index = snd_jack_get_available_index(card, name);
+	snd_jack_kctl_name_gen(jack_name, name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
+	kctl = snd_kctl_jack_new(jack_name, index, card);
+	if (!kctl)
+		return NULL;
+
+	jack_kctl = kzalloc(sizeof(*jack_kctl), GFP_KERNEL);
+
+	if (!jack_kctl)
+		goto error;
+
+	jack_kctl->kctl = kctl;
+	jack_kctl->mask_bits = mask;
+
+	kctl->private_data = jack_kctl;
+	kctl->private_free = snd_jack_kctl_private_free;
+
+	err = snd_ctl_add(card, jack_kctl->kctl);
+	if (err < 0)
+		goto error;
+	return jack_kctl;
+error:
+	snd_ctl_free_one(kctl);
+	return NULL;
+}
+
+/**
+ * snd_jack_add_new_kctls - Create a new snd_jack_kctl and add it to jack
+ * @jack:  the jack instance which the kctl will attaching to
+ * @name:  the name for the snd_kcontrol object
+ * @mask:  a bitmask of enum snd_jack_type values that can be detected
+ *         by this snd_jack_kctl object.
+ *
+ * Creates a new snd_kcontrol object, and assign it to the new created
+ *	     snd_jack_kctl object, then add it to the jack kctl_list.
+ *
+ * Return: Zero if successful, or a negative error code on failure.
+ */
+int snd_jack_add_new_kctls(struct snd_jack *jack, const char * name, int mask)
+{
+	struct snd_jack_kctl *jack_kctl;
+
+	jack_kctl = snd_jack_kctl_new(jack->card, name, mask);
+	if (!jack_kctl)
+		return -ENOMEM;
+
+	snd_jack_kctl_add(jack, jack_kctl);
+	return 0;
+}
+EXPORT_SYMBOL(snd_jack_add_new_kctls);
+
 /**
  * snd_jack_new - Create a new jack
  * @card:  the card instance
@@ -150,6 +278,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
 	if (err < 0)
 		goto fail_input;
 
+	jack->card = card;
+	INIT_LIST_HEAD(&jack->kctl_list);
+
 	*jjack = jack;
 
 	return 0;
@@ -230,6 +361,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 +377,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, list) {
+		snd_kctl_jack_report(jack->card, jack_kctl->kctl,
+					status & jack_kctl->mask_bits);
+	}
+
 }
 EXPORT_SYMBOL(snd_jack_report);
 
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
index 7f0f2c5..363f365 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -4,7 +4,6 @@ config SND_HDA
 	tristate
 	select SND_PCM
 	select SND_VMASTER
-	select SND_KCTL_JACK
 
 config SND_HDA_INTEL
 	tristate "HD Audio PCI"
-- 
1.9.1

[PATCH v6 2/6] ALSA: jack: extend snd_jack_new to support phantom jack

[Jie Yang]

For phantom jack, we won't create real jack device, but jack kctl
may be needed.

Here, we extend snd_jack_new() to support phantom jack creating:
pass in a bool param for [non-]phantom flag, and a non-Null param
(struct snd_jack_kctl **) to indicate that we need create kctl
at this jack creating stage.

We can also add kctl to a jack after the jack is created.

This make the integrating the existing HDA jack kctl and soc jack
kctl possible.

Signed-off-by: Jie Yang <yang.jie@intel.com>
---
 include/sound/jack.h            |  4 +--
 sound/core/jack.c               | 56 +++++++++++++++++++++++++----------------
 sound/pci/hda/hda_jack.c        |  2 +-
 sound/pci/oxygen/xonar_wm87x6.c |  2 +-
 sound/soc/soc-jack.c            |  2 +-
 5 files changed, 39 insertions(+), 27 deletions(-)

diff --git a/include/sound/jack.h b/include/sound/jack.h
index 8337000..11d772f 100644
--- a/include/sound/jack.h
+++ b/include/sound/jack.h
@@ -95,7 +95,7 @@ struct snd_jack_kctl {
 #ifdef CONFIG_SND_JACK
 
 int snd_jack_new(struct snd_card *card, const char *id, int type,
-		 struct snd_jack **jack);
+		 struct snd_jack **jack, bool phantom_jack, struct snd_jack_kctl **jjack_kctl);
 int snd_jack_add_new_kctls(struct snd_jack *jack, const char * name, int mask);
 void snd_jack_set_parent(struct snd_jack *jack, struct device *parent);
 int snd_jack_set_key(struct snd_jack *jack, enum snd_jack_types type,
@@ -105,7 +105,7 @@ void snd_jack_report(struct snd_jack *jack, int status);
 
 #else
 static inline int snd_jack_new(struct snd_card *card, const char *id, int type,
-			       struct snd_jack **jack)
+			       struct snd_jack **jack, bool phantom_jack, struct snd_jack_kctl **jjack_kctl)
 {
 	return 0;
 }
diff --git a/sound/core/jack.c b/sound/core/jack.c
index a03fc93..7dbbd7d 100644
--- a/sound/core/jack.c
+++ b/sound/core/jack.c
@@ -235,6 +235,10 @@ EXPORT_SYMBOL(snd_jack_add_new_kctls);
  * @type:  a bitmask of enum snd_jack_type values that can be detected by
  *         this jack
  * @jjack: Used to provide the allocated jack object to the caller.
+ * @phantom_jack: for phantom jack, only create needed kctl, won't create
+ *         real jackdevice
+ * @jjack_kctl: create kctl if non-NULL pointer passed in, and provide it to
+ *         the caller. also add it to the non-phantom jack kctl list
  *
  * Creates a new jack object.
  *
@@ -242,7 +246,7 @@ EXPORT_SYMBOL(snd_jack_add_new_kctls);
  * On success @jjack will be initialised.
  */
 int snd_jack_new(struct snd_card *card, const char *id, int type,
-		 struct snd_jack **jjack)
+		 struct snd_jack **jjack, bool phantom_jack, struct snd_jack_kctl **jjack_kctl)
 {
 	struct snd_jack *jack;
 	int err;
@@ -253,35 +257,43 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
 		.dev_disconnect = snd_jack_dev_disconnect,
 	};
 
-	jack = kzalloc(sizeof(struct snd_jack), GFP_KERNEL);
-	if (jack == NULL)
-		return -ENOMEM;
+	if (jjack_kctl)
+		*jjack_kctl = snd_jack_kctl_new(card, id, type);
 
-	jack->id = kstrdup(id, GFP_KERNEL);
+	/* don't creat real jack device for phantom jack */
+	if (!phantom_jack) {
+		jack = kzalloc(sizeof(struct snd_jack), GFP_KERNEL);
+		if (jack == NULL)
+			return -ENOMEM;
 
-	jack->input_dev = input_allocate_device();
-	if (jack->input_dev == NULL) {
-		err = -ENOMEM;
-		goto fail_input;
-	}
+		jack->id = kstrdup(id, GFP_KERNEL);
+
+		jack->input_dev = input_allocate_device();
+		if (jack->input_dev == NULL) {
+			err = -ENOMEM;
+			goto fail_input;
+		}
 
-	jack->input_dev->phys = "ALSA";
+		jack->input_dev->phys = "ALSA";
 
-	jack->type = type;
+		jack->type = type;
 
-	for (i = 0; i < SND_JACK_SWITCH_TYPES; i++)
-		if (type & (1 << i))
-			input_set_capability(jack->input_dev, EV_SW,
-					     jack_switch_types[i]);
+		for (i = 0; i < SND_JACK_SWITCH_TYPES; i++)
+			if (type & (1 << i))
+				input_set_capability(jack->input_dev, EV_SW,
+						     jack_switch_types[i]);
 
-	err = snd_device_new(card, SNDRV_DEV_JACK, jack, &ops);
-	if (err < 0)
-		goto fail_input;
+		err = snd_device_new(card, SNDRV_DEV_JACK, jack, &ops);
+		if (err < 0)
+			goto fail_input;
 
-	jack->card = card;
-	INIT_LIST_HEAD(&jack->kctl_list);
+		jack->card = card;
+		INIT_LIST_HEAD(&jack->kctl_list);
 
-	*jjack = jack;
+		if (jjack_kctl && *jjack_kctl)
+			snd_jack_kctl_add(jack, *jjack_kctl);
+		*jjack = jack;
+	}
 
 	return 0;
 
diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
index e664307..2136670 100644
--- a/sound/pci/hda/hda_jack.c
+++ b/sound/pci/hda/hda_jack.c
@@ -417,7 +417,7 @@ static int __snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid,
 	if (!phantom_jack) {
 		jack->type = get_input_jack_type(codec, nid);
 		err = snd_jack_new(codec->bus->card, name, jack->type,
-				   &jack->jack);
+				   &jack->jack, false, NULL);
 		if (err < 0)
 			return err;
 		jack->jack->private_data = jack;
diff --git a/sound/pci/oxygen/xonar_wm87x6.c b/sound/pci/oxygen/xonar_wm87x6.c
index 6ce6860..a37450b 100644
--- a/sound/pci/oxygen/xonar_wm87x6.c
+++ b/sound/pci/oxygen/xonar_wm87x6.c
@@ -286,7 +286,7 @@ static void xonar_ds_init(struct oxygen *chip)
 	xonar_enable_output(chip);
 
 	snd_jack_new(chip->card, "Headphone",
-		     SND_JACK_HEADPHONE, &data->hp_jack);
+		     SND_JACK_HEADPHONE, &data->hp_jack, false, NULL);
 	xonar_ds_handle_hp_jack(chip);
 
 	snd_component_add(chip->card, "WM8776");
diff --git a/sound/soc/soc-jack.c b/sound/soc/soc-jack.c
index 9f60c25..836368b 100644
--- a/sound/soc/soc-jack.c
+++ b/sound/soc/soc-jack.c
@@ -48,7 +48,7 @@ int snd_soc_card_jack_new(struct snd_soc_card *card, const char *id, int type,
 	INIT_LIST_HEAD(&jack->jack_zones);
 	BLOCKING_INIT_NOTIFIER_HEAD(&jack->notifier);
 
-	ret = snd_jack_new(card->snd_card, id, type, &jack->jack);
+	ret = snd_jack_new(card->snd_card, id, type, &jack->jack, false, NULL);
 	if (ret)
 		return ret;
 
-- 
1.9.1

[PATCH v6 3/6] ALSA: hda - Update to use the new jack kctls method

[Jie Yang]

In the new jack kctls design, snd_kcontrol can be created
during snd_jack_new(), or calling snd_jack_add_new_kctls()
to create and attach to the created jack later.

Here we create jack kctls at the jack creating stage, for
both phantom/non-phantom jack.

Signed-off-by: Jie Yang <yang.jie@intel.com>
---
 sound/pci/hda/hda_jack.c | 57 ++++++++++++++++++------------------------------
 sound/pci/hda/hda_jack.h |  4 +---
 2 files changed, 22 insertions(+), 39 deletions(-)

diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
index 2136670..52f4a4b 100644
--- a/sound/pci/hda/hda_jack.c
+++ b/sound/pci/hda/hda_jack.c
@@ -132,11 +132,11 @@ 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 +337,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_kctl || 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)
 {
@@ -371,13 +367,12 @@ static int get_input_jack_type(struct hda_codec *codec, hda_nid_t nid)
 	}
 }
 
-static void hda_free_jack_priv(struct snd_jack *jack)
+static void hda_free_jack_priv(struct snd_jack_kctl *jack_kctl)
 {
-	struct hda_jack_tbl *jacks = jack->private_data;
+	struct hda_jack_tbl *jacks = jack_kctl->private_data;
 	jacks->nid = 0;
 	jacks->jack = NULL;
 }
-#endif
 
 /**
  * snd_hda_jack_add_kctl - Add a kctl for the given pin
@@ -394,37 +389,27 @@ 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;
+	int err, state, type;
 
 	jack = snd_hda_jack_tbl_new(codec, nid);
 	if (!jack)
 		return 0;
-	if (jack->kctl)
+	if (jack->jack_kctl)
 		return 0; /* already created */
-	kctl = snd_kctl_jack_new(name, idx, codec);
-	if (!kctl)
-		return -ENOMEM;
-	err = snd_hda_ctl_add(codec, nid, kctl);
+
+	type = get_input_jack_type(codec, nid);
+	err = snd_jack_new(codec->bus->card, name, type,
+			   &jack->jack, phantom_jack, &jack->jack_kctl);
 	if (err < 0)
 		return err;
-	jack->kctl = kctl;
-	jack->phantom_jack = !!phantom_jack;
 
+	jack->phantom_jack = !!phantom_jack;
+	jack->type = type;
+	jack->jack_kctl->private_data = jack;
+	jack->jack_kctl->private_free = hda_free_jack_priv;
 	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,
-				   &jack->jack, false, NULL);
-		if (err < 0)
-			return err;
-		jack->jack->private_data = jack;
-		jack->jack->private_free = hda_free_jack_priv;
-		snd_jack_report(jack->jack, state ? jack->type : 0);
-	}
-#endif
+	snd_jack_report(jack->jack, state ? jack->type : 0);
+
 	return 0;
 }
 
@@ -453,10 +438,10 @@ static int get_unique_index(struct hda_codec *codec, const char *name, int idx)
 	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) {
+		if (jack->jack_kctl &&
+		    !strncmp(name, jack->jack_kctl->kctl->id.name, len) &&
+		    !strcmp(" Jack", jack->jack_kctl->kctl->id.name + len) &&
+		    jack->jack_kctl->kctl->id.index == idx) {
 			idx++;
 			goto again;
 		}
diff --git a/sound/pci/hda/hda_jack.h b/sound/pci/hda/hda_jack.h
index b279e32..346a639 100644
--- a/sound/pci/hda/hda_jack.h
+++ b/sound/pci/hda/hda_jack.h
@@ -39,11 +39,9 @@ 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_kctl *jack_kctl;
 	struct snd_jack *jack;
-#endif
 };
 
 struct hda_jack_tbl *
-- 
1.9.1

[PATCH v6 4/6] ASoC: jack: create kctls according to jack pins info

[Jie Yang]

In the new jack kctls design, we can create kctls according to
pins info:

1. during jack creating, snd_jack_new() with NULL jack_kctl;
2. for each pin, call snd_jack_add_kctls() to add one kctl
(it will call snd_ctl_add() to add kctl to card);

Signed-off-by: Jie Yang <yang.jie@intel.com>
---
 sound/soc/soc-jack.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/soc-jack.c b/sound/soc/soc-jack.c
index 836368b..44b3c8c 100644
--- a/sound/soc/soc-jack.c
+++ b/sound/soc/soc-jack.c
@@ -197,6 +197,7 @@ int snd_soc_jack_add_pins(struct snd_soc_jack *jack, int count,
 
 		INIT_LIST_HEAD(&pins[i].list);
 		list_add(&(pins[i].list), &jack->pins);
+		snd_jack_add_new_kctls(jack->jack, pins[i].pin, pins[i].mask);
 	}
 
 	/* Update to reflect the last reported status; canned jack
-- 
1.9.1

[PATCH v6 5/6] ALSA: jack: remove exporting ctljack functions

[Jie Yang]

After created jack embedded kctls and removed hda jack kctls, the funcs
snd_kctl_jack_new() and snd_kctl_jack_report() are merely internal called,
so here remove exporting them.

Signed-off-by: Jie Yang <yang.jie@intel.com>
---
 sound/core/ctljack.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/core/ctljack.c b/sound/core/ctljack.c
index e4b38fb..7a271f5 100644
--- a/sound/core/ctljack.c
+++ b/sound/core/ctljack.c
@@ -43,7 +43,6 @@ snd_kctl_jack_new(const char *name, int idx, void *private_data)
 	kctl->private_value = 0;
 	return kctl;
 }
-EXPORT_SYMBOL_GPL(snd_kctl_jack_new);
 
 void snd_kctl_jack_report(struct snd_card *card,
 			  struct snd_kcontrol *kctl, bool status)
@@ -53,4 +52,3 @@ void snd_kctl_jack_report(struct snd_card *card,
 	kctl->private_value = status;
 	snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
 }
-EXPORT_SYMBOL_GPL(snd_kctl_jack_report);
-- 
1.9.1

[PATCH v6 6/6] ALSA: Docs: Add documentation for Jack kcontrols

[Jie Yang]

Add documentation describing Jack embedded kcontrols, and how to
use it on HD-Audio and ASoC case.

Signed-off-by: Jie Yang <yang.jie@intel.com>
---
 Documentation/sound/alsa/Jack-Controls.txt | 48 ++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/sound/alsa/Jack-Controls.txt

diff --git a/Documentation/sound/alsa/Jack-Controls.txt b/Documentation/sound/alsa/Jack-Controls.txt
new file mode 100644
index 0000000..9e93947
--- /dev/null
+++ b/Documentation/sound/alsa/Jack-Controls.txt
@@ -0,0 +1,48 @@
+Why we need kcontrols for Jack?
+===============================
+
+ALSA using kcontrols to export controlling(switch, volume, Mux, ...) to
+user space, e.g. pulseaudio can switch off headphone and switch on
+speaker when no heaphone is pluged in.
+
+For each physical jack, there may be dynamic pluged in/out status which
+upper layer may be interested in. 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.
+
+So we create embeded kcontrols for each jack, report jack event via
+these kcontrols, userspace switch them to ack on these event, to
+implement what they are wanna to.
+
+Combining with ucm machanism, userpace can do fantasic controlling as
+they want to.
+
+Jack and kcontrols
+==================
+
+Each jack will have a kcontrol list, we can create a kcontrol and attach
+it to the jack, at jack creating stage. Also, we can add kcontrol to an
+exist jack, at anytime when you are happy.
+
+Those kcontrols will be freed automatically when the Jack is freed.
+
+How to create kcontrol embedded jack?
+=====================================
+
+To be compatible with previous HD-Audio jack and ASoC jack, we modify
+snd_jack_new() with adding two params:
+
+ - @phantom_jack: for phantom jack, only create needed kctl, won't create
+	real jack device.
+ - @jjack_kctl: create kctl if non-NULL pointer passed in, and provide
+	it to the caller. also add it to the non-phantom jack kctl list.
+
+For HDA jack, we can use phantom_jack = true for phantom jack creating,
+and the jack kctl pointer will be filled with the new created kcontrol.
+
+For ASoC jack, usually, we create kcontrols after jack is created, with
+calling snd_hda_jack_add_kctl() for each jack pins. And, the pin name
+will be assigned to the corresponding kcontrol name. So, to make
+userspace understand and recognize it, please assign proper name for
+each jack pin.
+
-- 
1.9.1

Re: [PATCH v6 1/6] ALSA: jack: create jack kcontrols for every jack input

[Takashi Iwai]

At Sat, 18 Apr 2015 18:04:15 +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.

Please give more description what you really changed.  It's not only
doing the additional kctl creation.  Also, a brief introduction about
what will follow in the patchset would be nice for readers.

> 
> 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  |  17 +++++-
>  sound/core/Kconfig    |   3 --
>  sound/core/Makefile   |   3 +-
>  sound/core/jack.c     | 141 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  sound/pci/hda/Kconfig |   1 -
>  5 files changed, 157 insertions(+), 8 deletions(-)
> 
> diff --git a/include/sound/jack.h b/include/sound/jack.h
> index 2182350..8337000 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,10 +84,19 @@ struct snd_jack {
>  	void (*private_free)(struct snd_jack *);
>  };
>  
> +struct snd_jack_kctl {
> +	struct snd_kcontrol *kctl;
> +	struct list_head list;		/* list of controls belong to the same jack */
> +	unsigned int mask_bits;	/* one of the corresponding bits of status change will report to this kctl */
> +	void *private_data;
> +	void (*private_free)(struct snd_jack_kctl *jack_kctl);
> +};

You don't have to expose this.  The hda_jack.c doesn't actually need
these whole bits, as it supposes 1:1 as jack:kctl mapping.  You can
get rid of hda_jack->kctl since snd_jack_report() does it already,
thus the private_data and the destructor can be dropped, too.

That said, the kctl management internal isn't the thing the driver
needs to care.


>  #ifdef CONFIG_SND_JACK
>  
>  int snd_jack_new(struct snd_card *card, const char *id, int type,
>  		 struct snd_jack **jack);
> +int snd_jack_add_new_kctls(struct snd_jack *jack, const char * name, int mask);

It adds a single kctl.  Using a plural (kctl"s") is confusing.


> +static int snd_jack_get_available_index(struct snd_card *card, const char *name)
> +{
> +	struct snd_ctl_elem_id sid;
> +
> +	memset(&sid, 0, sizeof(sid));
> +
> +	sid.index = 0;
> +	sid.iface = SNDRV_CTL_ELEM_IFACE_CARD;
> +	strlcpy(sid.name, name, sizeof(sid.name));
> +
> +	while (snd_ctl_find_id(card, &sid)) {
> +		sid.index++;
> +	}

Drop braces for a single line loop.

> +	return sid.index;
> +}

IMO, this function should be rather put to ctljack.c.  It's kctl
specific, and not really defining the jack implementation itself.


> +
> +static void snd_jack_kctl_private_free(struct snd_kcontrol *kctl)
> +{
> +	struct snd_jack_kctl *jack_kctl;
> +
> +	if (kctl) {

This NULL check is superfluous.

> +		jack_kctl = kctl->private_data;
> +		if (jack_kctl) {
> +			if (jack_kctl->private_free)
> +				jack_kctl->private_free(jack_kctl);
> +			list_del(&jack_kctl->list);
> +			kfree(jack_kctl);
> +		}
> +	}
> +
> +}
> +
> +static void snd_jack_kctl_name_gen(char *name, const char *jack_id, int size)
> +{
> +	size_t count = strlen(jack_id);
> +
> +	/* remove redundant " Jack" from jack_id */
> +	if (count >= 5)
> +		count = strncmp(&jack_id[count - 5], " Jack", 5) ? count : count - 5;
> +
> +	count = (size > count) ? count + 1 : size;
> +	/* name[count] will be filled to '\0' */
> +	strlcpy(name, jack_id, count);
> +}

This function should be moved to ctljack.c.

That is, split the code finding a unique index and the removal of
superfluous jack suffix as another patch.  There fold that code into
snd_kctl_jack_new() itself.  At the same time, the code doing the same
thing in hda_jack.c can be removed.  Then the index argument of
snd_kctl_jack_new() can be dropped.

As a bonus, you can save the stack size, too, by using a single string
buffer for the name string.  "Jack" suffix manipulation can be
simplified there.

> +
> +static void snd_jack_kctl_add(struct snd_jack *jack, struct snd_jack_kctl *jack_kctl)
> +{
> +	list_add_tail(&jack_kctl->list, &jack->kctl_list);
> +}
> +
> +/**
> + * snd_jack_kctl_new - Create a new snd_jack_kctl and return it
> + * @card:  the card instance
> + * @kctl_name:  the name for the snd_kcontrol object
> + * @mask:  a bitmask of enum snd_jack_type values that can be detected
> + *         by this snd_jack_kctl object.
> + *
> + * Creates a new snd_kcontrol object, and assign it to the new created
> + *	     snd_jack_kctl object.
> + *
> + * Return: The new created snd_jack_kctl object, or NULL if failed.
> + */
> +static struct snd_jack_kctl * snd_jack_kctl_new(struct snd_card *card, const char *name, unsigned int mask)
> +{
> +	struct snd_kcontrol *kctl;
> +	struct snd_jack_kctl *jack_kctl;
> +	int index, err;
> +	char jack_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> +
> +	index = snd_jack_get_available_index(card, name);
> +	snd_jack_kctl_name_gen(jack_name, name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
> +	kctl = snd_kctl_jack_new(jack_name, index, card);
> +	if (!kctl)
> +		return NULL;
> +
> +	jack_kctl = kzalloc(sizeof(*jack_kctl), GFP_KERNEL);
> +
> +	if (!jack_kctl)
> +		goto error;
> +
> +	jack_kctl->kctl = kctl;
> +	jack_kctl->mask_bits = mask;
> +
> +	kctl->private_data = jack_kctl;
> +	kctl->private_free = snd_jack_kctl_private_free;
> +
> +	err = snd_ctl_add(card, jack_kctl->kctl);
> +	if (err < 0)
> +		goto error;
> +	return jack_kctl;
> +error:
> +	snd_ctl_free_one(kctl);

This will be a double-free.  snd_ctl_add() itself frees the object at
error path.


Takashi

Re: [PATCH v6 1/6] ALSA: jack: create jack kcontrols for every jack input

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Sunday, April 19, 2015 4:26 PM
> To: Jie, Yang
> Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood, Liam R;
> tanu.kaskinen@linux.intel.com; Liam Girdwood
> Subject: Re: [PATCH v6 1/6] ALSA: jack: create jack kcontrols for every jack
> input
> 
> At Sat, 18 Apr 2015 18:04:15 +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.
> 
> Please give more description what you really changed.  It's not only doing the
> additional kctl creation.  Also, a brief introduction about what will follow in
> the patchset would be nice for readers.
 
Ah, forgot to update this as it has been change a lot from the v1, will do that.

> 
> >
> > 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  |  17 +++++-
> >  sound/core/Kconfig    |   3 --
> >  sound/core/Makefile   |   3 +-
> >  sound/core/jack.c     | 141
> +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  sound/pci/hda/Kconfig |   1 -
> >  5 files changed, 157 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/sound/jack.h b/include/sound/jack.h index
> > 2182350..8337000 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,10 +84,19 @@ struct snd_jack {
> >  	void (*private_free)(struct snd_jack *);  };
> >
> > +struct snd_jack_kctl {
> > +	struct snd_kcontrol *kctl;
> > +	struct list_head list;		/* list of controls belong to the same
> jack */
> > +	unsigned int mask_bits;	/* one of the corresponding bits of
> status change will report to this kctl */
> > +	void *private_data;
> > +	void (*private_free)(struct snd_jack_kctl *jack_kctl); };
> 
> You don't have to expose this.  The hda_jack.c doesn't actually need these
> whole bits, as it supposes 1:1 as jack:kctl mapping.  You can get rid of
> hda_jack->kctl since snd_jack_report() does it already, thus the private_data
> and the destructor can be dropped, too.
 
The patch 3/6 removes had_jack->kctl, here I added priate destructor for HDA
jack to reset hda_jack_tbl jacks->nid and jacks->jack to 0.
Seems these are needed for HDA jack logic, otherwise, I can already dropped
them. :(

> 
> That said, the kctl management internal isn't the thing the driver needs to
> care.
> 
> 
> >  #ifdef CONFIG_SND_JACK
> >
> >  int snd_jack_new(struct snd_card *card, const char *id, int type,
> >  		 struct snd_jack **jack);
> > +int snd_jack_add_new_kctls(struct snd_jack *jack, const char * name,
> > +int mask);
> 
> It adds a single kctl.  Using a plural (kctl"s") is confusing.

OK, will change to 'kctl'.

> 
> 
> > +static int snd_jack_get_available_index(struct snd_card *card, const
> > +char *name) {
> > +	struct snd_ctl_elem_id sid;
> > +
> > +	memset(&sid, 0, sizeof(sid));
> > +
> > +	sid.index = 0;
> > +	sid.iface = SNDRV_CTL_ELEM_IFACE_CARD;
> > +	strlcpy(sid.name, name, sizeof(sid.name));
> > +
> > +	while (snd_ctl_find_id(card, &sid)) {
> > +		sid.index++;
> > +	}
> 
> Drop braces for a single line loop.
 
OK.

> 
> > +	return sid.index;
> > +}
> 
> IMO, this function should be rather put to ctljack.c.  It's kctl specific, and not
> really defining the jack implementation itself.
 
Good idea, will consider this in next version.

> 
> 
> > +
> > +static void snd_jack_kctl_private_free(struct snd_kcontrol *kctl) {
> > +	struct snd_jack_kctl *jack_kctl;
> > +
> > +	if (kctl) {
> 
> This NULL check is superfluous.

OK, will remove it. 

> 
> > +		jack_kctl = kctl->private_data;
> > +		if (jack_kctl) {
> > +			if (jack_kctl->private_free)
> > +				jack_kctl->private_free(jack_kctl);
> > +			list_del(&jack_kctl->list);
> > +			kfree(jack_kctl);
> > +		}
> > +	}
> > +
> > +}
> > +
> > +static void snd_jack_kctl_name_gen(char *name, const char *jack_id,
> > +int size) {
> > +	size_t count = strlen(jack_id);
> > +
> > +	/* remove redundant " Jack" from jack_id */
> > +	if (count >= 5)
> > +		count = strncmp(&jack_id[count - 5], " Jack", 5) ? count :
> count -
> > +5;
> > +
> > +	count = (size > count) ? count + 1 : size;
> > +	/* name[count] will be filled to '\0' */
> > +	strlcpy(name, jack_id, count);
> > +}
> 
> This function should be moved to ctljack.c.
> 
> That is, split the code finding a unique index and the removal of superfluous
> jack suffix as another patch.  There fold that code into
 
Agree, will implement this in next version.

> snd_kctl_jack_new() itself.  At the same time, the code doing the same thing
> in hda_jack.c can be removed.  Then the index argument of
 
A little concern that there may be a little difference here:
hda_jack.c:get_unique_index() check the codec kctl list,
but in my patch we check card kctl list, I am afraid that it may influence the
exist HDA usage, wo just keep what HDA did before for HDA jack.

Takashi, do you think it's a risk to change from codec kctl list to card kctl list for
HDA part? 

> snd_kctl_jack_new() can be dropped.
> 
> As a bonus, you can save the stack size, too, by using a single string buffer for
> the name string.  "Jack" suffix manipulation can be simplified there.
 
Agree.

> 
> > +
> > +static void snd_jack_kctl_add(struct snd_jack *jack, struct
> > +snd_jack_kctl *jack_kctl) {
> > +	list_add_tail(&jack_kctl->list, &jack->kctl_list); }
> > +
> > +/**
> > + * snd_jack_kctl_new - Create a new snd_jack_kctl and return it
> > + * @card:  the card instance
> > + * @kctl_name:  the name for the snd_kcontrol object
> > + * @mask:  a bitmask of enum snd_jack_type values that can be detected
> > + *         by this snd_jack_kctl object.
> > + *
> > + * Creates a new snd_kcontrol object, and assign it to the new created
> > + *	     snd_jack_kctl object.
> > + *
> > + * Return: The new created snd_jack_kctl object, or NULL if failed.
> > + */
> > +static struct snd_jack_kctl * snd_jack_kctl_new(struct snd_card
> > +*card, const char *name, unsigned int mask) {
> > +	struct snd_kcontrol *kctl;
> > +	struct snd_jack_kctl *jack_kctl;
> > +	int index, err;
> > +	char jack_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> > +
> > +	index = snd_jack_get_available_index(card, name);
> > +	snd_jack_kctl_name_gen(jack_name, name,
> SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
> > +	kctl = snd_kctl_jack_new(jack_name, index, card);
> > +	if (!kctl)
> > +		return NULL;
> > +
> > +	jack_kctl = kzalloc(sizeof(*jack_kctl), GFP_KERNEL);
> > +
> > +	if (!jack_kctl)
> > +		goto error;
> > +
> > +	jack_kctl->kctl = kctl;
> > +	jack_kctl->mask_bits = mask;
> > +
> > +	kctl->private_data = jack_kctl;
> > +	kctl->private_free = snd_jack_kctl_private_free;
> > +
> > +	err = snd_ctl_add(card, jack_kctl->kctl);
> > +	if (err < 0)
> > +		goto error;
> > +	return jack_kctl;
> > +error:
> > +	snd_ctl_free_one(kctl);
> 
> This will be a double-free.  snd_ctl_add() itself frees the object at error path.
 
Thanks for reminding. Snd_ctl_free_one() for the first goto is needed, I will
remove it for the second goto.

> 
> 
> Takashi

Re: [PATCH v6 1/6] ALSA: jack: create jack kcontrols for every jack input

[Takashi Iwai]

At Mon, 20 Apr 2015 07:52:20 +0000,
Jie, Yang wrote:
> 
> > > @@ -82,10 +84,19 @@ struct snd_jack {
> > >  	void (*private_free)(struct snd_jack *);  };
> > >
> > > +struct snd_jack_kctl {
> > > +	struct snd_kcontrol *kctl;
> > > +	struct list_head list;		/* list of controls belong to the same
> > jack */
> > > +	unsigned int mask_bits;	/* one of the corresponding bits of
> > status change will report to this kctl */
> > > +	void *private_data;
> > > +	void (*private_free)(struct snd_jack_kctl *jack_kctl); };
> > 
> > You don't have to expose this.  The hda_jack.c doesn't actually need these
> > whole bits, as it supposes 1:1 as jack:kctl mapping.  You can get rid of
> > hda_jack->kctl since snd_jack_report() does it already, thus the private_data
> > and the destructor can be dropped, too.
>  
> The patch 3/6 removes had_jack->kctl, here I added priate destructor for HDA
> jack to reset hda_jack_tbl jacks->nid and jacks->jack to 0.
> Seems these are needed for HDA jack logic, otherwise, I can already dropped
> them. :(

It's not needed, as far as I understand your code.

Try to look from from the top-level POV: the hda driver just needs to
register a jack object via snd_jack_new(), reports it, and remove it
in destructor.  The private_data was needed for tracking the
codec-local kctls, but now they are managed in jack object itself,
thus we don't have to keep eyes on it.  The private_free is for
removing the kctl list, and since this is gone, we don't have to take
it, either.  In other words, we don't have to take care of kctls at
all in hda driver side.  This would be the ideal situation.

(snip)
> > snd_kctl_jack_new() itself.  At the same time, the code doing the same thing
> > in hda_jack.c can be removed.  Then the index argument of
>  
> A little concern that there may be a little difference here:
> hda_jack.c:get_unique_index() check the codec kctl list,
> but in my patch we check card kctl list, I am afraid that it may influence the
> exist HDA usage, wo just keep what HDA did before for HDA jack.
> 
> Takashi, do you think it's a risk to change from codec kctl list to card kctl list for
> HDA part? 

No, there shouldn't.  The id must be unique for the whole card, not
specific to codecs.  That is, the code looking through
snd_ctl_find_id() is even safer than the current version.


thanks,

Takashi

Re: [PATCH v6 1/6] ALSA: jack: create jack kcontrols for every jack input

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Monday, April 20, 2015 4:06 PM
> To: Jie, Yang
> Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood, Liam R;
> tanu.kaskinen@linux.intel.com; Liam Girdwood
> Subject: Re: [PATCH v6 1/6] ALSA: jack: create jack kcontrols for every jack
> input
> 
> At Mon, 20 Apr 2015 07:52:20 +0000,
> Jie, Yang wrote:
> >
> > > > @@ -82,10 +84,19 @@ struct snd_jack {
> > > >  	void (*private_free)(struct snd_jack *);  };
> > > >
> > > > +struct snd_jack_kctl {
> > > > +	struct snd_kcontrol *kctl;
> > > > +	struct list_head list;		/* list of controls belong to the same
> > > jack */
> > > > +	unsigned int mask_bits;	/* one of the corresponding bits of
> > > status change will report to this kctl */
> > > > +	void *private_data;
> > > > +	void (*private_free)(struct snd_jack_kctl *jack_kctl); };
> > >
> > > You don't have to expose this.  The hda_jack.c doesn't actually need
> > > these whole bits, as it supposes 1:1 as jack:kctl mapping.  You can
> > > get rid of hda_jack->kctl since snd_jack_report() does it already,
> > > thus the private_data and the destructor can be dropped, too.
> >
> > The patch 3/6 removes had_jack->kctl, here I added priate destructor
> > for HDA jack to reset hda_jack_tbl jacks->nid and jacks->jack to 0.
> > Seems these are needed for HDA jack logic, otherwise, I can already
> > dropped them. :(
> 
> It's not needed, as far as I understand your code.
> 
> Try to look from from the top-level POV: the hda driver just needs to register
> a jack object via snd_jack_new(), reports it, and remove it in destructor.  The
> private_data was needed for tracking the codec-local kctls, but now they are
> managed in jack object itself, thus we don't have to keep eyes on it.  The
> private_free is for removing the kctl list, and since this is gone, we don't have
> to take it, either.  In other words, we don't have to take care of kctls at all in
> hda driver side.  This would be the ideal situation.
 
Thanks for explaining it. Finally I make clear that jack private_free will call
hda_free_jack_priv() to reset jacks->nid/jack, so I have no worry now. :)

> 
> (snip)
> > > snd_kctl_jack_new() itself.  At the same time, the code doing the
> > > same thing in hda_jack.c can be removed.  Then the index argument of
> >
> > A little concern that there may be a little difference here:
> > hda_jack.c:get_unique_index() check the codec kctl list, but in my
> > patch we check card kctl list, I am afraid that it may influence the
> > exist HDA usage, wo just keep what HDA did before for HDA jack.
> >
> > Takashi, do you think it's a risk to change from codec kctl list to
> > card kctl list for HDA part?
> 
> No, there shouldn't.  The id must be unique for the whole card, not specific
> to codecs.  That is, the code looking through
> snd_ctl_find_id() is even safer than the current version.
 
OK, then I feel reassured to replace it. :)

> 
> 
> thanks,
> 
> Takashi

Re: [PATCH v6 4/6] ASoC: jack: create kctls according to jack pins info

[Mark Brown]

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

On Sat, Apr 18, 2015 at 06:04:18PM +0800, Jie Yang wrote:
> In the new jack kctls design, we can create kctls according to
> pins info:
> 
> 1. during jack creating, snd_jack_new() with NULL jack_kctl;
> 2. for each pin, call snd_jack_add_kctls() to add one kctl
> (it will call snd_ctl_add() to add kctl to card);

I'm a little confused why this isn't done by the jack core?

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

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

Re: [PATCH v6 4/6] ASoC: jack: create kctls according to jack pins info

[Jie, Yang]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Tuesday, April 21, 2015 5:21 AM
> To: Jie, Yang
> Cc: tiwai@suse.de; alsa-devel@alsa-project.org; Girdwood, Liam R;
> tanu.kaskinen@linux.intel.com
> Subject: Re: [PATCH v6 4/6] ASoC: jack: create kctls according to jack pins info
> 
> On Sat, Apr 18, 2015 at 06:04:18PM +0800, Jie Yang wrote:
> > In the new jack kctls design, we can create kctls according to pins
> > info:
> >
> > 1. during jack creating, snd_jack_new() with NULL jack_kctl; 2. for
> > each pin, call snd_jack_add_kctls() to add one kctl (it will call
> > snd_ctl_add() to add kctl to card);
> 
> I'm a little confused why this isn't done by the jack core?
 
The kctl creating and adding to jack is actually done by the jack core(inside
snd_jack_add_kctls()).

Mark, do you mean that you prefer doing this inside snd_jack_new()?

For we may need create multiple kctls for a jack, and I found we can reuse
the pins' info for those kctls' creating. 

For example, in broadwell.c, the jack broadwell_headset have 2 pins:
"Mic Jack" and "Headphone Jack", then 2 kctls with same names will be
created, and they are visible for PA, can be switched on/off by PA separately.

~Keyon