Patchwork alsa/soc: add locking to mpc5200-psc-ac97 driver

login
register
mail settings
Submitter Grant Likely
Date June 29, 2009, 11:42 p.m.
Message ID <20090629234214.12670.9082.stgit@localhost.localdomain>
Download mbox | patch
Permalink /patch/29300/
State Accepted, archived
Delegated to: Grant Likely
Headers show

Comments

Grant Likely - June 29, 2009, 11:42 p.m.
From: Grant Likely <grant.likely@secretlab.ca>

AC97 bus register read/write hooks need to provide locking, but the
mpc5200-psc-ac97 driver does not.  This patch adds a mutex around
the register access routines.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 sound/soc/fsl/mpc5200_dma.c      |    1 +
 sound/soc/fsl/mpc5200_dma.h      |    1 +
 sound/soc/fsl/mpc5200_psc_ac97.c |   17 ++++++++++++++++-
 3 files changed, 18 insertions(+), 1 deletions(-)
Jon Smirl - June 30, 2009, 12:26 a.m.
On Mon, Jun 29, 2009 at 7:42 PM, Grant Likely<grant.likely@secretlab.ca> wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> AC97 bus register read/write hooks need to provide locking, but the
> mpc5200-psc-ac97 driver does not.  This patch adds a mutex around
> the register access routines.

Does your touchscreen driver use this mutex? Or was this mutex needed
just for the AC97 driver?
Wolfram Sang - June 30, 2009, 6:18 a.m.
Hi Grant,

On Mon, Jun 29, 2009 at 05:42:21PM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> 
> AC97 bus register read/write hooks need to provide locking, but the
> mpc5200-psc-ac97 driver does not.  This patch adds a mutex around
> the register access routines.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> 
>  sound/soc/fsl/mpc5200_dma.c      |    1 +
>  sound/soc/fsl/mpc5200_dma.h      |    1 +
>  sound/soc/fsl/mpc5200_psc_ac97.c |   17 ++++++++++++++++-
>  3 files changed, 18 insertions(+), 1 deletions(-)
> 
> 
> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> index efec33a..f0a2d40 100644
> --- a/sound/soc/fsl/mpc5200_dma.c
> +++ b/sound/soc/fsl/mpc5200_dma.c
> @@ -456,6 +456,7 @@ int mpc5200_audio_dma_create(struct of_device *op)
>  		return -ENODEV;
>  
>  	spin_lock_init(&psc_dma->lock);
> +	mutex_init(&psc_dma->mutex);
>  	psc_dma->id = be32_to_cpu(*prop);
>  	psc_dma->irq = irq;
>  	psc_dma->psc_regs = regs;
> diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h
> index 2000803..8d396bb 100644
> --- a/sound/soc/fsl/mpc5200_dma.h
> +++ b/sound/soc/fsl/mpc5200_dma.h
> @@ -55,6 +55,7 @@ struct psc_dma {
>  	unsigned int irq;
>  	struct device *dev;
>  	spinlock_t lock;
> +	struct mutex mutex;
>  	u32 sicr;
>  	uint sysclk;
>  	int imr;
> diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c
> index 794a247..7eb5499 100644
> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
> @@ -34,13 +34,20 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
>  	int status;
>  	unsigned int val;
>  
> +	mutex_lock(&psc_dma->mutex);
> +
>  	/* Wait for command send status zero = ready */
>  	status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) &
>  				MPC52xx_PSC_SR_CMDSEND), 100, 0);
>  	if (status == 0) {
>  		pr_err("timeout on ac97 bus (rdy)\n");
> +		mutex_unlock(&psc_dma->mutex);
>  		return -ENODEV;
>  	}
> +
> +	/* Force clear the data valid bit */
> +	in_be32(&psc_dma->psc_regs->ac97_data);
> +

No mutex involved here. I think this is either a seperate patch or it needs at
least to be mentioned in the patch description.

