Re: [PATCH v4 3/8] ASoC: core: Add initial support for DAI multicodec

From: Benoit Cousson <bcousson@baylibre.com>
To: Lars-Peter Clausen <lars@metafoo.de>,
	broonie@kernel.org, lgirdwood@gmail.com
Cc: Fabien Parent <fparent@baylibre.com>,
	misael.lopez@ti.com, alsa-devel@alsa-project.org
Subject: Re: [PATCH v4 3/8] ASoC: core: Add initial support for DAI multicodec
Date: Tue, 01 Jul 2014 19:27:53 +0200	[thread overview]
Message-ID: <53B2EF99.5030006@baylibre.com> (raw)
In-Reply-To: <53B2B565.9080808@metafoo.de>

Hi Lars,

On 01/07/2014 15:19, Lars-Peter Clausen wrote:
> On 07/01/2014 09:47 AM, Benoit Cousson wrote:
>> DAI link assumes a one to one mapping between CPU DAI and CODEC. In
>> some cases, the same CPU DAI can be connected to several codecs.
>> This is the case for example, if you connect two mono codecs to the
>> same I2S link in order to have a stereo card.
>> The current ASoC implementation does not allow such setup.
>>
>> Add support for DAI links composed of a single CPU DAI and multiple
>> CODECs. Sound cards have to pass the CODECs array in the corresponding
>> DAI link through a new 'snd_soc_dai_link_component' struct. Each CODEC in
>> this array is described in the same manner single CODEC DAIs are
>> (either DT/OF node or codec_name).
>>
>> Based on an original code done by Misael.
>>
>> Signed-off-by: Benoit Cousson <bcousson@baylibre.com>
>> Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
>> Signed-off-by: Fabien Parent <fparent@baylibre.com>
>
> Almost. There are two more serious issues, the rest is trivial.

Gosh... Another new update to do :-)
I'll try to be faster this time...

>> ---
> [...]
>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
>> index 37a965c..3764150 100644
>> --- a/sound/soc/soc-core.c
>> +++ b/sound/soc/soc-core.c
>> @@ -554,7 +554,7 @@ int snd_soc_suspend(struct device *dev)
>>   {
>>       struct snd_soc_card *card = dev_get_drvdata(dev);
>>       struct snd_soc_codec *codec;
>> -    int i;
>> +    int i, j;
>>
>>       /* If the initialization of this soc device failed, there is no
>> codec
>>        * associated with it. Just bail out in this case.
>> @@ -574,14 +574,16 @@ int snd_soc_suspend(struct device *dev)
>>
>>       /* mute any active DACs */
>>       for (i = 0; i < card->num_rtd; i++) {
>> -        struct snd_soc_dai *dai = card->rtd[i].codec_dai;
>> -        struct snd_soc_dai_driver *drv = dai->driver;
>> +        for (j = 0; j < card->rtd[i].num_codecs; j++) {
>> +            struct snd_soc_dai *dai = card->rtd[i].codec_dais[j];
>> +            struct snd_soc_dai_driver *drv = dai->driver;
>>
>> -        if (card->rtd[i].dai_link->ignore_suspend)
>> -            continue;
>> +            if (card->rtd[i].dai_link->ignore_suspend)
>> +                continue;
>
> This check can actually stay outside the inner loop. We either want to
> mute all or none.

Indeed.

>
>>
>> -        if (drv->ops->digital_mute && dai->playback_active)
>> -            drv->ops->digital_mute(dai, 1);
>> +            if (drv->ops->digital_mute && dai->playback_active)
>> +                drv->ops->digital_mute(dai, 1);
>> +        }
>>       }
>>
>>       /* suspend all pcms */
> [...]
>> @@ -697,7 +703,7 @@ static void soc_resume_deferred(struct work_struct
>> *work)
>>       struct snd_soc_card *card =
>>               container_of(work, struct snd_soc_card,
>> deferred_resume_work);
>>       struct snd_soc_codec *codec;
>> -    int i;
>> +    int i, j;
>>
>>       /* our power state is still SNDRV_CTL_POWER_D3hot from suspend
>> time,
>>        * so userspace apps are blocked from touching us
>> @@ -758,14 +764,17 @@ static void soc_resume_deferred(struct
>> work_struct *work)
>>
>>       /* unmute any active DACs */
>>       for (i = 0; i < card->num_rtd; i++) {
>> -        struct snd_soc_dai *dai = card->rtd[i].codec_dai;
>> -        struct snd_soc_dai_driver *drv = dai->driver;
>>
>> -        if (card->rtd[i].dai_link->ignore_suspend)
>> -            continue;
>> +        for (j = 0; j < card->rtd[i].num_codecs; j++) {
>> +            struct snd_soc_dai *dai = card->rtd[i].codec_dais[j];
>> +            struct snd_soc_dai_driver *drv = dai->driver;
>> +
>> +            if (card->rtd[i].dai_link->ignore_suspend)
>> +                continue;
>
> Same as with the mute loop.

OK

>
>>
>> -        if (drv->ops->digital_mute && dai->playback_active)
>> -            drv->ops->digital_mute(dai, 0);
>> +            if (drv->ops->digital_mute && dai->playback_active)
>> +                drv->ops->digital_mute(dai, 0);
>> +        }
>>       }
>>
>>       for (i = 0; i < card->num_rtd; i++) {
> [...]                int num)
>> +static int soc_aux_dev_init(struct snd_soc_card *card, int num)
>>   {
>>       struct snd_soc_aux_dev *aux_dev = &card->aux_dev[num];
>>       struct snd_soc_pcm_runtime *rtd = &card->rtd_aux[num];
>> +    struct snd_soc_codec *codec;
>>       int ret;
>>
>>       rtd->card = card;
>>
>> +    codec = soc_find_codec(NULL, aux_dev->codec_name);
>
> We actually have support for binding the aux dev by DT node now. But I
> have a patch in my componentization branch that cleans all this up. Let
> me send it to you and then you can rebase on-top of that.

OK, I guess you are talking about soc_find_matching_codec API introduced 
by Sebatian. Are you gonna merge it with the soc_find_codec API?

>
>> +    if (!codec)
>> +        return -EPROBE_DEFER;
>> +
>>       /* do machine specific initialization */
>>       if (aux_dev->init) {
>>           ret = aux_dev->init(&codec->dapm);
>> @@ -1286,16 +1316,19 @@ static int soc_aux_dev_init(struct
>> snd_soc_card *card,
>>       return 0;
>>   }
>>
>> -static int soc_dai_link_init(struct snd_soc_card *card,
>> -                 struct snd_soc_codec *codec,
>> -                 int num)
>> +static int soc_dai_link_init(struct snd_soc_card *card, int num)
>>   {
>>       struct snd_soc_dai_link *dai_link =  &card->dai_link[num];
>>       struct snd_soc_pcm_runtime *rtd = &card->rtd[num];
>> -    int ret;
>> +    int i, ret;
>>
>>       rtd->card = card;
>>
>> +    for (i = 0; i < rtd->num_codecs; i++) {
>> +        /* Make sure all DAPM widgets are instantiated */
>> +        snd_soc_dapm_new_widgets(rtd->codec_dais[i]->codec->dapm.card);
>> +    }
>
> This is still a left over from a very early revision of this patch. We
> removed this in upstream a while ago.

Oops, sorry about that :-(

Thanks,
Benoit

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