Re: [PATCH 15/15] ASoC: soc-pcm: indicate warning if DPCM BE Codec has no settings

[Jerome Brunet <jbrunet@baylibre.com>]

On Wed 27 Mar 2024 at 01:06, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> 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