[PATCH 0/4] ASoC sti: corrections

[PATCH 0/4] ASoC sti: corrections

[Moise Gergaud]

Hello,
this patchset cleans obsolete code and corrects wrong behaviours.

regards
Moïse


Moise Gergaud (4):
  ASoC: sti: remove wrong error message
  ASoC: sti: rename ST proprietary DT properties
  ASoC: sti: set player private data
  ASoC: sti: reset iec60958 settings on close

 sound/soc/sti/uniperif_player.c | 50 ++++++++++++++++++++++++-----------------
 sound/soc/sti/uniperif_reader.c |  3 +--
 2 files changed, 31 insertions(+), 22 deletions(-)

-- 
1.9.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 1/4] ASoC: sti: remove wrong error message

[Moise Gergaud]

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

diff --git a/sound/soc/sti/uniperif_reader.c b/sound/soc/sti/uniperif_reader.c
index f791239..819eeaf 100644
--- a/sound/soc/sti/uniperif_reader.c
+++ b/sound/soc/sti/uniperif_reader.c
@@ -346,7 +346,6 @@ int uni_reader_init(struct platform_device *pdev,
 	reader->hw = &uni_reader_pcm_hw;
 	reader->dai_ops = &uni_reader_dai_ops;
 
-	dev_err(reader->dev, "%s: enter\n", __func__);
 	ret = uni_reader_parse_dt(pdev, reader);
 	if (ret < 0) {
 		dev_err(reader->dev, "Failed to parse DeviceTree");
-- 
1.9.1

[PATCH 2/4] ASoC: sti: rename ST proprietary DT properties

[Moise Gergaud]

"st," prefix has been added for ST proprietary DT properties.

Signed-off-by: Moise Gergaud <moise.gergaud@st.com>
Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 sound/soc/sti/uniperif_player.c | 6 +++---
 sound/soc/sti/uniperif_reader.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
index 843f037..1e19a7c 100644
--- a/sound/soc/sti/uniperif_player.c
+++ b/sound/soc/sti/uniperif_player.c
@@ -989,7 +989,7 @@ static int uni_player_parse_dt(struct platform_device *pdev,
 	if (!info)
 		return -ENOMEM;
 
-	if (of_property_read_u32(pnode, "version", &player->ver) ||
+	if (of_property_read_u32(pnode, "st,version", &player->ver) ||
 	    player->ver == SND_ST_UNIPERIF_VERSION_UNKNOWN) {
 		dev_err(dev, "Unknown uniperipheral version ");
 		return -EINVAL;
@@ -998,13 +998,13 @@ static int uni_player_parse_dt(struct platform_device *pdev,
 	if (player->ver >= SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0)
 		info->underflow_enabled = 1;
 
-	if (of_property_read_u32(pnode, "uniperiph-id", &info->id)) {
+	if (of_property_read_u32(pnode, "st,uniperiph-id", &info->id)) {
 		dev_err(dev, "uniperipheral id not defined");
 		return -EINVAL;
 	}
 
 	/* Read the device mode property */
-	if (of_property_read_string(pnode, "mode", &mode)) {
+	if (of_property_read_string(pnode, "st,mode", &mode)) {
 		dev_err(dev, "uniperipheral mode not defined");
 		return -EINVAL;
 	}
diff --git a/sound/soc/sti/uniperif_reader.c b/sound/soc/sti/uniperif_reader.c
index 819eeaf..8a0eb20 100644
--- a/sound/soc/sti/uniperif_reader.c
+++ b/sound/soc/sti/uniperif_reader.c
@@ -316,7 +316,7 @@ static int uni_reader_parse_dt(struct platform_device *pdev,
 	if (!info)
 		return -ENOMEM;
 
-	if (of_property_read_u32(node, "version", &reader->ver) ||
+	if (of_property_read_u32(node, "st,version", &reader->ver) ||
 	    reader->ver == SND_ST_UNIPERIF_VERSION_UNKNOWN) {
 		dev_err(&pdev->dev, "Unknown uniperipheral version ");
 		return -EINVAL;
-- 
1.9.1

[PATCH 3/4] ASoC: sti: set player private data

[Moise Gergaud]

Set substream player private data.
substream player private data is used in uni_player_irq_handler to lock,
stop & unlock the stream when interrupt indicates underflow/overflow.
If not set, then segmentation fault occurs.

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, 3 insertions(+)

diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
index 1e19a7c..5c2bc53 100644
--- a/sound/soc/sti/uniperif_player.c
+++ b/sound/soc/sti/uniperif_player.c
@@ -669,6 +669,7 @@ static int uni_player_startup(struct snd_pcm_substream *substream,
 {
 	struct sti_uniperiph_data *priv = snd_soc_dai_get_drvdata(dai);
 	struct uniperif *player = priv->dai_data.uni;
+	player->substream = substream;
 
 	player->clk_adj = 0;
 
@@ -950,6 +951,8 @@ static void uni_player_shutdown(struct snd_pcm_substream *substream,
 	if (player->state != UNIPERIF_STATE_STOPPED)
 		/* Stop the player */
 		uni_player_stop(player);
+
+	player->substream = NULL;
 }
 
 static int uni_player_parse_dt_clk_glue(struct platform_device *pdev,
-- 
1.9.1

[PATCH 4/4] ASoC: sti: reset iec60958 settings on close

[Moise Gergaud]

Reset IEC 60958 settings for next 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 | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
index 5c2bc53..b73e348 100644
--- a/sound/soc/sti/uniperif_player.c
+++ b/sound/soc/sti/uniperif_player.c
@@ -64,6 +64,23 @@ static const struct snd_pcm_hardware uni_player_pcm_hw = {
 	.buffer_bytes_max = 256 * PAGE_SIZE
 };
 
+static inline void reset_iec958_settings(struct uniperif *player)
+{
+	struct snd_aes_iec958 *iec958 = &player->stream_settings.iec958;
+
+	memset(iec958->status, 0, sizeof(iec958->status));
+
+	/* Broadcast reception category */
+	iec958->status[1] = IEC958_AES1_CON_GENERAL;
+	/* Do not take into account source or channel number */
+	iec958->status[2] = IEC958_AES2_CON_SOURCE_UNSPEC;
+	/* Sampling frequency not indicated */
+	iec958->status[3] = IEC958_AES3_CON_FS_NOTID;
+	/* Max sample word 24-bit, sample word length not indicated */
+	iec958->status[4] = IEC958_AES4_CON_MAX_WORDLEN_24 |
+			    IEC958_AES4_CON_WORDLEN_24_20;
+}
+
 static inline int reset_player(struct uniperif *player)
 {
 	int count = 10;
@@ -947,6 +964,11 @@ static void uni_player_shutdown(struct snd_pcm_substream *substream,
 {
 	struct sti_uniperiph_data *priv = snd_soc_dai_get_drvdata(dai);
 	struct uniperif *player = priv->dai_data.uni;
+	/*
+	 * Set default iec958 status bits done in close to allow to set
+	 * iec settings before next open pcm session
+	 */
+	reset_iec958_settings(player);
 
 	if (player->state != UNIPERIF_STATE_STOPPED)
 		/* Stop the player */
@@ -1089,23 +1111,8 @@ int uni_player_init(struct platform_device *pdev,
 	SET_UNIPERIF_CONFIG_IDLE_MOD_DISABLE(player);
 
 	if (UNIPERIF_PLAYER_TYPE_IS_IEC958(player)) {
-		/* Set default iec958 status bits  */
-
-		/* Consumer, PCM, copyright, 2ch, mode 0 */
-		player->stream_settings.iec958.status[0] = 0x00;
-		/* Broadcast reception category */
-		player->stream_settings.iec958.status[1] =
-					IEC958_AES1_CON_GENERAL;
-		/* Do not take into account source or channel number */
-		player->stream_settings.iec958.status[2] =
-					IEC958_AES2_CON_SOURCE_UNSPEC;
-		/* Sampling frequency not indicated */
-		player->stream_settings.iec958.status[3] =
-					IEC958_AES3_CON_FS_NOTID;
-		/* Max sample word 24-bit, sample word length not indicated */
-		player->stream_settings.iec958.status[4] =
-					IEC958_AES4_CON_MAX_WORDLEN_24 |
-					IEC958_AES4_CON_WORDLEN_24_20;
+		/* Set default iec958 status bits */
+		reset_iec958_settings(player);
 
 		player->num_ctrls = ARRAY_SIZE(snd_sti_iec_ctl);
 		player->snd_ctrls = snd_sti_iec_ctl[0];
-- 
1.9.1

Re: [PATCH 4/4] ASoC: sti: reset iec60958 settings on close

[Mark Brown]

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

On Thu, Nov 19, 2015 at 02:54:10PM +0100, Moise Gergaud wrote:
> Reset IEC 60958 settings for next PCM session.
> 
> Signed-off-by: Moise Gergaud <moise.gergaud@st.com>
> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

It's not 100% clear that we want to do this - normally controls are
persistent and don't reset themselves per session.  Is this something we
normally do for such controls?

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

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

Applied "ASoC: sti: set player private data" to the asoc tree

[Mark Brown]

The patch

   ASoC: sti: set player private data

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 36a65e2072625556191c6c616d65ed4f67f4f0d0 Mon Sep 17 00:00:00 2001
From: Moise Gergaud <moise.gergaud@st.com>
Date: Thu, 19 Nov 2015 14:54:09 +0100
Subject: [PATCH] ASoC: sti: set player private data

Set substream player private data.
substream player private data is used in uni_player_irq_handler to lock,
stop & unlock the stream when interrupt indicates underflow/overflow.
If not set, then segmentation fault occurs.

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, 3 insertions(+)

diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
index 1e19a7c6b7e8..5c2bc53f0a9b 100644
--- a/sound/soc/sti/uniperif_player.c
+++ b/sound/soc/sti/uniperif_player.c
@@ -669,6 +669,7 @@ static int uni_player_startup(struct snd_pcm_substream *substream,
 {
 	struct sti_uniperiph_data *priv = snd_soc_dai_get_drvdata(dai);
 	struct uniperif *player = priv->dai_data.uni;
+	player->substream = substream;
 
 	player->clk_adj = 0;
 
@@ -950,6 +951,8 @@ static void uni_player_shutdown(struct snd_pcm_substream *substream,
 	if (player->state != UNIPERIF_STATE_STOPPED)
 		/* Stop the player */
 		uni_player_stop(player);
+
+	player->substream = NULL;
 }
 
 static int uni_player_parse_dt_clk_glue(struct platform_device *pdev,
-- 
2.6.2

Applied "ASoC: sti: rename ST proprietary DT properties" to the asoc tree

[Mark Brown]

The patch

   ASoC: sti: rename ST proprietary DT properties

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 f9f51973d3a8559731a228e91ac29792b43046a5 Mon Sep 17 00:00:00 2001
From: Moise Gergaud <moise.gergaud@st.com>
Date: Thu, 19 Nov 2015 14:54:08 +0100
Subject: [PATCH] ASoC: sti: rename ST proprietary DT properties

"st," prefix has been added for ST proprietary DT properties.

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 | 6 +++---
 sound/soc/sti/uniperif_reader.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c
index 843f037a317d..1e19a7c6b7e8 100644
--- a/sound/soc/sti/uniperif_player.c
+++ b/sound/soc/sti/uniperif_player.c
@@ -989,7 +989,7 @@ static int uni_player_parse_dt(struct platform_device *pdev,
 	if (!info)
 		return -ENOMEM;
 
-	if (of_property_read_u32(pnode, "version", &player->ver) ||
+	if (of_property_read_u32(pnode, "st,version", &player->ver) ||
 	    player->ver == SND_ST_UNIPERIF_VERSION_UNKNOWN) {
 		dev_err(dev, "Unknown uniperipheral version ");
 		return -EINVAL;
@@ -998,13 +998,13 @@ static int uni_player_parse_dt(struct platform_device *pdev,
 	if (player->ver >= SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0)
 		info->underflow_enabled = 1;
 
-	if (of_property_read_u32(pnode, "uniperiph-id", &info->id)) {
+	if (of_property_read_u32(pnode, "st,uniperiph-id", &info->id)) {
 		dev_err(dev, "uniperipheral id not defined");
 		return -EINVAL;
 	}
 
 	/* Read the device mode property */
-	if (of_property_read_string(pnode, "mode", &mode)) {
+	if (of_property_read_string(pnode, "st,mode", &mode)) {
 		dev_err(dev, "uniperipheral mode not defined");
 		return -EINVAL;
 	}
diff --git a/sound/soc/sti/uniperif_reader.c b/sound/soc/sti/uniperif_reader.c
index 819eeafdf6b4..8a0eb2050169 100644
--- a/sound/soc/sti/uniperif_reader.c
+++ b/sound/soc/sti/uniperif_reader.c
@@ -316,7 +316,7 @@ static int uni_reader_parse_dt(struct platform_device *pdev,
 	if (!info)
 		return -ENOMEM;
 
-	if (of_property_read_u32(node, "version", &reader->ver) ||
+	if (of_property_read_u32(node, "st,version", &reader->ver) ||
 	    reader->ver == SND_ST_UNIPERIF_VERSION_UNKNOWN) {
 		dev_err(&pdev->dev, "Unknown uniperipheral version ");
 		return -EINVAL;
-- 
2.6.2

Applied "ASoC: sti: remove wrong error message" to the asoc tree

[Mark Brown]

The patch

   ASoC: sti: remove wrong error message

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 cd3ed08a86e8b5022f107aa72a1929b6417c1f42 Mon Sep 17 00:00:00 2001
From: Moise Gergaud <moise.gergaud@st.com>
Date: Thu, 19 Nov 2015 14:54:07 +0100
Subject: [PATCH] ASoC: sti: remove wrong error message

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_reader.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/soc/sti/uniperif_reader.c b/sound/soc/sti/uniperif_reader.c
index f791239a3087..819eeafdf6b4 100644
--- a/sound/soc/sti/uniperif_reader.c
+++ b/sound/soc/sti/uniperif_reader.c
@@ -346,7 +346,6 @@ int uni_reader_init(struct platform_device *pdev,
 	reader->hw = &uni_reader_pcm_hw;
 	reader->dai_ops = &uni_reader_dai_ops;
 
-	dev_err(reader->dev, "%s: enter\n", __func__);
 	ret = uni_reader_parse_dt(pdev, reader);
 	if (ret < 0) {
 		dev_err(reader->dev, "Failed to parse DeviceTree");
-- 
2.6.2

Re: [PATCH 4/4] ASoC: sti: reset iec60958 settings on close

[Moise Gergaud]

Hello,
To be compliant with SPDIF & HDMI-1.4 by using aplay, driver needs to 
set the channel status sampling freq = runtime rate; because channel 
status sampling freq is not set by aplay.
For HBRA, the application set the channel status sampling freq (that is 
different than the runtime rate).
=> by taking into account the 2 above cases, for each pcm session, 
driver shall be able to detect if the channel status sampling freq has 
already been set and set it if needed.

And also for robustness purpose: in case the channel status sampling 
freq is not set by the application, I think the driver shall set it.

Maybe I can limit my patch by resetting only the channel status sampling 
freq on close (actual patch reset all the fields of the channel status).

regards
Moïse


On 11/19/2015 06:50 PM, Mark Brown wrote:
> On Thu, Nov 19, 2015 at 02:54:10PM +0100, Moise Gergaud wrote:
>> Reset IEC 60958 settings for next PCM session.
>>
>> Signed-off-by: Moise Gergaud <moise.gergaud@st.com>
>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>
> It's not 100% clear that we want to do this - normally controls are
> persistent and don't reset themselves per session.  Is this something we
> normally do for such controls?
>

Re: [PATCH 4/4] ASoC: sti: reset iec60958 settings on close

[Mark Brown]

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

On Fri, Nov 20, 2015 at 10:11:00AM +0100, Moise Gergaud wrote:
> Hello,

Please don't top post, reply in line deleting unneeded text.  This
provides relevant context so people reading your mail can understand
what you are talking about.

> To be compliant with SPDIF & HDMI-1.4 by using aplay, driver needs to set
> the channel status sampling freq = runtime rate; because channel status
> sampling freq is not set by aplay.
> For HBRA, the application set the channel status sampling freq (that is
> different than the runtime rate).
> => by taking into account the 2 above cases, for each pcm session, driver
> shall be able to detect if the channel status sampling freq has already been
> set and set it if needed.

> And also for robustness purpose: in case the channel status sampling freq is
> not set by the application, I think the driver shall set it.

None of this addresses my concern, sorry - your change is just trashing
all the settings that the application is setting.  This is not what we
normally do with controls and is going to break correctly functioning
applications that play audio with the same parameters repeatedly.

> Maybe I can limit my patch by resetting only the channel status sampling
> freq on close (actual patch reset all the fields of the channel status).

This isn't something you should be inventing policies for in a single
driver, the kernel needs to offer consistent interfaces to applications
no matter which particular hardware the system is running.  If we want
to do something here it should be done at ALSA level.  It does seem like
a reasonable idea to put the sample rate into kernel control, I can't
think of a situation where we'd want to advertise the wrong sample rate,
but it does need to be in the core not a specific driver.

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

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

Re: [PATCH 4/4] ASoC: sti: reset iec60958 settings on close

[Moise Gergaud]

On 11/21/2015 01:45 PM, Mark Brown wrote:
> On Fri, Nov 20, 2015 at 10:11:00AM +0100, Moise Gergaud wrote:
>> Hello,
>
> Please don't top post, reply in line deleting unneeded text.  This
> provides relevant context so people reading your mail can understand
> what you are talking about.
>
>> To be compliant with SPDIF & HDMI-1.4 by using aplay, driver needs to set
>> the channel status sampling freq = runtime rate; because channel status
>> sampling freq is not set by aplay.
>> For HBRA, the application set the channel status sampling freq (that is
>> different than the runtime rate).
>> => by taking into account the 2 above cases, for each pcm session, driver
>> shall be able to detect if the channel status sampling freq has already been
>> set and set it if needed.
>
>> And also for robustness purpose: in case the channel status sampling freq is
>> not set by the application, I think the driver shall set it.
>
> None of this addresses my concern, sorry - your change is just trashing
> all the settings that the application is setting.  This is not what we
> normally do with controls and is going to break correctly functioning
> applications that play audio with the same parameters repeatedly.
>
>> Maybe I can limit my patch by resetting only the channel status sampling
>> freq on close (actual patch reset all the fields of the channel status).
>
> This isn't something you should be inventing policies for in a single
> driver, the kernel needs to offer consistent interfaces to applications
> no matter which particular hardware the system is running.  If we want
> to do something here it should be done at ALSA level.  It does seem like
> a reasonable idea to put the sample rate into kernel control, I can't
> think of a situation where we'd want to advertise the wrong sample rate,
> but it does need to be in the core not a specific driver.
>

Agree that this should be done at ALSA level.
Taking into consideration this will be done at ALSA level, I still need 
to correct ASOC STI driver: force the iec958 channel status sampling 
rate to the runtime rate without any condition. I'll provide patch V2.