Re: [PATCH v2 1/2] ALSA: jack: create jack kcontrols for every jack input device

["Jie, Yang" <yang.jie@intel.com>]

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