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

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

[Kuninori Morimoto]

Hi Mark

These are soc-pcm cleanup step5.
These are based on Bard Liao's "ASoC: Add Multi CPU DAI support"
patch-set.

[7/8] patch might exchange the behavior

Kuninori Morimoto (8):
  1) ASoC: soc-pcm: use defined stream
  2) ASoC: soc-pcm: remove duplicate be check from dpcm_add_paths()
  3) ASoC: soc-pcm: move dpcm_fe_dai_close()
  4) ASoC: soc-pcm: add dpcm_fe_dai_clean()
  5) ASoC: soc-pcm: use snd_soc_dai_get_pcm_stream() at dpcm_set_fe_runtime()
  6) ASoC: soc-pcm: check DAI's activity more simply
  7) ASoC: soc-pcm: Do Digital Mute for both CPU/Codec in same timing.
  8) ASoC: soc-pcm: tidyup dulicate handing at dpcm_fe_dai_startup()

 sound/soc/soc-pcm.c | 107 ++++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 59 deletions(-)

-- 
2.17.1

[PATCH 1/8] ASoC: soc-pcm: use defined stream

[Kuninori Morimoto]

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

Many functions defines "stream = substream->stream", but
some of them is using "substream->stream" instead of "stream".
It is pointless. This patch uses defined stream.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 90857138c823..8c27eb4d5e4c 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -644,8 +644,7 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
 		 * bailed out on a higher level, since there would be no
 		 * CODEC to support the transfer direction in that case.
 		 */
-		if (!snd_soc_dai_stream_valid(codec_dai,
-					      substream->stream))
+		if (!snd_soc_dai_stream_valid(codec_dai, stream))
 			continue;
 
 		codec_stream = snd_soc_dai_get_pcm_stream(codec_dai, stream);
@@ -2149,7 +2148,7 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
 
 	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
 
-	ret = dpcm_be_dai_startup(fe, fe_substream->stream);
+	ret = dpcm_be_dai_startup(fe, stream);
 	if (ret < 0) {
 		dev_err(fe->dev,"ASoC: failed to start some BEs %d\n", ret);
 		goto be_err;
@@ -2180,7 +2179,7 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
 	return 0;
 
 unwind:
-	dpcm_be_dai_startup_unwind(fe, fe_substream->stream);
+	dpcm_be_dai_startup_unwind(fe, stream);
 be_err:
 	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
 	return ret;
@@ -2234,7 +2233,7 @@ static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
 	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
 
 	/* shutdown the BEs */
-	dpcm_be_dai_shutdown(fe, substream->stream);
+	dpcm_be_dai_shutdown(fe, stream);
 
 	dev_dbg(fe->dev, "ASoC: close FE %s\n", fe->dai_link->name);
 
@@ -2412,9 +2411,9 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
 	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
 	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
 
-	memcpy(&fe->dpcm[substream->stream].hw_params, params,
+	memcpy(&fe->dpcm[stream].hw_params, params,
 			sizeof(struct snd_pcm_hw_params));
-	ret = dpcm_be_dai_hw_params(fe, substream->stream);
+	ret = dpcm_be_dai_hw_params(fe, stream);
 	if (ret < 0) {
 		dev_err(fe->dev,"ASoC: hw_params BE failed %d\n", ret);
 		goto out;
@@ -2736,7 +2735,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
 		goto out;
 	}
 
-	ret = dpcm_be_dai_prepare(fe, substream->stream);
+	ret = dpcm_be_dai_prepare(fe, stream);
 	if (ret < 0)
 		goto out;
 
-- 
2.17.1

[PATCH 2/8] ASoC: soc-pcm: remove duplicate be check from dpcm_add_paths()

[Kuninori Morimoto]

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

dpcm_add_paths() checks returned be from dpcm_get_be()

	static int dpcm_add_paths(...)
	{
		...
		for_each_dapm_widgets(list, i, widget) {
			...
			be = dpcm_get_be(...);
			...
			/* make sure BE is a real BE */
=>			if (!be->dai_link->no_pcm)
				continue;
			...
		}
		...
	}

But, dpcm_get_be() itself is checking it already.

	dpcm_get_be(...)
	{
		...
		for_each_card_rtds(card, be) {
=>			if (!be->dai_link->no_pcm)
				continue;
			...
			if (...)
=>				return be;
		}

		return NULL
	}

This patch removes duplicate check

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 8c27eb4d5e4c..e3a2c4f7757b 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1690,10 +1690,6 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream,
 			continue;
 		}
 
-		/* make sure BE is a real BE */
-		if (!be->dai_link->no_pcm)
-			continue;
-
 		/* don't connect if FE is not running */
 		if (!fe->dpcm[stream].runtime && !fe->fe_compr)
 			continue;
-- 
2.17.1

[PATCH 3/8] ASoC: soc-pcm: move dpcm_fe_dai_close()

[Kuninori Morimoto]

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

move dpcm_fe_dai_close() next to dpcm_fe_dai_open().
This is prepare for dpcm_fe_dai_open() cleanup

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index e3a2c4f7757b..3686dda097e2 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2978,6 +2978,26 @@ int soc_dpcm_runtime_update(struct snd_soc_card *card)
 	return ret;
 }
 
