SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Russell King - ARM Linux]

Trying Linus' latest (as of this morning) out on a PXA255 platform with
a WM9705 codec reveals a number of problems:

1. when suspend occurs, we turn the AC97 link off by setting the
GCR_ACLINK_OFF bit, and stopping the functional units clock.

On resume, we reconfigure the pins and re-enable the clock. 
Unfortunately, this results in GCR being left with zeros, and GSR
remains all zeros, and the codec is inaccessible, with lots of
warnings from the read/write functions scrolling by:

pxa2xx_ac97_write: write error (ac97_reg=118 GSR=0x0)                           
pxa2xx_ac97_read: read error (ac97_reg=118 GSR=0x0)                             
pxa2xx_ac97_read: read error (ac97_reg=118 GSR=0x0)                             
...

Setting GCR to '2' (to release cold reset) using devmem2 starts things
moving again, shutting up this warning.


2. maybe as a result of the above problem, the wm9705 touchscreen
driver doesn't reinitialize, causing loss of touchscreen.  Unbinding
and re-binding the driver restores the touchscreen, but with a very
long lag between touching the screen and it being registered.


I bring (2) up because I notice that the resume actions in (1) are
deferred.  Given that codecs have shared functions (such as
touchscreens) need to access the codec from their own resume
functions, how can this deferral be safe?

Liam - this problem has been noticed on a NetbookPro (I'm sure you
remember that platform...)

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Mark Brown]

On Mon, Mar 30, 2009 at 11:37:20PM +0100, Russell King - ARM Linux wrote:

> 1. when suspend occurs, we turn the AC97 link off by setting the
> GCR_ACLINK_OFF bit, and stopping the functional units clock.

> Setting GCR to '2' (to release cold reset) using devmem2 starts things
> moving again, shutting up this warning.

Hrm, I suspect this is a result of the second issue.

> 2. maybe as a result of the above problem, the wm9705 touchscreen
> driver doesn't reinitialize, causing loss of touchscreen.  Unbinding
> and re-binding the driver restores the touchscreen, but with a very
> long lag between touching the screen and it being registered.

> I bring (2) up because I notice that the resume actions in (1) are
> deferred.  Given that codecs have shared functions (such as
> touchscreens) need to access the codec from their own resume
> functions, how can this deferral be safe?

Other multi-function devices shouldn't have this problem since they
will not be relying on ASoC to resume their control interface (most
likely they will be using MFD to share the device).  It's not safe for
AC97 devices, though.

We only really need the deferral for non-AC97 devices - it's there since
some I2C buses are very slow and non-AC97 codecs often have large
numbers of registers to restore and require delays to bring the codec up
cleanly leading to a substantial impact on overall resume time.

Could you let me know if the patch below works, please?  I've not fully
tested it myself yet.

>From 2c7c47109e9cf68154128e5dc96077bdf5d9a15b Mon Sep 17 00:00:00 2001
From: Mark Brown <broonie@opensource.wolfsonmicro.com>
Date: Tue, 31 Mar 2009 11:27:03 +0100
Subject: [PATCH] ASoC: Don't defer resume work for AC97 codecs

AC97 devices may have other drivers hanging off them directly so need to
have resumed when the resume function returns meaning that we can't defer
the resume - complete it immediately for them. Non-AC97 devices should
not have other drivers hanging directly off the ASoC devices.

