[alsa-devel] [PATCH] ASoc: Handle multiple codecs with split playback / capture

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150814/572b7bb0/attachment.sig>