[PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared
@ 2009-07-02 17:57 ` Grant Likely
  0 siblings, 0 replies; 13+ messages in thread
From: Grant Likely @ 2009-07-02 17:57 UTC (permalink / raw)
  To: linuxppc-dev, alsa-devel, broonie; +Cc: w.sang

From: Grant Likely <grant.likely@secretlab.ca>

When doing register reads, it is possible for there to be a stale
data ready bit set which will cause subsequent reads to return
prematurely with incorrect data.  This patch fixes the issues by
ensuring stale data is cleared before starting another transaction.

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

 sound/soc/fsl/mpc5200_psc_ac97.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)


diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c
index 794a247..9b8503f 100644
--- a/sound/soc/fsl/mpc5200_psc_ac97.c
+++ b/sound/soc/fsl/mpc5200_psc_ac97.c
@@ -41,6 +41,10 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
 		pr_err("timeout on ac97 bus (rdy)\n");
 		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));

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared
@ 2009-07-02 17:57 ` Grant Likely
  0 siblings, 0 replies; 13+ messages in thread
From: Grant Likely @ 2009-07-02 17:57 UTC (permalink / raw)
  To: linuxppc-dev, alsa-devel, broonie

From: Grant Likely <grant.likely@secretlab.ca>

When doing register reads, it is possible for there to be a stale
data ready bit set which will cause subsequent reads to return
prematurely with incorrect data.  This patch fixes the issues by
ensuring stale data is cleared before starting another transaction.

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

 sound/soc/fsl/mpc5200_psc_ac97.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)


diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c
index 794a247..9b8503f 100644
--- a/sound/soc/fsl/mpc5200_psc_ac97.c
+++ b/sound/soc/fsl/mpc5200_psc_ac97.c
@@ -41,6 +41,10 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
 		pr_err("timeout on ac97 bus (rdy)\n");
 		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));
 

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] ASoC: add locking to mpc5200-psc-ac97 driver
  2009-07-02 17:57 ` Grant Likely
@ 2009-07-02 17:57   ` Grant Likely
  -1 siblings, 0 replies; 13+ messages in thread
From: Grant Likely @ 2009-07-02 17:57 UTC (permalink / raw)
  To: linuxppc-dev, alsa-devel, broonie; +Cc: w.sang

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 |   13 ++++++++++++-
 3 files changed, 14 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 9b8503f..7eb5499 100644
--- a/sound/soc/fsl/mpc5200_psc_ac97.c
+++ b/sound/soc/fsl/mpc5200_psc_ac97.c
@@ -34,11 +34,14 @@ 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;
 	}
 
@@ -54,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;
 }
 
@@ -72,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)

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] ASoC: add locking to mpc5200-psc-ac97 driver
@ 2009-07-02 17:57   ` Grant Likely
  0 siblings, 0 replies; 13+ messages in thread
From: Grant Likely @ 2009-07-02 17:57 UTC (permalink / raw)
  To: linuxppc-dev, alsa-devel, broonie

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 |   13 ++++++++++++-
 3 files changed, 14 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 9b8503f..7eb5499 100644
--- a/sound/soc/fsl/mpc5200_psc_ac97.c
+++ b/sound/soc/fsl/mpc5200_psc_ac97.c
@@ -34,11 +34,14 @@ 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;
 	}
 
@@ -54,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;
 }
 
@@ -72,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)

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: add locking to mpc5200-psc-ac97 driver
  2009-07-02 17:57   ` Grant Likely
@ 2009-07-02 19:06     ` Jon Smirl
  -1 siblings, 0 replies; 13+ messages in thread
From: Jon Smirl @ 2009-07-02 19:06 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, alsa-devel, broonie, w.sang

On Thu, Jul 2, 2009 at 1:57 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.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Acked-by: Jon Smirl <jonsmirl@gmail.com>

> ---
>
>  sound/soc/fsl/mpc5200_dma.c      |    1 +
>  sound/soc/fsl/mpc5200_dma.h      |    1 +
>  sound/soc/fsl/mpc5200_psc_ac97.c |   13 ++++++++++++-
>  3 files changed, 14 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 9b8503f..7eb5499 100644
> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
> @@ -34,11 +34,14 @@ 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;
>        }
>
> @@ -54,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;
>  }
>
> @@ -72,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)
>
>



-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: add locking to mpc5200-psc-ac97 driver
@ 2009-07-02 19:06     ` Jon Smirl
  0 siblings, 0 replies; 13+ messages in thread
From: Jon Smirl @ 2009-07-02 19:06 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, alsa-devel, broonie

