[alsa-devel] [PATCH v4 6/8] ASoC: compress: Add support for DAI multicodec

[Lars-Peter Clausen]

On 07/03/2014 01:39 PM, Benoit Cousson wrote:
> On 03/07/2014 13:20, Lars-Peter Clausen wrote:
>> On 07/03/2014 01:09 PM, Benoit Cousson wrote:
>>> 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?
>>>
>>
>> I guess it is ok. But I'm wondering if the direction shouldn't depend on
>> the CPU dai rather than the CODEC dai? E.g. it is possible to have a
>> CODEC with both playback and capture support in two dai_links, one for
>> capture, one for playback. The check above would create a
>> SND_COMPRESS_PLAYBACK stream in both cases. Even if the CPU dai only
>> supported capture in one of the cases.
>
> OK, I'll use the cpu_dai then.

Lets wait for Vinod's feedback on this first. He knows this code best and 
what the intentions are.

- Lars