>  	/* Send the read */
>  	out_be32(&psc_dma->psc_regs->ac97_cmd, (1<<31) | ((reg & 0x7f) << 24));
>  
> @@ -50,16 +57,19 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
>  	if (status == 0) {
>  		pr_err("timeout on ac97 read (val) %x\n",
>  				in_be16(&psc_dma->psc_regs->sr_csr.status));
> +		mutex_unlock(&psc_dma->mutex);
>  		return -ENODEV;
>  	}
>  	/* Get the data */
>  	val = in_be32(&psc_dma->psc_regs->ac97_data);
>  	if (((val >> 24) & 0x7f) != reg) {
>  		pr_err("reg echo error on ac97 read\n");
> +		mutex_unlock(&psc_dma->mutex);
>  		return -ENODEV;
>  	}
>  	val = (val >> 8) & 0xffff;
>  
> +	mutex_unlock(&psc_dma->mutex);
>  	return (unsigned short) val;
>  }
>  
> @@ -68,16 +78,21 @@ static void psc_ac97_write(struct snd_ac97 *ac97,
>  {
>  	int status;
>  
> +	mutex_lock(&psc_dma->mutex);
> +
>  	/* Wait for command status zero = ready */
>  	status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) &
>  				MPC52xx_PSC_SR_CMDSEND), 100, 0);
>  	if (status == 0) {
>  		pr_err("timeout on ac97 bus (write)\n");
> -		return;
> +		goto out;
>  	}
>  	/* Write data */
>  	out_be32(&psc_dma->psc_regs->ac97_cmd,
>  			((reg & 0x7f) << 24) | (val << 8));
> +
> + out:
> +	mutex_unlock(&psc_dma->mutex);
>  }
>  
>  static void psc_ac97_warm_reset(struct snd_ac97 *ac97)
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
Mark Brown - June 30, 2009, 8:59 a.m.
On Mon, Jun 29, 2009 at 08:26:12PM -0400, Jon Smirl wrote:

> Does your touchscreen driver use this mutex? Or was this mutex needed
> just for the AC97 driver?

It's in the register accesses so everything accessing the chip registers
will use it.
Jon Smirl - June 30, 2009, 1:42 p.m.
On Tue, Jun 30, 2009 at 2:18 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:

Wolfram, do you have any way to test the on the Phytec hardware? The
WM9712 on the baseboard supports a touchscreen, is it brought out to a
header?


> Hi Grant,
>
> On Mon, Jun 29, 2009 at 05:42:21PM -0600, Grant Likely wrote:
>> From: Grant Likely <grant.likely@secretlab.ca>
>>
>> AC97 bus register read/write hooks need to provide locking, but the
>> mpc5200-psc-ac97 driver does not.  This patch adds a mutex around
>> the register access routines.
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>> ---
>>
>>  sound/soc/fsl/mpc5200_dma.c      |    1 +
>>  sound/soc/fsl/mpc5200_dma.h      |    1 +
>>  sound/soc/fsl/mpc5200_psc_ac97.c |   17 ++++++++++++++++-
>>  3 files changed, 18 insertions(+), 1 deletions(-)
>>
>>
>> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
>> index efec33a..f0a2d40 100644
>> --- a/sound/soc/fsl/mpc5200_dma.c
>> +++ b/sound/soc/fsl/mpc5200_dma.c
>> @@ -456,6 +456,7 @@ int mpc5200_audio_dma_create(struct of_device *op)
>>               return -ENODEV;
>>
>>       spin_lock_init(&psc_dma->lock);
>> +     mutex_init(&psc_dma->mutex);
>>       psc_dma->id = be32_to_cpu(*prop);
>>       psc_dma->irq = irq;
>>       psc_dma->psc_regs = regs;
>> diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h
>> index 2000803..8d396bb 100644
>> --- a/sound/soc/fsl/mpc5200_dma.h
>> +++ b/sound/soc/fsl/mpc5200_dma.h
>> @@ -55,6 +55,7 @@ struct psc_dma {
>>       unsigned int irq;
>>       struct device *dev;
>>       spinlock_t lock;
>> +     struct mutex mutex;
>>       u32 sicr;
>>       uint sysclk;
>>       int imr;
>> diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c
>> index 794a247..7eb5499 100644
>> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
>> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
>> @@ -34,13 +34,20 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
>>       int status;
>>       unsigned int val;
>>
>> +     mutex_lock(&psc_dma->mutex);
>> +
>>       /* Wait for command send status zero = ready */
>>       status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) &
>>                               MPC52xx_PSC_SR_CMDSEND), 100, 0);
>>       if (status == 0) {
>>               pr_err("timeout on ac97 bus (rdy)\n");
>> +             mutex_unlock(&psc_dma->mutex);
>>               return -ENODEV;
>>       }
>> +
>> +     /* Force clear the data valid bit */
>> +     in_be32(&psc_dma->psc_regs->ac97_data);
>> +
>
> No mutex involved here. I think this is either a seperate patch or it needs at
> least to be mentioned in the patch description.
>
>>       /* Send the read */
>>       out_be32(&psc_dma->psc_regs->ac97_cmd, (1<<31) | ((reg & 0x7f) << 24));
>>
>> @@ -50,16 +57,19 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
>>       if (status == 0) {
>>               pr_err("timeout on ac97 read (val) %x\n",
>>                               in_be16(&psc_dma->psc_regs->sr_csr.status));
>> +             mutex_unlock(&psc_dma->mutex);
>>               return -ENODEV;
>>       }
>>       /* Get the data */
>>       val = in_be32(&psc_dma->psc_regs->ac97_data);
>>       if (((val >> 24) & 0x7f) != reg) {
>>               pr_err("reg echo error on ac97 read\n");
>> +             mutex_unlock(&psc_dma->mutex);
>>               return -ENODEV;
>>       }
>>       val = (val >> 8) & 0xffff;
>>
>> +     mutex_unlock(&psc_dma->mutex);
>>       return (unsigned short) val;
>>  }
>>
>> @@ -68,16 +78,21 @@ static void psc_ac97_write(struct snd_ac97 *ac97,
>>  {
>>       int status;
>>
>> +     mutex_lock(&psc_dma->mutex);
>> +
>>       /* Wait for command status zero = ready */
>>       status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) &
>>                               MPC52xx_PSC_SR_CMDSEND), 100, 0);
>>       if (status == 0) {
>>               pr_err("timeout on ac97 bus (write)\n");
>> -             return;
>> +             goto out;
>>       }
>>       /* Write data */
>>       out_be32(&psc_dma->psc_regs->ac97_cmd,
>>                       ((reg & 0x7f) << 24) | (val << 8));
>> +
>> + out:
>> +     mutex_unlock(&psc_dma->mutex);
>>  }
>>
>>  static void psc_ac97_warm_reset(struct snd_ac97 *ac97)
>>
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
>
> iEYEARECAAYFAkpJrkcACgkQD27XaX1/VRsY/QCgyx8IMANjokbNnrOQlXINRLeW
> lT4AnAy3ES9r3wriGkRN7ivnLA3zyRHb
> =RUPr
> -----END PGP SIGNATURE-----
>
>
Mark Brown - June 30, 2009, 1:53 p.m.
On Tue, Jun 30, 2009 at 09:42:06AM -0400, Jon Smirl wrote:

