[alsa-devel] [PATCH] ASoC: core: allow control index different from 0

[Arnaud Pouliquen]

On 10/09/2016 05:05 AM, Takashi Sakamoto wrote:
> Sorry to be late. I've review your driver in for-next branch of
> maintainer's tree.
> 
> On Oct 8 2016 01:41, Arnaud Pouliquen wrote:
>> Let clarify the my devs:
>>
>> 1) My original implementation is here :
>> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/soc/sti/sti_uniperif.c?h=sound-4.9-rc1#n232
>> this does not work due to index that is overwritten
>>  Notice that my issue is not limited to iec958 control, but controls in
>> generals
>>
>> 2) In parallel, I proposed generic code to implement:
>>  IEC control + possibility to attach DAI ctrl to PCM
>> device:http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105502.html
>>
>> Right now, I propose to focus on point 1) that integrates constraints
>> for DAIs and forget the patch-set that includes IEC generic control (if
>> you interested in this code, i can rework it in a second step)
> 
> The center of discussion now becomes transformed.
> 
> Anyway, at least, this patch (ASoC: core: allow control index different
> from 0) is exaggerated to your aim. The change influences to all of
> control element sets added by implementations in ALSA SoC part. (We have
> no guarantee that all drivers already set 0 to the field of template
> before calling with the template.)
Full in line with you. this patch is already in my trash.
> 
> And please write enough information about your aim of this patch, even
> if it's a part of your work for something large. It helps reviewers.
> Additionally, if you discuss the former patchset, please rebase them to
> current upstream, then post them again with enough comments, then start
> discussion.
Sorry if my approach to treat the topic is not good. My Aim is to find a
way to support multi instances of an alsa control.
As I suppose that i'm not alone to have this kind of problematic. I am
trying to look for a generic way to do it.

> 
> On Oct 8 2016 01:41, Arnaud Pouliquen wrote:
>> Perhaps this could be handled in a generic way in control.c?
>> Today if control is identical, snd_ctl_add returns an error.
>> Instead an auto-incrementation mechanism could be implemented to
>> increase index.
> 
> Just for ALSA SoC part, something like below (not tested). This never
> work well because 0 is still assigned to the index field later.
Index is forced to 0 in snd_soc_cnew. Not updated after, please find
comment in line in your patch.

> 
> But I still oppose this idea. This idea allows drivers to add control
> element sets of different types (int/int64/bytes etc...) with the same
> name and different index number. This certainly brings confusions to
> applications.
Today is already possible using the device or subdevice field instead of
index.
> 
> In a framework of ALSA SoC part, several drivers are associated to one
> sound card instance can add their own control element sets. There's no
> mechanism to prevent my concern. This idea is bad.
Please tell me if i misunderstand. So for you, there is no real solution
to do it in a generic way. Drivers has to implement it, if they want to
support.

Another solution is to declare a card per instance of control.
This should work for my use case and for use cases with several codecs
that declare same controls. But this should not work for DPCM...
The drawback for my use case, would be that i need to declare one card
per PCM device.

Regards
Arnaud
> 
>>From 30c6abb20ca548add8cddb6e056831efde365c3e Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto <o-takashi at sakamocchi.jp>
> Date: Sun, 9 Oct 2016 11:20:08 +0900
> Subject: [PATCH] hoge
> 
> ---
>  sound/soc/soc-core.c | 42 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index c0bbcd9..573ca73 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -2202,15 +2202,49 @@ static int snd_soc_add_controls(struct snd_card
> *card, struct device *dev,
>  	const struct snd_kcontrol_new *controls, int num_controls,
>  	const char *prefix, void *data)
>  {
> +	struct snd_kcontrol_new template;
> +	struct snd_ctl_elem_id id;
> +	struct snd_kcontrol *elem_set;
>  	int err, i;
> 
>  	for (i = 0; i < num_controls; i++) {
> -		const struct snd_kcontrol_new *control = &controls[i];
> -		err = snd_ctl_add(card, snd_soc_cnew(control, data,
> -						     control->name, prefix));
> +		template = controls[i];
> +
> +		id.iface = template.iface;
> +		id.device = template.device;
> +		id.subdevice = template.subdevice;
> +		memcpy(id.name, template.name, sizeof(id.name));
> +
> +		/*
> +		 * Seek duplicated element sets already registered in this
> +		 * sound card to use continuous number for index field.
> +		 */
> +		while (1) {
> +			id.index = template.index;
> +
> +			elem_set = snd_ctl_find_id(card, &id);
> +			if (elem_set == NULL)
> +				break;
> +
> +			if (elem_set->id.index > UINT_MAX - elem_set->count) {
> +				dev_err(dev, "ASoC: Failed to keep %s: %d\n",
> +					template.name, err);
> +				return -ENOSPC;
> +			}
> +			template.index = elem_set->count + elem_set->id.index;
> +		}
> +
> +		elem_set = snd_soc_cnew(&template, data, template.name, prefix);
index field is forced to 0 in snd_soc_cnew so eleme_set->id.index should
be overwritten to be taken into account.
> +		if (elem_set == NULL) {
> +			dev_err(dev, "ASoC: Failed to allocate %s: %d\n",
> +				template.name, err);
> +			return -ENOMEM;
> +		}
> +
> +		err = snd_ctl_add(card, elem_set);
>  		if (err < 0) {
>  			dev_err(dev, "ASoC: Failed to add %s: %d\n",
> -				control->name, err);
> +				template.name, err);
>  			return err;
>  		}
>  	}
>