On Wed 27 Mar 2024 at 01:06, Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
> Hi Jerome
>
> Thank you for your feedback
>
>> I'm not quite sure what you mean by "should have validation" and what
>> setting exactly we should validate ?
>>
>> I know I should be able to able to understand that
>> from the code below but, somehow I have trouble deciphering it.
>
> Current ASoC have validation for ^^^ part
>
> DPCM
> [CPU/xxxx]-[xxxx/Codec]
> ^^^^ (A)
> Normal
> [CPU/Codec]
> ^^^^^^^^^^^
>
> (In many case, this "xxxx" is "dummy")
Yes for many DPCM user, you have:
DPCM
[CPU/dummy]-[dummy/Codec]
FYI: on Amlogic it is mostly the following
(only considering DCPM, omitting C2C ...)
DPCM
[CPU-FE/dummy]-[CPU-BE/Codec]
With possibly several BE instances per FE, and several codecs per BE.
And there is even this for loopbacks:
DPCM
[CPU-FE/dummy]-[CPU-BE/dummy]
> By this patch-set, It will check all cases
>
> DPCM
> [CPU/xxxx]-[xxxx/Codec]
> ^^^^^^^^^ ^^^^^^^^^^ (B)
> Normal
> [CPU/Codec]
> ^^^^^^^^^^^
>
> At first, in [CPU/xxxx] case, "xxxx" part should be also checked
> (in many case, this "xxxx" is "dummy").
>
> And, because it didn't check (A) part before,
> (B) part might be error on some board (at least Intel board).
> To avoid such case, temporally it uses "dummy" instead of "Codec"
> before [15/15]. This means (B) part checked as like below.
>
> [xxxx/Codec] -> [xxxx/dummy]
>
> Because "dummy" will pass all cases, (B) part is almost same as no check.
> Yes, it is no meaning, but the code will be simple.
>
>> Where you have a CPU supporting both direction and 2 codecs, each
>> supporting 1 stream direction ? This is a valid i2s configuration.
> (snip)
>> > /*
>> > - * FIXME
>> > + * FIXME / CLEAN-UP-ME
>> > *
>> > * DPCM BE Codec has been no checked before.
>> > * It should be checked, but it breaks compatibility.
>> > * It ignores BE Codec here, so far.
>> > */
>> > - if (dai_link->no_pcm)
>> > - codec_dai = dummy_dai;
>> > + if ((dai_link->no_pcm) &&
>> > + ((cpu_play_t && !codec_play_t) ||
>> > + (cpu_capture_t && !codec_capture_t))) {
>> > + dev_warn_once(rtd->dev, "DCPM BE Codec has no stream settings (%s)\n",
>> > + codec_dai->name);
>>
>> Taking one codec at a time, would you trigger a warning for the use case I
>> described above ?
>
> Oops, indeed it will indicate warning in your case.
> How about this ?
>
> if ((dai_link->no_pcm) &&
^ Actually my comment applies to all links, DPCM backend or not
> (!codec_play_t && !codec_capture_t)) {
A codec that does not support playback and does not support capture does
not support much, does it ? ;)
I suppose you meant something like:
> (!cpu_play_t && !codec_capture_t)) {
Then at first glance, maybe ... CPU and codec seem to exclude each other but
that will only work as long as DCPM is limited to a single CPU per link.
> dev_warn_once(...)
> ...
> }
>
> Thank you for your help !!
>
> Best regards
> ---
> Renesas Electronics
> Ph.D. Kuninori Morimoto
--
Jerome
