[PATCH v2 0/8] ASoC: soc-pcm: cleanup each functions

[PATCH v2 0/8] ASoC: soc-pcm: cleanup each functions

[Kuninori Morimoto]

Hi Mark

These are v2 of soc-pcm cleanup patches.
These has no relationship to each other.
My 1 concern is [3/8] patch. I think it is no problem,
but I'm not 100% sure why current code was such code.

v1 -> v2
	- soc_cpu/codec_dai_name() is now inline function
	- rename soc_pcm_care_symmetry() to soc_pcm_update_symmetry()

Link: https://lore.kernel.org/r/87tupuqqc8.wl-kuninori.morimoto.gx@renesas.com

Kuninori Morimoto (8):
  ASoC: soc-pcm: check DAI activity under soc_pcm_apply_symmetry()
  ASoC: soc-pcm: add soc_cpu/codec_dai_name() macro
  ASoC: soc-pcm: direct copy at snd_soc_set_runtime_hwparams()
  ASoC: soc-pcm: add soc_pcm_update_symmetry()
  ASoC: soc-pcm: add soc_hw_sanity_check()
  ASoC: soc-pcm: fixup dpcm_be_dai_startup() user count
  ASoC: soc-pcm: remove unneeded !rtd->dai_link check
  ASoC: soc-pcm: share DPCM BE DAI stop operation

 include/sound/soc-dpcm.h |   8 +-
 sound/soc/soc-compress.c |   2 +-
 sound/soc/soc-pcm.c      | 243 ++++++++++++++++-----------------------
 3 files changed, 105 insertions(+), 148 deletions(-)

-- 
2.25.1

[PATCH 1/8] ASoC: soc-pcm: check DAI activity under soc_pcm_apply_symmetry()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

soc_pcm_apply_symmetry() is used like below in all cases.

	if (snd_soc_dai_active(dai)) {
		err = soc_pcm_apply_symmetry(fe_substream, dai);
		...
	}

Because of this style, the code is deep nested.
This patch checks it under soc_pcm_apply_symmetry(), and makes code simple.

	static int soc_pcm_apply_symmetry(...)
	{
		...
=>		if (!snd_soc_dai_active(...))
			return 0;
		...
	}

=>	ret = soc_pcm_apply_symmetry();
	if (ret < 0)
		...

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index ba8ffbf8a5d3..9b5ab7a05f65 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -349,6 +349,9 @@ static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream,
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
 	int ret;
 
