[PATCH v3 0/4] add IEC958 channel status control helper

[PATCH v3 0/4] add IEC958 channel status control helper

[Arnaud Pouliquen]

- Define helpers function  to handle 'IEC958 Playback Default' control
- add "pcm_new" ops  for DAI initialisations that need pcm runtime context. 
	This patchset is needed to be able to associate control to the 
        PCM device. The iec control is linked to the DAI but also to the PCM device.
        Furthermore, some platforms like sti support both HDMI and SPDIF
        outputs in parallel. Logical way to differentiate them is to link them
        to pcm device index. 
V3:
This patchset is extracted from following RFC
http://permalink.gmane.org/gmane.linux.alsa.devel/149876 (sti: add audio interface to the hdmi driver)

Notice that "ASoC: hdmi-codec: add IEC control" patch depends on integration of the hdmi-codec driver

     Patches update:
     - ALSA: pcm: add IEC958 channel status control helper
          - make mutex usage mandatory.
          - Set control index to pcm device id ( needed to be supported by iecset)
     - ASoC: core: add code to complete dai init after pcm creation
          -  fix dai param of pcm_new  ops for cpu dai.
     - ASoC: sti: use iec channel status control helper
     		new patch: implementation of the helper for sti cpu-dai 

V2:
     - patch: ALSA: pcm: add IEC958 channel status control helper
          - Return 1 instead of 0 in snd_pcm_iec958_put
          - Add .access field in control structure
          - I have kept condition on mutex for flexibility 
            (but could be cleaned to force user to use a mutex)

V1:
This RFC is the implementation of audio HDMI on sti platform based on 
generic hdmi-codec driver:
	https://patchwork.kernel.org/patch/7215271/ ("ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders")


Arnaud Pouliquen (4):
  ALSA: pcm: add IEC958 channel status control helper
  ASoC: core: add code to complete dai init after pcm creation
  ASoC: sti: use iec channel status control helper
  ASoC: hdmi-codec: add IEC control.

 include/sound/hdmi-codec.h      |   1 +
 include/sound/pcm_iec958.h      |  15 ++++++
 include/sound/soc-dai.h         |   7 +++
 sound/core/pcm_iec958.c         | 107 ++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/hdmi-codec.c   |  59 ++++++++++++++++------
 sound/soc/soc-core.c            |  14 ++++++
 sound/soc/sti/Kconfig           |   1 +
 sound/soc/sti/uniperif_player.c |  79 ++++++++++++-----------------
 8 files changed, 219 insertions(+), 62 deletions(-)

-- 
1.9.1

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

[Arnaud Pouliquen]

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

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 include/sound/pcm_iec958.h |  15 +++++++
 sound/core/pcm_iec958.c    | 107 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+)

diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
index 0eed397..08e4bab 100644
--- a/include/sound/pcm_iec958.h
+++ b/include/sound/pcm_iec958.h
@@ -3,7 +3,22 @@
 
 #include <linux/types.h>
 
+/*
+ * IEC 60958 controls parameters
+ * Describes channel status and associated callback
+ */
+struct snd_pcm_iec958_params {
+	/* call under mutex protection, when control is updated by user */
+	int (*ctrl_set)(void *pdata, u8 *status);
+
+	struct snd_aes_iec958 *iec;
+	void *pdata; /* user private data to retrieve context */
+	struct mutex *mutex; /* use to avoid race condition */
+};
+
 int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
 	size_t len);
 
+int snd_pcm_create_iec958_ctl(struct snd_pcm *pcm,
+			      struct snd_pcm_iec958_params *params, int stream);
 #endif
diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
index 36b2d7a..80e7e47 100644
--- a/sound/core/pcm_iec958.c
+++ b/sound/core/pcm_iec958.c
@@ -7,10 +7,93 @@
  */
 #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_iec958.h>
 
