Re: [PATCH v4 6/8] ASoC: compress: Add support for DAI multicodec

[Benoit Cousson <bcousson@baylibre.com>]

On 03/07/2014 08:41, Vinod Koul wrote:
> On Wed, Jul 02, 2014 at 02:53:50PM +0200, Benoit Cousson wrote:
>>>> @@ -622,23 +633,33 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
>>>>   {
>>>>   	struct snd_soc_codec *codec = rtd->codec;
>>>>   	struct snd_soc_platform *platform = rtd->platform;
>>>> -	struct snd_soc_dai *codec_dai = rtd->codec_dai;
>>>>   	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>>>>   	struct snd_compr *compr;
>>>>   	struct snd_pcm *be_pcm;
>>>>   	char new_name[64];
>>>> -	int ret = 0, direction = 0;
>>>> -
>>>> -	/* check client and interface hw capabilities */
>>>> -	snprintf(new_name, sizeof(new_name), "%s %s-%d",
>>>> -			rtd->dai_link->stream_name, codec_dai->name, num);
>>>> -
>>>> -	if (codec_dai->driver->playback.channels_min)
>>>> -		direction = SND_COMPRESS_PLAYBACK;
>>>> -	else if (codec_dai->driver->capture.channels_min)
>>>> -		direction = SND_COMPRESS_CAPTURE;
>>>> -	else
>>>> -		return -EINVAL;
>>>> +	int i, ret = 0, direction = -1;
>>>> +
>>>> +	for (i = 0; i < rtd->num_codecs; i++) {
>>>> +		struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
>>>> +		/* check client and interface hw capabilities */
>>>> +		snprintf(new_name, sizeof(new_name), "%s %s-%d",
>>>> +			 rtd->dai_link->stream_name, codec_dai->name, num);
>>>> +
>>>> +		if (direction < 0) {
>>>> +			if (codec_dai->driver->playback.channels_min)
>>>> +				direction = SND_COMPRESS_PLAYBACK;
>>>> +			else if (codec_dai->driver->capture.channels_min)
>>>> +				direction = SND_COMPRESS_CAPTURE;
>>> direction wont change with multiple codecs, so this loop makes no sense here.
>>> For simplcity perhaps we can use cpu_dai?
>>>> +			else
>>>> +				return -EINVAL;
>>>> +		} else {
>>> when will this get executed? You have initialized direction to -1 and if case is
>>> always true. I think compiler will simply remove the below sinppet.
>>
>> direction is set to -1, then the first iteration will set it to the
>> direction of the first codec_dai. The other iteration are just
>> checking that there are all in the same direction and will fail
>> otherwise.
>>
>>>> +			if ((codec_dai->driver->playback.channels_min &&
>>>> +					direction != SND_COMPRESS_PLAYBACK) ||
>>>> +			    (codec_dai->driver->capture.channels_min &&
>>>> +					direction != SND_COMPRESS_CAPTURE))
>>> and what exactly are we trying to check here?
>>
>> That every codec_dai are in the same direction.
>>
>> If you think this is pointless since every DAI are always in the
>> same direction, we can just remove it.
> I think that will simplify it. we certainly dont expect different direction,
> right?

Lars, Mark
Any thought on that? Does that sound reasonable to you to use only the 
first codec_dai to figure out the direction?

Thanks,
Benoit

-- 
Benoît Cousson
BayLibre
Embedded Linux Technology Lab
www.baylibre.com