> Wolfram, do you have any way to test the on the Phytec hardware? The
> WM9712 on the baseboard supports a touchscreen, is it brought out to a
> header?

You can probably test adequately by writing a dummy AC97 driver that
sits in a thread and does reads (which should be all that's required to
test the interaction).
Grant Likely - June 30, 2009, 4:50 p.m.
On Tue, Jun 30, 2009 at 12:18 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
>> +
>> +     /* Force clear the data valid bit */
>> +     in_be32(&psc_dma->psc_regs->ac97_data);
>> +
>
> No mutex involved here. I think this is either a seperate patch or it needs at
> least to be mentioned in the patch description.

Oops, that was sloppy.  Yes, I'll put this into a separate patch.  Thanks.

g.
Grant Likely - June 30, 2009, 4:53 p.m.
On Tue, Jun 30, 2009 at 2:59 AM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Jun 29, 2009 at 08:26:12PM -0400, Jon Smirl wrote:
>
>> Does your touchscreen driver use this mutex? Or was this mutex needed
>> just for the AC97 driver?
>
> It's in the register accesses so everything accessing the chip registers
> will use it.

The mutexes are needed.  The ac97 bus doesn't have any knowledge about
what codec(s) will be wired up to it so it must protect against
multiple access.  In my case both the wm9712 audio codec driver and
the wm9712 touchscreen driver perform register accesses.

g.
Jon Smirl - June 30, 2009, 6:33 p.m.
On Tue, Jun 30, 2009 at 12:50 PM, Grant Likely<grant.likely@secretlab.ca> wrote:
> On Tue, Jun 30, 2009 at 12:18 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
>>> +
>>> +     /* Force clear the data valid bit */
>>> +     in_be32(&psc_dma->psc_regs->ac97_data);
>>> +
>>
>> No mutex involved here. I think this is either a separate patch or it needs at
>> least to be mentioned in the patch description.
>
> Oops, that was sloppy.  Yes, I'll put this into a separate patch.  Thanks.