We only really need the deferral for non-AC97 devices - it's there since
some I2C buses are very slow and non-AC97 codecs often have large numbers
of registers to restore and require delays to bring the codec up cleanly
leading to a substantial impact on overall resume time.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: stable@kernel.org
---
 sound/soc/soc-core.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 6e710f7..6c62d4a 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -767,11 +767,21 @@ static int soc_resume(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_card *card = socdev->card;
+	struct snd_soc_dai *cpu_dai = card->dai_link[0].cpu_dai;
 
-	dev_dbg(socdev->dev, "scheduling resume work\n");
-
-	if (!schedule_work(&card->deferred_resume_work))
-		dev_err(socdev->dev, "resume work item may be lost\n");
+	/* AC97 devices might have other drivers hanging off them so
+	 * need to resume immediately.  Other drivers don't have that
+	 * problem and may take a substantial amount of time to resume
+	 * due to I/O costs and anti-pop so handle them out of line.
+	 */
+	if (cpu_dai->ac97_control) {
+		dev_dbg(socdev->dev, "Resuming AC97 immediately\n");
+		soc_resume_deferred(&card->deferred_resume_work);
+	} else {
+		dev_dbg(socdev->dev, "Scheduling resume work\n");
+		if (!schedule_work(&card->deferred_resume_work))
+			dev_err(socdev->dev, "resume work item may be lost\n");
+	}
 
 	return 0;
 }
-- 
1.6.2.1

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Liam Girdwood]

On Mon, 2009-03-30 at 23:37 +0100, Russell King - ARM Linux wrote:

> Liam - this problem has been noticed on a NetbookPro (I'm sure you
> remember that platform...)

I do indeed ;)

Mark, unfortunately my netbook is dead otherwise I could have tested.  

Liam

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Russell King - ARM Linux]

On Tue, Mar 31, 2009 at 11:30:48AM +0100, Mark Brown wrote:
> On Mon, Mar 30, 2009 at 11:37:20PM +0100, Russell King - ARM Linux wrote:
> > 1. when suspend occurs, we turn the AC97 link off by setting the
> > GCR_ACLINK_OFF bit, and stopping the functional units clock.
> 
> > Setting GCR to '2' (to release cold reset) using devmem2 starts things
> > moving again, shutting up this warning.
> 
> Hrm, I suspect this is a result of the second issue.
> 
> > 2. maybe as a result of the above problem, the wm9705 touchscreen
> > driver doesn't reinitialize, causing loss of touchscreen.  Unbinding
> > and re-binding the driver restores the touchscreen, but with a very
> > long lag between touching the screen and it being registered.
> 
> > I bring (2) up because I notice that the resume actions in (1) are
> > deferred.  Given that codecs have shared functions (such as
> > touchscreens) need to access the codec from their own resume
> > functions, how can this deferral be safe?
> 
> Other multi-function devices shouldn't have this problem since they
> will not be relying on ASoC to resume their control interface (most
> likely they will be using MFD to share the device).  It's not safe for
> AC97 devices, though.
> 
> We only really need the deferral for non-AC97 devices - it's there since
> some I2C buses are very slow and non-AC97 codecs often have large
> numbers of registers to restore and require delays to bring the codec up
> cleanly leading to a substantial impact on overall resume time.
> 
> Could you let me know if the patch below works, please?  I've not fully
> tested it myself yet.

I don't think this is the complete story.  Sometime between 2.6.27 and
2.6.29, the structure below /sys/devices/platform/soc-audio changed
(the ac97 codec moved to the top level.)

This clearly isn't right.  We want to resume the host side of the link
first, followed by the AC97 codec, followed by any sub drivers next.
The way to guarantee that is to ensure that the parenting of the devices
is correct.

In other words, this structure:

/sys/devices/platform/soc-audio/ac97-device/sub-devices-eg-touchscreen

I'm not sure where wm9705-ts currently appears in the device tree (I
need to resume the device, and sort out its resume quirks so that I can
see the sysfs layout... but I'm absolutely sure that the ac97 device
appears at /sys/devices/platform/ which is definitely wrong.

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Mark Brown]

On Tue, Mar 31, 2009 at 07:37:55PM +0100, Russell King - ARM Linux wrote:

> I don't think this is the complete story.  Sometime between 2.6.27 and
> 2.6.29, the structure below /sys/devices/platform/soc-audio changed
> (the ac97 codec moved to the top level.)

Oh, are you using the ac97.c codec driver?  With a normal ASoC codec
driver the AC97 codec is not visible as a separate device in the device
tree - if you could point me to your machine driver that'd be helpful.

