[PATCH] ASoC: pxa-ssp: Don't use SSCR0_SerClkDiv and SSCR0_SCR

[PATCH] ASoC: pxa-ssp: Don't use SSCR0_SerClkDiv and SSCR0_SCR

[Philipp Zabel]

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.
-- 
1.6.2.3

Re: [PATCH] ASoC: pxa-ssp: Don't use SSCR0_SerClkDiv and SSCR0_SCR

[Mark Brown]

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!

Re: [PATCH] ASoC: pxa-ssp: Don't use SSCR0_SerClkDiv and SSCR0_SCR

[Eric Miao]

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.

Re: [PATCH] ASoC: pxa-ssp: Don't use SSCR0_SerClkDiv and SSCR0_SCR

[pHilipp Zabel]

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

Re: [PATCH] ASoC: pxa-ssp: Don't use SSCR0_SerClkDiv and SSCR0_SCR

[Eric Miao]

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.