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

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

[Arnaud Pouliquen]

To differentiate control with same name only device field can be used.
But some applications like iecset use control index to differentiate
controls linked to several PCM devices or DAIs.
This patch suppress index overwriting to allow to use control index field.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 sound/soc/soc-core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4afa8db..39bc1a9 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2175,7 +2175,6 @@ struct snd_kcontrol *snd_soc_cnew(const struct snd_kcontrol_new *_template,
 	char *name = NULL;
 
 	memcpy(&template, _template, sizeof(template));
-	template.index = 0;
 
 	if (!long_name)
 		long_name = template.name;
-- 
1.9.1

Re: [PATCH] ASoC: core: allow control index different from 0

[Takashi Sakamoto]

Hi,

On Oct 5 2016 00:36, Arnaud Pouliquen wrote:
> To differentiate control with same name only device field can be used.
> But some applications like iecset use control index to differentiate
> controls linked to several PCM devices or DAIs.
> This patch suppress index overwriting to allow to use control index field.

An control element can be identified by two ways from user land:
 - A combination of name/index/interface/device/subdevice
 - A numerical identification number (numid)

In kernel land, some control elements can be managed as one control
element set, mainly due to saving memory usage. The index represents
offset of an element in the element set.

If your patch is applied, callers of snd_soc_cnew() in kernel land can
assign arbitrary index number to an element set, as the offset of the
first element.

According to current implementation, all of first element in element
sets had index number zero; e.g.
$ amixer controls
...
numid=52,iface=MIXER,name='byte-element-26'
numid=53,iface=MIXER,name='byte-element-26',index=1
numid=54,iface=MIXER,name='byte-element-26',index=2
numid=55,iface=MIXER,name='byte-element-26',index=3
numid=56,iface=MIXER,name='byte-element-26',index=4
...

On the other hand, after applying your patch, some element set with
index larger than zero can exist; e.g.
$ amixer controls
...
numid=52,iface=MIXER,name='byte-element-26',index=10
numid=53,iface=MIXER,name='byte-element-26',index=11
numid=54,iface=MIXER,name='byte-element-26',index=12
numid=55,iface=MIXER,name='byte-element-26',index=13
numid=56,iface=MIXER,name='byte-element-26',index=14
(here, I assign 10 as index to this element set)
(no other elements with name of 'byte-element-26')
...

In this case, elements with index 0-9 don't exist. This case is a bit
confusing to application developers, I think.

I can guess that developers for ALSA SoC part would like to avoid the
missing elements with index 0-9, by force to start indexing at zero.

So, if you'd like to start elements with index non-zero, it's better to
describe your case to get advantages from this patch.

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  sound/soc/soc-core.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 4afa8db..39bc1a9 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -2175,7 +2175,6 @@ struct snd_kcontrol *snd_soc_cnew(const struct snd_kcontrol_new *_template,
>  	char *name = NULL;
>  
>  	memcpy(&template, _template, sizeof(template));
> -	template.index = 0;
>  
>  	if (!long_name)
>  		long_name = template.name;

Regards

Takashi Sakamoto

Re: [PATCH] ASoC: core: allow control index different from 0

[Arnaud Pouliquen]

hi

On 10/05/2016 08:59 AM, Takashi Sakamoto wrote:
> Hi,
> 
> On Oct 5 2016 00:36, Arnaud Pouliquen wrote:
>> To differentiate control with same name only device field can be used.
>> But some applications like iecset use control index to differentiate
>> controls linked to several PCM devices or DAIs.
>> This patch suppress index overwriting to allow to use control index field.
> 
> An control element can be identified by two ways from user land:
>  - A combination of name/index/interface/device/subdevice
>  - A numerical identification number (numid)
> 
> In kernel land, some control elements can be managed as one control
> element set, mainly due to saving memory usage. The index represents
> offset of an element in the element set.
> 
> If your patch is applied, callers of snd_soc_cnew() in kernel land can
> assign arbitrary index number to an element set, as the offset of the
> first element.
> 
> According to current implementation, all of first element in element
> sets had index number zero; e.g.
> $ amixer controls
> ...
> numid=52,iface=MIXER,name='byte-element-26'
> numid=53,iface=MIXER,name='byte-element-26',index=1
> numid=54,iface=MIXER,name='byte-element-26',index=2
> numid=55,iface=MIXER,name='byte-element-26',index=3
> numid=56,iface=MIXER,name='byte-element-26',index=4
> ...
> 
> On the other hand, after applying your patch, some element set with
> index larger than zero can exist; e.g.
> $ amixer controls
> ...
> numid=52,iface=MIXER,name='byte-element-26',index=10
> numid=53,iface=MIXER,name='byte-element-26',index=11
> numid=54,iface=MIXER,name='byte-element-26',index=12
> numid=55,iface=MIXER,name='byte-element-26',index=13
> numid=56,iface=MIXER,name='byte-element-26',index=14
> (here, I assign 10 as index to this element set)
> (no other elements with name of 'byte-element-26')
> ...
> 
> In this case, elements with index 0-9 don't exist. This case is a bit
> confusing to application developers, I think.
> 
> I can guess that developers for ALSA SoC part would like to avoid the
> missing elements with index 0-9, by force to start indexing at zero.
> 
> So, if you'd like to start elements with index non-zero, it's better to
> describe your case to get advantages from this patch.

Thanks for the explanation. Here is my use case:
I have a card with 3 output devices:
 hw:0,0 HDMI
	associated controls:
		numid=1,iface=PCM,name='ELD'
		numid=2,iface=PCM,name='IEC958 Playback Default'
		numid=3,iface=PCM,name='Freq.Adjustment'
 hw:0,1 DAC
	associated controls:
		numid=4,iface=PCM,name=' Freq Adjustment'
 hw:0,2 SPDIF
	associated controls:
		numid=5,iface=PCM,name='IEC958 Playback Default'
		numid=6,iface=PCM,name='Freq Adjustment'

My concern is to differentiate the controls associated to the outputs.
Seems that it could be done using device field, but iecset is based on
index (or i missed something?).
Adding an option in iecset to address control by device could solve the
issue... but it is to good way to handle the use case?

Complementary solution would be that control device field corresponds to
PCM device, to allow to address PCM device with args (for instance AESx
params for iec)

This is linked to the management of DAI PCM controls that has already
been discussed in thread associated to this patch:
http://www.spinics.net/lists/alsa-devel/msg47611.html.
I can rework my patch (suppress iec generic control, to simplify it)
but this only treat DAI controls not BE and "dai less" codecs.

Regards
Arnaud

> 
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>  sound/soc/soc-core.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
>> index 4afa8db..39bc1a9 100644
>> --- a/sound/soc/soc-core.c
>> +++ b/sound/soc/soc-core.c
>> @@ -2175,7 +2175,6 @@ struct snd_kcontrol *snd_soc_cnew(const struct snd_kcontrol_new *_template,
>>  	char *name = NULL;
>>  
>>  	memcpy(&template, _template, sizeof(template));
>> -	template.index = 0;
>>  
>>  	if (!long_name)
>>  		long_name = template.name;
> 
> Regards
> 
> Takashi Sakamoto
> 

Re: [PATCH] ASoC: core: allow control index different from 0

[Takashi Sakamoto]

On Oct 5 2016 17:41, Arnaud Pouliquen wrote:
> Thanks for the explanation. Here is my use case:
> I have a card with 3 output devices:
>  hw:0,0 HDMI
> 	associated controls:
> 		numid=1,iface=PCM,name='ELD'
> 		numid=2,iface=PCM,name='IEC958 Playback Default'
> 		numid=3,iface=PCM,name='Freq.Adjustment'
>  hw:0,1 DAC
> 	associated controls:
> 		numid=4,iface=PCM,name=' Freq Adjustment'
>  hw:0,2 SPDIF
> 	associated controls:
> 		numid=5,iface=PCM,name='IEC958 Playback Default'
> 		numid=6,iface=PCM,name='Freq Adjustment'
> 
> My concern is to differentiate the controls associated to the outputs.
> Seems that it could be done using device field, but iecset is based on
> index (or i missed something?).
> Adding an option in iecset to address control by device could solve the
> issue... but it is to good way to handle the use case?
> 
> Complementary solution would be that control device field corresponds to
> PCM device, to allow to address PCM device with args (for instance AESx
> params for iec)
> 
> This is linked to the management of DAI PCM controls that has already
> been discussed in thread associated to this patch:
> http://www.spinics.net/lists/alsa-devel/msg47611.html.
> I can rework my patch (suppress iec generic control, to simplify it)
> but this only treat DAI controls not BE and "dai less" codecs.

Hm. In configuration space (namespace) of alsa-lib, the 'hw:0,1' PCM
node is a short representation of 'pcm.hw:0,1,0'; each member expresses:
 - pcm: interface
 - hw:  plugin name
 - 0:   card number
 - 1:   device number
 - 0:   subdevice number

Next, when identifying a control element, 'struct snd_ctl_elem_id' is
used in ALSA control core. Layout of the structure is in <sound/asound.h>:

struct snd_ctl_elem_id {
  unsigned int numid;
  snd_ctl_elem_iface_t iface;
  unsigned int device;
  unsigned int subdevice;
  unsigned char name[44];
  unsigned int index;
};

Under the above design, when using a proper combination of
iface/card/device/subdevice, we can represent the relationship between a
control element and a PCM node.

But in a case of control elements for IEC 60958 type, we need to read
this patch, commit ea9b43addc4d ("ALSA: hda - Fix broken workaround for
HDMI/SPDIF conflicts").
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b43add

In this commit for former Intel HDA driver, two issues were addressed:
 - multiple codecs gives functionalities of digital interface such as HDMI.
 - bug of mixer interface in alsa-lib

Due to the issues, 'index' field is used, instead of the design I described.

Although, the requirement for HDA codecs (verb parser) and your
requirement is different. I think it better for you to follow the above
design, then fix the bug of mixer interface in alsa-lib. At least, via
the other ctl-related APIs such as ctl/hctl, the control element can be
selected and processed in user land.

> This is linked to the management of DAI PCM controls that has already
> been discussed in thread associated to this patch:
> http://www.spinics.net/lists/alsa-devel/msg47611.html.
> I can rework my patch (suppress iec generic control, to simplify it)
> but this only treat DAI controls not BE and "dai less" codecs.

I can't follow all of messages in the related threads, but:

[alsa-devel] [PATCH v3 1/4] ALSA: pcm: add IEC958 channel status control
helper
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105152.html

Takashi Iwai wrote:
> Hmm, this doesn't always work.  It will create the substream_count
> ctls starting from the pcm dev# as index.  What if there are 2 PCM
> devices where both contain 4 substreams?

struct snd_ctl_elem_id.subdevice can represent the index of PCM
substream because current PCM core expresses the substreams as the
subdevices.
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/pcm.c?h=sound-4.9-rc1#n144

[alsa-devel] [PATCH v3 1/4] ALSA: pcm: add IEC958 channel status control
helper
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105158.html

Takashi Iwai wrote:
> This pretty much depends on the hardware design.  If each substream is
> really individual, you'd need to give the control for each substream.

I'll assist his advice for your issue.

In next time, please put URIs to your comment so that the other
developers can follow related issues.


Regards

Takshi Sakamoto

Re: [PATCH] ASoC: core: allow control index different from 0

[Arnaud Pouliquen]

On 10/06/2016 04:01 PM, Takashi Sakamoto wrote:
> On Oct 5 2016 17:41, Arnaud Pouliquen wrote:
>> Thanks for the explanation. Here is my use case:
>> I have a card with 3 output devices:
>>  hw:0,0 HDMI
>> 	associated controls:
>> 		numid=1,iface=PCM,name='ELD'
>> 		numid=2,iface=PCM,name='IEC958 Playback Default'
>> 		numid=3,iface=PCM,name='Freq.Adjustment'
>>  hw:0,1 DAC
>> 	associated controls:
>> 		numid=4,iface=PCM,name=' Freq Adjustment'
>>  hw:0,2 SPDIF
>> 	associated controls:
>> 		numid=5,iface=PCM,name='IEC958 Playback Default'
>> 		numid=6,iface=PCM,name='Freq Adjustment'
>>
>> My concern is to differentiate the controls associated to the outputs.
>> Seems that it could be done using device field, but iecset is based on
>> index (or i missed something?).
>> Adding an option in iecset to address control by device could solve the
>> issue... but it is to good way to handle the use case?
>>
>> Complementary solution would be that control device field corresponds to
>> PCM device, to allow to address PCM device with args (for instance AESx
>> params for iec)
>>
>> This is linked to the management of DAI PCM controls that has already
>> been discussed in thread associated to this patch:
>> http://www.spinics.net/lists/alsa-devel/msg47611.html.
>> I can rework my patch (suppress iec generic control, to simplify it)
>> but this only treat DAI controls not BE and "dai less" codecs.
> 
> Hm. In configuration space (namespace) of alsa-lib, the 'hw:0,1' PCM
> node is a short representation of 'pcm.hw:0,1,0'; each member expresses:
>  - pcm: interface
>  - hw:  plugin name
>  - 0:   card number
>  - 1:   device number
>  - 0:   subdevice number
> 
> Next, when identifying a control element, 'struct snd_ctl_elem_id' is
> used in ALSA control core. Layout of the structure is in <sound/asound.h>:
> 
> struct snd_ctl_elem_id {
>   unsigned int numid;
>   snd_ctl_elem_iface_t iface;
>   unsigned int device;
>   unsigned int subdevice;
>   unsigned char name[44];
>   unsigned int index;
> };
> 
> Under the above design, when using a proper combination of
> iface/card/device/subdevice, we can represent the relationship between a
> control element and a PCM node.
> 
> But in a case of control elements for IEC 60958 type, we need to read
> this patch, commit ea9b43addc4d ("ALSA: hda - Fix broken workaround for
> HDMI/SPDIF conflicts").
> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b43add
> 
> In this commit for former Intel HDA driver, two issues were addressed:
>  - multiple codecs gives functionalities of digital interface such as HDMI.
>  - bug of mixer interface in alsa-lib
not aware about alsa-lib issue... Have you an URI that details the issue?
> 
> Due to the issues, 'index' field is used, instead of the design I described.
> 
> Although, the requirement for HDA codecs (verb parser) and your
> requirement is different. I think it better for you to follow the above
> design, then fix the bug of mixer interface in alsa-lib. At least, via
> the other ctl-related APIs such as ctl/hctl, the control element can be
> selected and processed in user land.

First, sorry i think my mail is not Cristal clear

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)

STI DAI driver uses DAI control interface. Index is forced to 0 but as
device is also set, no error on ctrl registration.
Handle index based on HDA codec implementation means I need to bypass
DAI ctrl helpers... Should be nice to have a more generic way,
as several implementations can face the same issue.

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.

> 
>> This is linked to the management of DAI PCM controls that has already
>> been discussed in thread associated to this patch:
>> http://www.spinics.net/lists/alsa-devel/msg47611.html.
>> I can rework my patch (suppress iec generic control, to simplify it)
>> but this only treat DAI controls not BE and "dai less" codecs.
> 
> I can't follow all of messages in the related threads, but:
> 
> [alsa-devel] [PATCH v3 1/4] ALSA: pcm: add IEC958 channel status control
> helper
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105152.html
> 
> Takashi Iwai wrote:
>> Hmm, this doesn't always work.  It will create the substream_count
>> ctls starting from the pcm dev# as index.  What if there are 2 PCM
>> devices where both contain 4 substreams?
> 
> struct snd_ctl_elem_id.subdevice can represent the index of PCM
> substream because current PCM core expresses the substreams as the
> subdevices.
> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/pcm.c?h=sound-4.9-rc1#n144
> 
> [alsa-devel] [PATCH v3 1/4] ALSA: pcm: add IEC958 channel status control
> helper
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105158.html
> 
> Takashi Iwai wrote:
>> This pretty much depends on the hardware design.  If each substream is
>> really individual, you'd need to give the control for each substream.
> 
> I'll assist his advice for your issue.
For IEC generic ctrl, hope that this point has be integrated in V4:
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105505.html
index is no more overwritten by snd_pcm_create_iec958_ctl and subdevice
is added in function params

Thanks and Regards
Arnaud

Re: [PATCH] ASoC: core: allow control index different from 0

[Takashi Sakamoto]

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

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.

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.

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.

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.

>From 30c6abb20ca548add8cddb6e056831efde365c3e Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@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);
+		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;
 		}
 	}
-- 
2.7.4


Regards

Takashi Sakamoto

Re: [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@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;
>  		}
>  	}
> 

