[PATCH 0/3] ASoC: stm32: sai: add support of iec958 controls

[PATCH 0/3] ASoC: stm32: sai: add support of iec958 controls

[Olivier Moysan]

This patchset adds support of iec958 controls to STM32 SAI driver.

The patch makes use of iec958 control status helpers previously proposed
and discussed through the following threads:
https://patchwork.kernel.org/patch/8062601/
https://patchwork.kernel.org/patch/8091551/ (v2)
https://patchwork.kernel.org/patch/8462961/ (v3)
https://patchwork.kernel.org/patch/8533731/ (v4)

Olivier Moysan (3):
  ALSA: pcm: add IEC958 channel status control helper
  ASoC: stm32: sai: add iec958 controls support
  ASoC: dmaengine_pcm: document process callback

 include/sound/dmaengine_pcm.h |   2 +
 include/sound/pcm_iec958.h    |  19 +++++++
 sound/core/pcm_iec958.c       | 114 ++++++++++++++++++++++++++++++++++++++++++
 sound/soc/stm/Kconfig         |   1 +
 sound/soc/stm/stm32_sai_sub.c | 101 ++++++++++++++++++++++++++++++++-----
 5 files changed, 225 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH 1/3] ALSA: pcm: add IEC958 channel status control helper

[Olivier Moysan]

From: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Add IEC958 channel status helper that creates control to handle the
IEC60958 status bits.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 include/sound/pcm_iec958.h |  19 ++++++++
 sound/core/pcm_iec958.c    | 113 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+)

diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
index 0939aa45e2fe..3c9701a9b1b0 100644
--- a/include/sound/pcm_iec958.h
+++ b/include/sound/pcm_iec958.h
@@ -4,9 +4,28 @@
 
 #include <linux/types.h>
 
+/**
+ * struct snd_pcm_iec958_params: IEC 60958 controls parameters
+ * @ctrl_set: control set callback
+ * This callback is optional and shall be used to set associated driver
+ * configuration.
+ * @iec: Mandatory pointer to iec958 structure.
+ * @cs: Mandatory pointer to AES/IEC958  channel status bits.
+ * @cs_len: size in byte of the AES/IEC958  channel status bits.
+ * @private_data: Optional private pointer to driver context.
+ */
+struct snd_pcm_iec958_params {
+	int (*ctrl_set)(struct snd_pcm_iec958_params *iec_param);
+	unsigned char *cs;
+	unsigned char cs_len;
+	void *private_data;
+};
+
 int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
 	size_t len);
 
 int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
 					     u8 *cs, size_t len);
+int snd_pcm_add_iec958_ctl(struct snd_pcm *pcm, int subdevice, int stream,
+			   struct snd_pcm_iec958_params *params);
 #endif
diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
index 5e6aed64f451..aba1f522e98a 100644
--- a/sound/core/pcm_iec958.c
+++ b/sound/core/pcm_iec958.c
@@ -7,11 +7,88 @@
  */
 #include <linux/export.h>
 #include <linux/types.h>
+#include <linux/wait.h>
 #include <sound/asoundef.h>
+#include <sound/control.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/pcm_iec958.h>
 