On Thu, Jul 2, 2009 at 1:57 PM, Grant Likely<grant.likely@secretlab.ca> wro=
te:
> 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. =A0This patch adds a mutex around
> the register access routines.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Acked-by: Jon Smirl <jonsmirl@gmail.com>

> ---
>
> =A0sound/soc/fsl/mpc5200_dma.c =A0 =A0 =A0| =A0 =A01 +
> =A0sound/soc/fsl/mpc5200_dma.h =A0 =A0 =A0| =A0 =A01 +
> =A0sound/soc/fsl/mpc5200_psc_ac97.c | =A0 13 ++++++++++++-
> =A03 files changed, 14 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)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
>
> =A0 =A0 =A0 =A0spin_lock_init(&psc_dma->lock);
> + =A0 =A0 =A0 mutex_init(&psc_dma->mutex);
> =A0 =A0 =A0 =A0psc_dma->id =3D be32_to_cpu(*prop);
> =A0 =A0 =A0 =A0psc_dma->irq =3D irq;
> =A0 =A0 =A0 =A0psc_dma->psc_regs =3D 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 {
> =A0 =A0 =A0 =A0unsigned int irq;
> =A0 =A0 =A0 =A0struct device *dev;
> =A0 =A0 =A0 =A0spinlock_t lock;
> + =A0 =A0 =A0 struct mutex mutex;
> =A0 =A0 =A0 =A0u32 sicr;
> =A0 =A0 =A0 =A0uint sysclk;
> =A0 =A0 =A0 =A0int imr;
> diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc=
_ac97.c
> index 9b8503f..7eb5499 100644
> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
> @@ -34,11 +34,14 @@ static unsigned short psc_ac97_read(struct snd_ac97 *=
ac97, unsigned short reg)
> =A0 =A0 =A0 =A0int status;
> =A0 =A0 =A0 =A0unsigned int val;
>
> + =A0 =A0 =A0 mutex_lock(&psc_dma->mutex);
> +
> =A0 =A0 =A0 =A0/* Wait for command send status zero =3D ready */
> =A0 =A0 =A0 =A0status =3D spin_event_timeout(!(in_be16(&psc_dma->psc_regs=
->sr_csr.status) &
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0MPC52xx_PS=
C_SR_CMDSEND), 100, 0);
> =A0 =A0 =A0 =A0if (status =3D=3D 0) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("timeout on ac97 bus (rdy)\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mutex_unlock(&psc_dma->mutex);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
> =A0 =A0 =A0 =A0}
>
> @@ -54,16 +57,19 @@ static unsigned short psc_ac97_read(struct snd_ac97 *=
ac97, unsigned short reg)
> =A0 =A0 =A0 =A0if (status =3D=3D 0) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("timeout on ac97 read (val) %x\n",
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0in_be16(&p=
sc_dma->psc_regs->sr_csr.status));
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mutex_unlock(&psc_dma->mutex);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0/* Get the data */
> =A0 =A0 =A0 =A0val =3D in_be32(&psc_dma->psc_regs->ac97_data);
> =A0 =A0 =A0 =A0if (((val >> 24) & 0x7f) !=3D reg) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("reg echo error on ac97 read\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mutex_unlock(&psc_dma->mutex);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0val =3D (val >> 8) & 0xffff;
>
> + =A0 =A0 =A0 mutex_unlock(&psc_dma->mutex);
> =A0 =A0 =A0 =A0return (unsigned short) val;
> =A0}
>
> @@ -72,16 +78,21 @@ static void psc_ac97_write(struct snd_ac97 *ac97,
> =A0{
> =A0 =A0 =A0 =A0int status;
>
> + =A0 =A0 =A0 mutex_lock(&psc_dma->mutex);
> +
> =A0 =A0 =A0 =A0/* Wait for command status zero =3D ready */
> =A0 =A0 =A0 =A0status =3D spin_event_timeout(!(in_be16(&psc_dma->psc_regs=
->sr_csr.status) &
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0MPC52xx_PS=
C_SR_CMDSEND), 100, 0);
> =A0 =A0 =A0 =A0if (status =3D=3D 0) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("timeout on ac97 bus (write)\n");
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0/* Write data */
> =A0 =A0 =A0 =A0out_be32(&psc_dma->psc_regs->ac97_cmd,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0((reg & 0x7f) << 24) | (va=
l << 8));
> +
> + out:
> + =A0 =A0 =A0 mutex_unlock(&psc_dma->mutex);
> =A0}
>
> =A0static void psc_ac97_warm_reset(struct snd_ac97 *ac97)
>
>



--=20
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: add locking to mpc5200-psc-ac97 driver
  2009-07-02 17:57   ` Grant Likely
  (?)
  (?)
@ 2009-07-02 19:08   ` michael
  2009-07-03  7:12       ` Grant Likely
  -1 siblings, 1 reply; 13+ messages in thread
From: michael @ 2009-07-02 19:08 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, alsa-devel, broonie

Hi,

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 |   13 ++++++++++++-
>  3 files changed, 14 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 9b8503f..7eb5499 100644
> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
> @@ -34,11 +34,14 @@ 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;
>   
maybe define an err variable and and a goto out.

Michael

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: add locking to mpc5200-psc-ac97 driver
  2009-07-02 19:08   ` michael
@ 2009-07-03  7:12       ` Grant Likely
  0 siblings, 0 replies; 13+ messages in thread
From: Grant Likely @ 2009-07-03  7:12 UTC (permalink / raw)
  To: michael; +Cc: linuxppc-dev, alsa-devel, broonie

On Thu, Jul 2, 2009 at 1:08 PM, michael<michael@evidence.eu.com> wrote:
> Hi,
>
> 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 |   13 ++++++++++++-
>>  3 files changed, 14 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 9b8503f..7eb5499 100644
>> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
>> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
>> @@ -34,11 +34,14 @@ 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;
>>
>
> maybe define an err variable and and a goto out.

My first iteration did that, but after looking at it I decided I like
this better and it adds (slightly) fewer lines of code.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: add locking to mpc5200-psc-ac97 driver
@ 2009-07-03  7:12       ` Grant Likely
  0 siblings, 0 replies; 13+ messages in thread
From: Grant Likely @ 2009-07-03  7:12 UTC (permalink / raw)
  To: michael; +Cc: linuxppc-dev, alsa-devel, broonie

On Thu, Jul 2, 2009 at 1:08 PM, michael<michael@evidence.eu.com> wrote:
> Hi,
>
> 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. =A0This patch adds a mutex around
>> the register access routines.
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>> ---
>>
>> =A0sound/soc/fsl/mpc5200_dma.c =A0 =A0 =A0| =A0 =A01 +
>> =A0sound/soc/fsl/mpc5200_dma.h =A0 =A0 =A0| =A0 =A01 +
>> =A0sound/soc/fsl/mpc5200_psc_ac97.c | =A0 13 ++++++++++++-
>> =A03 files changed, 14 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)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
>> =A0 =A0 =A0 =A0 =A0spin_lock_init(&psc_dma->lock);
>> + =A0 =A0 =A0 mutex_init(&psc_dma->mutex);
>> =A0 =A0 =A0 =A0psc_dma->id =3D be32_to_cpu(*prop);
>> =A0 =A0 =A0 =A0psc_dma->irq =3D irq;
>> =A0 =A0 =A0 =A0psc_dma->psc_regs =3D 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 {
>> =A0 =A0 =A0 =A0unsigned int irq;
>> =A0 =A0 =A0 =A0struct device *dev;
>> =A0 =A0 =A0 =A0spinlock_t lock;
>> + =A0 =A0 =A0 struct mutex mutex;
>> =A0 =A0 =A0 =A0u32 sicr;
>> =A0 =A0 =A0 =A0uint sysclk;
>> =A0 =A0 =A0 =A0int imr;
>> diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c
>> b/sound/soc/fsl/mpc5200_psc_ac97.c
>> index 9b8503f..7eb5499 100644
>> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
>> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
>> @@ -34,11 +34,14 @@ static unsigned short psc_ac97_read(struct snd_ac97
>> *ac97, unsigned short reg)
>> =A0 =A0 =A0 =A0int status;
>> =A0 =A0 =A0 =A0unsigned int val;
>> =A0+ =A0 =A0 =A0 mutex_lock(&psc_dma->mutex);
>> +
>> =A0 =A0 =A0 =A0/* Wait for command send status zero =3D ready */
>> =A0 =A0 =A0 =A0status =3D
>> spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) &
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0MPC52xx_P=
SC_SR_CMDSEND), 100, 0);
>> =A0 =A0 =A0 =A0if (status =3D=3D 0) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("timeout on ac97 bus (rdy)\n");
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mutex_unlock(&psc_dma->mutex);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
>>
>
> maybe define an err variable and and a goto out.