I'll take a look at ac97.c tomorrow; there have been some problems found
with the way it registers in that timeframe so it's likely that one of
the fixes there has caused the regression.  The way it uses the generic
ALSA AC97 code means that it needs some special casing.

As a workaround I'd suggest using the generic PXA AC97 support in
sound/arm rather than an ASoC machine driver.  Switching your machine
driver to use the WM9705 codec driver should also avoid the problems.

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Ian Molton]

Russell King - ARM Linux wrote:
> Trying Linus' latest (as of this morning) out on a PXA255 platform with
> a WM9705 codec reveals a number of problems:

are you using my wm9705 soc codec, or something else?

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Mark Brown]

On Tue, Mar 31, 2009 at 07:37:55PM +0100, Russell King - ARM Linux wrote:

> I don't think this is the complete story.  Sometime between 2.6.27 and
> 2.6.29, the structure below /sys/devices/platform/soc-audio changed
> (the ac97 codec moved to the top level.)

I've reproduced and fixed this for the ASoC native driver case (patch
below); looking at the code and history I can't immediately see how this
ever worked properly - the parent appears to have been NULL ever since
ASoC was merged into mainline.  I can't reproduce this for the ac97.c
case, the code looks correct and the behaviour at runtime also appears
correct in my tests.

I also haven't manage to reproduce the ordering issues with the register
writes with or without my fixes so I can't be 100% confident that this
will fix the problem you're seeing.  If you could post your machine
driver that'd be helpful.

> I'm not sure where wm9705-ts currently appears in the device tree (I
> need to resume the device, and sort out its resume quirks so that I can
> see the sysfs layout... but I'm absolutely sure that the ac97 device
> appears at /sys/devices/platform/ which is definitely wrong.

Note that there *is* a device for the AC97 controller visible in the
device tree but that currently it plays no role in suspend and resume
when ASoC is being used, it exists only to tell ASoC that the AC97
device has probed.  Hopefully by 2.6.31 there will be internal
synchronisation within ASoC to do the suspend and resume of the entire
ASoC card over multiple parent buses when the first and last component
get called.

>From 2ad05c2b141d14a668467cb9ec50b8beb5d19a8d Mon Sep 17 00:00:00 2001
From: Mark Brown <broonie@opensource.wolfsonmicro.com>
Date: Wed, 1 Apr 2009 19:35:01 +0100
Subject: [PATCH] ASoC: Set parent for AC97 devices we register

Ensure that any AC97 devices that bind to the CODEC are below the
ASoC device in the device tree so the suspend and resume code can
figure out what order to handle them in.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/soc-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 6c62d4a..99712f6 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -98,7 +98,7 @@ static int soc_ac97_dev_register(struct snd_soc_codec *codec)
 	int err;
 
 	codec->ac97->dev.bus = &ac97_bus_type;
-	codec->ac97->dev.parent = NULL;
+	codec->ac97->dev.parent = codec->card->dev;
 	codec->ac97->dev.release = soc_ac97_device_release;
 
 	dev_set_name(&codec->ac97->dev, "%d-%d:%s",
-- 
1.6.2.1

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Russell King - ARM Linux]

On Tue, Mar 31, 2009 at 08:15:40PM +0100, Mark Brown wrote:
> On Tue, Mar 31, 2009 at 07:37:55PM +0100, Russell King - ARM Linux wrote:
> 
> > I don't think this is the complete story.  Sometime between 2.6.27 and
> > 2.6.29, the structure below /sys/devices/platform/soc-audio changed
> > (the ac97 codec moved to the top level.)
> 
> Oh, are you using the ac97.c codec driver?

No, I'm using the wm9705 driver.

> With a normal ASoC codec
> driver the AC97 codec is not visible as a separate device in the device
> tree - if you could point me to your machine driver that'd be helpful.

Not true - the codec appears as /sys/devices/0-0:WM9705 rather than a
sub-device of the AC97 controller.  Below this there are two sub-devices:

/sys/devices/0-0:WM9705/wm97xx-battery
/sys/devices/0-0:WM9705/wm97xx-touch

The wm97xx-ts touchscreen driver binds to the 0-0:WM9705 device, so
clearly unless 0-0:WM9705 is a child of the AC97 controller, things
aren't going to be guaranteed to be resumed in the right order.

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Russell King - ARM Linux]

On Wed, Apr 01, 2009 at 08:27:10PM +0100, Mark Brown wrote:
> On Tue, Mar 31, 2009 at 07:37:55PM +0100, Russell King - ARM Linux wrote:
> > I don't think this is the complete story.  Sometime between 2.6.27 and
> > 2.6.29, the structure below /sys/devices/platform/soc-audio changed
> > (the ac97 codec moved to the top level.)
> 
> I also haven't manage to reproduce the ordering issues with the register
> writes with or without my fixes so I can't be 100% confident that this
> will fix the problem you're seeing.  If you could post your machine
> driver that'd be helpful.

My machine driver is very simple and doesn't implement any suspend/resume
callbacks (that's what other things like soc-audio are doing, right?)

I've tried both your patches, and I still have to manually write to
GCR to bring the bus out of reset.  I'll investigate more in the coming
days.

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Mark Brown]

On Thu, Apr 02, 2009 at 01:13:48PM +0100, Russell King - ARM Linux wrote:

> My machine driver is very simple and doesn't implement any suspend/resume
> callbacks (that's what other things like soc-audio are doing, right?)

Yes, that's normal - you should only need to implement suspend and
resume callbacks for the machine if there is extra hardware on the board
that is handled by the machine driver.

> I've tried both your patches, and I still have to manually write to
> GCR to bring the bus out of reset.  I'll investigate more in the coming
> days.

Which PXA variant is the system using?

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Mark Brown]

On Thu, Apr 02, 2009 at 12:55:41PM +0100, Russell King - ARM Linux wrote:
> On Tue, Mar 31, 2009 at 08:15:40PM +0100, Mark Brown wrote:

> > With a normal ASoC codec
> > driver the AC97 codec is not visible as a separate device in the device
> > tree - if you could point me to your machine driver that'd be helpful.

> Not true - the codec appears as /sys/devices/0-0:WM9705 rather than a
> sub-device of the AC97 controller.  Below this there are two sub-devices:

Ah, I see what you mean - as you say that's actually the device for the
touch functionalty of the device rather than for the codec part which is
handled in soc-audio.

> /sys/devices/0-0:WM9705/wm97xx-battery
> /sys/devices/0-0:WM9705/wm97xx-touch

> The wm97xx-ts touchscreen driver binds to the 0-0:WM9705 device, so
> clearly unless 0-0:WM9705 is a child of the AC97 controller, things
> aren't going to be guaranteed to be resumed in the right order.

Right, the second patch I sent yesterday parents the device properly so
the ordering should now be OK there.  Did you try using the generic AC97
driver?  The code for handling the controller hardware is now shared
between the ASoC and that - if it fails with both that'd point at the
controller support.

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Russell King - ARM Linux]

On Thu, Apr 02, 2009 at 01:18:03PM +0100, Mark Brown wrote:
> On Thu, Apr 02, 2009 at 01:13:48PM +0100, Russell King - ARM Linux wrote:
> > I've tried both your patches, and I still have to manually write to
> > GCR to bring the bus out of reset.  I'll investigate more in the coming
> > days.
> 
> Which PXA variant is the system using?

PXA255.

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Russell King - ARM Linux]

On Thu, Apr 02, 2009 at 01:38:44PM +0100, Russell King - ARM Linux wrote:
> On Thu, Apr 02, 2009 at 01:18:03PM +0100, Mark Brown wrote:
> > On Thu, Apr 02, 2009 at 01:13:48PM +0100, Russell King - ARM Linux wrote:
> > > I've tried both your patches, and I still have to manually write to
> > > GCR to bring the bus out of reset.  I'll investigate more in the coming
> > > days.
> > 
> > Which PXA variant is the system using?
> 
> PXA255.

Right, adding some printk's, which include into pxa2xx-ac97's reset
functions reveals...

soc_resume: entry
soc_resume_deferred: entry
pxa2xx_ac97_resume:
soc_resume_deferred: exit
soc_resume: exit
wm97xx_resume: entry
pxa2xx_ac97_write: write error (ac97_reg=118 GSR=0x0)
pxa2xx_ac97_write: write error (ac97_reg=120 GSR=0x0)
pxa2xx_ac97_write: write error (ac97_reg=76 GSR=0x0)
pxa2xx_ac97_write: write error (ac97_reg=78 GSR=0x0)
pxa2xx_ac97_write: write error (ac97_reg=80 GSR=0x0)
pxa2xx_ac97_write: write error (ac97_reg=82 GSR=0x0)
pxa2xx_ac97_write: write error (ac97_reg=84 GSR=0x0)
pxa2xx_ac97_write: write error (ac97_reg=86 GSR=0x0)
wm97xx_resume: exit

So, basically, we're starting to access the AC97 bus without first
initializing the controller.  This can't work on _any_ PXA platform.

On any reset, GCR is initialized by the hardware to zero.  That means
that the PXA AC97 hardware is held in reset state, and the codec is
held in cold reset.

On resume or probe, we need to take the hardware out of cold reset.

That happens in the probe just fine because one of the first actions
that the core ALSA AC97 code does is to issue a cold reset - see
sound/pci/ac97/ac97_codec.c:snd_ac97_mixer() where it calls the
bus reset() method.

And here we have the problem... using Ian's wm9705 driver, it doesn't
implement any suspend/resume ops.  I assume Ian's e-series platforms
aren't suspend-able.

Having added the suspend/resume functions to the driver in the same
manner as soc/codecs/ac97.c:

static int wm9705_soc_suspend(struct platform_device *pdev, pm_message_t msg)
{
        struct snd_soc_device *socdev = platform_get_drvdata(pdev);
        snd_ac97_suspend(socdev->card->codec->ac97);
        return 0;
}

static int wm9705_soc_resume(struct platform_device *pdev)
{
        struct snd_soc_device *socdev = platform_get_drvdata(pdev);
        snd_ac97_resume(socdev->card->codec->ac97);
        return 0;
} 

I now find that the board crashes inexplicably on suspend... because...
Argh, Ian, what pile of mess have you created in wm9705?

There's *no* AC97 initialization in there.  Whereas ac97.c does:

        /* add codec as bus device for standard ac97 */
        ret = snd_ac97_bus(codec->card, 0, &soc_ac97_ops, NULL, &ac97_bus);
        if (ret < 0)
                goto bus_err;
        
        memset(&ac97_template, 0, sizeof(struct snd_ac97_template));
        ret = snd_ac97_mixer(ac97_bus, &ac97_template, &codec->ac97);
        if (ret < 0)
                goto bus_err;

You do:

        ret = snd_soc_new_ac97_codec(codec, &soc_ac97_ops, 0);

which doesn't actually give us an AC97 codec device - it only kmallocs
some memory for them.

Okay, I'm going to shelve this project for at least the next three
months now, and will return to it when (and if) the ASoC codec
situation improves.

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Mark Brown]

On Thu, Apr 02, 2009 at 02:31:24PM +0100, Russell King - ARM Linux wrote:

> So, basically, we're starting to access the AC97 bus without first
> initializing the controller.  This can't work on _any_ PXA platform.

Ah, that explains it - yes, this just isn't going to do suspend/resume
as-is (but see the patch below).

> That happens in the probe just fine because one of the first actions
> that the core ALSA AC97 code does is to issue a cold reset - see
> sound/pci/ac97/ac97_codec.c:snd_ac97_mixer() where it calls the
> bus reset() method.

