[alsa-devel] [PATCH 0/4] ASoC sti: corrections
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(-)
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");
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");
"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;
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;
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,
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,
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];
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?
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?
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.
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.
participants (2)
-
Mark Brown -
Moise Gergaud