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

Message ID 875zft97i4.wl-kuninori.morimoto.gx@renesas.com
State New
Headers show
Series
  • ASoC: soc-pcm cleanup step5
Related show

Commit Message

Kuninori Morimoto Feb. 26, 2020, 6:40 a.m. UTC
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(-)

Comments

Pierre-Louis Bossart Feb. 26, 2020, 7:04 p.m. UTC | #1
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);
>   	}
>
Kuninori Morimoto Feb. 27, 2020, 12:35 a.m. UTC | #2
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
Pierre-Louis Bossart Feb. 27, 2020, 12:40 a.m. UTC | #3
>>> -		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.
Kuninori Morimoto Feb. 27, 2020, 12:43 a.m. UTC | #4
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
Pierre-Louis Bossart Feb. 27, 2020, 1:23 a.m. UTC | #5
>>>>> -		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.
Kuninori Morimoto Feb. 27, 2020, 2 a.m. UTC | #6
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
Kuninori Morimoto Feb. 27, 2020, 2:34 a.m. UTC | #7
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

Patch
diff mbox series

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);
 	}