+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
+ *
+ * Callbacks are protected by a mutex provided by user.
+ */
+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);
+
+	if (!params->mutex)
+		return -EINVAL;
+
+	mutex_lock(params->mutex);
+	uctl->value.iec958.status[0] = params->iec->status[0];
+	uctl->value.iec958.status[1] = params->iec->status[1];
+	uctl->value.iec958.status[2] = params->iec->status[2];
+	uctl->value.iec958.status[3] = params->iec->status[3];
+	uctl->value.iec958.status[4] = params->iec->status[4];
+	mutex_unlock(params->mutex);
+
+	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;
+
+	if (!params->mutex)
+		return -EINVAL;
+
+	mutex_lock(params->mutex);
+	if (params->ctrl_set)
+		err = params->ctrl_set(params->pdata,
+				       uctl->value.iec958.status);
+	if (err < 0) {
+		mutex_unlock(params->mutex);
+		return err;
+	}
+
+	params->iec->status[0] = uctl->value.iec958.status[0];
+	params->iec->status[1] = uctl->value.iec958.status[1];
+	params->iec->status[2] = uctl->value.iec958.status[2];
+	params->iec->status[3] = uctl->value.iec958.status[3];
+	params->iec->status[4] = uctl->value.iec958.status[4];
+
+	mutex_unlock(params->mutex);
+
+	return 1;
+}
+
+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,
+	},
+};
+
 /**
  * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
  * @runtime: pcm runtime structure with ->rate filled in
@@ -93,3 +176,27 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
 	return len;
 }
 EXPORT_SYMBOL(snd_pcm_create_iec958_consumer);
+
+/**
+ * snd_pcm_create_iec958_ctl - create IEC958 channel status default control
+ * pcm: pcm device to associate to the control.
+ * 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_create_iec958_ctl(struct snd_pcm *pcm,
+			      struct snd_pcm_iec958_params *params, int stream)
+{
+	struct snd_kcontrol_new knew;
+
+	if (stream > SNDRV_PCM_STREAM_LAST)
+		return -EINVAL;
+
+	knew = iec958_ctls[stream];
+	knew.device = pcm->device;
+	knew.index = pcm->device;
+	knew.count = pcm->streams[stream].substream_count;
+	return snd_ctl_add(pcm->card, snd_ctl_new1(&knew, params));
+}
+EXPORT_SYMBOL(snd_pcm_create_iec958_ctl);
-- 
1.9.1

[PATCH v3 2/4] ASoC: core: add code to complete dai init after pcm creation

[Arnaud Pouliquen]

Some Controls defined in DAI need to be associated to PCM device (e.g. IEC60958).

This allows to perform post initialization in DAI after PCM device creation.

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

diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 964b7de..6969c83 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -205,6 +205,13 @@ struct snd_soc_dai_ops {
 	 */
 	snd_pcm_sframes_t (*delay)(struct snd_pcm_substream *,
 		struct snd_soc_dai *);
+
+	/*
+	 * function called by soc_probe_link_dais to post initialize DAI
+	 * after pcm device creation.
+	 * As example, can be used to link a controls to the pcm device
+	 */
+	int (*pcm_new)(struct snd_soc_pcm_runtime *, struct snd_soc_dai *);
 };
 
 /*
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 790ee2b..3899ce7 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1587,6 +1587,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
 {
 	struct snd_soc_dai_link *dai_link = rtd->dai_link;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	const struct snd_soc_dai_ops *ops;
 	int i, ret;
 
 	dev_dbg(card->dev, "ASoC: probe %s dai link %d late %d\n",
@@ -1662,6 +1663,19 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
 		}
 	}
 
+	for (i = 0; i < rtd->num_codecs; i++) {
+		ops = rtd->codec_dais[i]->driver->ops;
+		if (ops->pcm_new)
+			ret = ops->pcm_new(rtd, rtd->codec_dais[i]);
+		if (ret)
+			return ret;
+	}
+	ops = cpu_dai->driver->ops;
+	if (ops->pcm_new)
+		ret = ops->pcm_new(rtd, cpu_dai);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
-- 
1.9.1

[PATCH v3 3/4] ASoC: sti: use iec channel status control helper

[Arnaud Pouliquen]

Use helper function instead of internal function for iec control

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 sound/soc/sti/Kconfig           |  1 +
 sound/soc/sti/uniperif_player.c | 79 ++++++++++++++++-------------------------
 2 files changed, 32 insertions(+), 48 deletions(-)

diff --git a/sound/soc/sti/Kconfig b/sound/soc/sti/Kconfig
index 64a6900..8e616a4 100644
--- a/sound/soc/sti/Kconfig
+++ b/sound/soc/sti/Kconfig
@@ -6,6 +6,7 @@ menuconfig SND_SOC_STI
 	depends on SND_SOC
 	depends on ARCH_STI || COMPILE_TEST
 	select SND_SOC_GENERIC_DMAENGINE_PCM
+	select SND_PCM_IEC958
 	help
 		Say Y if you want to enable ASoC-support for
 		any of the STI platforms (e.g. STIH416).
diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
index 7aca6b9..8b8697b 100644
--- a/sound/soc/sti/uniperif_player.c
+++ b/sound/soc/sti/uniperif_player.c
@@ -12,6 +12,7 @@
 
 #include <sound/asoundef.h>
 #include <sound/soc.h>
+#include <sound/pcm_iec958.h>
 
 #include "uniperif.h"
 
@@ -250,7 +251,6 @@ static void uni_player_set_channel_status(struct uniperif *player,
 	 * sampling frequency. If no sample rate is already specified, then
 	 * set one.
 	 */