+static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
+{
+	struct snd_soc_pcm_runtime *fe = fe_substream->private_data;
+	struct snd_soc_dpcm *dpcm;
+	int stream = fe_substream->stream, ret;
+
+	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
+	ret = dpcm_fe_dai_shutdown(fe_substream);
+
+	/* mark FE's links ready to prune */
+	for_each_dpcm_be(fe, stream, dpcm)
+		dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE;
+
+	dpcm_be_disconnect(fe, stream);
+
+	fe->dpcm[stream].runtime = NULL;
+	mutex_unlock(&fe->card->mutex);
+	return ret;
+}
+
 static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream)
 {
 	struct snd_soc_pcm_runtime *fe = fe_substream->private_data;
@@ -3017,26 +3037,6 @@ static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream)
 	return ret;
 }
 
-static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
-{
-	struct snd_soc_pcm_runtime *fe = fe_substream->private_data;
-	struct snd_soc_dpcm *dpcm;
-	int stream = fe_substream->stream, ret;
-
-	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
-	ret = dpcm_fe_dai_shutdown(fe_substream);
-
-	/* mark FE's links ready to prune */
-	for_each_dpcm_be(fe, stream, dpcm)
-		dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE;
-
-	dpcm_be_disconnect(fe, stream);
-
-	fe->dpcm[stream].runtime = NULL;
-	mutex_unlock(&fe->card->mutex);
-	return ret;
-}
-
 /* create a new pcm */
 int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
 {
-- 
2.17.1

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

[Kuninori Morimoto]

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

dpcm_fe_dai_close() and error case of dpcm_fe_dai_open()
need to do same cleanup operation.
To avoid duplicate code, this patch adds dpcm_fe_dai_clean()
and use it.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 3686dda097e2..d578dbdfa846 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2978,14 +2978,11 @@ int soc_dpcm_runtime_update(struct snd_soc_card *card)
 	return ret;
 }
 
-static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
+static void dpcm_fe_dai_clean(struct snd_pcm_substream *fe_substream)
 {
 	struct snd_soc_pcm_runtime *fe = fe_substream->private_data;
 	struct snd_soc_dpcm *dpcm;
-	int stream = fe_substream->stream, ret;
-
-	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
-	ret = dpcm_fe_dai_shutdown(fe_substream);
+	int stream = fe_substream->stream;
 
 	/* mark FE's links ready to prune */
 	for_each_dpcm_be(fe, stream, dpcm)
@@ -2994,6 +2991,18 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
 	dpcm_be_disconnect(fe, stream);
 
 	fe->dpcm[stream].runtime = NULL;
+}
+
+static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
+{
+	struct snd_soc_pcm_runtime *fe = fe_substream->private_data;
+	int ret;
+
+	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
+	ret = dpcm_fe_dai_shutdown(fe_substream);
+
+	dpcm_fe_dai_clean(fe_substream);
+
 	mutex_unlock(&fe->card->mutex);
 	return ret;
 }
@@ -3001,7 +3010,6 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
 static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream)
 {
 	struct snd_soc_pcm_runtime *fe = fe_substream->private_data;
-	struct snd_soc_dpcm *dpcm;
 	struct snd_soc_dapm_widget_list *list;
 	int ret;
 	int stream = fe_substream->stream;
@@ -3021,14 +3029,8 @@ static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream)
 	dpcm_process_paths(fe, stream, &list, 1);
 
 	ret = dpcm_fe_dai_startup(fe_substream);
