-----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