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

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

[Pierre-Louis Bossart]

Previous updates to set dailink capabilities and check dailink
capabilities were based on a flawed assumption that all dais support
the same capabilities as the dailink. This is true for TDM
configurations but existing configurations use an amplifier and a
capture device on the same dailink, and the tests would prevent the
card from probing.

This patch modifies the snd_soc_dai_link_set_capabilities()
helper so that the dpcm_playback (resp. dpcm_capture) dailink
capabilities are set if at least one dai supports playback (resp. capture).

Likewise the checks are modified so that an error is reported only
when dpcm_playback (resp. dpcm_capture) is set but none of the CPU
DAIs support playback (resp. capture).

Fixes: 25612477d20b5 ('ASoC: soc-dai: set dai_link dpcm_ flags with a helper')
Fixes: b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks')
Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/soc-dai.c | 16 +++++++++-------
 sound/soc/soc-pcm.c | 42 ++++++++++++++++++++++++------------------
 2 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
index 98f0c98b06bb..ef1700e5e1bf 100644
--- a/sound/soc/soc-dai.c
+++ b/sound/soc/soc-dai.c
@@ -402,28 +402,30 @@ void snd_soc_dai_link_set_capabilities(struct snd_soc_dai_link *dai_link)
 	struct snd_soc_dai_link_component *codec;
 	struct snd_soc_dai *dai;
 	bool supported[SNDRV_PCM_STREAM_LAST + 1];