+	if (!snd_soc_dai_active(soc_dai))
+		return 0;
+
 #define __soc_pcm_apply_symmetry(name, NAME)				\
 	if (soc_dai->name && (soc_dai->driver->symmetric_##name ||	\
 			      rtd->dai_link->symmetric_##name)) {	\
@@ -765,11 +768,9 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 
 	/* Symmetry only applies if we've already got an active stream. */
 	for_each_rtd_dais(rtd, i, dai) {
-		if (snd_soc_dai_active(dai)) {
-			ret = soc_pcm_apply_symmetry(substream, dai);
-			if (ret != 0)
-				goto err;
-		}
+		ret = soc_pcm_apply_symmetry(substream, dai);
+		if (ret != 0)
+			goto err;
 	}
 
 	pr_debug("ASoC: %s <-> %s info:\n",
@@ -1693,11 +1694,9 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
 
 	for_each_rtd_cpu_dais (fe, i, fe_cpu_dai) {
 		/* Symmetry only applies if we've got an active stream. */
-		if (snd_soc_dai_active(fe_cpu_dai)) {
-			err = soc_pcm_apply_symmetry(fe_substream, fe_cpu_dai);
-			if (err < 0)
-				return err;
-		}
+		err = soc_pcm_apply_symmetry(fe_substream, fe_cpu_dai);
+		if (err < 0)
+			return err;
 	}
 
 	/* apply symmetry for BE */
@@ -1721,11 +1720,9 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
 
 		/* Symmetry only applies if we've got an active stream. */
 		for_each_rtd_dais(rtd, i, dai) {
-			if (snd_soc_dai_active(dai)) {
-				err = soc_pcm_apply_symmetry(fe_substream, dai);
-				if (err < 0)
-					return err;
-			}
+			err = soc_pcm_apply_symmetry(fe_substream, dai);
+			if (err < 0)
+				return err;
 		}
 	}
 
-- 
2.25.1

[PATCH 2/8] ASoC: soc-pcm: add soc_cpu/codec_dai_name() macro

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

soc-pcm needs DAI name and it will be "multicpu/multicodec" if it has
many DAIs. But current code is using very verbose for it.
This patch uses macro and makes code simple.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 9b5ab7a05f65..60e688b103d8 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -29,6 +29,15 @@
 
 #define DPCM_MAX_BE_USERS	8
 
+static inline const char *soc_cpu_dai_name(struct snd_soc_pcm_runtime *rtd)
+{
+	return (rtd)->num_cpus == 1 ? asoc_rtd_to_cpu(rtd, 0)->name : "multicpu";
+}
+static inline const char *soc_codec_dai_name(struct snd_soc_pcm_runtime *rtd)
+{
+	return (rtd)->num_codecs == 1 ? asoc_rtd_to_codec(rtd, 0)->name : "multicodec";
+}
+
 #ifdef CONFIG_DEBUG_FS
 static const char *dpcm_state_string(enum snd_soc_dpcm_state state)
 {
@@ -697,8 +706,8 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_soc_component *component;
 	struct snd_soc_dai *dai;
-	const char *codec_dai_name = "multicodec";
-	const char *cpu_dai_name = "multicpu";
+	const char *codec_dai_name = soc_codec_dai_name(rtd);
+	const char *cpu_dai_name = soc_cpu_dai_name(rtd);
 	int i, ret = 0;
 
 	for_each_rtd_components(rtd, i, component)
@@ -737,12 +746,6 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	/* Check that the codec and cpu DAIs are compatible */
 	soc_pcm_init_runtime_hw(substream);
 
-	if (rtd->num_codecs == 1)
-		codec_dai_name = asoc_rtd_to_codec(rtd, 0)->name;
-
-	if (rtd->num_cpus == 1)
-		cpu_dai_name = asoc_rtd_to_cpu(rtd, 0)->name;
-
 	if (soc_pcm_has_symmetry(substream))
 		runtime->hw.info |= SNDRV_PCM_INFO_JOINT_DUPLEX;
 
@@ -2741,8 +2744,7 @@ static int soc_create_pcm(struct snd_pcm **pcm,
 		else
 			snprintf(new_name, sizeof(new_name), "%s %s-%d",
 				rtd->dai_link->stream_name,
-				(rtd->num_codecs > 1) ?
-				"multicodec" : asoc_rtd_to_codec(rtd, 0)->name, num);
+				soc_codec_dai_name(rtd), num);
 
 		ret = snd_pcm_new(rtd->card->snd_card, new_name, num, playback,
 			capture, pcm);
@@ -2841,8 +2843,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 	pcm->no_device_suspend = true;
 out:
 	dev_dbg(rtd->card->dev, "%s <-> %s mapping ok\n",
-		(rtd->num_codecs > 1) ? "multicodec" : asoc_rtd_to_codec(rtd, 0)->name,
-		(rtd->num_cpus > 1)   ? "multicpu"   : asoc_rtd_to_cpu(rtd, 0)->name);
+		soc_codec_dai_name(rtd), soc_cpu_dai_name(rtd));
 	return ret;
 }
 
-- 
2.25.1

[PATCH 3/8] ASoC: soc-pcm: direct copy at snd_soc_set_runtime_hwparams()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

snd_soc_set_runtime_hwparams() is called from each driver
to initialize hw parameters,
but coping each parameters one-by-one.

Current code is not copying all parameters, but no big effect
if we do it. This patch copies all parameters by simple code.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 60e688b103d8..6f2de27cf18f 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -300,15 +300,8 @@ bool snd_soc_runtime_ignore_pmdown_time(struct snd_soc_pcm_runtime *rtd)
 int snd_soc_set_runtime_hwparams(struct snd_pcm_substream *substream,
 	const struct snd_pcm_hardware *hw)
 {
-	struct snd_pcm_runtime *runtime = substream->runtime;
-	runtime->hw.info = hw->info;
-	runtime->hw.formats = hw->formats;
-	runtime->hw.period_bytes_min = hw->period_bytes_min;
-	runtime->hw.period_bytes_max = hw->period_bytes_max;
-	runtime->hw.periods_min = hw->periods_min;
-	runtime->hw.periods_max = hw->periods_max;
-	runtime->hw.buffer_bytes_max = hw->buffer_bytes_max;
-	runtime->hw.fifo_size = hw->fifo_size;
+	substream->runtime->hw = *hw;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_set_runtime_hwparams);
-- 
2.25.1

Re: [PATCH 3/8] ASoC: soc-pcm: direct copy at snd_soc_set_runtime_hwparams()

[Pierre-Louis Bossart]

On 3/4/21 6:59 PM, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> snd_soc_set_runtime_hwparams() is called from each driver
> to initialize hw parameters,
> but coping each parameters one-by-one.
> 
> Current code is not copying all parameters, but no big effect
> if we do it. This patch copies all parameters by simple code.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>   sound/soc/soc-pcm.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 60e688b103d8..6f2de27cf18f 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -300,15 +300,8 @@ bool snd_soc_runtime_ignore_pmdown_time(struct snd_soc_pcm_runtime *rtd)
>   int snd_soc_set_runtime_hwparams(struct snd_pcm_substream *substream,
>   	const struct snd_pcm_hardware *hw)
>   {
> -	struct snd_pcm_runtime *runtime = substream->runtime;
> -	runtime->hw.info = hw->info;
> -	runtime->hw.formats = hw->formats;
> -	runtime->hw.period_bytes_min = hw->period_bytes_min;
> -	runtime->hw.period_bytes_max = hw->period_bytes_max;
> -	runtime->hw.periods_min = hw->periods_min;
> -	runtime->hw.periods_max = hw->periods_max;
> -	runtime->hw.buffer_bytes_max = hw->buffer_bytes_max;
> -	runtime->hw.fifo_size = hw->fifo_size;
> +	substream->runtime->hw = *hw;

This dates from commit db2a416556af0 in 2006 :-)

what was specifically excluded is the information on rates and channels.

Liam, any memories if this was intentional?

[PATCH 4/8] ASoC: soc-pcm: add soc_pcm_update_symmetry()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Current soc-pcm has soc_pcm_has_symmetry() and using it as

	if (soc_pcm_has_symmetry(substream))
		substream->runtime->hw.info |= SNDRV_PCM_INFO_JOINT_DUPLEX;

We want to share same operation as same function.
This patch adds soc_pcm_update_symmetry() and pack above code in
one function.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 6f2de27cf18f..4ea4e2af9134 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -410,7 +410,7 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-static bool soc_pcm_has_symmetry(struct snd_pcm_substream *substream)
+static void soc_pcm_update_symmetry(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
 	struct snd_soc_dai_link *link = rtd->dai_link;
@@ -427,7 +427,8 @@ static bool soc_pcm_has_symmetry(struct snd_pcm_substream *substream)
 			dai->driver->symmetric_channels ||
 			dai->driver->symmetric_sample_bits;
 
-	return symmetry;
+	if (symmetry)
+		substream->runtime->hw.info |= SNDRV_PCM_INFO_JOINT_DUPLEX;
 }
 
 static void soc_pcm_set_msb(struct snd_pcm_substream *substream, int bits)
@@ -739,8 +740,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	/* Check that the codec and cpu DAIs are compatible */
 	soc_pcm_init_runtime_hw(substream);
 
-	if (soc_pcm_has_symmetry(substream))
-		runtime->hw.info |= SNDRV_PCM_INFO_JOINT_DUPLEX;
+	soc_pcm_update_symmetry(substream);
 
 	ret = -EINVAL;
 	if (!runtime->hw.rates) {
@@ -1685,8 +1685,7 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
 	int i;
 
 	/* apply symmetry for FE */
-	if (soc_pcm_has_symmetry(fe_substream))
-		fe_substream->runtime->hw.info |= SNDRV_PCM_INFO_JOINT_DUPLEX;
+	soc_pcm_update_symmetry(fe_substream);
 
 	for_each_rtd_cpu_dais (fe, i, fe_cpu_dai) {
 		/* Symmetry only applies if we've got an active stream. */
@@ -1711,8 +1710,7 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
 		if (rtd->dai_link->be_hw_params_fixup)
 			continue;
 
-		if (soc_pcm_has_symmetry(be_substream))
-			be_substream->runtime->hw.info |= SNDRV_PCM_INFO_JOINT_DUPLEX;
+		soc_pcm_update_symmetry(be_substream);
 
 		/* Symmetry only applies if we've got an active stream. */
 		for_each_rtd_dais(rtd, i, dai) {
-- 
2.25.1

[PATCH 5/8] ASoC: soc-pcm: add soc_hw_sanity_check()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Current soc_pcm_open() is checking runtime->hw parameters, but having
such function is very helpful for reading code.

This patch adds new soc_hw_sanity_check() and checks runtime->hw
parameters there. And print its debug message there, too.

Debug message print out timing is exchanged after this patch,
but it is not a big deal, because it is for debug.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 4ea4e2af9134..910a6afe9f48 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -689,6 +689,44 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	return soc_pcm_clean(substream, 0);
 }
 
+static int soc_hw_sanity_check(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct snd_pcm_hardware *hw = &substream->runtime->hw;
+	const char *name_cpu = soc_cpu_dai_name(rtd);
+	const char *name_codec = soc_codec_dai_name(rtd);
+	const char *err_msg;
+	struct device *dev = rtd->dev;
+
+	err_msg = "rates";
+	if (!hw->rates)
+		goto config_err;
+
+	err_msg = "formats";
+	if (!hw->formats)
+		goto config_err;
+
+	err_msg = "channels";
+	if (!hw->channels_min || !hw->channels_max ||
+	     hw->channels_min  >  hw->channels_max)
+		goto config_err;
+
+	dev_dbg(dev, "ASoC: %s <-> %s info:\n",		name_codec,
+							name_cpu);
+	dev_dbg(dev, "ASoC: rate mask 0x%x\n",		hw->rates);
+	dev_dbg(dev, "ASoC: ch   min %d max %d\n",	hw->channels_min,
+							hw->channels_max);
+	dev_dbg(dev, "ASoC: rate min %d max %d\n",	hw->rate_min,
+							hw->rate_max);
+
+	return 0;
+
+config_err:
+	dev_err(dev, "ASoC: %s <-> %s No matching %s\n",
+		name_codec, name_cpu, err_msg);
+	return -EINVAL;
+}
+
 /*
  * Called by ALSA when a PCM substream is opened, the runtime->hw record is
  * then initialized and any private data can be allocated. This also calls
@@ -697,11 +735,8 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 static int soc_pcm_open(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
-	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_soc_component *component;
 	struct snd_soc_dai *dai;
-	const char *codec_dai_name = soc_codec_dai_name(rtd);
-	const char *cpu_dai_name = soc_cpu_dai_name(rtd);
 	int i, ret = 0;
 
 	for_each_rtd_components(rtd, i, component)
@@ -742,23 +777,9 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 
 	soc_pcm_update_symmetry(substream);
 
-	ret = -EINVAL;
-	if (!runtime->hw.rates) {
-		printk(KERN_ERR "ASoC: %s <-> %s No matching rates\n",
-			codec_dai_name, cpu_dai_name);
-		goto err;
-	}
-	if (!runtime->hw.formats) {
-		printk(KERN_ERR "ASoC: %s <-> %s No matching formats\n",
-			codec_dai_name, cpu_dai_name);
-		goto err;
-	}
-	if (!runtime->hw.channels_min || !runtime->hw.channels_max ||
-	    runtime->hw.channels_min > runtime->hw.channels_max) {
-		printk(KERN_ERR "ASoC: %s <-> %s No matching channels\n",
-				codec_dai_name, cpu_dai_name);
+	ret = soc_hw_sanity_check(substream);
+	if (ret < 0)
 		goto err;
-	}
 
 	soc_pcm_apply_msb(substream);
 
@@ -768,14 +789,6 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 		if (ret != 0)
 			goto err;
 	}
-
-	pr_debug("ASoC: %s <-> %s info:\n",
-		 codec_dai_name, cpu_dai_name);
-	pr_debug("ASoC: rate mask 0x%x\n", runtime->hw.rates);
-	pr_debug("ASoC: min ch %d max ch %d\n", runtime->hw.channels_min,
-		 runtime->hw.channels_max);
-	pr_debug("ASoC: min rate %d max rate %d\n", runtime->hw.rate_min,
-		 runtime->hw.rate_max);
 dynamic:
 	snd_soc_runtime_activate(rtd, substream->stream);
 	ret = 0;
-- 
2.25.1

[PATCH 6/8] ASoC: soc-pcm: fixup dpcm_be_dai_startup() user count

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

At dpcm_be_dai_startup_unwind(), it indicates error message at (1)
if this function was called with no users.
But, it doesn't use "continue" here. Thus, it will be minus
users at (2).

	void dpcm_be_dai_startup_unwind(...)
	{
		...
		for_each_dpcm_be(...) {
			...
(1)			if (be->dpcm[stream].users == 0)
				dev_err(...);

(2)			if (--be->dpcm[stream].users != 0)
				continue;

At dpcm_be_dai_startup(), it indicates error message if
user reached to MAX USERS at (A).
But, it doesn't use "continue" here. Thus, it will be over
MAX USERS at (B).

	int dpcm_be_dai_startup(...)
	{
		...
		for_each_dpcm_be(...) {
			...
(A)			if (be->dpcm[stream].users == DPCM_MAX_BE_USERS)
				dev_err(...);

(B)			if (be->dpcm[stream].users++ != 0)
				continue;

These are just bug. This patch fixup these.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 910a6afe9f48..626d6e0a3a15 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1432,10 +1432,12 @@ static void dpcm_be_dai_startup_unwind(struct snd_soc_pcm_runtime *fe,
 		struct snd_pcm_substream *be_substream =
 			snd_soc_dpcm_get_substream(be, stream);
 
-		if (be->dpcm[stream].users == 0)
+		if (be->dpcm[stream].users == 0) {
 			dev_err(be->dev, "ASoC: no users %s at close - state %d\n",
 				stream ? "capture" : "playback",
 				be->dpcm[stream].state);
+			continue;
+		}
 
 		if (--be->dpcm[stream].users != 0)
 			continue;
@@ -1472,10 +1474,12 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
 			continue;
 
 		/* first time the dpcm is open ? */
-		if (be->dpcm[stream].users == DPCM_MAX_BE_USERS)
+		if (be->dpcm[stream].users == DPCM_MAX_BE_USERS) {
 			dev_err(be->dev, "ASoC: too many users %s at open %d\n",
 				stream ? "capture" : "playback",
 				be->dpcm[stream].state);
+			continue;
+		}
 
 		if (be->dpcm[stream].users++ != 0)
 			continue;
@@ -1517,10 +1521,12 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
 		if (!snd_soc_dpcm_be_can_update(fe, be, stream))
 			continue;
 
-		if (be->dpcm[stream].users == 0)
+		if (be->dpcm[stream].users == 0) {
 			dev_err(be->dev, "ASoC: no users %s at close %d\n",
 				stream ? "capture" : "playback",
 				be->dpcm[stream].state);
+			continue;
+		}
 
 		if (--be->dpcm[stream].users != 0)
 			continue;
-- 
2.25.1

Re: [PATCH 6/8] ASoC: soc-pcm: fixup dpcm_be_dai_startup() user count

[Pierre-Louis Bossart]

On 3/4/21 7:00 PM, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> At dpcm_be_dai_startup_unwind(), it indicates error message at (1)
> if this function was called with no users.
> But, it doesn't use "continue" here. Thus, it will be minus
> users at (2).

suggested edit:

"Thus, users will be a negative number at (2)"

> 
> 	void dpcm_be_dai_startup_unwind(...)
> 	{
> 		...
> 		for_each_dpcm_be(...) {
> 			...
> (1)			if (be->dpcm[stream].users == 0)
> 				dev_err(...);
> 
> (2)			if (--be->dpcm[stream].users != 0)
> 				continue;

Re: [PATCH 6/8] ASoC: soc-pcm: fixup dpcm_be_dai_startup() user count

[Kuninori Morimoto]

Hi Pierre-Louis, Mark

Thank you for your review.

> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > 
> > At dpcm_be_dai_startup_unwind(), it indicates error message at (1)
> > if this function was called with no users.
> > But, it doesn't use "continue" here. Thus, it will be minus
> > users at (2).
> 
> suggested edit:
> 
> "Thus, users will be a negative number at (2)"

Ah it is better :)
Mark, can I repost it ?
I'm happy if you tidyup it if you are OK for appling.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 6/8] ASoC: soc-pcm: fixup dpcm_be_dai_startup() user count

[Mark Brown]

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

On Mon, Mar 08, 2021 at 07:37:28AM +0900, Kuninori Morimoto wrote:

> Ah it is better :)
> Mark, can I repost it ?

Sure.

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

[PATCH 7/8] ASoC: soc-pcm: remove unneeded !rtd->dai_link check

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

rtd->dai_link is setuped at soc_new_pcm_runtime(),
thus "rtd->dai_link == NULL" is never happen.
This patch removes unneeded !rtd->dai_link check

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 626d6e0a3a15..0ae386f0790e 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -165,9 +165,6 @@ static const struct file_operations dpcm_state_fops = {
 
 void soc_dpcm_debugfs_add(struct snd_soc_pcm_runtime *rtd)
 {
-	if (!rtd->dai_link)
-		return;
-
 	if (!rtd->dai_link->dynamic)
 		return;
 
-- 
2.25.1

[PATCH 8/8] ASoC: soc-pcm: share DPCM BE DAI stop operation

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

soc-pcm has very similar but different DPCM BE DAI stop operation at
	1) dpcm_be_dai_startup() error case rollback
	2) dpcm_be_dai_startup_unwind()
	3) dpcm_be_dai_shutdown()

The differences are
	1) for rollback
	2) Doesn't check by snd_soc_dpcm_be_can_update() (Is this bug ?)
	3) Do soc_pcm_hw_free() if it was not !OPENed and !HW_FREEed,
	   and call soc_pcm_close().

