Hi Morimoto-san,
I still wondering why dpcm_xxx flag itself is needed.
That's an excellent question indeed. And since you started a historical review, we can get a lot of information.
(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;
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). "
So (B) introduced these dpcm_xxx flags as override mechanisms, where the dailink information matters more than the dai information.
(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);
It helps to look at the commit message in detail:
" Normally the dpcm_playback/capture parameter should match the capabilities of the CPU DAI. However, there is no way to set that parameter from the device tree (e.g. with simple-audio-card or qcom sound cards). dpcm_playback/capture are always both set to 1. "
This is where the direction changed from "dpcm_xxx" being override of DAI capabilities to "dpcm_xxx" being required to match DAI capabilities, because some machine drivers incorrectly did an override that made no sense...
(C) is essentially (A) && (B)
Clearly there's a contradiction. If (C) was the correct solution, then it would revert the direction in (A) and report an error for dummy dais.
It's been my question from the beginning of this thread, when the direction information can come from 2 sources, which one do we trust?
(D) b73287f0b0745961b14e5ebcce92cc8ed24d4d52 ("ASoC: soc-pcm: dpcm: fix playback/capture checks") expanded it to multi connection.
You really want to look at
(E) 4f8721542f7b75954bfad98c51aa59d683d35b50 ("ASoC: core: use less strict tests for dailink capabilities")
" This patch modifies the snd_soc_dai_link_set_capabilities() helper so that the dpcm_playback (resp. dpcm_capture) dailink capabilities are set if at least one dai supports playback (resp. capture).
Likewise the checks are modified so that an error is reported only when dpcm_playback (resp. dpcm_capture) is set but none of the CPU DAIs support playback (resp. capture). "
This one has not fundamentally changed the reasons why (C) was introduced, the requirement is that dpcm_xxx be aligned with at least ONE DAI capability. It's still not clear how dummy-dais would be handled since the number of channels may or may not be set for those dummy-dais.
So, I would say nothing has changed, but become more complicated.
It's not really become more complicated, the open is which of (B) or (C) are correct.
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.
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...
And now I need a coffee refill :-)
Regards, -Pierre