[PATCH 0/3] ASoC: sti: correction for HBRA support

[PATCH 0/3] ASoC: sti: correction for HBRA support

[Moise Gergaud]

This patchset is V2 of "[PATCH 2/2] ASoC: sti: correction for HBRA support"

Moise Gergaud (3):
  ALSA: pcm: add IEC958 channel status update
  ASoC: sti: use pcm iec958 channel status helper
  ASoC: sti: correction for HBRA (High Bit Rate Audio) support

 include/sound/pcm_iec958.h      | 19 ++++++++-
 sound/core/pcm_iec958.c         | 78 +++++++++++++++++++++++++------------
 sound/soc/sti/Kconfig           |  1 +
 sound/soc/sti/uniperif_player.c | 86 +++++++++++++++++------------------------
 4 files changed, 107 insertions(+), 77 deletions(-)

-- 
1.9.1

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

[Moise Gergaud]

Add a helper to update only the IEC958 channel status sampling freq and
word length parameters from an ALSA snd_pcm_runtime structure, taking
account of the sample rate and sample size.

Signed-off-by: Moise Gergaud <moise.gergaud@st.com>
Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 include/sound/pcm_iec958.h | 19 ++++++++++-
 sound/core/pcm_iec958.c    | 78 +++++++++++++++++++++++++++++++---------------
 2 files changed, 71 insertions(+), 26 deletions(-)

diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
index 0eed397..0c84c69 100644
--- a/include/sound/pcm_iec958.h
+++ b/include/sound/pcm_iec958.h
@@ -3,7 +3,24 @@
 
 #include <linux/types.h>
 
+#ifdef CONFIG_SND_PCM_IEC958
 int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
-	size_t len);
+				   size_t len);
+
+int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
+				   size_t len);
+#else
+int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
+				   size_t len)
+{
+	return len;
+}
+
+int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
+				   size_t len)
+{
+	return len;
+}
+#endif
 
 #endif
diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
index 36b2d7a..abe3967 100644
--- a/sound/core/pcm_iec958.c
+++ b/sound/core/pcm_iec958.c
@@ -11,67 +11,72 @@
 #include <sound/pcm.h>
 #include <sound/pcm_iec958.h>
 
-/**
- * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
+/*
+ * snd_pcm_update_iec958_consumer - update consumer format IEC958 channel status
  * @runtime: pcm runtime structure with ->rate filled in
  * @cs: channel status buffer, at least four bytes
  * @len: length of channel status buffer
  *
- * Create the consumer format channel status data in @cs of maximum size
- * @len corresponding to the parameters of the PCM runtime @runtime.
- *
- * Drivers may wish to tweak the contents of the buffer after creation.
+ * Update sampling frequency and word length parameters of the consumer format
+ * channel status data in @cs of maximum size @len.
+ * Values correspond to the rate and format parameters of the PCM runtime
+ * @runtime.
  *
  * Returns: length of buffer, or negative error code if something failed.
  */
-int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
-	size_t len)
+int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
+				   size_t len)
 {
-	unsigned int fs, ws;
-
 	if (len < 4)
 		return -EINVAL;
 
+	cs[3] &= ~IEC958_AES3_CON_FS;
+
 	switch (runtime->rate) {
+	case 22050:
+		cs[3] |= IEC958_AES3_CON_FS_22050;
+		break;
 	case 32000:
-		fs = IEC958_AES3_CON_FS_32000;
+		cs[3] |= IEC958_AES3_CON_FS_32000;
 		break;
 	case 44100:
-		fs = IEC958_AES3_CON_FS_44100;
+		cs[3] |= IEC958_AES3_CON_FS_44100;
 		break;
 	case 48000:
-		fs = IEC958_AES3_CON_FS_48000;
+		cs[3] |= IEC958_AES3_CON_FS_48000;
 		break;
 	case 88200:
-		fs = IEC958_AES3_CON_FS_88200;
+		cs[3] |= IEC958_AES3_CON_FS_88200;
 		break;
 	case 96000:
-		fs = IEC958_AES3_CON_FS_96000;
+		cs[3] |= IEC958_AES3_CON_FS_96000;
 		break;
 	case 176400:
-		fs = IEC958_AES3_CON_FS_176400;
+		cs[3] |= IEC958_AES3_CON_FS_176400;
 		break;
 	case 192000:
-		fs = IEC958_AES3_CON_FS_192000;
+		cs[3] |= IEC958_AES3_CON_FS_192000;
 		break;
 	default:
 		return -EINVAL;
 	}
 
 	if (len > 4) {
+		cs[4] &= ~IEC958_AES4_CON_WORDLEN;
+
 		switch (snd_pcm_format_width(runtime->format)) {
 		case 16:
-			ws = IEC958_AES4_CON_WORDLEN_20_16;
+			cs[4] |= IEC958_AES4_CON_WORDLEN_20_16;
 			break;
 		case 18:
-			ws = IEC958_AES4_CON_WORDLEN_22_18;
+			cs[4] |= IEC958_AES4_CON_WORDLEN_22_18;
 			break;
 		case 20:
-			ws = IEC958_AES4_CON_WORDLEN_20_16 |
+			cs[4] |= IEC958_AES4_CON_WORDLEN_20_16 |
 			     IEC958_AES4_CON_MAX_WORDLEN_24;
 			break;
 		case 24:
-			ws = IEC958_AES4_CON_WORDLEN_24_20 |
+			cs[4] |= IEC958_AES4_CON_WORDLEN_24_20 |
 			     IEC958_AES4_CON_MAX_WORDLEN_24;
 			break;
 
@@ -80,15 +85,38 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
 		}
 	}
 
+	return len;
+}
+EXPORT_SYMBOL(snd_pcm_update_iec958_consumer);
+
+/**
+ * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
+ * @runtime: pcm runtime structure with ->rate filled in
+ * @cs: channel status buffer, at least four bytes
+ * @len: length of channel status buffer
+ *
+ * Create the consumer format channel status data in @cs of maximum size
+ * @len corresponding to the parameters of the PCM runtime @runtime.
+ *
+ * Drivers may wish to tweak the contents of the buffer after creation.
+ *
+ * Returns: length of buffer, or negative error code if something failed.
+ */
+int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
+				   size_t len)
+{
+	int ret;
+
 	memset(cs, 0, len);
 
+	ret = snd_pcm_update_iec958_consumer(runtime, cs, len);
+	if (ret < 0)
+		return ret;
+
 	cs[0] = IEC958_AES0_CON_NOT_COPYRIGHT | IEC958_AES0_CON_EMPHASIS_NONE;
 	cs[1] = IEC958_AES1_CON_GENERAL;
 	cs[2] = IEC958_AES2_CON_SOURCE_UNSPEC | IEC958_AES2_CON_CHANNEL_UNSPEC;
-	cs[3] = IEC958_AES3_CON_CLOCK_1000PPM | fs;
-
-	if (len > 4)
-		cs[4] = ws;
+	cs[3] |= IEC958_AES3_CON_CLOCK_1000PPM;
 
 	return len;
 }
