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

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

On 4/2/24 01:43, Kuninori Morimoto wrote:
> 
> Hi Pierre-Louis, again
> 
>>> The problem I have is with the following code (not shown with diff)
>>>
>>> 	if (dai_link->playback_only)
>>> 		has_capture = 0;
>>>
>>> 	if (dai_link->capture_only)
>>> 		has_playback = 0;
>>>
>>> So with this grand unification, all the loops above may make a decision
>>> that could be overridden by these two branches.
>>>
>>> This was not the case before for DPCM, all the 'has_capture' and
>>> 'has_playback' variables were used as a verification of the dai_link
>>> settings with an error thrown e.g. if the dpcm_playback was set without
>>> any DAIs supporting playback.
> 
> Hmm... above 2 branches are used for DPCM too before.

Not really, playback_only and capture_only were never set so those two
branches were always false.
> 
>>> Now the dailink settings are used unconditionally. There is one warning
>>> added if there are no settings for a dailink, but we've lost the
>>> detection of a mismatch between dailink and the set of cpu/codec dais
>>> that are part of this dailink.
> 
> Does this mean, for example you want to have warning/error by dpcm_playback
> flag if you are thinking it can use playback , but FE or BE playback was
> not valid ?

Again we had a verification that if the dpcm_playback was set at the
dailink level, it was actually supported by the dais.

We seem to have lost this verification. We only have an error when there
are no settings at all.

> 
> If so, yes indeed this patch removes such flags.
> But I wonder why you want to get it in case of DPCM only ?

It's helpful to detect invalid configurations. And I see to reason why
the removal of flags should change the functionality.

> I can understand if all case want to have it.
> 
> Then, I think we can check _only for this purpose too ?
> 
> (A)	if dai_link has playback_only        -> it should have has_playback
> (B)	if dai_link has capture_only         -> it should have has_capture
> (C)	if dai_link don't have both xxx_only -> it should have both has_xxx
> 
> I think we already have (A)(B) check. We want to add (C) check ?
> 
> If my understanding was correct, the things dpcm_xxx flag can do is also
> can do by xxx_only flag. But dpcm_xxx flag is used only DPCM, but xxx_only
> flag is used on all cases.

I am only concerned about mismatches between dailinks and dai
capabilities. The logic above is fine, but it's in the scope of the
dailink only.