Re: [PATCH] ASoC: core: allow control index different from 0

[Takashi Sakamoto]

On Oct 10 2016 18:26, Arnaud Pouliquen wrote:
> 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.

I should write that '0 is assigned to the index field later than
calculated index number is assigned'.

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

Please explain about the reason or show references to the reason that
usage of iface/card/device/subdevice combination is not convenient to
your issue. To me, this solves the issue (second issue in your previous
message) because at least, from user land, applications can identify a
control element corresponding to each PCM subdevice.

In other words, in your taste, why this is not a generic way for
something you face (this is not cleared yet to me).

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

Over-generalizing, you did.

In my insistent, the usage of index field for this purpose is not just
better. I've never said that no real solutions we have.

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

In my understanding, this idea is not good for simple-card
implementation in ALSA SoC part, too. The idea needs more complicated
driver to have several instances of sound card.


Regards

Takashi Sakamoto

Re: [PATCH] ASoC: core: allow control index different from 0

[Arnaud Pouliquen]

>>> 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.
> 
> Please explain about the reason or show references to the reason that
> usage of iface/card/device/subdevice combination is not convenient to
> your issue. To me, this solves the issue (second issue in your previous
> message) because at least, from user land, applications can identify a
> control element corresponding to each PCM subdevice.
> 
> In other words, in your taste, why this is not a generic way for
> something you face (this is not cleared yet to me).

