Message ID | 1426841719-14576-3-git-send-email-yang.jie@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
At Fri, 20 Mar 2015 16:55:18 +0800, Jie Yang wrote: > > Currently the ALSA jack core registers only input devices for each jack > registered. These jack input devices are not readable by userspace devices > that run as non root. > > This patch adds support for additionally registering jack kcontrol devices > for every input jack registered. This allows non root userspace to read > jack status. > > Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> > Modified-by: Jie Yang <yang.jie@intel.com> > Signed-off-by: Jie Yang <yang.jie@intel.com> > Reveiwed-by: Mark Brown <broonie@kernel.org> > --- > include/sound/jack.h | 8 +++++ > sound/core/jack.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 105 insertions(+), 2 deletions(-) > > diff --git a/include/sound/jack.h b/include/sound/jack.h > index 2182350..ef0f0ed 100644 > --- a/include/sound/jack.h > +++ b/include/sound/jack.h > @@ -73,6 +73,8 @@ enum snd_jack_types { > > struct snd_jack { > struct input_dev *input_dev; > + struct list_head kctl_list; > + struct snd_card *card; > int registered; > int type; > const char *id; > @@ -82,6 +84,12 @@ struct snd_jack { > void (*private_free)(struct snd_jack *); > }; > > +struct snd_jack_kctl { > + struct snd_kcontrol *kctl; > + struct list_head jack_list; /* list of controls belong to the same jack*/ > + unsigned int jack_bit_idx; /* the corresponding jack type bit index */ You can omit jack_ prefix here. > +}; > + > #ifdef CONFIG_SND_JACK Remove CONFIG_SND_JACK, both from Kconfig and ifdefs, as now it's always enabled together with CONFIG_SND_JACK. (Or do it later in the patch series.) > int snd_jack_new(struct snd_card *card, const char *id, int type, > diff --git a/sound/core/jack.c b/sound/core/jack.c > index 8658578..741924f 100644 > --- a/sound/core/jack.c > +++ b/sound/core/jack.c > @@ -24,6 +24,7 @@ > #include <linux/module.h> > #include <sound/jack.h> > #include <sound/core.h> > +#include <sound/control.h> > > static int jack_switch_types[SND_JACK_SWITCH_TYPES] = { > SW_HEADPHONE_INSERT, > @@ -54,7 +55,13 @@ static int snd_jack_dev_disconnect(struct snd_device *device) > static int snd_jack_dev_free(struct snd_device *device) > { > struct snd_jack *jack = device->device_data; > + struct snd_card *card = device->card; > + struct snd_jack_kctl *jack_kctl, *tmp_jack_kctl; > > + list_for_each_entry_safe(jack_kctl, tmp_jack_kctl, &jack->kctl_list, jack_list) { > + list_del(&jack_kctl->jack_list); Use list_del_init(). Otherwise it'll strike back. (list_del() will be called again in private_free callback from snd_ctl_remove(), and this can be broken.) > + snd_ctl_remove(card, jack_kctl->kctl); > + } > if (jack->private_free) > jack->private_free(jack); > > @@ -100,6 +107,73 @@ static int snd_jack_dev_register(struct snd_device *device) > return err; > } > > + > +/* get the first unused/available index number for the given kctl name */ > +static int get_available_index(struct snd_card *card, const char *name) > +{ > + struct snd_kcontrol *kctl; > + int idx = 0; > + int len = strlen(name); > + > + down_write(&card->controls_rwsem); Use down_read(), as Liam already suggested. > + next: > + list_for_each_entry(kctl, &card->controls, list) { > + if (!strncmp(name, kctl->id.name, len) && > + !strcmp(" Jack", kctl->id.name + len) && > + kctl->id.index == idx) { > + idx++; > + goto next; > + } > + } Better to split a small function, e.g. while (!jack_ctl_name_found(card, kctl)) idx++; than using ugly goto. > + up_write(&card->controls_rwsem); > + return idx; > +} > + > +static void kctl_private_free(struct snd_kcontrol *kctl) > +{ > + struct snd_jack_kctl *jack_kctl = kctl->private_data; > + list_del(&jack_kctl->jack_list); kfree() is forgotten. > +} > + > +static int snd_jack_new_kctl(struct snd_card *card, struct snd_jack *jack, int type) > +{ > + struct snd_kcontrol *kctl; > + struct snd_jack_kctl *jack_kctl; > + int i, err, index, state = 0 /* use 0 for default state ?? */; > + > + INIT_LIST_HEAD(&jack->kctl_list); > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > + int testbit = 1 << i; > + if (type & testbit) { With this implementation, you'll get multiple boolean kctls for a headset. I thought we agreed with creating a single boolean for headset? In the case of input device, the situation is a bit different, so we shouldn't mix up. > + index = get_available_index(card,jack->id); > + kctl = snd_kctl_jack_new(jack->id, index, card); > + if (!kctl) > + return -ENOMEM; > + > + err = snd_ctl_add(card, kctl); > + if (err < 0) > + return err; > + > + jack_kctl = kzalloc(sizeof(*jack_kctl), GFP_KERNEL); Missing NULL check. > + jack_kctl->kctl = kctl; > + > + kctl->private_data = jack_kctl; > + kctl->private_free = kctl_private_free; > + > + /* use jack_bit_idx for the kctl type bit */ > + jack_kctl->jack_bit_idx = i; This can be better to hold bit mask instead of bit index. Then in the below... > @@ -245,13 +327,26 @@ void snd_jack_report(struct snd_jack *jack, int status) .... > } > > input_sync(jack->input_dev); > + > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > + int testbit = 1 << i; > + if (jack->type & testbit) { > + list_for_each_entry(jack_kctl, &jack->kctl_list, jack_list) { > + if (jack_kctl->jack_bit_idx == i) { > + snd_kctl_jack_report(jack->card, jack_kctl->kctl, > + status & testbit); > + } .... you can reduce to a single loop. thanks, Takashi
At Fri, 20 Mar 2015 10:27:37 +0100, Takashi Iwai wrote: > > > +}; > > + > > #ifdef CONFIG_SND_JACK > > Remove CONFIG_SND_JACK, both from Kconfig and ifdefs, as now it's > always enabled together with CONFIG_SND_JACK. > (Or do it later in the patch series.) Disregard this comment, I was confused about CONFIG_SND_KCTL_JACK. Takashi
> > Currently the ALSA jack core registers only input devices for each jack > registered. These jack input devices are not readable by userspace devices > that run as non root. > > This patch adds support for additionally registering jack kcontrol devices > for every input jack registered. This allows non root userspace to read > jack status. > > Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> > Modified-by: Jie Yang <yang.jie@intel.com> > Signed-off-by: Jie Yang <yang.jie@intel.com> > Reveiwed-by: Mark Brown <broonie@kernel.org> > --- > include/sound/jack.h | 8 +++++ > sound/core/jack.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 105 insertions(+), 2 deletions(-) > > diff --git a/include/sound/jack.h b/include/sound/jack.h > index 2182350..ef0f0ed 100644 > --- a/include/sound/jack.h > +++ b/include/sound/jack.h > @@ -73,6 +73,8 @@ enum snd_jack_types { > > struct snd_jack { > struct input_dev *input_dev; > + struct list_head kctl_list; > + struct snd_card *card; > int registered; > int type; > const char *id; > @@ -82,6 +84,12 @@ struct snd_jack { > void (*private_free)(struct snd_jack *); > }; > > +struct snd_jack_kctl { > + struct snd_kcontrol *kctl; > + struct list_head jack_list; /* list of controls belong to the same jack*/ Does it mean that those multi io jacks (line in and mic jacks of desktop) and those headphone/mic combo jack of netbook have all the input and output kctls in this list ?
> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Friday, March 20, 2015 5:28 PM > To: Jie, Yang > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > Girdwood, Liam R; Liam Girdwood > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > input device > > At Fri, 20 Mar 2015 16:55:18 +0800, > Jie Yang wrote: > > > > Currently the ALSA jack core registers only input devices for each > > jack registered. These jack input devices are not readable by > > userspace devices that run as non root. > > > > This patch adds support for additionally registering jack kcontrol > > devices for every input jack registered. This allows non root > > userspace to read jack status. > > > > Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> > > Modified-by: Jie Yang <yang.jie@intel.com> > > Signed-off-by: Jie Yang <yang.jie@intel.com> > > Reveiwed-by: Mark Brown <broonie@kernel.org> > > --- > > include/sound/jack.h | 8 +++++ > > sound/core/jack.c | 99 > ++++++++++++++++++++++++++++++++++++++++++++++++++-- > > 2 files changed, 105 insertions(+), 2 deletions(-) > > > > diff --git a/include/sound/jack.h b/include/sound/jack.h index > > 2182350..ef0f0ed 100644 > > --- a/include/sound/jack.h > > +++ b/include/sound/jack.h > > @@ -73,6 +73,8 @@ enum snd_jack_types { > > > > struct snd_jack { > > struct input_dev *input_dev; > > + struct list_head kctl_list; > > + struct snd_card *card; > > int registered; > > int type; > > const char *id; > > @@ -82,6 +84,12 @@ struct snd_jack { > > void (*private_free)(struct snd_jack *); }; > > > > +struct snd_jack_kctl { > > + struct snd_kcontrol *kctl; > > + struct list_head jack_list; /* list of controls belong to > the same jack*/ > > + unsigned int jack_bit_idx; /* the corresponding jack type bit > index */ > > You can omit jack_ prefix here. [Keyon] OK. > > > +}; > > + > > #ifdef CONFIG_SND_JACK > > Remove CONFIG_SND_JACK, both from Kconfig and ifdefs, as now it's always > enabled together with CONFIG_SND_JACK. > (Or do it later in the patch series.) > > > int snd_jack_new(struct snd_card *card, const char *id, int type, > > diff --git a/sound/core/jack.c b/sound/core/jack.c index > > 8658578..741924f 100644 > > --- a/sound/core/jack.c > > +++ b/sound/core/jack.c > > @@ -24,6 +24,7 @@ > > #include <linux/module.h> > > #include <sound/jack.h> > > #include <sound/core.h> > > +#include <sound/control.h> > > > > static int jack_switch_types[SND_JACK_SWITCH_TYPES] = { > > SW_HEADPHONE_INSERT, > > @@ -54,7 +55,13 @@ static int snd_jack_dev_disconnect(struct > > snd_device *device) static int snd_jack_dev_free(struct snd_device > > *device) { > > struct snd_jack *jack = device->device_data; > > + struct snd_card *card = device->card; > > + struct snd_jack_kctl *jack_kctl, *tmp_jack_kctl; > > > > + list_for_each_entry_safe(jack_kctl, tmp_jack_kctl, &jack->kctl_list, > jack_list) { > > + list_del(&jack_kctl->jack_list); > > Use list_del_init(). Otherwise it'll strike back. > (list_del() will be called again in private_free callback from snd_ctl_remove(), > and this can be broken.) [Keyon] good! Thanks for pointing out this potential risk! > > > + snd_ctl_remove(card, jack_kctl->kctl); > > + } > > if (jack->private_free) > > jack->private_free(jack); > > > > @@ -100,6 +107,73 @@ static int snd_jack_dev_register(struct snd_device > *device) > > return err; > > } > > > > + > > +/* get the first unused/available index number for the given kctl > > +name */ static int get_available_index(struct snd_card *card, const > > +char *name) { > > + struct snd_kcontrol *kctl; > > + int idx = 0; > > + int len = strlen(name); > > + > > + down_write(&card->controls_rwsem); > > Use down_read(), as Liam already suggested. > > > + next: > > + list_for_each_entry(kctl, &card->controls, list) { > > + if (!strncmp(name, kctl->id.name, len) && > > + !strcmp(" Jack", kctl->id.name + len) && > > + kctl->id.index == idx) { > > + idx++; > > + goto next; > > + } > > + } > > Better to split a small function, e.g. > > while (!jack_ctl_name_found(card, kctl)) > idx++; > > than using ugly goto. [Keyon] OK, will do like that. > > > + up_write(&card->controls_rwsem); > > + return idx; > > +} > > + > > +static void kctl_private_free(struct snd_kcontrol *kctl) { > > + struct snd_jack_kctl *jack_kctl = kctl->private_data; > > + list_del(&jack_kctl->jack_list); > > kfree() is forgotten. [Keyon] right. > > > +} > > + > > +static int snd_jack_new_kctl(struct snd_card *card, struct snd_jack > > +*jack, int type) { > > + struct snd_kcontrol *kctl; > > + struct snd_jack_kctl *jack_kctl; > > + int i, err, index, state = 0 /* use 0 for default state ?? */; > > + > > + INIT_LIST_HEAD(&jack->kctl_list); > > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > > + int testbit = 1 << i; > > + if (type & testbit) { > > With this implementation, you'll get multiple boolean kctls for a headset. I > thought we agreed with creating a single boolean for headset? [Keyon] We agreed with creating multiple kctls for combo jack, e.g. headset. Furthermore, e.g., imagine that type = SND_JACK_HEADSET | SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is pressed, we will report to the 3rd kctl. > > In the case of input device, the situation is a bit different, so we shouldn't > mix up. > > > > + index = get_available_index(card,jack->id); > > + kctl = snd_kctl_jack_new(jack->id, index, card); > > + if (!kctl) > > + return -ENOMEM; > > + > > + err = snd_ctl_add(card, kctl); > > + if (err < 0) > > + return err; > > + > > + jack_kctl = kzalloc(sizeof(*jack_kctl), GFP_KERNEL); > > Missing NULL check. [Keyon] got it, add it soon. > > > + jack_kctl->kctl = kctl; > > + > > + kctl->private_data = jack_kctl; > > + kctl->private_free = kctl_private_free; > > + > > + /* use jack_bit_idx for the kctl type bit */ > > + jack_kctl->jack_bit_idx = i; > > This can be better to hold bit mask instead of bit index. [Keyon] good idea, will change to bit mask. > Then in the below... > > > @@ -245,13 +327,26 @@ void snd_jack_report(struct snd_jack *jack, int > > status) > .... > > } > > > > input_sync(jack->input_dev); > > + > > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > > + int testbit = 1 << i; > > + if (jack->type & testbit) { > > + list_for_each_entry(jack_kctl, &jack->kctl_list, > jack_list) { > > + if (jack_kctl->jack_bit_idx == i) { > > + snd_kctl_jack_report(jack->card, > jack_kctl->kctl, > > + status & > testbit); > > + } > > .... you can reduce to a single loop. > > > thanks, > > Takashi
At Fri, 20 Mar 2015 12:22:10 +0000, Jie, Yang wrote: > > > > +} > > > + > > > +static int snd_jack_new_kctl(struct snd_card *card, struct snd_jack > > > +*jack, int type) { > > > + struct snd_kcontrol *kctl; > > > + struct snd_jack_kctl *jack_kctl; > > > + int i, err, index, state = 0 /* use 0 for default state ?? */; > > > + > > > + INIT_LIST_HEAD(&jack->kctl_list); > > > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > > > + int testbit = 1 << i; > > > + if (type & testbit) { > > > > With this implementation, you'll get multiple boolean kctls for a headset. I > > thought we agreed with creating a single boolean for headset? > [Keyon] We agreed with creating multiple kctls for combo jack, e.g. headset. > Furthermore, e.g., imagine that type = SND_JACK_HEADSET | SND_JACK_BTN_0, > we will create 3 kctls for the jack, when BTN_0 is pressed, we will report to > the 3rd kctl. Hm, I don't remember that I agreed with multiple kctls... The multiple kctls have a significant drawback (multiple event calls for a single headset) and its behavior is incompatible with the current code (both the name change and the behavior change). That is, your patch will very likely break the existing applications. Takashi
> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Friday, March 20, 2015 8:27 PM > To: Jie, Yang > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > Girdwood, Liam R; Liam Girdwood > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > input device > > At Fri, 20 Mar 2015 12:22:10 +0000, > Jie, Yang wrote: > > > > > > +} > > > > + > > > > +static int snd_jack_new_kctl(struct snd_card *card, struct > > > > +snd_jack *jack, int type) { > > > > + struct snd_kcontrol *kctl; > > > > + struct snd_jack_kctl *jack_kctl; > > > > + int i, err, index, state = 0 /* use 0 for default state ?? */; > > > > + > > > > + INIT_LIST_HEAD(&jack->kctl_list); > > > > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > > > > + int testbit = 1 << i; > > > > + if (type & testbit) { > > > > > > With this implementation, you'll get multiple boolean kctls for a > > > headset. I thought we agreed with creating a single boolean for headset? > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g. > headset. > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET | > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is > > pressed, we will report to the 3rd kctl. > > Hm, I don't remember that I agreed with multiple kctls... > > The multiple kctls have a significant drawback (multiple event calls for a single > headset) and its behavior is incompatible with the current code (both the > name change and the behavior change). That is, your patch will very likely > break the existing applications. [Keyon] I am not very clear with the existing applications that using these kctl events(seems Android use input subsystem event? Which we didn't Change here. If I understand correctly, Pulseaudio uses jack switch controls, via the name, then we can use different name for headphone and mic here.) we will generate 2 event calls(one headphone, one mic) when Headset plug in/out, applications will receive these 2 events, and they can do anything, e.g. decide to switch on/off speaker/headphone. BTW, I haven't implemented the generating of combo jack kctls' name yet, currently, they looked like below: numid=12,iface=CARD,name='Headset Jack' numid=13,iface=CARD,name='Headset Jack',index=1 numid=14,iface=CARD,name='Headset Jack',index=2 once we have come to agreement, we can modify it in snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset Jack Speakers". > > > Takashi
At Fri, 20 Mar 2015 12:50:33 +0000, Jie, Yang wrote: > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Friday, March 20, 2015 8:27 PM > > To: Jie, Yang > > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > > Girdwood, Liam R; Liam Girdwood > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > > input device > > > > At Fri, 20 Mar 2015 12:22:10 +0000, > > Jie, Yang wrote: > > > > > > > > +} > > > > > + > > > > > +static int snd_jack_new_kctl(struct snd_card *card, struct > > > > > +snd_jack *jack, int type) { > > > > > + struct snd_kcontrol *kctl; > > > > > + struct snd_jack_kctl *jack_kctl; > > > > > + int i, err, index, state = 0 /* use 0 for default state ?? */; > > > > > + > > > > > + INIT_LIST_HEAD(&jack->kctl_list); > > > > > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > > > > > + int testbit = 1 << i; > > > > > + if (type & testbit) { > > > > > > > > With this implementation, you'll get multiple boolean kctls for a > > > > headset. I thought we agreed with creating a single boolean for headset? > > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g. > > headset. > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET | > > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is > > > pressed, we will report to the 3rd kctl. > > > > Hm, I don't remember that I agreed with multiple kctls... > > > > The multiple kctls have a significant drawback (multiple event calls for a single > > headset) and its behavior is incompatible with the current code (both the > > name change and the behavior change). That is, your patch will very likely > > break the existing applications. > [Keyon] I am not very clear with the existing applications that using these > kctl events(seems Android use input subsystem event? Which we didn't > Change here. If I understand correctly, Pulseaudio uses jack switch controls, > via the name, then we can use different name for headphone and mic here.) PA uses jack kctls. If you rename, how would you guarantee that the existing application will work as expected? PA doesn't have the definition of "Headset Speaker Jack" or such. And, no, we have no option "fix PA". Other way round: we are not allowed to break the current PA (or any user-space) behavior in general. > we will generate 2 event calls(one headphone, one mic) when Headset > plug in/out, applications will receive these 2 events, and they can do anything, > e.g. decide to switch on/off speaker/headphone. Won't this break any user-space stuff? > BTW, I haven't implemented the generating of combo jack kctls' name yet, > currently, they looked like below: > numid=12,iface=CARD,name='Headset Jack' > numid=13,iface=CARD,name='Headset Jack',index=1 > numid=14,iface=CARD,name='Headset Jack',index=2 > > once we have come to agreement, we can modify it in snd_jack_new_kctl(), > e.g., "Headset Jack Mic" and "Headset Jack Speakers". .... and how the existing user-space works without changing its code? Keyon, the most important point at this moment is to keep the compatibility. HD-audio is no new driver. It's a driver that has been present over a decade with (literally) thousands of variants. Please keep this in mind, and reconsider whether your patch will retain the compatibility, especially with PulseAudio. thanks, Takashi
> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Friday, March 20, 2015 9:21 PM > To: Jie, Yang > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > Girdwood, Liam R; Liam Girdwood; Tanu Kaskinen > (tanu.kaskinen@linux.intel.com) > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > input device > > At Fri, 20 Mar 2015 12:50:33 +0000, > Jie, Yang wrote: > > > > > -----Original Message----- > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > Sent: Friday, March 20, 2015 8:27 PM > > > To: Jie, Yang > > > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > > > Girdwood, Liam R; Liam Girdwood > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for > > > every jack input device > > > > > > At Fri, 20 Mar 2015 12:22:10 +0000, > > > Jie, Yang wrote: > > > > > > > > > > +} > > > > > > + > > > > > > +static int snd_jack_new_kctl(struct snd_card *card, struct > > > > > > +snd_jack *jack, int type) { > > > > > > + struct snd_kcontrol *kctl; > > > > > > + struct snd_jack_kctl *jack_kctl; > > > > > > + int i, err, index, state = 0 /* use 0 for default state ?? > > > > > > +*/; > > > > > > + > > > > > > + INIT_LIST_HEAD(&jack->kctl_list); > > > > > > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > > > > > > + int testbit = 1 << i; > > > > > > + if (type & testbit) { > > > > > > > > > > With this implementation, you'll get multiple boolean kctls for > > > > > a headset. I thought we agreed with creating a single boolean for > headset? > > > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g. > > > headset. > > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET | > > > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is > > > > pressed, we will report to the 3rd kctl. > > > > > > Hm, I don't remember that I agreed with multiple kctls... > > > > > > The multiple kctls have a significant drawback (multiple event calls > > > for a single > > > headset) and its behavior is incompatible with the current code > > > (both the name change and the behavior change). That is, your patch > > > will very likely break the existing applications. > > [Keyon] I am not very clear with the existing applications that using > > these kctl events(seems Android use input subsystem event? Which we > > didn't Change here. If I understand correctly, Pulseaudio uses jack > > switch controls, via the name, then we can use different name for > > headphone and mic here.) > > PA uses jack kctls. > > If you rename, how would you guarantee that the existing application will > work as expected? PA doesn't have the definition of "Headset Speaker Jack" > or such. > > And, no, we have no option "fix PA". Other way round: we are not allowed > to break the current PA (or any user-space) behavior in general. > > > we will generate 2 event calls(one headphone, one mic) when Headset > > plug in/out, applications will receive these 2 events, and they can > > do anything, e.g. decide to switch on/off speaker/headphone. > > Won't this break any user-space stuff? > > > BTW, I haven't implemented the generating of combo jack kctls' name > > yet, currently, they looked like below: > > numid=12,iface=CARD,name='Headset Jack' > > numid=13,iface=CARD,name='Headset Jack',index=1 > > numid=14,iface=CARD,name='Headset Jack',index=2 > > > > once we have come to agreement, we can modify it in > > snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset Jack > Speakers". > > .... and how the existing user-space works without changing its code? > > > Keyon, the most important point at this moment is to keep the compatibility. > HD-audio is no new driver. It's a driver that has been present over a decade > with (literally) thousands of variants. > Please keep this in mind, and reconsider whether your patch will retain the > compatibility, especially with PulseAudio. [Keyon] understood. Then we should follow the HD-audio style, So, what do you suggest here? Should we create only one single Boolean kctls for headset, and report true when status in headphone bit it true? Then we need a tricky exception mapping here? Sorry, I am a little confusing here, because Mark suggest to create multiple controls for multiple bits jack, and you also agreed with that. :( > > > thanks, > > Takashi
At Fri, 20 Mar 2015 13:49:24 +0000, Jie, Yang wrote: > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Friday, March 20, 2015 9:21 PM > > To: Jie, Yang > > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > > Girdwood, Liam R; Liam Girdwood; Tanu Kaskinen > > (tanu.kaskinen@linux.intel.com) > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > > input device > > > > At Fri, 20 Mar 2015 12:50:33 +0000, > > Jie, Yang wrote: > > > > > > > -----Original Message----- > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > Sent: Friday, March 20, 2015 8:27 PM > > > > To: Jie, Yang > > > > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > > > > Girdwood, Liam R; Liam Girdwood > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for > > > > every jack input device > > > > > > > > At Fri, 20 Mar 2015 12:22:10 +0000, > > > > Jie, Yang wrote: > > > > > > > > > > > > +} > > > > > > > + > > > > > > > +static int snd_jack_new_kctl(struct snd_card *card, struct > > > > > > > +snd_jack *jack, int type) { > > > > > > > + struct snd_kcontrol *kctl; > > > > > > > + struct snd_jack_kctl *jack_kctl; > > > > > > > + int i, err, index, state = 0 /* use 0 for default state ?? > > > > > > > +*/; > > > > > > > + > > > > > > > + INIT_LIST_HEAD(&jack->kctl_list); > > > > > > > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > > > > > > > + int testbit = 1 << i; > > > > > > > + if (type & testbit) { > > > > > > > > > > > > With this implementation, you'll get multiple boolean kctls for > > > > > > a headset. I thought we agreed with creating a single boolean for > > headset? > > > > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g. > > > > headset. > > > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET | > > > > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is > > > > > pressed, we will report to the 3rd kctl. > > > > > > > > Hm, I don't remember that I agreed with multiple kctls... > > > > > > > > The multiple kctls have a significant drawback (multiple event calls > > > > for a single > > > > headset) and its behavior is incompatible with the current code > > > > (both the name change and the behavior change). That is, your patch > > > > will very likely break the existing applications. > > > [Keyon] I am not very clear with the existing applications that using > > > these kctl events(seems Android use input subsystem event? Which we > > > didn't Change here. If I understand correctly, Pulseaudio uses jack > > > switch controls, via the name, then we can use different name for > > > headphone and mic here.) > > > > PA uses jack kctls. > > > > If you rename, how would you guarantee that the existing application will > > work as expected? PA doesn't have the definition of "Headset Speaker Jack" > > or such. > > > > And, no, we have no option "fix PA". Other way round: we are not allowed > > to break the current PA (or any user-space) behavior in general. > > > > > we will generate 2 event calls(one headphone, one mic) when Headset > > > plug in/out, applications will receive these 2 events, and they can > > > do anything, e.g. decide to switch on/off speaker/headphone. > > > > Won't this break any user-space stuff? > > > > > BTW, I haven't implemented the generating of combo jack kctls' name > > > yet, currently, they looked like below: > > > numid=12,iface=CARD,name='Headset Jack' > > > numid=13,iface=CARD,name='Headset Jack',index=1 > > > numid=14,iface=CARD,name='Headset Jack',index=2 > > > > > > once we have come to agreement, we can modify it in > > > snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset Jack > > Speakers". > > > > .... and how the existing user-space works without changing its code? > > > > > > Keyon, the most important point at this moment is to keep the compatibility. > > HD-audio is no new driver. It's a driver that has been present over a decade > > with (literally) thousands of variants. > > Please keep this in mind, and reconsider whether your patch will retain the > > compatibility, especially with PulseAudio. > [Keyon] understood. Then we should follow the HD-audio style, So, what do > you suggest here? Should we create only one single Boolean kctls for headset, > and report true when status in headphone bit it true? Then we need a tricky > exception mapping here? > > Sorry, I am a little confusing here, because Mark suggest to create multiple controls > for multiple bits jack, and you also agreed with that. :( Just prepare two exceptions for SND_JACK_HEADSET and SND_JACK_VIDEOUT. These are already defined as single types in sound/jack.h. The code can't be so tricky if you write properly :) Takashi
On Fri, 2015-03-20 at 15:18 +0100, Takashi Iwai wrote: > At Fri, 20 Mar 2015 13:49:24 +0000, > Jie, Yang wrote: > > > > > -----Original Message----- > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > Sent: Friday, March 20, 2015 9:21 PM > > > To: Jie, Yang > > > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > > > Girdwood, Liam R; Liam Girdwood; Tanu Kaskinen > > > (tanu.kaskinen@linux.intel.com) > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > > > input device > > > > > > At Fri, 20 Mar 2015 12:50:33 +0000, > > > Jie, Yang wrote: > > > > > > > > > -----Original Message----- > > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > > Sent: Friday, March 20, 2015 8:27 PM > > > > > To: Jie, Yang > > > > > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > > > > > Girdwood, Liam R; Liam Girdwood > > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for > > > > > every jack input device > > > > > > > > > > At Fri, 20 Mar 2015 12:22:10 +0000, > > > > > Jie, Yang wrote: > > > > > > > > > > > > > > +} > > > > > > > > + > > > > > > > > +static int snd_jack_new_kctl(struct snd_card *card, struct > > > > > > > > +snd_jack *jack, int type) { > > > > > > > > + struct snd_kcontrol *kctl; > > > > > > > > + struct snd_jack_kctl *jack_kctl; > > > > > > > > + int i, err, index, state = 0 /* use 0 for default state ?? > > > > > > > > +*/; > > > > > > > > + > > > > > > > > + INIT_LIST_HEAD(&jack->kctl_list); > > > > > > > > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > > > > > > > > + int testbit = 1 << i; > > > > > > > > + if (type & testbit) { > > > > > > > > > > > > > > With this implementation, you'll get multiple boolean kctls for > > > > > > > a headset. I thought we agreed with creating a single boolean for > > > headset? > > > > > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g. > > > > > headset. > > > > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET | > > > > > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is > > > > > > pressed, we will report to the 3rd kctl. > > > > > > > > > > Hm, I don't remember that I agreed with multiple kctls... > > > > > > > > > > The multiple kctls have a significant drawback (multiple event calls > > > > > for a single > > > > > headset) and its behavior is incompatible with the current code > > > > > (both the name change and the behavior change). That is, your patch > > > > > will very likely break the existing applications. > > > > [Keyon] I am not very clear with the existing applications that using > > > > these kctl events(seems Android use input subsystem event? Which we > > > > didn't Change here. If I understand correctly, Pulseaudio uses jack > > > > switch controls, via the name, then we can use different name for > > > > headphone and mic here.) > > > > > > PA uses jack kctls. > > > > > > If you rename, how would you guarantee that the existing application will > > > work as expected? PA doesn't have the definition of "Headset Speaker Jack" > > > or such. > > > > > > And, no, we have no option "fix PA". Other way round: we are not allowed > > > to break the current PA (or any user-space) behavior in general. > > > > > > > we will generate 2 event calls(one headphone, one mic) when Headset > > > > plug in/out, applications will receive these 2 events, and they can > > > > do anything, e.g. decide to switch on/off speaker/headphone. > > > > > > Won't this break any user-space stuff? > > > > > > > BTW, I haven't implemented the generating of combo jack kctls' name > > > > yet, currently, they looked like below: > > > > numid=12,iface=CARD,name='Headset Jack' > > > > numid=13,iface=CARD,name='Headset Jack',index=1 > > > > numid=14,iface=CARD,name='Headset Jack',index=2 > > > > > > > > once we have come to agreement, we can modify it in > > > > snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset Jack > > > Speakers". > > > > > > .... and how the existing user-space works without changing its code? > > > > > > > > > Keyon, the most important point at this moment is to keep the compatibility. > > > HD-audio is no new driver. It's a driver that has been present over a decade > > > with (literally) thousands of variants. > > > Please keep this in mind, and reconsider whether your patch will retain the > > > compatibility, especially with PulseAudio. > > [Keyon] understood. Then we should follow the HD-audio style, So, what do > > you suggest here? Should we create only one single Boolean kctls for headset, > > and report true when status in headphone bit it true? Then we need a tricky > > exception mapping here? > > > > Sorry, I am a little confusing here, because Mark suggest to create multiple controls > > for multiple bits jack, and you also agreed with that. :( > > Just prepare two exceptions for SND_JACK_HEADSET and > SND_JACK_VIDEOUT. These are already defined as single types in > sound/jack.h. The code can't be so tricky if you write properly :) Can someone clarify what is the current plan? Will there be changes to the control naming or behaviour for existing hardware? PulseAudio's jack handling seems somewhat messy to me, so it may be difficult to understand what kind of changes will break things and what won't. I think these are the jack controls that PulseAudio currently recognizes that are most important in this discussion: * Headset Mic Jack * Headphone Mic Jack * Mic Jack * Headphone Jack "Headset Mic Jack" indicates the availability of a headset mic. In PulseAudio it doesn't imply anything about the headset speaker availability, even though logically, if there's a headset mic plugged in, surely the headset speakers are available too. "Headphone Mic Jack" indicates the availability of either headphones or a microphone. There's no information about which of those is plugged in, just that one of those devices is plugged in. This control is *not* for headsets, according to the commit that added the support for that control to PulseAudio[1]. "Mic Jack" indicates the availability of a standalone microphone. I don't know if the kernel currently exposes both "Headset Mick Jack" and "Mic Jack" if there's one physical jack that is able to distinguish between the two device types, but I suppose it would be good to have both controls in that case, so that applications know what kind of device has been plugged in. "Headphone Jack" indicates the availability of headphones. PulseAudio doesn't currently have support for any kind of "Headset Speaker Jack" control, so I guess the kernel currently uses "Headphone Jack" also with physical jacks that support headsets. One thing that is unclear for me is that how are those jacks represented that support any of headsets/headphones/microphones, but don't provide information about which device type has been plugged in. I know David has made a UI for Ubuntu for selecting the device type once something has been plugged in to such jack, but I don't remember how the UI can know that the jack supports all of those three device types. [1] http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=7369a53ab5f606e87a3cd1cd4eebd40226bab090
On 2015-03-23 11:56, Tanu Kaskinen wrote: > One thing that is unclear for me is that how are those jacks represented > that support any of headsets/headphones/microphones, but don't provide > information about which device type has been plugged in. For headphone or headset, independent switches: * "Headphone Jack" * "Headset Mic Jack" For headphone or headset, one hw switch only: * "Headphone Jack" * "Headset Mic Phantom Jack" Headphone or mic, one hw switch: * "Headphone Mic Jack" Headphone, headset, or mic, one hw switch only: * "Headphone Mic Jack" * "Headset Mic Phantom Jack" Headphone, headset, or mic, one switch for hp/mic and the other for the headset mic: * "Headphone Mic Jack" * "Headset Mic Jack" The first one is the most common one, but all of the other ones exist, especially on recent Dell machines. > I know David > has made a UI for Ubuntu for selecting the device type once something > has been plugged in to such jack, but I don't remember how the UI can > know that the jack supports all of those three device types. For reference, the code is here: http://bazaar.launchpad.net/~unity-settings-daemon-team/unity-settings-daemon/trunk/files/head:/plugins/media-keys/what-did-you-plug-in/
At Mon, 23 Mar 2015 12:51:07 +0100, David Henningsson wrote: > > > > On 2015-03-23 11:56, Tanu Kaskinen wrote: > > One thing that is unclear for me is that how are those jacks represented > > that support any of headsets/headphones/microphones, but don't provide > > information about which device type has been plugged in. > > For headphone or headset, independent switches: > > * "Headphone Jack" > * "Headset Mic Jack" > > For headphone or headset, one hw switch only: > > * "Headphone Jack" > * "Headset Mic Phantom Jack" > > Headphone or mic, one hw switch: > > * "Headphone Mic Jack" > > Headphone, headset, or mic, one hw switch only: > > * "Headphone Mic Jack" > * "Headset Mic Phantom Jack" > > Headphone, headset, or mic, one switch for hp/mic and the other for the > headset mic: > > * "Headphone Mic Jack" > * "Headset Mic Jack" > > The first one is the most common one, but all of the other ones exist, > especially on recent Dell machines. Are all these about a single headset/headphone jack? If so, yeah, it's really messy. Takashi
On Mon, Mar 23, 2015 at 12:51:07PM +0100, David Henningsson wrote: > On 2015-03-23 11:56, Tanu Kaskinen wrote: > >One thing that is unclear for me is that how are those jacks represented > >that support any of headsets/headphones/microphones, but don't provide > >information about which device type has been plugged in. > For headphone or headset, independent switches: > * "Headphone Jack" > * "Headset Mic Jack" > For headphone or headset, one hw switch only: > * "Headphone Jack" > * "Headset Mic Phantom Jack" I'd have expected these to be the same thing, just with the second case always having the same state for both switches.
On 2015-03-23 17:41, Mark Brown wrote: > On Mon, Mar 23, 2015 at 12:51:07PM +0100, David Henningsson wrote: >> On 2015-03-23 11:56, Tanu Kaskinen wrote: > >>> One thing that is unclear for me is that how are those jacks represented >>> that support any of headsets/headphones/microphones, but don't provide >>> information about which device type has been plugged in. > >> For headphone or headset, independent switches: > >> * "Headphone Jack" >> * "Headset Mic Jack" > >> For headphone or headset, one hw switch only: > >> * "Headphone Jack" >> * "Headset Mic Phantom Jack" > > I'd have expected these to be the same thing, just with the second case > always having the same state for both switches. We need to distinguish the use cases: in case of independent switches, we can make the assumption that if the user plugged in a headset, the user wants to use the headset mic instead of the internal mic, so we switch to it. In the other case, where we don't know if the user plugged in a headphone or a headset, we need to ask the user what (s)he plugged in, so we can determine whether to select headphone+internal mic or headphone+headset mic.
On Tue, Mar 24, 2015 at 07:50:39AM +0100, David Henningsson wrote: > On 2015-03-23 17:41, Mark Brown wrote: > >>For headphone or headset, one hw switch only: > >> * "Headphone Jack" > >> * "Headset Mic Phantom Jack" > >I'd have expected these to be the same thing, just with the second case > >always having the same state for both switches. > We need to distinguish the use cases: in case of independent switches, we > can make the assumption that if the user plugged in a headset, the user > wants to use the headset mic instead of the internal mic, so we switch to > it. > In the other case, where we don't know if the user plugged in a headphone or > a headset, we need to ask the user what (s)he plugged in, so we can > determine whether to select headphone+internal mic or headphone+headset mic. Hrm, I can see that. However it feels wrong to have no state change at all on the microphone part of the jack - I think I would have expected this case to be flagged as a separate mode, say "Tied", so that simpler applications have more chance to do the right thing. It's a bit of a separate issue though and it's a bit late now as we've already shipped the ABI for the kcontrols.
> > > >>For headphone or headset, one hw switch only: > > > >> * "Headphone Jack" > > >> * "Headset Mic Phantom Jack" > > > >I'd have expected these to be the same thing, just with the second case > > >always having the same state for both switches. > > > We need to distinguish the use cases: in case of independent switches, we > > can make the assumption that if the user plugged in a headset, the user > > wants to use the headset mic instead of the internal mic, so we switch to > > it. > > > In the other case, where we don't know if the user plugged in a headphone or > > a headset, we need to ask the user what (s)he plugged in, so we can > > determine whether to select headphone+internal mic or headphone+headset mic.:: Does the user maunal of those notebook explicitly state that the combo jack support headset and headphone ( or mic) ? if not, why do the driver need to support both headphone and headset ( or mic ) especially when the icon near the jack is headset
>> >> >>>> One thing that is unclear for me is that how are those jacks represented >>>> that support any of headsets/headphones/microphones, but don't provide >>>> information about which device type has been plugged in. >> >> >>> For headphone or headset, independent switches: >> >> >>> * "Headphone Jack" >>> * "Headset Mic Jack" >> >> >>> For headphone or headset, one hw switch only: >> >> >>> * "Headphone Jack" >>> * "Headset Mic Phantom Jack" >> >> >> I'd have expected these to be the same thing, just with the second case >> always having the same state for both switches. > > > We need to distinguish the use cases: in case of independent switches, we can make the assumption that if the user plugged in a headset, the user wants to use the headset mic instead of the internal mic, so we switch to it. > What will happen for those Dell Alienware 14, 15 and 17 which have three jacks: headset, headphone and mic http://www.dell.com/support/home/us/en/19/product-support/product/alienware-17/manuals Alienware 17 Quick Start Guide
> -----Original Message----- > From: Tanu Kaskinen [mailto:tanu.kaskinen@linux.intel.com] > Sent: Monday, March 23, 2015 6:57 PM > To: Takashi Iwai > Cc: Jie, Yang; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa- > project.org; Girdwood, Liam R; Liam Girdwood; David Henningsson > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > input device > > On Fri, 2015-03-20 at 15:18 +0100, Takashi Iwai wrote: > > At Fri, 20 Mar 2015 13:49:24 +0000, > > Jie, Yang wrote: > > > > > > > -----Original Message----- > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > Sent: Friday, March 20, 2015 9:21 PM > > > > To: Jie, Yang > > > > Cc: perex@perex.cz; broonie@kernel.org; > > > > alsa-devel@alsa-project.org; Girdwood, Liam R; Liam Girdwood; Tanu > > > > Kaskinen > > > > (tanu.kaskinen@linux.intel.com) > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for > > > > every jack input device > > > > > > > > At Fri, 20 Mar 2015 12:50:33 +0000, Jie, Yang wrote: > > > > > > > > > > > -----Original Message----- > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > > > Sent: Friday, March 20, 2015 8:27 PM > > > > > > To: Jie, Yang > > > > > > Cc: perex@perex.cz; broonie@kernel.org; > > > > > > alsa-devel@alsa-project.org; Girdwood, Liam R; Liam Girdwood > > > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols > > > > > > for every jack input device > > > > > > > > > > > > At Fri, 20 Mar 2015 12:22:10 +0000, Jie, Yang wrote: > > > > > > > > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +static int snd_jack_new_kctl(struct snd_card *card, > > > > > > > > > +struct snd_jack *jack, int type) { > > > > > > > > > + struct snd_kcontrol *kctl; > > > > > > > > > + struct snd_jack_kctl *jack_kctl; > > > > > > > > > + int i, err, index, state = 0 /* use 0 for default state ?? > > > > > > > > > +*/; > > > > > > > > > + > > > > > > > > > + INIT_LIST_HEAD(&jack->kctl_list); > > > > > > > > > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > > > > > > > > > + int testbit = 1 << i; > > > > > > > > > + if (type & testbit) { > > > > > > > > > > > > > > > > With this implementation, you'll get multiple boolean > > > > > > > > kctls for a headset. I thought we agreed with creating a > > > > > > > > single boolean for > > > > headset? > > > > > > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g. > > > > > > headset. > > > > > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET | > > > > > > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when > > > > > > > BTN_0 is pressed, we will report to the 3rd kctl. > > > > > > > > > > > > Hm, I don't remember that I agreed with multiple kctls... > > > > > > > > > > > > The multiple kctls have a significant drawback (multiple event > > > > > > calls for a single > > > > > > headset) and its behavior is incompatible with the current > > > > > > code (both the name change and the behavior change). That is, > > > > > > your patch will very likely break the existing applications. > > > > > [Keyon] I am not very clear with the existing applications that > > > > > using these kctl events(seems Android use input subsystem event? > > > > > Which we didn't Change here. If I understand correctly, > > > > > Pulseaudio uses jack switch controls, via the name, then we can > > > > > use different name for headphone and mic here.) > > > > > > > > PA uses jack kctls. > > > > > > > > If you rename, how would you guarantee that the existing > > > > application will work as expected? PA doesn't have the definition of > "Headset Speaker Jack" > > > > or such. > > > > > > > > And, no, we have no option "fix PA". Other way round: we are not > > > > allowed to break the current PA (or any user-space) behavior in general. > > > > > > > > > we will generate 2 event calls(one headphone, one mic) when > > > > > Headset plug in/out, applications will receive these 2 events, > > > > > and they can do anything, e.g. decide to switch on/off > speaker/headphone. > > > > > > > > Won't this break any user-space stuff? > > > > > > > > > BTW, I haven't implemented the generating of combo jack kctls' > > > > > name yet, currently, they looked like below: > > > > > numid=12,iface=CARD,name='Headset Jack' > > > > > numid=13,iface=CARD,name='Headset Jack',index=1 > > > > > numid=14,iface=CARD,name='Headset Jack',index=2 > > > > > > > > > > once we have come to agreement, we can modify it in > > > > > snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset Jack > > > > Speakers". > > > > > > > > .... and how the existing user-space works without changing its code? > > > > > > > > > > > > Keyon, the most important point at this moment is to keep the > compatibility. > > > > HD-audio is no new driver. It's a driver that has been present > > > > over a decade with (literally) thousands of variants. > > > > Please keep this in mind, and reconsider whether your patch will > > > > retain the compatibility, especially with PulseAudio. > > > [Keyon] understood. Then we should follow the HD-audio style, So, > > > what do you suggest here? Should we create only one single Boolean > > > kctls for headset, and report true when status in headphone bit it > > > true? Then we need a tricky exception mapping here? > > > > > > Sorry, I am a little confusing here, because Mark suggest to create > > > multiple controls for multiple bits jack, and you also agreed with > > > that. :( > > > > Just prepare two exceptions for SND_JACK_HEADSET and > SND_JACK_VIDEOUT. > > These are already defined as single types in sound/jack.h. The code > > can't be so tricky if you write properly :) > > Can someone clarify what is the current plan? Will there be changes to the > control naming or behaviour for existing hardware? [Keyon] Hi Tanu, currently we plan to create kctls/switches at jack creating stage, and attached them to the jack. So, we need to decide the naming of these kctls/switches and make sure they will be understood by PA correctly. At the same time, we create jacks with snd_jack_types as parameter passed in, so, we need decision about what kctls/switches do we need create, for each separate or combo(bits) type jack: enum snd_jack_types { SND_JACK_HEADPHONE = 0x0001, SND_JACK_MICROPHONE = 0x0002, SND_JACK_HEADSET = SND_JACK_HEADPHONE | SND_JACK_MICROPHONE, SND_JACK_LINEOUT = 0x0004, SND_JACK_MECHANICAL = 0x0008, /* If detected separately */ SND_JACK_VIDEOOUT = 0x0010, SND_JACK_AVOUT = SND_JACK_LINEOUT | SND_JACK_VIDEOOUT, SND_JACK_LINEIN = 0x0020, /* Kept separate from switches to facilitate implementation */ SND_JACK_BTN_0 = 0x4000, SND_JACK_BTN_1 = 0x2000, SND_JACK_BTN_2 = 0x1000, SND_JACK_BTN_3 = 0x0800, SND_JACK_BTN_4 = 0x0400, SND_JACK_BTN_5 = 0x0200, }; So, 1. we need these naming, what you summarized below, that's quite helpful. 2. I am a little concern if the exist snd_jack_types enum can indicate all diverse existing hardware, e.g. " Headphone Mic Jack " as you mentioned below, can we use any bits combination for this kind of jack? SND_JACK_HEADPHONE | SND_JACK_MICROPHONE seems can't tell difference With "Headset Mic Jack" + "Headphone Jack"? > > PulseAudio's jack handling seems somewhat messy to me, so it may be > difficult to understand what kind of changes will break things and what won't. > I think these are the jack controls that PulseAudio currently recognizes that > are most important in this discussion: > > * Headset Mic Jack > * Headphone Mic Jack > * Mic Jack > * Headphone Jack > > "Headset Mic Jack" indicates the availability of a headset mic. In PulseAudio it > doesn't imply anything about the headset speaker availability, even though > logically, if there's a headset mic plugged in, surely the headset speakers are > available too. > > "Headphone Mic Jack" indicates the availability of either headphones or a > microphone. There's no information about which of those is plugged in, just > that one of those devices is plugged in. This control is *not* for headsets, > according to the commit that added the support for that control to > PulseAudio[1]. > > "Mic Jack" indicates the availability of a standalone microphone. I don't know > if the kernel currently exposes both "Headset Mick Jack" and "Mic Jack" if > there's one physical jack that is able to distinguish between the two device > types, but I suppose it would be good to have both controls in that case, so > that applications know what kind of device has been plugged in. > > "Headphone Jack" indicates the availability of headphones. PulseAudio > doesn't currently have support for any kind of "Headset Speaker Jack" > control, so I guess the kernel currently uses "Headphone Jack" also with > physical jacks that support headsets. > > One thing that is unclear for me is that how are those jacks represented that > support any of headsets/headphones/microphones, but don't provide > information about which device type has been plugged in. I know David has > made a UI for Ubuntu for selecting the device type once something has been > plugged in to such jack, but I don't remember how the UI can know that the > jack supports all of those three device types. > > [1] > http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=7369a53ab5 > f606e87a3cd1cd4eebd40226bab090 > > -- > Tanu
On Wed, 2015-03-25 at 07:53 +0000, Jie, Yang wrote: > > -----Original Message----- > > From: Tanu Kaskinen [mailto:tanu.kaskinen@linux.intel.com] > > Sent: Monday, March 23, 2015 6:57 PM > > To: Takashi Iwai > > Cc: Jie, Yang; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa- > > project.org; Girdwood, Liam R; Liam Girdwood; David Henningsson > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > > input device > > > > On Fri, 2015-03-20 at 15:18 +0100, Takashi Iwai wrote: > > > At Fri, 20 Mar 2015 13:49:24 +0000, > > > Jie, Yang wrote: > > > > > > > > > -----Original Message----- > > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > > Sent: Friday, March 20, 2015 9:21 PM > > > > > To: Jie, Yang > > > > > Cc: perex@perex.cz; broonie@kernel.org; > > > > > alsa-devel@alsa-project.org; Girdwood, Liam R; Liam Girdwood; Tanu > > > > > Kaskinen > > > > > (tanu.kaskinen@linux.intel.com) > > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for > > > > > every jack input device > > > > > > > > > > At Fri, 20 Mar 2015 12:50:33 +0000, Jie, Yang wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > > > > Sent: Friday, March 20, 2015 8:27 PM > > > > > > > To: Jie, Yang > > > > > > > Cc: perex@perex.cz; broonie@kernel.org; > > > > > > > alsa-devel@alsa-project.org; Girdwood, Liam R; Liam Girdwood > > > > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols > > > > > > > for every jack input device > > > > > > > > > > > > > > At Fri, 20 Mar 2015 12:22:10 +0000, Jie, Yang wrote: > > > > > > > > > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > +static int snd_jack_new_kctl(struct snd_card *card, > > > > > > > > > > +struct snd_jack *jack, int type) { > > > > > > > > > > + struct snd_kcontrol *kctl; > > > > > > > > > > + struct snd_jack_kctl *jack_kctl; > > > > > > > > > > + int i, err, index, state = 0 /* use 0 for default state ?? > > > > > > > > > > +*/; > > > > > > > > > > + > > > > > > > > > > + INIT_LIST_HEAD(&jack->kctl_list); > > > > > > > > > > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > > > > > > > > > > + int testbit = 1 << i; > > > > > > > > > > + if (type & testbit) { > > > > > > > > > > > > > > > > > > With this implementation, you'll get multiple boolean > > > > > > > > > kctls for a headset. I thought we agreed with creating a > > > > > > > > > single boolean for > > > > > headset? > > > > > > > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g. > > > > > > > headset. > > > > > > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET | > > > > > > > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when > > > > > > > > BTN_0 is pressed, we will report to the 3rd kctl. > > > > > > > > > > > > > > Hm, I don't remember that I agreed with multiple kctls... > > > > > > > > > > > > > > The multiple kctls have a significant drawback (multiple event > > > > > > > calls for a single > > > > > > > headset) and its behavior is incompatible with the current > > > > > > > code (both the name change and the behavior change). That is, > > > > > > > your patch will very likely break the existing applications. > > > > > > [Keyon] I am not very clear with the existing applications that > > > > > > using these kctl events(seems Android use input subsystem event? > > > > > > Which we didn't Change here. If I understand correctly, > > > > > > Pulseaudio uses jack switch controls, via the name, then we can > > > > > > use different name for headphone and mic here.) > > > > > > > > > > PA uses jack kctls. > > > > > > > > > > If you rename, how would you guarantee that the existing > > > > > application will work as expected? PA doesn't have the definition of > > "Headset Speaker Jack" > > > > > or such. > > > > > > > > > > And, no, we have no option "fix PA". Other way round: we are not > > > > > allowed to break the current PA (or any user-space) behavior in general. > > > > > > > > > > > we will generate 2 event calls(one headphone, one mic) when > > > > > > Headset plug in/out, applications will receive these 2 events, > > > > > > and they can do anything, e.g. decide to switch on/off > > speaker/headphone. > > > > > > > > > > Won't this break any user-space stuff? > > > > > > > > > > > BTW, I haven't implemented the generating of combo jack kctls' > > > > > > name yet, currently, they looked like below: > > > > > > numid=12,iface=CARD,name='Headset Jack' > > > > > > numid=13,iface=CARD,name='Headset Jack',index=1 > > > > > > numid=14,iface=CARD,name='Headset Jack',index=2 > > > > > > > > > > > > once we have come to agreement, we can modify it in > > > > > > snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset Jack > > > > > Speakers". > > > > > > > > > > .... and how the existing user-space works without changing its code? > > > > > > > > > > > > > > > Keyon, the most important point at this moment is to keep the > > compatibility. > > > > > HD-audio is no new driver. It's a driver that has been present > > > > > over a decade with (literally) thousands of variants. > > > > > Please keep this in mind, and reconsider whether your patch will > > > > > retain the compatibility, especially with PulseAudio. > > > > [Keyon] understood. Then we should follow the HD-audio style, So, > > > > what do you suggest here? Should we create only one single Boolean > > > > kctls for headset, and report true when status in headphone bit it > > > > true? Then we need a tricky exception mapping here? > > > > > > > > Sorry, I am a little confusing here, because Mark suggest to create > > > > multiple controls for multiple bits jack, and you also agreed with > > > > that. :( > > > > > > Just prepare two exceptions for SND_JACK_HEADSET and > > SND_JACK_VIDEOUT. > > > These are already defined as single types in sound/jack.h. The code > > > can't be so tricky if you write properly :) > > > > Can someone clarify what is the current plan? Will there be changes to the > > control naming or behaviour for existing hardware? > [Keyon] Hi Tanu, currently we plan to create kctls/switches at jack creating > stage, and attached them to the jack. > So, we need to decide the naming of these kctls/switches and make sure > they will be understood by PA correctly. So the plan is to avoid any changes to what kctls applications see? That's very good. > At the same time, we create jacks with snd_jack_types as parameter passed > in, so, we need decision about what kctls/switches do we need create, for > each separate or combo(bits) type jack: > enum snd_jack_types { > SND_JACK_HEADPHONE = 0x0001, > SND_JACK_MICROPHONE = 0x0002, > SND_JACK_HEADSET = SND_JACK_HEADPHONE | SND_JACK_MICROPHONE, > SND_JACK_LINEOUT = 0x0004, > SND_JACK_MECHANICAL = 0x0008, /* If detected separately */ > SND_JACK_VIDEOOUT = 0x0010, > SND_JACK_AVOUT = SND_JACK_LINEOUT | SND_JACK_VIDEOOUT, > SND_JACK_LINEIN = 0x0020, > > /* Kept separate from switches to facilitate implementation */ > SND_JACK_BTN_0 = 0x4000, > SND_JACK_BTN_1 = 0x2000, > SND_JACK_BTN_2 = 0x1000, > SND_JACK_BTN_3 = 0x0800, > SND_JACK_BTN_4 = 0x0400, > SND_JACK_BTN_5 = 0x0200, > }; > > So, > 1. we need these naming, what you summarized below, that's quite helpful. David's summary is even more helpful! > 2. I am a little concern if the exist snd_jack_types enum can indicate all diverse > existing hardware, e.g. " Headphone Mic Jack " as you mentioned below, can > we use any bits combination for this kind of jack? > SND_JACK_HEADPHONE | SND_JACK_MICROPHONE seems can't tell difference > With "Headset Mic Jack" + "Headphone Jack"? Yes, it seems the existing enumeration isn't able to do that distinction. Can the enumeration be extended? Another thing that might be a problem is that this jack enumeration doesn't make distinction between what device types can be used with the jack, and what level of device type detection is available. That is, is a headset jack only able to tell that "something was plugged in", or is it also able to tell whether that something was headphones, a microphone or a headset. (I suppose this might essentially be the same topic as discussed in the "ALSA: jack: Refactoring for jack kctls" thread about phantom jacks.)
Thank you for comprehensive summarizing, David. Can you help give more description as below? ~Keyon > -----Original Message----- > From: David Henningsson [mailto:david.henningsson@canonical.com] > Sent: Monday, March 23, 2015 7:51 PM > To: Tanu Kaskinen; Takashi Iwai > Cc: Jie, Yang; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa- > project.org; Girdwood, Liam R; Liam Girdwood > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > input device > > > > On 2015-03-23 11:56, Tanu Kaskinen wrote: > > One thing that is unclear for me is that how are those jacks > > represented that support any of headsets/headphones/microphones, but > > don't provide information about which device type has been plugged in. > > For headphone or headset, independent switches: > > * "Headphone Jack" > * "Headset Mic Jack" [Keyon] here for the most common headset jack (SND_JACK_HEADPHONE | SND_JACK_MICROPHONE), we should create two independent kctls/switches--"Headphone Jack" + "Headset Mic Jack", right? so, is it possible that there is only "Mic Jack" in this case? I mean that only input(no output, no physical headphone/speaker jack) jack exist. If yes, then we may need change "Headset Mic Jack" to "Mic Jack"? > > For headphone or headset, one hw switch only: [Keyon] I am sorry I am not familiar with the jack HW circuit. What "one hw switch only" here means? Does it means that -- for headset Jack, the status(connected/disconnected) of HP pin is always exactly same with that of Mic pin? > > * "Headphone Jack" > * "Headset Mic Phantom Jack" [Keyon] for Headset of this type, do we need create only "Headset Mic Phantom Jack" kctl, or "Headphone Jack" kctl is also needed? > > Headphone or mic, one hw switch: [Keyon]I am fresh about this kind of hw jack, it should use different segment of the plug, seems we don't need check the actual connected status at the jack creation stage, just creating "Headphone Mic Jack" should works, right? > > * "Headphone Mic Jack" > > Headphone, headset, or mic, one hw switch only: [Keyon] how many kctls should we create for this? > > * "Headphone Mic Jack" > * "Headset Mic Phantom Jack" > > Headphone, headset, or mic, one switch for hp/mic and the other for the > headset mic: [Keyon] I can't imagine how this works, it's too messy for me. :( > > * "Headphone Mic Jack" > * "Headset Mic Jack" > > The first one is the most common one, but all of the other ones exist, > especially on recent Dell machines. > > > I know David > > has made a UI for Ubuntu for selecting the device type once something > > has been plugged in to such jack, but I don't remember how the UI can > > know that the jack supports all of those three device types. > > For reference, the code is here: > http://bazaar.launchpad.net/~unity-settings-daemon-team/unity-settings- > daemon/trunk/files/head:/plugins/media-keys/what-did-you-plug-in/ > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic
> -----Original Message----- > From: Tanu Kaskinen [mailto:tanu.kaskinen@linux.intel.com] > Sent: Wednesday, March 25, 2015 5:28 PM > To: Jie, Yang > Cc: Takashi Iwai; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa- > project.org; Girdwood, Liam R; Liam Girdwood; David Henningsson > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > input device > > On Wed, 2015-03-25 at 07:53 +0000, Jie, Yang wrote: > > > -----Original Message----- > > > From: Tanu Kaskinen [mailto:tanu.kaskinen@linux.intel.com] > > > Sent: Monday, March 23, 2015 6:57 PM > > > To: Takashi Iwai > > > Cc: Jie, Yang; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa- > > > project.org; Girdwood, Liam R; Liam Girdwood; David Henningsson > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for > > > every jack input device > > > > > > On Fri, 2015-03-20 at 15:18 +0100, Takashi Iwai wrote: > > > > At Fri, 20 Mar 2015 13:49:24 +0000, Jie, Yang wrote: > > > > > > > > > > > -----Original Message----- > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > > > Sent: Friday, March 20, 2015 9:21 PM > > > > > > To: Jie, Yang > > > > > > Cc: perex@perex.cz; broonie@kernel.org; > > > > > > alsa-devel@alsa-project.org; Girdwood, Liam R; Liam Girdwood; > > > > > > Tanu Kaskinen > > > > > > (tanu.kaskinen@linux.intel.com) > > > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols > > > > > > for every jack input device > > > > > > > > > > > > At Fri, 20 Mar 2015 12:50:33 +0000, Jie, Yang wrote: > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > > > > > Sent: Friday, March 20, 2015 8:27 PM > > > > > > > > To: Jie, Yang > > > > > > > > Cc: perex@perex.cz; broonie@kernel.org; > > > > > > > > alsa-devel@alsa-project.org; Girdwood, Liam R; Liam > > > > > > > > Girdwood > > > > > > > > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack > > > > > > > > kcontrols for every jack input device > > > > > > > > > > > > > > > > At Fri, 20 Mar 2015 12:22:10 +0000, Jie, Yang wrote: > > > > > > > > > > > > > > > > > > > > +} > > > > > > > > > > > + > > > > > > > > > > > +static int snd_jack_new_kctl(struct snd_card *card, > > > > > > > > > > > +struct snd_jack *jack, int type) { > > > > > > > > > > > + struct snd_kcontrol *kctl; > > > > > > > > > > > + struct snd_jack_kctl *jack_kctl; > > > > > > > > > > > + int i, err, index, state = 0 /* use 0 for default state ?? > > > > > > > > > > > +*/; > > > > > > > > > > > + > > > > > > > > > > > + INIT_LIST_HEAD(&jack->kctl_list); > > > > > > > > > > > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > > > > > > > > > > > + int testbit = 1 << i; > > > > > > > > > > > + if (type & testbit) { > > > > > > > > > > > > > > > > > > > > With this implementation, you'll get multiple boolean > > > > > > > > > > kctls for a headset. I thought we agreed with > > > > > > > > > > creating a single boolean for > > > > > > headset? > > > > > > > > > [Keyon] We agreed with creating multiple kctls for combo jack, > e.g. > > > > > > > > headset. > > > > > > > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET > > > > > > > > > | SND_JACK_BTN_0, we will create 3 kctls for the jack, > > > > > > > > > when > > > > > > > > > BTN_0 is pressed, we will report to the 3rd kctl. > > > > > > > > > > > > > > > > Hm, I don't remember that I agreed with multiple kctls... > > > > > > > > > > > > > > > > The multiple kctls have a significant drawback (multiple > > > > > > > > event calls for a single > > > > > > > > headset) and its behavior is incompatible with the current > > > > > > > > code (both the name change and the behavior change). That > > > > > > > > is, your patch will very likely break the existing applications. > > > > > > > [Keyon] I am not very clear with the existing applications > > > > > > > that using these kctl events(seems Android use input subsystem > event? > > > > > > > Which we didn't Change here. If I understand correctly, > > > > > > > Pulseaudio uses jack switch controls, via the name, then we > > > > > > > can use different name for headphone and mic here.) > > > > > > > > > > > > PA uses jack kctls. > > > > > > > > > > > > If you rename, how would you guarantee that the existing > > > > > > application will work as expected? PA doesn't have the > > > > > > definition of > > > "Headset Speaker Jack" > > > > > > or such. > > > > > > > > > > > > And, no, we have no option "fix PA". Other way round: we are > > > > > > not allowed to break the current PA (or any user-space) behavior in > general. > > > > > > > > > > > > > we will generate 2 event calls(one headphone, one mic) when > > > > > > > Headset plug in/out, applications will receive these 2 > > > > > > > events, and they can do anything, e.g. decide to switch > > > > > > > on/off > > > speaker/headphone. > > > > > > > > > > > > Won't this break any user-space stuff? > > > > > > > > > > > > > BTW, I haven't implemented the generating of combo jack kctls' > > > > > > > name yet, currently, they looked like below: > > > > > > > numid=12,iface=CARD,name='Headset Jack' > > > > > > > numid=13,iface=CARD,name='Headset Jack',index=1 > > > > > > > numid=14,iface=CARD,name='Headset Jack',index=2 > > > > > > > > > > > > > > once we have come to agreement, we can modify it in > > > > > > > snd_jack_new_kctl(), e.g., "Headset Jack Mic" and "Headset > > > > > > > Jack > > > > > > Speakers". > > > > > > > > > > > > .... and how the existing user-space works without changing its code? > > > > > > > > > > > > > > > > > > Keyon, the most important point at this moment is to keep the > > > compatibility. > > > > > > HD-audio is no new driver. It's a driver that has been > > > > > > present over a decade with (literally) thousands of variants. > > > > > > Please keep this in mind, and reconsider whether your patch > > > > > > will retain the compatibility, especially with PulseAudio. > > > > > [Keyon] understood. Then we should follow the HD-audio style, > > > > > So, what do you suggest here? Should we create only one single > > > > > Boolean kctls for headset, and report true when status in > > > > > headphone bit it true? Then we need a tricky exception mapping here? > > > > > > > > > > Sorry, I am a little confusing here, because Mark suggest to > > > > > create multiple controls for multiple bits jack, and you also > > > > > agreed with that. :( > > > > > > > > Just prepare two exceptions for SND_JACK_HEADSET and > > > SND_JACK_VIDEOUT. > > > > These are already defined as single types in sound/jack.h. The > > > > code can't be so tricky if you write properly :) > > > > > > Can someone clarify what is the current plan? Will there be changes > > > to the control naming or behaviour for existing hardware? > > [Keyon] Hi Tanu, currently we plan to create kctls/switches at jack > > creating stage, and attached them to the jack. > > So, we need to decide the naming of these kctls/switches and make sure > > they will be understood by PA correctly. > > So the plan is to avoid any changes to what kctls applications see? > That's very good. > > > At the same time, we create jacks with snd_jack_types as parameter > > passed in, so, we need decision about what kctls/switches do we need > > create, for each separate or combo(bits) type jack: > > enum snd_jack_types { > > SND_JACK_HEADPHONE = 0x0001, > > SND_JACK_MICROPHONE = 0x0002, > > SND_JACK_HEADSET = SND_JACK_HEADPHONE | > SND_JACK_MICROPHONE, > > SND_JACK_LINEOUT = 0x0004, > > SND_JACK_MECHANICAL = 0x0008, /* If detected separately */ > > SND_JACK_VIDEOOUT = 0x0010, > > SND_JACK_AVOUT = SND_JACK_LINEOUT | SND_JACK_VIDEOOUT, > > SND_JACK_LINEIN = 0x0020, > > > > /* Kept separate from switches to facilitate implementation */ > > SND_JACK_BTN_0 = 0x4000, > > SND_JACK_BTN_1 = 0x2000, > > SND_JACK_BTN_2 = 0x1000, > > SND_JACK_BTN_3 = 0x0800, > > SND_JACK_BTN_4 = 0x0400, > > SND_JACK_BTN_5 = 0x0200, > > }; > > > > So, > > 1. we need these naming, what you summarized below, that's quite helpful. > > David's summary is even more helpful! > > > 2. I am a little concern if the exist snd_jack_types enum can indicate > > all diverse existing hardware, e.g. " Headphone Mic Jack " as you > > mentioned below, can we use any bits combination for this kind of jack? > > SND_JACK_HEADPHONE | SND_JACK_MICROPHONE seems can't tell > difference > > With "Headset Mic Jack" + "Headphone Jack"? > > Yes, it seems the existing enumeration isn't able to do that distinction. Can > the enumeration be extended? [Keyon] I think we can extend it if needed. > > Another thing that might be a problem is that this jack enumeration doesn't > make distinction between what device types can be used with the jack, and > what level of device type detection is available. That is, is a headset jack only [Keyon] our goal for this patch set should be achieve this, IMO. > able to tell that "something was plugged in", or is it also able to tell whether > that something was headphones, a microphone or a headset. (I suppose this > might essentially be the same topic as discussed in the "ALSA: jack: > Refactoring for jack kctls" thread about phantom jacks.) [Keyon] yes, we creating kctls and adding them to the kctl list which belong to the jack, hopefully, it may help giving out these information. > > -- > Tanu
> > Headphone or mic, one hw switch: > [Keyon]I am fresh about this kind of hw jack, it should use different > segment of the plug, seems we don't need check the actual connected > status at the jack creation stage, just creating "Headphone Mic Jack" > should works, right? > > > > * "Headphone Mic Jack" > > > https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/sound/pci/hda?id=5780b627e24113323427c102175296ae43dfb9d7 http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=7369a53ab5f606e87a3cd1cd4eebd40226bab090 http://www.asus.com/Notebooks_Ultrabooks/Eee_PC_1015PX/specifications/
>> >> One thing that is unclear for me is that how are those jacks represented >> that support any of headsets/headphones/microphones, but don't provide >> information about which device type has been plugged in. > > > For headphone or headset, independent switches: > > * "Headphone Jack" > * "Headset Mic Jack" > > For headphone or headset, one hw switch only: > > * "Headphone Jack" > * "Headset Mic Phantom Jack" https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/log/sound/pci/hda?qt=grep&q=dell-headset-multi I don't understand how can Dell OptiPlex 3030 All-in-One and XPS 15 laoptop can use the same pin config "dell-headset-multi" ? There is line out jack in aio spec : Universal Headset (Side)1 Line-out(Rear) While xps 15 notebook has internal speaker and headset
On 2015-03-25 14:53, Jie, Yang wrote: > > Thank you for comprehensive summarizing, David. > Can you help give more description as below? > > ~Keyon > >> -----Original Message----- >> From: David Henningsson [mailto:david.henningsson@canonical.com] >> Sent: Monday, March 23, 2015 7:51 PM >> To: Tanu Kaskinen; Takashi Iwai >> Cc: Jie, Yang; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa- >> project.org; Girdwood, Liam R; Liam Girdwood >> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack >> input device >> >> >> >> On 2015-03-23 11:56, Tanu Kaskinen wrote: >>> One thing that is unclear for me is that how are those jacks >>> represented that support any of headsets/headphones/microphones, but >>> don't provide information about which device type has been plugged in. >> >> For headphone or headset, independent switches: >> >> * "Headphone Jack" >> * "Headset Mic Jack" > [Keyon] here for the most common headset jack > (SND_JACK_HEADPHONE | SND_JACK_MICROPHONE), we should create > two independent kctls/switches--"Headphone Jack" + "Headset Mic Jack", > right? > > so, is it possible that there is only "Mic Jack" in this case? I mean that > only input(no output, no physical headphone/speaker jack) jack exist. > > If yes, then we may need change "Headset Mic Jack" to "Mic Jack"? I'm not sure I understand your question - of course there are input only jacks, but they would then be SND_JACK_MICROPHONE only, and also labelled "Mic Jack". But that's not a headset jack, that's a microphone jack. >> For headphone or headset, one hw switch only: > [Keyon] I am sorry I am not familiar with the jack HW circuit. What "one > hw switch only" here means? Does it means that -- for headset Jack, the > status(connected/disconnected) of HP pin is always exactly same with > that of Mic pin? There is only one jack detection input from HW. Regardless of whether you plug in headphone or a headset, the HW switch would detect "plugged in". When the jack is unplugged, the HW switch would detect "unplugged". The HW cannot tell us whether you plugged in a headphone or headset. >> * "Headphone Jack" >> * "Headset Mic Phantom Jack" > [Keyon] for Headset of this type, do we need create only "Headset Mic > Phantom Jack" kctl, or "Headphone Jack" kctl is also needed? We need both kctls. >> Headphone or mic, one hw switch: > [Keyon]I am fresh about this kind of hw jack, it should use different > segment of the plug, seems we don't need check the actual connected > status at the jack creation stage, just creating "Headphone Mic Jack" > should works, right? It is not very common. It was used on some Asus netbooks a while ago. But yes, we should just create a "Headphone Mic Jack". >> >> * "Headphone Mic Jack" >> >> Headphone, headset, or mic, one hw switch only: > [Keyon] how many kctls should we create for this? >> >> * "Headphone Mic Jack" >> * "Headset Mic Phantom Jack" Same as today, two: "Headphone Mic Jack" and "Headset Mic Phantom Jack". >> Headphone, headset, or mic, one switch for hp/mic and the other for the >> headset mic: > [Keyon] I can't imagine how this works, it's too messy for me. :( >> >> * "Headphone Mic Jack" >> * "Headset Mic Jack" Nothing plugged in: "Headphone Mic Jack" = false, "Headset Mic Jack" = false Headphones plugged in: "Headphone Mic Jack" = true, "Headset Mic Jack" = false Headset plugged in: "Headphone Mic Jack" = true, "Headset Mic Jack" = true Mic plugged in: "Headphone Mic Jack" = true, "Headset Mic Jack" = false As you can see, "Headphones" and "Mic" results in the same output, hence userspace needs to ask the user if (s)he plugged in a headphone or mic.
Thank you so much, David! So, I summarized as below table: Jack Type kctls/switches name detection description Headphone Headphone Jack HP plugged in/unplugged Mic Mic Jack Mic plugged in/unplugged Headset Headphone Jack Nothing plugged in: ""Headphone Jack"" = false, ""Headset Mic Jack"" = false Headset Mic Jack" Headphones plugged in: ""Headphone Jack"" = true, ""Headset Mic Jack"" = false Headset plugged in: ""Headphone Jack"" = true, ""Headset Mic Jack"" = true Mic plugged in: ""Headphone Jack"" = true ""Headset Mic Jack"" = false(should not plugged in Mic, detect wrongly)" independent switches Headset Mic Headphone Jack Nothing plugged in: ""Headphone Jack"" = false, ""Headset Mic Phantom Jack"" = false Headset Mic Phantom Jack Headphones plugged in: ""Headphone Jack"" = true, ""Headset Mic Phantom Jack"" = false? Headset plugged in: ""Headphone Jack"" = false, ""Headset Mic Phantom Jack"" = true ? Mic plugged in: ""Headphone Jack"" = true, ""Headset Mic Phantom Jack"" = false(should not plugged in Mic, detect wrongly)" one hw switch only Headphone Mic Headphone Mic Jack Nothing plugged in: ""Headphone Mic Jack"" = false Headphones plugged in: ""Headphone Mic Jack"" = true Mic plugged in: ""Headphone Mic Jack"" = true Headset plugged in: ""Headphone Mic Jack"" = true?" one hw switch Headset Headphone Mic Headphone Mic Jack Nothing plugged in: ""Headphone Mic Jack"" = false, ""Headset Mic Phantom Jack"" = false Headset Mic Phantom Jack Headphones plugged in: ""Headphone Mic Jack"" = true, ""Headset Mic Phantom Jack"" = false Headset plugged in: ""Headphone Mic Jack"" = false, ""Headset Mic Phantom Jack"" = true ? Mic plugged in: ""Headphone Mic Jack"" = true, ""Headset Mic Phantom Jack"" = false" one hw switch only Headphone Mic Headset Headphone Mic Jack Nothing plugged in: ""Headphone Mic Jack"" = false, ""Headset Mic Jack"" = false Headset Mic Jack Headphones plugged in: ""Headphone Mic Jack"" = true, ""Headset Mic Jack"" = false Headset plugged in: ""Headphone Mic Jack"" = true, ""Headset Mic Jack"" = true Mic plugged in: ""Headphone Mic Jack"" = true, ""Headset Mic Jack"" = false one switch for hp/mic and the other for the headset mic so, we may need extend snd_jack_types enum, by adding types: Headset Mic, Headphone Mic, Headset Headphone Mic, Headphone Mic Headset. enum snd_jack_types { SND_JACK_HEADPHONE = 0x0001, SND_JACK_MICROPHONE = 0x0002, SND_JACK_HEADSET = SND_JACK_HEADPHONE | SND_JACK_MICROPHONE, SND_JACK_LINEOUT = 0x0004, SND_JACK_MECHANICAL = 0x0008, /* If detected separately */ SND_JACK_VIDEOOUT = 0x0010, SND_JACK_AVOUT = SND_JACK_LINEOUT | SND_JACK_VIDEOOUT, SND_JACK_LINEIN = 0x0020, SND_JACK_HEADSET_MIC = 0x0040, /* one hw switch only */ SND_JACK_HEADPHONE_MIC = 0x0080, /* one hw switch only, headphone, or mic */ SND_JACK_HEADSET_ HEADPHONE_MIC = 0x0100, /* one hw switch only, headset, headphone, or mic*/ SND_JACK_ HEADPHONE_HEADSET_ MIC = 0x0200, /* headset, headphone, or mic; one switch for hp/mic and the other for the headset mic */ /* Kept separate from switches to facilitate implementation */ SND_JACK_BTN_0 = 0x8000, SND_JACK_BTN_1 = 0x4000, SND_JACK_BTN_2 = 0x2000, SND_JACK_BTN_3 = 0x1000, SND_JACK_BTN_4 = 0x0800, SND_JACK_BTN_5 = 0x0400, }; And then, we may create corresponding kctls during jack creating. Any comment? ~Keyon > -----Original Message----- > From: David Henningsson [mailto:david.henningsson@canonical.com] > Sent: Thursday, March 26, 2015 2:42 PM > To: Jie, Yang; Tanu Kaskinen; Takashi Iwai > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > Girdwood, Liam R; Liam Girdwood > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > input device > > > > On 2015-03-25 14:53, Jie, Yang wrote: > > > > Thank you for comprehensive summarizing, David. > > Can you help give more description as below? > > > > ~Keyon > > > >> -----Original Message----- > >> From: David Henningsson [mailto:david.henningsson@canonical.com] > >> Sent: Monday, March 23, 2015 7:51 PM > >> To: Tanu Kaskinen; Takashi Iwai > >> Cc: Jie, Yang; perex@perex.cz; broonie@kernel.org; alsa-devel@alsa- > >> project.org; Girdwood, Liam R; Liam Girdwood > >> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for > >> every jack input device > >> > >> > >> > >> On 2015-03-23 11:56, Tanu Kaskinen wrote: > >>> One thing that is unclear for me is that how are those jacks > >>> represented that support any of headsets/headphones/microphones, > but > >>> don't provide information about which device type has been plugged in. > >> > >> For headphone or headset, independent switches: > >> > >> * "Headphone Jack" > >> * "Headset Mic Jack" > > [Keyon] here for the most common headset jack (SND_JACK_HEADPHONE > | > > SND_JACK_MICROPHONE), we should create two independent > > kctls/switches--"Headphone Jack" + "Headset Mic Jack", right? > > > > so, is it possible that there is only "Mic Jack" in this case? I mean > > that only input(no output, no physical headphone/speaker jack) jack exist. > > > > If yes, then we may need change "Headset Mic Jack" to "Mic Jack"? > > I'm not sure I understand your question - of course there are input only jacks, > but they would then be SND_JACK_MICROPHONE only, and also labelled > "Mic Jack". But that's not a headset jack, that's a microphone jack. > > >> For headphone or headset, one hw switch only: > > [Keyon] I am sorry I am not familiar with the jack HW circuit. What > > "one hw switch only" here means? Does it means that -- for headset > > Jack, the > > status(connected/disconnected) of HP pin is always exactly same with > > that of Mic pin? > > There is only one jack detection input from HW. Regardless of whether you > plug in headphone or a headset, the HW switch would detect "plugged in". > When the jack is unplugged, the HW switch would detect "unplugged". > > The HW cannot tell us whether you plugged in a headphone or headset. > > >> * "Headphone Jack" > >> * "Headset Mic Phantom Jack" > > [Keyon] for Headset of this type, do we need create only "Headset Mic > > Phantom Jack" kctl, or "Headphone Jack" kctl is also needed? > > We need both kctls. > > >> Headphone or mic, one hw switch: > > [Keyon]I am fresh about this kind of hw jack, it should use different > > segment of the plug, seems we don't need check the actual connected > > status at the jack creation stage, just creating "Headphone Mic Jack" > > should works, right? > > It is not very common. It was used on some Asus netbooks a while ago. > But yes, we should just create a "Headphone Mic Jack". > > >> > >> * "Headphone Mic Jack" > >> > >> Headphone, headset, or mic, one hw switch only: > > [Keyon] how many kctls should we create for this? > >> > >> * "Headphone Mic Jack" > >> * "Headset Mic Phantom Jack" > > Same as today, two: "Headphone Mic Jack" and "Headset Mic Phantom Jack". > > >> Headphone, headset, or mic, one switch for hp/mic and the other for > >> the headset mic: > > [Keyon] I can't imagine how this works, it's too messy for me. :( > >> > >> * "Headphone Mic Jack" > >> * "Headset Mic Jack" > > Nothing plugged in: > "Headphone Mic Jack" = false, "Headset Mic Jack" = false Headphones > plugged in: > "Headphone Mic Jack" = true, "Headset Mic Jack" = false Headset plugged > in: > "Headphone Mic Jack" = true, "Headset Mic Jack" = true Mic plugged in: > "Headphone Mic Jack" = true, "Headset Mic Jack" = false > > As you can see, "Headphones" and "Mic" results in the same output, hence > userspace needs to ask the user if (s)he plugged in a headphone or mic. > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic
On 2015-03-26 09:29, Jie, Yang wrote: > > Thank you so much, David! > > So, I summarized as below table: I've added how I see them with the new API, see below for further comments: > > Jack Type kctls/switches name detection description > > Headphone Headphone Jack HP plugged in/unplugged SND_JACK_HEADPHONE > > Mic Mic Jack Mic plugged in/unplugged SND_JACK_MICROPHONE > > Headset Headphone Jack Nothing plugged in: ""Headphone Jack"" = false, ""Headset Mic Jack"" = false > Headset Mic Jack" Headphones plugged in: ""Headphone Jack"" = true, ""Headset Mic Jack"" = false > Headset plugged in: ""Headphone Jack"" = true, ""Headset Mic Jack"" = true > Mic plugged in: ""Headphone Jack"" = true ""Headset Mic Jack"" = false(should not plugged in Mic, detect wrongly)" independent switches SND_JACK_HEADSET "Mic plugged in" case is irrelevant - not supported by hw > > Headset Mic Headphone Jack Nothing plugged in: ""Headphone Jack"" = false, ""Headset Mic Phantom Jack"" = false > Headset Mic Phantom Jack Headphones plugged in: ""Headphone Jack"" = true, ""Headset Mic Phantom Jack"" = false? > Headset plugged in: ""Headphone Jack"" = false, ""Headset Mic Phantom Jack"" = true ? > Mic plugged in: ""Headphone Jack"" = true, ""Headset Mic Phantom Jack"" = false(should not plugged in Mic, detect wrongly)" one hw switch only SND_JACK_HEADPHONE, the "Headset Mic Phantom Jack" is created manually > Headphone Mic Headphone Mic Jack Nothing plugged in: ""Headphone Mic Jack"" = false > Headphones plugged in: ""Headphone Mic Jack"" = true > Mic plugged in: ""Headphone Mic Jack"" = true > Headset plugged in: ""Headphone Mic Jack"" = true?" one hw switch SND_JACK_HEADPHONE (probably) > Headset Headphone Mic Headphone Mic Jack Nothing plugged in: ""Headphone Mic Jack"" = false, ""Headset Mic Phantom Jack"" = false > Headset Mic Phantom Jack Headphones plugged in: ""Headphone Mic Jack"" = true, ""Headset Mic Phantom Jack"" = false > Headset plugged in: ""Headphone Mic Jack"" = false, ""Headset Mic Phantom Jack"" = true ? > Mic plugged in: ""Headphone Mic Jack"" = true, ""Headset Mic Phantom Jack"" = false" one hw switch only SND_JACK_HEADPHONE (probably), the "Headset Mic Phantom Jack" is created manually > Headphone Mic Headset Headphone Mic Jack Nothing plugged in: ""Headphone Mic Jack"" = false, ""Headset Mic Jack"" = false > Headset Mic Jack Headphones plugged in: ""Headphone Mic Jack"" = true, ""Headset Mic Jack"" = false > Headset plugged in: ""Headphone Mic Jack"" = true, ""Headset Mic Jack"" = true > Mic plugged in: ""Headphone Mic Jack"" = true, ""Headset Mic Jack"" = false > one switch for hp/mic and the other for the headset mic SND_JACK_HEADSET (probably) > > so, we may need extend snd_jack_types enum, by adding types: Headset Mic, Headphone Mic, Headset Headphone Mic, Headphone Mic Headset. > > enum snd_jack_types { > SND_JACK_HEADPHONE = 0x0001, > SND_JACK_MICROPHONE = 0x0002, > SND_JACK_HEADSET = SND_JACK_HEADPHONE | SND_JACK_MICROPHONE, > SND_JACK_LINEOUT = 0x0004, > SND_JACK_MECHANICAL = 0x0008, /* If detected separately */ > SND_JACK_VIDEOOUT = 0x0010, > SND_JACK_AVOUT = SND_JACK_LINEOUT | SND_JACK_VIDEOOUT, > SND_JACK_LINEIN = 0x0020, > > SND_JACK_HEADSET_MIC = 0x0040, /* one hw switch only */ > SND_JACK_HEADPHONE_MIC = 0x0080, /* one hw switch only, headphone, or mic */ > SND_JACK_HEADSET_ HEADPHONE_MIC = 0x0100, /* one hw switch only, headset, headphone, or mic*/ > SND_JACK_ HEADPHONE_HEADSET_ MIC = 0x0200, /* headset, headphone, or mic; one switch for hp/mic and the other for the headset mic */ This seems overly complex. I don't think we really need to add all of that. First, for the phantom jacks. We'll need phantom jacks not only for "Headset Mic Phantom Jack" but also for "Internal Speaker Phantom Jack" and "Internal Mic Phantom Jack", and probably some others as well. So that means that the HDA driver needs to still create some kctls using the current kctl API instead of your new unified API. A phantom jack is merely a marker to userspace indicating what hardware is present. It always returns "plugged in" when trying to read it. With the phantom jacks dealt with separately, the question remains about the Headphone Mic stuff. If the name remains the way it is, maybe all we need is a SND_JACK_HEADPHONE (or SND_JACK_HEADSET) jack with the "Headphone Mic Jack" name and then userspace would know that this jack could potentially accomodate a microphone too. Otherwise we might need to add a SND_JACK_HEADPHONE_OR_MIC constant, but I prefer that the names are kept properly, because there might be other relevant info added to the name too (e g "Front Headphone Jack" instead of "Headphone Jack").
OK, then I am thinking we can add jack kctls creating code like below: snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jjack) { ... Switch(type | SND_JACK_HEADSET) { case SND_JACK_MICROPHONE: create "Mic Jack" kctl; break; case SND_JACK_HEADSET: if (id == "Headphone Mic Headset") { create " Headphone Mic Jack " kctl; create " Headset Mic Jack " kctl; } else { create " Headphone Jack " kctl; create " Headset Mic Jack " kctl; } break; case SND_JACK_HEADPHONE: if (id == "Headset Mic") { create " Headphone Jack " kctl; // create " Headset Mic Phantom Jack " kctl; } else if (id == "Headphone Mic") { create " Headphone Mic Jack " kctl; } else if (id == "Headset Headphone Mic") { create " Headphone Mic Jack " kctl; // create " Headset Mic Phantom Jack " kctl; } else { create " Headphone Jack " kctl; } break; default: create "Mic Jack" kctl; break; } ... } Jack Type Jack name kctls/switches name SND_JACK_HEADPHONE Headphone Headphone Jack SND_JACK_MICROPHONE Mic Mic Jack SND_JACK_HEADSET Headset Headphone Jack Headset Mic Jack SND_JACK_HEADPHONE Headset Mic Headphone Jack Headset Mic Phantom Jack SND_JACK_HEADPHONE Headphone Mic Headphone Mic Jack SND_JACK_HEADPHONE Headset Headphone Mic Headphone Mic Jack Headset Mic Phantom Jack SND_JACK_HEADSET Headphone Mic Headset Headphone Mic Jack Headset Mic Jack ~Keyon > -----Original Message----- > From: David Henningsson [mailto:david.henningsson@canonical.com] > Sent: Thursday, March 26, 2015 5:06 PM > To: Jie, Yang; Tanu Kaskinen; Takashi Iwai > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > Girdwood, Liam R; Liam Girdwood > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > input device > > > > On 2015-03-26 09:29, Jie, Yang wrote: > > > > Thank you so much, David! > > > > So, I summarized as below table: > > I've added how I see them with the new API, see below for further > comments: > > > > > Jack Type kctls/switches name > detection > > description > > > > Headphone Headphone Jack > HP plugged in/unplugged > > SND_JACK_HEADPHONE > > > > > Mic Mic Jack > Mic plugged in/unplugged > > SND_JACK_MICROPHONE > > > > > Headset Headphone Jack > Nothing plugged in: ""Headphone Jack"" = > false, ""Headset Mic Jack"" = false > > Headset Mic Jack" > Headphones plugged in: ""Headphone > Jack"" = true, ""Headset Mic Jack"" = false > > > Headset plugged in: ""Headphone Jack"" = > true, ""Headset Mic Jack"" = true > > > Mic plugged in: ""Headphone Jack"" = true > ""Headset Mic Jack"" = false(should not plugged in Mic, detect wrongly)" > independent switches > > SND_JACK_HEADSET > > "Mic plugged in" case is irrelevant - not supported by hw > > > > > Headset Mic Headphone Jack > Nothing plugged in: ""Headphone Jack"" = > false, ""Headset Mic Phantom Jack"" = false > > Headset Mic Phantom Jack > Headphones plugged in: ""Headphone > Jack"" = true, ""Headset Mic Phantom Jack"" = false? > > > Headset plugged in: ""Headphone Jack"" = > false, ""Headset Mic Phantom Jack"" = true ? > > > Mic plugged in: ""Headphone Jack"" = true, > ""Headset Mic Phantom Jack"" = false(should not plugged in Mic, detect > wrongly)" one hw switch only > > SND_JACK_HEADPHONE, the "Headset Mic Phantom Jack" is created > manually > > > Headphone Mic Headphone Mic Jack > Nothing plugged in: ""Headphone Mic Jack"" > = false > > > Headphones plugged in: ""Headphone Mic > Jack"" = true > > > Mic plugged in: ""Headphone Mic Jack"" = > true > > > Headset plugged in: ""Headphone Mic > Jack"" = true?" > one hw switch > > SND_JACK_HEADPHONE (probably) > > > Headset Headphone Mic Headphone Mic Jack > Nothing plugged in: ""Headphone Mic Jack"" > = false, ""Headset Mic Phantom Jack"" = false > > Headset Mic Phantom Jack > Headphones plugged in: ""Headphone Mic > Jack"" = true, ""Headset Mic Phantom Jack"" = false > > > Headset plugged in: ""Headphone Mic > Jack"" = false, ""Headset Mic Phantom Jack"" = true ? > > > Mic plugged in: ""Headphone Mic Jack"" = > true, ""Headset Mic Phantom Jack"" = false" > one hw switch only > > SND_JACK_HEADPHONE (probably), the "Headset Mic Phantom Jack" is > created manually > > > Headphone Mic Headset Headphone Mic Jack > Nothing plugged in: ""Headphone Mic Jack"" > = false, ""Headset Mic Jack"" = false > > Headset Mic Jack > Headphones plugged in: ""Headphone Mic > Jack"" = true, ""Headset Mic Jack"" = false > > > Headset plugged in: ""Headphone Mic > Jack"" = true, ""Headset Mic Jack"" = true > > > Mic plugged in: ""Headphone Mic Jack"" = > true, ""Headset Mic Jack"" = false > > > > one switch for hp/mic > and the other for the > > headset mic > > SND_JACK_HEADSET (probably) > > > > > so, we may need extend snd_jack_types enum, by adding types: Headset > Mic, Headphone Mic, Headset Headphone Mic, Headphone Mic Headset. > > > > enum snd_jack_types { > > SND_JACK_HEADPHONE = 0x0001, > > SND_JACK_MICROPHONE = 0x0002, > > SND_JACK_HEADSET = SND_JACK_HEADPHONE | > SND_JACK_MICROPHONE, > > SND_JACK_LINEOUT = 0x0004, > > SND_JACK_MECHANICAL = 0x0008, /* If detected separately */ > > SND_JACK_VIDEOOUT = 0x0010, > > SND_JACK_AVOUT = SND_JACK_LINEOUT | > SND_JACK_VIDEOOUT, > > SND_JACK_LINEIN = 0x0020, > > > > SND_JACK_HEADSET_MIC = 0x0040, /* one hw switch only */ > > SND_JACK_HEADPHONE_MIC = 0x0080, /* one hw switch only, > headphone, or mic */ > > SND_JACK_HEADSET_ HEADPHONE_MIC = 0x0100, /* one hw > switch only, headset, headphone, or mic*/ > > SND_JACK_ HEADPHONE_HEADSET_ MIC = 0x0200, /* headset, > headphone, or mic; one switch for hp/mic and the other for the headset mic > */ > > This seems overly complex. I don't think we really need to add all of that. > > First, for the phantom jacks. We'll need phantom jacks not only for "Headset > Mic Phantom Jack" but also for "Internal Speaker Phantom Jack" > and "Internal Mic Phantom Jack", and probably some others as well. So that > means that the HDA driver needs to still create some kctls using the current > kctl API instead of your new unified API. > > A phantom jack is merely a marker to userspace indicating what hardware is > present. It always returns "plugged in" when trying to read it. > > With the phantom jacks dealt with separately, the question remains about > the Headphone Mic stuff. If the name remains the way it is, maybe all we > need is a SND_JACK_HEADPHONE (or SND_JACK_HEADSET) jack with the > "Headphone Mic Jack" name and then userspace would know that this jack > could potentially accomodate a microphone too. Otherwise we might need > to add a SND_JACK_HEADPHONE_OR_MIC constant, but I prefer that the > names are kept properly, because there might be other relevant info added > to the name too (e g "Front Headphone Jack" instead of "Headphone Jack"). > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic
Sorry, Correct a typo(removing line "create "Mic Jack" kctl;" in default case) snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jjack) { ... Switch(type | SND_JACK_HEADSET) { case SND_JACK_MICROPHONE: create "Mic Jack" kctl; break; case SND_JACK_HEADSET: if (id == "Headphone Mic Headset") { create " Headphone Mic Jack " kctl; create " Headset Mic Jack " kctl; } else { create " Headphone Jack " kctl; create " Headset Mic Jack " kctl; } break; case SND_JACK_HEADPHONE: if (id == "Headset Mic") { create " Headphone Jack " kctl; // create " Headset Mic Phantom Jack " kctl; } else if (id == "Headphone Mic") { create " Headphone Mic Jack " kctl; } else if (id == "Headset Headphone Mic") { create " Headphone Mic Jack " kctl; // create " Headset Mic Phantom Jack " kctl; } else { create " Headphone Jack " kctl; } break; default: break; } ... } Jack Type Jack name kctls/switches name SND_JACK_HEADPHONE Headphone Headphone Jack SND_JACK_MICROPHONE Mic Mic Jack SND_JACK_HEADSET Headset Headphone Jack Headset Mic Jack SND_JACK_HEADPHONE Headset Mic Headphone Jack Headset Mic Phantom Jack SND_JACK_HEADPHONE Headphone Mic Headphone Mic Jack SND_JACK_HEADPHONE Headset Headphone Mic Headphone Mic Jack Headset Mic Phantom Jack SND_JACK_HEADSET Headphone Mic Headset Headphone Mic Jack Headset Mic Jack ~Keyon
l > > > OK, then I am thinking we can add jack kctls creating code like below: > > snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jjack) { > ... > Switch(type | SND_JACK_HEADSET) > { > case SND_JACK_MICROPHONE: > create "Mic Jack" kctl; > break; > case SND_JACK_HEADSET: > if (id == "Headphone Mic Headset") { > create " Headphone Mic Jack " kctl; > create " Headset Mic Jack " kctl; > } else { > create " Headphone Jack " kctl; > create " Headset Mic Jack " kctl; > } > break; > case SND_JACK_HEADPHONE: > if (id == "Headset Mic") { > create " Headphone Jack " kctl; > // create " Headset Mic Phantom Jack " kctl; > } else if (id == "Headphone Mic") { > create " Headphone Mic Jack " kctl; > } else if (id == "Headset Headphone Mic") { > create " Headphone Mic Jack " kctl; > // create " Headset Mic Phantom Jack " kctl; > } else { > create " Headphone Jack " kctl; > } > break; > default: > create "Mic Jack" kctl; > break; > } > ... > } https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/pci/hda/hda_jack.c If you look at snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid, const char *name, int idx) which add kctl and call snd_jack_new if it is not phantom It is unlikely snd_jack_new() create those kctl For mobile phone or tablet , there is one and only one input : headset for notebook and desktop, there are dual headphone jacks, four line out jacks for 7.1 surround, line in, phantom jack for spdif, surround speakers , hp and mic for those desktop using ac97 front audio panel
From: Raymond Yau [mailto:superquad.vortex2@gmail.com] Sent: Friday, March 27, 2015 8:39 AM To: Jie, Yang Cc: ALSA Development Mailing List; Liam Girdwood; Takashi Iwai; David Henningsson; Girdwood, Liam R; Tanu Kaskinen; broonie@kernel.org Subject: Re: [alsa-devel] [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device l > > > OK, then I am thinking we can add jack kctls creating code like below: > > snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jjack) { > ... > Switch(type | SND_JACK_HEADSET) > { > case SND_JACK_MICROPHONE: > create "Mic Jack" kctl; > break; > case SND_JACK_HEADSET: > if (id == "Headphone Mic Headset") { > create " Headphone Mic Jack " kctl; > create " Headset Mic Jack " kctl; > } else { > create " Headphone Jack " kctl; > create " Headset Mic Jack " kctl; > } > break; > case SND_JACK_HEADPHONE: > if (id == "Headset Mic") { > create " Headphone Jack " kctl; > // create " Headset Mic Phantom Jack " kctl; > } else if (id == "Headphone Mic") { > create " Headphone Mic Jack " kctl; > } else if (id == "Headset Headphone Mic") { > create " Headphone Mic Jack " kctl; > // create " Headset Mic Phantom Jack " kctl; > } else { > create " Headphone Jack " kctl; > } > break; > default: > create "Mic Jack" kctl; > break; > } > ... > } https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/pci/hda/hda_jack.c If you look at snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid, const char *name, int idx) which add kctl and call snd_jack_new if it is not phantom [Keyon] it’s good that we don’t need handle phantom jack in snd_jack_new(). It is unlikely snd_jack_new() create those kctl For mobile phone or tablet , there is one and only one input : headset [Keyon] that’s OK. for notebook and desktop, there are dual headphone jacks, four line out jacks for 7.1 surround, line in, phantom jack for spdif, surround speakers , hp and mic for those desktop using ac97 front audio panel [Keyon] we will create kctls for each headphone/mic/headset jacks, but not for line in/out and phantom jacks, is this OK?
On 2015-03-26 13:43, Jie, Yang wrote: > > Sorry, Correct a typo(removing line "create "Mic Jack" kctl;" in default case) It looks like your code can be simplified as: switch (type | SND_JACK_HEADSET) { case SND_JACK_HEADSET: create "Headset Mic Jack" kctl; /* fall through */ case SND_JACK_MICROPHONE: case SND_JACK_HEADPHONE: create format("%s Jack", id) kctl; } ...that way, prefixes such as "Front Headphone Jack" are preserved too, which is a good thing. // David > > snd_jack_new(struct snd_card *card, const char *id, int type, struct snd_jack **jjack) { ... > Switch(type | SND_JACK_HEADSET) > { > case SND_JACK_MICROPHONE: > create "Mic Jack" kctl; > break; > case SND_JACK_HEADSET: > if (id == "Headphone Mic Headset") { > create " Headphone Mic Jack " kctl; > create " Headset Mic Jack " kctl; > } else { > create " Headphone Jack " kctl; > create " Headset Mic Jack " kctl; > } > break; > case SND_JACK_HEADPHONE: > if (id == "Headset Mic") { > create " Headphone Jack " kctl; > // create " Headset Mic Phantom Jack " kctl; > } else if (id == "Headphone Mic") { > create " Headphone Mic Jack " kctl; > } else if (id == "Headset Headphone Mic") { > create " Headphone Mic Jack " kctl; > // create " Headset Mic Phantom Jack " kctl; > } else { > create " Headphone Jack " kctl; > } > break; > default: > break; > } > ... > } > > > Jack Type Jack name kctls/switches name > SND_JACK_HEADPHONE Headphone Headphone Jack > SND_JACK_MICROPHONE Mic Mic Jack > SND_JACK_HEADSET Headset Headphone Jack > Headset Mic Jack > SND_JACK_HEADPHONE Headset Mic Headphone Jack > Headset Mic Phantom Jack > SND_JACK_HEADPHONE Headphone Mic Headphone Mic Jack > SND_JACK_HEADPHONE Headset Headphone Mic Headphone Mic Jack > Headset Mic Phantom Jack > SND_JACK_HEADSET Headphone Mic Headset Headphone Mic Jack > Headset Mic Jack > > ~Keyon >
> -----Original Message----- > From: David Henningsson [mailto:david.henningsson@canonical.com] > Sent: Friday, March 27, 2015 2:50 PM > To: Jie, Yang; Tanu Kaskinen; Takashi Iwai > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > Girdwood, Liam R; Liam Girdwood > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > input device > > > > On 2015-03-26 13:43, Jie, Yang wrote: > > > > Sorry, Correct a typo(removing line "create "Mic Jack" kctl;" in > > default case) > > It looks like your code can be simplified as: [Keyon] thanks, I will try simplify them. > > switch (type | SND_JACK_HEADSET) { > case SND_JACK_HEADSET: > create "Headset Mic Jack" kctl; > /* fall through */ > case SND_JACK_MICROPHONE: > case SND_JACK_HEADPHONE: > create format("%s Jack", id) kctl; [Keyon] I don't want to fill "id" to kctl name, because sometimes it can be string which can't be understood by PA, such as: soc/intel/mfld_machine.c: ret_val = snd_soc_card_jack_new(runtime->card, soc/intel/mfld_machine.c- "Intel(R) MID Audio Jack", SND_JACK_HEADSET | soc/intel/mfld_machine.c- SND_JACK_BTN_0 | SND_JACK_BTN_1, &mfld_jack, soc/omap/ams-delta.c: ret = snd_soc_card_jack_new(card, "hook_switch", SND_JACK_HEADSET, soc/omap/ams-delta.c- &ams_delta_hook_switch, NULL, 0); > } > > ...that way, prefixes such as "Front Headphone Jack" are preserved too, [Keyon] if PA can recognize "Front Headphone Jack" and "Rear Headphone Jack", then I prefer to add a determine here: if (!strcmp(id, "Front Headphone Jack") || !strcmp(id, "Rear Headphone Jack")) create format("%s Jack", id) kctl; > which is a good thing. > > // David > > > > > snd_jack_new(struct snd_card *card, const char *id, int type, struct > snd_jack **jjack) { ... > > Switch(type | SND_JACK_HEADSET) > > { > > case SND_JACK_MICROPHONE: > > create "Mic Jack" kctl; > > break; > > case SND_JACK_HEADSET: > > if (id == "Headphone Mic Headset") { > > create " Headphone Mic Jack " kctl; > > create " Headset Mic Jack " kctl; > > } else { > > create " Headphone Jack " kctl; > > create " Headset Mic Jack " kctl; > > } > > break; > > case SND_JACK_HEADPHONE: > > if (id == "Headset Mic") { > > create " Headphone Jack " kctl; > > // create " Headset Mic Phantom Jack " kctl; > > } else if (id == "Headphone Mic") { > > create " Headphone Mic Jack " kctl; > > } else if (id == "Headset Headphone Mic") { > > create " Headphone Mic Jack " kctl; > > // create " Headset Mic Phantom Jack " kctl; > > } else { > > create " Headphone Jack " kctl; > > } > > break; > > default: > > break; > > } > > ... > > } > > > > > > Jack Type Jack name kctls/switches > name > > SND_JACK_HEADPHONE Headphone Headphone > Jack > > SND_JACK_MICROPHONE Mic Mic Jack > > SND_JACK_HEADSET Headset Headphone > Jack > > Headset Mic > Jack > > SND_JACK_HEADPHONE Headset Mic Headphone > Jack > > Headset Mic > Phantom Jack > > SND_JACK_HEADPHONE Headphone Mic Headphone > Mic Jack > > SND_JACK_HEADPHONE Headset Headphone Mic Headphone > Mic Jack > > Headset Mic > Phantom Jack > > SND_JACK_HEADSET Headphone Mic Headset Headphone > Mic Jack > > Headset Mic > Jack > > > > ~Keyon > > > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic
On 2015-03-27 08:45, Jie, Yang wrote: >> -----Original Message----- >> From: David Henningsson [mailto:david.henningsson@canonical.com] >> Sent: Friday, March 27, 2015 2:50 PM >> To: Jie, Yang; Tanu Kaskinen; Takashi Iwai >> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; >> Girdwood, Liam R; Liam Girdwood >> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack >> input device >> >> >> >> On 2015-03-26 13:43, Jie, Yang wrote: >>> >>> Sorry, Correct a typo(removing line "create "Mic Jack" kctl;" in >>> default case) >> >> It looks like your code can be simplified as: > [Keyon] thanks, I will try simplify them. >> >> switch (type | SND_JACK_HEADSET) { >> case SND_JACK_HEADSET: >> create "Headset Mic Jack" kctl; >> /* fall through */ >> case SND_JACK_MICROPHONE: >> case SND_JACK_HEADPHONE: >> create format("%s Jack", id) kctl; > [Keyon] I don't want to fill "id" to kctl name, because sometimes it can be > string which can't be understood by PA, such as: > > soc/intel/mfld_machine.c: ret_val = snd_soc_card_jack_new(runtime->card, > soc/intel/mfld_machine.c- "Intel(R) MID Audio Jack", SND_JACK_HEADSET | > soc/intel/mfld_machine.c- SND_JACK_BTN_0 | SND_JACK_BTN_1, &mfld_jack, > > soc/omap/ams-delta.c: ret = snd_soc_card_jack_new(card, "hook_switch", SND_JACK_HEADSET, > soc/omap/ams-delta.c- &ams_delta_hook_switch, NULL, 0); Ok, good point. It's a matter of different conventions between ASoC and HDA. In ASoC, naming is wild west. In HDA, naming is (mostly) consistent and we depend on the exact naming for userspace to know exactly how the jack functions. Another example is HDMI/DisplayPort, where the hw often supports several ports and we need to know which jack belongs to which PCM device, and we use the name of the jack to determine that. I would very much prefer that all the ASoC jack names (and mixer control names!) would be fixed up to be consistent to the extent that makes sense, but the ASoC people haven't really agreed to do that, but instead prefer to use a large database of UCM files. :-( And seen from a UCM context, that the ASoC jacks are labelled terribly inconsistent, isn't that big of a deal given that UCM will specify the jack name anyhow. >> } >> >> ...that way, prefixes such as "Front Headphone Jack" are preserved too, > [Keyon] if PA can recognize "Front Headphone Jack" and "Rear Headphone Jack", then I > prefer to add a determine here: > > if (!strcmp(id, "Front Headphone Jack") || !strcmp(id, "Rear Headphone Jack")) > create format("%s Jack", id) kctl; This will not scale, there are too many variants.
> -----Original Message----- > From: David Henningsson [mailto:david.henningsson@canonical.com] > Sent: Friday, March 27, 2015 4:01 PM > To: Jie, Yang; Tanu Kaskinen; Takashi Iwai > Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > Girdwood, Liam R; Liam Girdwood > Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack > input device > > > > On 2015-03-27 08:45, Jie, Yang wrote: > >> -----Original Message----- > >> From: David Henningsson [mailto:david.henningsson@canonical.com] > >> Sent: Friday, March 27, 2015 2:50 PM > >> To: Jie, Yang; Tanu Kaskinen; Takashi Iwai > >> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; > >> Girdwood, Liam R; Liam Girdwood > >> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for > >> every jack input device > >> > >> > >> > >> On 2015-03-26 13:43, Jie, Yang wrote: > >>> > >>> Sorry, Correct a typo(removing line "create "Mic Jack" kctl;" in > >>> default case) > >> > >> It looks like your code can be simplified as: > > [Keyon] thanks, I will try simplify them. > >> > >> switch (type | SND_JACK_HEADSET) { > >> case SND_JACK_HEADSET: > >> create "Headset Mic Jack" kctl; > >> /* fall through */ > >> case SND_JACK_MICROPHONE: > >> case SND_JACK_HEADPHONE: > >> create format("%s Jack", id) kctl; > > [Keyon] I don't want to fill "id" to kctl name, because sometimes it > > can be string which can't be understood by PA, such as: > > > > soc/intel/mfld_machine.c: ret_val = snd_soc_card_jack_new(runtime- > >card, > > soc/intel/mfld_machine.c- "Intel(R) MID Audio Jack", > SND_JACK_HEADSET | > > soc/intel/mfld_machine.c- SND_JACK_BTN_0 | > SND_JACK_BTN_1, &mfld_jack, > > > > soc/omap/ams-delta.c: ret = snd_soc_card_jack_new(card, > "hook_switch", SND_JACK_HEADSET, > > soc/omap/ams-delta.c- &ams_delta_hook_switch, NULL, 0); > > Ok, good point. It's a matter of different conventions between ASoC and > HDA. In ASoC, naming is wild west. In HDA, naming is (mostly) consistent and > we depend on the exact naming for userspace to know exactly how the jack > functions. > > Another example is HDMI/DisplayPort, where the hw often supports several > ports and we need to know which jack belongs to which PCM device, and we > use the name of the jack to determine that. > > I would very much prefer that all the ASoC jack names (and mixer control > names!) would be fixed up to be consistent to the extent that makes sense, > but the ASoC people haven't really agreed to do that, but instead prefer to > use a large database of UCM files. :-( And seen from a UCM context, that the > ASoC jacks are labelled terribly inconsistent, isn't that big of a deal given that > UCM will specify the jack name anyhow. [Keyon] that's the bad status. :( Maybe we can just make HDA jack kctls works and leave not standard name soc jack kctls invisible for PA? > > >> } > >> > >> ...that way, prefixes such as "Front Headphone Jack" are preserved too, > > [Keyon] if PA can recognize "Front Headphone Jack" and "Rear Headphone > Jack", then I > > prefer to add a determine here: > > > > if (!strcmp(id, "Front Headphone Jack") || !strcmp(id, "Rear > Headphone Jack")) > > create format("%s Jack", id) kctl; > > This will not scale, there are too many variants. > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic
> > > > > > > > +} > > > > > + > > > > > +static int snd_jack_new_kctl(struct snd_card *card, struct > > > > > +snd_jack *jack, int type) { > > > > > + struct snd_kcontrol *kctl; > > > > > + struct snd_jack_kctl *jack_kctl; > > > > > + int i, err, index, state = 0 /* use 0 for default state ?? */; > > > > > + > > > > > + INIT_LIST_HEAD(&jack->kctl_list); > > > > > + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { > > > > > + int testbit = 1 << i; > > > > > + if (type & testbit) { > > > > > > > > With this implementation, you'll get multiple boolean kctls for a > > > > headset. I thought we agreed with creating a single boolean for headset? > > > [Keyon] We agreed with creating multiple kctls for combo jack, e.g. > > headset. > > > Furthermore, e.g., imagine that type = SND_JACK_HEADSET | > > > SND_JACK_BTN_0, we will create 3 kctls for the jack, when BTN_0 is > > > pressed, we will report to the 3rd kctl. > > > > Hm, I don't remember that I agreed with multiple kctls... > > > > The multiple kctls have a significant drawback (multiple event calls for a single > > headset) and its behavior is incompatible with the current code (both the > > name change and the behavior change). That is, your patch will very likely > > break the existing applications. > [Keyon] I am not very clear with the existing applications that using these > kctl events(seems Android use input subsystem event? Which we didn't > Change here. If I understand correctly, Pulseaudio uses jack switch controls, > via the name, then we can use different name for headphone and mic here.) > > we will generate 2 event calls(one headphone, one mic) when Headset > plug in/out, applications will receive these 2 events, and they can do anything, > e.g. decide to switch on/off speaker/headphone. Why do you need to generate two event (hp and mic) if soc jack only support headset ? For mobile phone and tablet which only support headset using TRRRS connector and most of them don't support headphone using TRRS connector. When the headset jack kctl is on, this implies headphone and headset mic are used
On 2015-03-27 10:11, Jie, Yang wrote: >> -----Original Message----- >> From: David Henningsson [mailto:david.henningsson@canonical.com] >> Sent: Friday, March 27, 2015 4:01 PM >> To: Jie, Yang; Tanu Kaskinen; Takashi Iwai >> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; >> Girdwood, Liam R; Liam Girdwood >> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack >> input device >> >> >> >> On 2015-03-27 08:45, Jie, Yang wrote: >>>> -----Original Message----- >>>> From: David Henningsson [mailto:david.henningsson@canonical.com] >>>> Sent: Friday, March 27, 2015 2:50 PM >>>> To: Jie, Yang; Tanu Kaskinen; Takashi Iwai >>>> Cc: perex@perex.cz; broonie@kernel.org; alsa-devel@alsa-project.org; >>>> Girdwood, Liam R; Liam Girdwood >>>> Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for >>>> every jack input device >>>> >>>> >>>> >>>> On 2015-03-26 13:43, Jie, Yang wrote: >>>>> >>>>> Sorry, Correct a typo(removing line "create "Mic Jack" kctl;" in >>>>> default case) >>>> >>>> It looks like your code can be simplified as: >>> [Keyon] thanks, I will try simplify them. >>>> >>>> switch (type | SND_JACK_HEADSET) { >>>> case SND_JACK_HEADSET: >>>> create "Headset Mic Jack" kctl; >>>> /* fall through */ >>>> case SND_JACK_MICROPHONE: >>>> case SND_JACK_HEADPHONE: >>>> create format("%s Jack", id) kctl; >>> [Keyon] I don't want to fill "id" to kctl name, because sometimes it >>> can be string which can't be understood by PA, such as: >>> >>> soc/intel/mfld_machine.c: ret_val = snd_soc_card_jack_new(runtime- >>> card, >>> soc/intel/mfld_machine.c- "Intel(R) MID Audio Jack", >> SND_JACK_HEADSET | >>> soc/intel/mfld_machine.c- SND_JACK_BTN_0 | >> SND_JACK_BTN_1, &mfld_jack, >>> >>> soc/omap/ams-delta.c: ret = snd_soc_card_jack_new(card, >> "hook_switch", SND_JACK_HEADSET, >>> soc/omap/ams-delta.c- &ams_delta_hook_switch, NULL, 0); >> >> Ok, good point. It's a matter of different conventions between ASoC and >> HDA. In ASoC, naming is wild west. In HDA, naming is (mostly) consistent and >> we depend on the exact naming for userspace to know exactly how the jack >> functions. >> >> Another example is HDMI/DisplayPort, where the hw often supports several >> ports and we need to know which jack belongs to which PCM device, and we >> use the name of the jack to determine that. >> >> I would very much prefer that all the ASoC jack names (and mixer control >> names!) would be fixed up to be consistent to the extent that makes sense, >> but the ASoC people haven't really agreed to do that, but instead prefer to >> use a large database of UCM files. :-( And seen from a UCM context, that the >> ASoC jacks are labelled terribly inconsistent, isn't that big of a deal given that >> UCM will specify the jack name anyhow. > [Keyon] that's the bad status. :( > Maybe we can just make HDA jack kctls works and leave not standard name > soc jack kctls invisible for PA? If with "invisible for PA" you mean that there are still kctls created, but their naming of the jacks are non-standard, yes, I think this is an okay outcome of your patch. I still think ASoC should change their jack naming to be more consistent, but that can be addressed in some future patch set.
diff --git a/include/sound/jack.h b/include/sound/jack.h index 2182350..ef0f0ed 100644 --- a/include/sound/jack.h +++ b/include/sound/jack.h @@ -73,6 +73,8 @@ enum snd_jack_types { struct snd_jack { struct input_dev *input_dev; + struct list_head kctl_list; + struct snd_card *card; int registered; int type; const char *id; @@ -82,6 +84,12 @@ struct snd_jack { void (*private_free)(struct snd_jack *); }; +struct snd_jack_kctl { + struct snd_kcontrol *kctl; + struct list_head jack_list; /* list of controls belong to the same jack*/ + unsigned int jack_bit_idx; /* the corresponding jack type bit index */ +}; + #ifdef CONFIG_SND_JACK int snd_jack_new(struct snd_card *card, const char *id, int type, diff --git a/sound/core/jack.c b/sound/core/jack.c index 8658578..741924f 100644 --- a/sound/core/jack.c +++ b/sound/core/jack.c @@ -24,6 +24,7 @@ #include <linux/module.h> #include <sound/jack.h> #include <sound/core.h> +#include <sound/control.h> static int jack_switch_types[SND_JACK_SWITCH_TYPES] = { SW_HEADPHONE_INSERT, @@ -54,7 +55,13 @@ static int snd_jack_dev_disconnect(struct snd_device *device) static int snd_jack_dev_free(struct snd_device *device) { struct snd_jack *jack = device->device_data; + struct snd_card *card = device->card; + struct snd_jack_kctl *jack_kctl, *tmp_jack_kctl; + list_for_each_entry_safe(jack_kctl, tmp_jack_kctl, &jack->kctl_list, jack_list) { + list_del(&jack_kctl->jack_list); + snd_ctl_remove(card, jack_kctl->kctl); + } if (jack->private_free) jack->private_free(jack); @@ -100,6 +107,73 @@ static int snd_jack_dev_register(struct snd_device *device) return err; } + +/* get the first unused/available index number for the given kctl name */ +static int get_available_index(struct snd_card *card, const char *name) +{ + struct snd_kcontrol *kctl; + int idx = 0; + int len = strlen(name); + + down_write(&card->controls_rwsem); + next: + list_for_each_entry(kctl, &card->controls, list) { + if (!strncmp(name, kctl->id.name, len) && + !strcmp(" Jack", kctl->id.name + len) && + kctl->id.index == idx) { + idx++; + goto next; + } + } + up_write(&card->controls_rwsem); + return idx; +} + +static void kctl_private_free(struct snd_kcontrol *kctl) +{ + struct snd_jack_kctl *jack_kctl = kctl->private_data; + list_del(&jack_kctl->jack_list); +} + +static int snd_jack_new_kctl(struct snd_card *card, struct snd_jack *jack, int type) +{ + struct snd_kcontrol *kctl; + struct snd_jack_kctl *jack_kctl; + int i, err, index, state = 0 /* use 0 for default state ?? */; + + INIT_LIST_HEAD(&jack->kctl_list); + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { + int testbit = 1 << i; + if (type & testbit) { + index = get_available_index(card,jack->id); + kctl = snd_kctl_jack_new(jack->id, index, card); + if (!kctl) + return -ENOMEM; + + err = snd_ctl_add(card, kctl); + if (err < 0) + return err; + + jack_kctl = kzalloc(sizeof(*jack_kctl), GFP_KERNEL); + + jack_kctl->kctl = kctl; + + kctl->private_data = jack_kctl; + kctl->private_free = kctl_private_free; + + /* use jack_bit_idx for the kctl type bit */ + jack_kctl->jack_bit_idx = i; + + list_add_tail(&jack_kctl->jack_list, &jack->kctl_list); + + /* set initial jack state */ + snd_kctl_jack_report(card, kctl, state & testbit); + } + } + + return 0; +} + /** * snd_jack_new - Create a new jack * @card: the card instance @@ -138,7 +212,7 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, } jack->input_dev->phys = "ALSA"; - + jack->card = card; jack->type = type; for (i = 0; i < SND_JACK_SWITCH_TYPES; i++) @@ -150,10 +224,17 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, if (err < 0) goto fail_input; + /* register jack kcontrols */ + err = snd_jack_new_kctl(card, jack, type); + if (err < 0) + goto fail_kctl; + *jjack = jack; return 0; +fail_kctl: + snd_device_free(card, jack); fail_input: input_free_device(jack->input_dev); kfree(jack->id); @@ -230,6 +311,7 @@ EXPORT_SYMBOL(snd_jack_set_key); */ void snd_jack_report(struct snd_jack *jack, int status) { + struct snd_jack_kctl *jack_kctl; int i; if (!jack) @@ -245,13 +327,26 @@ void snd_jack_report(struct snd_jack *jack, int status) for (i = 0; i < ARRAY_SIZE(jack_switch_types); i++) { int testbit = 1 << i; - if (jack->type & testbit) + if (jack->type & testbit) { input_report_switch(jack->input_dev, jack_switch_types[i], status & testbit); + } } input_sync(jack->input_dev); + + for (i = 0; i < fls(SND_JACK_BTN_0); i++) { + int testbit = 1 << i; + if (jack->type & testbit) { + list_for_each_entry(jack_kctl, &jack->kctl_list, jack_list) { + if (jack_kctl->jack_bit_idx == i) { + snd_kctl_jack_report(jack->card, jack_kctl->kctl, + status & testbit); + } + } + } + } } EXPORT_SYMBOL(snd_jack_report);