-	mutex_lock(&player->ctrl_lock);
 	if (runtime) {
 		switch (runtime->rate) {
 		case 22050:
@@ -327,7 +327,6 @@ static void uni_player_set_channel_status(struct uniperif *player,
 		player->stream_settings.iec958.status[3 + (n * 4)] << 24;
 		SET_UNIPERIF_CHANNEL_STA_REGN(player, n, status);
 	}
-	mutex_unlock(&player->ctrl_lock);
 
 	/* Update the channel status */
 	if (player->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0)
@@ -390,7 +389,9 @@ static int uni_player_prepare_iec958(struct uniperif *player,
 	SET_UNIPERIF_CTRL_ZERO_STUFF_HW(player);
 
 	/* Update the channel status */
+	mutex_lock(&player->ctrl_lock);
 	uni_player_set_channel_status(player, runtime);
+	mutex_unlock(&player->ctrl_lock);
 
 	/* Clear the user validity user bits */
 	SET_UNIPERIF_USER_VALIDITY_VALIDITY_LR(player, 0);
@@ -541,60 +542,18 @@ static int uni_player_prepare_pcm(struct uniperif *player,
 /*
  * ALSA uniperipheral iec958 controls
  */
-static int  uni_player_ctl_iec958_info(struct snd_kcontrol *kcontrol,
-				       struct snd_ctl_elem_info *uinfo)
-{
-	uinfo->type = SNDRV_CTL_ELEM_TYPE_IEC958;
-	uinfo->count = 1;
-
-	return 0;
-}
 
-static int uni_player_ctl_iec958_get(struct snd_kcontrol *kcontrol,
-				     struct snd_ctl_elem_value *ucontrol)
+static int uni_player_ctl_iec958_set(void *pdata, u8 *status)
 {
-	struct snd_soc_dai *dai = snd_kcontrol_chip(kcontrol);
+	struct snd_soc_dai *dai = pdata;
 	struct sti_uniperiph_data *priv = snd_soc_dai_get_drvdata(dai);
 	struct uniperif *player = priv->dai_data.uni;
-	struct snd_aes_iec958 *iec958 = &player->stream_settings.iec958;
-
-	mutex_lock(&player->ctrl_lock);
-	ucontrol->value.iec958.status[0] = iec958->status[0];
-	ucontrol->value.iec958.status[1] = iec958->status[1];
-	ucontrol->value.iec958.status[2] = iec958->status[2];
-	ucontrol->value.iec958.status[3] = iec958->status[3];
-	mutex_unlock(&player->ctrl_lock);
-	return 0;
-}
-
-static int uni_player_ctl_iec958_put(struct snd_kcontrol *kcontrol,
-				     struct snd_ctl_elem_value *ucontrol)
-{
-	struct snd_soc_dai *dai = snd_kcontrol_chip(kcontrol);
-	struct sti_uniperiph_data *priv = snd_soc_dai_get_drvdata(dai);
-	struct uniperif *player = priv->dai_data.uni;
-	struct snd_aes_iec958 *iec958 =  &player->stream_settings.iec958;
-
-	mutex_lock(&player->ctrl_lock);
-	iec958->status[0] = ucontrol->value.iec958.status[0];
-	iec958->status[1] = ucontrol->value.iec958.status[1];
-	iec958->status[2] = ucontrol->value.iec958.status[2];
-	iec958->status[3] = ucontrol->value.iec958.status[3];
-	mutex_unlock(&player->ctrl_lock);
 
 	uni_player_set_channel_status(player, NULL);
 
 	return 0;
 }
 
-static struct snd_kcontrol_new uni_player_iec958_ctl = {
-	.iface = SNDRV_CTL_ELEM_IFACE_PCM,
-	.name = SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT),
-	.info = uni_player_ctl_iec958_info,
-	.get = uni_player_ctl_iec958_get,
-	.put = uni_player_ctl_iec958_put,
-};
-
 /*
  * uniperif rate adjustement control
  */
@@ -659,7 +618,6 @@ static struct snd_kcontrol_new *snd_sti_pcm_ctl[] = {
 };
 
 static struct snd_kcontrol_new *snd_sti_iec_ctl[] = {
-	&uni_player_iec958_ctl,
 	&uni_player_clk_adj_ctl,
 };
 
@@ -1030,6 +988,30 @@ static int uni_player_parse_dt(struct platform_device *pdev,
 	return 0;
 }
 
+static int uni_player_pcm_new(struct snd_soc_pcm_runtime *rtd,
+			      struct snd_soc_dai *dai)
+{
+	struct sti_uniperiph_data *priv = snd_soc_dai_get_drvdata(dai);
+	struct uniperif *player = priv->dai_data.uni;
+	struct snd_pcm_iec958_params *params;
+
+	if (!UNIPERIF_PLAYER_TYPE_IS_IEC958(player))
+		return 0;
+
+	params = devm_kzalloc(dai->dev, sizeof(*params), GFP_KERNEL);
+	if (!params)
+		return -ENOMEM;
+
+	params->iec = &player->stream_settings.iec958;
+	params->pdata = player;
+	params->mutex = &player->ctrl_lock;
+	params->pdata = dai;
+	params->ctrl_set = uni_player_ctl_iec958_set;
+
+	return snd_pcm_create_iec958_ctl(rtd->pcm, params,
+					 SNDRV_PCM_STREAM_PLAYBACK);
+}
+
 static const struct snd_soc_dai_ops uni_player_dai_ops = {
 		.startup = uni_player_startup,
 		.shutdown = uni_player_shutdown,
@@ -1037,7 +1019,8 @@ static const struct snd_soc_dai_ops uni_player_dai_ops = {
 		.trigger = uni_player_trigger,
 		.hw_params = sti_uniperiph_dai_hw_params,
 		.set_fmt = sti_uniperiph_dai_set_fmt,
-		.set_sysclk = uni_player_set_sysclk
+		.set_sysclk = uni_player_set_sysclk,
+		.pcm_new = uni_player_pcm_new,
 };
 
 int uni_player_init(struct platform_device *pdev,
-- 
1.9.1

[PATCH v3 4/4] ASoC: hdmi-codec: add IEC control.

[Arnaud Pouliquen]

Create 'IEC958 Playback Default' controls to support IEC61937 formats.
the use of the alsa control is optional, using 'iec_ctl' flag.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 include/sound/hdmi-codec.h    |  1 +
 sound/soc/codecs/hdmi-codec.c | 57 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
index fc3a481..4366d9f 100644
--- a/include/sound/hdmi-codec.h
+++ b/include/sound/hdmi-codec.h
@@ -92,6 +92,7 @@ struct hdmi_codec_pdata {
 	const struct hdmi_codec_ops *ops;
 	uint i2s:1;
 	uint spdif:1;
+	uint iec_ctl:1;
 	int max_i2s_channels;
 };
 
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index bc47b9a..29d30e5 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -32,6 +32,7 @@ struct hdmi_codec_priv {
 	struct snd_pcm_substream *current_stream;
 	struct snd_pcm_hw_constraint_list ratec;
 	uint8_t eld[MAX_ELD_BYTES];
+	struct snd_aes_iec958 iec;
 };
 
 static const struct snd_soc_dapm_widget hdmi_widgets[] = {
@@ -122,26 +123,29 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
 				struct snd_soc_dai *dai)
 {
 	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
-	struct hdmi_codec_params hp = {
-		.iec = {
-			.status = { 0 },
-			.subcode = { 0 },
-			.pad = 0,
-			.dig_subframe = { 0 },
-		}
-	};
+	struct hdmi_codec_params hp;
 	int ret;
 
 	dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
 		params_width(params), params_rate(params),
 		params_channels(params));
 
-	ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status,
-						       sizeof(hp.iec.status));
-	if (ret < 0) {
-		dev_err(dai->dev, "Creating IEC958 channel status failed %d\n",
-			ret);
-		return ret;
+	mutex_lock(&hcp->current_stream_lock);
+	hp.iec = hcp->iec;
+	mutex_unlock(&hcp->current_stream_lock);
+
+	if (!hcp->hcd.iec_ctl) {
+		/*
+		* only PCM format supported
+		*channel status set according to runtime parameters
+		*/
+		ret = snd_pcm_create_iec958_consumer_hw_params(params,
+					hp.iec.status, sizeof(hp.iec.status));
+		if (ret < 0) {
+			dev_err(dai->dev, "Creating IEC958 status failed %d\n",
+				ret);
+			return ret;
+		}
 	}
 
 	ret = hdmi_codec_new_stream(substream, dai);
@@ -248,12 +252,37 @@ static int hdmi_codec_digital_mute(struct snd_soc_dai *dai, int mute)
 	return 0;
 }
 
+static int hdmi_codec_pcm_new(struct snd_soc_pcm_runtime *rtd,
+			      struct snd_soc_dai *dai)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+	struct snd_pcm_iec958_params *iec958_params;
+
+	dev_dbg(dai->dev, "%s()\n", __func__);
+
+	if (!hcp->hcd.iec_ctl)
+		return 0;
+
+	iec958_params = devm_kzalloc(dai->dev, sizeof(*iec958_params),
+				     GFP_KERNEL);
+	if (!iec958_params)
+		return -ENOMEM;
+
+	iec958_params->iec = &hcp->iec;
+	iec958_params->pdata = hcp;
+	iec958_params->mutex = &hcp->current_stream_lock;
+
+	return snd_pcm_create_iec958_ctl(rtd->pcm, iec958_params,
+					 SNDRV_PCM_STREAM_PLAYBACK);
+}
+
 static const struct snd_soc_dai_ops hdmi_dai_ops = {
 	.startup	= hdmi_codec_startup,
 	.shutdown	= hdmi_codec_shutdown,
 	.hw_params	= hdmi_codec_hw_params,
 	.set_fmt	= hdmi_codec_set_fmt,
 	.digital_mute	= hdmi_codec_digital_mute,
+	.pcm_new        = hdmi_codec_pcm_new,
 };
 
 
-- 
1.9.1

Re: [PATCH v3 1/4] ALSA: pcm: add IEC958 channel status control helper

[Takashi Iwai]

On Tue, 01 Mar 2016 09:19:14 +0100,
Arnaud Pouliquen wrote:
> 
> Add IEC958 channel status helper that creates control to handle the
> IEC60958 status bits.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

What is the reason to make the mutex pointer instead of the own one?
Any need for sharing the mutex?

And, if it has to be assigned explicitly by user, you have to write it
explicitly, too.

Another small nitpicking:

> ---
>  include/sound/pcm_iec958.h |  15 +++++++
>  sound/core/pcm_iec958.c    | 107 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 122 insertions(+)
> 
> diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
> index 0eed397..08e4bab 100644
> --- a/include/sound/pcm_iec958.h
> +++ b/include/sound/pcm_iec958.h
> @@ -3,7 +3,22 @@
>  
>  #include <linux/types.h>
>  
> +/*
> + * IEC 60958 controls parameters
> + * Describes channel status and associated callback
> + */
> +struct snd_pcm_iec958_params {
> +	/* call under mutex protection, when control is updated by user */
> +	int (*ctrl_set)(void *pdata, u8 *status);
> +
> +	struct snd_aes_iec958 *iec;
> +	void *pdata; /* user private data to retrieve context */
> +	struct mutex *mutex; /* use to avoid race condition */
> +};
> +
>  int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
>  	size_t len);
>  
> +int snd_pcm_create_iec958_ctl(struct snd_pcm *pcm,
> +			      struct snd_pcm_iec958_params *params, int stream);
>  #endif
> diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
> index 36b2d7a..80e7e47 100644
> --- a/sound/core/pcm_iec958.c
> +++ b/sound/core/pcm_iec958.c
> @@ -7,10 +7,93 @@
>   */
>  #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_iec958.h>
>  
> +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;
> +}

No static?


> +
> +/**
> + * IEC958 channel status default controls callbacks
> + *
> + * Callbacks are protected by a mutex provided by user.
> + */

You don't need the kernel-doc comment "/**" for static functions, in
general.  It's basically for API, not for internal ones.

> +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);
> +
> +	if (!params->mutex)
> +		return -EINVAL;
> +
> +	mutex_lock(params->mutex);
> +	uctl->value.iec958.status[0] = params->iec->status[0];
> +	uctl->value.iec958.status[1] = params->iec->status[1];
> +	uctl->value.iec958.status[2] = params->iec->status[2];
> +	uctl->value.iec958.status[3] = params->iec->status[3];
> +	uctl->value.iec958.status[4] = params->iec->status[4];

