[alsa-devel] [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device
Jie, Yang
yang.jie at intel.com
Fri Mar 20 13:22:10 CET 2015
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Friday, March 20, 2015 5:28 PM
> To: Jie, Yang
> Cc: perex at perex.cz; broonie at kernel.org; alsa-devel at 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 at linux.intel.com>
> > Modified-by: Jie Yang <yang.jie at intel.com>
> > Signed-off-by: Jie Yang <yang.jie at intel.com>
> > Reveiwed-by: Mark Brown <broonie at 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
More information about the Alsa-devel
mailing list