My first iteration did that, but after looking at it I decided I like
this better and it adds (slightly) fewer lines of code.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared
  2009-07-02 17:57 ` Grant Likely
@ 2009-07-02 19:06   ` Jon Smirl
  -1 siblings, 0 replies; 13+ messages in thread
From: Jon Smirl @ 2009-07-02 19:06 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, alsa-devel, broonie, w.sang

On Thu, Jul 2, 2009 at 1:57 PM, Grant Likely<grant.likely@secretlab.ca> wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> When doing register reads, it is possible for there to be a stale
> data ready bit set which will cause subsequent reads to return
> prematurely with incorrect data.  This patch fixes the issues by
> ensuring stale data is cleared before starting another transaction.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Acked-by: Jon Smirl <jonsmirl@gmail.com>

> ---
>
>  sound/soc/fsl/mpc5200_psc_ac97.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
>
> diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c
> index 794a247..9b8503f 100644
> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
> @@ -41,6 +41,10 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
>                pr_err("timeout on ac97 bus (rdy)\n");
>                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));
>
>
>



-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared
@ 2009-07-02 19:06   ` Jon Smirl
  0 siblings, 0 replies; 13+ messages in thread
From: Jon Smirl @ 2009-07-02 19:06 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, alsa-devel, broonie

On Thu, Jul 2, 2009 at 1:57 PM, Grant Likely<grant.likely@secretlab.ca> wro=
te:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> When doing register reads, it is possible for there to be a stale
> data ready bit set which will cause subsequent reads to return
> prematurely with incorrect data. =A0This patch fixes the issues by
> ensuring stale data is cleared before starting another transaction.
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Acked-by: Jon Smirl <jonsmirl@gmail.com>

> ---
>
> =A0sound/soc/fsl/mpc5200_psc_ac97.c | =A0 =A04 ++++
> =A01 files changed, 4 insertions(+), 0 deletions(-)
>
>
> diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc=
_ac97.c
> index 794a247..9b8503f 100644
> --- a/sound/soc/fsl/mpc5200_psc_ac97.c
> +++ b/sound/soc/fsl/mpc5200_psc_ac97.c
> @@ -41,6 +41,10 @@ static unsigned short psc_ac97_read(struct snd_ac97 *a=
c97, unsigned short reg)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pr_err("timeout on ac97 bus (rdy)\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENODEV;
> =A0 =A0 =A0 =A0}
> +
> + =A0 =A0 =A0 /* Force clear the data valid bit */
> + =A0 =A0 =A0 in_be32(&psc_dma->psc_regs->ac97_data);
> +
> =A0 =A0 =A0 =A0/* Send the read */
> =A0 =A0 =A0 =A0out_be32(&psc_dma->psc_regs->ac97_cmd, (1<<31) | ((reg & 0=
x7f) << 24));
>
>
>



--=20
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared
  2009-07-02 17:57 ` Grant Likely
