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

[Benoit Cousson]

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.

Thanks,
Benoit

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