* [PATCH V2] ASoC: tlv320aic3x: Fix codec pll configure bug
@ 2012-06-26 6:03 Hebbar, Gururaja
2012-06-26 8:54 ` Mark Brown
2012-06-26 9:01 ` Prchal Jiří
0 siblings, 2 replies; 5+ messages in thread
From: Hebbar, Gururaja @ 2012-06-26 6:03 UTC (permalink / raw)
To: linux-arm-kernel
In sound/soc/codecs/tlv320aic3x.c
data = snd_soc_read(codec, AIC3X_PLL_PROGA_REG);
snd_soc_write(codec, AIC3X_PLL_PROGA_REG,
data | (pll_p << PLLP_SHIFT));
In the above code, pll-p value is OR'ed with previous value without
clearing it. Bug is not seen if pll-p value doesn't change across
Sampling frequency.
However on some platforms (like AM335x EVM-SK), pll-p may have different
values across different sampling frequencies. In such case, above code
configures the pll with a wrong value.
Because of this bug, when a audio stream is played with pll value
different from previous stream, audio is heard as differently.
Signed-off-by: Hebbar, Gururaja <gururaja.hebbar@ti.com>
---
chnages in V2:
modify subject to indicate ASOC and codec as tlv320aic3x
sound/soc/codecs/tlv320aic3x.c | 2 +-
sound/soc/codecs/tlv320aic3x.h | 1 +
2 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index d0dbac1..8c1977b 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -963,7 +963,7 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
}
found:
- data = snd_soc_read(codec, AIC3X_PLL_PROGA_REG);
+ data = snd_soc_read(codec, AIC3X_PLL_PROGA_REG) & ~PLLP_MASK;
snd_soc_write(codec, AIC3X_PLL_PROGA_REG,
data | (pll_p << PLLP_SHIFT));
snd_soc_write(codec, AIC3X_OVRF_STATUS_AND_PLLR_REG,
diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
index 06a1978..16d9999 100644
--- a/sound/soc/codecs/tlv320aic3x.h
+++ b/sound/soc/codecs/tlv320aic3x.h
@@ -166,6 +166,7 @@
/* PLL registers bitfields */
#define PLLP_SHIFT 0
+#define PLLP_MASK 7
#define PLLQ_SHIFT 3
#define PLLR_SHIFT 0
#define PLLJ_SHIFT 2
--
1.7.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH V2] ASoC: tlv320aic3x: Fix codec pll configure bug
2012-06-26 6:03 [PATCH V2] ASoC: tlv320aic3x: Fix codec pll configure bug Hebbar, Gururaja
@ 2012-06-26 8:54 ` Mark Brown
2012-06-26 14:07 ` Hebbar, Gururaja
2012-06-26 9:01 ` Prchal Jiří
1 sibling, 1 reply; 5+ messages in thread
From: Mark Brown @ 2012-06-26 8:54 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 26, 2012 at 11:33:43AM +0530, Hebbar, Gururaja wrote:
> In sound/soc/codecs/tlv320aic3x.c
>
> data = snd_soc_read(codec, AIC3X_PLL_PROGA_REG);
> snd_soc_write(codec, AIC3X_PLL_PROGA_REG,
> data | (pll_p << PLLP_SHIFT));
>
> In the above code, pll-p value is OR'ed with previous value without
> clearing it. Bug is not seen if pll-p value doesn't change across
> Sampling frequency.
A better fix is to change the code to use snd_soc_update_bits() rather
than open code it. This is more idiomatic and will suppress writes if
they don't change anything.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120626/ebaf029c/attachment.sig>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH V2] ASoC: tlv320aic3x: Fix codec pll configure bug
2012-06-26 8:54 ` Mark Brown
@ 2012-06-26 14:07 ` Hebbar, Gururaja
0 siblings, 0 replies; 5+ messages in thread
From: Hebbar, Gururaja @ 2012-06-26 14:07 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 26, 2012 at 14:24:03, Mark Brown wrote:
> On Tue, Jun 26, 2012 at 11:33:43AM +0530, Hebbar, Gururaja wrote:
> > In sound/soc/codecs/tlv320aic3x.c
> >
> > data = snd_soc_read(codec, AIC3X_PLL_PROGA_REG);
> > snd_soc_write(codec, AIC3X_PLL_PROGA_REG,
> > data | (pll_p << PLLP_SHIFT));
> >
> > In the above code, pll-p value is OR'ed with previous value without
> > clearing it. Bug is not seen if pll-p value doesn't change across
> > Sampling frequency.
>
> A better fix is to change the code to use snd_soc_update_bits() rather
> than open code it. This is more idiomatic and will suppress writes if
> they don't change anything.
>
Sure will resend the patch. Thanks for the review
Regards,
Gururaja
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH V2] ASoC: tlv320aic3x: Fix codec pll configure bug
2012-06-26 6:03 [PATCH V2] ASoC: tlv320aic3x: Fix codec pll configure bug Hebbar, Gururaja
2012-06-26 8:54 ` Mark Brown
@ 2012-06-26 9:01 ` Prchal Jiří
2012-06-26 14:08 ` Hebbar, Gururaja
1 sibling, 1 reply; 5+ messages in thread
From: Prchal Jiří @ 2012-06-26 9:01 UTC (permalink / raw)
To: linux-arm-kernel
Hi Gururaja,
shouldn't be better to use:
snd_soc_update_bits(codec, AIC3X_PLL_PROGA_REG, PLLP_MASK, pll_p);
instead of "read mask write" ?
Even at this place you don't need to keep rest of register (PLL_Q and PLL_enable), so yust writing is OK.
snd_soc_write(codec, AIC3X_PLL_PROGA_REG, pll_p << PLLP_SHIFT);
Dne 26.6.2012 08:03, Hebbar, Gururaja napsal(a):
> In sound/soc/codecs/tlv320aic3x.c
>
> data = snd_soc_read(codec, AIC3X_PLL_PROGA_REG);
> snd_soc_write(codec, AIC3X_PLL_PROGA_REG,
> data | (pll_p<< PLLP_SHIFT));
>
> In the above code, pll-p value is OR'ed with previous value without
> clearing it. Bug is not seen if pll-p value doesn't change across
> Sampling frequency.
>
> However on some platforms (like AM335x EVM-SK), pll-p may have different
> values across different sampling frequencies. In such case, above code
> configures the pll with a wrong value.
> Because of this bug, when a audio stream is played with pll value
> different from previous stream, audio is heard as differently.
>
> Signed-off-by: Hebbar, Gururaja<gururaja.hebbar@ti.com>
> ---
> chnages in V2:
> modify subject to indicate ASOC and codec as tlv320aic3x
>
> sound/soc/codecs/tlv320aic3x.c | 2 +-
> sound/soc/codecs/tlv320aic3x.h | 1 +
> 2 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
> index d0dbac1..8c1977b 100644
> --- a/sound/soc/codecs/tlv320aic3x.c
> +++ b/sound/soc/codecs/tlv320aic3x.c
> @@ -963,7 +963,7 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
> }
>
> found:
> - data = snd_soc_read(codec, AIC3X_PLL_PROGA_REG);
> + data = snd_soc_read(codec, AIC3X_PLL_PROGA_REG)& ~PLLP_MASK;
> snd_soc_write(codec, AIC3X_PLL_PROGA_REG,
> data | (pll_p<< PLLP_SHIFT));
> snd_soc_write(codec, AIC3X_OVRF_STATUS_AND_PLLR_REG,
> diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
> index 06a1978..16d9999 100644
> --- a/sound/soc/codecs/tlv320aic3x.h
> +++ b/sound/soc/codecs/tlv320aic3x.h
> @@ -166,6 +166,7 @@
>
> /* PLL registers bitfields */
> #define PLLP_SHIFT 0
> +#define PLLP_MASK 7
> #define PLLQ_SHIFT 3
> #define PLLR_SHIFT 0
> #define PLLJ_SHIFT 2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH V2] ASoC: tlv320aic3x: Fix codec pll configure bug
2012-06-26 9:01 ` Prchal Jiří
@ 2012-06-26 14:08 ` Hebbar, Gururaja
0 siblings, 0 replies; 5+ messages in thread
From: Hebbar, Gururaja @ 2012-06-26 14:08 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 26, 2012 at 14:31:42, Prchal Ji?? wrote:
> Hi Gururaja,
> shouldn't be better to use:
>
> snd_soc_update_bits(codec, AIC3X_PLL_PROGA_REG, PLLP_MASK, pll_p);
>
> instead of "read mask write" ?
Sure will resend the patch. Thanks for the review
> Even at this place you don't need to keep rest of register (PLL_Q and PLL_enable), so yust writing is OK.
>
> snd_soc_write(codec, AIC3X_PLL_PROGA_REG, pll_p << PLLP_SHIFT);
>
> Dne 26.6.2012 08:03, Hebbar, Gururaja napsal(a):
> > In sound/soc/codecs/tlv320aic3x.c
> >
> > data = snd_soc_read(codec, AIC3X_PLL_PROGA_REG);
> > snd_soc_write(codec, AIC3X_PLL_PROGA_REG,
> > data | (pll_p<< PLLP_SHIFT));
> >
> > In the above code, pll-p value is OR'ed with previous value without
> > clearing it. Bug is not seen if pll-p value doesn't change across
> > Sampling frequency.
> >
> > However on some platforms (like AM335x EVM-SK), pll-p may have different
> > values across different sampling frequencies. In such case, above code
> > configures the pll with a wrong value.
> > Because of this bug, when a audio stream is played with pll value
> > different from previous stream, audio is heard as differently.
> >
> > Signed-off-by: Hebbar, Gururaja<gururaja.hebbar@ti.com>
> > ---
> > chnages in V2:
> > modify subject to indicate ASOC and codec as tlv320aic3x
> >
> > sound/soc/codecs/tlv320aic3x.c | 2 +-
> > sound/soc/codecs/tlv320aic3x.h | 1 +
> > 2 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
> > index d0dbac1..8c1977b 100644
> > --- a/sound/soc/codecs/tlv320aic3x.c
> > +++ b/sound/soc/codecs/tlv320aic3x.c
> > @@ -963,7 +963,7 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
> > }
> >
> > found:
> > - data = snd_soc_read(codec, AIC3X_PLL_PROGA_REG);
> > + data = snd_soc_read(codec, AIC3X_PLL_PROGA_REG)& ~PLLP_MASK;
> > snd_soc_write(codec, AIC3X_PLL_PROGA_REG,
> > data | (pll_p<< PLLP_SHIFT));
> > snd_soc_write(codec, AIC3X_OVRF_STATUS_AND_PLLR_REG,
> > diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
> > index 06a1978..16d9999 100644
> > --- a/sound/soc/codecs/tlv320aic3x.h
> > +++ b/sound/soc/codecs/tlv320aic3x.h
> > @@ -166,6 +166,7 @@
> >
> > /* PLL registers bitfields */
> > #define PLLP_SHIFT 0
> > +#define PLLP_MASK 7
> > #define PLLQ_SHIFT 3
> > #define PLLR_SHIFT 0
> > #define PLLJ_SHIFT 2
>
>
Regards,
Gururaja
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-06-26 14:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-26 6:03 [PATCH V2] ASoC: tlv320aic3x: Fix codec pll configure bug Hebbar, Gururaja
2012-06-26 8:54 ` Mark Brown
2012-06-26 14:07 ` Hebbar, Gururaja
2012-06-26 9:01 ` Prchal Jiří
2012-06-26 14:08 ` Hebbar, Gururaja
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).