We can share same code by
	1) hw_free is not needed. Needs last dpcm as rollback.
	2) hw_free is not needed.
	3) hw_free is     needed.

This patch adds new dpcm_be_dai_stop() and share these 3.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc-dpcm.h |  8 +++-
 sound/soc/soc-compress.c |  2 +-
 sound/soc/soc-pcm.c      | 94 +++++++++-------------------------------
 3 files changed, 28 insertions(+), 76 deletions(-)

diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h
index 0f6c50b17bba..d76cb1eeeaca 100644
--- a/include/sound/soc-dpcm.h
+++ b/include/sound/soc-dpcm.h
@@ -149,7 +149,8 @@ void dpcm_path_put(struct snd_soc_dapm_widget_list **list);
 int dpcm_process_paths(struct snd_soc_pcm_runtime *fe,
 	int stream, struct snd_soc_dapm_widget_list **list, int new);
 int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream);
-int dpcm_be_dai_shutdown(struct snd_soc_pcm_runtime *fe, int stream);
+void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream,
+		      int do_hw_free, struct snd_soc_dpcm *last);
 void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream);
 void dpcm_clear_pending_state(struct snd_soc_pcm_runtime *fe, int stream);
 int dpcm_be_dai_hw_free(struct snd_soc_pcm_runtime *fe, int stream);
