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

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

Hi Pierre-Louis
Cc Mark

> Your explanation seems to contradict the sentence above "This
> availability check should be available not only for DPCM, but for all
> connections."
>
> Can we actually do this 'availability check' for non-DPCM connections.
>
> > How about this ?
> > 
> > 	If either playback or capture assertion flag was presented,
> > 	not presented direction will be disabled by ASoC even if
> > 	it was available.
> 
> Did you mean
> 
> "
> The playback (resp. capture) direction will be disabled by ASoC if the
> playback_assertion (resp. capture) flag is false - even if this
> direction was available at the DAI level
> "
> > (0, 0) : Both are not must item. available direction is used as-is.
> >          But it will be error if nothing was available.
> 
> That new wording makes me even more confused. What does 'available'
> refer to and at which level is this?
>
> This seems also to contradict the definitions above, "available
> direction is used as-is" is not aligned with "not presented direction
> will be disabled by ASoC even if it was available".

It is complicated by the attempt to merge dpcm_xxx and xxx_only flags.
And I noticed that my one of other attemption was not indicated.

Let's cleanup what does this patch-set want to do

I still wondering why dpcm_xxx flag itself is needed.

(A) Before, it checks channels_min for DPCM, same as current non-DPCM.
This is very clear for me. Let's name this as "validation check"

	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
		if (cpu_dai->driver->playback.channels_min)
			playback = 1;
		if (cpu_dai->driver->capture.channels_min)
			capture = 1;

(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;

(C) 9b5db059366ae2087e07892b5fc108f81f4ec189
("ASoC: soc-pcm: dpcm: Only allow playback/capture if supported")
checks channels_min (= validation check) again

	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
		cpu_dai = asoc_rtd_to_cpu(rtd, 0);
		...
		playback = rtd->dai_link->dpcm_playback &&
			snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK);
		capture = rtd->dai_link->dpcm_capture &&
			snd_soc_dai_stream_valid(cpu_dai, SNDRV_PCM_STREAM_CAPTURE);

(D) b73287f0b0745961b14e5ebcce92cc8ed24d4d52
("ASoC: soc-pcm: dpcm: fix playback/capture checks") expanded it to
multi connection.

So, I would say nothing has changed, but become more complicated.
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;

So my opinion is this dpcm_xxx is unnecessary flag that only complicate
matters. I guess almost all Card don't need this flag, this means
"validation check" only is veryenough, same as current non-DPCM.

But because of these history, dpcm_xxx flag have been used as
"passage permit" or "gate way". It doesn't try to "validation check" if
dpcm_xxx flag was not set. This is the reason why I try to merge
dpcm_xxx and xxx_only flag. These are doing the same things with
dirrerent flags, IMO.

OTOH, some Card want to detect error if expected direction
(playback/capture) was not valid. I guess this is your commitment (?).

So, let's keep xxx_only flag as-is, and use dpcm_xxx as "available_check".
I'm not sure what is the good naming, but for example
"playback_available_check" flag means "owner is expecting playback is
valid/available, and want to receive error if not".

I'm not sure how many owner want this flag, thus I think "option flag"
is very enough (= not mandatory, as I mentioned in the patch-set).

If we makes these checks generalize,
For DPCM, (for example new DPCM) it can remove/ignore "available_check"
flag if it don't need, same as current non-DPCM.
And for non-DPCM, it can use "available_check" if needed,
same as current DPCM.

What do you think ? what is your opinion ?

Thank you for your help !!

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