Re: [RFT v3 3/3] ASoC: core: Add support for DAI multicodec

[Benoit Cousson <bcousson@baylibre.com>]

Hi Lars,

I finally got some BW to address your second email :-)

On 06/05/2014 13:24, Lars-Peter Clausen wrote:
> On 04/24/2014 02:01 PM, 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).
>>
>> Fix a trailing space in the header as well.
>>
>> 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>
>
> Some thoughts on the parameter fixup.
>
>> ---
>>   include/sound/soc-dai.h  |   5 +
>>   include/sound/soc.h      |  13 ++
>>   sound/soc/soc-compress.c |  83 +++++---
>>   sound/soc/soc-core.c     | 364 ++++++++++++++++++++++-----------
>>   sound/soc/soc-dapm.c     |  71 ++++---
>>   sound/soc/soc-pcm.c      | 512
>> ++++++++++++++++++++++++++++++++---------------
>>   6 files changed, 715 insertions(+), 333 deletions(-)
>>
>> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
>> index fad7676..6985851 100644
>> --- a/include/sound/soc-dai.h
>> +++ b/include/sound/soc-dai.h
>> @@ -274,6 +274,11 @@ struct snd_soc_dai {
>>       struct snd_soc_codec *codec;
>>       struct snd_soc_component *component;
>>
>> +    /* CODEC TDM slot masks and params (for fixup) */
>> +    unsigned int tx_mask;
>> +    unsigned int rx_mask;
>> +    struct snd_pcm_hw_params params[2];
>
> snd_pcm_hw_params is rather large, given that for a lot of setups we
> don't need it it might be better to make this a pointer and only
> allocate the params struct when needed.

OK.

>
> [...]
>> @@ -3624,17 +3697,26 @@ static int
>> snd_soc_xlate_tdm_slot_mask(unsigned int slots,
>>   int snd_soc_dai_set_tdm_slot(struct snd_soc_dai *dai,
>>       unsigned int tx_mask, unsigned int rx_mask, int slots, int
>> slot_width)
>>   {
>> +    int ret = 0;
>> +
>>       if (dai->driver && dai->driver->ops->xlate_tdm_slot_mask)
>>           dai->driver->ops->xlate_tdm_slot_mask(slots,
>>                           &tx_mask, &rx_mask);
>>       else
>>           snd_soc_xlate_tdm_slot_mask(slots, &tx_mask, &rx_mask);
>>
>> +    if (dai->codec) {
>> +        dai->tx_mask = tx_mask;
>> +        dai->rx_mask = rx_mask;
>> +    }
>
> I don't think it makes sense to artificially limit this to just CODECs.
> While we do not support multiple CPU DAIs (yet?) adding the check for
> CODECs just makes the code more complex, but doesn't gain us anything.

OK, makes sense.

>
>
> [...]
>> +    for (i = 0; i < rtd->num_codecs; i++) {
>> +        struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
>> +        struct snd_pcm_hw_params *codec_params;
>> +
>> +        codec_params = &codec_dai->params[substream->stream];
>
> The fixed up params are only ever used in here. Do we really need to
> keep two copies per CODEC DAI around?

Well, we can indeed, create a local copy that will be used for the 
fixup. Moreover, it will avoid allocating the data dynamically.

>> +
>> +        /* copy params for each codec */
>> +        memcpy(codec_params, params, sizeof(struct snd_pcm_hw_params));
>> +
>> +        /* fixup params based on TDM slot masks */
>> +        if (codec_dai->tx_mask)
>> +            soc_pcm_codec_params_fixup(codec_params,
>> +                           codec_dai->tx_mask);
>> +        if (codec_dai->rx_mask)
>> +            soc_pcm_codec_params_fixup(codec_params,
>> +                           codec_dai->rx_mask);
>> +
>
> If we fix-up the number of channels to be always the number of slots
> should we also setup a constraint for userspace that the number of
> channels must match the number of channels specified in the CPU DAI
> mask? Or should we check how many of the channels in the CODEC mask are
> actually active and fix-up the the number of channels according to that?

That's a pretty good question.

I've tried to check the current users of the snd_soc_dai_set_tdm_slot to 
understand how it is used in cpu/codec dais.

There are only ~10 users, and it seems that the definition of the mask 
is dependent of the user. I don't know if we can really enforce anything 
because of that :-(

>> +        if (codec_dai->driver->ops &&
>> +            codec_dai->driver->ops->hw_params) {
>> +            ret = codec_dai->driver->ops->hw_params(substream,
>> +                        codec_params, codec_dai);
>> +            if (ret < 0) {
>> +                dev_err(codec_dai->dev,
>> +                    "ASoC: can't set %s hw params: %d\n",
>> +                    codec_dai->name, ret);
>> +                goto codec_err;
>> +            }
>>           }
>
> I think it makes sense to factor this whole block out into a helper
> function. E.g. soc_dai_hw_params(struct snd_soc_dai* dai, struct
> hw_params *params);. This will make it possible to also use it in other
> places like the dapm_dai_link_event() function.

OK, good point.

Thanks,
Benoit

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