@@ -159,4 +160,9 @@ int dpcm_be_dai_prepare(struct snd_soc_pcm_runtime *fe, int stream);
 int dpcm_dapm_stream_event(struct snd_soc_pcm_runtime *fe, int dir,
 	int event);
 
+#define dpcm_be_dai_startup_rollback(fe, stream, last)	\
+						dpcm_be_dai_stop(fe, stream, 0, last)
+#define dpcm_be_dai_startup_unwind(fe, stream)	dpcm_be_dai_stop(fe, stream, 0, NULL)
+#define dpcm_be_dai_shutdown(fe, stream)	dpcm_be_dai_stop(fe, stream, 1, NULL)
+
 #endif
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 246a5e32e22a..89445ba0e86b 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -189,7 +189,7 @@ static int soc_compr_free_fe(struct snd_compr_stream *cstream)
 	if (ret < 0)
 		dev_err(fe->dev, "Compressed ASoC: hw_free failed: %d\n", ret);
 
-	ret = dpcm_be_dai_shutdown(fe, stream);
+	dpcm_be_dai_shutdown(fe, stream);
 
 	/* mark FE's links ready to prune */
 	for_each_dpcm_be(fe, stream, dpcm)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 0ae386f0790e..a27385ab7b55 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1417,18 +1417,24 @@ void dpcm_clear_pending_state(struct snd_soc_pcm_runtime *fe, int stream)
 	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
 }
 