A loop might be better :)  Let compiler optimize it.

> +	mutex_unlock(params->mutex);
> +
> +	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;
> +
> +	if (!params->mutex)
> +		return -EINVAL;
> +
> +	mutex_lock(params->mutex);
> +	if (params->ctrl_set)
> +		err = params->ctrl_set(params->pdata,
> +				       uctl->value.iec958.status);

So, in your design, ctrl_set isn't mandatory?

> +	if (err < 0) {
> +		mutex_unlock(params->mutex);
> +		return err;
> +	}
> +
> +	params->iec->status[0] = uctl->value.iec958.status[0];
> +	params->iec->status[1] = uctl->value.iec958.status[1];
> +	params->iec->status[2] = uctl->value.iec958.status[2];
> +	params->iec->status[3] = uctl->value.iec958.status[3];
> +	params->iec->status[4] = uctl->value.iec958.status[4];
> +
> +	mutex_unlock(params->mutex);
> +
> +	return 1;

Strictly speaking, the return value of put callback should be zero if
the value isn't changed.  This will avoid the ctl value change
notification.


> +}
> +
> +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,
> +	},
> +};
> +
>  /**
>   * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
>   * @runtime: pcm runtime structure with ->rate filled in
> @@ -93,3 +176,27 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
>  	return len;
>  }
>  EXPORT_SYMBOL(snd_pcm_create_iec958_consumer);
> +
> +/**
> + * snd_pcm_create_iec958_ctl - create IEC958 channel status default control
> + * pcm: pcm device to associate to the control.
> + * 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

Put "@" prefix for arguments in kernel-doc.

> + * Returns:  negative error code if something failed.
> + */
> +int snd_pcm_create_iec958_ctl(struct snd_pcm *pcm,
> +			      struct snd_pcm_iec958_params *params, int stream)
> +{
> +	struct snd_kcontrol_new knew;
> +
> +	if (stream > SNDRV_PCM_STREAM_LAST)
> +		return -EINVAL;
> +
> +	knew = iec958_ctls[stream];
> +	knew.device = pcm->device;
> +	knew.index = pcm->device;
> +	knew.count = pcm->streams[stream].substream_count;

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

I admit that the current ctl <-> PCM mapping is messing.  There are
some heuristics and you're trying to follow that.  But blindly
applying to all cases doesn't seem to work.


thanks,

Takashi

Re: [PATCH v3 1/4] ALSA: pcm: add IEC958 channel status control helper

[Arnaud Pouliquen]

On 03/01/2016 10:12 AM, Takashi Iwai wrote:
> On Tue, 01 Mar 2016 09:19:14 +0100,
> Arnaud Pouliquen wrote:
>>
>> Add IEC958 channel status helper that creates control to handle the
>> IEC60958 status bits.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> 
> What is the reason to make the mutex pointer instead of the own one?
> Any need for sharing the mutex?
Yes, need to share mutex to avoid race condition between control update
and action on pcm stream ( hw_param or prepare)
> 
> And, if it has to be assigned explicitly by user, you have to write it
> explicitly, too.
ok, if not enough explicit, i will add comment in snd_pcm_iec958_params
definition
> 
> Another small nitpicking:

>> +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;
>> +
>> +	if (!params->mutex)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(params->mutex);
>> +	if (params->ctrl_set)
>> +		err = params->ctrl_set(params->pdata,
>> +				       uctl->value.iec958.status);
> 
> So, in your design, ctrl_set isn't mandatory?
Hypothesis is that for some hardwares, callback is not
needed, only channels status values are needed. As example
some hardwares could not want to support switch from audio to none-audio
mode without stopping PCM.

> 
>> +int snd_pcm_create_iec958_ctl(struct snd_pcm *pcm,
>> +			      struct snd_pcm_iec958_params *params, int stream)
>> +{
>> +	struct snd_kcontrol_new knew;
>> +
>> +	if (stream > SNDRV_PCM_STREAM_LAST)
>> +		return -EINVAL;
>> +
>> +	knew = iec958_ctls[stream];
>> +	knew.device = pcm->device;
>> +	knew.index = pcm->device;
>> +	knew.count = pcm->streams[stream].substream_count;
> 
> Hmm, this doesn't always work.  It will create the substream_count
> ctls starting from the pcm dev# as index.  What if there are 2 PCM
> devices where both contain 4 substreams?
> 
> I admit that the current ctl <-> PCM mapping is messing.  There are
> some heuristics and you're trying to follow that.  But blindly
> applying to all cases doesn't seem to work.
> 
yes this is not clean.
i'm not very familiar with substream usage, so any suggestion is welcome.
The only use case I have in mind is a HDMI connected through 4  I2S data
wire...
I can see 2 options:
- Associate control only to pcm device.
- Create a control per sub-device

Do we really need to associate one IEC control per substream?

Thanks
Arnaud

Re: [PATCH v3 1/4] ALSA: pcm: add IEC958 channel status control helper

[Takashi Iwai]

On Tue, 01 Mar 2016 11:46:54 +0100,
Arnaud Pouliquen wrote:
> 
> 
> 
> On 03/01/2016 10:12 AM, Takashi Iwai wrote:
> > On Tue, 01 Mar 2016 09:19:14 +0100,
> > Arnaud Pouliquen wrote:
> >>
> >> Add IEC958 channel status helper that creates control to handle the
> >> IEC60958 status bits.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> > 
> > What is the reason to make the mutex pointer instead of the own one?
> > Any need for sharing the mutex?
> Yes, need to share mutex to avoid race condition between control update
> and action on pcm stream ( hw_param or prepare)

Hrm....  I don't know whether this is in a good form.  At least, it's
a big confusing to me.  In general, a mandatory mutex is usually
assigned to its own, not referring to an external one.

> > And, if it has to be assigned explicitly by user, you have to write it
> > explicitly, too.
> ok, if not enough explicit, i will add comment in snd_pcm_iec958_params
> definition
> > 
> > Another small nitpicking:
> 
> >> +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;
> >> +
> >> +	if (!params->mutex)
> >> +		return -EINVAL;
> >> +
> >> +	mutex_lock(params->mutex);
> >> +	if (params->ctrl_set)
> >> +		err = params->ctrl_set(params->pdata,
> >> +				       uctl->value.iec958.status);
> > 
> > So, in your design, ctrl_set isn't mandatory?
> Hypothesis is that for some hardwares, callback is not
> needed, only channels status values are needed. As example
> some hardwares could not want to support switch from audio to none-audio
> mode without stopping PCM.

OK, fair enough.  Then put the comment that this callback is
optional.

> >> +int snd_pcm_create_iec958_ctl(struct snd_pcm *pcm,
> >> +			      struct snd_pcm_iec958_params *params, int stream)
> >> +{
> >> +	struct snd_kcontrol_new knew;
> >> +
> >> +	if (stream > SNDRV_PCM_STREAM_LAST)
> >> +		return -EINVAL;
> >> +
> >> +	knew = iec958_ctls[stream];
> >> +	knew.device = pcm->device;
> >> +	knew.index = pcm->device;
> >> +	knew.count = pcm->streams[stream].substream_count;
> > 
> > Hmm, this doesn't always work.  It will create the substream_count
> > ctls starting from the pcm dev# as index.  What if there are 2 PCM
> > devices where both contain 4 substreams?
> > 
> > I admit that the current ctl <-> PCM mapping is messing.  There are
> > some heuristics and you're trying to follow that.  But blindly
> > applying to all cases doesn't seem to work.
> > 
> yes this is not clean.
> i'm not very familiar with substream usage, so any suggestion is welcome.
> The only use case I have in mind is a HDMI connected through 4  I2S data
> wire...
> I can see 2 options:
> - Associate control only to pcm device.
> - Create a control per sub-device
> 
> Do we really need to associate one IEC control per substream?

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

I think you can pass the decision to the caller side: instead of
defining it in the function there, give via arguments.


Takashi

Re: [PATCH v3 1/4] ALSA: pcm: add IEC958 channel status control helper

[Arnaud Pouliquen]

On 03/01/2016 11:57 AM, Takashi Iwai wrote:
> On Tue, 01 Mar 2016 11:46:54 +0100,
> Arnaud Pouliquen wrote:
>>
>>
>>
>> On 03/01/2016 10:12 AM, Takashi Iwai wrote:
>>> On Tue, 01 Mar 2016 09:19:14 +0100,
>>> Arnaud Pouliquen wrote:
>>>>
>>>> Add IEC958 channel status helper that creates control to handle the
>>>> IEC60958 status bits.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>>>
>>> What is the reason to make the mutex pointer instead of the own one?
>>> Any need for sharing the mutex?
>> Yes, need to share mutex to avoid race condition between control update
>> and action on pcm stream ( hw_param or prepare)
> 
> Hrm....  I don't know whether this is in a good form.  At least, it's
> a big confusing to me.  In general, a mandatory mutex is usually
> assigned to its own, not referring to an external one.
> 
Severals drivers that use this control use a mutex (e.g ac97_codec.c).
So need to shared it between driver and the help function.
In my first version mutex was optional (used if not null). I have made
it mandatory after discussions with Russel.
Forcing driver to use it to avoid race condition make also sense...
Now,I have no fixed idea on it, just need a consensus.


>>> And, if it has to be assigned explicitly by user, you have to write it
>>> explicitly, too.
>> ok, if not enough explicit, i will add comment in snd_pcm_iec958_params
>> definition
>>>
>>> Another small nitpicking:
>>
>>>> +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;
>>>> +
>>>> +	if (!params->mutex)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(params->mutex);
>>>> +	if (params->ctrl_set)
>>>> +		err = params->ctrl_set(params->pdata,
>>>> +				       uctl->value.iec958.status);
>>>
>>> So, in your design, ctrl_set isn't mandatory?
>> Hypothesis is that for some hardwares, callback is not
>> needed, only channels status values are needed. As example
>> some hardwares could not want to support switch from audio to none-audio
>> mode without stopping PCM.
> 
> OK, fair enough.  Then put the comment that this callback is
> optional.
> 
>>>> +int snd_pcm_create_iec958_ctl(struct snd_pcm *pcm,
>>>> +			      struct snd_pcm_iec958_params *params, int stream)
>>>> +{
>>>> +	struct snd_kcontrol_new knew;
>>>> +
>>>> +	if (stream > SNDRV_PCM_STREAM_LAST)
>>>> +		return -EINVAL;
>>>> +
>>>> +	knew = iec958_ctls[stream];
>>>> +	knew.device = pcm->device;
>>>> +	knew.index = pcm->device;
>>>> +	knew.count = pcm->streams[stream].substream_count;
>>>
>>> Hmm, this doesn't always work.  It will create the substream_count
>>> ctls starting from the pcm dev# as index.  What if there are 2 PCM
>>> devices where both contain 4 substreams?
>>>
>>> I admit that the current ctl <-> PCM mapping is messing.  There are
>>> some heuristics and you're trying to follow that.  But blindly
>>> applying to all cases doesn't seem to work.
>>>
>> yes this is not clean.
>> i'm not very familiar with substream usage, so any suggestion is welcome.
>> The only use case I have in mind is a HDMI connected through 4  I2S data
>> wire...
>> I can see 2 options:
>> - Associate control only to pcm device.
>> - Create a control per sub-device
>>
>> Do we really need to associate one IEC control per substream?
> 
> This pretty much depends on the hardware design.  If each substream is
> really individual, you'd need to give the control for each substream.
> 
> I think you can pass the decision to the caller side: instead of
> defining it in the function there, give via arguments.
Ok, one argument to determine if control has to be associated to device
or sub-devices is sufficient?
or should i define one argument per sub device?

Thanks
Arnaud

Re: [PATCH v3 2/4] ASoC: core: add code to complete dai init after pcm creation

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 551 bytes --]

On Tue, Mar 01, 2016 at 09:19:15AM +0100, Arnaud Pouliquen wrote:
> Some Controls defined in DAI need to be associated to PCM device (e.g. IEC60958).
> 
> This allows to perform post initialization in DAI after PCM device creation.

Rather than add an explicit callback and make drivers open code this
I'd prefer to see us either just move the control creation entirely (I
can't immediately think of a particular need for the current ordering)
or add a data based mechanism like we currently have.  Why do we need to
do this via a callback?

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v3 2/4] ASoC: core: add code to complete dai init after pcm creation

[Arnaud Pouliquen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 03/02/2016 05:03 AM, Mark Brown wrote:
> On Tue, Mar 01, 2016 at 09:19:15AM +0100, Arnaud Pouliquen wrote:
>> Some Controls defined in DAI need to be associated to PCM device 
>> (e.g. IEC60958).
>> 
>> This allows to perform post initialization in DAI after PCM 
>> device creation.
> 
> Rather than add an explicit callback and make drivers open code 
> this I'd prefer to see us either just move the control creation 
> entirely (I can't immediately think of a particular need for the 
> current ordering) or add a data based mechanism like we currently 
> have.  Why do we need to do this via a callback?
> 
For time being I identify only a need to link controls to pcm device.
So no other justification to implement the pcm_new callback.

I implemented the pcm_new callback because straightforward way to
handle generic iec958 control proposed in
[PATCH v3 1/4] ALSA: pcm: add IEC958 channel status control helper

Here is an alternative but with higher impact in soc-core:
1)Create 2 helper functions in soc-core:
 - snd_soc_add_dai_pcm_controls:
 	=>register a control that needs to be linked to pcm device.
	 if dai probed(pcm device exist) => link control to pcm device
	 else postpone link creation in soc_probe_link_dais
 -  snd_soc_add_dai_iec_control
	same but based on snd_pcm_create_iec958_ctrl

2) add 2 news fields in snd-soc-dai struct
struct snd_kcontrol *pcm_kctl /* list control that will be linked to
pcm device*/
struct snd_pcm_iec958_params *params /* if not null iec control is
created */

Abandon helper function for iec control should simplify implementation
but that also makes sense to have helper for this generic control...

Is is sometime that should be more reasonable?

Thanks
Arnaud
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)

