Re: [PATCH v3 01/23] ASoC: soc-pcm: cleanup soc_get_playback_capture()

[Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>]

Hi Pierre-Louis

Thank you for your feedback

> > (B) commit 1e9de42f4324b91ce2e9da0939cab8fc6ae93893
> > ("Explicitly set BE DAI link supported stream directions") force use to
> > dpcm_xxx flag
> > 
> > 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> > 		playback = rtd->dai_link->dpcm_playback;
> > 		capture = rtd->dai_link->dpcm_capture;
> 
> The reason for this (B) addition is very clear from the commit message
> 
> "
> Some BE DAIs can be "dummy" (when the DSP is controlling the DAI) and as
> such wont have set a minimum number of playback or capture channels
> required for BE DAI registration (to establish supported stream directions).
> "

I'm still not yet 100% understand around this "dummy" DAI, but is it
*not* soc-utils.c :: dummy_dai, but some original dummy DAI is used
somewhere ?

I know ${LINUX}/sound/soc/codecs/hda.c :: card_binder_dai is one of
the DAI which is used but doesn't have channels_min.
I think it is used as BE "Codec", but code is checking "CPU" side.

Do you know what does this "BE dummy DAI" specifically means here?

> > Or if (B) added dpcm_xxx as "option flag", it was understandable for me.
> > like this
> > 
> > 	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
> > 		playback = (cpu_dai->driver->playback.channels_min > 0) ||
> > 			   rtd->dai_link->dpcm_playback;
> > 		capture  = (cpu_dai->driver->capture.channels_min  > 0) ||
> > 			   rtd->dai_link->dpcm_capture;
> 
> That would essentially revert (C), since the direction would ignore the
> actual capabilities of the DAI.
> 
> IMHO, we should really try with this revert and go back to the initial
> definition of (A), where dpcm_xxx is an optional escape mechanism to
> allow machine drivers to set the direction. I would guess that would
> impact mostly DT/simple-card users and Qualcomm, if the commit message
> of (C) is still relevant.

I can agree that above code makes dpcm_xxx flag option, and allow to
machine drivers to set direction without thinking actual DAI capabilities.
I think it is effective if it was around (C) timing.

	(A) : checked CPU capabilities
	(B) : uses dpcm_xxx flag
	(C) : checks both dpcm_xxx and capabilities
	...

But *current* ASoC is checking both "actual capabilities" and "dpcm_xxx"
flag in the same time, and have no problems today.

Around (A)-(B) timing, some DAI had issue (= channels_min was not set).
Let's name it as "no_chan_DAI". It should be CPU DAI and used as BE
if my understanding was correct.

Because "no_chan_DAI" exist, (B) was added.

But after that (C) was added, and it checks channels_min again.
It continues today. This means "no_chan_DAI" today have channels_min,
otherwise it can't work today.

If my above understanding were all correct, do we still need dpcm_xxx ?
because "no_chan_DAI" is no longer exist.
Just remove dpcm_xxx seems no problem, I guess...

> Then we can discuss about merging code and removing flags. For now we
> have different directions/opinions on something that's 10 years old,
> last modified 4 years ago. We will break lots of eggs if we don't first
> agree on what works and what doesn't.
> 
> This 23-patch code merge/simplification is premature at this point IMHO,
> we should first land in a state where everyone is happy with how
> dpcm_xxx flags need to be handled. We can merge dpcm_xxx and xxx_only
> flags later when we understand what they are supposed to do...

Thank you for your help. The problem becoming more clear.
Let's focus to "revert C code" or "remove dpcm_xxx" first.

> And now I need a coffee refill :-)

Enjoy :)


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto