[PATCH] ALSA: ASoC: fix PXA SSP port resume

[PATCH] ALSA: ASoC: fix PXA SSP port resume

[Daniel Mack]

Invalidate the cached hardware format on resume for PXA SSP ports.
Otherwise hw_params() will bail out early at the next stream start,
leaving the registers in a bogus state.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Philipp Zabel <philipp.zabel@gmail.com>
---
 sound/soc/pxa/pxa-ssp.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
index 3bd7712..ebde673 100644
--- a/sound/soc/pxa/pxa-ssp.c
+++ b/sound/soc/pxa/pxa-ssp.c
@@ -146,6 +146,9 @@ static int pxa_ssp_resume(struct snd_soc_dai *cpu_dai)
 {
 	struct ssp_priv *priv = cpu_dai->private_data;
 
+	/* the cached format information is invalid now */
+	priv->dai_fmt = (unsigned int) -1;
+
 	if (!cpu_dai->active)
 		return 0;
 
-- 
1.6.3.3

Re: [PATCH] ALSA: ASoC: fix PXA SSP port resume

[Mark Brown]

On Thu, Jan 28, 2010 at 06:34:18PM +0800, Daniel Mack wrote:
> Invalidate the cached hardware format on resume for PXA SSP ports.
> Otherwise hw_params() will bail out early at the next stream start,
> leaving the registers in a bogus state.

Would it not be better to write the cached state back to the hardware?
Putting an invalid value in the cache seems like asking for trouble.

Re: [PATCH] ALSA: ASoC: fix PXA SSP port resume

[Daniel Mack]

On Fri, Jan 29, 2010 at 10:13:08AM +0000, Mark Brown wrote:
> On Thu, Jan 28, 2010 at 06:34:18PM +0800, Daniel Mack wrote:
> > Invalidate the cached hardware format on resume for PXA SSP ports.
> > Otherwise hw_params() will bail out early at the next stream start,
> > leaving the registers in a bogus state.
> 
> Would it not be better to write the cached state back to the hardware?
> Putting an invalid value in the cache seems like asking for trouble.

Hmm. I considered that, but the reason why the system failed to resume
in my case was that the ssp port was not active at this time. Otherwise,
the suspend/resume code would have already done the right thing.

And as the next client will indirectly call hw_params() again, the code
there will do the right thing with the invalidated cache.

Restoring the register values from priv->dai_fmt would imply adding code
to do everything what the other bit-fiddling functions do in a reverse
manner. Which is something I'd like to avoid :)

Another idea is to unconditionally save and restore the register set,
and deal with possible side-effects. Not sure what's really better.

Daniel

Re: [PATCH] ALSA: ASoC: fix PXA SSP port resume

[Mark Brown]

On Fri, Jan 29, 2010 at 12:08:36PM +0100, Daniel Mack wrote:

> And as the next client will indirectly call hw_params() again, the code
> there will do the right thing with the invalidated cache.

Yeah, I know the code is actually reasonably safe - it just doesn't give
me the warm and fuzzies to do the invalidation by programming a random
value in there simply on the off chance that at some point in the future
that winds up being a valid value and so the same problem recurs.

> Restoring the register values from priv->dai_fmt would imply adding code
> to do everything what the other bit-fiddling functions do in a reverse
> manner. Which is something I'd like to avoid :)

Oh, right.  I'd forgotten that that was what it was actually storing.

> Another idea is to unconditionally save and restore the register set,
> and deal with possible side-effects. Not sure what's really better.

I think that'd be the the most robust solution with the way things are
structured in the driver.

[PATCH] ALSA: ASoC: fix PXA SSP port resume

[Daniel Mack]

Unconditionally save the register states in resume and restore them
again at resume time. Register contents are not preserved over suspend,
and the driver takes false assumptions about them otherwise.

The clock must be enabled to access the register block.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Philipp Zabel <philipp.zabel@gmail.com>
---
 sound/soc/pxa/pxa-ssp.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
index 3bd7712..e69397f 100644
--- a/sound/soc/pxa/pxa-ssp.c
+++ b/sound/soc/pxa/pxa-ssp.c
@@ -135,10 +135,11 @@ static int pxa_ssp_suspend(struct snd_soc_dai *cpu_dai)
 	struct ssp_priv *priv = cpu_dai->private_data;
 
 	if (!cpu_dai->active)
-		return 0;
+		clk_enable(priv->dev.ssp->clk);
 
 	ssp_save_state(&priv->dev, &priv->state);
 	clk_disable(priv->dev.ssp->clk);
+
 	return 0;
 }
 
@@ -146,12 +147,13 @@ static int pxa_ssp_resume(struct snd_soc_dai *cpu_dai)
 {
 	struct ssp_priv *priv = cpu_dai->private_data;
 
-	if (!cpu_dai->active)
-		return 0;
-
 	clk_enable(priv->dev.ssp->clk);
 	ssp_restore_state(&priv->dev, &priv->state);
-	ssp_enable(&priv->dev);
+
+	if (cpu_dai->active)
+		ssp_enable(&priv->dev);
+	else
+		clk_disable(priv->dev.ssp->clk);
 
 	return 0;
 }
-- 
1.6.3.3

Re: [PATCH] ALSA: ASoC: fix PXA SSP port resume

[Liam Girdwood]

Looks fine except for commit message (sorry to nitpick).

On Tue, 2010-02-02 at 11:32 +0800, Daniel Mack wrote:
> Unconditionally save the register states in resume 

I assume you mean suspend here ?

> and restore them
> again at resume time. Register contents are not preserved over suspend,

were not being preserved

> and the driver takes false assumptions about them otherwise.
> 

Thanks

Liam

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

[PATCH] ALSA: ASoC: fix PXA SSP port resume

[Daniel Mack]

Unconditionally save the register states when suspending and restore
them again at resume time. Register contents were not preserved over
suspend, and hence the driver takes false assumptions about them.

The clock must be enabled to access the register block.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Philipp Zabel <philipp.zabel@gmail.com>
---
 sound/soc/pxa/pxa-ssp.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
index 3bd7712..e69397f 100644
--- a/sound/soc/pxa/pxa-ssp.c
+++ b/sound/soc/pxa/pxa-ssp.c
@@ -135,10 +135,11 @@ static int pxa_ssp_suspend(struct snd_soc_dai *cpu_dai)
 	struct ssp_priv *priv = cpu_dai->private_data;
 
 	if (!cpu_dai->active)
-		return 0;
+		clk_enable(priv->dev.ssp->clk);
 
 	ssp_save_state(&priv->dev, &priv->state);
 	clk_disable(priv->dev.ssp->clk);
+
 	return 0;
 }
 
@@ -146,12 +147,13 @@ static int pxa_ssp_resume(struct snd_soc_dai *cpu_dai)
 {
 	struct ssp_priv *priv = cpu_dai->private_data;
 
-	if (!cpu_dai->active)
-		return 0;
-
 	clk_enable(priv->dev.ssp->clk);
 	ssp_restore_state(&priv->dev, &priv->state);
-	ssp_enable(&priv->dev);
+
+	if (cpu_dai->active)
+		ssp_enable(&priv->dev);
+	else
+		clk_disable(priv->dev.ssp->clk);
 
 	return 0;
 }
-- 
1.6.3.3

Re: [PATCH] ALSA: ASoC: fix PXA SSP port resume

[Mark Brown]

On Tue, Feb 02, 2010 at 06:45:27PM +0800, Daniel Mack wrote:
> Unconditionally save the register states when suspending and restore
> them again at resume time. Register contents were not preserved over
> suspend, and hence the driver takes false assumptions about them.
> 
> The clock must be enabled to access the register block.

Applied, thanks.