[PATCH 0/2] correction of iec958 channel status sampling freq setting

[PATCH 0/2] correction of iec958 channel status sampling freq setting

[Moise Gergaud]

patch "ASoC: sti: set iec958 channel status sampling freq" corresponds to the
V2 of "[PATCH 4/4] ASoC: sti: reset iec60958 settings on close"

Moise Gergaud (2):
  ASoC: sti: set iec958 channel status sampling freq
  ASoC: sti: correction for HBRA support

 sound/soc/sti/uniperif_player.c | 73 +++++++++++++++++++++++++----------------
 1 file changed, 44 insertions(+), 29 deletions(-)

-- 
1.9.1

[PATCH 1/2] ASoC: sti: set iec958 channel status sampling freq

[Moise Gergaud]

Previously, the iec958 channels status sampling freq was set only if not
already set. It means that it is not updated for next PCM sessions.
With this patch, we ensure the iec958 channels status sampling freq is set
to the runtime rate for each PCM session.

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

diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
index 5c2bc53..7aca6b9 100644
--- a/sound/soc/sti/uniperif_player.c
+++ b/sound/soc/sti/uniperif_player.c
@@ -251,8 +251,7 @@ static void uni_player_set_channel_status(struct uniperif *player,
 	 * set one.
 	 */
 	mutex_lock(&player->ctrl_lock);
-	if (runtime && (player->stream_settings.iec958.status[3]
-					== IEC958_AES3_CON_FS_NOTID)) {
+	if (runtime) {
 		switch (runtime->rate) {
 		case 22050:
 			player->stream_settings.iec958.status[3] =
-- 
1.9.1

[PATCH 2/2] ASoC: sti: correction for HBRA 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 | 72 +++++++++++++++++++++++++----------------
 1 file changed, 44 insertions(+), 28 deletions(-)

diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
index 7aca6b9..d6b7cea 100644
--- a/sound/soc/sti/uniperif_player.c
+++ b/sound/soc/sti/uniperif_player.c
@@ -244,55 +244,70 @@ static void uni_player_set_channel_status(struct uniperif *player,
 {
 	int n;
 	unsigned int status;
+	unsigned char *aes3 = &player->stream_settings.iec958.status[3];
+	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);
-	if (runtime) {
+
+	if ((runtime->channels == 8) &&
+	    (runtime->rate < cs_rate[*aes3 & 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);
+
+		*aes3 &= ~IEC958_AES3_CON_FS;
+
 		switch (runtime->rate) {
 		case 22050:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_22050;
+			*aes3 |= IEC958_AES3_CON_FS_22050;
 			break;
 		case 44100:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_44100;
+			*aes3 |= IEC958_AES3_CON_FS_44100;
 			break;
 		case 88200:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_88200;
+			*aes3 |= IEC958_AES3_CON_FS_88200;
 			break;
 		case 176400:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_176400;
+			*aes3 |= IEC958_AES3_CON_FS_176400;
 			break;
 		case 24000:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_24000;
+			*aes3 |= IEC958_AES3_CON_FS_24000;
 			break;
 		case 48000:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_48000;
+			*aes3 |= IEC958_AES3_CON_FS_48000;
 			break;
 		case 96000:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_96000;
+			*aes3 |= IEC958_AES3_CON_FS_96000;
 			break;
 		case 192000:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_192000;
+			*aes3 |= IEC958_AES3_CON_FS_192000;
 			break;
 		case 32000:
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_32000;
+			*aes3 |= IEC958_AES3_CON_FS_32000;
 			break;
 		default:
 			/* Mark as sampling frequency not indicated */
-			player->stream_settings.iec958.status[3] =
-						IEC958_AES3_CON_FS_NOTID;
+			*aes3 |= IEC958_AES3_CON_FS_NOTID;
 			break;
 		}
 	}
@@ -318,7 +333,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 |=
@@ -398,9 +413,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);
 
@@ -563,6 +575,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 +587,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

Applied "ASoC: sti: set iec958 channel status sampling freq" to the asoc tree

[Mark Brown]

The patch

   ASoC: sti: set iec958 channel status sampling freq

has been applied to the asoc tree at

   git://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 0d3f3c9a48d758454b0f57ca3eccd9ea3f6a4724 Mon Sep 17 00:00:00 2001
From: Moise Gergaud <moise.gergaud@st.com>
Date: Tue, 24 Nov 2015 14:16:35 +0100
Subject: [PATCH] ASoC: sti: set iec958 channel status sampling freq

Previously, the iec958 channels status sampling freq was set only if not
already set. It means that it is not updated for next PCM sessions.
With this patch, we ensure the iec958 channels status sampling freq is set
to the runtime rate for each PCM session.

Signed-off-by: Moise Gergaud <moise.gergaud@st.com>
Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/sti/uniperif_player.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
index 843f037a317d..148bcd7dbf03 100644
--- a/sound/soc/sti/uniperif_player.c
+++ b/sound/soc/sti/uniperif_player.c
@@ -251,8 +251,7 @@ static void uni_player_set_channel_status(struct uniperif *player,
 	 * set one.
 	 */
 	mutex_lock(&player->ctrl_lock);
-	if (runtime && (player->stream_settings.iec958.status[3]
-					== IEC958_AES3_CON_FS_NOTID)) {
+	if (runtime) {
 		switch (runtime->rate) {
 		case 22050:
 			player->stream_settings.iec958.status[3] =
-- 
2.6.2

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

[Mark Brown]

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

On Tue, Nov 24, 2015 at 02:16:36PM +0100, Moise Gergaud wrote:
> 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)

You might want to expand HBRA...

>  		switch (runtime->rate) {
>  		case 22050:
> -			player->stream_settings.iec958.status[3] =
> -						IEC958_AES3_CON_FS_22050;
> +			*aes3 |= IEC958_AES3_CON_FS_22050;
>  			break;

This feels like someting we could have as a generic function for other
drivers to use - can you add a core function and then call it from your
driver please?

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

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

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

[Moise Gergaud]

On 11/30/2015 05:30 PM, Mark Brown wrote:
> On Tue, Nov 24, 2015 at 02:16:36PM +0100, Moise Gergaud wrote:
>> 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)
>
> You might want to expand HBRA...
you mean in term of frequency?
>
>>   		switch (runtime->rate) {
>>   		case 22050:
>> -			player->stream_settings.iec958.status[3] =
>> -						IEC958_AES3_CON_FS_22050;
>> +			*aes3 |= IEC958_AES3_CON_FS_22050;
>>   			break;
>
> This feels like someting we could have as a generic function for other
> drivers to use - can you add a core function and then call it from your
> driver please?
>
yes, I will add something like snd_pcm_update_iec958_consumer to update 
fs and ws.

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

[Mark Brown]

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

On Tue, Dec 01, 2015 at 10:52:14AM +0100, Moise Gergaud wrote:
> On 11/30/2015 05:30 PM, Mark Brown wrote:
> >On Tue, Nov 24, 2015 at 02:16:36PM +0100, Moise Gergaud wrote:
> >>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)

> >You might want to expand HBRA...

> you mean in term of frequency?

No, I mean tell people what the acronym means.  It's probably *not*
Herefordshire Breech-Loading Rifle Association which is the first hit on
Google for me and even adding iec958 to the search term mostly just
turns up this driver.

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

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

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

[Moise Gergaud]

On 12/01/2015 12:35 PM, Mark Brown wrote:
> On Tue, Dec 01, 2015 at 10:52:14AM +0100, Moise Gergaud wrote:
>> On 11/30/2015 05:30 PM, Mark Brown wrote:
>>> On Tue, Nov 24, 2015 at 02:16:36PM +0100, Moise Gergaud wrote:
>>>> 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)
>
>>> You might want to expand HBRA...
>
>> you mean in term of frequency?
>
> No, I mean tell people what the acronym means.  It's probably *not*
> Herefordshire Breech-Loading Rifle Association which is the first hit on
> Google for me and even adding iec958 to the search term mostly just
> turns up this driver.
>

ok, sorry, I didn't get your point.
I'll expand the acronym.
HBRA = High Bit Rate Audio