+static int snd_pcm_iec958_info(struct snd_kcontrol *kcontrol,
+			       struct snd_ctl_elem_info *uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_IEC958;
+	uinfo->count = 1;
+	return 0;
+}
+
+/*
+ * IEC958 channel status default controls callbacks
+ */
+static int snd_pcm_iec958_get(struct snd_kcontrol *kcontrol,
+			      struct snd_ctl_elem_value *uctl)
+{
+	struct snd_pcm_iec958_params *params = snd_kcontrol_chip(kcontrol);
+	int i;
+
+	for (i = 0; i < params->cs_len; i++)
+		uctl->value.iec958.status[i] = params->cs[i];
+
+	return 0;
+}
+
+static int snd_pcm_iec958_put(struct snd_kcontrol *kcontrol,
+			      struct snd_ctl_elem_value *uctl)
+{
+	struct snd_pcm_iec958_params *params = snd_kcontrol_chip(kcontrol);
+	int err = 0;
+	unsigned int i, updated = 0;
+	unsigned char old_status[5];
+
+	for (i = 0; i < params->cs_len; i++) {
+		if (params->cs[i] != uctl->value.iec958.status[i])
+			updated = 1;
+	}
+
+	if (!updated)
+		return 0;
+
+	/* Store current status to restore them in error case */
+	for (i = 0; i < params->cs_len; i++) {
+		old_status[i] = params->cs[i];
+		params->cs[i] = uctl->value.iec958.status[i];
+	}
+
+	if (params->ctrl_set)
+		err = params->ctrl_set(params);
+	if (err < 0) {
+		for (i = 0; i < params->cs_len; i++)
+			params->cs[i] = old_status[i];
+	}
+
+	return err;
+}
+
+static const struct snd_kcontrol_new iec958_ctls[] = {
+	{
+		.access = (SNDRV_CTL_ELEM_ACCESS_READWRITE |
+			   SNDRV_CTL_ELEM_ACCESS_VOLATILE),
+		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
+		.name = SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT),
+		.info = snd_pcm_iec958_info,
+		.get = snd_pcm_iec958_get,
+		.put = snd_pcm_iec958_put,
+	},
+	{
+		.access = (SNDRV_CTL_ELEM_ACCESS_READ |
+			   SNDRV_CTL_ELEM_ACCESS_VOLATILE),
+		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
+		.name = SNDRV_CTL_NAME_IEC958("", CAPTURE, DEFAULT),
+		.info = snd_pcm_iec958_info,
+		.get = snd_pcm_iec958_get,
+	},
+};
+
 static int create_iec958_consumer(uint rate, uint sample_width,
 				  u8 *cs, size_t len)
 {
@@ -21,6 +98,9 @@ static int create_iec958_consumer(uint rate, uint sample_width,
 		return -EINVAL;
 
 	switch (rate) {
+	case 0:
+		fs = IEC958_AES3_CON_FS_NOTID;
+		break;
 	case 32000:
 		fs = IEC958_AES3_CON_FS_32000;
 		break;
@@ -48,6 +128,9 @@ static int create_iec958_consumer(uint rate, uint sample_width,
 
 	if (len > 4) {
 		switch (sample_width) {
+		case 0:
+			ws = IEC958_AES4_CON_WORDLEN_NOTID;
+			break;
 		case 16:
 			ws = IEC958_AES4_CON_WORDLEN_20_16;
 			break;
@@ -124,3 +207,33 @@ int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
 				      cs, len);
 }
 EXPORT_SYMBOL(snd_pcm_create_iec958_consumer_hw_params);
+
+/**
+ * snd_pcm_add_iec958_ctl - Add a IEC958 control associated to the pcm device
+ * @pcm: pcm device to associate to the control.
+ * @subdevice: subdevice index.Must be set to 0 if unused
+ * @iec958: snd_pcm_iec958_params structure that contains callbacks
+ *          and channel status buffer.
+ * @stream: stream type SNDRV_PCM_STREAM_PLAYBACK or SNDRV_PCM_STREAM_CATURE.
+ * Returns:  negative error code if something failed.
+ */
+int snd_pcm_add_iec958_ctl(struct snd_pcm *pcm, int subdevice, int stream,
+			   struct snd_pcm_iec958_params *params)
+{
+	struct snd_kcontrol_new knew;
+
+	if (stream > SNDRV_PCM_STREAM_LAST)
+		return -EINVAL;
+	if (!params->cs)
+		return -EINVAL;
+	if (params->cs_len < 4)
+		return -EINVAL;
+
+	create_iec958_consumer(0, 0, params->cs, params->cs_len);
+	knew = iec958_ctls[stream];
+	knew.device = pcm->device;
+	knew.subdevice = subdevice;
+
+	return snd_ctl_add(pcm->card, snd_ctl_new1(&knew, params));
+}
+EXPORT_SYMBOL(snd_pcm_add_iec958_ctl);
-- 
1.9.1

[PATCH 2/3] ASoC: stm32: sai: add iec958 controls support

[Olivier Moysan]

Add support of iec958 controls for STM32 SAI.

Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 sound/core/pcm_iec958.c       |   1 +
 sound/soc/stm/Kconfig         |   1 +
 sound/soc/stm/stm32_sai_sub.c | 101 +++++++++++++++++++++++++++++++++++++-----
 3 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
index aba1f522e98a..c34735ac3c48 100644
--- a/sound/core/pcm_iec958.c
+++ b/sound/core/pcm_iec958.c
@@ -19,6 +19,7 @@ static int snd_pcm_iec958_info(struct snd_kcontrol *kcontrol,
 {
 	uinfo->type = SNDRV_CTL_ELEM_TYPE_IEC958;
 	uinfo->count = 1;
+
 	return 0;
 }
 
diff --git a/sound/soc/stm/Kconfig b/sound/soc/stm/Kconfig
index 48f9ddd94016..9b2681397dba 100644
--- a/sound/soc/stm/Kconfig
+++ b/sound/soc/stm/Kconfig
@@ -6,6 +6,7 @@ config SND_SOC_STM32_SAI
 	depends on SND_SOC
 	select SND_SOC_GENERIC_DMAENGINE_PCM
 	select REGMAP_MMIO
+	select SND_PCM_IEC958
 	help
 	  Say Y if you want to enable SAI for STM32
 
diff --git a/sound/soc/stm/stm32_sai_sub.c b/sound/soc/stm/stm32_sai_sub.c
index cfeb219e1d78..c2e487e133aa 100644
--- a/sound/soc/stm/stm32_sai_sub.c
+++ b/sound/soc/stm/stm32_sai_sub.c
@@ -26,6 +26,7 @@
 #include <sound/asoundef.h>
 #include <sound/core.h>
 #include <sound/dmaengine_pcm.h>
+#include <sound/pcm_iec958.h>
 #include <sound/pcm_params.h>
 
 #include "stm32_sai.h"
@@ -96,7 +97,7 @@
  * @slot_mask: rx or tx active slots mask. set at init or at runtime
  * @data_size: PCM data width. corresponds to PCM substream width.
  * @spdif_frm_cnt: S/PDIF playback frame counter
- * @spdif_status_bits: S/PDIF status bits
+ * @snd_aes_iec958: iec958 data
  */
 struct stm32_sai_sub_data {
 	struct platform_device *pdev;
@@ -125,7 +126,7 @@ struct stm32_sai_sub_data {
 	int slot_mask;
 	int data_size;
 	unsigned int spdif_frm_cnt;
-	unsigned char spdif_status_bits[SAI_IEC60958_STATUS_BYTES];
+	struct snd_aes_iec958 iec958;
 };
 
 enum stm32_sai_fifo_th {
@@ -184,10 +185,6 @@ static bool stm32_sai_sub_writeable_reg(struct device *dev, unsigned int reg)
 	}
 }
 
-static const unsigned char default_status_bits[SAI_IEC60958_STATUS_BYTES] = {
-	0, 0, 0, IEC958_AES3_CON_FS_48000,
-};
-
 static const struct regmap_config stm32_sai_sub_regmap_config_f4 = {
 	.reg_bits = 32,
 	.reg_stride = 4,
@@ -619,6 +616,59 @@ static void stm32_sai_set_frame(struct snd_soc_dai *cpu_dai)
 	}
 }
 
+static void stm32_sai_set_channel_status(struct stm32_sai_sub_data *sai,
+					 struct snd_pcm_runtime *runtime)
+{
+	if (!runtime)
+		return;
+
+	/* Force the sample rate according to runtime rate */
+	switch (runtime->rate) {
+	case 22050:
+		sai->iec958.status[3] = IEC958_AES3_CON_FS_22050;
+		break;
+	case 44100:
+		sai->iec958.status[3] = IEC958_AES3_CON_FS_44100;
+		break;
+	case 88200:
+		sai->iec958.status[3] = IEC958_AES3_CON_FS_88200;
+		break;
+	case 176400:
+		sai->iec958.status[3] = IEC958_AES3_CON_FS_176400;
+		break;
+	case 24000:
+		sai->iec958.status[3] = IEC958_AES3_CON_FS_24000;
+		break;
+	case 48000:
+		sai->iec958.status[3] = IEC958_AES3_CON_FS_48000;
+		break;
+	case 96000:
+		sai->iec958.status[3] = IEC958_AES3_CON_FS_96000;
+		break;
+	case 192000:
+		sai->iec958.status[3] = IEC958_AES3_CON_FS_192000;
+		break;
+	case 32000:
+		sai->iec958.status[3] = IEC958_AES3_CON_FS_32000;
+		break;
+	default:
+		sai->iec958.status[3] = IEC958_AES3_CON_FS_NOTID;
+		break;
+	}
+}
+
+static int stm32_sai_iec958_set(struct snd_pcm_iec958_params *iec_param)
+{
+	struct stm32_sai_sub_data *sai = iec_param->private_data;
+
+	if (!sai->substream)
+		return 0;
+
+	stm32_sai_set_channel_status(sai, sai->substream->runtime);
+
+	return 0;
+}
+
 static int stm32_sai_configure_clock(struct snd_soc_dai *cpu_dai,
 				     struct snd_pcm_hw_params *params)
 {
@@ -709,7 +759,11 @@ static int stm32_sai_hw_params(struct snd_pcm_substream *substream,
 
 	sai->data_size = params_width(params);
 
-	if (!STM_SAI_PROTOCOL_IS_SPDIF(sai)) {
+	if (STM_SAI_PROTOCOL_IS_SPDIF(sai)) {
+		/* Rate not already set in runtime structure */
+		substream->runtime->rate = params_rate(params);
+		stm32_sai_set_channel_status(sai, substream->runtime);
+	} else {
 		ret = stm32_sai_set_slots(cpu_dai);
 		if (ret < 0)
 			return ret;
@@ -789,6 +843,28 @@ static void stm32_sai_shutdown(struct snd_pcm_substream *substream,
 	sai->substream = NULL;
 }
 
+static int stm32_sai_pcm_new(struct snd_soc_pcm_runtime *rtd,
+			     struct snd_soc_dai *cpu_dai)
+{
+	struct stm32_sai_sub_data *sai = dev_get_drvdata(cpu_dai->dev);
+	struct snd_pcm_iec958_params *iec_param;
+
+	if (STM_SAI_PROTOCOL_IS_SPDIF(sai)) {
+		dev_dbg(&sai->pdev->dev, "%s: register iec controls", __func__);
+		iec_param = devm_kzalloc(&sai->pdev->dev, sizeof(*iec_param),
+					 GFP_KERNEL);
+		iec_param->ctrl_set = stm32_sai_iec958_set;
+		iec_param->private_data = sai;
+		iec_param->cs = sai->iec958.status;
+		iec_param->cs_len = 5;
+		return snd_pcm_add_iec958_ctl(rtd->pcm, 0,
+					      SNDRV_PCM_STREAM_PLAYBACK,
+					      iec_param);
+	}
+
+	return 0;
+}
+
 static int stm32_sai_dai_probe(struct snd_soc_dai *cpu_dai)
 {
 	struct stm32_sai_sub_data *sai = dev_get_drvdata(cpu_dai->dev);
@@ -809,6 +885,10 @@ static int stm32_sai_dai_probe(struct snd_soc_dai *cpu_dai)
 	else
 		snd_soc_dai_init_dma_data(cpu_dai, NULL, &sai->dma_params);
 
+	/* Next settings are not relevant for spdif mode */
+	if (STM_SAI_PROTOCOL_IS_SPDIF(sai))
+		return 0;
+
 	cr1_mask = SAI_XCR1_RX_TX;
 	if (STM_SAI_IS_CAPTURE(sai))
 		cr1 |= SAI_XCR1_RX_TX;
@@ -820,10 +900,6 @@ static int stm32_sai_dai_probe(struct snd_soc_dai *cpu_dai)
 				     sai->synco, sai->synci);
 	}
 
-	if (STM_SAI_PROTOCOL_IS_SPDIF(sai))
-		memcpy(sai->spdif_status_bits, default_status_bits,
-		       sizeof(default_status_bits));
-
 	cr1_mask |= SAI_XCR1_SYNCEN_MASK;
 	cr1 |= SAI_XCR1_SYNCEN_SET(sai->sync);
 
@@ -861,7 +937,7 @@ static int stm32_sai_pcm_process_spdif(struct snd_pcm_substream *substream,
 		/* Set channel status bit */
 		byte = frm_cnt >> 3;
 		mask = 1 << (frm_cnt - (byte << 3));
-		if (sai->spdif_status_bits[byte] & mask)
+		if (sai->iec958.status[byte] & mask)
 			*ptr |= 0x04000000;
 		ptr++;
 
@@ -888,6 +964,7 @@ static int stm32_sai_pcm_process_spdif(struct snd_pcm_substream *substream,
 static struct snd_soc_dai_driver stm32_sai_playback_dai[] = {
 {
 		.probe = stm32_sai_dai_probe,
+		.pcm_new = stm32_sai_pcm_new,
 		.id = 1, /* avoid call to fmt_single_name() */
 		.playback = {
 			.channels_min = 1,
-- 
1.9.1

[PATCH 3/3] ASoC: dmaengine_pcm: document process callback

[Olivier Moysan]

Add missing description of process callback.

Fixes: 78648092ef46 ("ASoC: dmaengine_pcm: add processing support")
Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 include/sound/dmaengine_pcm.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
index 47ef486852ed..e3481eebdd98 100644
--- a/include/sound/dmaengine_pcm.h
+++ b/include/sound/dmaengine_pcm.h
@@ -118,6 +118,8 @@ void snd_dmaengine_pcm_set_config_from_dai_data(
  *   PCM substream. Will be called from the PCM drivers hwparams callback.
  * @compat_request_channel: Callback to request a DMA channel for platforms
  *   which do not use devicetree.
+ * @process: Callback used to apply processing on samples transferred from/to
+ *   user space.
  * @compat_filter_fn: Will be used as the filter function when requesting a
  *  channel for platforms which do not use devicetree. The filter parameter
  *  will be the DAI's DMA data.
-- 
1.9.1

Applied "ASoC: dmaengine_pcm: document process callback" to the asoc tree

[Mark Brown]

The patch

   ASoC: dmaengine_pcm: document process callback

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 2e78a5562ebc2e4bbdfd7f7729385b3acc94c36e Mon Sep 17 00:00:00 2001
From: Olivier Moysan <olivier.moysan@st.com>
Date: Tue, 13 Mar 2018 17:27:08 +0100
Subject: [PATCH] ASoC: dmaengine_pcm: document process callback

Add missing description of process callback.

Fixes: 78648092ef46 ("ASoC: dmaengine_pcm: add processing support")
Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/sound/dmaengine_pcm.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
index 47ef486852ed..e3481eebdd98 100644
--- a/include/sound/dmaengine_pcm.h
+++ b/include/sound/dmaengine_pcm.h
@@ -118,6 +118,8 @@ void snd_dmaengine_pcm_set_config_from_dai_data(
  *   PCM substream. Will be called from the PCM drivers hwparams callback.
  * @compat_request_channel: Callback to request a DMA channel for platforms
  *   which do not use devicetree.
+ * @process: Callback used to apply processing on samples transferred from/to
+ *   user space.
  * @compat_filter_fn: Will be used as the filter function when requesting a
  *  channel for platforms which do not use devicetree. The filter parameter
  *  will be the DAI's DMA data.
-- 
2.16.2

Re: [PATCH 0/3] ASoC: stm32: sai: add support of iec958 controls

[Olivier MOYSAN]

Hello,

I guess the blocking patch in this patchset is the patch "add IEC958 
channel status control helper". This patch has been reviewed several 
times, but did not get a ack so far.
If you think these helpers will not be merged, I will reintegrate the 
corresponding code in stm driver.
Please let me know, if I need to prepare a v2 without helpers, or if we 
can go further in the review of iec helpers patch ?

Best regards
olivier


On 03/13/2018 05:27 PM, Olivier Moysan wrote:
> This patchset adds support of iec958 controls to STM32 SAI driver.
> 
> The patch makes use of iec958 control status helpers previously proposed
> and discussed through the following threads:
> https://patchwork.kernel.org/patch/8062601/
> https://patchwork.kernel.org/patch/8091551/ (v2)
> https://patchwork.kernel.org/patch/8462961/ (v3)
> https://patchwork.kernel.org/patch/8533731/ (v4)
> 
> Olivier Moysan (3):
>    ALSA: pcm: add IEC958 channel status control helper
>    ASoC: stm32: sai: add iec958 controls support
>    ASoC: dmaengine_pcm: document process callback
> 
>   include/sound/dmaengine_pcm.h |   2 +
>   include/sound/pcm_iec958.h    |  19 +++++++
>   sound/core/pcm_iec958.c       | 114 ++++++++++++++++++++++++++++++++++++++++++
>   sound/soc/stm/Kconfig         |   1 +
>   sound/soc/stm/stm32_sai_sub.c | 101 ++++++++++++++++++++++++++++++++-----
>   5 files changed, 225 insertions(+), 12 deletions(-)
> 

Re: [PATCH 0/3] ASoC: stm32: sai: add support of iec958 controls

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On Tue, Apr 17, 2018 at 08:29:17AM +0000, Olivier MOYSAN wrote:

> I guess the blocking patch in this patchset is the patch "add IEC958 
> channel status control helper". This patch has been reviewed several 
> times, but did not get a ack so far.
> If you think these helpers will not be merged, I will reintegrate the 
> corresponding code in stm driver.
> Please let me know, if I need to prepare a v2 without helpers, or if we 
> can go further in the review of iec helpers patch ?

I don't mind either way but you're right here, I'm waiting for Takashi
to review the first patch.  I'd probably be OK with it just integrated
into the driver if we have to go that way though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] ASoC: stm32: sai: add support of iec958 controls

[Olivier MOYSAN]

Hello Takashi,

On 04/17/2018 01:17 PM, Mark Brown wrote:
> On Tue, Apr 17, 2018 at 08:29:17AM +0000, Olivier MOYSAN wrote:
> 
>> I guess the blocking patch in this patchset is the patch "add IEC958
>> channel status control helper". This patch has been reviewed several
>> times, but did not get a ack so far.
>> If you think these helpers will not be merged, I will reintegrate the
>> corresponding code in stm driver.
>> Please let me know, if I need to prepare a v2 without helpers, or if we
>> can go further in the review of iec helpers patch ?
> 
> I don't mind either way but you're right here, I'm waiting for Takashi
> to review the first patch.  I'd probably be OK with it just integrated
> into the driver if we have to go that way though.
> 

Kind reminder.
Would you have some comments on this patchset ?

Thanks
olivier

Re: [alsa-devel] [PATCH 0/3] ASoC: stm32: sai: add support of iec958 controls

[Arnaud Pouliquen]

Hi Takashi,

On 04/17/2018 01:17 PM, Mark Brown wrote:
> On Tue, Apr 17, 2018 at 08:29:17AM +0000, Olivier MOYSAN wrote:
> 
>> I guess the blocking patch in this patchset is the patch "add IEC958 
>> channel status control helper". This patch has been reviewed several 
>> times, but did not get a ack so far.
>> If you think these helpers will not be merged, I will reintegrate the 
>> corresponding code in stm driver.
>> Please let me know, if I need to prepare a v2 without helpers, or if we 
>> can go further in the review of iec helpers patch ?
> 
> I don't mind either way but you're right here, I'm waiting for Takashi
> to review the first patch.  I'd probably be OK with it just integrated
> into the driver if we have to go that way though.

Gentlemen reminder for this patch set. We would appreciate to have your
feedback on iec helper.
>From our point of view it could be useful to have a generic management
of the iec controls based on helpers instead of redefining them in DAIs.
Having the same behavior for these controls could be useful to ensure
coherence of the control management used by application (for instance
Gstreamer uses it to determine iec raw mode capability for iec61937 streams)


Thanks,
Arnaud

Re: [PATCH 2/3] ASoC: stm32: sai: add iec958 controls support

[Arnaud Pouliquen]

Hi Olivier,

On 03/13/2018 05:27 PM, Olivier MOYSAN wrote:
> Add support of iec958 controls for STM32 SAI.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
> ---
>  sound/core/pcm_iec958.c       |   1 +
>  sound/soc/stm/Kconfig         |   1 +
>  sound/soc/stm/stm32_sai_sub.c | 101
> +++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 91 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
> index aba1f522e98a..c34735ac3c48 100644
> --- a/sound/core/pcm_iec958.c
> +++ b/sound/core/pcm_iec958.c
> @@ -19,6 +19,7 @@ static int snd_pcm_iec958_info(struct snd_kcontrol
> *kcontrol,
>  {
>          uinfo->type = SNDRV_CTL_ELEM_TYPE_IEC958;
>          uinfo->count = 1;
> +
>          return 0;
>  }
Seems that this should be part of your patch 1/3

Regards,

Arnaud
>  
> diff --git a/sound/soc/stm/Kconfig b/sound/soc/stm/Kconfig
> index 48f9ddd94016..9b2681397dba 100644
> --- a/sound/soc/stm/Kconfig
> +++ b/sound/soc/stm/Kconfig
> @@ -6,6 +6,7 @@ config SND_SOC_STM32_SAI
>          depends on SND_SOC
>          select SND_SOC_GENERIC_DMAENGINE_PCM
>          select REGMAP_MMIO
> +       select SND_PCM_IEC958
>          help
>            Say Y if you want to enable SAI for STM32
>  
> diff --git a/sound/soc/stm/stm32_sai_sub.c b/sound/soc/stm/stm32_sai_sub.c
> index cfeb219e1d78..c2e487e133aa 100644
> --- a/sound/soc/stm/stm32_sai_sub.c
> +++ b/sound/soc/stm/stm32_sai_sub.c
> @@ -26,6 +26,7 @@
>  #include <sound/asoundef.h>
>  #include <sound/core.h>
>  #include <sound/dmaengine_pcm.h>
> +#include <sound/pcm_iec958.h>
>  #include <sound/pcm_params.h>
>  
>  #include "stm32_sai.h"
> @@ -96,7 +97,7 @@
>   * @slot_mask: rx or tx active slots mask. set at init or at runtime
>   * @data_size: PCM data width. corresponds to PCM substream width.
>   * @spdif_frm_cnt: S/PDIF playback frame counter
> - * @spdif_status_bits: S/PDIF status bits
> + * @snd_aes_iec958: iec958 data
>   */
>  struct stm32_sai_sub_data {
>          struct platform_device *pdev;
> @@ -125,7 +126,7 @@ struct stm32_sai_sub_data {
>          int slot_mask;
>          int data_size;
>          unsigned int spdif_frm_cnt;
> -       unsigned char spdif_status_bits[SAI_IEC60958_STATUS_BYTES];
> +       struct snd_aes_iec958 iec958;
>  };
>  
>  enum stm32_sai_fifo_th {
> @@ -184,10 +185,6 @@ static bool stm32_sai_sub_writeable_reg(struct
> device *dev, unsigned int reg)
>          }
>  }
>  
> -static const unsigned char
> default_status_bits[SAI_IEC60958_STATUS_BYTES] = {
> -       0, 0, 0, IEC958_AES3_CON_FS_48000,
> -};
> -
>  static const struct regmap_config stm32_sai_sub_regmap_config_f4 = {
>          .reg_bits = 32,
>          .reg_stride = 4,
> @@ -619,6 +616,59 @@ static void stm32_sai_set_frame(struct snd_soc_dai
> *cpu_dai)
>          }
>  }
>  
> +static void stm32_sai_set_channel_status(struct stm32_sai_sub_data *sai,
> +                                        struct snd_pcm_runtime *runtime)
> +{
> +       if (!runtime)
> +               return;
> +
> +       /* Force the sample rate according to runtime rate */
> +       switch (runtime->rate) {
> +       case 22050:
> +               sai->iec958.status[3] = IEC958_AES3_CON_FS_22050;
> +               break;
> +       case 44100:
> +               sai->iec958.status[3] = IEC958_AES3_CON_FS_44100;
> +               break;
> +       case 88200:
> +               sai->iec958.status[3] = IEC958_AES3_CON_FS_88200;
> +               break;
> +       case 176400:
> +               sai->iec958.status[3] = IEC958_AES3_CON_FS_176400;
> +               break;
> +       case 24000:
> +               sai->iec958.status[3] = IEC958_AES3_CON_FS_24000;
> +               break;
> +       case 48000:
> +               sai->iec958.status[3] = IEC958_AES3_CON_FS_48000;
> +               break;
> +       case 96000:
> +               sai->iec958.status[3] = IEC958_AES3_CON_FS_96000;
> +               break;
> +       case 192000:
> +               sai->iec958.status[3] = IEC958_AES3_CON_FS_192000;
> +               break;
> +       case 32000:
> +               sai->iec958.status[3] = IEC958_AES3_CON_FS_32000;
> +               break;
> +       default:
> +               sai->iec958.status[3] = IEC958_AES3_CON_FS_NOTID;
> +               break;
> +       }
> +}
> +
> +static int stm32_sai_iec958_set(struct snd_pcm_iec958_params *iec_param)
> +{
> +       struct stm32_sai_sub_data *sai = iec_param->private_data;
> +
> +       if (!sai->substream)
> +               return 0;
> +
> +       stm32_sai_set_channel_status(sai, sai->substream->runtime);
> +
> +       return 0;
> +}
> +
>  static int stm32_sai_configure_clock(struct snd_soc_dai *cpu_dai,
>                                       struct snd_pcm_hw_params *params)
>  {
> @@ -709,7 +759,11 @@ static int stm32_sai_hw_params(struct
> snd_pcm_substream *substream,
>  
>          sai->data_size = params_width(params);
>  
> -       if (!STM_SAI_PROTOCOL_IS_SPDIF(sai)) {
> +       if (STM_SAI_PROTOCOL_IS_SPDIF(sai)) {
> +               /* Rate not already set in runtime structure */
> +               substream->runtime->rate = params_rate(params);
> +               stm32_sai_set_channel_status(sai, substream->runtime);
> +       } else {
>                  ret = stm32_sai_set_slots(cpu_dai);
>                  if (ret < 0)
>                          return ret;
> @@ -789,6 +843,28 @@ static void stm32_sai_shutdown(struct
> snd_pcm_substream *substream,
>          sai->substream = NULL;
>  }
>  
> +static int stm32_sai_pcm_new(struct snd_soc_pcm_runtime *rtd,
> +                            struct snd_soc_dai *cpu_dai)
> +{
> +       struct stm32_sai_sub_data *sai = dev_get_drvdata(cpu_dai->dev);
> +       struct snd_pcm_iec958_params *iec_param;
> +
> +       if (STM_SAI_PROTOCOL_IS_SPDIF(sai)) {
> +               dev_dbg(&sai->pdev->dev, "%s: register iec controls",
> __func__);
> +               iec_param = devm_kzalloc(&sai->pdev->dev,
> sizeof(*iec_param),
> +                                        GFP_KERNEL);
> +               iec_param->ctrl_set = stm32_sai_iec958_set;
> +               iec_param->private_data = sai;
> +               iec_param->cs = sai->iec958.status;
> +               iec_param->cs_len = 5;
> +               return snd_pcm_add_iec958_ctl(rtd->pcm, 0,
> +                                             SNDRV_PCM_STREAM_PLAYBACK,
> +                                             iec_param);
> +       }
> +
> +       return 0;
> +}
> +
>  static int stm32_sai_dai_probe(struct snd_soc_dai *cpu_dai)
>  {
>          struct stm32_sai_sub_data *sai = dev_get_drvdata(cpu_dai->dev);
> @@ -809,6 +885,10 @@ static int stm32_sai_dai_probe(struct snd_soc_dai
> *cpu_dai)
>          else
>                  snd_soc_dai_init_dma_data(cpu_dai, NULL, &sai->dma_params);
>  
> +       /* Next settings are not relevant for spdif mode */
> +       if (STM_SAI_PROTOCOL_IS_SPDIF(sai))
> +               return 0;
> +
>          cr1_mask = SAI_XCR1_RX_TX;
>          if (STM_SAI_IS_CAPTURE(sai))
>                  cr1 |= SAI_XCR1_RX_TX;
> @@ -820,10 +900,6 @@ static int stm32_sai_dai_probe(struct snd_soc_dai
> *cpu_dai)
>                                       sai->synco, sai->synci);
>          }
>  
> -       if (STM_SAI_PROTOCOL_IS_SPDIF(sai))
> -               memcpy(sai->spdif_status_bits, default_status_bits,
> -                      sizeof(default_status_bits));
> -
>          cr1_mask |= SAI_XCR1_SYNCEN_MASK;
>          cr1 |= SAI_XCR1_SYNCEN_SET(sai->sync);
>  
> @@ -861,7 +937,7 @@ static int stm32_sai_pcm_process_spdif(struct
> snd_pcm_substream *substream,
>                  /* Set channel status bit */
>                  byte = frm_cnt >> 3;
>                  mask = 1 << (frm_cnt - (byte << 3));
> -               if (sai->spdif_status_bits[byte] & mask)
> +               if (sai->iec958.status[byte] & mask)
>                          *ptr |= 0x04000000;
>                  ptr++;
>  
> @@ -888,6 +964,7 @@ static int stm32_sai_pcm_process_spdif(struct
> snd_pcm_substream *substream,
>  static struct snd_soc_dai_driver stm32_sai_playback_dai[] = {
>  {
>                  .probe = stm32_sai_dai_probe,
> +               .pcm_new = stm32_sai_pcm_new,
>                  .id = 1, /* avoid call to fmt_single_name() */
>                  .playback = {
>                          .channels_min = 1,
> -- 
> 1.9.1
> 

Re: [alsa-devel] [PATCH 0/3] ASoC: stm32: sai: add support of iec958 controls

[Takashi Iwai]

On Tue, 05 Jun 2018 17:50:57 +0200,
Arnaud Pouliquen wrote:
> 
> Hi Takashi,
> 
> On 04/17/2018 01:17 PM, Mark Brown wrote:
> > On Tue, Apr 17, 2018 at 08:29:17AM +0000, Olivier MOYSAN wrote:
> > 
> >> I guess the blocking patch in this patchset is the patch "add IEC958 
> >> channel status control helper". This patch has been reviewed several 
> >> times, but did not get a ack so far.
> >> If you think these helpers will not be merged, I will reintegrate the 
> >> corresponding code in stm driver.
> >> Please let me know, if I need to prepare a v2 without helpers, or if we 
> >> can go further in the review of iec helpers patch ?
> > 
> > I don't mind either way but you're right here, I'm waiting for Takashi
> > to review the first patch.  I'd probably be OK with it just integrated
> > into the driver if we have to go that way though.
> 
> Gentlemen reminder for this patch set. We would appreciate to have your
> feedback on iec helper.
> From our point of view it could be useful to have a generic management
> of the iec controls based on helpers instead of redefining them in DAIs.
> Having the same behavior for these controls could be useful to ensure
> coherence of the control management used by application (for instance
> Gstreamer uses it to determine iec raw mode capability for iec61937 streams)

Oh sorry for the late reply, I've totally overlooked the thread.

And, another sorry: the patchset doesn't look convincing enough to
me.

First off, the provided API definition appears somewhat
unconventional, the mixture of the ops, the static data and the
dynamic data.

Moreover, this is only for your driver, ATM.  If it were an API that
does clean up the already existing usages, I'd happily apply it. There
are lots of drivers creating and controlling the IEC958 ctls even
now.

Also, the patchset requires more fine-tuning, in anyways.  The changes
in create_iec958_consumre() are basically irrelevant, should be split
off as an individual fix.  Also, the new function doesn't create the
"XXX Mask" controls.  And the byte comparison should be replaced with
memcmp(), etc, etc.

So, please proceed rather with the open codes for now.  If you can
provide a patch that cleans up the *multiple* driver codes, again,
I'll happily take it.  But it can be done anytime later, too.


thanks,

Takashi

Re: [alsa-devel] [PATCH 0/3] ASoC: stm32: sai: add support of iec958 controls

[Arnaud Pouliquen]

On 06/05/2018 08:29 PM, Takashi Iwai wrote:
> On Tue, 05 Jun 2018 17:50:57 +0200,
> Arnaud Pouliquen wrote:
>> 
>> Hi Takashi,
>> 
>> On 04/17/2018 01:17 PM, Mark Brown wrote:
>> > On Tue, Apr 17, 2018 at 08:29:17AM +0000, Olivier MOYSAN wrote:
>> > 
>> >> I guess the blocking patch in this patchset is the patch "add IEC958 
>> >> channel status control helper". This patch has been reviewed several 
>> >> times, but did not get a ack so far.
>> >> If you think these helpers will not be merged, I will reintegrate the 
>> >> corresponding code in stm driver.
>> >> Please let me know, if I need to prepare a v2 without helpers, or if we 
>> >> can go further in the review of iec helpers patch ?
>> > 
>> > I don't mind either way but you're right here, I'm waiting for Takashi
>> > to review the first patch.  I'd probably be OK with it just integrated
>> > into the driver if we have to go that way though.
>> 
>> Gentlemen reminder for this patch set. We would appreciate to have your
>> feedback on iec helper.
>> From our point of view it could be useful to have a generic management
>> of the iec controls based on helpers instead of redefining them in DAIs.
>> Having the same behavior for these controls could be useful to ensure
>> coherence of the control management used by application (for instance
>> Gstreamer uses it to determine iec raw mode capability for iec61937 streams)
> 
> Oh sorry for the late reply, I've totally overlooked the thread.
> 
> And, another sorry: the patchset doesn't look convincing enough to
> me.
> 
> First off, the provided API definition appears somewhat
> unconventional, the mixture of the ops, the static data and the
> dynamic data.
Sorry i can't figure out your point. I suppose that you speak about the
snd_pcm_iec958_params.
what would be a more conventional API?

> 
> Moreover, this is only for your driver, ATM.  
It is also compatible with the sound/sti driver, even if we does not
propose patch yet. We also plan to propose an implementation, for the
HDMI_codec that would need to export a control to allow none-audio mode.

>If it were an API that
> does clean up the already existing usages, I'd happily apply it. There
> are lots of drivers creating and controlling the IEC958 ctls even
> now.
> 
> Also, the patchset requires more fine-tuning, in anyways.  The changes
> in create_iec958_consumre() are basically irrelevant, should be split
> off as an individual fix.  it is linked to the control, as not possible in existing implementation
(rate and width are get from hwparam or runtime). But no problem we can
split it in a separate patch.

Also, the new function doesn't create the
> "XXX Mask" controls.  And the byte comparison should be replaced with
> memcmp(), etc, etc.
Yes mask are missing, can be added. For the rest could you comment
directly in code? i suppose that you want to replace the for loops by
memcmp, memcpy...
> 
> So, please proceed rather with the open codes for now.  If you can
> provide a patch that cleans up the *multiple* driver codes, again,
> I'll happily take it.  But it can be done anytime later, too.
Not simple to clean up the other drivers as this control is a PCM
control, that is mainly implemented as a mixer or card control.
This means that it should be registered on the pcm_new in CPU DAI or in
the DAI codec, to be able to bind it to the PCM device.
Inpact is not straigthforward as this could generate regression on driver.

For now We can add the clean up on the sti driver based on this helper,
and we are working on the HDMI_codec, we could also use this helper to
export the control....

So if you estimate that it is interesting to purchase on this helper we
can try to come back with a patch set that implements the helper for
the 3 drivers.

The other option, is that we drop the helpers, and implement the control
directly in our drivers.

Please just tell us if we should continue to propose the helpers or not.

Thanks,
Arnaud

> 
> 
> thanks,
> 
> Takashi

Re: [alsa-devel] [PATCH 0/3] ASoC: stm32: sai: add support of iec958 controls

[Takashi Iwai]

On Wed, 06 Jun 2018 11:31:45 +0200,
Arnaud Pouliquen wrote:
> 
> 
> 
> On 06/05/2018 08:29 PM, Takashi Iwai wrote:
> > On Tue, 05 Jun 2018 17:50:57 +0200,
> > Arnaud Pouliquen wrote:
> >> 
> >> Hi Takashi,
> >> 
> >> On 04/17/2018 01:17 PM, Mark Brown wrote:
> >> > On Tue, Apr 17, 2018 at 08:29:17AM +0000, Olivier MOYSAN wrote:
> >> > 
> >> >> I guess the blocking patch in this patchset is the patch "add IEC958 
> >> >> channel status control helper". This patch has been reviewed several 
> >> >> times, but did not get a ack so far.
> >> >> If you think these helpers will not be merged, I will reintegrate the 
> >> >> corresponding code in stm driver.
> >> >> Please let me know, if I need to prepare a v2 without helpers, or if we 
> >> >> can go further in the review of iec helpers patch ?
> >> > 
> >> > I don't mind either way but you're right here, I'm waiting for Takashi
> >> > to review the first patch.  I'd probably be OK with it just integrated
> >> > into the driver if we have to go that way though.
> >> 
> >> Gentlemen reminder for this patch set. We would appreciate to have your
> >> feedback on iec helper.
> >> From our point of view it could be useful to have a generic management
> >> of the iec controls based on helpers instead of redefining them in DAIs.
> >> Having the same behavior for these controls could be useful to ensure
> >> coherence of the control management used by application (for instance
> >> Gstreamer uses it to determine iec raw mode capability for iec61937 streams)
> > 
> > Oh sorry for the late reply, I've totally overlooked the thread.
> > 
> > And, another sorry: the patchset doesn't look convincing enough to
> > me.
> > 
> > First off, the provided API definition appears somewhat
> > unconventional, the mixture of the ops, the static data and the
> > dynamic data.
> Sorry i can't figure out your point. I suppose that you speak about the
> snd_pcm_iec958_params.
> what would be a more conventional API?

Imagine you'd want to put a const to the data passed to the API for
hardening.  The current struct is a mixture of static and dynamic
data.


> > Moreover, this is only for your driver, ATM.  
> It is also compatible with the sound/sti driver, even if we does not
> propose patch yet. We also plan to propose an implementation, for the
> HDMI_codec that would need to export a control to allow none-audio mode.
> 
> >If it were an API that
> > does clean up the already existing usages, I'd happily apply it. There
> > are lots of drivers creating and controlling the IEC958 ctls even
> > now.
> > 
> > Also, the patchset requires more fine-tuning, in anyways.  The changes
> > in create_iec958_consumre() are basically irrelevant, should be split
> > off as an individual fix.  it is linked to the control, as not possible in existing implementation
> (rate and width are get from hwparam or runtime). But no problem we can
> split it in a separate patch.
> 
> Also, the new function doesn't create the
> > "XXX Mask" controls.  And the byte comparison should be replaced with
> > memcmp(), etc, etc.
> Yes mask are missing, can be added. For the rest could you comment
> directly in code? i suppose that you want to replace the for loops by
> memcmp, memcpy...

Right.

> > So, please proceed rather with the open codes for now.  If you can
> > provide a patch that cleans up the *multiple* driver codes, again,
> > I'll happily take it.  But it can be done anytime later, too.
> Not simple to clean up the other drivers as this control is a PCM
> control, that is mainly implemented as a mixer or card control.
> This means that it should be registered on the pcm_new in CPU DAI or in
> the DAI codec, to be able to bind it to the PCM device.
> Inpact is not straigthforward as this could generate regression on driver.

Yes, and that's my point.  The application of API is relatively
limited -- although the API itself has nothing to do with ASoC at
all.

> For now We can add the clean up on the sti driver based on this helper,
> and we are working on the HDMI_codec, we could also use this helper to
> export the control....
> 
> So if you estimate that it is interesting to purchase on this helper we
> can try to come back with a patch set that implements the helper for
> the 3 drivers.

Right.  Basically there are two cases we add a new API:

1. It's absolutely new and nothing else can do it
2. API simplifies the whole tree, not only one you're trying to add.

And in this case, let's prove 2 at first, that the API *is* actually
useful in multiple situations we already have.  Then I'll happily ack
for that.  More drivers cleanup, better.  At best, think of more
range above ASoC, as you're proposing ALSA core API, not the ASoC
one.

> The other option, is that we drop the helpers, and implement the control
> directly in our drivers.

This is of course another, maybe easier option.

> Please just tell us if we should continue to propose the helpers or not.

I have no preference over two ways, but am only interested in the
resulting patches :)


thanks,

Takashi

Re: [alsa-devel] [PATCH 0/3] ASoC: stm32: sai: add support of iec958 controls

[Arnaud Pouliquen]

On 06/06/2018 11:47 AM, Takashi Iwai wrote:
> On Wed, 06 Jun 2018 11:31:45 +0200,
> Arnaud Pouliquen wrote:
>> 
>> 
>> 
>> On 06/05/2018 08:29 PM, Takashi Iwai wrote:
>> > On Tue, 05 Jun 2018 17:50:57 +0200,
>> > Arnaud Pouliquen wrote:
>> >> 
>> >> Hi Takashi,
>> >> 
>> >> On 04/17/2018 01:17 PM, Mark Brown wrote:
>> >> > On Tue, Apr 17, 2018 at 08:29:17AM +0000, Olivier MOYSAN wrote:
>> >> > 
>> >> >> I guess the blocking patch in this patchset is the patch "add IEC958 
>> >> >> channel status control helper". This patch has been reviewed several 
>> >> >> times, but did not get a ack so far.
>> >> >> If you think these helpers will not be merged, I will reintegrate the 
>> >> >> corresponding code in stm driver.
>> >> >> Please let me know, if I need to prepare a v2 without helpers, or if we 
>> >> >> can go further in the review of iec helpers patch ?
>> >> > 
>> >> > I don't mind either way but you're right here, I'm waiting for Takashi
>> >> > to review the first patch.  I'd probably be OK with it just integrated
>> >> > into the driver if we have to go that way though.
>> >> 
>> >> Gentlemen reminder for this patch set. We would appreciate to have your
>> >> feedback on iec helper.
>> >> From our point of view it could be useful to have a generic management
>> >> of the iec controls based on helpers instead of redefining them in DAIs.
>> >> Having the same behavior for these controls could be useful to ensure
>> >> coherence of the control management used by application (for instance
>> >> Gstreamer uses it to determine iec raw mode capability for iec61937 streams)
>> > 
>> > Oh sorry for the late reply, I've totally overlooked the thread.
>> > 
>> > And, another sorry: the patchset doesn't look convincing enough to
>> > me.
>> > 
>> > First off, the provided API definition appears somewhat
>> > unconventional, the mixture of the ops, the static data and the
>> > dynamic data.
>> Sorry i can't figure out your point. I suppose that you speak about the
>> snd_pcm_iec958_params.
>> what would be a more conventional API?
> 
> Imagine you'd want to put a const to the data passed to the API for
> hardening.  The current struct is a mixture of static and dynamic
> data.
> 
> 
>> > Moreover, this is only for your driver, ATM.  
>> It is also compatible with the sound/sti driver, even if we does not
>> propose patch yet. We also plan to propose an implementation, for the
>> HDMI_codec that would need to export a control to allow none-audio mode.
>> 
>> >If it were an API that
>> > does clean up the already existing usages, I'd happily apply it. There
>> > are lots of drivers creating and controlling the IEC958 ctls even
>> > now.
>> > 
>> > Also, the patchset requires more fine-tuning, in anyways.  The changes
>> > in create_iec958_consumre() are basically irrelevant, should be split
>> > off as an individual fix.  it is linked to the control, as not possible in existing implementation
>> (rate and width are get from hwparam or runtime). But no problem we can
>> split it in a separate patch.
>> 
>> Also, the new function doesn't create the
>> > "XXX Mask" controls.  And the byte comparison should be replaced with
>> > memcmp(), etc, etc.
>> Yes mask are missing, can be added. For the rest could you comment
>> directly in code? i suppose that you want to replace the for loops by
>> memcmp, memcpy...
> 
> Right.
> 
>> > So, please proceed rather with the open codes for now.  If you can
>> > provide a patch that cleans up the *multiple* driver codes, again,
>> > I'll happily take it.  But it can be done anytime later, too.
>> Not simple to clean up the other drivers as this control is a PCM
>> control, that is mainly implemented as a mixer or card control.
>> This means that it should be registered on the pcm_new in CPU DAI or in
>> the DAI codec, to be able to bind it to the PCM device.
>> Inpact is not straigthforward as this could generate regression on driver.
> 
> Yes, and that's my point.  The application of API is relatively
> limited -- although the API itself has nothing to do with ASoC at
> all.
> 
>> For now We can add the clean up on the sti driver based on this helper,
>> and we are working on the HDMI_codec, we could also use this helper to
>> export the control....
>> 
>> So if you estimate that it is interesting to purchase on this helper we
>> can try to come back with a patch set that implements the helper for
>> the 3 drivers.
> 
> Right.  Basically there are two cases we add a new API:
> 
> 1. It's absolutely new and nothing else can do it
> 2. API simplifies the whole tree, not only one you're trying to add.
> 
> And in this case, let's prove 2 at first, that the API *is* actually
> useful in multiple situations we already have.  Then I'll happily ack
> for that.  More drivers cleanup, better.  At best, think of more
> range above ASoC, as you're proposing ALSA core API, not the ASoC
> one.
> 
>> The other option, is that we drop the helpers, and implement the control
>> directly in our drivers.
> 
> This is of course another, maybe easier option.
> 
>> Please just tell us if we should continue to propose the helpers or not.
> 
> I have no preference over two ways, but am only interested in the
> resulting patches :)

My tentative here was to start to introduce helpers at ALSA level (not
only ASoC) to have a generic implementation of the this generic control.
Today the snd_pcm_create_iec958_consumer_hw_params just allow to fill
AES based on runtime parameters, but not to offer a generic management
of iec control.
Now you are right i'm developing under ASoC and i have not the whole
knowledge of the ALSA drivers, an probably too limited view of the iec
controls usage.

Based on your feedback i think (at least in a first step) we will choose
the easiest option for the STM driver...

Thanks
Arnaud


> 
> 
> thanks,
> 
> Takashi