[RFC 0/4] ALSA controls management using index/device/sub-devices fields

[RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Arnaud Pouliquen]

I tried to summarize my observation based on discussions concerning the
instantiation of the ALSA controls, initiate in following thread:
[PATCH] ASoC: core: allow control index different from 0 
    https://www.spinics.net/lists/ALSA-devel/msg55415.html

My test environment: sti platform, 3 playback devices with associated instances of controls

card 0: STIB2120 [STI-B2120], device 0: Uni Player #0 (HDMI)-i2s-hifi 
	numid=1,iface=PCM,name='ELD'
	numid=3,iface=PCM,name='IEC958 Playback Default'
	numid=2,iface=PCM,name='PCM Playback Oversampling Freq.
				Adjustment'
card 0: STIB2120 [STI-B2120], device 1: Uni Player #2 (DAC)-sas-dai-dac 
	numid=4,iface=PCM,name='PCM Playback Oversampling Freq.
				Adjustment',device=2
card 0: STIB2120 [STI-B2120], device 2: Uni Player #3 (SPDIF)-sas-dai-
spdif-out
	numid=6,iface=PCM,name='IEC958 Playback Default',device=2
	numid=5,iface=PCM,name='PCM Playback Oversampling Freq.
				Adjustment',device=3

1) From user land point of view 

1-1) How to Instantiate generic ALSA control

The  point is the handling of multi instance of generic ALSA controls.
In this case "prefix" can not be used. Controls have to be identified
by a combination of device/sub-device/index

Examples of controls that seem generics:
iface=PCM,name='Capture Channel Map'
iface=PCM,name='Playback Channel Map'
iface=PCM,name='ELD'

iface=MIXER,name='Master Playback Switch'
iface=MIXER,name='Master Playback Volume'
iface=MIXER,name='Capture Switch'
iface=MIXER,name='Capture Volume'

iface=MIXER/PCM,name='IEC958 Playback Con Mask'
iface=MIXER/PCM,name='IEC958 Playback Pro Mask'
iface=MIXER/PCM,name='IEC958 Playback Default'
iface=MIXER/PCM,name='IEC958 Playback Switch'

1-2) Different ways of using control instantiation
 Here is a short benchmark on applications using ALSA controls:
- iecset and "amixer scontrols" base control instantiation on "index".
- GStreamer and pulseaudio seems based on "device" (through the ALSA
  card conf files).
- "amixer controls" support both.

 I supposse that this is the reason why "device" and "index fields
  are  both fixed in HDA-intel.cong aligned with driver.  

1-3) Questions:
   - If i consider description provided by Takashi Sakamoto in thread:
    https://www.spinics.net/lists/ALSA-devel/msg55451.html

    Should we always apply this rules?
      - MIXER control type: as it is not linked to PCM device but card,
	"index" is used to instantiate control.
      - PCM control type: as it is linked to PCM device,
        "device/subdevice" is used to instantiate control.
    Or could we consider that a control can be instantiated using
      device/subdevice fields  and/or index fields?

   - Concerning IEC controls, should we consider them as PCM or MIXER
     type controls? In current implementations both are used...

2) From driver point of view

2-1) None SOC drivers:
Today solution seems to base indexation on both "index" and "device"
number. Device indexation seems not linked to PCM device number
Example in  ea9b43addc4d ("ALSA: hda - Fix broken workaround for HDMI/SPDIF conflicts").
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b43add

=> The drawback is that each driver has to implement it. Could be nice
   to have in generic code....

2-2) SOC drivers:
  -  it is not possible to use "index" using helper functions as
    snd_soc_cnew forces index to 0.
    Solution could be to implement in soc_core as done in HDA driver. 
   => duplication of the code in ASoC and None ASoC drivers.
  - Relation chip between control and PCM device. if rules mentioned
    in 1-3 should be respected, need to link control to PCM device.
    This point seems quite tricky... At least DAI controls can be
    simply linked to PCM device during DAI-link probing. 	

3) Patches proposed:
Based on these observations, here are some patches that i tested in my
environment. There are complementary,  and could answer to some 
limitations mentioned above.
  
3-1) Alsa-utils patch

- iecset: allow to select control with device and sub-device numbers
  This patch allows to access to 2 iec controls differentiated by
  device/sub-devices numbers
=> For me, this patch is mandatory to be able to address the ASoC IEC
   controls, in case of no fix is implemented to allows index field
   update in ASoC.

3-2) Alsa driver patches
  - ASoC: core: allow PCM control binding to PCM device
  	Add relationship between DAIs PCM controls and PCM device.

  - ALSA: control: increment index field for duplicated control.
   	Generic implementation of the patch proposed in HDA driver
        (http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b43add)

  - ASoC: sti: use bind_pcm_ctl
  	implementation of bind_pcm_ctl for sti driver.
        
 Best Regards,
 Arnaud
-- 
1.9.1

[RFC 1/4] ASoC: core: allow PCM control binding to PCM device

[Arnaud Pouliquen]

Add relationchip between DAI PCM controls and PCM device. 
For some generic controls (for instance iec controls), it is useful 
to link control to the PCM device. Especially for application that uses a
instance of the control based on the PCM device number.

Implementation is based on bind_pcm_ctl field in dai_driver struct
to enable the link. When set to true DAI PCM controls device field 
is forced to the PCM device number.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 include/sound/soc-dai.h |   3 ++
 sound/soc/soc-core.c    | 118 ++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 117 insertions(+), 4 deletions(-)

diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 964b7de..52e89ee 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -233,6 +233,8 @@ struct snd_soc_dai_driver {
 	int (*compress_new)(struct snd_soc_pcm_runtime *rtd, int num);
 	/* DAI is also used for the control bus */
 	bool bus_control;
+	/* Bind IFACE_PCM controls to the PCM device */
+	bool bind_pcm_ctl;
 
 	/* ops */
 	const struct snd_soc_dai_ops *ops;
@@ -292,6 +294,7 @@ struct snd_soc_dai {
 	unsigned int rx_mask;
 
 	struct list_head list;
+	struct list_head list_pcm_ctl;
 };
 
 static inline void *snd_soc_dai_get_dma_data(const struct snd_soc_dai *dai,
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4afa8db..148b90f 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -58,6 +58,11 @@ static LIST_HEAD(platform_list);
 static LIST_HEAD(codec_list);
 static LIST_HEAD(component_list);
 
+struct snd_soc_dai_pcm_ctls {
+	struct snd_kcontrol *controls;
+	struct list_head list;
+};
+
 /*
  * This is a timeout to do a DAPM powerdown after a stream is closed().
  * It can be used to eliminate pops between different playback streams, e.g.
@@ -1362,6 +1367,32 @@ static void soc_set_name_prefix(struct snd_soc_card *card,
 	}
 }
 
+static int soc_link_dai_pcm_controls(struct snd_soc_card *card,
+	struct snd_soc_dai *dai, struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_dai_pcm_ctls *ctls, *_ctls;
+	struct snd_kcontrol *kctl;
+	int i, err;
+
+	list_for_each_entry_safe(ctls, _ctls,  &dai->list_pcm_ctl, list) {
+		kctl = ctls->controls;
+		kctl->id.device = rtd->pcm->device;
+
+		err = snd_ctl_add(card->snd_card, kctl);
+		if (err < 0) {
+			dev_err(card->dev,
+				"ASoC: Can't link %s: %s control(s) to %s :%d\n",
+				dai->name, kctl->id.name,
+				rtd->dai_link->stream_name, err);
+			return err;
+		}
+		list_del(&ctls->list);
+		kfree(ctls);
+	}
+
+	return 0;
+}
+
 static int soc_probe_component(struct snd_soc_card *card,
 	struct snd_soc_component *component)
 {
@@ -1598,7 +1629,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
 		struct snd_soc_pcm_runtime *rtd, int order)
 {
 	struct snd_soc_dai_link *dai_link = rtd->dai_link;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	struct snd_soc_dai *dai, *cpu_dai = rtd->cpu_dai;
 	int i, ret;
 
 	dev_dbg(card->dev, "ASoC: probe %s dai link %d late %d\n",
@@ -1663,6 +1694,24 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
 				       dai_link->stream_name, ret);
 				return ret;
 			}
+
+			/* link  CPU DAI pcm controls to pcm device */
+			if (!list_empty(&cpu_dai->list_pcm_ctl)) {
+				ret = soc_link_dai_pcm_controls(card, cpu_dai,
+								rtd);
+				if (ret < 0)
+					return ret;
+			}
+
+			/* link CODEC DAI pcm control to pcm device */
+			for (i = 0; i < rtd->num_codecs; i++) {
+				dai = rtd->codec_dais[i];
+				if (!list_empty(&dai->list_pcm_ctl))
+					ret = soc_link_dai_pcm_controls(card,
+								      dai, rtd);
+				if (ret < 0)
+					return ret;
+			}
 		} else {
 			INIT_DELAYED_WORK(&rtd->delayed_work,
 						codec2codec_close_delayed_work);
@@ -2218,6 +2267,49 @@ static int snd_soc_add_controls(struct snd_card *card, struct device *dev,
 	return 0;
 }
 
+static int snd_soc_add_dai_pcm_controls(struct snd_soc_card *soc_card,
+	struct snd_soc_dai *dai, const struct snd_kcontrol_new *controls,
+	int num_controls, const char *prefix, void *data)
+{
+	struct snd_soc_pcm_runtime *rtd;
+	struct snd_soc_dai_pcm_ctls *ctls_list;
+	struct snd_kcontrol *kctl;
+	int i, dai_found = 0;
+
+	for (i = 0; i < num_controls; i++) {
+		const struct snd_kcontrol_new *control = &controls[i];
+
+		if (control->iface != SNDRV_CTL_ELEM_IFACE_PCM) {
+			dev_err(dai->dev, "%s: not a pcm device control !!!\n",
+				control->name);
+			return -EINVAL;
+		}
+		kctl = snd_soc_cnew(control, data, control->name, prefix);
+		if (IS_ERR(kctl))
+			return PTR_ERR(kctl);
+
+		ctls_list = kzalloc(sizeof(*ctls_list), GFP_KERNEL);
+		ctls_list->controls = kctl;
+		list_add(&ctls_list->list, &dai->list_pcm_ctl);
+	}
+
+	if (dai->probed) {
+		/* pcm device exists. Control can be linked to it */
+		list_for_each_entry(rtd, &soc_card->rtd_list, list) {
+			if (dai == rtd->cpu_dai) {
+				dai_found = 1;
+				break;
+			}
+		}
+		if (!dai_found)
+			return -EINVAL;
+
+		soc_link_dai_pcm_controls(soc_card, dai, rtd);
+	}
+
+	return 0;
+}
+
 struct snd_kcontrol *snd_soc_card_get_kcontrol(struct snd_soc_card *soc_card,
 					       const char *name)
 {
@@ -2323,10 +2415,26 @@ EXPORT_SYMBOL_GPL(snd_soc_add_card_controls);
 int snd_soc_add_dai_controls(struct snd_soc_dai *dai,
 	const struct snd_kcontrol_new *controls, int num_controls)
 {
-	struct snd_card *card = dai->component->card->snd_card;
+	struct snd_soc_card *soc_card = dai->component->card;
+	struct snd_card *card = soc_card->snd_card;
+	int ret, i;
 
-	return snd_soc_add_controls(card, dai->dev, controls, num_controls,
-			NULL, dai);
+	if (!dai->driver || !dai->driver->bind_pcm_ctl)
+		return snd_soc_add_controls(card, dai->dev, controls,
+			num_controls, NULL, dai);
+
+	for (i = 0; i < num_controls; i++) {
+		if (controls[i].iface == SNDRV_CTL_ELEM_IFACE_PCM)
+			ret = snd_soc_add_dai_pcm_controls(soc_card, dai,
+				&controls[i], 1, NULL, dai);
+		else
+			ret = snd_soc_add_controls(card, dai->dev, &controls[i],
+				 1, NULL, dai);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_add_dai_controls);
 
@@ -2806,6 +2914,8 @@ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component,
 	if (!dai->driver->ops)
 		dai->driver->ops = &null_dai_ops;
 
+	INIT_LIST_HEAD(&dai->list_pcm_ctl);
+
 	list_add(&dai->list, &component->dai_list);
 	component->num_dai++;
 
-- 
1.9.1

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

[Arnaud Pouliquen]

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@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;
-- 
1.9.1

[RFC 3/4] ASoC: sti: use bind_pcm_ctl

[Arnaud Pouliquen]

Bind PCM control to PCM device created during DAI linking.

Change-Id: Idbc6cb8421fd3b8fd6e2fb93c84578410ea7b227
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 sound/soc/sti/sti_uniperif.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sound/soc/sti/sti_uniperif.c b/sound/soc/sti/sti_uniperif.c
index 549fac3..a958144 100644
--- a/sound/soc/sti/sti_uniperif.c
+++ b/sound/soc/sti/sti_uniperif.c
@@ -245,8 +245,6 @@ static int sti_uniperiph_dai_create_ctrl(struct snd_soc_dai *dai)
 		 * Uniperipheral instance ID
 		 */
 		ctrl = &uni->snd_ctrls[i];
-		ctrl->index = uni->id;
-		ctrl->device = uni->id;
 	}
 
 	return snd_soc_add_dai_controls(dai, uni->snd_ctrls, uni->num_ctrls);
@@ -348,7 +346,8 @@ static int sti_uniperiph_dai_probe(struct snd_soc_dai *dai)
 static const struct snd_soc_dai_driver sti_uniperiph_dai_template = {
 	.probe = sti_uniperiph_dai_probe,
 	.suspend = sti_uniperiph_dai_suspend,
-	.resume = sti_uniperiph_dai_resume
+	.resume = sti_uniperiph_dai_resume,
+	.bind_pcm_ctl = true,
 };
 
 static const struct snd_soc_component_driver sti_uniperiph_dai_component = {
-- 
1.9.1

[RFC 4/4 ] iecset: allow to select control with device and sub-device numbers.

[Arnaud Pouliquen]

ASoc implementation force the index to 0.
In case of multi devices that export the iec control, we need device and
subdevice fields to differentiate and address the different controls.
This patch extends "-D" option to be able to address control using
PCM device/sub-device id.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 iecset/iecset.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/iecset/iecset.c b/iecset/iecset.c
index 92a93e8..8d7a800 100644
--- a/iecset/iecset.c
+++ b/iecset/iecset.c
@@ -89,6 +89,8 @@ static void usage(void)
 	printf("Usage: iecset [options] [cmd arg...]\n");
 	printf("Options:\n");
 	printf("    -D device   specifies the control device to use\n");
+	printf("                for \"-Dhw:X,Y,Z\" Only control device X is mandatory\n");
+	printf("                the device(Y) and subdevice(Z) are optinal (default = 0)\n");
 	printf("    -c card     specifies the card number to use (equiv. with -Dhw:#)\n");
 	printf("    -n number   specifies the control index number (default = 0)\n");
 	printf("    -x          dump the dump the AESx hex code for IEC958 PCM parameters\n");
@@ -99,6 +101,15 @@ static void usage(void)
 	}
 }
 
+static int parse_device(char *parms, int *ctl_dev, int *dev, int *sdev)
+{
+
+	if (strncmp(parms, "hw", 2))
+		return 1;
+	sscanf(parms,"hw:%d,%d,%d",ctl_dev, dev, sdev);
+
+	return 0;
+}
 
 /*
  * parse iecset commands
@@ -312,6 +323,7 @@ int main(int argc, char **argv)
 	const char *dev = "default";
 	const char *spdif_str = SND_CTL_NAME_IEC958("", PLAYBACK, DEFAULT);
 	int spdif_index = -1;
+	int cdev, spdif_dev = -1, spdif_sdev = -1;
 	snd_ctl_t *ctl;
 	snd_ctl_elem_list_t *clist;
 	snd_ctl_elem_id_t *cid;
@@ -330,7 +342,11 @@ int main(int argc, char **argv)
 	while ((c = getopt(argc, argv, "D:c:n:xhi")) != -1) {
 		switch (c) {
 		case 'D':
-			dev = optarg;
+			if (parse_device(optarg, &cdev, &spdif_dev, &spdif_sdev))
+				dev = optarg;
+			else
+				sprintf(tmpname, "hw:%d", cdev);
+				dev = tmpname;
 			break;
 		case 'c':
 			i = atoi(optarg);
@@ -376,15 +392,19 @@ int main(int argc, char **argv)
 	}
 
 	controls = snd_ctl_elem_list_get_used(clist);
-	for (cidx = 0; cidx < controls; cidx++) {
-		if (!strcmp(snd_ctl_elem_list_get_name(clist, cidx), spdif_str))
-			if (spdif_index < 0 ||
-			    snd_ctl_elem_list_get_index(clist, cidx) == spdif_index)
+	for (cidx = 0; cidx < controls; cidx++)
+		if (!strcmp(snd_ctl_elem_list_get_name(clist, cidx), spdif_str)) {
+			if ((spdif_index < 0 ||
+			     snd_ctl_elem_list_get_index(clist, cidx) == spdif_index) &&
+			    (spdif_dev < 0 ||
+			     snd_ctl_elem_list_get_device(clist, cidx) == spdif_dev) &&
+			    (spdif_sdev < 0 ||
+			     snd_ctl_elem_list_get_subdevice(clist, cidx) == spdif_sdev))
 				break;
 	}
 	if (cidx >= controls) {
-		fprintf(stderr, "control \"%s\" (index %d) not found\n",
-			spdif_str, spdif_index);
+		fprintf(stderr, "control \"%s\" (index %d, device %d, subdevice %d) not found\n",
+			spdif_str, spdif_index, spdif_dev, spdif_sdev);
 		return 1;
 	}
 
-- 
1.9.1

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Clemens Ladisch]

Arnaud Pouliquen wrote:
> 1-1) How to Instantiate generic ALSA control
>
> The  point is the handling of multi instance of generic ALSA controls.
> In this case "prefix" can not be used. Controls have to be identified
> by a combination of device/sub-device/index

Controls are identified either
- by their numid, or
- by the combination of iface/device/subdevice/name/index.

The device/subdevice fields are used to link controls with PCM devices;
this is needed for per-stream volume controls or for the S/PDIF "Mask"
controls, which set properties of some PCM stream.

>     Should we always apply this rules?
>       - MIXER control type: as it is not linked to PCM device but card,
> 	"index" is used to instantiate control.

"index" is _always_ used.

>       - PCM control type: as it is linked to PCM device,
>         "device/subdevice" is used to instantiate control.
>     Or could we consider that a control can be instantiated using
>       device/subdevice fields  and/or index fields?

The amixer tool does not allow controls with the same iface/name/index
values, regardless of the (sub)device values.  This appears to be a bug
in amixer (because the behaviour is different from the kernel's), but
the (sub)device fields were never intended to be used by MIXER controls,
and PCM controls typically are inactive when their stream is closed.

>    - Concerning IEC controls, should we consider them as PCM or MIXER
>      type controls? In current implementations both are used...

MIXER controls are shown in alsamixer.  PCM controls are hidden, and
accessed by software.

Something like "IEC958 Playback Switch" should be MIXER, and "IEC958
Playback Con Mask", PCM.

> 2) From driver point of view
>
> 2-1) None SOC drivers:
> Today solution seems to base indexation on both "index" and "device"
> number. Device indexation seems not linked to PCM device number
> Example in  ea9b43addc4d ("ALSA: hda - Fix broken workaround for HDMI/SPDIF conflicts").
> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b43add
>
> => The drawback is that each driver has to implement it. Could be nice
>    to have in generic code....

In the good old times, the ALSA framework assumed that a driver knows
all its controls, and handles conflicts by assignind index values.

It would be useful to let snd_ctl_add() assign a new index automatically,
but only if explicitly requested (e.g., something like
"#define SNDRV_CTL_INDEX_AUTO -1").

> 2-2) SOC drivers:
>   - Relation chip between control and PCM device. if rules mentioned
>     in 1-3 should be respected, need to link control to PCM device.

What is this link needed for?  Is that control set by software (PCM), or
by the user (MIXER)?


Regards,
Clemens

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Arnaud Pouliquen]

Hello Clemens,

On 11/08/2016 10:52 AM, Clemens Ladisch wrote:
> Arnaud Pouliquen wrote:
>> 1-1) How to Instantiate generic ALSA control
>>
>> The  point is the handling of multi instance of generic ALSA controls.
>> In this case "prefix" can not be used. Controls have to be identified
>> by a combination of device/sub-device/index
> 
> Controls are identified either
> - by their numid, or
> - by the combination of iface/device/subdevice/name/index.
> 
> The device/subdevice fields are used to link controls with PCM devices;
> this is needed for per-stream volume controls or for the S/PDIF "Mask"
> controls, which set properties of some PCM stream.
Seems that today device/subdevice are also used to differentiate PCM
control, using a "virtual" device/subdevice that is not linked to a
"real PCM device. This is what i understand when i parse HDA-Intel.conf
for instance HDA-Intel.pcm.hdmi.1: "DEVICE=7,"

> 
>>     Should we always apply this rules?
>>       - MIXER control type: as it is not linked to PCM device but card,
>> 	"index" is used to instantiate control.
> 
> "index" is _always_ used.
> 
>>       - PCM control type: as it is linked to PCM device,
>>         "device/subdevice" is used to instantiate control.
>>     Or could we consider that a control can be instantiated using
>>       device/subdevice fields  and/or index fields?
> 
> The amixer tool does not allow controls with the same iface/name/index
> values, regardless of the (sub)device values.  This appears to be a bug
> in amixer (because the behaviour is different from the kernel's), but
> the (sub)device fields were never intended to be used by MIXER controls,
In fact only amixer simple controls are not handled using (sub)device
fields. if simple control is only MIXER controls, it is not a bug as it
respect rules.

> and PCM controls typically are inactive when their stream is closed.
if we consider that 'IEC958 Playback Default' is a PCM control. This is
not compatible with iecset application that sets the control without
opening the PCM device.

> 
>>    - Concerning IEC controls, should we consider them as PCM or MIXER
>>      type controls? In current implementations both are used...
> 
> MIXER controls are shown in alsamixer.  PCM controls are hidden, and
> accessed by software.
> 
> Something like "IEC958 Playback Switch" should be MIXER, and "IEC958
> Playback Con Mask", PCM.
> 
>> 2) From driver point of view
>>
>> 2-1) None SOC drivers:
>> Today solution seems to base indexation on both "index" and "device"
>> number. Device indexation seems not linked to PCM device number
>> Example in  ea9b43addc4d ("ALSA: hda - Fix broken workaround for HDMI/SPDIF conflicts").
>> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b43add
>>
>> => The drawback is that each driver has to implement it. Could be nice
>>    to have in generic code....
> 
> In the good old times, the ALSA framework assumed that a driver knows
> all its controls, and handles conflicts by assignind index values.
> 
> It would be useful to let snd_ctl_add() assign a new index automatically,
> but only if explicitly requested (e.g., something like
> "#define SNDRV_CTL_INDEX_AUTO -1").
you right, seems more reasonable, I will update this for a V2, if
everyone is ok for this solution...

> 
>> 2-2) SOC drivers:
>>   - Relation chip between control and PCM device. if rules mentioned
>>     in 1-3 should be respected, need to link control to PCM device.
> 
> What is this link needed for?  Is that control set by software (PCM), or
> by the user (MIXER)?
It is for PCM controls only.
Today In ASoC implementation, DAI driver is not directly linked to the
PCM device. So it is not possible for PCM controls to set a coherent
PCM device field value.
For patch proposed , it could also be interesting to use a
SNDRV_CTL_DEVICE_AUTO -1 to be coherent with your proposal above.

Thanks and Regards,
Arnaud

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Takashi Iwai]

On Tue, 08 Nov 2016 15:16:29 +0100,
Arnaud Pouliquen wrote:
> 
> Hello Clemens,
> 
> On 11/08/2016 10:52 AM, Clemens Ladisch wrote:
> > Arnaud Pouliquen wrote:
> >> 1-1) How to Instantiate generic ALSA control
> >>
> >> The  point is the handling of multi instance of generic ALSA controls.
> >> In this case "prefix" can not be used. Controls have to be identified
> >> by a combination of device/sub-device/index
> > 
> > Controls are identified either
> > - by their numid, or
> > - by the combination of iface/device/subdevice/name/index.
> > 
> > The device/subdevice fields are used to link controls with PCM devices;
> > this is needed for per-stream volume controls or for the S/PDIF "Mask"
> > controls, which set properties of some PCM stream.
> Seems that today device/subdevice are also used to differentiate PCM
> control, using a "virtual" device/subdevice that is not linked to a
> "real PCM device. This is what i understand when i parse HDA-Intel.conf
> for instance HDA-Intel.pcm.hdmi.1: "DEVICE=7,"

The HD-audio shouldn't be taken as the good reference.  It has its own
mapping there just to keep the backward compatibility with the old
implementation that was basically broken.


> > 
> >>     Should we always apply this rules?
> >>       - MIXER control type: as it is not linked to PCM device but card,
> >> 	"index" is used to instantiate control.
> > 
> > "index" is _always_ used.
> > 
> >>       - PCM control type: as it is linked to PCM device,
> >>         "device/subdevice" is used to instantiate control.
> >>     Or could we consider that a control can be instantiated using
> >>       device/subdevice fields  and/or index fields?
> > 
> > The amixer tool does not allow controls with the same iface/name/index
> > values, regardless of the (sub)device values.  This appears to be a bug
> > in amixer (because the behaviour is different from the kernel's), but
> > the (sub)device fields were never intended to be used by MIXER controls,
> In fact only amixer simple controls are not handled using (sub)device
> fields. if simple control is only MIXER controls, it is not a bug as it
> respect rules.
> 
> > and PCM controls typically are inactive when their stream is closed.
> if we consider that 'IEC958 Playback Default' is a PCM control. This is
> not compatible with iecset application that sets the control without
> opening the PCM device.

Yes, it's a slight inconsistency.  But it's specific to "Default"
stuff.  All other controls are usually tied with PCM open/close.

> > 
> >>    - Concerning IEC controls, should we consider them as PCM or MIXER
> >>      type controls? In current implementations both are used...
> > 
> > MIXER controls are shown in alsamixer.  PCM controls are hidden, and
> > accessed by software.
> > 
> > Something like "IEC958 Playback Switch" should be MIXER, and "IEC958
> > Playback Con Mask", PCM.
> > 
> >> 2) From driver point of view
> >>
> >> 2-1) None SOC drivers:
> >> Today solution seems to base indexation on both "index" and "device"
> >> number. Device indexation seems not linked to PCM device number
> >> Example in  ea9b43addc4d ("ALSA: hda - Fix broken workaround for HDMI/SPDIF conflicts").
> >> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b43add
> >>
> >> => The drawback is that each driver has to implement it. Could be nice
> >>    to have in generic code....
> > 
> > In the good old times, the ALSA framework assumed that a driver knows
> > all its controls, and handles conflicts by assignind index values.
> > 
> > It would be useful to let snd_ctl_add() assign a new index automatically,
> > but only if explicitly requested (e.g., something like
> > "#define SNDRV_CTL_INDEX_AUTO -1").
> you right, seems more reasonable, I will update this for a V2, if
> everyone is ok for this solution...

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.


thanks,

Takashi

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Arnaud Pouliquen]

On 11/08/2016 03:30 PM, Takashi Iwai wrote:
> On Tue, 08 Nov 2016 15:16:29 +0100,
> Arnaud Pouliquen wrote:
>>
>> Hello Clemens,
>>
>> On 11/08/2016 10:52 AM, Clemens Ladisch wrote:
>>> Arnaud Pouliquen wrote:
>>>> 1-1) How to Instantiate generic ALSA control
>>>>
>>>> The  point is the handling of multi instance of generic ALSA controls.
>>>> In this case "prefix" can not be used. Controls have to be identified
>>>> by a combination of device/sub-device/index
>>>
>>> Controls are identified either
>>> - by their numid, or
>>> - by the combination of iface/device/subdevice/name/index.
>>>
>>> The device/subdevice fields are used to link controls with PCM devices;
>>> this is needed for per-stream volume controls or for the S/PDIF "Mask"
>>> controls, which set properties of some PCM stream.
>> Seems that today device/subdevice are also used to differentiate PCM
>> control, using a "virtual" device/subdevice that is not linked to a
>> "real PCM device. This is what i understand when i parse HDA-Intel.conf
>> for instance HDA-Intel.pcm.hdmi.1: "DEVICE=7,"
> 
> The HD-audio shouldn't be taken as the good reference.  It has its own
> mapping there just to keep the backward compatibility with the old
> implementation that was basically broken.
> 
> 
>>>
>>>>     Should we always apply this rules?
>>>>       - MIXER control type: as it is not linked to PCM device but card,
>>>> 	"index" is used to instantiate control.
>>>
>>> "index" is _always_ used.
>>>
>>>>       - PCM control type: as it is linked to PCM device,
>>>>         "device/subdevice" is used to instantiate control.
>>>>     Or could we consider that a control can be instantiated using
>>>>       device/subdevice fields  and/or index fields?
>>>
>>> The amixer tool does not allow controls with the same iface/name/index
>>> values, regardless of the (sub)device values.  This appears to be a bug
>>> in amixer (because the behaviour is different from the kernel's), but
>>> the (sub)device fields were never intended to be used by MIXER controls,
>> In fact only amixer simple controls are not handled using (sub)device
>> fields. if simple control is only MIXER controls, it is not a bug as it
>> respect rules.
>>
>>> and PCM controls typically are inactive when their stream is closed.
>> if we consider that 'IEC958 Playback Default' is a PCM control. This is
>> not compatible with iecset application that sets the control without
>> opening the PCM device.
> 
> Yes, it's a slight inconsistency.  But it's specific to "Default"
> stuff.  All other controls are usually tied with PCM open/close.
> 
Thanks for the clarification.
In this case, "[RFC 4/4 ] iecset: allow to select control with device
and sub-device numbers" seems coherent, to allow access to this PCM
controls with device/subdevice values...
I will propose it in a separate patch-set to not mix kernel and
application patch...

As 'IEC958 Playback Default' is specific, i will also try to
rework and propose a new version of
[PATCH v4 3/6] ALSA: pcm: add IEC958 channel status control helper
(http://www.spinics.net/lists/alsa-devel/msg47615.html)

>>>
>>>>    - Concerning IEC controls, should we consider them as PCM or MIXER
>>>>      type controls? In current implementations both are used...
>>>
>>> MIXER controls are shown in alsamixer.  PCM controls are hidden, and
>>> accessed by software.
>>>
>>> Something like "IEC958 Playback Switch" should be MIXER, and "IEC958
>>> Playback Con Mask", PCM.
>>>
>>>> 2) From driver point of view
>>>>
>>>> 2-1) None SOC drivers:
>>>> Today solution seems to base indexation on both "index" and "device"
>>>> number. Device indexation seems not linked to PCM device number
>>>> Example in  ea9b43addc4d ("ALSA: hda - Fix broken workaround for HDMI/SPDIF conflicts").
>>>> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b43add
>>>>
>>>> => The drawback is that each driver has to implement it. Could be nice
>>>>    to have in generic code....
>>>
>>> In the good old times, the ALSA framework assumed that a driver knows
>>> all its controls, and handles conflicts by assignind index values.
>>>
>>> It would be useful to let snd_ctl_add() assign a new index automatically,
>>> but only if explicitly requested (e.g., something like
>>> "#define SNDRV_CTL_INDEX_AUTO -1").
>> you right, seems more reasonable, I will update this for a V2, if
>> everyone is ok for this solution...
> 
> 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.

I will send a V2 that integrates SNDRV_CTL_INDEX_AUTO (and
SNDRV_CTL_DEVICE_AUTO for the ASoC PCM link)

Thanks & Regards
Arnaud

Re: [RFC 2/4] Alsa: control: increment index field for duplicated control.

[Takashi Sakamoto]

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


Regards

Takashi Sakamoto

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

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Takashi Sakamoto]

Hi,

On Nov 8 2016 17:11, Arnaud Pouliquen wrote:
> 3) Patches proposed:
> Based on these observations, here are some patches that i tested in my
> environment. There are complementary,  and could answer to some
> limitations mentioned above.
>
> 3-1) Alsa-utils patch
>
> - iecset: allow to select control with device and sub-device numbers
>   This patch allows to access to 2 iec controls differentiated by
>   device/sub-devices numbers
> => For me, this patch is mandatory to be able to address the ASoC IEC
>    controls, in case of no fix is implemented to allows index field
>    update in ASoC.
>
> 3-2) Alsa driver patches
>   - ASoC: core: allow PCM control binding to PCM device
>   	Add relationship between DAIs PCM controls and PCM device.
>
>   - ALSA: control: increment index field for duplicated control.
>    	Generic implementation of the patch proposed in HDA driver
>         (http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b43add)
>
>   - ASoC: sti: use bind_pcm_ctl
>   	implementation of bind_pcm_ctl for sti driver.

In my view, this patchset includes two attempts:
1.to add a framework into ALSA SoC part to relate some control element 
sets to PCM devices
2.to allow drivers in ALSA SoC part to add some control element sets 
from the same codes according to entries of dtb

To achieve above 2nd attempt, it changes ALSA control core to accept 
several control element sets with the same name by assigning proper 
indexes to the sets.

Well, since your former messages, you continue using the word, 'general' 
or 'generic'. If it were somewhat general, it should satisfy 
requirements in whole this subsystem. Actually, there's none of such 
requirements outside of ALSA SoC part. For the drivers outside of ALSA 
SoC part, design of target sound card is fixed in advance, therefore 
programmers can assign control element sets to PCM devices in usual 
ways. At least, current infrastructure in ALSA control core can satisfy 
the programmers.

Therefore, I think that your attempts includes over-generalization. In 
theory, it can bring over-specification easily and it causes code 
complication or unmaintained codes.


Focusing on ALSA SoC part, there's a requirement to register control 
element sets from the same code, in fact. So I wonder why you don't 
start your explanation in an aspect of it. In short, I can't understand 
the reason that you adhere to such an inappropriate generalization for 
this subsystem.

In this patchset, you adhere to usage of 'index' field. But there's 
another way; assigning different identification information to the 
control element sets. Let us to start discussion from this point? At 
least, Iwai-san said, former driver for Intel High Definition Audio is 
necessarily not a good example for a model to c
onsider about this issue and the usage of 'index' is not 
well-generalized[1].

Finally, it's better to clear main points of your issue, for practical 
and meaningful discussion for better approaches, before starting this 
discussion, I think. At least, there's over-generalization, 
misunderstandings of design and interfaces, driver-specific historical 
reasons and so on.


[1] [alsa-devel] [RFC 0/4] ALSA controls management using 
index/device/sub-devices fields
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/114430.html


Regards

Takashi Sakamoto

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Arnaud Pouliquen]

Hi,

On 11/09/2016 01:33 PM, Takashi Sakamoto wrote:
> Hi,
> 
> On Nov 8 2016 17:11, Arnaud Pouliquen wrote:
>> 3) Patches proposed:
>> Based on these observations, here are some patches that i tested in my
>> environment. There are complementary,  and could answer to some
>> limitations mentioned above.
>>
>> 3-1) Alsa-utils patch
>>
>> - iecset: allow to select control with device and sub-device numbers
>>   This patch allows to access to 2 iec controls differentiated by
>>   device/sub-devices numbers
>> => For me, this patch is mandatory to be able to address the ASoC IEC
>>    controls, in case of no fix is implemented to allows index field
>>    update in ASoC.
>>
>> 3-2) Alsa driver patches
>>   - ASoC: core: allow PCM control binding to PCM device
>>   	Add relationship between DAIs PCM controls and PCM device.
>>
>>   - ALSA: control: increment index field for duplicated control.
>>    	Generic implementation of the patch proposed in HDA driver
>>         (http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b43add)
>>
>>   - ASoC: sti: use bind_pcm_ctl
>>   	implementation of bind_pcm_ctl for sti driver.
> 
> In my view, this patchset includes two attempts:
> 1.to add a framework into ALSA SoC part to relate some control element 
> sets to PCM devices
> 2.to allow drivers in ALSA SoC part to add some control element sets 
> from the same codes according to entries of dtb
> 
> To achieve above 2nd attempt, it changes ALSA control core to accept 
> several control element sets with the same name by assigning proper 
> indexes to the sets.
> 
> Well, since your former messages, you continue using the word, 'general' 
> or 'generic'. If it were somewhat general, it should satisfy 
> requirements in whole this subsystem. Actually, there's none of such 
> requirements outside of ALSA SoC part. For the drivers outside of ALSA 
> SoC part, design of target sound card is fixed in advance, therefore 
> programmers can assign control element sets to PCM devices in usual 
> ways. At least, current infrastructure in ALSA control core can satisfy 
> the programmers.
> 
> Therefore, I think that your attempts includes over-generalization. In 
> theory, it can bring over-specification easily and it causes code 
> complication or unmaintained codes.

I based my RFC on the fact that i was facing same kind of problem than
HDA driver (last time i mention it). In this context, i don't think that
it was senseless to have a global approach.
If not the good approach, let's refocus on ASoC, that's fine by me.
> 
> 
> Focusing on ALSA SoC part, there's a requirement to register control 
> element sets from the same code, in fact. So I wonder why you don't 
> start your explanation in an aspect of it. In short, I can't understand 
> the reason that you adhere to such an inappropriate generalization for 
> this subsystem.
> 
> In this patchset, you adhere to usage of 'index' field. But there's 
> another way; assigning different identification information to the 
> control element sets. Let us to start discussion from this point? At 
> least, Iwai-san said, former driver for Intel High Definition Audio is 
> necessarily not a good example for a model to c
> onsider about this issue and the usage of 'index' is not 
> well-generalized[1].
> 
> Finally, it's better to clear main points of your issue, for practical 
> and meaningful discussion for better approaches, before starting this 
> discussion, I think. At least, there's over-generalization, 
> misunderstandings of design and interfaces, driver-specific historical 
> reasons and so on.
> 
> 
> [1] [alsa-devel] [RFC 0/4] ALSA controls management using 
> index/device/sub-devices fields
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/114430.html
> 
So I propose to continue discussion on a simple and concrete use case:
The 'IEC958 Playback Default' control.

In my ASoC driver i have one HDMI and one SPDIF outputs.
so I have 2'IEC958 Playback Default' PCM controls.
=> Each control can be set independently.

Regarding control identification field (struct snd_ctl_elem_i):
 .numid;  => set by ALSA framework
 .iface;  => must be SNDRV_CTL_ELEM_IFACE_PCM
 .device; => must be linked to PCM device , but not possible for
             ASoC DAI...
 .subdevice => not used in ASoC implementation
 .name      => 'IEC958 Playback Default'
 .index     => not used in ASoC, forced to 0 by snd_soc_cnew

Other solution: use control "prefix"? not possible as control has a
pre-defined name.

=> Only solution to differentiate the control: "device" field.
   (that seems coherent as it a PCM device).

Issues:
     - use "device" in ASOC DAI driver means that driver needs to
       define a "virtual" PCM device value, not the PCM device.
	=> this break the rule that mention that PCM control should be
        linked to a PCM device.
       Furthermore, this "virtual" value has to be aligned with the one
       defined in alsa-lib conf file(s).
     - iecset uses only index to differentiate IEC controls. But in
       ASoC implementation this is not possible as index is forced to 0.

Regards
Arnaud

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Takashi Sakamoto]

Hi,

On Nov 9 2016 23:38, Arnaud Pouliquen wrote:
>> In my view, this patchset includes two attempts:
>> 1.to add a framework into ALSA SoC part to relate some control element
>> sets to PCM devices
>> 2.to allow drivers in ALSA SoC part to add some control element sets
>> from the same codes according to entries of dtb
>>
>> To achieve above 2nd attempt, it changes ALSA control core to accept
>> several control element sets with the same name by assigning proper
>> indexes to the sets.
>>
>> Well, since your former messages, you continue using the word, 'general'
>> or 'generic'. If it were somewhat general, it should satisfy
>> requirements in whole this subsystem. Actually, there's none of such
>> requirements outside of ALSA SoC part. For the drivers outside of ALSA
>> SoC part, design of target sound card is fixed in advance, therefore
>> programmers can assign control element sets to PCM devices in usual
>> ways. At least, current infrastructure in ALSA control core can satisfy
>> the programmers.
>>
>> Therefore, I think that your attempts includes over-generalization. In
>> theory, it can bring over-specification easily and it causes code
>> complication or unmaintained codes.
>
> I based my RFC on the fact that i was facing same kind of problem than
> HDA driver (last time i mention it). In this context, i don't think that
> it was senseless to have a global approach.

Unless you have special interests in verb parser of HDA specification 
and work for it, it's better not to address to it. Else, you would be 
involved into more complicated stories; old and new Intel HDA driver, 
historical reasons of former Intel HDA driver, variations of HDA codecs 
produced by several vendors, vendor-dependent quirks of ACPI table, and 
so on.

> If not the good approach, let's refocus on ASoC, that's fine by me.

In my taste, this approach is better for your main issue. After solving 
the issue and you have enough rest to work for more-generic one, you 
could do it.

>> Focusing on ALSA SoC part, there's a requirement to register control
>> element sets from the same code, in fact. So I wonder why you don't
>> start your explanation in an aspect of it. In short, I can't understand
>> the reason that you adhere to such an inappropriate generalization for
>> this subsystem.
>>
>> In this patchset, you adhere to usage of 'index' field. But there's
>> another way; assigning different identification information to the
>> control element sets. Let us to start discussion from this point? At
>> least, Iwai-san said, former driver for Intel High Definition Audio is
>> necessarily not a good example for a model to c
>> onsider about this issue and the usage of 'index' is not
>> well-generalized[1].
>>
>> Finally, it's better to clear main points of your issue, for practical
>> and meaningful discussion for better approaches, before starting this
>> discussion, I think. At least, there's over-generalization,
>> misunderstandings of design and interfaces, driver-specific historical
>> reasons and so on.
>>
>>
>> [1] [alsa-devel] [RFC 0/4] ALSA controls management using
>> index/device/sub-devices fields
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/114430.html
>>
> So I propose to continue discussion on a simple and concrete use case:
> The 'IEC958 Playback Default' control.
>
> In my ASoC driver i have one HDMI and one SPDIF outputs.
> so I have 2'IEC958 Playback Default' PCM controls.
> => Each control can be set independently.

Yes. It's a demand in your issue.
(here, I ignore a process to generalize the issue for wider range in 
this subsystem because it's another issue.)

> Regarding control identification field (struct snd_ctl_elem_i):
>  .numid;  => set by ALSA framework
>  .iface;  => must be SNDRV_CTL_ELEM_IFACE_PCM
>  .device; => must be linked to PCM device , but not possible for
>              ASoC DAI...
>  .subdevice => not used in ASoC implementation
>  .name      => 'IEC958 Playback Default'
>  .index     => not used in ASoC, forced to 0 by snd_soc_cnew
>
> Other solution: use control "prefix"? not possible as control has a
> pre-defined name.
>
> => Only solution to differentiate the control: "device" field.
>    (that seems coherent as it a PCM device).

This is one of solutions we can perform for this issue. As a feasibility 
study, in the end of this message, I wrote a small program with a 
feature of 'user-defined control element set' in ALSA control core.

I think usage of 'device' field is better than usage of prefix for 
'name' field. It clearly represents the relationship between control 
element set and PCM devices. And it's just suitable for this issue.

> Issues:
>      - use "device" in ASOC DAI driver means that driver needs to
>        define a "virtual" PCM device value, not the PCM device.
> 	=> this break the rule that mention that PCM control should be
>         linked to a PCM device.

Please show me where related codes and structures are. At least, I 
cannot understand what you said because it's really abstracted.

>        Furthermore, this "virtual" value has to be aligned with the one
>        defined in alsa-lib conf file(s).

Ditto.

>      - iecset uses only index to differentiate IEC controls. But in
>        ASoC implementation this is not possible as index is forced to 0.

_Apparently_, mixer APIs in alsa-lib is not well-designed to represent 
capacity of ALSA control core. It's not better to judge somwthing 
according to its design.

Although we need to improve iecset tool, this is another issue.

-------- 8< --------

#include <stdio.h>
#include <stdlib.h>

#include <string.h>
#include <errno.h>

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#include <sys/ioctl.h>

#include <unistd.h>

#include <sound/asound.h>

int main(void)
{
     int fd;
     struct snd_ctl_elem_info info = {0};
     int first_numid = 0;
     int second_numid = 0;

     fd = open("/dev/snd/controlC0", O_RDWR);
     if (fd < 0) {
         printf("open(2): %s\n", strerror(errno));
         return EXIT_FAILURE;
     }

     /* Identification information. */
     info.id.iface = SNDRV_CTL_ELEM_IFACE_PCM;
     info.id.subdevice = 0;
     strncpy(info.id.name, "IEC958 Playback Default", sizeof(info.id.name));
     info.id.index = 0;

     /* Common information. */
     info.type = SNDRV_CTL_ELEM_TYPE_IEC958;
	info.access = SNDRV_CTL_ELEM_ACCESS_READWRITE;
     info.count = 1;
     info.owner = 1;

     /* Add the first control element set. */
     info.id.numid = 0;
     info.id.device = 0;
     if (ioctl(fd, SNDRV_CTL_IOCTL_ELEM_ADD, &info) < 0) {
         printf("ioctl(2): %s\n", strerror(errno));
         goto end;
     }
     first_numid = info.id.numid;

     /* Add the second control element set. */
     info.id.numid = 0;
     info.id.device = 1;
     if (ioctl(fd, SNDRV_CTL_IOCTL_ELEM_ADD, &info) < 0) {
     }
     second_numid = info.id.numid;
end:
     if (first_numid > 0) {
         info.id.numid = first_numid;
         ioctl(fd, SNDRV_CTL_IOCTL_ELEM_REMOVE, &info);
     }

     if (second_numid > 0) {
         info.id.numid = second_numid;
         ioctl(fd, SNDRV_CTL_IOCTL_ELEM_REMOVE, &info);
     }

     close(fd);

     return EXIT_SUCCESS;
}

Regards

Takashi Sakamoto

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Takashi Sakamoto]