The core ALSA AC97 code shouldn't be getting involved for ASoC-specific
CODEC drivers.  An ASoC-specific driver should be doing whatever is
required to the CODEC directly - one of the reasons for having an
ASoC-specific driver is to allow more detailed management of the device
than can be supported within the standard ALSA AC97 framework.

ac97.c calls into the standard ALSA AC97 code to provide an adaption
layer which allows the generic AC97 support to be used with an ASoC AC97
controller driver for devices which don't need or want any additional
support.

> I now find that the board crashes inexplicably on suspend... because...
> Argh, Ian, what pile of mess have you created in wm9705?

> You do:

>         ret = snd_soc_new_ac97_codec(codec, &soc_ac97_ops, 0);

> which doesn't actually give us an AC97 codec device - it only kmallocs
> some memory for them.

That's what an ASoC AC97 driver should do - it is responsible for
bringing up the link by issuing a reset operation (which the wm9705
driver is doing) and doing any other setup required.  The ASoC core will
then go and register the AC97 bus device for child devices like the 

> Okay, I'm going to shelve this project for at least the next three
> months now, and will return to it when (and if) the ASoC codec
> situation improves.

Before doing that could you please try the patch below to wm9705.c
implementing suspend and resume operations?  It's completely
speculative, I don't have a WM9705 test system to hand but I'm
moderately confident

>From e904fe0f0653ba66c20faa97034b99ee87602cff Mon Sep 17 00:00:00 2001
From: Mark Brown <broonie@opensource.wolfsonmicro.com>
Date: Thu, 2 Apr 2009 15:05:55 +0100
Subject: [PATCH] ASoC: Implement suspend and resume operations for WM9705

Compile tested only.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/wm9705.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c
index 3265817..0564476 100644
--- a/sound/soc/codecs/wm9705.c
+++ b/sound/soc/codecs/wm9705.c
@@ -317,6 +317,41 @@ static int wm9705_reset(struct snd_soc_codec *codec)
 	return -EIO;
 }
 
+#ifdef CONFIG_PM
+static int wm9705_soc_suspend(struct platform_device *pdev)
+{
+	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec = socdev->card->codec;
+
+	ac97_write(codec, AC97_POWERDOWN, 0xffff);
+
+	return 0;
+}
+
+static int wm9705_soc_resume(struct platform_device *pdev)
+{
+	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec = socdev->card->codec;
+	int i, ret;
+	u16 *cache = codec->reg_cache;
+
+	ret = wm9705_reset(codec);
+	if (ret < 0) {
+		printk(KERN_ERR "could not reset AC97 codec\n");
+		return ret;
+	}
+
+	for (i = 2; i < ARRAY_SIZE(wm9705_reg) << 1; i += 2) {
+		soc_ac97_ops.write(codec->ac97, i, cache[i>>1]);
+	}
+
+	return 0;
+}
+#else
+#define wm9705_soc_suspend NULL
+#define wm9705_soc_resume NULL
+#endif
+
 static int wm9705_soc_probe(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
@@ -407,6 +442,8 @@ static int wm9705_soc_remove(struct platform_device *pdev)
 struct snd_soc_codec_device soc_codec_dev_wm9705 = {
 	.probe = 	wm9705_soc_probe,
 	.remove = 	wm9705_soc_remove,
+	.suspend =	wm9705_soc_suspend,
+	.resume =	wm9705_soc_resume,
 };
 EXPORT_SYMBOL_GPL(soc_codec_dev_wm9705);
 
-- 
1.6.2.1

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Russell King - ARM Linux]

On Thu, Apr 02, 2009 at 03:12:01PM +0100, Mark Brown wrote:
> Before doing that could you please try the patch below to wm9705.c
> implementing suspend and resume operations?  It's completely
> speculative, I don't have a WM9705 test system to hand but I'm
> moderately confident

No real change - since I got rid of the debug printk's I scattered in
last time around, I don't know if this patch has changed anything.

It's actually worse though - using my devmem2 hack no longer restores
things to a functional state.  devmem2 says the control register is
now set as:

Value at address 0x4050000C: 0xC0002

but I'm still seeing lots of:

pxa2xx_ac97_write: write error (ac97_reg=118 GSR=0x0)                           
pxa2xx_ac97_read: read error (ac97_reg=118 GSR=0x0)                             
pxa2xx_ac97_read: read error (ac97_reg=118 GSR=0x0)                             
pxa2xx_ac97_read: read error (ac97_reg=118 GSR=0x0)                             
...

messages.  Before, setting the control register to '2' (thereby
releasing the cold reset) caused things to start working again.

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Mark Brown]

On Thu, Apr 02, 2009 at 03:40:17PM +0100, Russell King - ARM Linux wrote:

> It's actually worse though - using my devmem2 hack no longer restores
> things to a functional state.  devmem2 says the control register is
> now set as:

Gah, I see one problem with the patch I sent.  The one below *might* do
better if you have time to test it.  In any case, thanks a lot for your
help here - I'm fairly confident that the remaining problems you're
seeing are due to the lack of suspend and resume operations in the
WM9705 driver.

>From eeec4f57eb9d8f5f45d9536319e0eb130cc3ba28 Mon Sep 17 00:00:00 2001
From: Mark Brown <broonie@opensource.wolfsonmicro.com>
Date: Thu, 2 Apr 2009 15:49:41 +0100
Subject: [PATCH] ASoC: Implement suspend and resume operations for WM9705

Compile tested only.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/wm9705.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c
index 3265817..2109b50 100644
--- a/sound/soc/codecs/wm9705.c
+++ b/sound/soc/codecs/wm9705.c
@@ -317,6 +317,41 @@ static int wm9705_reset(struct snd_soc_codec *codec)
 	return -EIO;
 }
 
+#ifdef CONFIG_PM
+static int wm9705_soc_suspend(struct platform_device *pdev)
+{
+	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec = socdev->card->codec;
+
+	soc_ac97_ops.write(codec->ac97, AC97_POWERDONW, 0xffff);
+
+	return 0;
+}
+
+static int wm9705_soc_resume(struct platform_device *pdev)
+{
+	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec = socdev->card->codec;
+	int i, ret;
+	u16 *cache = codec->reg_cache;
+
+	ret = wm9705_reset(codec);
+	if (ret < 0) {
+		printk(KERN_ERR "could not reset AC97 codec\n");
+		return ret;
+	}
+
+	for (i = 2; i < ARRAY_SIZE(wm9705_reg) << 1; i += 2) {
+		soc_ac97_ops.write(codec->ac97, i, cache[i>>1]);
+	}
+
+	return 0;
+}
+#else
+#define wm9705_soc_suspend NULL
+#define wm9705_soc_resume NULL
+#endif
+
 static int wm9705_soc_probe(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
@@ -407,6 +442,8 @@ static int wm9705_soc_remove(struct platform_device *pdev)
 struct snd_soc_codec_device soc_codec_dev_wm9705 = {
 	.probe = 	wm9705_soc_probe,
 	.remove = 	wm9705_soc_remove,
+	.suspend =	wm9705_soc_suspend,
+	.resume =	wm9705_soc_resume,
 };
 EXPORT_SYMBOL_GPL(soc_codec_dev_wm9705);
 
-- 
1.6.2.1

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Russell King - ARM Linux]

On Thu, Apr 02, 2009 at 03:53:30PM +0100, Mark Brown wrote:
> On Thu, Apr 02, 2009 at 03:40:17PM +0100, Russell King - ARM Linux wrote:
> 
> > It's actually worse though - using my devmem2 hack no longer restores
> > things to a functional state.  devmem2 says the control register is
> > now set as:
> 
> Gah, I see one problem with the patch I sent.  The one below *might* do
> better if you have time to test it.  In any case, thanks a lot for your
> help here - I'm fairly confident that the remaining problems you're
> seeing are due to the lack of suspend and resume operations in the
> WM9705 driver.
> 
> >From eeec4f57eb9d8f5f45d9536319e0eb130cc3ba28 Mon Sep 17 00:00:00 2001
> From: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Date: Thu, 2 Apr 2009 15:49:41 +0100
> Subject: [PATCH] ASoC: Implement suspend and resume operations for WM9705
> 
> Compile tested only.