-static void dpcm_be_dai_startup_unwind(struct snd_soc_pcm_runtime *fe,
-	int stream)
+void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream,
+		      int do_hw_free, struct snd_soc_dpcm *last)
 {
 	struct snd_soc_dpcm *dpcm;
 
 	/* disable any enabled and non active backends */
 	for_each_dpcm_be(fe, stream, dpcm) {
-
 		struct snd_soc_pcm_runtime *be = dpcm->be;
 		struct snd_pcm_substream *be_substream =
 			snd_soc_dpcm_get_substream(be, stream);
 
+		if (dpcm == last)
+			return;
+
+		/* is this op for this BE ? */
+		if (!snd_soc_dpcm_be_can_update(fe, be, stream))
+			continue;
+
 		if (be->dpcm[stream].users == 0) {
 			dev_err(be->dev, "ASoC: no users %s at close - state %d\n",
 				stream ? "capture" : "playback",
@@ -1439,8 +1445,15 @@ static void dpcm_be_dai_startup_unwind(struct snd_soc_pcm_runtime *fe,
 		if (--be->dpcm[stream].users != 0)
 			continue;
 
-		if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_OPEN)
-			continue;
+		if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_OPEN) {
+			if (!do_hw_free)
+				continue;
+
+			if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_HW_FREE) {
+				soc_pcm_hw_free(be_substream);
+				be->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
+			}
+		}
 
 		soc_pcm_close(be_substream);
 		be_substream->runtime = NULL;
@@ -1509,32 +1522,7 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
 	return count;
 
 unwind:
-	/* disable any enabled and non active backends */
-	for_each_dpcm_be_rollback(fe, stream, dpcm) {
-		struct snd_soc_pcm_runtime *be = dpcm->be;
-		struct snd_pcm_substream *be_substream =
-			snd_soc_dpcm_get_substream(be, stream);
-
-		if (!snd_soc_dpcm_be_can_update(fe, be, stream))
-			continue;
-
-		if (be->dpcm[stream].users == 0) {
-			dev_err(be->dev, "ASoC: no users %s at close %d\n",
-				stream ? "capture" : "playback",
-				be->dpcm[stream].state);
-			continue;
-		}
-
-		if (--be->dpcm[stream].users != 0)
-			continue;
-
-		if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_OPEN)
-			continue;
-
-		soc_pcm_close(be_substream);
-		be_substream->runtime = NULL;
-		be->dpcm[stream].state = SND_SOC_DPCM_STATE_CLOSE;
-	}
+	dpcm_be_dai_startup_rollback(fe, stream, dpcm);
 
 	return err;
 }
