On Thu, Aug 13, 2015 at 04:12:56PM +0200, Ricard Wanderlof wrote:
(Referring to this thread: http://mailman.alsa-project.org/pipermail/alsa-devel/2015-June/093214.html)
I shouldn't need to read another mailing list thread as the first part of a commit log, and if referring to external information you should always include a human readable description of what you're referring to so that people can understand your message without having to start another program. As it is I'm in a plane over the Atlantic so I can't tell what you're talking about here, sorry.
As noted in the thread, there might be more places where the new
Please write the commit log to describe the change without needing to refer to other threads.
/**
- snd_soc_dai_stream_valid() - check if a DAI supports the given stream
- @dai: The DAI to check for stream support
- @stream: The stream type (playback / capture) to check for
You're adding externally readable kerneldoc on a static function so nothing outside this file can use it.
- Checks if the DAI in question supports the indicated stream type.
- Needed to skip over codecs when iterating over
- snd_soc_pcm_runtime->codec_dais in order to skip codecs that are playback-
- or capture-only, which would otherwise inhibit capture or playback,
- respectively, to the other codec in a multiple codec / common I2S bus
- environment.
- */
This mostly seems to describe a potential use of this function, not what it is supposed to do.
+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;
The !! is redundant here.
/* 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. */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;
So what happens if we end up in these loops and nothing matches? It'd be good to document the considerations there.
@@ -827,6 +859,10 @@ 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. */if (!snd_soc_dai_stream_valid(codec_dai, substream->stream))continue;- /* copy params for each codec */ codec_params = *params;
We're still calling hw_params() for all CODECs, even ones that happen to not be suitable... is that sensible? It might be in a case where the CODEC for one direction clocks everything but I can see problems occurring if the transmit and receive paths are separately clocked which is going to be quite common for things like i.MX systems. If the two are tied together there's going to be constraints between the two directions.
I'd expect that we could take a pretty good guess as to what the best thing to do is based on the symmetry constraints we have for the link but I think we do want to think about this early.