-- 
1.9.1

[PATCH 2/3] ASoC: sti: use pcm iec958 channel status helper

[Moise Gergaud]

Use pcm iec958 channel status helper to update sampling freq parameter.

Signed-off-by: Moise Gergaud <moise.gergaud@st.com>
Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 sound/soc/sti/Kconfig           |  1 +
 sound/soc/sti/uniperif_player.c | 58 ++++++++---------------------------------
 2 files changed, 12 insertions(+), 47 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..b55e412 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"
 
@@ -244,6 +245,7 @@ static void uni_player_set_channel_status(struct uniperif *player,
 {
 	int n;
 	unsigned int status;
+	unsigned char *aes = player->stream_settings.iec958.status;
 
 	/*
 	 * Some AVRs and TVs require the channel status to contain a correct
@@ -251,51 +253,9 @@ static void uni_player_set_channel_status(struct uniperif *player,
 	 * set one.
 	 */
 	mutex_lock(&player->ctrl_lock);
-	if (runtime) {
-		switch (runtime->rate) {
-		case 22050:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_22050;
-			break;
-		case 44100:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_44100;
-			break;
-		case 88200:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_88200;
-			break;
-		case 176400:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_176400;
-			break;
-		case 24000:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_24000;
-			break;
-		case 48000:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_48000;
-			break;
-		case 96000:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_96000;
-			break;
-		case 192000:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_192000;
-			break;
-		case 32000:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_32000;
-			break;
-		default:
-			/* Mark as sampling frequency not indicated */
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_NOTID;
-			break;
-		}
-	}
+
+	/* update channel status sampling freq */
+	snd_pcm_update_iec958_consumer(runtime, aes, 4);
 
 	/* Audio mode:
 	 * Use audio mode status to select PCM or encoded mode
@@ -318,7 +278,7 @@ static void uni_player_set_channel_status(struct uniperif *player,
 	/* Program the new channel status */
 	for (n = 0; n < 6; ++n) {
 		status  =
-		player->stream_settings.iec958.status[0 + (n * 4)] & 0xf;
+		player->stream_settings.iec958.status[0 + (n * 4)] & 0xff;
 		status |=
 		player->stream_settings.iec958.status[1 + (n * 4)] << 8;
 		status |=
@@ -563,6 +523,7 @@ static int uni_player_ctl_iec958_get(struct snd_kcontrol *kcontrol,
 	ucontrol->value.iec958.status[1] = iec958->status[1];
 	ucontrol->value.iec958.status[2] = iec958->status[2];
 	ucontrol->value.iec958.status[3] = iec958->status[3];
+	ucontrol->value.iec958.status[4] = iec958->status[4];
 	mutex_unlock(&player->ctrl_lock);
 	return 0;
 }
@@ -574,15 +535,18 @@ static int uni_player_ctl_iec958_put(struct snd_kcontrol *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;
+	struct snd_pcm_substream *substream = player->substream;
 
 	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];
+	iec958->status[4] = ucontrol->value.iec958.status[4];
 	mutex_unlock(&player->ctrl_lock);
 
-	uni_player_set_channel_status(player, NULL);
+	if (substream && substream->runtime)
+		uni_player_set_channel_status(player, substream->runtime);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 3/3] ASoC: sti: correction for HBRA (High Bit Rate Audio) support

[Moise Gergaud]

Detect HBRA based on rate and channels used in the pcm session
In case of HBRA:
- CH_STS repeat is disabled
- channel status sampling freq is not set if already set (because it can
  be a multiple of the runtime rate)

Signed-off-by: Moise Gergaud <moise.gergaud@st.com>
Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 sound/soc/sti/uniperif_player.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
index b55e412..cf99dee 100644
--- a/sound/soc/sti/uniperif_player.c
+++ b/sound/soc/sti/uniperif_player.c
@@ -246,16 +246,39 @@ static void uni_player_set_channel_status(struct uniperif *player,
 	int n;
 	unsigned int status;
 	unsigned char *aes = player->stream_settings.iec958.status;
+	const unsigned int cs_rate[] = {
+		44100, 0, 48000, 32000, 22050, 24000,
+		88200, 768000, 176400, 192000
+	};
 
 	/*
 	 * Some AVRs and TVs require the channel status to contain a correct
-	 * sampling frequency. If no sample rate is already specified, then
-	 * set one.
+	 * sampling freq.
+	 * In case of HBRA is not detected:
+	 *   set the channel status sampling freq
+	 * In case of HBRA is detected:
+	 *   channel status sampling freq is already set; it can be a multiple
+	 *   of runtime rate
+	 *
+	 * HBRA is detected when:
+	 *   runtime channels is 8 and runtime->rate < channel status rate
 	 */
+
 	mutex_lock(&player->ctrl_lock);
 
-	/* update channel status sampling freq */
-	snd_pcm_update_iec958_consumer(runtime, aes, 4);
+	if ((runtime->channels == 8) &&
+	    (runtime->rate < cs_rate[aes[3] & IEC958_AES3_CON_FS])) {
+		/*
+		 * Consecutive frames repetition of Z preamble needs to be
+		 * disabled in case of HBRA
+		 */
+		SET_UNIPERIF_CONFIG_REPEAT_CHL_STS_DISABLE(player);
+	} else {
+		SET_UNIPERIF_CONFIG_REPEAT_CHL_STS_ENABLE(player);
+
+		/* update channel status sampling freq */
+		snd_pcm_update_iec958_consumer(runtime, aes, 4);
+	}
 
 	/* Audio mode:
 	 * Use audio mode status to select PCM or encoded mode
@@ -358,9 +381,6 @@ static int uni_player_prepare_iec958(struct uniperif *player,
 	/* Disable one-bit audio mode */
 	SET_UNIPERIF_CONFIG_ONE_BIT_AUD_DISABLE(player);
 
-	/* Enable consecutive frames repetition of Z preamble (not for HBRA) */
-	SET_UNIPERIF_CONFIG_REPEAT_CHL_STS_ENABLE(player);
-
 	/* Change to SUF0_SUBF1 and left/right channels swap! */
 	SET_UNIPERIF_CONFIG_SUBFRAME_SEL_SUBF1_SUBF0(player);
 
-- 
1.9.1

Re: [PATCH 1/3] ALSA: pcm: add IEC958 channel status update

[Takashi Iwai]

On Wed, 02 Dec 2015 15:22:04 +0100,
Moise Gergaud wrote:
> 
> Add a helper to update only the IEC958 channel status sampling freq and
> word length parameters from an ALSA snd_pcm_runtime structure, taking
> account of the sample rate and sample size.
> 
> Signed-off-by: Moise Gergaud <moise.gergaud@st.com>
> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  include/sound/pcm_iec958.h | 19 ++++++++++-
>  sound/core/pcm_iec958.c    | 78 +++++++++++++++++++++++++++++++---------------
>  2 files changed, 71 insertions(+), 26 deletions(-)
> 
> diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
> index 0eed397..0c84c69 100644
> --- a/include/sound/pcm_iec958.h
> +++ b/include/sound/pcm_iec958.h
> @@ -3,7 +3,24 @@
>  
>  #include <linux/types.h>
>  
> +#ifdef CONFIG_SND_PCM_IEC958
>  int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> -	size_t len);
> +				   size_t len);
> +
> +int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> +				   size_t len);
> +#else
> +int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> +				   size_t len)
> +{
> +	return len;
> +}
> +
> +int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> +				   size_t len)
> +{
> +	return len;
> +}