-	if (ret < 0) {
-		/* clean up all links */
-		for_each_dpcm_be(fe, stream, dpcm)
-			dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE;
-
-		dpcm_be_disconnect(fe, stream);
-		fe->dpcm[stream].runtime = NULL;
-	}
+	if (ret < 0)
+		dpcm_fe_dai_clean(fe_substream);
 
 	dpcm_clear_pending_state(fe, stream);
 	dpcm_path_put(&list);
-- 
2.17.1

[PATCH 5/8] ASoC: soc-pcm: use snd_soc_dai_get_pcm_stream() at dpcm_set_fe_runtime()

[Kuninori Morimoto]

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

We already have snd_soc_dai_get_pcm_stream(),
let's use it

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index d578dbdfa846..4a14f10b8c90 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2022,7 +2022,6 @@ static void dpcm_set_fe_runtime(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_dai *cpu_dai;
-	struct snd_soc_dai_driver *cpu_dai_drv;
 	int i;
 
 	for_each_rtd_cpu_dai(rtd, i, cpu_dai) {
@@ -2033,11 +2032,9 @@ static void dpcm_set_fe_runtime(struct snd_pcm_substream *substream)
 		if (!snd_soc_dai_stream_valid(cpu_dai, substream->stream))
 			continue;
 
-		cpu_dai_drv = cpu_dai->driver;
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-			dpcm_init_runtime_hw(runtime, &cpu_dai_drv->playback);
-		else
-			dpcm_init_runtime_hw(runtime, &cpu_dai_drv->capture);
+		dpcm_init_runtime_hw(runtime,
+			snd_soc_dai_get_pcm_stream(cpu_dai,
+						   substream->stream));
 	}
 
 	dpcm_runtime_merge_format(substream, &runtime->hw.formats);
-- 
2.17.1

[PATCH 6/8] ASoC: soc-pcm: check DAI's activity more simply

[Kuninori Morimoto]

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

DAI is counting its activity, and both playback/capture directional
activity. When considering mute, DAI's activity is enough instead of
caring both playback/capture.
This patch makes mute check simply.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 4a14f10b8c90..7e0464ec802e 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1202,7 +1202,6 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_dai *cpu_dai;
 	struct snd_soc_dai *codec_dai;
-	bool playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
 	int i;
 
 	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
@@ -1226,11 +1225,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
 
 	/* apply codec digital mute */
 	for_each_rtd_codec_dai(rtd, i, codec_dai) {
-		int playback_active = codec_dai->stream_active[SNDRV_PCM_STREAM_PLAYBACK];
-		int capture_active  = codec_dai->stream_active[SNDRV_PCM_STREAM_CAPTURE];
-
-		if ((playback && playback_active == 1) ||
-		    (!playback && capture_active == 1))
+		if (codec_dai->active == 1)
 			snd_soc_dai_digital_mute(codec_dai, 1,
 						 substream->stream);
 	}
-- 
2.17.1

[PATCH 7/8] ASoC: soc-pcm: Do Digital Mute for both CPU/Codec in same timing.

[Kuninori Morimoto]

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

Digital Mute for CPU   is done at soc_pcm_close(), and
Digital Mute for Codec is done at soc_pcm_hw_free().
It is just confusable.
This patch do Digital Mute for both CPU/Codec in same timing.
Then, it cares DAI activity

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 7e0464ec802e..1f7a86c4bc02 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -760,9 +760,6 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 
 	snd_soc_runtime_deactivate(rtd, substream->stream);
 
-	for_each_rtd_cpu_dai(rtd, i, cpu_dai)
-		snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream);
-
 	for_each_rtd_cpu_dai(rtd, i, cpu_dai)
 		snd_soc_dai_shutdown(cpu_dai, substream);
 
@@ -1229,6 +1226,11 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
 			snd_soc_dai_digital_mute(codec_dai, 1,
 						 substream->stream);
 	}
+	for_each_rtd_cpu_dai(rtd, i, cpu_dai) {
+		if (cpu_dai->active == 1)
+			snd_soc_dai_digital_mute(cpu_dai, 1,
+						 substream->stream);
+	}
 
 	/* free any machine hw params */
 	soc_rtd_hw_free(rtd, substream);