+	bool supported_cpu;
+	bool supported_codec;
 	int direction;
 	int i;
 
 	for_each_pcm_streams(direction) {
-		supported[direction] = true;
+		supported_cpu = false;
+		supported_codec = false;
 
 		for_each_link_cpus(dai_link, i, cpu) {
 			dai = snd_soc_find_dai(cpu);
-			if (!dai || !snd_soc_dai_stream_valid(dai, direction)) {
-				supported[direction] = false;
+			if (dai && snd_soc_dai_stream_valid(dai, direction)) {
+				supported_cpu = true;
 				break;
 			}
 		}
-		if (!supported[direction])
-			continue;
 		for_each_link_codecs(dai_link, i, codec) {
 			dai = snd_soc_find_dai(codec);
-			if (!dai || !snd_soc_dai_stream_valid(dai, direction)) {
-				supported[direction] = false;
+			if (dai && snd_soc_dai_stream_valid(dai, direction)) {
+				supported_codec = true;
 				break;
 			}
 		}
+		supported[direction] = supported_cpu && supported_codec;
 	}
 
 	dai_link->dpcm_playback = supported[SNDRV_PCM_STREAM_PLAYBACK];
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index f2c7c85ad40c..aac61df0c50e 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2743,30 +2743,36 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 		if (rtd->dai_link->dpcm_playback) {
 			stream = SNDRV_PCM_STREAM_PLAYBACK;
 
-			for_each_rtd_cpu_dais(rtd, i, cpu_dai)
-				if (!snd_soc_dai_stream_valid(cpu_dai,
-							      stream)) {
-					dev_err(rtd->card->dev,
-						"CPU DAI %s for rtd %s does not support playback\n",
-						cpu_dai->name,
-						rtd->dai_link->stream_name);
-					return -EINVAL;
+			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
+				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
+					playback = 1;
+					break;
 				}
-			playback = 1;
+			}
+
+			if (!playback) {
+				dev_err(rtd->card->dev,
+					"No CPU DAIs support playback for stream %s\n",
+					rtd->dai_link->stream_name);
+				return -EINVAL;
+			}
 		}
 		if (rtd->dai_link->dpcm_capture) {
 			stream = SNDRV_PCM_STREAM_CAPTURE;
 
-			for_each_rtd_cpu_dais(rtd, i, cpu_dai)
-				if (!snd_soc_dai_stream_valid(cpu_dai,
-							      stream)) {
-					dev_err(rtd->card->dev,
-						"CPU DAI %s for rtd %s does not support capture\n",
-						cpu_dai->name,
-						rtd->dai_link->stream_name);
-					return -EINVAL;
+			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
+				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
+					capture = 1;
+					break;
 				}
-			capture = 1;
+			}
+
+			if (!capture) {
+				dev_err(rtd->card->dev,
+					"No CPU DAIs support capture for stream %s\n",
+					rtd->dai_link->stream_name);
+				return -EINVAL;
+			}
 		}
 	} else {
 		/* Adapt stream for codec2codec links */
-- 
2.25.1

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

[Jerome Brunet]

On Thu 23 Jul 2020 at 20:05, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:

> Previous updates to set dailink capabilities and check dailink
> capabilities were based on a flawed assumption that all dais support
> the same capabilities as the dailink. This is true for TDM
> configurations

Actually it is not true for TDM either, having codecs with different
playback/capture caps is valid with TDM as well

> but existing configurations use an amplifier and a
> capture device on the same dailink, and the tests would prevent the
> card from probing.
>
> This patch modifies the snd_soc_dai_link_set_capabilities()
> helper so that the dpcm_playback (resp. dpcm_capture) dailink
> capabilities are set if at least one dai supports playback (resp. capture).
>
> Likewise the checks are modified so that an error is reported only
> when dpcm_playback (resp. dpcm_capture) is set but none of the CPU
> DAIs support playback (resp. capture).

This was not is the original proposal you sent ...

>
> Fixes: 25612477d20b5 ('ASoC: soc-dai: set dai_link dpcm_ flags with a helper')
> Fixes: b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks')
> Suggested-by: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/soc/soc-dai.c | 16 +++++++++-------
>  sound/soc/soc-pcm.c | 42 ++++++++++++++++++++++++------------------
>  2 files changed, 33 insertions(+), 25 deletions(-)
>
> diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
> index 98f0c98b06bb..ef1700e5e1bf 100644
> --- a/sound/soc/soc-dai.c
> +++ b/sound/soc/soc-dai.c
> @@ -402,28 +402,30 @@ void snd_soc_dai_link_set_capabilities(struct snd_soc_dai_link *dai_link)
>  	struct snd_soc_dai_link_component *codec;
>  	struct snd_soc_dai *dai;
>  	bool supported[SNDRV_PCM_STREAM_LAST + 1];
> +	bool supported_cpu;
> +	bool supported_codec;
>  	int direction;
>  	int i;
>  
>  	for_each_pcm_streams(direction) {
> -		supported[direction] = true;
> +		supported_cpu = false;
> +		supported_codec = false;
>  
>  		for_each_link_cpus(dai_link, i, cpu) {
>  			dai = snd_soc_find_dai(cpu);
> -			if (!dai || !snd_soc_dai_stream_valid(dai, direction)) {
> -				supported[direction] = false;
> +			if (dai && snd_soc_dai_stream_valid(dai, direction)) {
> +				supported_cpu = true;
>  				break;
>  			}
>  		}
> -		if (!supported[direction])
> -			continue;
>  		for_each_link_codecs(dai_link, i, codec) {
>  			dai = snd_soc_find_dai(codec);
> -			if (!dai || !snd_soc_dai_stream_valid(dai, direction)) {
> -				supported[direction] = false;
> +			if (dai && snd_soc_dai_stream_valid(dai, direction)) {
> +				supported_codec = true;
>  				break;
>  			}
>  		}
> +		supported[direction] = supported_cpu && supported_codec;
>  	}
>  
>  	dai_link->dpcm_playback = supported[SNDRV_PCM_STREAM_PLAYBACK];
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index f2c7c85ad40c..aac61df0c50e 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2743,30 +2743,36 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>  		if (rtd->dai_link->dpcm_playback) {
>  			stream = SNDRV_PCM_STREAM_PLAYBACK;
>  
> -			for_each_rtd_cpu_dais(rtd, i, cpu_dai)
> -				if (!snd_soc_dai_stream_valid(cpu_dai,
> -							      stream)) {
> -					dev_err(rtd->card->dev,
> -						"CPU DAI %s for rtd %s does not support playback\n",
> -						cpu_dai->name,
> -						rtd->dai_link->stream_name);
> -					return -EINVAL;
> +			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
> +				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
> +					playback = 1;
> +					break;
>  				}
> -			playback = 1;
> +			}
> +
> +			if (!playback) {
> +				dev_err(rtd->card->dev,
> +					"No CPU DAIs support playback for stream %s\n",
> +					rtd->dai_link->stream_name);
> +				return -EINVAL;
> +			}

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 ?

[0]: https://lore.kernel.org/r/1j1rla9s22.fsf@starbuckisacylon.baylibre.com

>  		}
>  		if (rtd->dai_link->dpcm_capture) {
>  			stream = SNDRV_PCM_STREAM_CAPTURE;
>  
> -			for_each_rtd_cpu_dais(rtd, i, cpu_dai)
> -				if (!snd_soc_dai_stream_valid(cpu_dai,
> -							      stream)) {
> -					dev_err(rtd->card->dev,
> -						"CPU DAI %s for rtd %s does not support capture\n",
> -						cpu_dai->name,
> -						rtd->dai_link->stream_name);
> -					return -EINVAL;
> +			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
> +				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
> +					capture = 1;
> +					break;
>  				}
> -			capture = 1;
> +			}
> +
> +			if (!capture) {
> +				dev_err(rtd->card->dev,
> +					"No CPU DAIs support capture for stream %s\n",
> +					rtd->dai_link->stream_name);
> +				return -EINVAL;
> +			}
>  		}
>  	} else {
>  		/* Adapt stream for codec2codec links */

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

[Pierre-Louis Bossart]

> 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 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.

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.

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

[Jerome Brunet]

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.

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

[Pierre-Louis Bossart]

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.

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

[Jerome Brunet]

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.

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

[Pierre-Louis Bossart]

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.

[PATCH] ASoC: core: restore dpcm flags semantics

[Jerome Brunet]

commit b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks')
changed dpcm_playback and dpcm_capture semantic by throwing an error if
these flags are not aligned with DAIs capabilities on the link.

The former semantic did not force the flags and DAI caps to be aligned.
The flag previously allowed card drivers to disable a stream direction on
a link (whether or not such feature is deemed useful).

With change ('ASoC: core: use less strict tests for dailink capabilities')
an error is thrown if the flags and and the DAI caps are not aligned. Those
parameters were not meant to aligned initially. No technical reason was
given about why cards should now be considered "broken" in such condition
is not met, or why it should be considered to be an improvement to enforce
that.

Forcing the flags to be aligned with DAI caps just make the information
the flag carry redundant with DAI caps, breaking a few cards along the way.

This change drops the added error conditions and restore the initial flag
semantics.

Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---

 Hi Mark,

 Because b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks')
 introduced more than one problem, the change
 "ASoC: core: use less strict tests for dailink capabilities" [0] is still
 necessary but the change of semantic remains a problem with it.

 This patch applies on top of it.

 sound/soc/soc-pcm.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 00ac1cbf6f88..2e205b738eae 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2749,13 +2749,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 					break;
 				}
 			}
