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

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

On 7/27/20 10:15 AM, Jerome Brunet wrote:
> 
> On Mon 27 Jul 2020 at 16:13, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
> 
>> 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.
> 
> Maybe. That's not point
> The point is
> * No explanation has been provided so far about why throwing an error
>    like done here (or in the previous change) makes it more usefull.
>    The semantic change just make it redundant with the information
>    coming from the DAI caps. The new semantic makes the flag even more
>    useless.
>    
> * Throwing an error like break cards that used to work nicely for no
>    gain
>    
> * This adds yet another level of complexity that was not necessary
>    before (snd_soc_dai_link_set_capabilities())
> 
>> What does 'no restrictions' on a stream mean?
> 
> I thought the code was fairly simple but I can explain
> - A dai_link has 2 stream directions. The direction can be enabled
>    if the DAIs on the link supports it.
> - A direction could be forcefully disabled at the dai_link level using
>    those flags (restrict the direction). I suppose to give more control
>    to the card driver.
> 
> I did not write that code, I have no idea if those flags are any use to
> anyone.
> 
>> 'anything goes' is not a scalable design principle.
> 
> What does scalability has to do with the matter ?
> 
> In the end, I'm just asking to drop the error condition you added.
> 
> You want to rework/remove some flags, I think it is a great idea.
> I even willing to help out, but not in a way that makes things complex
> and redundant.

Not going to remove that check, sorry. That would allow for broken 
configuration to keep existing forever.
Over and out.