iQIcBAEBAgAGBQJW1rM1AAoJEP0ZQ+DAfqbfTnYP/3ILQb0WXjIEel9u40UcT5/z
eN6kZSurKaNJq0kX3XpORa7USBct0jxXdgaQEsxMMZxLcc2nkJuxOMgHjtEb/EDj
337mRT+TPG/pdg2e/AH+rafNsUSSgZfuyl1LudLmgxasEVk2cTEJTmz287LmNK9O
CSp5U8gs0zGnKiUcEdqgSHLdJYNxJJ87kD7i3d510fx1koYjDPxfgxUnhqIKiqTP
S9mEC6y0UqD7RfTVBPdYTHk8qGtJjrPvhuR1I8HiEfa/YUonFvvxxP7ec5ic56cv
GQ0PYRLhUkH+45OIoB46rfvu2yCNtFbTqBC64QIV8x7FhbI8gLqkMAzvtmyKE2uk
sZ9ZWri9HYpo9QNmVfFyqHoR7UcXKI4XmSZ7eVAcAPFlTf9K3dnPYpNPOeJ+lUoH
bAj0I+Rq/rpPVEJnhggSqQPeDk6jyFffxZqIx4A3UEIuwgNtIv2QlWzQ7FhTwuGJ
FQM94xqfIT5V7e75FPUkPC5ktTCcXFhg7ClLk3frP4kjXzFKg5BcvRsfT1ZyHSd2
hYRwKIqIp8nsgiM5LYO3ujAz3UJDDtbP9dPE4G+foXokvQWsFl15lh2+4Knm1SNv
jZ3MXL/PDaTQydTQ8Hs+jL9Xclpk/wFyIgDI9JYHiA1htuWnCvVF6GZCxBBup2jC
R1wZ8q8kjN6JFjyaWgYl
=gmEu
-----END PGP SIGNATURE-----

Re: [PATCH v3 2/4] ASoC: core: add code to complete dai init after pcm creation

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1017 bytes --]

On Wed, Mar 02, 2016 at 10:32:37AM +0100, Arnaud Pouliquen wrote:

> Here is an alternative but with higher impact in soc-core:
> 1)Create 2 helper functions in soc-core:
>  - snd_soc_add_dai_pcm_controls:
>  	=>register a control that needs to be linked to pcm device.
> 	 if dai probed(pcm device exist) => link control to pcm device
> 	 else postpone link creation in soc_probe_link_dais
>  -  snd_soc_add_dai_iec_control
> 	same but based on snd_pcm_create_iec958_ctrl

The whole point is to avoid functions...

> 2) add 2 news fields in snd-soc-dai struct
> struct snd_kcontrol *pcm_kctl /* list control that will be linked to
> pcm device*/
> struct snd_pcm_iec958_params *params /* if not null iec control is
> created */

> Abandon helper function for iec control should simplify implementation
> but that also makes sense to have helper for this generic control...

This seems better though I'm confused about what exactly these would be,
I've not looked at the first patch since it seemed to have problems.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]