-- 
2.17.1

[PATCH 8/8] ASoC: soc-pcm: tidyup dulicate handing at dpcm_fe_dai_startup()

[Kuninori Morimoto]

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

error handling at dpcm_fe_dai_startup() has duplicate code.
This patch tidyup it.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 1f7a86c4bc02..2fdd90437a6f 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2165,11 +2165,9 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
 		goto unwind;
 	}
 
-	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
-	return 0;
-
 unwind:
-	dpcm_be_dai_startup_unwind(fe, stream);
+	if (ret < 0)
+		dpcm_be_dai_startup_unwind(fe, stream);
 be_err:
 	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
 	return ret;
-- 
2.17.1

Re: [PATCH 8/8] ASoC: soc-pcm: tidyup dulicate handing at dpcm_fe_dai_startup()

[Pierre-Louis Bossart]

On 2/26/20 12:41 AM, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> error handling at dpcm_fe_dai_startup() has duplicate code.
> This patch tidyup it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>   sound/soc/soc-pcm.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 1f7a86c4bc02..2fdd90437a6f 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2165,11 +2165,9 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
>   		goto unwind;
>   	}

nit-pick: since the two lines below are removed, the 'goto unwind' above 
becomes unnecessary. I don't mind if you leave it for symmetry with the 
rest of the error handling flow.

> -	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
> -	return 0;
> -
>   unwind:
> -	dpcm_be_dai_startup_unwind(fe, stream);
> +	if (ret < 0)
> +		dpcm_be_dai_startup_unwind(fe, stream);
>   be_err:
>   	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>   	return ret;
> 

Re: [PATCH 0/8] ASoC: soc-pcm cleanup step5

[Pierre-Louis Bossart]

On 2/26/20 12:39 AM, Kuninori Morimoto wrote:
> 
> Hi Mark
> 
> These are soc-pcm cleanup step5.
> These are based on Bard Liao's "ASoC: Add Multi CPU DAI support"
> patch-set.
> 
> [7/8] patch might exchange the behavior
> 
> Kuninori Morimoto (8):
>    1) ASoC: soc-pcm: use defined stream
>    2) ASoC: soc-pcm: remove duplicate be check from dpcm_add_paths()
>    3) ASoC: soc-pcm: move dpcm_fe_dai_close()
>    4) ASoC: soc-pcm: add dpcm_fe_dai_clean()
>    5) ASoC: soc-pcm: use snd_soc_dai_get_pcm_stream() at dpcm_set_fe_runtime()
>    6) ASoC: soc-pcm: check DAI's activity more simply
>    7) ASoC: soc-pcm: Do Digital Mute for both CPU/Codec in same timing.
>    8) ASoC: soc-pcm: tidyup dulicate handing at dpcm_fe_dai_startup()

Looks good to me (only minor nit-picks)

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

if you give 2-3 days we'll provide a Tested-by tag.

> 
>   sound/soc/soc-pcm.c | 107 ++++++++++++++++++++------------------------
>   1 file changed, 48 insertions(+), 59 deletions(-)
> 

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

[Ranjani Sridharan]

On Wed, 2020-02-26 at 15:40 +0900, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> dpcm_fe_dai_close() and error case of dpcm_fe_dai_open()
> need to do same cleanup operation.
> To avoid duplicate code, this patch adds dpcm_fe_dai_clean()
> and use it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  sound/soc/soc-pcm.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 3686dda097e2..d578dbdfa846 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2978,14 +2978,11 @@ int soc_dpcm_runtime_update(struct
> snd_soc_card *card)
>  	return ret;
>  }
>  
> -static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
> +static void dpcm_fe_dai_clean(struct snd_pcm_substream
> *fe_substream)
The series looks good to me, Morimoto-san. But a minor suggestion if
you're doing a v2 to address other comments. Could you please change
the name of the function above to dpcm_fe_dai_cleanup() instead?

Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Thanks,
Ranjani

Re: [PATCH 6/8] ASoC: soc-pcm: check DAI's activity more simply

[Pierre-Louis Bossart]

