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

[Vinod Koul]

On Thu, Jul 03, 2014 at 01:46:23PM +0200, Benoit Cousson wrote:
> 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 :-)
cpu_dai should be okay :)

BUT Now thinking over this again, does this make sense do do with multiple
codecs??

For folks haveing decoders in DSP inside SoC, the compressed device will be
actually a FE. The BE will be PCM to which codec would be linked. The folks
supporting decoders inside codec like WM wont ever need this.

Mark, do you agree to this?

So re-thinking again on why we need this??

-- 
~Vinod