Re: [PATCH v2 01/16] ASoC: soc-pcm.c: cleanup soc_get_playback_capture()

[Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>]

On 4/7/24 22:55, Kuninori Morimoto wrote:
> 
> Hi Pierre-Louis, again
> 
>> dpcm_xxx is used to declare that the DAI/dailink is possible to use
>> playback/capture. For example dpcm_playback means the DAI / dailink
>> should playback-able, if not it is error.
>>
>> xxx_only is used to limit the playback/capture.
>> For example the DAI / dailink can use both playback and capture,
>> but want to use playback only for some reasons, we can use playback_only.
> 
> My pervious patch-set was "try to merge dpcm_xxx and xxx_only flag",
> but next patch will be "expand assertion flag to all connection".
> This "assertion flag" was originaly dpcm_xxx.
> 
> In next patch-set, it will assume for example current "dpcm_playback"
> as "playback_assertion". It can be used not only for DPCM, but
> all connection, but is not mandatory option.
> 
> Its pseudo code is like below, but what do you think ?
> 
> 	soc_get_playback_capture(...)
> 	{
> 		...
> 		/*
> 		 * get HW / DAI availability
> 		 */
> 		for_each_rtd_ch_maps(...) {
> 			...
> 			has_playback = xxx;
> 			has_capture  = xxx;
> 		}
> 
> 		/*
> 		 * "xxx_assersion" was "dpcm_xxx" before, but expand to
> 		 * all connection. It is not mandatory option.
> 		 * It will be error if dai_link has xxx_assersion flag,
> 		 * but DAI was not valid
> 		 */
> 		if (dai_link->playback_assertion && !has_playback) {
> 			dev_err(rtd->dev, ...);
> 			return -EINVAL;
>  		}
> 		if (dai_link->capture_assertion  && !has_capture) {
> 			dev_err(rtd->dev, ...);
> 			return -EINVAL;
> 		}
> 
> 		/*
> 		 * xxx_only flag limits availability. It will indicate warning
> 		 * if DAI was not valid.
> 		 */
> 		if (dai_link->playback_only) {
> 			if (!has_capture)
> 				dev_warn(rtd->dev, ...);
> 			has_capture = 0;
> 		}
> 
> 		if (dai_link->capture_only) {
> 			if (!has_playback)
> 				dev_warn(rtd->dev, ...);
> 			has_playback = 0;
> 		}
> 
> 		/*
> 		 * No Playback, No Capture is error
> 		 */
> 		if (!has_playback && !has_capture) {
> 			dev_err(rtd->dev, ...);
> 			return -EINVAL;
> 		}
> 		...
> 	}

The code looks fine, but what are we trying to achieve?
I thought the idea was to have a single field at the dailink, and with
the example above we would still have two - just like today.
This looks like a lot of code churn in many drivers for limited
benefits. Or I am missing something?