[alsa-devel] [PATCH v2] ASoC: Handle multiple codecs with split playback / capture
Add the capability to use multiple codecs on the same DAI linke where one codec is used for playback and another one is used for capture.
Tested on a setup using an SSM2518 for playback and an ICS43432 I2S MEMS microphone for capture.
Signed-off-by: Ricard Wanderlof ricardw@axis.com ---
The patch was created after a discussion on the alsa-devel mailing list as to how best to implement this functionality. (http://mailman.alsa-project.org/pipermail/alsa-devel/2015-June/093214.html).
V2: Minor code change, otherwise the differences compared to V1 are solely related to comments, in particular considerations given to the potential consequences of the patch. After consideration, it seems to me that the patch as it stands augments the current framework with the functionality indicated in the commit message above, without restricting other current uses. Further functionality could be added, but IMHO that should be done as the need arises, when it can be properly tested in a real-world setup.
sound/soc/soc-pcm.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 35fe58f4..08407ba 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -34,6 +34,24 @@
#define DPCM_MAX_BE_USERS 8
+/* + * snd_soc_dai_stream_valid() - check if a DAI supports the given stream + * + * Returns true if the DAI supports the indicated stream type. + */ +static bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream) +{ + struct snd_soc_pcm_stream *codec_stream; + + if (stream == SNDRV_PCM_STREAM_PLAYBACK) + codec_stream = &dai->driver->playback; + else + codec_stream = &dai->driver->capture; + + /* If the codec specifies any rate at all, it supports the stream. */ + return codec_stream->rates; +} + /** * snd_soc_runtime_activate() - Increment active count for PCM runtime components * @rtd: ASoC PCM runtime that is activated @@ -371,6 +389,20 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
/* first calculate min/max only for CODECs in the DAI link */ for (i = 0; i < rtd->num_codecs; i++) { + + /* + * Skip CODECs which don't support the current stream type. + * Otherwise, since the rate, channel, and format values will + * zero in that case, we would have no usable settings left, + * causing the resulting setup to fail. + * At least one CODEC should match, otherwise we should have + * bailed out on a higher level, since there would be no + * CODEC to support the transfer direction in that case. + */ + if (!snd_soc_dai_stream_valid(rtd->codec_dais[i], + substream->stream)) + continue; + codec_dai_drv = rtd->codec_dais[i]->driver; if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) codec_stream = &codec_dai_drv->playback; @@ -827,6 +859,23 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *codec_dai = rtd->codec_dais[i]; struct snd_pcm_hw_params codec_params;
+ /* + * Skip CODECs which don't support the current stream type, + * the idea being that if a CODEC is not used for the currently + * set up transfer direction, it should not need to be + * configured, especially since the configuration used might + * not even be supported by that CODEC. There may be cases + * however where a CODEC needs to be set up although it is + * actually not being used for the transfer, e.g. if a + * capture-only CODEC is acting as an LRCLK and/or BCLK master + * for the DAI link including a playback-only CODEC. + * If this becomes necessary, we will have to augment the + * machine driver setup with information on how to act, so + * we can do the right thing here. + */ + if (!snd_soc_dai_stream_valid(codec_dai, substream->stream)) + continue; + /* copy params for each codec */ codec_params = *params;
On 8/19/15 10:32 AM, Ricard Wanderlof wrote:
Add the capability to use multiple codecs on the same DAI linke where one codec is used for playback and another one is used for capture.
Tested on a setup using an SSM2518 for playback and an ICS43432 I2S MEMS microphone for capture.
Signed-off-by: Ricard Wanderlof ricardw@axis.com
The patch was created after a discussion on the alsa-devel mailing list as to how best to implement this functionality. (http://mailman.alsa-project.org/pipermail/alsa-devel/2015-June/093214.html).
V2: Minor code change, otherwise the differences compared to V1 are solely related to comments, in particular considerations given to the potential consequences of the patch. After consideration, it seems to me that the patch as it stands augments the current framework with the functionality indicated in the commit message above, without restricting other current uses. Further functionality could be added, but IMHO that should be done as the need arises, when it can be properly tested in a real-world setup.
sound/soc/soc-pcm.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 35fe58f4..08407ba 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -34,6 +34,24 @@
#define DPCM_MAX_BE_USERS 8
+/*
- snd_soc_dai_stream_valid() - check if a DAI supports the given stream
- Returns true if the DAI supports the indicated stream type.
- */
+static bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream) +{
- struct snd_soc_pcm_stream *codec_stream;
- if (stream == SNDRV_PCM_STREAM_PLAYBACK)
codec_stream = &dai->driver->playback;- else
codec_stream = &dai->driver->capture;- /* If the codec specifies any rate at all, it supports the stream. */
- return codec_stream->rates;
+}
- /**
- snd_soc_runtime_activate() - Increment active count for PCM runtime components
- @rtd: ASoC PCM runtime that is activated
@@ -371,6 +389,20 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
/* first calculate min/max only for CODECs in the DAI link */ for (i = 0; i < rtd->num_codecs; i++) {
/** Skip CODECs which don't support the current stream type.* Otherwise, since the rate, channel, and format values will* zero in that case, we would have no usable settings left,* causing the resulting setup to fail.* At least one CODEC should match, otherwise we should have* bailed out on a higher level, since there would be no* CODEC to support the transfer direction in that case.*/if (!snd_soc_dai_stream_valid(rtd->codec_dais[i],substream->stream))
Maybe I misunderstood but shouldn't there be some sort of verification that the codecs can use the same number of slots between playback and capture if they share the same LRCLK/FS? e.g. it's not uncommon to have 4 mic capture and 2 ch playback. If the capture and playback is handled by different chips you'd still need to maintain some level of consistency.
continue;- codec_dai_drv = rtd->codec_dais[i]->driver; if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) codec_stream = &codec_dai_drv->playback;
@@ -827,6 +859,23 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *codec_dai = rtd->codec_dais[i]; struct snd_pcm_hw_params codec_params;
/** Skip CODECs which don't support the current stream type,* the idea being that if a CODEC is not used for the currently* set up transfer direction, it should not need to be* configured, especially since the configuration used might* not even be supported by that CODEC. There may be cases* however where a CODEC needs to be set up although it is* actually not being used for the transfer, e.g. if a* capture-only CODEC is acting as an LRCLK and/or BCLK master* for the DAI link including a playback-only CODEC.* If this becomes necessary, we will have to augment the* machine driver setup with information on how to act, so* we can do the right thing here.*/if (!snd_soc_dai_stream_valid(codec_dai, substream->stream))continue;- /* copy params for each codec */ codec_params = *params;
On Thu, Aug 20, 2015 at 10:45:21AM -0500, Pierre-Louis Bossart wrote:
On 8/19/15 10:32 AM, Ricard Wanderlof wrote:
/** Skip CODECs which don't support the current stream type.* Otherwise, since the rate, channel, and format values will* zero in that case, we would have no usable settings left,* causing the resulting setup to fail.* At least one CODEC should match, otherwise we should have* bailed out on a higher level, since there would be no* CODEC to support the transfer direction in that case.*/if (!snd_soc_dai_stream_valid(rtd->codec_dais[i],substream->stream))
Maybe I misunderstood but shouldn't there be some sort of verification that the codecs can use the same number of slots between playback and capture if they share the same LRCLK/FS? e.g. it's not uncommon to have 4 mic capture and 2 ch playback. If the capture and playback is handled by different chips you'd still need to maintain some level of consistency.
Yes, this was my point about it being hard to work out what's going on with regard to things being shared.
On Thu, 20 Aug 2015, Pierre-Louis Bossart wrote:
/**
- snd_soc_runtime_activate() - Increment active count for PCM runtime components
- @rtd: ASoC PCM runtime that is activated
@@ -371,6 +389,20 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
/* first calculate min/max only for CODECs in the DAI link */ for (i = 0; i < rtd->num_codecs; i++) {
/** Skip CODECs which don't support the current stream type.* Otherwise, since the rate, channel, and format values will* zero in that case, we would have no usable settings left,* causing the resulting setup to fail.* At least one CODEC should match, otherwise we should have* bailed out on a higher level, since there would be no* CODEC to support the transfer direction in that case.*/if (!snd_soc_dai_stream_valid(rtd->codec_dais[i],substream->stream))Maybe I misunderstood but shouldn't there be some sort of verification that the codecs can use the same number of slots between playback and capture if they share the same LRCLK/FS? e.g. it's not uncommon to have 4 mic capture and 2 ch playback. If the capture and playback is handled by different chips you'd still need to maintain some level of consistency.
You're probably right, although it may be that that sort of thing is handled at a higher level, e.g. the machine driver in such a setup would configure both codecs to use TDM before we even get to this function, I don't know.
I think we'd have to take care of that situation if and when it arises. The setup I have here doesn't allow me to test it.
/Ricard
Any comments, ACKs or NAKs on the patch below? It's been about a week since I posted it.
/Ricard
On Wed, 19 Aug 2015, Ricard Wanderlof wrote:
Add the capability to use multiple codecs on the same DAI linke where one codec is used for playback and another one is used for capture.
Tested on a setup using an SSM2518 for playback and an ICS43432 I2S MEMS microphone for capture.
Signed-off-by: Ricard Wanderlof ricardw@axis.com
The patch was created after a discussion on the alsa-devel mailing list as to how best to implement this functionality. (http://mailman.alsa-project.org/pipermail/alsa-devel/2015-June/093214.html).
V2: Minor code change, otherwise the differences compared to V1 are solely related to comments, in particular considerations given to the potential consequences of the patch. After consideration, it seems to me that the patch as it stands augments the current framework with the functionality indicated in the commit message above, without restricting other current uses. Further functionality could be added, but IMHO that should be done as the need arises, when it can be properly tested in a real-world setup.
sound/soc/soc-pcm.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 35fe58f4..08407ba 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -34,6 +34,24 @@
#define DPCM_MAX_BE_USERS 8
+/*
- snd_soc_dai_stream_valid() - check if a DAI supports the given stream
- Returns true if the DAI supports the indicated stream type.
- */
+static bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream) +{
- struct snd_soc_pcm_stream *codec_stream;
- if (stream == SNDRV_PCM_STREAM_PLAYBACK)
codec_stream = &dai->driver->playback;- else
codec_stream = &dai->driver->capture;- /* If the codec specifies any rate at all, it supports the stream. */
- return codec_stream->rates;
+}
/**
- snd_soc_runtime_activate() - Increment active count for PCM runtime components
- @rtd: ASoC PCM runtime that is activated
@@ -371,6 +389,20 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
/* first calculate min/max only for CODECs in the DAI link */ for (i = 0; i < rtd->num_codecs; i++) {
/** Skip CODECs which don't support the current stream type.* Otherwise, since the rate, channel, and format values will* zero in that case, we would have no usable settings left,* causing the resulting setup to fail.* At least one CODEC should match, otherwise we should have* bailed out on a higher level, since there would be no* CODEC to support the transfer direction in that case.*/if (!snd_soc_dai_stream_valid(rtd->codec_dais[i],substream->stream))continue;- codec_dai_drv = rtd->codec_dais[i]->driver; if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) codec_stream = &codec_dai_drv->playback;
@@ -827,6 +859,23 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *codec_dai = rtd->codec_dais[i]; struct snd_pcm_hw_params codec_params;
/** Skip CODECs which don't support the current stream type,* the idea being that if a CODEC is not used for the currently* set up transfer direction, it should not need to be* configured, especially since the configuration used might* not even be supported by that CODEC. There may be cases* however where a CODEC needs to be set up although it is* actually not being used for the transfer, e.g. if a* capture-only CODEC is acting as an LRCLK and/or BCLK master* for the DAI link including a playback-only CODEC.* If this becomes necessary, we will have to augment the* machine driver setup with information on how to act, so* we can do the right thing here.*/if (!snd_soc_dai_stream_valid(codec_dai, substream->stream))continue;- /* copy params for each codec */ codec_params = *params;
-- 1.7.10.4
-- Ricard Wolf Wanderlöf ricardw(at)axis.com Axis Communications AB, Lund, Sweden www.axis.com Phone +46 46 272 2016 Fax +46 46 13 61 30
On Thu, Sep 03, 2015 at 05:05:14PM +0200, Ricard Wanderlof wrote:
Any comments, ACKs or NAKs on the patch below? It's been about a week since I posted it.
/Ricard
On Wed, 19 Aug 2015, Ricard Wanderlof wrote:
Please don't top post or send content free pings, they only add to the mail volume, and allow a reasonable amount of time for review. A week is not reasonable, people travel, take holidays or whatever - two weeks is at the *lower* bound.
On Thu, 3 Sep 2015, Mark Brown wrote:
On Thu, Sep 03, 2015 at 05:05:14PM +0200, Ricard Wanderlof wrote:
Any comments, ACKs or NAKs on the patch below? It's been about a week since I posted it.
/Ricard
On Wed, 19 Aug 2015, Ricard Wanderlof wrote:
Please don't top post or send content free pings, they only add to the mail volume, and allow a reasonable amount of time for review. A week is not reasonable, people travel, take holidays or whatever - two weeks is at the *lower* bound.
Sure no problem. It seemed to me that subsequently submitted patches had been reviewed and applied, which led me to think that this one had gotten lost in the maelstrom, and people also tend to forget or miss things so a friendly reminder is not usually out of order. I've gotten the opposite response at other times when querying after a month, getting the response, 'why didn't you ask earlier'.
No rush otherwise, I'll just sit tight.
/Ricard
On Thu, Sep 03, 2015 at 05:59:35PM +0200, Ricard Wanderlof wrote:
Sure no problem. It seemed to me that subsequently submitted patches had been reviewed and applied, which led me to think that this one had gotten lost in the maelstrom, and people also tend to forget or miss things so a friendly reminder is not usually out of order. I've gotten the opposite response at other times when querying after a month, getting the response, 'why didn't you ask earlier'.
This is a complicated patch submitted right at the end of the development cycle which means it's harder than most to review, and obviously your other patch needed quite a few revisions. And, just to emphasise, content free pings are just a waste of space - nobody can review an empty e-mail, if your patch has been lost then your mail is not an adequate replacement.
The patch
ASoC: Handle multiple codecs with split playback / capture
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 cde79035c6cf578dd33dfea3e39666efc27cbcf2 Mon Sep 17 00:00:00 2001
From: Ricard Wanderlof ricard.wanderlof@axis.com Date: Mon, 24 Aug 2015 14:16:51 +0200 Subject: [PATCH] ASoC: Handle multiple codecs with split playback / capture
Add the capability to use multiple codecs on the same DAI linke where one codec is used for playback and another one is used for capture.
Tested on a setup using an SSM2518 for playback and an ICS43432 I2S MEMS microphone for capture.
No verification is made that the playback and capture codec setups are compatible in terms of number of TDM slots (where applicable).
Signed-off-by: Ricard Wanderlof ricardw@axis.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-pcm.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 70e4b9d..3173958 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -34,6 +34,24 @@
#define DPCM_MAX_BE_USERS 8
+/* + * snd_soc_dai_stream_valid() - check if a DAI supports the given stream + * + * Returns true if the DAI supports the indicated stream type. + */ +static bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream) +{ + struct snd_soc_pcm_stream *codec_stream; + + if (stream == SNDRV_PCM_STREAM_PLAYBACK) + codec_stream = &dai->driver->playback; + else + codec_stream = &dai->driver->capture; + + /* If the codec specifies any rate at all, it supports the stream. */ + return codec_stream->rates; +} + /** * snd_soc_runtime_activate() - Increment active count for PCM runtime components * @rtd: ASoC PCM runtime that is activated @@ -371,6 +389,20 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
/* first calculate min/max only for CODECs in the DAI link */ for (i = 0; i < rtd->num_codecs; i++) { + + /* + * Skip CODECs which don't support the current stream type. + * Otherwise, since the rate, channel, and format values will + * zero in that case, we would have no usable settings left, + * causing the resulting setup to fail. + * At least one CODEC should match, otherwise we should have + * bailed out on a higher level, since there would be no + * CODEC to support the transfer direction in that case. + */ + if (!snd_soc_dai_stream_valid(rtd->codec_dais[i], + substream->stream)) + continue; + codec_dai_drv = rtd->codec_dais[i]->driver; if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) codec_stream = &codec_dai_drv->playback; @@ -827,6 +859,23 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *codec_dai = rtd->codec_dais[i]; struct snd_pcm_hw_params codec_params;
+ /* + * Skip CODECs which don't support the current stream type, + * the idea being that if a CODEC is not used for the currently + * set up transfer direction, it should not need to be + * configured, especially since the configuration used might + * not even be supported by that CODEC. There may be cases + * however where a CODEC needs to be set up although it is + * actually not being used for the transfer, e.g. if a + * capture-only CODEC is acting as an LRCLK and/or BCLK master + * for the DAI link including a playback-only CODEC. + * If this becomes necessary, we will have to augment the + * machine driver setup with information on how to act, so + * we can do the right thing here. + */ + if (!snd_soc_dai_stream_valid(codec_dai, substream->stream)) + continue; + /* copy params for each codec */ codec_params = *params;
participants (3)
-
Mark Brown -
Pierre-Louis Bossart -
Ricard Wanderlof