[PATCH] ASoC: soc-pcm: Only do snd_soc_dai_digital_mute() in hw_free()

[PATCH] ASoC: soc-pcm: Only do snd_soc_dai_digital_mute() in hw_free()

[Nicolin Chen]

This patch removed the redundant snd_soc_dai_digital_mute() in close() since
it is better to mute in hw_free() which is slightly earlier and symmetrical
for the case of reconfiguration, 'aplay 44100.wav 48000.wav' for example:
open()->hw_params()->prepare(unmute)->playing->hw_free(mute)->hw_params()->
parepare(unmute)->playing->hw_free(mute)->close()

This patch also corrects the condition of the snd_soc_dai_digital_mute() in
hw_free() because it would not be executed as we only decrease codec->active
in snd_soc_close().

Signed-off-by: Nicolin Chen <b42378@freescale.com>
---
 sound/soc/soc-pcm.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index eb340a8..eac82858 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -474,11 +474,6 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	codec_dai->active--;
 	codec->active--;
 
-	/* Muting the DAC suppresses artifacts caused during digital
-	 * shutdown, for example from stopping clocks.
-	 */
-	snd_soc_dai_digital_mute(codec_dai, 1, substream->stream);
-
 	if (cpu_dai->driver->ops->shutdown)
 		cpu_dai->driver->ops->shutdown(substream, cpu_dai);
 
@@ -689,7 +684,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
 	struct snd_soc_platform *platform = rtd->platform;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
-	struct snd_soc_codec *codec = rtd->codec;
+	bool playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ? 1 : 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
@@ -707,7 +702,8 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
 	}
 
 	/* apply codec digital mute */
-	if (!codec->active)
+	if ((playback && codec_dai->playback_active == 1) ||
+			(!playback && codec_dai->capture_active == 1))
 		snd_soc_dai_digital_mute(codec_dai, 1, substream->stream);
 
 	/* free any machine hw params */
-- 
1.8.4

Re: [PATCH] ASoC: soc-pcm: Only do snd_soc_dai_digital_mute() in hw_free()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1185 bytes --]

On Tue, Dec 03, 2013 at 05:16:06PM +0800, Nicolin Chen wrote:

> @@ -689,7 +684,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
>  	struct snd_soc_platform *platform = rtd->platform;
>  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>  	struct snd_soc_dai *codec_dai = rtd->codec_dai;
> -	struct snd_soc_codec *codec = rtd->codec;
> +	bool playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ? 1 : 0;

Please don't use the ternery operator, you could've just used the result
of the comparison directly here.

>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>  
> @@ -707,7 +702,8 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
>  	}
>  
>  	/* apply codec digital mute */
> -	if (!codec->active)
> +	if ((playback && codec_dai->playback_active == 1) ||
> +			(!playback && codec_dai->capture_active == 1))
>  		snd_soc_dai_digital_mute(codec_dai, 1, substream->stream);

This is a bit confusing to read due to the strange indentation (the
second line is massively indented with respect to the first line for
some reason).  This hunk also ought to be split out as a separate patch
since it's a bug fix.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: soc-pcm: Only do snd_soc_dai_digital_mute() in hw_free()

[Nicolin Chen]

On Tue, Dec 03, 2013 at 03:04:23PM +0000, Mark Brown wrote:
> On Tue, Dec 03, 2013 at 05:16:06PM +0800, Nicolin Chen wrote:
> 
> > @@ -689,7 +684,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
> >  	struct snd_soc_platform *platform = rtd->platform;
> >  	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> >  	struct snd_soc_dai *codec_dai = rtd->codec_dai;
> > -	struct snd_soc_codec *codec = rtd->codec;
> > +	bool playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ? 1 : 0;
> 
> Please don't use the ternery operator, you could've just used the result
> of the comparison directly here.

Oh, a bad habit. Thank you for teaching me this.

> 
> >  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> >  
> > @@ -707,7 +702,8 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
> >  	}
> >  
> >  	/* apply codec digital mute */
> > -	if (!codec->active)
> > +	if ((playback && codec_dai->playback_active == 1) ||
> > +			(!playback && codec_dai->capture_active == 1))
> >  		snd_soc_dai_digital_mute(codec_dai, 1, substream->stream);
> 
> This is a bit confusing to read due to the strange indentation (the
> second line is massively indented with respect to the first line for
> some reason).  This hunk also ought to be split out as a separate patch
> since it's a bug fix.

I'll refine the indentation and split the patch into two.

Thank you indeed.
Nicolin Chen