These must be static inline.
And, shouldn't they return an error?


Takashi

Re: [PATCH 2/3] ASoC: sti: use pcm iec958 channel status helper

[Takashi Iwai]

On Wed, 02 Dec 2015 15:22:05 +0100,
Moise Gergaud wrote:
> 
> Use pcm iec958 channel status helper to update sampling freq parameter.
> 
> Signed-off-by: Moise Gergaud <moise.gergaud@st.com>
> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  sound/soc/sti/Kconfig           |  1 +
>  sound/soc/sti/uniperif_player.c | 58 ++++++++---------------------------------
>  2 files changed, 12 insertions(+), 47 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..b55e412 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"
>  
> @@ -244,6 +245,7 @@ static void uni_player_set_channel_status(struct uniperif *player,
>  {
>  	int n;
>  	unsigned int status;
> +	unsigned char *aes = player->stream_settings.iec958.status;
>  
>  	/*
>  	 * Some AVRs and TVs require the channel status to contain a correct
> @@ -251,51 +253,9 @@ static void uni_player_set_channel_status(struct uniperif *player,
>  	 * set one.
>  	 */
>  	mutex_lock(&player->ctrl_lock);
> -	if (runtime) {
> -		switch (runtime->rate) {
> -		case 22050:
> -			player->stream_settings.iec958.status[3] =
> -						IEC958_AES3_CON_FS_22050;
> -			break;
> -		case 44100:
> -			player->stream_settings.iec958.status[3] =
> -						IEC958_AES3_CON_FS_44100;
> -			break;
> -		case 88200:
> -			player->stream_settings.iec958.status[3] =
> -						IEC958_AES3_CON_FS_88200;
> -			break;
> -		case 176400:
> -			player->stream_settings.iec958.status[3] =
> -						IEC958_AES3_CON_FS_176400;
> -			break;
> -		case 24000:
> -			player->stream_settings.iec958.status[3] =
> -						IEC958_AES3_CON_FS_24000;
> -			break;
> -		case 48000:
> -			player->stream_settings.iec958.status[3] =
> -						IEC958_AES3_CON_FS_48000;
> -			break;
> -		case 96000:
> -			player->stream_settings.iec958.status[3] =
> -						IEC958_AES3_CON_FS_96000;
> -			break;
> -		case 192000:
> -			player->stream_settings.iec958.status[3] =
> -						IEC958_AES3_CON_FS_192000;
> -			break;
> -		case 32000:
> -			player->stream_settings.iec958.status[3] =
> -						IEC958_AES3_CON_FS_32000;
> -			break;
> -		default:
> -			/* Mark as sampling frequency not indicated */
> -			player->stream_settings.iec958.status[3] =
> -						IEC958_AES3_CON_FS_NOTID;
> -			break;
> -		}
> -	}
> +
> +	/* update channel status sampling freq */
> +	snd_pcm_update_iec958_consumer(runtime, aes, 4);
>  
>  	/* Audio mode:
>  	 * Use audio mode status to select PCM or encoded mode

The changes until this point are indeed about the conversion with a
helper function.  However....


> @@ -318,7 +278,7 @@ static void uni_player_set_channel_status(struct uniperif *player,
>  	/* Program the new channel status */
>  	for (n = 0; n < 6; ++n) {
>  		status  =
> -		player->stream_settings.iec958.status[0 + (n * 4)] & 0xf;
> +		player->stream_settings.iec958.status[0 + (n * 4)] & 0xff;
>  		status |=
>  		player->stream_settings.iec958.status[1 + (n * 4)] << 8;
>  		status |=
> @@ -563,6 +523,7 @@ static int uni_player_ctl_iec958_get(struct snd_kcontrol *kcontrol,
>  	ucontrol->value.iec958.status[1] = iec958->status[1];
>  	ucontrol->value.iec958.status[2] = iec958->status[2];
>  	ucontrol->value.iec958.status[3] = iec958->status[3];
> +	ucontrol->value.iec958.status[4] = iec958->status[4];
>  	mutex_unlock(&player->ctrl_lock);
>  	return 0;
>  }
> @@ -574,15 +535,18 @@ static int uni_player_ctl_iec958_put(struct snd_kcontrol *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;
> +	struct snd_pcm_substream *substream = player->substream;
>  
>  	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];
> +	iec958->status[4] = ucontrol->value.iec958.status[4];
>  	mutex_unlock(&player->ctrl_lock);

These changes are basically irrelevant.  They look like real fixes,
and if it's really so, they should be split to a different patch.


Takashi

Re: [PATCH 1/3] ALSA: pcm: add IEC958 channel status update

[Russell King - ARM Linux]

On Wed, Dec 02, 2015 at 03:22:04PM +0100, Moise Gergaud wrote:
> Add a helper to update only the IEC958 channel status sampling freq and
> word length parameters from an ALSA snd_pcm_runtime structure, taking
> account of the sample rate and sample size.
> 
> Signed-off-by: Moise Gergaud <moise.gergaud@st.com>
> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

NAK.

> diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
> index 0eed397..0c84c69 100644
> --- a/include/sound/pcm_iec958.h
> +++ b/include/sound/pcm_iec958.h
> @@ -3,7 +3,24 @@
>  
>  #include <linux/types.h>
>  
> +#ifdef CONFIG_SND_PCM_IEC958
>  int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> -	size_t len);
> +				   size_t len);
> +
> +int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> +				   size_t len);
> +#else
> +int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> +				   size_t len)
> +{
> +	return len;
> +}
> +
> +int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> +				   size_t len)
> +{
> +	return len;
> +}