I beg to disagree with that statement...

sound/soc/codecs/wm9705.c: In function `wm9705_soc_suspend':
sound/soc/codecs/wm9705.c:326: error: `AC97_POWERDONW' undeclared (first use in this function)
sound/soc/codecs/wm9705.c:326: error: (Each undeclared identifier is reported only once
sound/soc/codecs/wm9705.c:326: error: for each function it appears in.)
sound/soc/codecs/wm9705.c: At top level:
sound/soc/codecs/wm9705.c:445: warning: initialization from incompatible pointer type

Fixing these results in something which can be built, and along with
your other two for the soc-core resume and the ac97 device parent,
the complaints from the touchscreen driver no longer appear.

Thanks.

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Mark Brown]

On Thu, Apr 02, 2009 at 04:09:22PM +0100, Russell King - ARM Linux wrote:
> On Thu, Apr 02, 2009 at 03:53:30PM +0100, Mark Brown wrote:

> > Compile tested only.

> I beg to disagree with that statement...

Sorry about that - spot the use of git commit --amend.

> Fixing these results in something which can be built, and along with
> your other two for the soc-core resume and the ac97 device parent,
> the complaints from the touchscreen driver no longer appear.

Excellent - I'll push the fixed patches to Takashi when he's back from
vacation.  Thanks again for your work tracking this down.

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Ian Molton]

Russell King - ARM Linux wrote:

> And here we have the problem... using Ian's wm9705 driver, it doesn't
> implement any suspend/resume ops.  I assume Ian's e-series platforms
> aren't suspend-able.

Correct - at present.

> I now find that the board crashes inexplicably on suspend... because...
> Argh, Ian, what pile of mess have you created in wm9705?

Um no. Its correct. The core handles the init.

Anyhow, I see that you and Mark have tracked this down, nice work.

Nice to know that my codec driver has come in useful beyond my own 
e-series platform.

-Ian

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Russell King - ARM Linux]

On Thu, Apr 02, 2009 at 04:22:34PM +0100, Mark Brown wrote:
> On Thu, Apr 02, 2009 at 04:09:22PM +0100, Russell King - ARM Linux wrote:
> > On Thu, Apr 02, 2009 at 03:53:30PM +0100, Mark Brown wrote:
> 
> > > Compile tested only.
> 
> > I beg to disagree with that statement...
> 
> Sorry about that - spot the use of git commit --amend.
> 
> > Fixing these results in something which can be built, and along with
> > your other two for the soc-core resume and the ac97 device parent,
> > the complaints from the touchscreen driver no longer appear.
> 
> Excellent - I'll push the fixed patches to Takashi when he's back from
> vacation.  Thanks again for your work tracking this down.

I notice that the fixes were merged, minus one:

sound/soc/codecs/wm9705.c: At top level:
sound/soc/codecs/wm9705.c:445: warning: initialization from incompatible pointer type

so you might find this trivial patch useful.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c
index 6e23a81..c2d1a7a 100644
--- a/sound/soc/codecs/wm9705.c
+++ b/sound/soc/codecs/wm9705.c
@@ -318,7 +318,7 @@ static int wm9705_reset(struct snd_soc_codec *codec)
 }
 
 #ifdef CONFIG_PM
-static int wm9705_soc_suspend(struct platform_device *pdev)
+static int wm9705_soc_suspend(struct platform_device *pdev, pm_message_t msg)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_codec *codec = socdev->card->codec;

Re: SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume

[Mark Brown]

On Sat, Apr 18, 2009 at 10:11:53AM +0100, Russell King - ARM Linux wrote:

> so you might find this trivial patch useful.

Applied, thanks.