From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device Date: Fri, 20 Mar 2015 10:27:37 +0100 Message-ID: References: <1426841719-14576-1-git-send-email-yang.jie@intel.com> <1426841719-14576-3-git-send-email-yang.jie@intel.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 7E0B22606EB for ; Fri, 20 Mar 2015 10:27:39 +0100 (CET) In-Reply-To: <1426841719-14576-3-git-send-email-yang.jie@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Jie Yang Cc: Liam Girdwood , alsa-devel@alsa-project.org, broonie@kernel.org, liam.r.girdwood@intel.com List-Id: alsa-devel@alsa-project.org 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 > Modified-by: Jie Yang > Signed-off-by: Jie Yang > Reveiwed-by: Mark Brown > --- > 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 > #include > #include > +#include > > 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