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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

> (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@renesas.com>
> ---
>  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;
>  	}
>