iface/card/device/subdevice can be used when control is linked to a PCM
device.
Main difference for ASoC drivers is that controls associated to CPU_DAIs
and CODEC_DAIs are not linked to PCM device.
That's why i had mentioned in a previous mail a patch that proposed to
link DAI to PCM (it just a first version for concept).
[PATCH v4 1/6] ASoC: core: add snd_soc_add_dai_pcm_controls helper:
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105501.html

This could link control to PCM device for CPU and CODEC DAI in ASoC
implementation. But issue still there for DPCM implementation where
back-end is not linked to the PCM device, or for CODECS without DAI
interface, as mentioned in feedbacks:
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105570.html
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105660.html

Also we can imagine 2 instances of the same codec (for instance HDMI1
and HDMI2 output for TV and AVR). They can export twice the same
controls. In this case issue can also exists for none PCM controls (such
as MIXER controls).

The last point is the iecset application. It uses card/index and not
iface/card/device/subdevice. But this also makes sense if iec control
is not linked to PCM device.

>> 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.
> 
> In my understanding, this idea is not good for simple-card
> implementation in ALSA SoC part, too. The idea needs more complicated
> driver to have several instances of sound card.
Simple card can be instantiate in DT to create several sound cards.
But look like more a backup that a main solution, from my point of view.