-
-			if (!playback) {
-				dev_err(rtd->card->dev,
-					"No CPU DAIs support playback for stream %s\n",
-					rtd->dai_link->stream_name);
-				return -EINVAL;
-			}
 		}
 		if (rtd->dai_link->dpcm_capture) {
 			stream = SNDRV_PCM_STREAM_CAPTURE;
@@ -2766,13 +2759,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 					break;
 				}
 			}
-
-			if (!capture) {
-				dev_err(rtd->card->dev,
-					"No CPU DAIs support capture for stream %s\n",
-					rtd->dai_link->stream_name);
-				return -EINVAL;
-			}
 		}
 	} else {
 		/* Adapt stream for codec2codec links */
-- 
2.25.4

Re: [PATCH] ASoC: core: restore dpcm flags semantics

[Pierre-Louis Bossart]

On 7/29/20 10:46 AM, Jerome Brunet wrote:
> commit b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks')
> changed dpcm_playback and dpcm_capture semantic by throwing an error if
> these flags are not aligned with DAIs capabilities on the link.
> 
> The former semantic did not force the flags and DAI caps to be aligned.
> The flag previously allowed card drivers to disable a stream direction on
> a link (whether or not such feature is deemed useful).
> 
> With change ('ASoC: core: use less strict tests for dailink capabilities')
> an error is thrown if the flags and and the DAI caps are not aligned. Those
> parameters were not meant to aligned initially. No technical reason was
> given about why cards should now be considered "broken" in such condition
> is not met, or why it should be considered to be an improvement to enforce
> that.
> 
> Forcing the flags to be aligned with DAI caps just make the information
> the flag carry redundant with DAI caps, breaking a few cards along the way.
> 
> This change drops the added error conditions and restore the initial flag
> semantics.

or rather lack thereof.

I am ok to move dev_err to dev_warn and remove the return -EINVAL, but I 
maintain that we have to reach a point where configurations make sense 
before we can clean them up. If we implicitly push issues under the rug 
by not even being aware of them we'll never make progress.

> 
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
> 
>   Hi Mark,
> 
>   Because b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks')
>   introduced more than one problem, the change
>   "ASoC: core: use less strict tests for dailink capabilities" [0] is still
>   necessary but the change of semantic remains a problem with it.
> 
>   This patch applies on top of it.
> 
>   sound/soc/soc-pcm.c | 14 --------------
>   1 file changed, 14 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 00ac1cbf6f88..2e205b738eae 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2749,13 +2749,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>   					break;
>   				}
>   			}
> -
> -			if (!playback) {
> -				dev_err(rtd->card->dev,
> -					"No CPU DAIs support playback for stream %s\n",
> -					rtd->dai_link->stream_name);
> -				return -EINVAL;
> -			}
>   		}
>   		if (rtd->dai_link->dpcm_capture) {
>   			stream = SNDRV_PCM_STREAM_CAPTURE;
> @@ -2766,13 +2759,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>   					break;
>   				}
>   			}
> -
> -			if (!capture) {
> -				dev_err(rtd->card->dev,
> -					"No CPU DAIs support capture for stream %s\n",
> -					rtd->dai_link->stream_name);
> -				return -EINVAL;
> -			}
>   		}
>   	} else {
>   		/* Adapt stream for codec2codec links */
> 

Re: [PATCH] ASoC: core: restore dpcm flags semantics

[Jerome Brunet]

On Wed 29 Jul 2020 at 17:56, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:

> On 7/29/20 10:46 AM, Jerome Brunet wrote:
>> commit b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks')
>> changed dpcm_playback and dpcm_capture semantic by throwing an error if
>> these flags are not aligned with DAIs capabilities on the link.
>>
>> The former semantic did not force the flags and DAI caps to be aligned.
>> The flag previously allowed card drivers to disable a stream direction on
>> a link (whether or not such feature is deemed useful).
>>
>> With change ('ASoC: core: use less strict tests for dailink capabilities')
>> an error is thrown if the flags and and the DAI caps are not aligned. Those
>> parameters were not meant to aligned initially. No technical reason was
>> given about why cards should now be considered "broken" in such condition
>> is not met, or why it should be considered to be an improvement to enforce
>> that.
>>
>> Forcing the flags to be aligned with DAI caps just make the information
>> the flag carry redundant with DAI caps, breaking a few cards along the way.
>>
>> This change drops the added error conditions and restore the initial flag
>> semantics.
>
> or rather lack thereof.

Again, why ? All there is so far is your personal preference. no facts.

 * What we had gave capabilities to the link, independent of the DAI
   components. ASoC just computes the intersection of all that to
   determine which direction needs to be enabled. Seems rather simple
   and straight forward.
 * It worked for every user of DPCM so a far.

Your changes:
 * Causes regression
 * Makes information redundant. The code used to build the flag in
   snd_soc_dai_link_set_capabilities() and check it soc_new_pcm() is
   more or less the same. It just adds complexity and waste cycles.

I don't see the upside to it.
 
>
> I am ok to move dev_err to dev_warn and remove the return -EINVAL, but I
> maintain that we have to reach a point where configurations make sense
> before we can clean them up. If we implicitly push issues under the rug by
> not even being aware of them we'll never make progress.

But why should we bother the user with that ? How is throwing this
error/warning an improvement ? What does not make sense in the
configuration ? What are we pushing under the rug exactly ?

I'm willing to go your way, even help you out, but you need to:
 * explain concretely why changing the semantics improve the overall
   situation, concretely ?
 * update all the users of DPCM. Causing regression is not OK.

Carrying redundant information makes things complex and error prone.
If you really want to update this, here is another proposition:
 * Removing snd_soc_dai_link_set_capabilities()
 * Removing both flags completely
 * Let ASoC figure out what is needed based on the components present.

>
>>
>> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>
>>   Hi Mark,
>>
>>   Because b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks')
>>   introduced more than one problem, the change
>>   "ASoC: core: use less strict tests for dailink capabilities" [0] is still
>>   necessary but the change of semantic remains a problem with it.
>>
>>   This patch applies on top of it.
>>
>>   sound/soc/soc-pcm.c | 14 --------------
>>   1 file changed, 14 deletions(-)
>>
>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
>> index 00ac1cbf6f88..2e205b738eae 100644
>> --- a/sound/soc/soc-pcm.c
>> +++ b/sound/soc/soc-pcm.c
>> @@ -2749,13 +2749,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>>   					break;
>>   				}
>>   			}
>> -
>> -			if (!playback) {
>> -				dev_err(rtd->card->dev,
>> -					"No CPU DAIs support playback for stream %s\n",
>> -					rtd->dai_link->stream_name);
>> -				return -EINVAL;
>> -			}
>>   		}
>>   		if (rtd->dai_link->dpcm_capture) {
>>   			stream = SNDRV_PCM_STREAM_CAPTURE;
>> @@ -2766,13 +2759,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
>>   					break;
>>   				}
>>   			}
>> -
>> -			if (!capture) {
>> -				dev_err(rtd->card->dev,
>> -					"No CPU DAIs support capture for stream %s\n",
>> -					rtd->dai_link->stream_name);
>> -				return -EINVAL;
>> -			}
>>   		}
>>   	} else {
>>   		/* Adapt stream for codec2codec links */
>>

Re: [PATCH] ASoC: core: restore dpcm flags semantics

[Pierre-Louis Bossart]

On 7/30/20 4:04 AM, Jerome Brunet wrote:
> 
> On Wed 29 Jul 2020 at 17:56, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
> 
>> On 7/29/20 10:46 AM, Jerome Brunet wrote:
>>> commit b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks')
>>> changed dpcm_playback and dpcm_capture semantic by throwing an error if
>>> these flags are not aligned with DAIs capabilities on the link.
>>>
>>> The former semantic did not force the flags and DAI caps to be aligned.
>>> The flag previously allowed card drivers to disable a stream direction on
>>> a link (whether or not such feature is deemed useful).
>>>
>>> With change ('ASoC: core: use less strict tests for dailink capabilities')
>>> an error is thrown if the flags and and the DAI caps are not aligned. Those
>>> parameters were not meant to aligned initially. No technical reason was
>>> given about why cards should now be considered "broken" in such condition
>>> is not met, or why it should be considered to be an improvement to enforce
>>> that.
>>>
>>> Forcing the flags to be aligned with DAI caps just make the information
>>> the flag carry redundant with DAI caps, breaking a few cards along the way.
>>>
>>> This change drops the added error conditions and restore the initial flag
>>> semantics.
>>
>> or rather lack thereof.
> 
> Again, why ? All there is so far is your personal preference. no facts.

What would be the meaning/purpose of a dailink with .dpcm_capture set, 
with only dais that support playback only?

What would be the meaning/purpose of a dailink with .capture_only set, 
but with a dai that supports playback?

What happens if none of these flags are set?

What happens when all these flags are set?

No one seems to know, so my suggestion is to align first on consistent 
configurations, then see what can be removed.

>   * What we had gave capabilities to the link, independent of the DAI
>     components. ASoC just computes the intersection of all that to
>     determine which direction needs to be enabled. Seems rather simple
>     and straight forward.

that's what my last patch did, and when there is no intersection it 
complains. Please clarify what you expect when there is no overlap 
between dai and dailink capabilities. Keep in mind that we have a mix of 
hard-codec configuration and DT-created ones, your case is not the 
general one.

>   * It worked for every user of DPCM so a far.

Not completely true, when Morimoto-san added snd_soc_dai_stream_valid() 
it exposed tons of cases where the information on direction was not 
provided in a reliable at the DAI level. I will assert that we are still 
finding out cases with broken DAI configurations, and as a result we 
will also find broken dailink configurations. Your picture of DPCM as a 
perfectly functional system that I broke is a distortion of reality.

The reality is that we have to work in steps, first make sure all DAIs 
are properly described, then work on the dailinks and optimize at a 
later point. we will need warnings to find out what the problem cases 
are, and move slowly.

Re: [PATCH] ASoC: core: restore dpcm flags semantics

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

On Thu, Jul 30, 2020 at 11:04:53AM +0200, Jerome Brunet wrote:

> Carrying redundant information makes things complex and error prone.
> If you really want to update this, here is another proposition:
>  * Removing snd_soc_dai_link_set_capabilities()
>  * Removing both flags completely
>  * Let ASoC figure out what is needed based on the components present.

My understanding is that that was broadly where we were headed with this
stuff - snd_soc_dai_link_set_capabilities() is trying to figure things
out from the components already, it's storing the flags as a cache but
could be modified so we use it every time we need a value.

> 
> >
> >>
> >> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >> ---
> >>
> >>   Hi Mark,
> >>
> >>   Because b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks')
> >>   introduced more than one problem, the change
> >>   "ASoC: core: use less strict tests for dailink capabilities" [0] is still
> >>   necessary but the change of semantic remains a problem with it.
> >>
> >>   This patch applies on top of it.
> >>
> >>   sound/soc/soc-pcm.c | 14 --------------
> >>   1 file changed, 14 deletions(-)
> >>
> >> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> >> index 00ac1cbf6f88..2e205b738eae 100644
> >> --- a/sound/soc/soc-pcm.c
> >> +++ b/sound/soc/soc-pcm.c
> >> @@ -2749,13 +2749,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
> >>   					break;
> >>   				}
> >>   			}
> >> -
> >> -			if (!playback) {
> >> -				dev_err(rtd->card->dev,
> >> -					"No CPU DAIs support playback for stream %s\n",
> >> -					rtd->dai_link->stream_name);
> >> -				return -EINVAL;
> >> -			}
> >>   		}
> >>   		if (rtd->dai_link->dpcm_capture) {
> >>   			stream = SNDRV_PCM_STREAM_CAPTURE;
> >> @@ -2766,13 +2759,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
> >>   					break;
> >>   				}
> >>   			}
> >> -
> >> -			if (!capture) {
> >> -				dev_err(rtd->card->dev,
> >> -					"No CPU DAIs support capture for stream %s\n",
> >> -					rtd->dai_link->stream_name);
> >> -				return -EINVAL;
> >> -			}
> >>   		}
> >>   	} else {
> >>   		/* Adapt stream for codec2codec links */
> >>
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: core: restore dpcm flags semantics

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

On Thu, Jul 30, 2020 at 11:06:23AM -0500, Pierre-Louis Bossart wrote:
> On 7/30/20 4:04 AM, Jerome Brunet wrote:
> > On Wed 29 Jul 2020 at 17:56, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
> > > On 7/29/20 10:46 AM, Jerome Brunet wrote:

> > > > The flag previously allowed card drivers to disable a stream direction on
> > > > a link (whether or not such feature is deemed useful).

Right, and I can see a use case for this if someone has a board that
for some reason didn't physically connect one of the directions for some
reason - perhaps they were running out of pins or something.  It's not
clear if anyone's actually doing that though.

> > > > Forcing the flags to be aligned with DAI caps just make the information
> > > > the flag carry redundant with DAI caps, breaking a few cards along the way.

> > > > This change drops the added error conditions and restore the initial flag
> > > > semantics.

I'm not 100% clear, have we actually found cases where the flags are
used or is this something found through inspection and review?

> >   * It worked for every user of DPCM so a far.

> Not completely true, when Morimoto-san added snd_soc_dai_stream_valid() it
> exposed tons of cases where the information on direction was not provided in
> a reliable at the DAI level. I will assert that we are still finding out
> cases with broken DAI configurations, and as a result we will also find
> broken dailink configurations. Your picture of DPCM as a perfectly
> functional system that I broke is a distortion of reality.

> The reality is that we have to work in steps, first make sure all DAIs are
> properly described, then work on the dailinks and optimize at a later point.
> we will need warnings to find out what the problem cases are, and move
> slowly.

This was all triggered by Morimoto-san's changes like you say.  DPCM has
quite a lot of problems in general, here IIRC the issues were that we
had multiple different ways of doing similar things which it wasn't
quite clear if people were even using.  The intention with the warnings
was to remove them one way or another, they're mainly intended to flush
out actual active usage of the flags as opposed to redundant usage of
them which could be confused/broken.

This could definitely have been clearer in the changelogs though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: core: restore dpcm flags semantics

[Jerome Brunet]

On Thu 30 Jul 2020 at 18:06, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:

> On 7/30/20 4:04 AM, Jerome Brunet wrote:
>>
>> On Wed 29 Jul 2020 at 17:56, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
>>
>>> On 7/29/20 10:46 AM, Jerome Brunet wrote:
>>>> commit b73287f0b0745 ('ASoC: soc-pcm: dpcm: fix playback/capture checks')
>>>> changed dpcm_playback and dpcm_capture semantic by throwing an error if
>>>> these flags are not aligned with DAIs capabilities on the link.
>>>>
>>>> The former semantic did not force the flags and DAI caps to be aligned.
>>>> The flag previously allowed card drivers to disable a stream direction on
>>>> a link (whether or not such feature is deemed useful).
>>>>
>>>> With change ('ASoC: core: use less strict tests for dailink capabilities')
>>>> an error is thrown if the flags and and the DAI caps are not aligned. Those
>>>> parameters were not meant to aligned initially. No technical reason was
>>>> given about why cards should now be considered "broken" in such condition
>>>> is not met, or why it should be considered to be an improvement to enforce
>>>> that.
>>>>
>>>> Forcing the flags to be aligned with DAI caps just make the information
>>>> the flag carry redundant with DAI caps, breaking a few cards along the way.
>>>>
>>>> This change drops the added error conditions and restore the initial flag
>>>> semantics.
>>>
>>> or rather lack thereof.
>>
>> Again, why ? All there is so far is your personal preference. no facts.
>
> What would be the meaning/purpose of a dailink with .dpcm_capture set, with
> only dais that support playback only?
>
> What would be the meaning/purpose of a dailink with .capture_only set, but
> with a dai that supports playback?

You get to throw an error in those case

>
> What happens if none of these flags are set?

I think I already suggested to throw an error in the initial review of
your patch

>
> What happens when all these flags are set?

I don't see the problem here

>
> No one seems to know, so my suggestion is to align first on consistent
> configurations, then see what can be removed.
>
>>   * What we had gave capabilities to the link, independent of the DAI
>>     components. ASoC just computes the intersection of all that to
>>     determine which direction needs to be enabled. Seems rather simple
>>     and straight forward.
>
> that's what my last patch did, and when there is no intersection it
> complains. Please clarify what you expect when there is no overlap between
> dai and dailink capabilities. Keep in mind that we have a mix of hard-codec
> configuration and DT-created ones, your case is not the general one.
>
>>   * It worked for every user of DPCM so a far.
>
> Not completely true, when Morimoto-san added snd_soc_dai_stream_valid() it
> exposed tons of cases where the information on direction was not provided
> in a reliable at the DAI level. I will assert that we are still finding out
> cases with broken DAI configurations, and as a result we will also find
> broken dailink configurations. Your picture of DPCM as a perfectly
> functional system that I broke is a distortion of reality.

If it was not working, it was certainly not clear in the changelog.
What's clear is the regression it caused

>
> The reality is that we have to work in steps, first make sure all DAIs are
> properly described, then work on the dailinks and optimize at a later
> point. we will need warnings to find out what the problem cases are, and
> move slowly.

Sure, have it your way

Re: [PATCH] ASoC: core: restore dpcm flags semantics

[Jerome Brunet]

On Thu 30 Jul 2020 at 20:52, Mark Brown <broonie@kernel.org> wrote:

> On Thu, Jul 30, 2020 at 11:06:23AM -0500, Pierre-Louis Bossart wrote:
>> On 7/30/20 4:04 AM, Jerome Brunet wrote:
>> > On Wed 29 Jul 2020 at 17:56, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
>> > > On 7/29/20 10:46 AM, Jerome Brunet wrote:
>
>> > > > The flag previously allowed card drivers to disable a stream direction on
>> > > > a link (whether or not such feature is deemed useful).
>
> Right, and I can see a use case for this if someone has a board that
> for some reason didn't physically connect one of the directions for some
> reason - perhaps they were running out of pins or something.  It's not
> clear if anyone's actually doing that though.
>
>> > > > Forcing the flags to be aligned with DAI caps just make the information
>> > > > the flag carry redundant with DAI caps, breaking a few cards along the way.
>
>> > > > This change drops the added error conditions and restore the initial flag
>> > > > semantics.
>
> I'm not 100% clear, have we actually found cases where the flags are
> used or is this something found through inspection and review?

One last thing I'd like to understand. Is this behavior of throwing an
error going to applied to the non-DPCM case as well ? so at least thing
are consistent between both cases ?

IOW:
 * An error is now throw if dpcm_capture is set on the link and the CPU
 DAI support playback_only
 * on non-DPCM links, will an error be thrown as well if playback_only
 is not set and the CPU on the link happen to not support capture ?
 
>
>> >   * It worked for every user of DPCM so a far.
>
>> Not completely true, when Morimoto-san added snd_soc_dai_stream_valid() it
>> exposed tons of cases where the information on direction was not provided in
>> a reliable at the DAI level. I will assert that we are still finding out
>> cases with broken DAI configurations, and as a result we will also find
>> broken dailink configurations. Your picture of DPCM as a perfectly
>> functional system that I broke is a distortion of reality.
>
>> The reality is that we have to work in steps, first make sure all DAIs are
>> properly described, then work on the dailinks and optimize at a later point.
>> we will need warnings to find out what the problem cases are, and move
>> slowly.
>
> This was all triggered by Morimoto-san's changes like you say.  DPCM has
> quite a lot of problems in general, here IIRC the issues were that we
> had multiple different ways of doing similar things which it wasn't
> quite clear if people were even using.  The intention with the warnings
> was to remove them one way or another, they're mainly intended to flush
> out actual active usage of the flags as opposed to redundant usage of
> them which could be confused/broken.
>
> This could definitely have been clearer in the changelogs though.

Re: [PATCH] ASoC: core: restore dpcm flags semantics

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On Fri, Jul 31, 2020 at 02:16:43PM +0200, Jerome Brunet wrote:

> One last thing I'd like to understand. Is this behavior of throwing an
> error going to applied to the non-DPCM case as well ? so at least thing
> are consistent between both cases ?

> IOW:
>  * An error is now throw if dpcm_capture is set on the link and the CPU
>  DAI support playback_only

We should definitely complain about that.

>  * on non-DPCM links, will an error be thrown as well if playback_only
>  is not set and the CPU on the link happen to not support capture ?

I think we should move towards not needing to do that for DPCM.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

On Thu, 23 Jul 2020 13:05:33 -0500, Pierre-Louis Bossart wrote:
> Previous updates to set dailink capabilities and check dailink
> capabilities were based on a flawed assumption that all dais support
> the same capabilities as the dailink. This is true for TDM
> configurations but existing configurations use an amplifier and a
> capture device on the same dailink, and the tests would prevent the
> card from probing.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: core: use less strict tests for dailink capabilities
      commit: 4f8721542f7b75954bfad98c51aa59d683d35b50

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark