[PATCH 0/5] ASoC: minor cleanup for soc_get_playback_capture()

[PATCH 0/5] ASoC: minor cleanup for soc_get_playback_capture()

[Kuninori Morimoto]

Hi Mark

This is minor cleanup patches for soc_get_playback_capture().

Link: https://lore.kernel.org/r/87ttw1gqgn.wl-kuninori.morimoto.gx@renesas.com
Link: https://lore.kernel.org/r/87353uqjiu.wl-kuninori.morimoto.gx@renesas.com
Link: https://lore.kernel.org/r/ab3f0c0a-62fd-a468-b3cf-0e4b59bac6ae@linux.intel.com

Kuninori Morimoto (5):
  ASoC: soc-pcm.c: indicate error if stream has no playback no capture
  ASoC: soc-pcm.c: use dai_link on soc_get_playback_capture()
  ASoC: soc-pcm.c: cleanup soc_get_playback_capture() error
  ASoC: soc-pcm.c: use temporary variable at soc_get_playback_capture()
  ASoC: soc-pcm.c: tidyup playback/capture_only at soc_get_playback_capture()

 sound/soc/soc-pcm.c | 58 ++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH 1/5] ASoC: soc-pcm.c: indicate error if stream has no playback no capture

[Kuninori Morimoto]

soc_get_playback_capture() (A) returns number of substreams for
playback/capture (B).
ASoC will probe the Sound Card and mapps CPU<->Codec pair.

(A)	static int soc_get_playback_capture(...,
(B)				    int *playback, int *capture)
	{
		...
		if (rtd->dai_link->playback_only) {
			*playback = 1;
			*capture = 0;
		}

		if (rtd->dai_link->capture_only) {
			*playback = 0;
			*capture = 1;
		}
(C)
		return 0;
	}

But it might be no playback no capture if it returns playback=0, capture=0.
It is very difficult to notice about it. This patch indicates error at (C)
then.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/soc-pcm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 7247f44faa1c..fe65994485f8 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2815,6 +2815,13 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 		*capture = 1;
 	}
 
+	if (!*playback && !*capture) {
+		dev_err(rtd->dev, "substream %s has no playback, no capture\n",
+			rtd->dai_link->stream_name);
+
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH 2/5] ASoC: soc-pcm.c: use dai_link on soc_get_playback_capture()

[Kuninori Morimoto]

soc_get_playback_capture() (A) is using rtd->dai_link->xxx everywhere.
Because of that, 1 line is unnecessarily long and not readable.

(A)	static int soc_get_playback_capture(...)
	{
		if (rtd->dai_link->dynamic ...) {
		    ^^^^^^^^^^^^^
			...
		} else {
			int cpu_capture = rtd->dai_link->c2c_params ?
					  ^^^^^^^^^^^^^
			...
		}

		if (rtd->dai_link->playback_only) {
		    ^^^^^^^^^^^^^
			...
		}
		...
	}

This patch uses variable "dai_link" to be clear code.
Nothing changes the meanings.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/soc-pcm.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index fe65994485f8..db3fbe1af2ce 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2731,19 +2731,20 @@ static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream)
 static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 				    int *playback, int *capture)
 {
+	struct snd_soc_dai_link *dai_link = rtd->dai_link;
 	struct snd_soc_dai *cpu_dai;
 	int i;
 
-	if (rtd->dai_link->dynamic && rtd->dai_link->num_cpus > 1) {
+	if (dai_link->dynamic && dai_link->num_cpus > 1) {
 		dev_err(rtd->dev,
 			"DPCM doesn't support Multi CPU for Front-Ends yet\n");
 		return -EINVAL;
 	}
 
-	if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) {
+	if (dai_link->dynamic || dai_link->no_pcm) {
 		int stream;
 
-		if (rtd->dai_link->dpcm_playback) {
+		if (dai_link->dpcm_playback) {
 			stream = SNDRV_PCM_STREAM_PLAYBACK;
 
 			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
@@ -2755,11 +2756,11 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 			if (!*playback) {
 				dev_err(rtd->card->dev,
 					"No CPU DAIs support playback for stream %s\n",
-					rtd->dai_link->stream_name);
+					dai_link->stream_name);
 				return -EINVAL;
 			}
 		}
-		if (rtd->dai_link->dpcm_capture) {
+		if (dai_link->dpcm_capture) {
 			stream = SNDRV_PCM_STREAM_CAPTURE;
 
 			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
@@ -2772,7 +2773,7 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 			if (!*capture) {
 				dev_err(rtd->card->dev,
 					"No CPU DAIs support capture for stream %s\n",
-					rtd->dai_link->stream_name);
+					dai_link->stream_name);
 				return -EINVAL;
 			}
 		}
@@ -2780,15 +2781,15 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 		struct snd_soc_dai *codec_dai;
 
 		/* Adapt stream for codec2codec links */
-		int cpu_capture = rtd->dai_link->c2c_params ?
+		int cpu_capture = dai_link->c2c_params ?
 			SNDRV_PCM_STREAM_PLAYBACK : SNDRV_PCM_STREAM_CAPTURE;
-		int cpu_playback = rtd->dai_link->c2c_params ?
+		int cpu_playback = dai_link->c2c_params ?
 			SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
 
 		for_each_rtd_codec_dais(rtd, i, codec_dai) {
-			if (rtd->dai_link->num_cpus == 1) {
+			if (dai_link->num_cpus == 1) {
 				cpu_dai = asoc_rtd_to_cpu(rtd, 0);
-			} else if (rtd->dai_link->num_cpus == rtd->dai_link->num_codecs) {
+			} else if (dai_link->num_cpus == dai_link->num_codecs) {
 				cpu_dai = asoc_rtd_to_cpu(rtd, i);
 			} else {
 				dev_err(rtd->card->dev,
@@ -2805,19 +2806,19 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 		}
 	}
 
-	if (rtd->dai_link->playback_only) {
+	if (dai_link->playback_only) {
 		*playback = 1;
 		*capture = 0;
 	}
 
-	if (rtd->dai_link->capture_only) {
+	if (dai_link->capture_only) {
 		*playback = 0;
 		*capture = 1;
 	}
 
 	if (!*playback && !*capture) {
 		dev_err(rtd->dev, "substream %s has no playback, no capture\n",
-			rtd->dai_link->stream_name);
+			dai_link->stream_name);
 
 		return -EINVAL;
 	}
-- 
2.25.1

[PATCH 3/5] ASoC: soc-pcm.c: cleanup soc_get_playback_capture() error

[Kuninori Morimoto]

soc_get_playback_capture() (A) checks dai_link status, and indicate error
if it was not matching (B).

(A)	static int soc_get_playback_capture(...)
	{
		...
 ^		if (dai_link->dynamic && dai_link->num_cpus > 1) {
 |			dev_err(rtd->dev,
(B)				"DPCM doesn't support Multi CPU for Front-Ends yet\n");
 |			return -EINVAL;
 v		}
		...
	}

We can use 100 char for 1 line today. This patch cleanup error code line.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/soc-pcm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index db3fbe1af2ce..47da3be0ff46 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2736,8 +2736,7 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 	int i;
 
 	if (dai_link->dynamic && dai_link->num_cpus > 1) {
-		dev_err(rtd->dev,
-			"DPCM doesn't support Multi CPU for Front-Ends yet\n");
+		dev_err(rtd->dev, "DPCM doesn't support Multi CPU for Front-Ends yet\n");
 		return -EINVAL;
 	}
 
-- 
2.25.1

[PATCH 4/5] ASoC: soc-pcm.c: use temporary variable at soc_get_playback_capture()

[Kuninori Morimoto]

soc_get_playback_capture() (A) returns number of substreams for
playback/capture (B).

(A)	static int soc_get_playback_capture(...,
(B)					int *playback, int *capture)
	{
		...
		for_each_xxx(...) {
			if (xxx)
				return -EINVAL;
=>			*playback = 1;
			...
=>			*capture = 1;
			...
		}
		...
	}

But, it is directly updating playback/capture which is the result of this
function even though it might be error. It should be updated in case of
succeed only. This patch updates it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/soc-pcm.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 47da3be0ff46..b3d569e7ba61 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2733,6 +2733,8 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 {
 	struct snd_soc_dai_link *dai_link = rtd->dai_link;
 	struct snd_soc_dai *cpu_dai;
+	int has_playback = 0;
+	int has_capture  = 0;
 	int i;
 
 	if (dai_link->dynamic && dai_link->num_cpus > 1) {
@@ -2748,11 +2750,11 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 
 			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
 				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
-					*playback = 1;
+					has_playback = 1;
 					break;
 				}
 			}
-			if (!*playback) {
+			if (!has_playback) {
 				dev_err(rtd->card->dev,
 					"No CPU DAIs support playback for stream %s\n",
 					dai_link->stream_name);
@@ -2764,12 +2766,12 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 
 			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
 				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
-					*capture = 1;
+					has_capture = 1;
 					break;
 				}
 			}
 
-			if (!*capture) {
+			if (!has_capture) {
 				dev_err(rtd->card->dev,
 					"No CPU DAIs support capture for stream %s\n",
 					dai_link->stream_name);
@@ -2798,30 +2800,33 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 
 			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) &&
 			    snd_soc_dai_stream_valid(cpu_dai,   cpu_playback))
-				*playback = 1;
+				has_playback = 1;
 			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) &&
 			    snd_soc_dai_stream_valid(cpu_dai,   cpu_capture))
-				*capture = 1;
+				has_capture = 1;
 		}
 	}
 
 	if (dai_link->playback_only) {
-		*playback = 1;
-		*capture = 0;
+		has_playback = 1;
+		has_capture = 0;
 	}
 
 	if (dai_link->capture_only) {
-		*playback = 0;
-		*capture = 1;
+		has_playback = 0;
+		has_capture = 1;
 	}
 
-	if (!*playback && !*capture) {
+	if (!has_playback && !has_capture) {
 		dev_err(rtd->dev, "substream %s has no playback, no capture\n",
 			dai_link->stream_name);
 
 		return -EINVAL;
 	}
 
+	*playback = has_playback;
+	*capture  = has_capture;
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH 4/5] ASoC: soc-pcm.c: use temporary variable at soc_get_playback_capture()

[Amadeusz Sławiński]

On 5/30/2023 2:50 AM, Kuninori Morimoto wrote:
> soc_get_playback_capture() (A) returns number of substreams for
> playback/capture (B).
> 
> (A)	static int soc_get_playback_capture(...,
> (B)					int *playback, int *capture)
> 	{
> 		...
> 		for_each_xxx(...) {
> 			if (xxx)
> 				return -EINVAL;
> =>			*playback = 1;
> 			...
> =>			*capture = 1;
> 			...
> 		}
> 		...
> 	}
> 
> But, it is directly updating playback/capture which is the result of this
> function even though it might be error. It should be updated in case of
> succeed only. This patch updates it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>   sound/soc/soc-pcm.c | 27 ++++++++++++++++-----------
>   1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 47da3be0ff46..b3d569e7ba61 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2733,6 +2733,8 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
>   {
>   	struct snd_soc_dai_link *dai_link = rtd->dai_link;
>   	struct snd_soc_dai *cpu_dai;
> +	int has_playback = 0;
> +	int has_capture  = 0;
>   	int i;
>   
>   	if (dai_link->dynamic && dai_link->num_cpus > 1) {
> @@ -2748,11 +2750,11 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
>   
>   			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
>   				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
> -					*playback = 1;
> +					has_playback = 1;
>   					break;
>   				}
>   			}
> -			if (!*playback) {
> +			if (!has_playback) {
>   				dev_err(rtd->card->dev,
>   					"No CPU DAIs support playback for stream %s\n",
>   					dai_link->stream_name);
> @@ -2764,12 +2766,12 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
>   
>   			for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
>   				if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
> -					*capture = 1;
> +					has_capture = 1;
>   					break;
>   				}
>   			}
>   
> -			if (!*capture) {
> +			if (!has_capture) {
>   				dev_err(rtd->card->dev,
>   					"No CPU DAIs support capture for stream %s\n",
>   					dai_link->stream_name);
> @@ -2798,30 +2800,33 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
>   
>   			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) &&
>   			    snd_soc_dai_stream_valid(cpu_dai,   cpu_playback))
> -				*playback = 1;
> +				has_playback = 1;
>   			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) &&
>   			    snd_soc_dai_stream_valid(cpu_dai,   cpu_capture))
> -				*capture = 1;
> +				has_capture = 1;
>   		}
>   	}
>   
>   	if (dai_link->playback_only) {
> -		*playback = 1;
> -		*capture = 0;
> +		has_playback = 1;
> +		has_capture = 0;
>   	}
>   
>   	if (dai_link->capture_only) {
> -		*playback = 0;
> -		*capture = 1;
> +		has_playback = 0;
> +		has_capture = 1;
>   	}
>   
> -	if (!*playback && !*capture) {
> +	if (!has_playback && !has_capture) {
>   		dev_err(rtd->dev, "substream %s has no playback, no capture\n",
>   			dai_link->stream_name);
>   
>   		return -EINVAL;
>   	}
>   
> +	*playback = has_playback;
> +	*capture  = has_capture;
> +
>   	return 0;
>   }
>   

Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

[PATCH 5/5] ASoC: soc-pcm.c: tidyup playback/capture_only at soc_get_playback_capture()

[Kuninori Morimoto]

soc_get_playback_capture() (A) returns number of substreams for
playback/capture, and then, we can use playback/capture_only flag (X)(Y).

(A)     static int soc_get_playback_capture(...)
	{
		...
(X)		if (dai_link->playback_only) {
(*)			*playback = 1;
			*capture = 0;
		}

(Y)		if (dai_link->capture_only) {
			*playback = 0;
(*)			*capture = 1;
		}
		...
	}

But this flag should not have effect to opposite side stream (*).
This patch tidyup it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/soc-pcm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index b3d569e7ba61..159670612de3 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2807,15 +2807,11 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 		}
 	}
 
-	if (dai_link->playback_only) {
-		has_playback = 1;
+	if (dai_link->playback_only)
 		has_capture = 0;
-	}
 
-	if (dai_link->capture_only) {
+	if (dai_link->capture_only)
 		has_playback = 0;
-		has_capture = 1;
-	}
 
 	if (!has_playback && !has_capture) {
 		dev_err(rtd->dev, "substream %s has no playback, no capture\n",
-- 
2.25.1

Re: [PATCH 0/5] ASoC: minor cleanup for soc_get_playback_capture()

[Pierre-Louis Bossart]

> This is minor cleanup patches for soc_get_playback_capture().
> 
> Link: https://lore.kernel.org/r/87ttw1gqgn.wl-kuninori.morimoto.gx@renesas.com
> Link: https://lore.kernel.org/r/87353uqjiu.wl-kuninori.morimoto.gx@renesas.com
> Link: https://lore.kernel.org/r/ab3f0c0a-62fd-a468-b3cf-0e4b59bac6ae@linux.intel.com
> 
> Kuninori Morimoto (5):
>   ASoC: soc-pcm.c: indicate error if stream has no playback no capture
>   ASoC: soc-pcm.c: use dai_link on soc_get_playback_capture()
>   ASoC: soc-pcm.c: cleanup soc_get_playback_capture() error
>   ASoC: soc-pcm.c: use temporary variable at soc_get_playback_capture()
>   ASoC: soc-pcm.c: tidyup playback/capture_only at soc_get_playback_capture()

Very nice cleanup, thank you Morimoto-san for splitting the steps in
different patches to make the changes simple to identify.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

FWIW test results can be found at the following link (no new issues
found) https://github.com/thesofproject/linux/pull/4392

Re: [PATCH 0/5] ASoC: minor cleanup for soc_get_playback_capture()

[Mark Brown]

On Tue, 30 May 2023 00:49:27 +0000, Kuninori Morimoto wrote:
> This is minor cleanup patches for soc_get_playback_capture().
> 
> Link: https://lore.kernel.org/r/87ttw1gqgn.wl-kuninori.morimoto.gx@renesas.com
> Link: https://lore.kernel.org/r/87353uqjiu.wl-kuninori.morimoto.gx@renesas.com
> Link: https://lore.kernel.org/r/ab3f0c0a-62fd-a468-b3cf-0e4b59bac6ae@linux.intel.com
> 
> Kuninori Morimoto (5):
>   ASoC: soc-pcm.c: indicate error if stream has no playback no capture
>   ASoC: soc-pcm.c: use dai_link on soc_get_playback_capture()
>   ASoC: soc-pcm.c: cleanup soc_get_playback_capture() error
>   ASoC: soc-pcm.c: use temporary variable at soc_get_playback_capture()
>   ASoC: soc-pcm.c: tidyup playback/capture_only at soc_get_playback_capture()
> 
> [...]

Applied to

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

Thanks!

[1/5] ASoC: soc-pcm.c: indicate error if stream has no playback no capture
      commit: 092830cf550667d5fa6286605167d232f2c1f61e
[2/5] ASoC: soc-pcm.c: use dai_link on soc_get_playback_capture()
      commit: cfcb31c456b15e298f88fb5ebedf7b32b009d32d
[3/5] ASoC: soc-pcm.c: cleanup soc_get_playback_capture() error
      commit: a1c0221fa5baeae6c9dc30294c2c6d01f1f4379b
[4/5] ASoC: soc-pcm.c: use temporary variable at soc_get_playback_capture()
      commit: c3e9b6d6ef5a0a3e841c3aa29e7afc48a0b73806
[5/5] ASoC: soc-pcm.c: tidyup playback/capture_only at soc_get_playback_capture()
      commit: e1f653ce847bab7285dd135cabe3ce544e574c75

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