Re: [PATCH 09/10] ASoC: Intel: sof_sdw: Add support for Cirrus Logic CS35L56

[Richard Fitzgerald <rf@opensource.cirrus.com>]

On 21/02/2023 18:03, Pierre-Louis Bossart wrote:
> 
>>>> +                      card->components);
>>>> +    if (!card->components)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    ret = snd_soc_dapm_new_controls(&card->dapm,
>>>> +                    cs35l56_sof_widgets,
>>>> ARRAY_SIZE(cs35l56_sof_widgets));
>>>> +    if (ret) {
>>>> +        dev_err(card->dev, "Widgets add failed: %d\n", ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    ret = snd_soc_dapm_add_routes(&card->dapm, cs35l56_sof_map, count);
>>>> +    if (ret) {
>>>> +        dev_err(card->dev, "Map add %d failed: %d\n", count, ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    /* Enable one feedback TX per amp on different slots */
>>>> +    for_each_rtd_codec_dais(rtd, i, codec_dai) {
>>>> +        ret = snd_soc_dai_set_tdm_slot(codec_dai, 0x3, 1 << i, 4, 16);
>>>
>>> TDM slots? Not getting how this would work with SoundWire?
>>>
>>
>> Strictly speaking Soundwire is TDM (the frame time is divided up into
>> slots for each sample...).
>>
>> The problem is if you have N amps on the dailink all feeding back audio
>> on the same bus. Their DP slots are all programmed to the same positions
>> in the frame, same as for the playback. So you have 4 amps all trying to
>> send 6 audio channels in the same positions in the frame and you'll just
>> get a ton of bus clash interrupts.
>>
>> So we use the set_tdm_slot() like we do with I2S TDM to set which slots
>> are active for each amp.
>>
>> I can't see that there's any obvious "generic" way that the manager code
>> can automatically figure out how many channels to enable on each amp and
>> what order to map them, so we do it here. Just as with I2S TDM - you
>> have many slots and many codecs but the machine driver has to tell it
>> how to map those.
> 
> IIRC Bard did the same thing recently, and the order of the feedback
> channels is really defined by the order in which the peripheral devices
> are added in the dailink. See
> https://github.com/thesofproject/linux/pull/4108
> 

I don't really see how it is determining how to split the channels.
I see there is a discussion about that but I didn't see any explanation
that I could understand.

Also that only seems to have been done for IPCv4. What if my system
uses IPCv3 ?

Anyway, I'll drop the machine driver patches for now until the IPC code
is stable and consistent behavior for v3 and v4.

> There's also another open related to the number of channels, we need to
> patch what the CPU DAI can handle, see
> https://github.com/thesofproject/linux/pull/4136 or
> https://github.com/thesofproject/linux/pull/4134