@ 2009-07-03  9:59   ` Mark Brown
  -1 siblings, 0 replies; 13+ messages in thread
From: Mark Brown @ 2009-07-03  9:59 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, alsa-devel, w.sang

On Thu, Jul 02, 2009 at 11:57:19AM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> 
> When doing register reads, it is possible for there to be a stale
> data ready bit set which will cause subsequent reads to return
> prematurely with incorrect data.  This patch fixes the issues by
> ensuring stale data is cleared before starting another transaction.

Applied both, thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared
@ 2009-07-03  9:59   ` Mark Brown
  0 siblings, 0 replies; 13+ messages in thread
From: Mark Brown @ 2009-07-03  9:59 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, alsa-devel

On Thu, Jul 02, 2009 at 11:57:19AM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> 
> When doing register reads, it is possible for there to be a stale
> data ready bit set which will cause subsequent reads to return
> prematurely with incorrect data.  This patch fixes the issues by
> ensuring stale data is cleared before starting another transaction.

Applied both, thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2009-07-03 10:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-07-02 17:57 [PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared Grant Likely
2009-07-02 17:57 ` Grant Likely
2009-07-02 17:57 ` [PATCH 2/2] ASoC: add locking to mpc5200-psc-ac97 driver Grant Likely
2009-07-02 17:57   ` Grant Likely
2009-07-02 19:06   ` Jon Smirl
2009-07-02 19:06     ` Jon Smirl
2009-07-02 19:08   ` michael
2009-07-03  7:12     ` Grant Likely
2009-07-03  7:12       ` Grant Likely
2009-07-02 19:06 ` [PATCH 1/2] ASoC: Fix mpc5200-psc-ac97 to ensure the data ready bit is cleared Jon Smirl
2009-07-02 19:06   ` Jon Smirl
2009-07-03  9:59 ` Mark Brown
2009-07-03  9:59   ` Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.