Firstly, this is dangerous.  If you don't have anything defined here,
then the channel status ends up being undefined.

The intention here is that if you're a user of this, then select the
config symbol.  That makes providing stubs totally unnecessary and
removes this problem.

> +#endif
>  
>  #endif
> diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
> index 36b2d7a..abe3967 100644
> --- a/sound/core/pcm_iec958.c
> +++ b/sound/core/pcm_iec958.c
> @@ -11,67 +11,72 @@
>  #include <sound/pcm.h>
>  #include <sound/pcm_iec958.h>
>  
> -/**
> - * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
> +/*
> + * snd_pcm_update_iec958_consumer - update consumer format IEC958 channel status
>   * @runtime: pcm runtime structure with ->rate filled in
>   * @cs: channel status buffer, at least four bytes
>   * @len: length of channel status buffer
>   *
> - * Create the consumer format channel status data in @cs of maximum size
> - * @len corresponding to the parameters of the PCM runtime @runtime.
> - *
> - * Drivers may wish to tweak the contents of the buffer after creation.
> + * Update sampling frequency and word length parameters of the consumer format
> + * channel status data in @cs of maximum size @len.
> + * Values correspond to the rate and format parameters of the PCM runtime
> + * @runtime.
>   *
>   * Returns: length of buffer, or negative error code if something failed.
>   */
> -int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> -	size_t len)
> +int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> +				   size_t len)
>  {
> -	unsigned int fs, ws;
> -
>  	if (len < 4)
>  		return -EINVAL;
>  
> +	cs[3] &= ~IEC958_AES3_CON_FS;
> +
>  	switch (runtime->rate) {
> +	case 22050:
> +		cs[3] |= IEC958_AES3_CON_FS_22050;
> +		break;
>  	case 32000:
> -		fs = IEC958_AES3_CON_FS_32000;
> +		cs[3] |= IEC958_AES3_CON_FS_32000;
>  		break;
>  	case 44100:
> -		fs = IEC958_AES3_CON_FS_44100;
> +		cs[3] |= IEC958_AES3_CON_FS_44100;
>  		break;
>  	case 48000:
> -		fs = IEC958_AES3_CON_FS_48000;
> +		cs[3] |= IEC958_AES3_CON_FS_48000;
>  		break;
>  	case 88200:
> -		fs = IEC958_AES3_CON_FS_88200;
> +		cs[3] |= IEC958_AES3_CON_FS_88200;
>  		break;
>  	case 96000:
> -		fs = IEC958_AES3_CON_FS_96000;
> +		cs[3] |= IEC958_AES3_CON_FS_96000;
>  		break;
>  	case 176400:
> -		fs = IEC958_AES3_CON_FS_176400;
> +		cs[3] |= IEC958_AES3_CON_FS_176400;
>  		break;
>  	case 192000:
> -		fs = IEC958_AES3_CON_FS_192000;
> +		cs[3] |= IEC958_AES3_CON_FS_192000;
>  		break;

Again, I'm not happy with this change.  The intention here is that we
validate all arguments before we change anything.  This breaks that
principle.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/3] ALSA: pcm: add IEC958 channel status update

[Russell King - ARM Linux]

On Wed, Dec 02, 2015 at 03:22:04PM +0100, Moise Gergaud wrote:
> Add a helper to update only the IEC958 channel status sampling freq and
> word length parameters from an ALSA snd_pcm_runtime structure, taking
> account of the sample rate and sample size.

What's also missing is an explanation of why this change is needed.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/3] ALSA: pcm: add IEC958 channel status update

[Moise Gergaud]

On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
> On Wed, Dec 02, 2015 at 03:22:04PM +0100, Moise Gergaud wrote:
>> Add a helper to update only the IEC958 channel status sampling freq and
>> word length parameters from an ALSA snd_pcm_runtime structure, taking
>> account of the sample rate and sample size.
>>
>> Signed-off-by: Moise Gergaud <moise.gergaud@st.com>
>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>
> NAK.
>
>> diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
>> index 0eed397..0c84c69 100644
>> --- a/include/sound/pcm_iec958.h
>> +++ b/include/sound/pcm_iec958.h
>> @@ -3,7 +3,24 @@
>>
>>   #include <linux/types.h>
>>
>> +#ifdef CONFIG_SND_PCM_IEC958
>>   int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
>> -	size_t len);
>> +				   size_t len);
>> +
>> +int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
>> +				   size_t len);
>> +#else
>> +int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
>> +				   size_t len)
>> +{
>> +	return len;
>> +}
>> +
>> +int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
>> +				   size_t len)
>> +{
>> +	return len;
>> +}
>
> Firstly, this is dangerous.  If you don't have anything defined here,
> then the channel status ends up being undefined.
>
> The intention here is that if you're a user of this, then select the
> config symbol.  That makes providing stubs totally unnecessary and
> removes this problem.

agree

>
>> +#endif
>>
>>   #endif
>> diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
>> index 36b2d7a..abe3967 100644
>> --- a/sound/core/pcm_iec958.c
>> +++ b/sound/core/pcm_iec958.c
>> @@ -11,67 +11,72 @@
>>   #include <sound/pcm.h>
>>   #include <sound/pcm_iec958.h>
>>
>> -/**
>> - * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
>> +/*
>> + * snd_pcm_update_iec958_consumer - update consumer format IEC958 channel status
>>    * @runtime: pcm runtime structure with ->rate filled in
>>    * @cs: channel status buffer, at least four bytes
>>    * @len: length of channel status buffer
>>    *
>> - * Create the consumer format channel status data in @cs of maximum size
>> - * @len corresponding to the parameters of the PCM runtime @runtime.
>> - *
>> - * Drivers may wish to tweak the contents of the buffer after creation.
>> + * Update sampling frequency and word length parameters of the consumer format
>> + * channel status data in @cs of maximum size @len.
>> + * Values correspond to the rate and format parameters of the PCM runtime
>> + * @runtime.
>>    *
>>    * Returns: length of buffer, or negative error code if something failed.
>>    */
>> -int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
>> -	size_t len)
>> +int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
>> +				   size_t len)
>>   {
>> -	unsigned int fs, ws;
>> -
>>   	if (len < 4)
>>   		return -EINVAL;
>>
>> +	cs[3] &= ~IEC958_AES3_CON_FS;
>> +
>>   	switch (runtime->rate) {
>> +	case 22050:
>> +		cs[3] |= IEC958_AES3_CON_FS_22050;
>> +		break;
>>   	case 32000:
>> -		fs = IEC958_AES3_CON_FS_32000;
>> +		cs[3] |= IEC958_AES3_CON_FS_32000;
>>   		break;
>>   	case 44100:
>> -		fs = IEC958_AES3_CON_FS_44100;
>> +		cs[3] |= IEC958_AES3_CON_FS_44100;
>>   		break;
>>   	case 48000:
>> -		fs = IEC958_AES3_CON_FS_48000;
>> +		cs[3] |= IEC958_AES3_CON_FS_48000;
>>   		break;
>>   	case 88200:
>> -		fs = IEC958_AES3_CON_FS_88200;
>> +		cs[3] |= IEC958_AES3_CON_FS_88200;
>>   		break;
>>   	case 96000:
>> -		fs = IEC958_AES3_CON_FS_96000;
>> +		cs[3] |= IEC958_AES3_CON_FS_96000;
>>   		break;
>>   	case 176400:
>> -		fs = IEC958_AES3_CON_FS_176400;
>> +		cs[3] |= IEC958_AES3_CON_FS_176400;
>>   		break;
>>   	case 192000:
>> -		fs = IEC958_AES3_CON_FS_192000;
>> +		cs[3] |= IEC958_AES3_CON_FS_192000;
>>   		break;
>
> Again, I'm not happy with this change.  The intention here is that we
> validate all arguments before we change anything.  This breaks that
> principle.
>
We propose this change for compressed mode (IEC61937/non linear PCM).
In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then 
we cannot use snd_pcm_create_iec958_consumer.
So we propose a second function that only updates the sampling freq and 
word length. Other parameters are unchanged (set by user).

In a previous patch proposal, we managed channel status sampling freq 
directly in our driver. Mark suggested us to manage it in a generic 
function (could be useful for other drivers).

Re: [PATCH 1/3] ALSA: pcm: add IEC958 channel status update

[Russell King - ARM Linux]

On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
> On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
> >Again, I'm not happy with this change.  The intention here is that we
> >validate all arguments before we change anything.  This breaks that
> >principle.
> >
> We propose this change for compressed mode (IEC61937/non linear PCM).
> In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we
> cannot use snd_pcm_create_iec958_consumer.

Why can you not use snd_pcm_create_iec958_consumer() for this case?  You
are allowed to modify the values you get afterwards in order to handle
this case if you so wish to toggle this bit.

However, if you are using data mode, you really should be using the AES
bytes passed by the application through alsalib and into the kernel via
the IEC958 controls.  There are three controls specified for this:

SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be
changed for consumer mode.
SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be
changed for professional mode.
SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable
bits.

When using this, it is userspace's responsibility to set the sample rate
appropriately for the stream.

The only case to use snd_pcm_create_iec958_consumer() is to generate the
status bytes for PCM audio, where the userspace API doesn't facilitiate
generating and/or passing the AES status bytes.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/3] ALSA: pcm: add IEC958 channel status update

[Takashi Iwai]

On Thu, 03 Dec 2015 12:13:04 +0100,
Russell King - ARM Linux wrote:
> 
> On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
> > On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
> > >Again, I'm not happy with this change.  The intention here is that we
> > >validate all arguments before we change anything.  This breaks that
> > >principle.
> > >
> > We propose this change for compressed mode (IEC61937/non linear PCM).
> > In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we
> > cannot use snd_pcm_create_iec958_consumer.
> 
> Why can you not use snd_pcm_create_iec958_consumer() for this case?  You
> are allowed to modify the values you get afterwards in order to handle
> this case if you so wish to toggle this bit.
> 
> However, if you are using data mode, you really should be using the AES
> bytes passed by the application through alsalib and into the kernel via
> the IEC958 controls.  There are three controls specified for this:
> 
> SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be
> changed for consumer mode.
> SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be
> changed for professional mode.
> SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable
> bits.
> 
> When using this, it is userspace's responsibility to set the sample rate
> appropriately for the stream.
> 
> The only case to use snd_pcm_create_iec958_consumer() is to generate the
> status bytes for PCM audio, where the userspace API doesn't facilitiate
> generating and/or passing the AES status bytes.

That's true.  OTOH, in most drivers, we try to be kind not to be too
strict for this requirement and adjust the status bits accordingly.
This comes from the lessons we learned how user-space doesn't care the
things.

So, if we want to keep that good uncle attitude, this change doesn't
look so bad to me.


thanks,

Takashi

Re: [PATCH 1/3] ALSA: pcm: add IEC958 channel status update

[Moise Gergaud]

On 12/03/2015 01:03 PM, Takashi Iwai wrote:
> On Thu, 03 Dec 2015 12:13:04 +0100,
> Russell King - ARM Linux wrote:
>>
>> On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
>>> On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
>>>> Again, I'm not happy with this change.  The intention here is that we
>>>> validate all arguments before we change anything.  This breaks that
>>>> principle.
>>>>
>>> We propose this change for compressed mode (IEC61937/non linear PCM).
>>> In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we
>>> cannot use snd_pcm_create_iec958_consumer.
>>
>> Why can you not use snd_pcm_create_iec958_consumer() for this case?  You
>> are allowed to modify the values you get afterwards in order to handle
>> this case if you so wish to toggle this bit.
>>
>> However, if you are using data mode, you really should be using the AES
>> bytes passed by the application through alsalib and into the kernel via
>> the IEC958 controls.  There are three controls specified for this:
>>
>> SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be
>> changed for consumer mode.
>> SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be
>> changed for professional mode.
>> SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable
>> bits.
>>
>> When using this, it is userspace's responsibility to set the sample rate
>> appropriately for the stream.
>>
>> The only case to use snd_pcm_create_iec958_consumer() is to generate the
>> status bytes for PCM audio, where the userspace API doesn't facilitiate
>> generating and/or passing the AES status bytes.
>
> That's true.  OTOH, in most drivers, we try to be kind not to be too
> strict for this requirement and adjust the status bits accordingly.
> This comes from the lessons we learned how user-space doesn't care the
> things.
>
> So, if we want to keep that good uncle attitude, this change doesn't
> look so bad to me.
>
shall I deliver a new version of this patch with header correction or 
shall I abandon this patch?
>
> thanks,
>
> Takashi
>

Re: [PATCH 1/3] ALSA: pcm: add IEC958 channel status update

[Takashi Iwai]

On Mon, 07 Dec 2015 09:46:02 +0100,
Moise Gergaud wrote:
> 
> On 12/03/2015 01:03 PM, Takashi Iwai wrote:
> > On Thu, 03 Dec 2015 12:13:04 +0100,
> > Russell King - ARM Linux wrote:
> >>
> >> On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
> >>> On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
> >>>> Again, I'm not happy with this change.  The intention here is that we
> >>>> validate all arguments before we change anything.  This breaks that
> >>>> principle.
> >>>>
> >>> We propose this change for compressed mode (IEC61937/non linear PCM).
> >>> In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we
> >>> cannot use snd_pcm_create_iec958_consumer.
> >>
> >> Why can you not use snd_pcm_create_iec958_consumer() for this case?  You
> >> are allowed to modify the values you get afterwards in order to handle
> >> this case if you so wish to toggle this bit.
> >>
> >> However, if you are using data mode, you really should be using the AES
> >> bytes passed by the application through alsalib and into the kernel via
> >> the IEC958 controls.  There are three controls specified for this:
> >>
> >> SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be
> >> changed for consumer mode.
> >> SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be
> >> changed for professional mode.
> >> SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable
> >> bits.
> >>
> >> When using this, it is userspace's responsibility to set the sample rate
> >> appropriately for the stream.
> >>
> >> The only case to use snd_pcm_create_iec958_consumer() is to generate the
> >> status bytes for PCM audio, where the userspace API doesn't facilitiate
> >> generating and/or passing the AES status bytes.
> >
> > That's true.  OTOH, in most drivers, we try to be kind not to be too
> > strict for this requirement and adjust the status bits accordingly.
> > This comes from the lessons we learned how user-space doesn't care the
> > things.
> >
> > So, if we want to keep that good uncle attitude, this change doesn't
> > look so bad to me.
> >
> shall I deliver a new version of this patch with header correction or 
> shall I abandon this patch?

It's up to you whether you want further convincing Russell :)
I'd happily merge this once when we get his ack.


thanks,

Takashi

Re: [PATCH 1/3] ALSA: pcm: add IEC958 channel status update

[Moise Gergaud]

On 12/07/2015 10:51 AM, Takashi Iwai wrote:
> On Mon, 07 Dec 2015 09:46:02 +0100,
> Moise Gergaud wrote:
>>
>> On 12/03/2015 01:03 PM, Takashi Iwai wrote:
>>> On Thu, 03 Dec 2015 12:13:04 +0100,
>>> Russell King - ARM Linux wrote:
>>>>
>>>> On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
>>>>> On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
>>>>>> Again, I'm not happy with this change.  The intention here is that we
>>>>>> validate all arguments before we change anything.  This breaks that
>>>>>> principle.
>>>>>>
>>>>> We propose this change for compressed mode (IEC61937/non linear PCM).
>>>>> In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we
>>>>> cannot use snd_pcm_create_iec958_consumer.
>>>>
>>>> Why can you not use snd_pcm_create_iec958_consumer() for this case?  You
>>>> are allowed to modify the values you get afterwards in order to handle
>>>> this case if you so wish to toggle this bit.
>>>>
>>>> However, if you are using data mode, you really should be using the AES
>>>> bytes passed by the application through alsalib and into the kernel via
>>>> the IEC958 controls.  There are three controls specified for this:
>>>>
>>>> SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be
>>>> changed for consumer mode.
>>>> SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be
>>>> changed for professional mode.
>>>> SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable
>>>> bits.
>>>>
>>>> When using this, it is userspace's responsibility to set the sample rate
>>>> appropriately for the stream.
>>>>
>>>> The only case to use snd_pcm_create_iec958_consumer() is to generate the
>>>> status bytes for PCM audio, where the userspace API doesn't facilitiate
>>>> generating and/or passing the AES status bytes.
>>>
>>> That's true.  OTOH, in most drivers, we try to be kind not to be too
>>> strict for this requirement and adjust the status bits accordingly.
>>> This comes from the lessons we learned how user-space doesn't care the
>>> things.
>>>
>>> So, if we want to keep that good uncle attitude, this change doesn't
>>> look so bad to me.
>>>
>> shall I deliver a new version of this patch with header correction or
>> shall I abandon this patch?
>
> It's up to you whether you want further convincing Russell :)
> I'd happily merge this once when we get his ack.
>
>
> thanks,
>
> Takashi
>

I share Takashi's view; unfortunately, we cannot always trust user space.
And so, for robustness reason, I think drivers shall ensure that channel 
status params are correct. For that purpose, the update helper function 
could be useful.

Re: [PATCH 1/3] ALSA: pcm: add IEC958 channel status update

[Russell King - ARM Linux]

On Tue, Dec 08, 2015 at 03:30:22PM +0100, Moise Gergaud wrote:
> On 12/07/2015 10:51 AM, Takashi Iwai wrote:
> >On Mon, 07 Dec 2015 09:46:02 +0100,
> >Moise Gergaud wrote:
> >>
> >>On 12/03/2015 01:03 PM, Takashi Iwai wrote:
> >>>On Thu, 03 Dec 2015 12:13:04 +0100,
> >>>Russell King - ARM Linux wrote:
> >>>>
> >>>>On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
> >>>>>On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
> >>>>>>Again, I'm not happy with this change.  The intention here is that we
> >>>>>>validate all arguments before we change anything.  This breaks that
> >>>>>>principle.
> >>>>>>
> >>>>>We propose this change for compressed mode (IEC61937/non linear PCM).
> >>>>>In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we
> >>>>>cannot use snd_pcm_create_iec958_consumer.
> >>>>
> >>>>Why can you not use snd_pcm_create_iec958_consumer() for this case?  You
> >>>>are allowed to modify the values you get afterwards in order to handle
> >>>>this case if you so wish to toggle this bit.
> >>>>
> >>>>However, if you are using data mode, you really should be using the AES
> >>>>bytes passed by the application through alsalib and into the kernel via
> >>>>the IEC958 controls.  There are three controls specified for this:
> >>>>
> >>>>SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be
> >>>>changed for consumer mode.
> >>>>SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be
> >>>>changed for professional mode.
> >>>>SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable
> >>>>bits.
> >>>>
> >>>>When using this, it is userspace's responsibility to set the sample rate
> >>>>appropriately for the stream.
> >>>>
> >>>>The only case to use snd_pcm_create_iec958_consumer() is to generate the
> >>>>status bytes for PCM audio, where the userspace API doesn't facilitiate
> >>>>generating and/or passing the AES status bytes.
> >>>
> >>>That's true.  OTOH, in most drivers, we try to be kind not to be too
> >>>strict for this requirement and adjust the status bits accordingly.
> >>>This comes from the lessons we learned how user-space doesn't care the
> >>>things.
> >>>
> >>>So, if we want to keep that good uncle attitude, this change doesn't
> >>>look so bad to me.
> >>>
> >>shall I deliver a new version of this patch with header correction or
> >>shall I abandon this patch?
> >
> >It's up to you whether you want further convincing Russell :)
> >I'd happily merge this once when we get his ack.
> >
> >
> >thanks,
> >
> >Takashi
> >
> 
> I share Takashi's view; unfortunately, we cannot always trust user space.
> And so, for robustness reason, I think drivers shall ensure that channel
> status params are correct. For that purpose, the update helper function
> could be useful.

I don't, and now that you mention about not trusting userspace, I'm
entrenching into that position.  I have hardware where userspace must get
it correct: I need to use the IEC958 alsalib plugin, and that relies on
userspace getting it right - the kernel plays no part in generating the
channel status, it's all handled in userspace.  I do have kernel-side
channel status generation for 24-bit 4-byte PCM audio though, because
it's easy to merge in the channel status information (basically doing
what the IEC958 alsalib plugin does.)  I'd much rather that was moved
out to userspace though.

What you're suggesting creates two separate classes of driver:
* those which always overwrite the channel status information from
  userspace, which will make userspace lazy and _more_ buggy.
* those which have to use the channel status information from
  userspace.

If the second is a minority, what we end up with are bugs with channel
status information being missed, because programs will work on the
"majority" of drivers which overwrite the userspace specified channel
status.

IMHO, that's really not a sane position.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 0/3] ASoC: sti: correction for HBRA support

[Moise Gergaud]

On 12/02/2015 03:22 PM, Moise Gergaud wrote:
> This patchset is V2 of "[PATCH 2/2] ASoC: sti: correction for HBRA support"
>
> Moise Gergaud (3):
>    ALSA: pcm: add IEC958 channel status update
>    ASoC: sti: use pcm iec958 channel status helper
>    ASoC: sti: correction for HBRA (High Bit Rate Audio) support
>
>   include/sound/pcm_iec958.h      | 19 ++++++++-
>   sound/core/pcm_iec958.c         | 78 +++++++++++++++++++++++++------------
>   sound/soc/sti/Kconfig           |  1 +
>   sound/soc/sti/uniperif_player.c | 86 +++++++++++++++++------------------------
>   4 files changed, 107 insertions(+), 77 deletions(-)
>

patchset abandoned