[alsa-devel] [RFC 2/4] Alsa: control: increment index field for duplicated control.

[Arnaud Pouliquen]

Hello Takashi,

On 11/09/2016 05:04 AM, Takashi Sakamoto wrote:
> Hi,
> 
> On Nov 8 2016 17:11, Arnaud Pouliquen wrote:
>> Instead of returning an error when a control is already present, the
>> index field is incremented and a warning message is printed.
>> This allows drivers to instanciate same control without
>> device and sub device number management ( MIXER type controls).
>>
>> Change-Id: Ifcc60dca9d1cf4c3a424bb9653296678aa7953cb
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen at st.com>
>> ---
>>  sound/core/control.c | 28 ++++++++++++++++++----------
>>  1 file changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/sound/core/control.c b/sound/core/control.c
>> index fb096cb..014e3f4 100644
>> --- a/sound/core/control.c
>> +++ b/sound/core/control.c
>> @@ -365,6 +365,7 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
>>  	struct snd_ctl_elem_id id;
>>  	unsigned int idx;
>>  	unsigned int count;
>> +	struct snd_kcontrol *elem_set;
>>  	int err = -EINVAL;
>>
>>  	if (! kcontrol)
>> @@ -376,17 +377,24 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
>>  		goto error;
>>
>>  	down_write(&card->controls_rwsem);
>> -	if (snd_ctl_find_id(card, &id)) {
>> -		up_write(&card->controls_rwsem);
>> -		dev_err(card->dev, "control %i:%i:%i:%s:%i is already present\n",
>> -					id.iface,
>> -					id.device,
>> -					id.subdevice,
>> -					id.name,
>> -					id.index);
>> -		err = -EBUSY;
>> -		goto error;
>> +	while (elem_set = snd_ctl_find_id(card, &id)) {
>> +		id.index++;
>> +		if (id.index > UINT_MAX - kcontrol->count) {
>> +			up_write(&card->controls_rwsem);
>> +			dev_err(card->dev, "no more free index for control %i:%i:%i:%s\n",
>> +				id.iface, id.device, id.subdevice, id.name);
>> +			err = -ENOSPC;
>> +			goto error;
>> +		}
>> +	}
>> +	if (kcontrol->id.index != id.index) {
>> +		dev_warn(card->dev, "control %i:%i:%i:%s:%i is already present\n",
>> +			 id.iface, id.device, id.subdevice, id.name, id.index);
>> +		dev_warn(card->dev, "control index updated from %i to %i\n",
>> +			 kcontrol->id.index, id.index);
>> +		kcontrol->id.index = id.index;
>>  	}
>> +
>>  	if (snd_ctl_find_hole(card, kcontrol->count) < 0) {
>>  		up_write(&card->controls_rwsem);
>>  		err = -ENOMEM;
> 
> As Iwai-san explained below, this integration is a bit 
> over-specification in control core, because your issue is specific in 
> ALSA SoC part.
> 
> 
> On Nov 8 2016 23:30, Takashi Iwai wrote:
>  > Right, this behavior must be optional.  For the normal drivers, the
>  > duplicated controls *are* errors, and we catch it instead.  For
>  > drivers that are aware of confliction and want the automatic
>  > workaround (e.g. HD-audio driver does it already), this kind of code
>  > would be useful to be in the common place.
> (http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/114430.html)
> 
> For drivers outside of ALSA SoC part, developers can and should program 
> control element sets with unique identification information, because 
> design of sound card is fixed at development time. Therefore, these 
> codes should be in ALSA SoC core, as I introduced once.
> 
> When we have the same requirement for drivers outside of ALSA SoC part, 
> then we're going to move these codes from ALSA SoC core to ALSA control 
> core, I think.

I have generalized this in control.c, precisely because, as you pointed,
HDA-audio as same requirement. In other words, this could also seem as
an helper for the control indexation.
Now no problem to limit it to ASoC as you propose (need to implement it
in snd_soc_cnew to take into account prefix).

Now regarding the discussion and the set of patches in my RFC:
In my environment, this patch is mandatory to be able to address 'IEC958
Playback Default' control using iecset application.
if patch in iecset is accepted, it is no more mandatory.
(http://www.spinics.net/lists/alsa-devel/msg56480.html)

So the right way to do it, is to propose the iecset patch in a first step.
Then if it is rejected or if you estimate that patch makes sense anyway
for other "generic" controls i will rework it and propose it.

Thanks and Regards
Arnaud