[PATCH 3/4] pxa2xx-i2s: Cleaner use of the FIFOs

[PATCH 3/4] pxa2xx-i2s: Cleaner use of the FIFOs

[Karl Beldan]

The FIFO logic and the registers are reset at stream startup with
SACR0_RST => the sibling function might suffer and the FIFOs might
unpleasantly fill up whence the calls to pxa_i2s_wait all over the place.
This gets rid of it.

Signed-off-by: Karl Beldan <karl.beldan@mobile-devices.fr>
---
 sound/soc/pxa/pxa2xx-i2s.c |   20 +-------------------
 1 files changed, 1 insertions(+), 19 deletions(-)

diff --git a/sound/soc/pxa/pxa2xx-i2s.c b/sound/soc/pxa/pxa2xx-i2s.c
index 0e53d07..755e2d1 100644
--- a/sound/soc/pxa/pxa2xx-i2s.c
+++ b/sound/soc/pxa/pxa2xx-i2s.c
@@ -106,25 +106,12 @@ static int pxa2xx_i2s_startup(struct snd_pcm_substream *substream,
 	if (IS_ERR(clk_i2s))
 		return PTR_ERR(clk_i2s);
 
-	if (!cpu_dai->active) {
-		SACR0 |= SACR0_RST;
+	if (!cpu_dai->active)
 		SACR0 = 0;
-	}
 
 	return 0;
 }
 
-/* wait for I2S controller to be ready */
-static int pxa_i2s_wait(void)
-{
-	int i;
-
-	/* flush the Rx FIFO */
-	for(i = 0; i < 16; i++)
-		SADR;
-	return 0;
-}
-
 static int pxa2xx_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 		unsigned int fmt)
 {
@@ -169,7 +156,6 @@ static int pxa2xx_i2s_hw_params(struct snd_pcm_substream *substream,
 
 	BUG_ON(IS_ERR(clk_i2s));
 	clk_enable(clk_i2s);
-	pxa_i2s_wait();
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		cpu_dai->dma_data = &pxa2xx_i2s_pcm_stereo_out;
@@ -256,7 +242,6 @@ static void pxa2xx_i2s_shutdown(struct snd_pcm_substream *substream,
 
 	if ((SACR1 & (SACR1_DREC | SACR1_DRPL)) == (SACR1_DREC | SACR1_DRPL)) {
 		SACR0 &= ~SACR0_ENB;
-		pxa_i2s_wait();
 		clk_disable(clk_i2s);
 	}
 }
@@ -275,7 +260,6 @@ static int pxa2xx_i2s_suspend(struct snd_soc_dai *dai)
 
 	/* deactivate link */
 	SACR0 &= ~SACR0_ENB;
-	pxa_i2s_wait();
 	return 0;
 }
 
@@ -284,8 +268,6 @@ static int pxa2xx_i2s_resume(struct snd_soc_dai *dai)
 	if (!dai->active)
 		return 0;
 
-	pxa_i2s_wait();
-
 	SACR0 = pxa_i2s.sacr0 &= ~SACR0_ENB;
 	SACR1 = pxa_i2s.sacr1;
 	SAIMR = pxa_i2s.saimr;
-- 
1.6.3.rc1.34.g0be9b

-- 
Karl

Re: [PATCH 3/4] pxa2xx-i2s: Cleaner use of the FIFOs

[Mark Brown]

On Fri, May 08, 2009 at 01:53:55AM +0200, Karl Beldan wrote:

> The FIFO logic and the registers are reset at stream startup with
> SACR0_RST => the sibling function might suffer and the FIFOs might
> unpleasantly fill up whence the calls to pxa_i2s_wait all over the place.
> This gets rid of it.

The change to stream startup makes sense but...

> @@ -256,7 +242,6 @@ static void pxa2xx_i2s_shutdown(struct snd_pcm_substream *substream,
>  
>  	if ((SACR1 & (SACR1_DREC | SACR1_DRPL)) == (SACR1_DREC | SACR1_DRPL)) {
>  		SACR0 &= ~SACR0_ENB;
> -		pxa_i2s_wait();
>  		clk_disable(clk_i2s);
>  	}
>  }
> @@ -275,7 +260,6 @@ static int pxa2xx_i2s_suspend(struct snd_soc_dai *dai)
>  
>  	/* deactivate link */
>  	SACR0 &= ~SACR0_ENB;
> -	pxa_i2s_wait();
>  	return 0;
>  }

...there's also changes to the suspend and resume paths here which seem
like they'd be as well not to do simply for robustness.  Is there any
great reason for doing this or are you just doing it for neatness, the
changelog isn't entirely clear?