Now that you have added the mutexes, do you ever need to force clear
the valid bit?
Maybe log an error if this happens so that we can track down why.
Grant Likely - June 30, 2009, 7:08 p.m.
On Tue, Jun 30, 2009 at 12:33 PM, Jon Smirl<jonsmirl@gmail.com> wrote:
> On Tue, Jun 30, 2009 at 12:50 PM, Grant Likely<grant.likely@secretlab.ca> wrote:
>> On Tue, Jun 30, 2009 at 12:18 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
>>>> +
>>>> +     /* Force clear the data valid bit */
>>>> +     in_be32(&psc_dma->psc_regs->ac97_data);
>>>> +
>>>
>>> No mutex involved here. I think this is either a separate patch or it needs at
>>> least to be mentioned in the patch description.
>>
>> Oops, that was sloppy.  Yes, I'll put this into a separate patch.  Thanks.
>
> Now that you have added the mutexes, do you ever need to force clear
> the valid bit?
> Maybe log an error if this happens so that we can track down why.

I know exactly why it happened.  spin_event_timeout() had a bug that
would cause it to report a false positive timeout.  The bug is fixed
and queued for merging.

However, I still think this explicit clear should be applied.  It is
just a cheap register read and it prevents the driver from getting
wedged after a timeout does event, for whatever reason.

g.
Wolfram Sang - July 1, 2009, 8:56 a.m.
Hi Jon,

> Wolfram, do you have any way to test the on the Phytec hardware? The
> WM9712 on the baseboard supports a touchscreen, is it brought out to a
> header?

Sorry, I didn't get it: Shall I test something specific?

The touchscreen connector is X19 BTW (assuming a PCM-973 baseboard).

Regards,

   Wolfram
Jon Smirl - July 1, 2009, 1:32 p.m.
On Wed, Jul 1, 2009 at 4:56 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
> Hi Jon,
>
>> Wolfram, do you have any way to test the on the Phytec hardware? The
>> WM9712 on the baseboard supports a touchscreen, is it brought out to a
>> header?
>
> Sorry, I didn't get it: Shall I test something specific?

I don't own a touchscreen.

AFAIK no one has ever plugged a touchscreen into the PCM-973 to see if
works since there hasn't been a driver previously. Just do a general
test so that you can tell customers that it works.

>
> The touchscreen connector is X19 BTW (assuming a PCM-973 baseboard).
>
> Regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
>
> iEYEARECAAYFAkpLJMEACgkQD27XaX1/VRs/cwCgn7p7ffxr8WGoW7MwALkBMKtD
> VtYAoIa5/viinyvFF5cQP3BvH9WP3a5I
> =Zrq3
> -----END PGP SIGNATURE-----
>
>
Wolfram Sang - July 1, 2009, 1:55 p.m.
> > Sorry, I didn't get it: Shall I test something specific?
> 
> I don't own a touchscreen.
> 
> AFAIK no one has ever plugged a touchscreen into the PCM-973 to see if
> works since there hasn't been a driver previously. Just do a general
> test so that you can tell customers that it works.

I am not sure, we have a suitable touchscreen. Also, I am afraid this is a bit
too much to do besides my regular day work right now. Sorry.

Regards,

   Wolfram
Grant Likely - July 1, 2009, 2:44 p.m.
On Wed, Jul 1, 2009 at 7:55 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
>
>> > Sorry, I didn't get it: Shall I test something specific?
>>
>> I don't own a touchscreen.
>>
>> AFAIK no one has ever plugged a touchscreen into the PCM-973 to see if
>> works since there hasn't been a driver previously.
>> Just do a general
>> test so that you can tell customers that it works.
>
> I am not sure, we have a suitable touchscreen. Also, I am afraid this is a bit
> too much to do besides my regular day work right now. Sorry.

I've now tested this on a client's board which uses the pcm030 with a
custom base board (based on the development board) which uses the same
codec.  It works there.  I don't have a discrete touchscreen to wire
up to the PCM-973 though.

g.
Eric Millbrandt - July 2, 2009, 1:51 p.m.
-----Original Message-----
From: linuxppc-dev-bounces+emillbrandt=dekaresearch.com@lists.ozlabs.org
[mailto:linuxppc-dev-bounces+emillbrandt=dekaresearch.com@lists.ozlabs.o
rg] On Behalf Of Grant Likely
Sent: Wednesday, July 01, 2009 10:45
To: Wolfram Sang
Cc: linuxppc-dev@ozlabs.org; alsa-devel@alsa-project.org;
broonie@sirena.org.uk
Subject: Re: [PATCH] alsa/soc: add locking to mpc5200-psc-ac97 driver