@@ -1782,46 +1770,6 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
 	return ret;
 }
 
-int dpcm_be_dai_shutdown(struct snd_soc_pcm_runtime *fe, int stream)
-{
-	struct snd_soc_dpcm *dpcm;
-
-	/* only shutdown BEs that are either sinks or sources to this FE DAI */
-	for_each_dpcm_be(fe, stream, dpcm) {
-
-		struct snd_soc_pcm_runtime *be = dpcm->be;
-		struct snd_pcm_substream *be_substream =
-			snd_soc_dpcm_get_substream(be, stream);
-
-		/* is this op for this BE ? */
-		if (!snd_soc_dpcm_be_can_update(fe, be, stream))
-			continue;
-
-		if (be->dpcm[stream].users == 0)
-			dev_err(be->dev, "ASoC: no users %s at close - state %d\n",
-				stream ? "capture" : "playback",
-				be->dpcm[stream].state);
-
-		if (--be->dpcm[stream].users != 0)
-			continue;
-
-		if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_HW_FREE) &&
-		    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_OPEN)) {
-			soc_pcm_hw_free(be_substream);
-			be->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
-		}
-
-		dev_dbg(be->dev, "ASoC: close BE %s\n",
-			be->dai_link->name);
-
-		soc_pcm_close(be_substream);
-		be_substream->runtime = NULL;
-
-		be->dpcm[stream].state = SND_SOC_DPCM_STATE_CLOSE;
-	}
-	return 0;
-}
-
 static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
