[alsa-devel] [PATCH 0/3] ASoC: stm32: sai: add support of iec958 controls
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(-)
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);
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,
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.
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.
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(-)
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.
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
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
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
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
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
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
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:
- It's absolutely new and nothing else can do it
- 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
participants (4)
-
Arnaud Pouliquen -
Mark Brown -
Olivier Moysan -
Takashi Iwai