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

[Takashi Iwai <tiwai@suse.de>]

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