Re: [PATCH] ASoC: core: use less strict tests for dailink capabilities

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

On 7/27/20 4:42 AM, Jerome Brunet wrote:
> 
> On Fri 24 Jul 2020 at 21:05, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
> 
>>> Again, this is changing the original meaning of the flag from "playback
>>> allowed" to "playback required".
>>>
>>> This patch (or the orignal) does not explain why this change of meaning
>>> is necessary ? The point I was making here [0] still stands.
>>>
>>> If your evil plan is to get rid of 2 of the 4 flags, why go through the
>>> trouble of the changing the meaning and effect of one them ?
>>
>> My intent was to have a non-ambiguous definition.
> 
> I still fail to understand how it was ambiguous and how throwing an
> error for something that used to work well so far is making things better.
> 
> Maybe there could be have been a better name for it, but what it did was
> clear.
> 
> The flag is even (briefly) documented:
> 	/* DPCM capture and Playback support */
> 	unsigned int dpcm_capture:1;
> 	unsigned int dpcm_playback:1;
> 
> "Support" means the dai_link supports it, not that it is required for it
> work. This is what was implemented.
> 
>>
>> I don't know 'playback allowed' means. What is the point of using this flag
>> if it may or may not accurately describe what is actually implemented? And
>> how can we converge the use of flags since in the contrary 'playback_only'
>> is actually a clear indication of what the link does. We've got to align on
>> the semantics, and I really don't see the point of watering-down
>> definitions. When things are optional or poorly defined, the confusion
>> continues.
> 
> The problem is that commit b73287f0b074 ("ASoC: soc-pcm: dpcm: fix
> playback/capture checks") has changed the semantic:
> * without actually warning that it was doing so in the commit description
> * breaking things for other who relied on the previous semantics
> 
> Previous semantics of the flag allowed to disable a stream direction on
> a link which could have otherwise had it working, if the stream had it.
> It added information/control on the link at least.
> 
> New flag semantics forces the flag and stream capabilities to be somehow
> aligned. This is not clearing the confusion, this is redundant
> information. How is this helping the framework or the users ?
> 
>>
>> WFIW, my 'evil' plan was to rename 'dpcm_playback' as 'can_playback' (same
>> for capture) and replace 'playback_only' by 'can_playback = 1; can_capture
>> = 0'. So this first step was really to align them on the expected behavior
>> and minimal requirements.
> 
> IMO the previous flag semantics was inverted yes, but aligned:
> 
> playback_only = 1 was the same as dpcm_capture = 0
> capture_only = 1 was the same as dpcm_playback = 0
> 
> Having both *_only set does not make sense for a stream, same as having
> none of dpcm_*
> 
> Having none of *_only flag means there is no restriction on the stream,
> same as having both dpcm_* set.
> 
> This seems aligned to me.

Makes no sense to me to have information that's useless. What does 'no 
restrictions' on a stream mean? 'anything goes' is not a scalable design 
principle.