On Wed, Jul 1, 2009 at 7:55 AM, Wolfram Sang<w.sang@pengutronix.de>
wrote:
>
>> > Sorry, I didn't get it: Shall I test something specific?
>>
>> I don't own a touchscreen.
>>
>> AFAIK no one has ever plugged a touchscreen into the PCM-973 to see
if
>> works since there hasn't been a driver previously.
>> Just do a general
>> test so that you can tell customers that it works.
>
> I am not sure, we have a suitable touchscreen. Also, I am afraid this
is a bit
> too much to do besides my regular day work right now. Sorry.

I've now tested this on a client's board which uses the pcm030 with a
custom base board (based on the development board) which uses the same
codec.  It works there.  I don't have a discrete touchscreen to wire
up to the PCM-973 though.

g.

Patch

diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
index efec33a..f0a2d40 100644
--- a/sound/soc/fsl/mpc5200_dma.c
+++ b/sound/soc/fsl/mpc5200_dma.c
@@ -456,6 +456,7 @@  int mpc5200_audio_dma_create(struct of_device *op)
 		return -ENODEV;
 
 	spin_lock_init(&psc_dma->lock);
+	mutex_init(&psc_dma->mutex);
 	psc_dma->id = be32_to_cpu(*prop);
 	psc_dma->irq = irq;
 	psc_dma->psc_regs = regs;
diff --git a/sound/soc/fsl/mpc5200_dma.h b/sound/soc/fsl/mpc5200_dma.h
index 2000803..8d396bb 100644
--- a/sound/soc/fsl/mpc5200_dma.h
+++ b/sound/soc/fsl/mpc5200_dma.h
@@ -55,6 +55,7 @@  struct psc_dma {
 	unsigned int irq;
 	struct device *dev;
 	spinlock_t lock;
+	struct mutex mutex;
 	u32 sicr;
 	uint sysclk;
 	int imr;
diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c
index 794a247..7eb5499 100644
--- a/sound/soc/fsl/mpc5200_psc_ac97.c
+++ b/sound/soc/fsl/mpc5200_psc_ac97.c
@@ -34,13 +34,20 @@  static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
 	int status;
 	unsigned int val;
 
+	mutex_lock(&psc_dma->mutex);
+
 	/* Wait for command send status zero = ready */
 	status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) &
 				MPC52xx_PSC_SR_CMDSEND), 100, 0);
 	if (status == 0) {
 		pr_err("timeout on ac97 bus (rdy)\n");
+		mutex_unlock(&psc_dma->mutex);
 		return -ENODEV;
 	}
+
+	/* Force clear the data valid bit */
+	in_be32(&psc_dma->psc_regs->ac97_data);
+
 	/* Send the read */
 	out_be32(&psc_dma->psc_regs->ac97_cmd, (1<<31) | ((reg & 0x7f) << 24));
 
@@ -50,16 +57,19 @@  static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
 	if (status == 0) {
 		pr_err("timeout on ac97 read (val) %x\n",
 				in_be16(&psc_dma->psc_regs->sr_csr.status));
+		mutex_unlock(&psc_dma->mutex);
 		return -ENODEV;
 	}
 	/* Get the data */
 	val = in_be32(&psc_dma->psc_regs->ac97_data);
 	if (((val >> 24) & 0x7f) != reg) {
 		pr_err("reg echo error on ac97 read\n");
+		mutex_unlock(&psc_dma->mutex);
 		return -ENODEV;
 	}
 	val = (val >> 8) & 0xffff;
 
+	mutex_unlock(&psc_dma->mutex);
 	return (unsigned short) val;
 }
 
@@ -68,16 +78,21 @@  static void psc_ac97_write(struct snd_ac97 *ac97,
 {
 	int status;
 
+	mutex_lock(&psc_dma->mutex);
+
 	/* Wait for command status zero = ready */
 	status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) &
 				MPC52xx_PSC_SR_CMDSEND), 100, 0);
 	if (status == 0) {
 		pr_err("timeout on ac97 bus (write)\n");
-		return;
+		goto out;
 	}
 	/* Write data */
 	out_be32(&psc_dma->psc_regs->ac97_cmd,
 			((reg & 0x7f) << 24) | (val << 8));
+
+ out:
+	mutex_unlock(&psc_dma->mutex);
 }
 
 static void psc_ac97_warm_reset(struct snd_ac97 *ac97)