[PATCH] ASoC: samsung - Don't setup i2s if already active.

[PATCH] ASoC: samsung - Don't setup i2s if already active.

[Dylan Reid]

If the dai is already running when setup is called, skip setup.  Setting
up again caused set_sysclk to set rclk_srcrate to other->rclk_srcrate,
other would often be set to the default of 44.1.  Playing a 48k stream
then adding a 48k capture stream would lead to both streams set to 44.1
(The value of other->rclk_srcrate).

Similarly in shutdown, if either playback or capture is still active,
return instead of turning off the clocks.

Signed-off-by: Dylan Reid <dgreid@chromium.org>
---
 sound/soc/samsung/i2s.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index 40b00a1..b9935dd 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -637,6 +637,10 @@ static int i2s_startup(struct snd_pcm_substream *substream,
 	struct i2s_dai *other = i2s->pri_dai ? : i2s->sec_dai;
 	unsigned long flags;
 
+	/* Check not already running. */
+	if (dai->playback_active || dai->capture_active)
+		return 0;
+
 	spin_lock_irqsave(&lock, flags);
 
 	i2s->mode |= DAI_OPENED;
@@ -661,6 +665,10 @@ static void i2s_shutdown(struct snd_pcm_substream *substream,
 	struct i2s_dai *other = i2s->pri_dai ? : i2s->sec_dai;
 	unsigned long flags;
 
+	/* Check not still running. */
+	if (dai->playback_active || dai->capture_active)
+		return;
+
 	spin_lock_irqsave(&lock, flags);
 
 	i2s->mode &= ~DAI_OPENED;
-- 
1.7.12.146.g16d26b1

Re: [PATCH] ASoC: samsung - Don't setup i2s if already active.

[Mark Brown]

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

On Thu, Nov 08, 2012 at 02:46:56PM -0800, Dylan Reid wrote:
> If the dai is already running when setup is called, skip setup.  Setting
> up again caused set_sysclk to set rclk_srcrate to other->rclk_srcrate,
> other would often be set to the default of 44.1.  Playing a 48k stream
> then adding a 48k capture stream would lead to both streams set to 44.1
> (The value of other->rclk_srcrate).

This doesn't seem the obvious fix here - surely if the setup comes out
with the wrong answer the configuration it was asked for doesn't match
the configuration the device currently has and therefore we should
return an error as the new stream will have an incorrect setup?

Really this driver should probably be being redone in terms of the DPCM
code, it predates it and is a very simple case of it but it's in the
same ballpark feature wise.

> Similarly in shutdown, if either playback or capture is still active,
> return instead of turning off the clocks.

This seems sensible; another option is to do clk_enable() for both
streams then let the clock framework refcount for us.

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

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

Re: [PATCH] ASoC: samsung - Don't setup i2s if already active.

[Dylan Reid]

On Fri, Nov 9, 2012 at 8:08 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Nov 08, 2012 at 02:46:56PM -0800, Dylan Reid wrote:
>> If the dai is already running when setup is called, skip setup.  Setting
>> up again caused set_sysclk to set rclk_srcrate to other->rclk_srcrate,
>> other would often be set to the default of 44.1.  Playing a 48k stream
>> then adding a 48k capture stream would lead to both streams set to 44.1
>> (The value of other->rclk_srcrate).
>
> This doesn't seem the obvious fix here - surely if the setup comes out
> with the wrong answer the configuration it was asked for doesn't match
> the configuration the device currently has and therefore we should
> return an error as the new stream will have an incorrect setup?

Thanks Mark, sorry I didn't explain this better.  It is getting the
correct answer.  However if the same format is requested for another
stream, startup will clear rclk_srcrate.  rclk_srcrate being cleared
causes i2s_set_fmt to call i2s_set_sysclk.  i2s_set_sysclk assumes it
should use the "other" setting for rclk_srcrate (because something is
active).

for example:

aplay -Dhw:0 some_48k_file.wav
followed by
arecord -Dhw:0 -f dat /tmp/foo.wav

both 48k streams, but when the arecord is started, the aplay will have
its rate set to what ever the unused secondary DAI is using
(initialized to 44.1).

>
> Really this driver should probably be being redone in terms of the DPCM
> code, it predates it and is a very simple case of it but it's in the
> same ballpark feature wise.

That looks promising, not only for this but for a few other issues.
I'll take investigate some more, thanks for the suggestion.

>
>> Similarly in shutdown, if either playback or capture is still active,
>> return instead of turning off the clocks.
>
> This seems sensible; another option is to do clk_enable() for both
> streams then let the clock framework refcount for us.