Regards
Arnaud

Re: [PATCH] ASoC: core: allow control index different from 0

[Charles Keepax]

On Mon, Oct 10, 2016 at 11:26:00AM +0200, Arnaud Pouliquen wrote:
> 
> 
> 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:
> > 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.

Apologies if I am missing the mark here, I haven't been following
this thread in great detail. But if your main concern here is
multiple instances of the same CODEC creating the same controls
the normal way to handle that in ASoC is using a
snd_soc_codec_conf struct which lets you add a prefix to all the
controls from a specific instance of a CODEC. See
sound/soc/samsung/bells.c for an example using it.

Thanks,
Charles

Re: [PATCH] ASoC: core: allow control index different from 0

[Arnaud Pouliquen]

On 10/11/2016 10:30 AM, Charles Keepax wrote:
> On Mon, Oct 10, 2016 at 11:26:00AM +0200, Arnaud Pouliquen wrote:
>>
>>
>> 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:
>>> 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.
> 
> Apologies if I am missing the mark here, I haven't been following
> this thread in great detail. But if your main concern here is
> multiple instances of the same CODEC creating the same controls
> the normal way to handle that in ASoC is using a
> snd_soc_codec_conf struct which lets you add a prefix to all the
> controls from a specific instance of a CODEC. See
> sound/soc/samsung/bells.c for an example using it.

Codec multi instance is one of the use cases. CPU_DAI multi instance is
another one (and my main concern).
But use of a prefix_name is a good point that i have not prospected yet,
thanks.
Today it is not implemented neither for CPU_DAIs nor for simple card.
But my main drawback of the prefix name, is that this breaks the naming
of generic controls like IEC. So i can't use it.

Regards
Arnaud