On 2/26/20 12:40 AM, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> DAI is counting its activity, and both playback/capture directional
> activity. When considering mute, DAI's activity is enough instead of
> caring both playback/capture.
> This patch makes mute check simply.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>   sound/soc/soc-pcm.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 4a14f10b8c90..7e0464ec802e 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -1202,7 +1202,6 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
>   	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>   	struct snd_soc_dai *cpu_dai;
>   	struct snd_soc_dai *codec_dai;
> -	bool playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
>   	int i;
>   
>   	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
> @@ -1226,11 +1225,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
>   
>   	/* apply codec digital mute */
>   	for_each_rtd_codec_dai(rtd, i, codec_dai) {
> -		int playback_active = codec_dai->stream_active[SNDRV_PCM_STREAM_PLAYBACK];
> -		int capture_active  = codec_dai->stream_active[SNDRV_PCM_STREAM_CAPTURE];
> -
> -		if ((playback && playback_active == 1) ||
> -		    (!playback && capture_active == 1))
> +		if (codec_dai->active == 1)

nit-pick: we have two tests in soc-pcm.c

if (codec_dai->active)
if (codec_dai->active == 1)

The two are functionality equivalent but it'd be good to choose one 
version - or possibly use 'active' as a boolean.

>   			snd_soc_dai_digital_mute(codec_dai, 1,
>   						 substream->stream);
>   	}
> 

Re: [PATCH 8/8] ASoC: soc-pcm: tidyup dulicate handing at dpcm_fe_dai_startup()

[Kuninori Morimoto]

Hi Pierre-Louis

> > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> > index 1f7a86c4bc02..2fdd90437a6f 100644
> > --- a/sound/soc/soc-pcm.c
> > +++ b/sound/soc/soc-pcm.c
> > @@ -2165,11 +2165,9 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
> >   		goto unwind;
> >   	}
> 
> nit-pick: since the two lines below are removed, the 'goto unwind'
> above becomes unnecessary. I don't mind if you leave it for symmetry
> with the rest of the error handling flow.

Oops ? Indeed.
Thank you for your review
Will fixup it in v2

Thank you for your help !!
Best regards
---
Kuninori Morimoto

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

[Kuninori Morimoto]

Hi Ranjani

Thank you for your review

> > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> > index 3686dda097e2..d578dbdfa846 100644
> > --- a/sound/soc/soc-pcm.c
> > +++ b/sound/soc/soc-pcm.c
> > @@ -2978,14 +2978,11 @@ int soc_dpcm_runtime_update(struct
> > snd_soc_card *card)
> >  	return ret;
> >  }
> >  
> > -static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
> > +static void dpcm_fe_dai_clean(struct snd_pcm_substream
> > *fe_substream)
> The series looks good to me, Morimoto-san. But a minor suggestion if
> you're doing a v2 to address other comments. Could you please change
> the name of the function above to dpcm_fe_dai_cleanup() instead?

Yeah, will do.
Thanks

> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Thank you for your help !!
Best regards
---
Kuninori Morimoto

Re: [PATCH 6/8] ASoC: soc-pcm: check DAI's activity more simply

[Kuninori Morimoto]

Hi Pierre-Louis

