[alsa-devel] [PATCH v4 0/5] ALSA: jack: Refactoring for jack kctls
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, a. call snd_jack_kctl_new()(in __snd_hda_jack_add_kctl()) to new a jack_kctl(also new kctl); and use snd_hda_ctl_add to add kctl to card; b. new a hda_jack_tbl, appoint jack_tbl->jack_kctl = jack_kctl; c. for non-phantom jack, call snd_jack_new() with this jack_kctl, then the jack_kctl will be added to the new created jack;
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 (5): ALSA: jack: create jack kcontrols for every jack input ALSA: jack: add a parameter to pass kctl for snd_jack_new ALSA: hda - Update to use the new jack kctls method ASoC: jack: create kctls according to jack pins info ALSA: jack: remove export snd_kctl_jack_new()
include/sound/jack.h | 25 ++++++- sound/core/ctljack.c | 1 - sound/core/jack.c | 152 +++++++++++++++++++++++++++++++++++++++- sound/pci/hda/hda_jack.c | 52 +++++++------- sound/pci/hda/hda_jack.h | 4 +- sound/pci/oxygen/xonar_wm87x6.c | 2 +- sound/soc/soc-jack.c | 3 +- 7 files changed, 200 insertions(+), 39 deletions(-)
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 | 21 ++++++++ sound/core/jack.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 161 insertions(+), 1 deletion(-)
diff --git a/include/sound/jack.h b/include/sound/jack.h index 2182350..07f7035 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,20 @@ 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
+struct snd_jack_kctl * snd_jack_kctl_new(struct snd_card *card, const char *kctl_name, unsigned int mask); 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,6 +105,10 @@ 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 struct snd_jack_kctl * snd_jack_kctl_new(struct snd_card *card, const char *kctl_name, unsigned int mask) +{ + return NULL; +}
static inline int snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jack) @@ -100,6 +116,11 @@ static inline int snd_jack_new(struct snd_card *card, const char *id, int type, 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/jack.c b/sound/core/jack.c index 8658578..c74111f 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,97 @@ 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 (strstr(jack_id, " Jack")) + 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. + */ +struct snd_jack_kctl * snd_jack_kctl_new(struct snd_card *card, const char *kctl_name, unsigned int mask) +{ + struct snd_kcontrol *kctl; + struct snd_jack_kctl *jack_kctl; + int index; + + index = snd_jack_get_available_index(card, kctl_name); + kctl = snd_kctl_jack_new(kctl_name, index, card); + if (!kctl) + return NULL; + + jack_kctl = kzalloc(sizeof(*jack_kctl), GFP_KERNEL); + + if (!jack_kctl) { + kfree(kctl);//needed? + return NULL; + } + + jack_kctl->kctl = kctl; + jack_kctl->mask_bits = mask; + + kctl->private_data = jack_kctl; + kctl->private_free = snd_jack_kctl_private_free; + + return jack_kctl; +} +EXPORT_SYMBOL(snd_jack_kctl_new); + /** * snd_jack_new - Create a new jack * @card: the card instance @@ -163,6 +261,39 @@ fail_input: EXPORT_SYMBOL(snd_jack_new);
/** + * 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; + int err; + char jack_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN]; + + snd_jack_kctl_name_gen(jack_name, name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN); + + jack_kctl = snd_jack_kctl_new(jack->card, jack_name, mask); + if (!jack_kctl) + return -ENOMEM; + + err = snd_ctl_add(jack->card, jack_kctl->kctl); + if (err < 0) + return err; + + snd_jack_kctl_add(jack, jack_kctl); + return 0; +} +EXPORT_SYMBOL(snd_jack_add_new_kctls); + +/** * snd_jack_set_parent - Set the parent device for a jack * * @jack: The jack to configure @@ -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);
Currentlly, for HDA jack, we will create jack kctl before calling snd_jack_new(). In our new design, this kctl should belong to the jack, so we add a param(struct snd_jack_kctl *) to pass the kctl, then we can add the kctl to jack during jack creating.
This make our design more compatible with existing HDA jack kctl.
Signed-off-by: Jie Yang yang.jie@intel.com --- include/sound/jack.h | 4 ++-- sound/core/jack.c | 11 +++++++++-- sound/pci/hda/hda_jack.c | 2 +- sound/pci/oxygen/xonar_wm87x6.c | 2 +- sound/soc/soc-jack.c | 2 +- 5 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/include/sound/jack.h b/include/sound/jack.h index 07f7035..5b580f8 100644 --- a/include/sound/jack.h +++ b/include/sound/jack.h @@ -96,7 +96,7 @@ struct snd_jack_kctl {
struct snd_jack_kctl * snd_jack_kctl_new(struct snd_card *card, const char *kctl_name, unsigned int mask); int snd_jack_new(struct snd_card *card, const char *id, int type, - struct snd_jack **jack); + struct snd_jack **jack, struct snd_jack_kctl *jack_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, @@ -111,7 +111,7 @@ static inline struct snd_jack_kctl * snd_jack_kctl_new(struct snd_card *card, co }
static inline int snd_jack_new(struct snd_card *card, const char *id, int type, - struct snd_jack **jack) + struct snd_jack **jack, struct snd_jack_kctl *jack_kctl) { return 0; } diff --git a/sound/core/jack.c b/sound/core/jack.c index c74111f..b6e7be1 100644 --- a/sound/core/jack.c +++ b/sound/core/jack.c @@ -205,6 +205,8 @@ EXPORT_SYMBOL(snd_jack_kctl_new); * @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. + * @kctl: if kctl != NULL, it means we don't need create new kcontrol for this + * jack, just assign it to the jack. * * Creates a new jack object. * @@ -212,7 +214,7 @@ EXPORT_SYMBOL(snd_jack_kctl_new); * 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, struct snd_jack_kctl *jack_kctl) { struct snd_jack *jack; int err; @@ -236,7 +238,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++) @@ -248,6 +250,11 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, if (err < 0) goto fail_input;
+ INIT_LIST_HEAD(&jack->kctl_list); + + if (jack_kctl) + snd_jack_kctl_add(jack, jack_kctl); + *jjack = jack;
return 0; diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c index e664307..9397250 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, 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..d30ec63 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, 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..3c87e32 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, NULL); if (ret) return ret;
In the new jack kctls design, snd_kcontrol is created during snd_jack_kctl creating, then, we can wrap it in struct hda_jack_tbl. The new method to create HDA jack kctl as below:
1. call snd_jack_kctl_new()(in __snd_hda_jack_add_kctl()) to new a jack_kctl(also new kctl), and use snd_hda_ctl_add to add kctl to card; 2. new a hda_jack_tbl, appoint jack_kctl to jack_tbl->jack_kctl; 3. for non-phantom jack, call snd_jack_new() with this jack_kctl, then the jack_kctl will be added to the new created jack;
Signed-off-by: Jie Yang yang.jie@intel.com --- sound/pci/hda/hda_jack.c | 52 ++++++++++++++++++++++-------------------------- sound/pci/hda/hda_jack.h | 4 +--- 2 files changed, 25 insertions(+), 31 deletions(-)
diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c index 9397250..9a1ba35 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,38 @@ 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; + struct snd_jack_kctl *jack_kctl; + 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) + + type = get_input_jack_type(codec, nid); + jack_kctl = snd_jack_kctl_new(codec->bus->card, name, type); + if (!jack_kctl) return -ENOMEM; - err = snd_hda_ctl_add(codec, nid, kctl); + + err = snd_hda_ctl_add(codec, nid, jack_kctl->kctl); if (err < 0) return err; - jack->kctl = kctl; + jack->jack_kctl = jack_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 + snd_kctl_jack_report(codec->bus->card, jack_kctl->kctl, state); if (!phantom_jack) { - jack->type = get_input_jack_type(codec, nid); + jack->type = type; err = snd_jack_new(codec->bus->card, name, jack->type, - &jack->jack, NULL); + &jack->jack, jack_kctl); if (err < 0) return err; - jack->jack->private_data = jack; - jack->jack->private_free = hda_free_jack_priv; + jack->jack_kctl->private_data = jack; + jack->jack_kctl->private_free = hda_free_jack_priv; snd_jack_report(jack->jack, state ? jack->type : 0); } -#endif return 0; }
@@ -453,10 +449,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->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 *
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 3c87e32..7099222 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
After created jack embedded kctls and removed hda jack kctls, the func snd_kctl_jack_new() is merely internal called, so here remove exporting it.
Signed-off-by: Jie Yang yang.jie@intel.com --- sound/core/ctljack.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/core/ctljack.c b/sound/core/ctljack.c index e4b38fb..8702fb1 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)
On Fri, Apr 03, 2015 at 08:07:13PM +0800, Jie Yang wrote:
Currently only hda will create kctls for hda jacks, for asoc, people may need create jack kctls in specific driver.
Sorry, you'll have got some pushed messages for this - I applied it locally to have a look at it and pushed it by mistake.
At Fri, 3 Apr 2015 20:07:13 +0800, 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 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);
So, if I understand correctly, there is no association between input jack and ctl jack names at all? The former is created via snd_jack_new() while the latter is created from each pin name string.
For hda, a. call snd_jack_kctl_new()(in __snd_hda_jack_add_kctl()) to new a jack_kctl(also new kctl); and use snd_hda_ctl_add to add kctl to card; b. new a hda_jack_tbl, appoint jack_tbl->jack_kctl = jack_kctl; c. for non-phantom jack, call snd_jack_new() with this jack_kctl, then the jack_kctl will be added to the new created jack;
This would work, yes. But, I have some uneasy feeling, something not well digested...
Ideally, we want a single API for representing both input and kctl jacks. Now, with this implementation, essentially it's still two API calls -- snd_jack_new() for input jacks and snd_jack_kctl_new() for kctl jacks. (Actually oxygen driver still seems lacking kctl jack because of this.) The obvious difference from the current code is that there is an internal association from kctls to input-jack object (although the relation is invisible in outside). Hmm...
thanks,
Takashi
Changes in v4:
- use snd_ctl_find_id() to get avaliable index;
- add initial kctl for snd_jack_new(compatible for HDA);
- add struct snd_jack_kctl * field to struct hda_jack_tbl;
- new kctls for soc jack during jack pins creating.
- add a patch to remove exporting snd_kctl_jack_new().
Changes in v3:
- replace bit index with bit mask in jack_kctl;
- 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:
- define jack_kctl struct, and put jack kctl related stuff there;
- add a patch to remove the existing controls for HDA jack.
Jie Yang (5): ALSA: jack: create jack kcontrols for every jack input ALSA: jack: add a parameter to pass kctl for snd_jack_new ALSA: hda - Update to use the new jack kctls method ASoC: jack: create kctls according to jack pins info ALSA: jack: remove export snd_kctl_jack_new()
include/sound/jack.h | 25 ++++++- sound/core/ctljack.c | 1 - sound/core/jack.c | 152 +++++++++++++++++++++++++++++++++++++++- sound/pci/hda/hda_jack.c | 52 +++++++------- sound/pci/hda/hda_jack.h | 4 +- sound/pci/oxygen/xonar_wm87x6.c | 2 +- sound/soc/soc-jack.c | 3 +- 7 files changed, 200 insertions(+), 39 deletions(-)
-- 1.9.1
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, April 08, 2015 12:07 AM To: Jie, Yang Cc: alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R Subject: Re: [alsa-devel] [PATCH v4 0/5] ALSA: jack: Refactoring for jack kctls
At Fri, 3 Apr 2015 20:07:13 +0800, 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 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);
So, if I understand correctly, there is no association between input jack and ctl jack names at all? The former is created via snd_jack_new() while the latter is created from each pin name string.
Yes, I finally thought out this solution, it provide enough flexibility for jack kctl Naming -- leave it to ASoC specific driver, each machine driver can define its jacks and kctl names for each jack(via jack pins name).
For hda, a. call snd_jack_kctl_new()(in __snd_hda_jack_add_kctl()) to new a jack_kctl(also new kctl); and use snd_hda_ctl_add to add kctl to card; b. new a hda_jack_tbl, appoint jack_tbl->jack_kctl = jack_kctl; c. for non-phantom jack, call snd_jack_new() with this jack_kctl, then the jack_kctl will be added to the new created jack;
This would work, yes. But, I have some uneasy feeling, something not well digested...
I thought out this solution, aiming to change the exist HDA jack kctl logic smallest, which may be safest for compatibility.
And I tested on some HDA platforms, the created kctls are totally same with those generated by the existed upstream code.
And, the worst case I can imagine, is that we will fallback: don't change any HDA related code(not apply patch 3/5: ALSA: hda - Update to use the new jack kctls method), then HDA input jack keep totally same with before.
Ideally, we want a single API for representing both input and kctl jacks. Now, with this implementation, essentially it's still two API calls -- snd_jack_new() for input jacks and snd_jack_kctl_new() for kctl jacks. (Actually oxygen
For jack creating, we use the same API -- snd_jack_new();
For kctl creating, yes, we use different APIs: snd_jack_kctl_new() for input jacks(HDA), snd_jack_add_kctls() for kctl jacks(ASoC).
There are 2 reasons that I made them different: 1. a. for HDA phantom jack, in the old/exist logic, __snd_hda_jack_add_kctl() will also call snd_kctl_jack_new() and snd_hda_ctl_add(), it will create kctls and add them to card(also assigning some arrays, they are different with calling snd_ctl_add() only, which is what we will do for ASoC kctl adding); b. for HDA input/phantom jack, all of those occurs before calling snd_jack_new().
So, we have to split jack new and kctl new functions here, because for HDA phantom/input jack and ASoC kctl jack, they are quite different here.
2. we may need generate kctl name for ASoC kctls(snd_jack_kctl_name_gen(), removing suffix " Jack" as you proposed before, or everything needed after); but for HDA input jack kctls, the naming has been finished before calling __snd_hda_jack_add_kctl(), we should not change them anymore.
driver still seems lacking kctl jack because of this.) The obvious difference from the current code is that there is an internal association from kctls to input-jack object (although the relation is invisible in outside). Hmm...
Yes, in my solution, we provide two jack kctls creating method: 1. create kctl --> [created jack with this kctl] --> [add more kctls to the jack] ... 2. create a jack --> [add kctls to the jack]...
HDA input jack use the 1st method, and ASoC kctl jack use the 2nd method. And, this is only my suggested usage for HDA and ASoC now, we can change it anytime needed. What the most important is, we provided these two methods and enough flexibility...
thanks, ~Keyon
thanks,
Takashi
Changes in v4:
- 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:
- 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:
- 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 (5): ALSA: jack: create jack kcontrols for every jack input ALSA: jack: add a parameter to pass kctl for snd_jack_new ALSA: hda - Update to use the new jack kctls method ASoC: jack: create kctls according to jack pins info ALSA: jack: remove export snd_kctl_jack_new()
include/sound/jack.h | 25 ++++++- sound/core/ctljack.c | 1 - sound/core/jack.c | 152
+++++++++++++++++++++++++++++++++++++++-
sound/pci/hda/hda_jack.c | 52 +++++++------- sound/pci/hda/hda_jack.h | 4 +- sound/pci/oxygen/xonar_wm87x6.c | 2 +- sound/soc/soc-jack.c | 3 +- 7 files changed, 200 insertions(+), 39 deletions(-)
-- 1.9.1
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
2015-4-8 上午12:07 於 "Takashi Iwai" tiwai@suse.de 寫道：
At Fri, 3 Apr 2015 20:07:13 +0800, 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 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);
So, if I understand correctly, there is no association between input jack and ctl jack names at all? The former is created via snd_jack_new() while the latter is created from each pin name string.
For hda, a. call snd_jack_kctl_new()(in __snd_hda_jack_add_kctl()) to new a jack_kctl(also new kctl); and use snd_hda_ctl_add to add kctl to card; b. new a hda_jack_tbl, appoint jack_tbl->jack_kctl = jack_kctl; c. for non-phantom jack, call snd_jack_new() with this jack_kctl, then the jack_kctl will be added to the new created jack;
This would work, yes. But, I have some uneasy feeling, something not well digested...
Ideally, we want a single API for representing both input and kctl jacks. Now, with this implementation, essentially it's still two API calls -- snd_jack_new() for input jacks and snd_jack_kctl_new() for kctl jacks. (Actually oxygen driver still seems lacking kctl jack because of this.) The obvious difference from the current code is that there is an internal association from kctls to input-jack object (although the relation is invisible in outside). Hmm...
Do pulseaudio need oxygen change from input device to kctl since there is no headphone playback volume and pulseaudio drop the support multi channel volume control recently ?
Card hw:0 'DS'/'Asus Virtuoso 66 (rev 2) at 0xe800, irq 16' Mixer name : 'AV200' Components : 'WM8776 WM8766 AV200' Controls : 31 Simple ctrls : 25 Simple mixer control 'Master',0 Capabilities: pvolume pswitch pswitch-joined penum Playback channels: Front Left - Front Right - Rear Left - Rear Right - Front Center - Woofer - Side Left - Side Right Limits: Playback 135 - 255 Mono: Front Left: Playback 255 [100%] [0.00dB] [on] Front Right: Playback 255 [100%] [0.00dB] [on] Rear Left: Playback 255 [100%] [0.00dB] [on] Rear Right: Playback 255 [100%] [0.00dB] [on] Front Center: Playback 255 [100%] [0.00dB] [on] Woofer: Playback 255 [100%] [0.00dB] [on] Side Left: Playback 255 [100%] [0.00dB] [on] Side Right: Playback 255 [100%] [0.00dB] [on] Simple mixer control 'Headphone',0 Capabilities: pvolume pswitch pswitch-joined penum Playback channels: Front Left - Front Right Limits: Playback 61 - 127 Mono: Front Left: Playback 127 [100%] [on] Front Right: Playback 127 [100%] [on]
On 2015-04-07 18:06, Takashi Iwai wrote:
This would work, yes. But, I have some uneasy feeling, something not well digested...
Ideally, we want a single API for representing both input and kctl jacks.
Maybe this is somewhat my fault for steering Yang in that direction. But the requirements are somewhat different.
HDA has the phantom jacks, and the exact naming for each kctl requirements. ASoC has the combination/button requirements, i e one jack can represent more than one kctl.
The phantom jack requirement means that the snd_kctl_jack_new API cannot be removed straight away; we could move it to be internal to HDA (it's not much code anyway), but I don't see a need for that.
But the HDA code can be moved around to look like this:
if (phantom_jack) { snd_kctl_jack_new(); } else { snd_jack_new(); snd_jack_add_new_kctls(); }
Now the HDA looks more like the ASoC variant. Yang, what do you think about that? That would make the API simpler, wouldn't it?
At Wed, 08 Apr 2015 09:20:56 +0200, David Henningsson wrote:
On 2015-04-07 18:06, Takashi Iwai wrote:
This would work, yes. But, I have some uneasy feeling, something not well digested...
Ideally, we want a single API for representing both input and kctl jacks.
Maybe this is somewhat my fault for steering Yang in that direction. But the requirements are somewhat different.
HDA has the phantom jacks, and the exact naming for each kctl requirements. ASoC has the combination/button requirements, i e one jack can represent more than one kctl.
The phantom jack requirement means that the snd_kctl_jack_new API cannot be removed straight away; we could move it to be internal to HDA (it's not much code anyway), but I don't see a need for that.
But the HDA code can be moved around to look like this:
if (phantom_jack) { snd_kctl_jack_new(); } else { snd_jack_new(); snd_jack_add_new_kctls(); }Now the HDA looks more like the ASoC variant. Yang, what do you think about that? That would make the API simpler, wouldn't it?
Well, what I thought was rather to allow snd_jack_new() creating a phantom jack, too, with some flag. When a phantom flag is set, it creates no input jack device but only kctl jacks.
Takashi
On 2015-04-08 09:34, Takashi Iwai wrote:
At Wed, 08 Apr 2015 09:20:56 +0200, David Henningsson wrote:
On 2015-04-07 18:06, Takashi Iwai wrote:
This would work, yes. But, I have some uneasy feeling, something not well digested...
Ideally, we want a single API for representing both input and kctl jacks.
Maybe this is somewhat my fault for steering Yang in that direction. But the requirements are somewhat different.
HDA has the phantom jacks, and the exact naming for each kctl requirements. ASoC has the combination/button requirements, i e one jack can represent more than one kctl.
The phantom jack requirement means that the snd_kctl_jack_new API cannot be removed straight away; we could move it to be internal to HDA (it's not much code anyway), but I don't see a need for that.
But the HDA code can be moved around to look like this:
if (phantom_jack) { snd_kctl_jack_new(); } else { snd_jack_new(); snd_jack_add_new_kctls(); }Now the HDA looks more like the ASoC variant. Yang, what do you think about that? That would make the API simpler, wouldn't it?
Well, what I thought was rather to allow snd_jack_new() creating a phantom jack, too, with some flag. When a phantom flag is set, it creates no input jack device but only kctl jacks.
Yes, that sounds even better - that would make it easy for non-HDA drivers to create phantom jacks too, if they want.
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Wednesday, April 08, 2015 3:21 PM To: Takashi Iwai; Jie, Yang Cc: alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R Subject: Re: [alsa-devel] [PATCH v4 0/5] ALSA: jack: Refactoring for jack kctls
On 2015-04-07 18:06, Takashi Iwai wrote:
This would work, yes. But, I have some uneasy feeling, something not well digested...
Ideally, we want a single API for representing both input and kctl jacks.
Maybe this is somewhat my fault for steering Yang in that direction. But the requirements are somewhat different.
HDA has the phantom jacks, and the exact naming for each kctl requirements. ASoC has the combination/button requirements, i e one jack can represent more than one kctl.
The phantom jack requirement means that the snd_kctl_jack_new API cannot be removed straight away; we could move it to be internal to HDA (it's not much code anyway), but I don't see a need for that.
But the HDA code can be moved around to look like this:
if (phantom_jack) { snd_kctl_jack_new();
Let me add line here to clarify: + snd_hda_ctl_add();
} else { snd_jack_new(); snd_jack_add_new_kctls();
I thought of this at the beginning, as you talk below, it looks more like what we did for ASoC case.
What I concern here is that it may make input jack kctl looks like a little different with before(I am not sure, maybe you guys are more clearly about what will occurs without calling snd_ctl_add()-->snd_array_new()...)?
}Now the HDA looks more like the ASoC variant. Yang, what do you think about that? That would make the API simpler, wouldn't it?
Here repeat what I stated in another reply:
For jack creating, we use the same API -- snd_jack_new();
For kctl creating, yes, we use different APIs: snd_jack_kctl_new() for input jacks(HDA), snd_jack_add_kctls() for kctl jacks(ASoC).
There are 2 reasons that I made them different: 1. a. for HDA phantom jack, in the old/exist logic, __snd_hda_jack_add_kctl() will also call snd_kctl_jack_new() and snd_hda_ctl_add(), it will create kctls and add them to card(also assigning some arrays, they are different with calling snd_ctl_add() only, which is what we will do for ASoC kctl adding); b. for HDA input/phantom jack, all of those occurs before calling snd_jack_new().
So, we have to split jack new and kctl new functions here, because for HDA phantom/input jack and ASoC kctl jack, they are quite different here.
2. we may need generate kctl name for ASoC kctls(snd_jack_kctl_name_gen(), removing suffix " Jack" as you proposed before, or everything needed after); but for HDA input jack kctls, the naming has been finished before calling __snd_hda_jack_add_kctl(), we should not change them anymore.
Thanks, ~Keyon
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
On 2015-04-08 09:59, Jie, Yang wrote:
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Wednesday, April 08, 2015 3:21 PM To: Takashi Iwai; Jie, Yang Cc: alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R Subject: Re: [alsa-devel] [PATCH v4 0/5] ALSA: jack: Refactoring for jack kctls
On 2015-04-07 18:06, Takashi Iwai wrote:
This would work, yes. But, I have some uneasy feeling, something not well digested...
Ideally, we want a single API for representing both input and kctl jacks.
Maybe this is somewhat my fault for steering Yang in that direction. But the requirements are somewhat different.
HDA has the phantom jacks, and the exact naming for each kctl requirements. ASoC has the combination/button requirements, i e one jack can represent more than one kctl.
The phantom jack requirement means that the snd_kctl_jack_new API cannot be removed straight away; we could move it to be internal to HDA (it's not much code anyway), but I don't see a need for that.
But the HDA code can be moved around to look like this:
if (phantom_jack) { snd_kctl_jack_new();Let me add line here to clarify:
snd_hda_ctl_add();} else { snd_jack_new(); snd_jack_add_new_kctls();I thought of this at the beginning, as you talk below, it looks more like what we did for ASoC case.
What I concern here is that it may make input jack kctl looks like a little different with before(I am not sure, maybe you guys are more clearly about what will occurs without calling snd_ctl_add()-->snd_array_new()...)?
I'm not sure I understand this question. Maybe the answers below help answering it?
}Now the HDA looks more like the ASoC variant. Yang, what do you think about that? That would make the API simpler, wouldn't it?
Here repeat what I stated in another reply:
For jack creating, we use the same API -- snd_jack_new();
For kctl creating, yes, we use different APIs: snd_jack_kctl_new() for input jacks(HDA), snd_jack_add_kctls() for kctl jacks(ASoC).
There are 2 reasons that I made them different:
- a. for HDA phantom jack, in the old/exist logic, __snd_hda_jack_add_kctl()
will also call snd_kctl_jack_new() and snd_hda_ctl_add(), it will create kctls and add them to card(also assigning some arrays, they are different with calling snd_ctl_add() only, which is what we will do for ASoC kctl adding);
Actually, now that I look at snd_hda_ctl_add, I don't know why we need to call it for HDA jacks. There does not seem to be anything relevant for HDA jacks there. I think we can just call snd_ctl_add for HDA jacks too.
With your refactoring, all we need to remember is to remove the jacks (i e both the input device and the kctls) in both snd_hda_codec_reset and snd_hda_codec_free, and I suspect this is already taken care of?
b. for HDA input/phantom jack, all of those occurs before callingsnd_jack_new().
My point is that it would be possible to move the HDA code around so that snd_jack_new is called first. And with Takashi's suggestion of a "create_input_dev" flag, it would be called for both real and phantom jacks.
So, we have to split jack new and kctl new functions here, because for HDA phantom/input jack and ASoC kctl jack, they are quite different here.
- we may need generate kctl name for ASoC kctls(snd_jack_kctl_name_gen(),
removing suffix " Jack" as you proposed before, or everything needed after); but for HDA input jack kctls, the naming has been finished before calling __snd_hda_jack_add_kctl(), we should not change them anymore.
snd_jack_kctl_name_gen seems only to remove a " Jack" suffix. I think we can safely call it for both ASoC and HDA jacks. If a HDA jack ends up being labelled something with " Jack Jack", it's a bug anyway.
(Side note: but you might want to fix snd_jack_kctl_name_gen to make strange names like "My Jacket Computer" not be cropped to "My Jacket Com")
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Wednesday, April 08, 2015 4:28 PM To: Jie, Yang; Takashi Iwai Cc: alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R Subject: Re: [alsa-devel] [PATCH v4 0/5] ALSA: jack: Refactoring for jack kctls
On 2015-04-08 09:59, Jie, Yang wrote:
-----Original Message----- From: David Henningsson [mailto:david.henningsson@canonical.com] Sent: Wednesday, April 08, 2015 3:21 PM To: Takashi Iwai; Jie, Yang Cc: alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R Subject: Re: [alsa-devel] [PATCH v4 0/5] ALSA: jack: Refactoring for jack kctls
On 2015-04-07 18:06, Takashi Iwai wrote:
This would work, yes. But, I have some uneasy feeling, something not well digested...
Ideally, we want a single API for representing both input and kctl jacks.
Maybe this is somewhat my fault for steering Yang in that direction. But the requirements are somewhat different.
HDA has the phantom jacks, and the exact naming for each kctl
requirements.
ASoC has the combination/button requirements, i e one jack can represent more than one kctl.
The phantom jack requirement means that the snd_kctl_jack_new API cannot be removed straight away; we could move it to be internal to HDA (it's not much code anyway), but I don't see a need for that.
But the HDA code can be moved around to look like this:
if (phantom_jack) { snd_kctl_jack_new();Let me add line here to clarify:
snd_hda_ctl_add();} else { snd_jack_new(); snd_jack_add_new_kctls();I thought of this at the beginning, as you talk below, it looks more like what we did for ASoC case.
What I concern here is that it may make input jack kctl looks like a little different with before(I am not sure, maybe you guys are more clearly about what will occurs without calling snd_ctl_add()-- snd_array_new()...)?
I'm not sure I understand this question. Maybe the answers below help answering it?
Right.
}Now the HDA looks more like the ASoC variant. Yang, what do you think about that? That would make the API simpler, wouldn't it?
Here repeat what I stated in another reply:
For jack creating, we use the same API -- snd_jack_new();
For kctl creating, yes, we use different APIs: snd_jack_kctl_new() for input jacks(HDA), snd_jack_add_kctls() for kctl jacks(ASoC).
There are 2 reasons that I made them different:
- a. for HDA phantom jack, in the old/exist logic,
__snd_hda_jack_add_kctl() will also call snd_kctl_jack_new() and snd_hda_ctl_add(), it will create kctls and add them to card(also assigning some arrays, they are different with calling snd_ctl_add() only, which is what we will do for ASoC kctl adding);
Actually, now that I look at snd_hda_ctl_add, I don't know why we need to call it for HDA jacks. There does not seem to be anything relevant for HDA jacks there. I think we can just call snd_ctl_add for HDA jacks too.
OK, then it may make life easier. Hi Takashi, you agree with this? Do we need add those kctls to the HDA codec, or to hda_nid_item?
With your refactoring, all we need to remember is to remove the jacks (i e both the input device and the kctls) in both snd_hda_codec_reset and snd_hda_codec_free, and I suspect this is already taken care of?
Yes, in my solution, almost nothing change for HDA jack, that is: in snd_hda_codec_reset/free, snd_hda_ctls_clear() is called, where the kctls will be removed; snd_hda_jack_tbl_clear() is called, Where the jacks will be removed.
b. for HDA input/phantom jack, all of those occurs before callingsnd_jack_new().
My point is that it would be possible to move the HDA code around so that snd_jack_new is called first. And with Takashi's suggestion of a "create_input_dev" flag, it would be called for both real and phantom jacks.
I can add this if it is really needed.
So, we have to split jack new and kctl new functions here, because for HDA phantom/input jack and ASoC kctl jack, they are quite different here.
- we may need generate kctl name for ASoC
kctls(snd_jack_kctl_name_gen(), removing suffix " Jack" as you proposed before, or everything needed after); but for HDA input jack kctls, the naming has been finished before calling
__snd_hda_jack_add_kctl(), we should not change them anymore.
snd_jack_kctl_name_gen seems only to remove a " Jack" suffix. I think we can safely call it for both ASoC and HDA jacks. If a HDA jack ends up being labelled something with " Jack Jack", it's a bug anyway.
(Side note: but you might want to fix snd_jack_kctl_name_gen to make strange names like "My Jacket Computer" not be cropped to "My Jacket Com")
Thanks for pointing out this, fixed now.
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
At Wed, 8 Apr 2015 09:18:10 +0000, Jie, Yang wrote:
}Now the HDA looks more like the ASoC variant. Yang, what do you think about that? That would make the API simpler, wouldn't it?
Here repeat what I stated in another reply:
For jack creating, we use the same API -- snd_jack_new();
For kctl creating, yes, we use different APIs: snd_jack_kctl_new() for input jacks(HDA), snd_jack_add_kctls() for kctl jacks(ASoC).
There are 2 reasons that I made them different:
- a. for HDA phantom jack, in the old/exist logic,
__snd_hda_jack_add_kctl() will also call snd_kctl_jack_new() and snd_hda_ctl_add(), it will create kctls and add them to card(also assigning some arrays, they are different with calling snd_ctl_add() only, which is what we will do for ASoC kctl adding);
Actually, now that I look at snd_hda_ctl_add, I don't know why we need to call it for HDA jacks. There does not seem to be anything relevant for HDA jacks there. I think we can just call snd_ctl_add for HDA jacks too.
OK, then it may make life easier. Hi Takashi, you agree with this? Do we need add those kctls to the HDA codec, or to hda_nid_item?
It's been added in the local list to manage the kctls belonging to a codec more easily. But if snd_hda_codec_free() and _reset() can remove them gracefully, there is no big reason to keep tracking there.
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, April 08, 2015 5:22 PM To: Jie, Yang Cc: David Henningsson; alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R Subject: Re: [alsa-devel] [PATCH v4 0/5] ALSA: jack: Refactoring for jack kctls
At Wed, 8 Apr 2015 09:18:10 +0000, Jie, Yang wrote:
}Now the HDA looks more like the ASoC variant. Yang, what do you think about that? That would make the API simpler, wouldn't it?
Here repeat what I stated in another reply:
For jack creating, we use the same API -- snd_jack_new();
For kctl creating, yes, we use different APIs: snd_jack_kctl_new() for input jacks(HDA), snd_jack_add_kctls() for kctl jacks(ASoC).
There are 2 reasons that I made them different:
- a. for HDA phantom jack, in the old/exist logic,
__snd_hda_jack_add_kctl() will also call snd_kctl_jack_new() and snd_hda_ctl_add(), it will create kctls and add them to card(also assigning some arrays, they are different with calling snd_ctl_add() only, which is what we will do for ASoC kctl adding);
Actually, now that I look at snd_hda_ctl_add, I don't know why we need to call it for HDA jacks. There does not seem to be anything relevant for HDA jacks there. I think we can just call snd_ctl_add for HDA
jacks too.
OK, then it may make life easier. Hi Takashi, you agree with this? Do we need add those kctls to the HDA codec, or to hda_nid_item?
It's been added in the local list to manage the kctls belonging to a codec more easily. But if snd_hda_codec_free() and _reset() can remove them gracefully, there is no big reason to keep tracking there.
I am not sure if snd_hda_codec_free() and _reset() can remove them, with removing tracking them in HDA codec in my current patch series. To be more simply and safely, I just keep this old tracking ATM, and we can add patch to optimize them later if needed, do you agree?
Thanks, Keyon
Takashi
At Wed, 8 Apr 2015 14:14:28 +0000, Jie, Yang wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, April 08, 2015 5:22 PM To: Jie, Yang Cc: David Henningsson; alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R Subject: Re: [alsa-devel] [PATCH v4 0/5] ALSA: jack: Refactoring for jack kctls
At Wed, 8 Apr 2015 09:18:10 +0000, Jie, Yang wrote:
}Now the HDA looks more like the ASoC variant. Yang, what do you think about that? That would make the API simpler, wouldn't it?
Here repeat what I stated in another reply:
For jack creating, we use the same API -- snd_jack_new();
For kctl creating, yes, we use different APIs: snd_jack_kctl_new() for input jacks(HDA), snd_jack_add_kctls() for kctl jacks(ASoC).
There are 2 reasons that I made them different:
- a. for HDA phantom jack, in the old/exist logic,
__snd_hda_jack_add_kctl() will also call snd_kctl_jack_new() and snd_hda_ctl_add(), it will create kctls and add them to card(also assigning some arrays, they are different with calling snd_ctl_add() only, which is what we will do for ASoC kctl adding);
Actually, now that I look at snd_hda_ctl_add, I don't know why we need to call it for HDA jacks. There does not seem to be anything relevant for HDA jacks there. I think we can just call snd_ctl_add for HDA
jacks too.
OK, then it may make life easier. Hi Takashi, you agree with this? Do we need add those kctls to the HDA codec, or to hda_nid_item?
It's been added in the local list to manage the kctls belonging to a codec more easily. But if snd_hda_codec_free() and _reset() can remove them gracefully, there is no big reason to keep tracking there.
I am not sure if snd_hda_codec_free() and _reset() can remove them, with removing tracking them in HDA codec in my current patch series. To be more simply and safely, I just keep this old tracking ATM, and we can add patch to optimize them later if needed, do you agree?
Well, this isn't about optimization, but rather relevant with the design -- how more deeply two infrastructures can be integrated.
Takashi
On 2015-04-08 16:14, Jie, Yang wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, April 08, 2015 5:22 PM To: Jie, Yang Cc: David Henningsson; alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R Subject: Re: [alsa-devel] [PATCH v4 0/5] ALSA: jack: Refactoring for jack kctls
At Wed, 8 Apr 2015 09:18:10 +0000, Jie, Yang wrote:
OK, then it may make life easier. Hi Takashi, you agree with this? Do we need add those kctls to the HDA codec, or to hda_nid_item?
It's been added in the local list to manage the kctls belonging to a codec more easily. But if snd_hda_codec_free() and _reset() can remove them gracefully, there is no big reason to keep tracking there.
I am not sure if snd_hda_codec_free() and _reset() can remove them, with removing tracking them in HDA codec in my current patch series. To be more simply and safely, I just keep this old tracking ATM, and we can add patch to optimize them later if needed, do you agree?
Is it possible for the snd_jack object to own its kctls, so when snd_jack_dev_free is called, that would free the kctls too? Or at least have some kind of flag that would allow snd_jack_free to also delete its associated kctls?
(And then snd_hda_codec_reset/free would end up calling snd_jack_dev_free.)
At Wed, 08 Apr 2015 20:47:15 +0200, David Henningsson wrote:
On 2015-04-08 16:14, Jie, Yang wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, April 08, 2015 5:22 PM To: Jie, Yang Cc: David Henningsson; alsa-devel@alsa-project.org; broonie@kernel.org; Girdwood, Liam R Subject: Re: [alsa-devel] [PATCH v4 0/5] ALSA: jack: Refactoring for jack kctls
At Wed, 8 Apr 2015 09:18:10 +0000, Jie, Yang wrote:
OK, then it may make life easier. Hi Takashi, you agree with this? Do we need add those kctls to the HDA codec, or to hda_nid_item?
It's been added in the local list to manage the kctls belonging to a codec more easily. But if snd_hda_codec_free() and _reset() can remove them gracefully, there is no big reason to keep tracking there.
I am not sure if snd_hda_codec_free() and _reset() can remove them, with removing tracking them in HDA codec in my current patch series. To be more simply and safely, I just keep this old tracking ATM, and we can add patch to optimize them later if needed, do you agree?
Is it possible for the snd_jack object to own its kctls, so when snd_jack_dev_free is called, that would free the kctls too? Or at least have some kind of flag that would allow snd_jack_free to also delete its associated kctls?
(And then snd_hda_codec_reset/free would end up calling snd_jack_dev_free.)
Yes, freeing jack table should release the included kctls. We didn't do this in hda_free.c, but it should be done when integrated into jack.c.
Takashi
participants (6)
-
David Henningsson -
Jie Yang -
Jie, Yang
-
Mark Brown -
Raymond Yau -
Takashi Iwai