@@ -2371,9 +2319,7 @@ static int dpcm_run_update_shutdown(struct snd_soc_pcm_runtime *fe, int stream)
 	if (err < 0)
 		dev_err(fe->dev,"ASoC: hw_free FE failed %d\n", err);
 
-	err = dpcm_be_dai_shutdown(fe, stream);
-	if (err < 0)
-		dev_err(fe->dev,"ASoC: shutdown FE failed %d\n", err);
+	dpcm_be_dai_shutdown(fe, stream);
 
 	/* run the stream event for each BE */
 	dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_NOP);
-- 
2.25.1

[PATCH 0/8] ASoC: soc-pcm: cleanup each functions

[Kuninori Morimoto]

Hi Mark

These are soc-pcm cleanup patches, but has no relationship
to each other.
My 1 concern is [3/8] patch. I think it is no problem,
but I'm not 100% sure why current code was such code.

Kuninori Morimoto (8):
  1) ASoC: soc-pcm: check DAI activity under soc_pcm_apply_symmetry()
  2) ASoC: soc-pcm: add soc_cpu/codec_dai_name() macro
  3) ASoC: soc-pcm: direct copy at snd_soc_set_runtime_hwparams()
  4) ASoC: soc-pcm: add soc_pcm_care_symmetry()
  5) ASoC: soc-pcm: add soc_hw_sanity_check()
  6) ASoC: soc-pcm: fixup dpcm_be_dai_startup() user count
  7) ASoC: soc-pcm: remove unneeded !rtd->dai_link check
  8) ASoC: soc-pcm: share DPCM BE DAI stop operation

 include/sound/soc-dpcm.h |   8 +-
 sound/soc/soc-compress.c |   2 +-
 sound/soc/soc-pcm.c      | 239 +++++++++++++++------------------------
 3 files changed, 101 insertions(+), 148 deletions(-)