> > @@ -1226,11 +1225,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
> >     	/* apply codec digital mute */
> >   	for_each_rtd_codec_dai(rtd, i, codec_dai) {
> > -		int playback_active = codec_dai->stream_active[SNDRV_PCM_STREAM_PLAYBACK];
> > -		int capture_active  = codec_dai->stream_active[SNDRV_PCM_STREAM_CAPTURE];
> > -
> > -		if ((playback && playback_active == 1) ||
> > -		    (!playback && capture_active == 1))
> > +		if (codec_dai->active == 1)
> 
> nit-pick: we have two tests in soc-pcm.c
> 
> if (codec_dai->active)
> if (codec_dai->active == 1)
> 
> The two are functionality equivalent but it'd be good to choose one
> version - or possibly use 'active' as a boolean.

In my understanding, dai->active can be 0/1/2.
But yes, we want to call mute if codec_dai is working

	/* Call Mute if Codec DAI is working */
	if (codec_dai->active)

	/* Call Mute if Codec DAI is used only for Playback or Capture */
	if (codec_dai->active == 1)

Thank you for your help !!
Best regards
---
Kuninori Morimoto

Re: [PATCH 6/8] ASoC: soc-pcm: check DAI's activity more simply

[Pierre-Louis Bossart]

>>> -		if ((playback && playback_active == 1) ||
>>> -		    (!playback && capture_active == 1))
>>> +		if (codec_dai->active == 1)
>>
>> nit-pick: we have two tests in soc-pcm.c
>>
>> if (codec_dai->active)
>> if (codec_dai->active == 1)
>>
>> The two are functionality equivalent but it'd be good to choose one
>> version - or possibly use 'active' as a boolean.
> 
> In my understanding, dai->active can be 0/1/2.

I see, I guess I missed this completely. Thanks Morimoto-san for the 
precision.

Re: [PATCH 6/8] ASoC: soc-pcm: check DAI's activity more simply

[Kuninori Morimoto]

Hi Pierre-Louis

> >>> -		if ((playback && playback_active == 1) ||
> >>> -		    (!playback && capture_active == 1))
> >>> +		if (codec_dai->active == 1)
> >> 
> >> nit-pick: we have two tests in soc-pcm.c
> >> 
> >> if (codec_dai->active)
> >> if (codec_dai->active == 1)
> >> 
> >> The two are functionality equivalent but it'd be good to choose one
> >> version - or possibly use 'active' as a boolean.
> > 
> > In my understanding, dai->active can be 0/1/2.
> 
> I see, I guess I missed this completely. Thanks Morimoto-san for the
> precision.

But, we want to use "if (codec_dai->active)" anyway.
Your review indicated my mistake.

Thank you for your help !!
Best regards
---
Kuninori Morimoto

Re: [PATCH 6/8] ASoC: soc-pcm: check DAI's activity more simply

[Pierre-Louis Bossart]

>>>>> -		if ((playback && playback_active == 1) ||
>>>>> -		    (!playback && capture_active == 1))
>>>>> +		if (codec_dai->active == 1)
>>>>
>>>> nit-pick: we have two tests in soc-pcm.c
>>>>
>>>> if (codec_dai->active)
>>>> if (codec_dai->active == 1)
>>>>
>>>> The two are functionality equivalent but it'd be good to choose one
>>>> version - or possibly use 'active' as a boolean.
>>>
>>> In my understanding, dai->active can be 0/1/2.
>>
>> I see, I guess I missed this completely. Thanks Morimoto-san for the
>> precision.
> 
> But, we want to use "if (codec_dai->active)" anyway.
> Your review indicated my mistake.

Not in this case though, the initial idea was to do the mute when only 
playback or capture were enabled? If you mute when both are enabled then 
that's a real change.

Re: [PATCH 6/8] ASoC: soc-pcm: check DAI's activity more simply

[Kuninori Morimoto]

Hi Pierre-Louis

> Not in this case though, the initial idea was to do the mute when only
> playback or capture were enabled? If you mute when both are enabled
> then that's a real change.

Let's check original

	if ((playback && playback_active == 1) ||
	    (!playback && capture_active == 1))
		snd_soc_dai_digital_mute(...)

It calls mute if Playback or Capture is working.
and my patch (v2) is

	if (codec_dai->activity)
		snd_soc_dai_digital_mute(...)

It calls mute if Codec DAI is working (= Playback or Capture).
I think it doesn't change behavior.

Thank you for your help !!
Best regards
---
Kuninori Morimoto

Re: [PATCH 6/8] ASoC: soc-pcm: check DAI's activity more simply

[Kuninori Morimoto]

Hi Pierre-Louis, again

> > Not in this case though, the initial idea was to do the mute when only
> > playback or capture were enabled? If you mute when both are enabled
> > then that's a real change.
> 
> Let's check original
> 
> 	if ((playback && playback_active == 1) ||
> 	    (!playback && capture_active == 1))
> 		snd_soc_dai_digital_mute(...)
> 
> It calls mute if Playback or Capture is working.
> and my patch (v2) is
> 
> 	if (codec_dai->activity)
> 		snd_soc_dai_digital_mute(...)
> 
> It calls mute if Codec DAI is working (= Playback or Capture).
> I think it doesn't change behavior.

Oops !
I was wrong. I will post mail to v2 patch for detail

Thank you for your help !!
Best regards
---
Kuninori Morimoto