[alsa-devel] [PATCH] ASoC: pxa-ssp: Don't use SSCR0_SerClkDiv and SSCR0_SCR
Those macros are just screwed as soon as CONFIG_PXA25x is enabled.
This patch - changes ssp_set_scr to take an ssp_dev pointer instead of ssp_device - adds a corresponding ssp_get_scr function.
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com
---
To me, using ssp_dev seems to be cleaner, as all the places where ssp_set_scr is called, we already have an ssp_dev *ssp = priv->dev.ssp set up, which allows us to call ssp_set_scr(ssp, ...) instead of ssp_set_scr(&priv->dev, ...). Same for ssp_get_scr. --- sound/soc/pxa/pxa-ssp.c | 36 ++++++++++++++++++++++++++++-------- 1 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 308a657..d979c5c 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -280,12 +280,33 @@ static int pxa_ssp_resume(struct snd_soc_dai *cpu_dai) * ssp_set_clkdiv - set SSP clock divider * @div: serial clock rate divider */ -static void ssp_set_scr(struct ssp_dev *dev, u32 div) +static void ssp_set_scr(struct ssp_device *ssp, u32 div) { - struct ssp_device *ssp = dev->ssp; - u32 sscr0 = ssp_read_reg(dev->ssp, SSCR0) & ~SSCR0_SCR; + u32 sscr0 = ssp_read_reg(ssp, SSCR0); + + if (cpu_is_pxa25x() && ssp->type == PXA25x_SSP) { + sscr0 &= ~0x0000ff00; + sscr0 |= ((div - 2)/2) << 8; /* 2..512 */ + } else { + sscr0 &= ~0x000fff00; + sscr0 |= (div - 1) << 8; /* 1..4096 */ + } + ssp_write_reg(ssp, SSCR0, sscr0); +} + +/** + * ssp_get_clkdiv - get SSP clock divider + */ +static u32 ssp_get_scr(struct ssp_device *ssp) +{ + u32 sscr0 = ssp_read_reg(ssp, SSCR0); + u32 div;
- ssp_write_reg(ssp, SSCR0, (sscr0 | SSCR0_SerClkDiv(div))); + if (cpu_is_pxa25x() && ssp->type == PXA25x_SSP) + div = ((sscr0 >> 8) & 0xff) * 2 + 2; + else + div = ((sscr0 >> 8) & 0xfff) + 1; + return div; }
/* @@ -326,7 +347,7 @@ static int pxa_ssp_set_dai_sysclk(struct snd_soc_dai *cpu_dai, break; case PXA_SSP_CLK_AUDIO: priv->sysclk = 0; - ssp_set_scr(&priv->dev, 1); + ssp_set_scr(ssp, 1); sscr0 |= SSCR0_ACS; break; default: @@ -387,7 +408,7 @@ static int pxa_ssp_set_dai_clkdiv(struct snd_soc_dai *cpu_dai, ssp_write_reg(ssp, SSACD, val); break; case PXA_SSP_DIV_SCR: - ssp_set_scr(&priv->dev, div); + ssp_set_scr(ssp, div); break; default: return -ENODEV; @@ -674,8 +695,7 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream, case SND_SOC_DAIFMT_I2S: sspsp = ssp_read_reg(ssp, SSPSP);
- if (((sscr0 & SSCR0_SCR) == SSCR0_SerClkDiv(4)) && - (width == 16)) { + if ((ssp_get_scr(ssp) == 4) && (width == 16)) { /* This is a special case where the bitclk is 64fs * and we're not dealing with 2*32 bits of audio * samples.
On Fri, Apr 17, 2009 at 11:39:38AM +0200, Philipp Zabel wrote:
To me, using ssp_dev seems to be cleaner, as all the places where ssp_set_scr is called, we already have an ssp_dev *ssp = priv->dev.ssp set up, which allows us to call ssp_set_scr(ssp, ...) instead of ssp_set_scr(&priv->dev, ...). Same for ssp_get_scr.
Yeah, the combination of ssp_dev and ssp_device is icky in general and largely historical as a result of a partially done transition of the driver to ssp_device.
I'll apply the patch - thanks!
On Fri, Apr 17, 2009 at 6:22 PM, Mark Brown broonie@sirena.org.uk wrote:
On Fri, Apr 17, 2009 at 11:39:38AM +0200, Philipp Zabel wrote:
To me, using ssp_dev seems to be cleaner, as all the places where ssp_set_scr is called, we already have an ssp_dev *ssp = priv->dev.ssp set up, which allows us to call ssp_set_scr(ssp, ...) instead of ssp_set_scr(&priv->dev, ...). Same for ssp_get_scr.
Yeah, the combination of ssp_dev and ssp_device is icky in general and largely historical as a result of a partially done transition of the driver to ssp_device.
I'm working on that clean up. A lot of historical and dependency issues indeed.
And the patch looks OK. The condition of cpu_is_pxa25x() might not be necessary though, ssp->type == PXA25x_SSP already implies that and is more specific.
On Sun, Apr 19, 2009 at 4:01 PM, Eric Miao eric.y.miao@gmail.com wrote:
On Fri, Apr 17, 2009 at 6:22 PM, Mark Brown broonie@sirena.org.uk wrote:
On Fri, Apr 17, 2009 at 11:39:38AM +0200, Philipp Zabel wrote:
To me, using ssp_dev seems to be cleaner, as all the places where ssp_set_scr is called, we already have an ssp_dev *ssp = priv->dev.ssp set up, which allows us to call ssp_set_scr(ssp, ...) instead of ssp_set_scr(&priv->dev, ...). Same for ssp_get_scr.
Yeah, the combination of ssp_dev and ssp_device is icky in general and largely historical as a result of a partially done transition of the driver to ssp_device.
I'm working on that clean up. A lot of historical and dependency issues indeed.
Glad to hear that. I think once SSP is cleaned up, the single USB gadget controller driver is the only problem left for kernels supporting both PXA25x/27x at the same time.
And the patch looks OK. The condition of cpu_is_pxa25x() might not be necessary though, ssp->type == PXA25x_SSP already implies that and is more specific.
Thanks. The idea was that the compiler can remove the PXA25x_SSP branch for kernels without PXA25x support this way. I guess the savings are negligible here, but it shouldn't hurt PXA25x-only kernels either.
regards Philipp
On Sun, Apr 19, 2009 at 11:03 PM, pHilipp Zabel philipp.zabel@gmail.com wrote:
On Sun, Apr 19, 2009 at 4:01 PM, Eric Miao eric.y.miao@gmail.com wrote:
On Fri, Apr 17, 2009 at 6:22 PM, Mark Brown broonie@sirena.org.uk wrote:
On Fri, Apr 17, 2009 at 11:39:38AM +0200, Philipp Zabel wrote:
To me, using ssp_dev seems to be cleaner, as all the places where ssp_set_scr is called, we already have an ssp_dev *ssp = priv->dev.ssp set up, which allows us to call ssp_set_scr(ssp, ...) instead of ssp_set_scr(&priv->dev, ...). Same for ssp_get_scr.
Yeah, the combination of ssp_dev and ssp_device is icky in general and largely historical as a result of a partially done transition of the driver to ssp_device.
I'm working on that clean up. A lot of historical and dependency issues indeed.
Glad to hear that. I think once SSP is cleaned up, the single USB gadget controller driver is the only problem left for kernels supporting both PXA25x/27x at the same time.
And the patch looks OK. The condition of cpu_is_pxa25x() might not be necessary though, ssp->type == PXA25x_SSP already implies that and is more specific.
Thanks. The idea was that the compiler can remove the PXA25x_SSP branch for kernels without PXA25x support this way. I guess the savings are negligible here, but it shouldn't hurt PXA25x-only kernels either.
This is very smart. However, the problem is that cpu_is_pxa25x() implies pxa21x, pxa250, pxa255, pxa26x at the moment, and that pxa255/pxa26x has additional ASSP and NSSP which resembles much the PXA27x_SSP (with additional 4-bit for SCR). I don't think too much about that optimization, yet I wonder if these cases are also counted in.
participants (3)
-
Eric Miao -
Mark Brown -
Philipp Zabel