On Nov 9 2016 23:38, Arnaud Pouliquen wrote:
> Other solution: use control "prefix"? not possible as control has a
> pre-defined name.

I note that we can give unique names to control element set for IEC 
60958. According to this document, Between "IEC958" and the rest, we can 
put arbitrary string.
https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/Documentation/sound/designs/control-names.rst

For your information.


Takashi Sakamoto

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Arnaud Pouliquen]

hello Takashi,

On 11/12/2016 05:23 AM, Takashi Sakamoto wrote:
>> So I propose to continue discussion on a simple and concrete use case:
>> The 'IEC958 Playback Default' control.
>>
>> In my ASoC driver i have one HDMI and one SPDIF outputs.
>> so I have 2'IEC958 Playback Default' PCM controls.
>> => Each control can be set independently.
> 
> Yes. It's a demand in your issue.
> (here, I ignore a process to generalize the issue for wider range in 
> this subsystem because it's another issue.)
> 
>> Regarding control identification field (struct snd_ctl_elem_i):
>>  .numid;  => set by ALSA framework
>>  .iface;  => must be SNDRV_CTL_ELEM_IFACE_PCM
>>  .device; => must be linked to PCM device , but not possible for
>>              ASoC DAI...
>>  .subdevice => not used in ASoC implementation
>>  .name      => 'IEC958 Playback Default'
>>  .index     => not used in ASoC, forced to 0 by snd_soc_cnew
>>
>> Other solution: use control "prefix"? not possible as control has a
>> pre-defined name.
>>
>> => Only solution to differentiate the control: "device" field.
>>    (that seems coherent as it a PCM device).
> 
> This is one of solutions we can perform for this issue. As a feasibility 
> study, in the end of this message, I wrote a small program with a 
> feature of 'user-defined control element set' in ALSA control core.
> 
> I think usage of 'device' field is better than usage of prefix for 
> 'name' field. It clearly represents the relationship between control 
> element set and PCM devices. And it's just suitable for this issue.
> 
This is also my view.
Now base on your mail, that point possibility to extend name
(http://www.spinics.net/lists/alsa-devel/msg56713.html):
> I note that we can give unique names to control element set for IEC
> 60958. According to this document, Between "IEC958" and the rest, we can
> put arbitrary string.
>
https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/Documentation/sound/designs/control-names.rst

I missed this, Seems that name is also another solution for IEC controls.
Gstreamer and pulseaudio use PCM name to set the AES value (e.g.
'B2120.pcm.hdmi.0:CARD=0,AES0=4,AES1=130,AES2=0,AES3=2' so simple
to adapt alsa conf file with good name.
iecset could be adapted in a quite simple way using device name or an
new option ( e.g. "-e name control name extension IEC958 <name> Playback
Default")

But, perhaps better to use device... as it match with expected PCM
control rule and as it could be applied for controls
based on standard syntax: [LOCATION] SOURCE [CHANNEL] [DIRECTION] FUNCTION

>> Issues:
>>      - use "device" in ASOC DAI driver means that driver needs to
>>        define a "virtual" PCM device value, not the PCM device.
>> 	=> this break the rule that mention that PCM control should be
>>         linked to a PCM device.
> 
> Please show me where related codes and structures are. At least, I 
> cannot understand what you said because it's really abstracted.
As example, here is my current implementation (correspond to the piece
of code i would like to rework):
http://www.lingrok.org/xref/linux-linus/sound/soc/sti/sti_uniperif.c#232
- "device" field is set to the instance ID of the DAI uniperiph player.
- "index" filed is also set but overwritten by ASoC core.
In this implementation what i name "virtual device is the "device" value
uni->id
HDMI: uni->id = 0 ( PCM device associated is hw:0,0)
SPDIF: uni->id = 3 ( PCM device associated is hw:0,2)

If i don't set the device value then i can not create both controls.
A better way should be to set device to PCM device.
This was the topic of [RFC 1/4] ASoC: core: allow PCM control binding to
PCM device (http://www.spinics.net/lists/alsa-devel/msg56482.html).

> 
>>        Furthermore, this "virtual" value has to be aligned with the one
>>        defined in alsa-lib conf file(s).
> 
> Ditto.
I have not upstreamed my card conf file but an example is available at
end of my mail.
Value of the device field must be aligned with ASoC driver device values:
HDMI: STI-B2120.pcm.hdmi.0.hooks.0.device=0
SPDIF: STI-B2120.pcm.iec958.0.hooks.0.device=3

If these controls were associated to the PCM device, I should not have
to fix the value but I should be able to retrieve it.

> 
>>      - iecset uses only index to differentiate IEC controls. But in
>>        ASoC implementation this is not possible as index is forced to 0.
> 
> _Apparently_, mixer APIs in alsa-lib is not well-designed to represent 
> capacity of ALSA control core. It's not better to judge somwthing 
> according to its design.
Not sure to follow you... I can not see any issue in alsa-lib. Just a
limitation of using iecset with ASoC implementation.
> 
> Although we need to improve iecset tool, this is another issue.
> 
>From my point of view, iecset is part of the topic even if i agree that
details on potential update could/should be discuss in a separate thread.
In case, PCM control device value is not aligned with PCM device, a
workaround exists, even if it does not respect the alignment between PCM
control and PCM device...
For iecset, i have no solution except patch it, if we consider that
index field incrementation is not the good solution for PCM controls.
So for me it is important to keep it in mind in discussions.

-------------

<confdir:pcm/iec958.conf>
<confdir:pcm/hdmi.conf>

STI-B2120.pcm.iec958.common {
	@args [ CARD DEVICE CTLINDEX AES0 AES1 AES2 AES3 ]
	@args.CARD {
		type string
	}
	@args.DEVICE {
		type integer
	}
	@args.CTLINDEX {
		type integer
	}
	@args.AES0 {
		type integer
		# consumer, not-copyright, emphasis-none, mode=0
		default 0x00
	}
	@args.AES1 {
		type integer
		# original, PCM coder
		default 0x00
	}
	@args.AES2 {
		type integer
		# source and channel
		default 0x00
	}
	@args.AES3 {
		type integer
		# fs=48000Hz, clock accuracy=1000ppm
		default 0x00
	}
	type hooks
	slave.pcm {
		type hw
		card $CARD
		device $DEVICE
	}
	hooks.0 {
		type ctl_elems
		hook_args [
		{
			interface PCM
			name "IEC958 Playback Default"
			device 3
			lock true
			preserve true
			value [ $AES0 $AES1 $AES2 $AES3 ]
		}
		]
	}
	hint.device $DEVICE
}

STI-B2120.pcm.iec958.0 {
	@args [ CARD AES0 AES1 AES2 AES3 ]
	@args.CARD { type string }
	@args.AES0 { type integer }
	@args.AES1 { type integer }
	@args.AES2 { type integer }
	@args.AES3 { type integer }
	@func refer
	name {
		@func concat
		strings [
			"cards.STI-B2120.pcm.iec958.common:"
			"CARD=" $CARD ","
			"DEVICE=2,"
			"CTLINDEX=0,"
			"AES0=" $AES0 ","
			"AES1=" $AES1 ","
			"AES2=" $AES2 ","
			"AES3=" $AES3
		]
	}
}

STI-B2120.pcm.hdmi.common {
	@args [ CARD DEVICE CTLINDEX AES0 AES1 AES2 AES3 ]
	@args.CARD {
		type string
		default 0
	}
	@args.DEVICE {
		type integer
		default 0
	}
	@args.CTLINDEX {
		type integer
	}
	@args.AES0 {
		type integer
		# consumer, not-copyright, emphasis-none, mode=0
		default 0x00
	}
	@args.AES1 {
		type integer
		# original, PCM coder
		default 0x00
	}
	@args.AES2 {
		type integer
		# source and channel
		default 0x00
	}
	@args.AES3 {
		type integer
		# fs=48000Hz, clock accuracy=1000ppm
		default 0x00I missed this, Seems that name is also another solution
for IEC controls.

	}
	type hooks
	slave.pcm {
		type hw
		card $CARD
		device $DEVICE
	}
	hooks.0 {
		type ctl_elems
		hook_args [
			{
				interface PCM
				name "IEC958 Playback Default"
				device 0
				lock true
				preserve true
				value [ $AES0 $AES1 $AES2 $AES3 ]
			}
		]
	}
	hint.device $DEVICE
}
STI-B2120.pcm.hdmi.0 {
	@args [ CARD AES0 AES1 AES2 AES3 ]
	@args.CARD { type string }
	@args.AES0 { type integer }
	@args.AES1 { type integer }
	@args.AES2 { type integer }
	@args.AES3 { type integer }
	@func refer
	name {
		@func concat
		strings [
			"cards.STI-B2120.pcm.hdmi.common:"
			"CARD=" $CARD ","
			"DEVICE=0,"
			"CTLINDEX=0,"
			"AES0=" $AES0 ","
			"AES1=" $AES1 ","
			"AES2=" $AES2 ","
			"AES3=" $AES3
		]
	}
}

Thanks and Regards
Arnaud

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Takashi Sakamoto]

On Nov 14 2016 22:58, Arnaud Pouliquen wrote:
>>> Issues:
>>>      - use "device" in ASOC DAI driver means that driver needs to
>>>        define a "virtual" PCM device value, not the PCM device.
>>> 	=> this break the rule that mention that PCM control should be
>>>         linked to a PCM device.
>>
>> Please show me where related codes and structures are. At least, I
>> cannot understand what you said because it's really abstracted.
> As example, here is my current implementation (correspond to the piece
> of code i would like to rework):
> http://www.lingrok.org/xref/linux-linus/sound/soc/sti/sti_uniperif.c#232
> - "device" field is set to the instance ID of the DAI uniperiph player.
> - "index" filed is also set but overwritten by ASoC core.
>
> In this implementation what i name "virtual device is the "device" value
> uni->id
> HDMI: uni->id = 0 ( PCM device associated is hw:0,0)
> SPDIF: uni->id = 3 ( PCM device associated is hw:0,2)
>
> If i don't set the device value then i can not create both controls.
> A better way should be to set device to PCM device.
> This was the topic of [RFC 1/4] ASoC: core: allow PCM control binding to
> PCM device (http://www.spinics.net/lists/alsa-devel/msg56482.html).

In below call graph, each module cannot directly indicate the 'device' 
number because 'struct snd_soc_card.num_rtd' is incremented by ALSA SoC 
core and the number is used as an argument for 'snd_pcm_new_internal()' 
or 'snd_pcm_new()'.

(sound/soc/soc-core.c)
snd_soc_register_card()
->snd_soc_instantiate_card()
   ->soc_bind_dai_link() (each on card->num_links/dai_link_list)
     ->soc_new_pcm_runtime()
     ->soc_add_pcm_runtime()
       list_add_tail(&card->rtd_list)
       rtd->num = card->num_rtd
       card->num_rtd++
   ->soc_probe_link_dais() (each on card->rtd_list)
     (sound/soc/soc-pcm.c)
     ->soc_new_pcm(rtd, num = rtd->num)
       ->snd_pcm_new_internal(num)
       ->snd_pcm_new(num)
       struct snd_soc_pcm_runtime.pcm = pcm
   ->snd_card_regiser()

Thus, modules are forced to 'guess' the card structure in a point of PCM
character devices. I think you expressed this as 'virtual'.

Well, as a glance of ALSA SoC core, we have a call of 'struct 
snd_soc_card.late_probe()' just before 'snd_card_register()'. We can 
probably add ad-hoc control element sets for each PCM character devices.

snd_soc_register_card()
->snd_soc_instantiate_card()
   ->soc_bind_dai_link()
   ->soc_probe_link_dais()
   ->snd_soc_add_card_controls()
   ->struct snd_soc_card.late_probe()
   ->snd_card_register()

But it seems to be a bit cumbersome. I think it better to add a smart 
framework for the PCM-related controls. In this point, I agree with your 
direction.

>>>        Furthermore, this "virtual" value has to be aligned with the one
>>>        defined in alsa-lib conf file(s).
>>
>> Ditto.
> I have not upstreamed my card conf file but an example is available at
> end of my mail.
> Value of the device field must be aligned with ASoC driver device values:
> HDMI: STI-B2120.pcm.hdmi.0.hooks.0.device=0
> SPDIF: STI-B2120.pcm.iec958.0.hooks.0.device=3
>
> If these controls were associated to the PCM device, I should not have
> to fix the value but I should be able to retrieve it.

As long as I know, 'pcm/iec958.conf' and 'pcm/hdmi.conf' use different 
strategy to handle PCM-related control element set for IEC958 type. Your 
configuration does not work well for pcm.iec958 plugin.

>>>      - iecset uses only index to differentiate IEC controls. But in
>>>        ASoC implementation this is not possible as index is forced
to 0.
>>
>> _Apparently_, mixer APIs in alsa-lib is not well-designed to represent
>> capacity of ALSA control core. It's not better to judge somwthing
>> according to its design.
> Not sure to follow you... I can not see any issue in alsa-lib. Just a
> limitation of using iecset with ASoC implementation.
>>
>> Although we need to improve iecset tool, this is another issue.
>>
> From my point of view, iecset is part of the topic even if i agree that
> details on potential update could/should be discuss in a separate thread.
> In case, PCM control device value is not aligned with PCM device, a
> workaround exists, even if it does not respect the alignment between PCM
> control and PCM device...
> For iecset, i have no solution except patch it, if we consider that
> index field incrementation is not the good solution for PCM controls.
> So for me it is important to keep it in mind in discussions.

Yes, enough later, we need to discuss about user space.

However, your issue includes a batch of issues on both of kernel land 
and user land. Intractableness, toughness, hardness, something like that 
for each of them is not so light and small. I think it better to solve 
these child issues step by steap. In this context, I said 'another'.


Regards

Takashi Sakamoto

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Arnaud Pouliquen]

On 11/16/2016 10:34 AM, Takashi Sakamoto wrote:
> On Nov 14 2016 22:58, Arnaud Pouliquen wrote:
>>>> Issues:
>>>>      - use "device" in ASOC DAI driver means that driver needs to
>>>>        define a "virtual" PCM device value, not the PCM device.
>>>> 	=> this break the rule that mention that PCM control should be
>>>>         linked to a PCM device.
>>>
>>> Please show me where related codes and structures are. At least, I
>>> cannot understand what you said because it's really abstracted.
>> As example, here is my current implementation (correspond to the piece
>> of code i would like to rework):
>> http://www.lingrok.org/xref/linux-linus/sound/soc/sti/sti_uniperif.c#232
>> - "device" field is set to the instance ID of the DAI uniperiph player.
>> - "index" filed is also set but overwritten by ASoC core.
>>
>> In this implementation what i name "virtual device is the "device" value
>> uni->id
>> HDMI: uni->id = 0 ( PCM device associated is hw:0,0)
>> SPDIF: uni->id = 3 ( PCM device associated is hw:0,2)
>>
>> If i don't set the device value then i can not create both controls.
>> A better way should be to set device to PCM device.
>> This was the topic of [RFC 1/4] ASoC: core: allow PCM control binding to
>> PCM device (http://www.spinics.net/lists/alsa-devel/msg56482.html).
> 
> In below call graph, each module cannot directly indicate the 'device' 
> number because 'struct snd_soc_card.num_rtd' is incremented by ALSA SoC 
> core and the number is used as an argument for 'snd_pcm_new_internal()' 
> or 'snd_pcm_new()'.
> 
> (sound/soc/soc-core.c)
> snd_soc_register_card()
> ->snd_soc_instantiate_card()
>    ->soc_bind_dai_link() (each on card->num_links/dai_link_list)
>      ->soc_new_pcm_runtime()
>      ->soc_add_pcm_runtime()
>        list_add_tail(&card->rtd_list)
>        rtd->num = card->num_rtd
>        card->num_rtd++
>    ->soc_probe_link_dais() (each on card->rtd_list)
>      (sound/soc/soc-pcm.c)
>      ->soc_new_pcm(rtd, num = rtd->num)
>        ->snd_pcm_new_internal(num)
>        ->snd_pcm_new(num)
>        struct snd_soc_pcm_runtime.pcm = pcm
>    ->snd_card_regiser()
> 
> Thus, modules are forced to 'guess' the card structure in a point of PCM
> character devices. I think you expressed this as 'virtual'.
Yes. Or fix an arbitrary PCM device value if not possible to guess.
> 
> Well, as a glance of ALSA SoC core, we have a call of 'struct 
> snd_soc_card.late_probe()' just before 'snd_card_register()'. We can 
> probably add ad-hoc control element sets for each PCM character devices.
> 
> snd_soc_register_card()
> ->snd_soc_instantiate_card()
>    ->soc_bind_dai_link()
>    ->soc_probe_link_dais()
>    ->snd_soc_add_card_controls()
>    ->struct snd_soc_card.late_probe()
>    ->snd_card_register()
> 
> But it seems to be a bit cumbersome. I think it better to add a smart 
> framework for the PCM-related controls. In this point, I agree with your 
> direction.

Another point is that, at least in theory, a DAI driver can add a
control after card creation. In this case PCM control should also be
associated to PCM device. Late probe could not handle it.

If we are aligned on direction, what is your suggestion to continue
discussion?
Do you want to continue discussion based on [RFC 1/4] ASoC: core: allow
PCM control binding to PCM device?
Other?

> 
>>>>        Furthermore, this "virtual" value has to be aligned with the one
>>>>        defined in alsa-lib conf file(s).
>>>
>>> Ditto.
>> I have not upstreamed my card conf file but an example is available at
>> end of my mail.
>> Value of the device field must be aligned with ASoC driver device values:
>> HDMI: STI-B2120.pcm.hdmi.0.hooks.0.device=0
>> SPDIF: STI-B2120.pcm.iec958.0.hooks.0.device=3
>>
>> If these controls were associated to the PCM device, I should not have
>> to fix the value but I should be able to retrieve it.
> 
> As long as I know, 'pcm/iec958.conf' and 'pcm/hdmi.conf' use different 
> strategy to handle PCM-related control element set for IEC958 type. Your 
> configuration does not work well for pcm.iec958 plugin.
> 

Please could you detail?
HDMI supports more configurations than SPDIF (e.g HBRA). Furthermore
application can retrieve HDMI sinks capability with EDID. So full agree
that strategy can be different. That's why Two IEC controls are needed.
But focusing on 'pcm/iec958.conf' and 'pcm/hdmi.conf', they are almost
identical...

Thanks and Regards

Arnaud

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Takashi Sakamoto]

On Nov 6 2016 22:43, Arnaud Pouliquen wrote:
>> Well, as a glance of ALSA SoC core, we have a call of 'struct
>> snd_soc_card.late_probe()' just before 'snd_card_register()'. We can
>> probably add ad-hoc control element sets for each PCM character devices.
>>
>> snd_soc_register_card()
>> ->snd_soc_instantiate_card()
>>    ->soc_bind_dai_link()
>>    ->soc_probe_link_dais()
>>    ->snd_soc_add_card_controls()
>>    ->struct snd_soc_card.late_probe()
>>    ->snd_card_register()
>>
>> But it seems to be a bit cumbersome. I think it better to add a smart
>> framework for the PCM-related controls. In this point, I agree with your
>> direction.
>
> Another point is that, at least in theory, a DAI driver can add a
> control after card creation. In this case PCM control should also be
> associated to PCM device. Late probe could not handle it.

I don't think so. But in your case, 'snd-soc-simple-card' is used to 
maintain own sound card instance, and this module has no handlers for 
the callback. So we don't utilize the callback for this aim unless 
integrating the module.

> If we are aligned on direction, what is your suggestion to continue
> discussion?
> Do you want to continue discussion based on [RFC 1/4] ASoC: core: allow
> PCM control binding to PCM device?
> Other?

If you focused on ALSA SoC part only, I'd not continue this discussion 
more. I'm not a developer for the part, and join in this discussion just 
for ALSA control interface.

>>> I have not upstreamed my card conf file but an example is available at
>>> end of my mail.
>>> Value of the device field must be aligned with ASoC driver device values:
>>> HDMI: STI-B2120.pcm.hdmi.0.hooks.0.device=0
>>> SPDIF: STI-B2120.pcm.iec958.0.hooks.0.device=3
>>>
>>> If these controls were associated to the PCM device, I should not have
>>> to fix the value but I should be able to retrieve it.
>>
>> As long as I know, 'pcm/iec958.conf' and 'pcm/hdmi.conf' use different
>> strategy to handle PCM-related control element set for IEC958 type. Your
>> configuration does not work well for pcm.iec958 plugin.
>>
>
> Please could you detail?
> HDMI supports more configurations than SPDIF (e.g HBRA). Furthermore
> application can retrieve HDMI sinks capability with EDID. So full agree
> that strategy can be different. That's why Two IEC controls are needed.
> But focusing on 'pcm/iec958.conf' and 'pcm/hdmi.conf', they are almost
> identical...

I prefer discuss after deciding your approach on kernel side. But in 
previous message I misunderstood that S/PDIF interface of your STI SoC 
in system side can handle IEC 60958 sub-frame. Actually, it seems not to 
be. In this case, usage of pcm hook for 'ctl_elems' type might be 
appropriate. I'm sorry to have confused you.


Regards

Takashi Sakamoto

Re: [RFC 0/4] ALSA controls management using index/device/sub-devices fields

[Arnaud Pouliquen]

>> If we are aligned on direction, what is your suggestion to continue
>> discussion?
>> Do you want to continue discussion based on [RFC 1/4] ASoC: core: allow
>> PCM control binding to PCM device?
>> Other?
> 
> If you focused on ALSA SoC part only, I'd not continue this discussion 
> more. I'm not a developer for the part, and join in this discussion just 
> for ALSA control interface.
> 

Anyway thanks for your time and your support.

Regards
Arnaud