> (X) part is for DPCM, and it checks playback/capture availability
> if dai_link has dpcm_playback/capture flag (a)(b).
> This availability check should be available not only for DPCM, but for
> all connections. But it is not mandatory option. Let's name it as
> assertion.
I don't follow the 'not mandatory option'. Why not make these
'assertions' mandatory? What happens in case the the option is not present?
> In case of having assertion flag, non specific side will be disabled.
Not following the wording, multiple negatives and not clear on what
'side' refers to (direction or DPCM/non-DPCM).
> For example if it has playback_assertion but not have capture_assertion,
> capture will be force disabled.
>
> dpcm_playback -> playabck_assertion = 1
>
> dpcm_capture -> capture_assertion = 1
>
> playback_only -> playback_assertion = 1
> capture_assertion = 0
>
> capture_only -> playback_assertion = 0
> capture_assertion = 1
>
> By expanding this assertion to all connections, we can use same code
> for all connections, this means code can be more cleanup.
I see a contradiction between "expanding the assertion to all
connections" and "not mandatory".
> Current validation check on DPCM ignores Codec DAI, but there is no
> reason to do it. We should check both CPU/Codec in all connection.
"there's no reason to do so" ?
> This patch expands validation check to all cases.
>
> [CPU/xxxx]-[yyyy/Codec]
> *****
>
> In many case (not all case), above [xxxx][yyyy] part are "dummy" DAI
> which is always valid for both playback/capture.
> But unfortunately DPCM BE Codec (**** part) had been no validation
> check before, and some Codec driver doesn't have necessary settings for
> it. This means all cases validation check breaks compatibility on some
> vender's drivers. Thus this patch temporary uses dummy DAI at BPCM BE
vendor
> Codec part, and avoid compatibility error. But it should be removed in
> the future.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
> include/sound/soc.h | 9 +++
> sound/soc/soc-pcm.c | 143 +++++++++++++++++++++++++-------------------
> 2 files changed, 92 insertions(+), 60 deletions(-)
>
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 0376f7e4c15d..e604d74f6e33 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -809,6 +809,15 @@ struct snd_soc_dai_link {
> unsigned int dpcm_capture:1;
> unsigned int dpcm_playback:1;
>
> + /*
> + * Capture / Playback support assertion. Having assertion flag is not mandatory.
> + * In case of having assertion flag, non specific side will be disabled.
again the 'not mandatory' and 'non specific side will be disabled' are
confusing.
> + /*
> + * Assertion check
> + *
> + * playback_assertion = 0 No assertion check.
> + * capture_assertion = 0 ASoC will use detected playback/capture as-is.
> + * No playback, No capture will be error.
did you mean "this combination will be handled as an error" ?
It's probably best to have a different presentation, to avoid
confusions. Using multiple lines without a separator isn't great.
Suggested example:
playback_assertion = 0 capture_assertion = 0
this combination will be handled as an error
playback_assertion = 1 capture_assertion = 0
the DAIs in this dailink must support playback.
ASoC will disable capture.
In other words "playback_only"
> + *
> + * playback_assertion = 1 DAI must playback available. ASoC will disable capture.
> + * capture_assertion = 0 In other words "playback_only"
> + *
> + * playback_assertion = 0 DAI must capture available. ASoC will disable playback.
> + * capture_assertion = 1 In other words "capture_only"
> + *
> + * playback_assertion = 1 DAI must both playback/capture available.
> + * capture_assertion = 1
nit-pick: the 'must X available' does not read well, 'must support X' is
probably what you meant.
> + */
> + if (dai_link->playback_assertion) {
> + if (!has_playback) {
> + dev_err(rtd->dev, "%s playback assertion check error\n", dai_link->stream_name);
"invalid playback_assertion" ?
> + return -EINVAL;
> + }
> + /* makes it plyaback only */
typo: playback
> + if (!dai_link->capture_assertion)
> + has_capture = 0;
> + }
> + if (dai_link->capture_assertion) {
> + if (!has_capture) {
> + dev_err(rtd->dev, "%s capture assertion check error\n", dai_link->stream_name);
> + return -EINVAL;
> + }
> + /* makes it capture only */
> + if (!dai_link->playback_assertion)
> + has_playback = 0;
> + }
we probably want a dev_ log when the has_playback/capture is overridden?
>
> + /*
> + * Detect Mismatch
> + */
> if (!has_playback && !has_capture) {
> dev_err(rtd->dev, "substream %s has no playback, no capture\n",
> dai_link->stream_name);
> -
> return -EINVAL;
> }
>
