| Message ID | alpine.DEB.2.02.1508191717300.27635@lnxricardw1.se.axis.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cde79035c6cf578dd33dfea3e39666efc27cbcf2 |
| Headers | show |
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
On Fri, 21 Aug 2015, Pierre-Louis Bossart wrote: > On 8/21/15 2:11 AM, Ricard Wanderlof wrote: > > > > 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. > > ok, sounds fine. maybe you could add this to the comments or commit > message then so that this assumption is known. Ok, good point. Will do. /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.
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;
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(+)