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

From: Benoit Cousson <bcousson@baylibre.com>
To: Lars-Peter Clausen <lars@metafoo.de>,
	Vinod Koul <vinod.koul@intel.com>,
	broonie@kernel.org
Cc: misael.lopez@ti.com, lgirdwood@gmail.com, alsa-devel@alsa-project.org
Subject: Re: [PATCH v4 6/8] ASoC: compress: Add support for DAI multicodec
Date: Thu, 03 Jul 2014 13:46:23 +0200	[thread overview]
Message-ID: <53B5428F.9050100@baylibre.com> (raw)
In-Reply-To: <53B541F0.1040105@metafoo.de>

On 03/07/2014 13:43, Lars-Peter Clausen wrote:
> 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.

Sure, but he was the first one to propose using that :-)

>>>>>>> direction wont change with multiple codecs, so this loop makes no
>>>>>>> sense
>>>>>>> here.
>>>>>>> For simplcity perhaps we can use cpu_dai?

Regards,
Benoit

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