-- 
2.25.1

[PATCH 5/8] ASoC: soc-pcm: add soc_hw_sanity_check()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Current soc_pcm_open() is checking runtime->hw parameters, but having
such function is very helpful for reading code.

This patch adds new soc_hw_sanity_check() and checks runtime->hw
parameters there. And print its debug message there, too.

Debug message print out timing is exchanged after this patch,
but it is not a big deal, because it is for debug.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 76c7f04cfd1a..aa1f26ccb0dc 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -685,6 +685,44 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	return soc_pcm_clean(substream, 0);
 }
 
+static int soc_hw_sanity_check(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct snd_pcm_hardware *hw = &substream->runtime->hw;
+	const char *name_cpu = soc_cpu_dai_name(rtd);
+	const char *name_codec = soc_codec_dai_name(rtd);
+	const char *err_msg;
+	struct device *dev = rtd->dev;
+
+	err_msg = "rates";
+	if (!hw->rates)
+		goto config_err;
+
+	err_msg = "formats";
+	if (!hw->formats)
+		goto config_err;
+
+	err_msg = "channels";
+	if (!hw->channels_min || !hw->channels_max ||
+	     hw->channels_min  >  hw->channels_max)
+		goto config_err;
+
+	dev_dbg(dev, "ASoC: %s <-> %s info:\n",		name_codec,
+							name_cpu);
+	dev_dbg(dev, "ASoC: rate mask 0x%x\n",		hw->rates);
+	dev_dbg(dev, "ASoC: ch   min %d max %d\n",	hw->channels_min,
+							hw->channels_max);
+	dev_dbg(dev, "ASoC: rate min %d max %d\n",	hw->rate_min,
+							hw->rate_max);
+
+	return 0;
+
+config_err:
+	dev_err(dev, "ASoC: %s <-> %s No matching %s\n",
+		name_codec, name_cpu, err_msg);
+	return -EINVAL;
+}
+
 /*
  * Called by ALSA when a PCM substream is opened, the runtime->hw record is
  * then initialized and any private data can be allocated. This also calls
@@ -693,11 +731,8 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 static int soc_pcm_open(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
-	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_soc_component *component;
 	struct snd_soc_dai *dai;
-	const char *codec_dai_name = soc_codec_dai_name(rtd);
-	const char *cpu_dai_name = soc_cpu_dai_name(rtd);
 	int i, ret = 0;
 
 	for_each_rtd_components(rtd, i, component)
@@ -738,23 +773,9 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 
 	soc_pcm_care_symmetry(substream);
 
-	ret = -EINVAL;
-	if (!runtime->hw.rates) {
-		printk(KERN_ERR "ASoC: %s <-> %s No matching rates\n",
-			codec_dai_name, cpu_dai_name);
-		goto err;
-	}
-	if (!runtime->hw.formats) {
-		printk(KERN_ERR "ASoC: %s <-> %s No matching formats\n",
-			codec_dai_name, cpu_dai_name);
-		goto err;
-	}
-	if (!runtime->hw.channels_min || !runtime->hw.channels_max ||
-	    runtime->hw.channels_min > runtime->hw.channels_max) {
-		printk(KERN_ERR "ASoC: %s <-> %s No matching channels\n",
-				codec_dai_name, cpu_dai_name);
+	ret = soc_hw_sanity_check(substream);
+	if (ret < 0)
 		goto err;
-	}
 
 	soc_pcm_apply_msb(substream);
 
@@ -764,14 +785,6 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 		if (ret != 0)
 			goto err;
 	}
-
-	pr_debug("ASoC: %s <-> %s info:\n",
-		 codec_dai_name, cpu_dai_name);
-	pr_debug("ASoC: rate mask 0x%x\n", runtime->hw.rates);
-	pr_debug("ASoC: min ch %d max ch %d\n", runtime->hw.channels_min,
-		 runtime->hw.channels_max);
-	pr_debug("ASoC: min rate %d max rate %d\n", runtime->hw.rate_min,
-		 runtime->hw.rate_max);
 dynamic:
 	snd_soc_runtime_activate(rtd, substream->stream);
 	ret = 0;
-- 
2.25.1