Re: [PATCH 3/4] pxa2xx-i2s: Cleaner use of the FIFOs

[Karl Beldan]

On Fri, May 8, 2009 at 12:57 PM, Mark Brown <broonie@sirena.org.uk> wrote:
> On Fri, May 08, 2009 at 01:53:55AM +0200, Karl Beldan wrote:
>
>> The FIFO logic and the registers are reset at stream startup with
>> SACR0_RST => the sibling function might suffer and the FIFOs might
>> unpleasantly fill up whence the calls to pxa_i2s_wait all over the place.
>> This gets rid of it.
>
> The change to stream startup makes sense but...
>
>> @@ -256,7 +242,6 @@ static void pxa2xx_i2s_shutdown(struct snd_pcm_substream *substream,
>>
>>       if ((SACR1 & (SACR1_DREC | SACR1_DRPL)) == (SACR1_DREC | SACR1_DRPL)) {
>>               SACR0 &= ~SACR0_ENB;
>> -             pxa_i2s_wait();
>>               clk_disable(clk_i2s);
>>       }
>>  }
>> @@ -275,7 +260,6 @@ static int pxa2xx_i2s_suspend(struct snd_soc_dai *dai)
>>
>>       /* deactivate link */
>>       SACR0 &= ~SACR0_ENB;
>> -     pxa_i2s_wait();
>>       return 0;
>>  }
>
> ...there's also changes to the suspend and resume paths here which seem
> like they'd be as well not to do simply for robustness.  Is there any
> great reason for doing this or are you just doing it for neatness, the
> changelog isn't entirely clear?
>

The logic was crippled by this reset and the activation of both
functions each time .. at the time
I may have over overlooked that the fifo still might still need
flushing when cleaning that mess up.
One such situation I can think of right now might be when calling
snd_pcm_release_substream: it
does 'hw_free' before 'close' so pcm  won't flush cpu_dai fifo and we
end up with 'garbage' in the fifo and
one spurious irq.
Anyways being unsure of the sequence of events it might well be uncary
to get rid of these, will get back to you.
I will be able to test this monday or so.

BTW, thx for the review.


-- 
Karl

Re: [PATCH 3/4] pxa2xx-i2s: Cleaner use of the FIFOs

[Karl Beldan]

On Fri, May 8, 2009 at 10:07 PM, Karl Beldan <karl.beldan@gmail.com> wrote:
> On Fri, May 8, 2009 at 12:57 PM, Mark Brown <broonie@sirena.org.uk> wrote:
>> On Fri, May 08, 2009 at 01:53:55AM +0200, Karl Beldan wrote:
>>
>>> The FIFO logic and the registers are reset at stream startup with
>>> SACR0_RST => the sibling function might suffer and the FIFOs might
>>> unpleasantly fill up whence the calls to pxa_i2s_wait all over the place.
>>> This gets rid of it.
>>
>> The change to stream startup makes sense but...
>>
>>> @@ -256,7 +242,6 @@ static void pxa2xx_i2s_shutdown(struct snd_pcm_substream *substream,
>>>
>>>       if ((SACR1 & (SACR1_DREC | SACR1_DRPL)) == (SACR1_DREC | SACR1_DRPL)) {
>>>               SACR0 &= ~SACR0_ENB;
>>> -             pxa_i2s_wait();
>>>               clk_disable(clk_i2s);
>>>       }
>>>  }
>>> @@ -275,7 +260,6 @@ static int pxa2xx_i2s_suspend(struct snd_soc_dai *dai)
>>>
>>>       /* deactivate link */
>>>       SACR0 &= ~SACR0_ENB;
>>> -     pxa_i2s_wait();
>>>       return 0;
>>>  }
>>
>> ...there's also changes to the suspend and resume paths here which seem
>> like they'd be as well not to do simply for robustness.  Is there any
>> great reason for doing this or are you just doing it for neatness, the
>> changelog isn't entirely clear?
>>
>
> The logic was crippled by this reset and the activation of both
> functions each time .. at the time
> I may have over overlooked that the fifo still might still need
> flushing when cleaning that mess up.
> One such situation I can think of right now might be when calling
> snd_pcm_release_substream: it
> does 'hw_free' before 'close' so pcm  won't flush cpu_dai fifo and we
> end up with 'garbage' in the fifo and
> one spurious irq.
> Anyways being unsure of the sequence of events it might well be uncary
> to get rid of these, will get back to you.

If I recall correctly flushing the fifo works only when the
corresponding function is enabled, making the calls in
shutdown/suspend useless, whence the overlook, will check.

-- 
Karl