[PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Daniel Mack]

[-- Attachment #1: Type: text/plain, Size: 289 bytes --]

This patch makes the tlv320aic33 driver skip the initialisation of the
PLL in case the sysclk is 256 * samplerate. Had to do some minor
refactoring too - the check whether a sysclk set by set_sysclk() is
valid is now done in set_hw_params().

Signed-off-by: Daniel Mack <daniel@caiaq.de>


[-- Attachment #2: alsa-tlv320aic33-skip-pll.diff --]
[-- Type: text/x-diff, Size: 3009 bytes --]

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index 630684f..1c96c36 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -720,7 +720,7 @@ static inline int aic3x_get_divs(int mclk, int rate)
 			return i;
 	}
 
-	return 0;
+	return -1;
 }
 
 static int aic3x_hw_params(struct snd_pcm_substream *substream,
@@ -730,11 +730,40 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_device *socdev = rtd->socdev;
 	struct snd_soc_codec *codec = socdev->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
-	int i;
+	int i, skip_pll;
 	u8 data, pll_p, pll_r, pll_j;
 	u16 pll_d;
 
-	i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+	/* select data word length */
+	data =
+	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		data |= (0x01 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		data |= (0x02 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		data |= (0x03 << 4);
+		break;
+	}
+	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
+
+	/* Do not use the PLL in case our sysclk is 256 * sample rate.
+	 * In this case, use a fake entry in the dividers table to get
+	 * the reference freq */
+	skip_pll = (aic3x->sysclk == 256 * params_rate(params));
+
+	if (skip_pll)
+		i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+	else
+		i = aic3x_get_divs(aic3x_divs[0].mclk, params_rate(params));
+
+	if (i < 0)
+		return -EINVAL;
 
 	/* Route Left DAC to left channel input and
 	 * right DAC to right channel input */
@@ -755,6 +784,9 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	}
 	aic3x_write(codec, AIC3X_CODEC_DATAPATH_REG, data);
 
+	if (skip_pll)
+		return 0;
+
 	/* codec sample rate select */
 	data = aic3x_divs[i].sr_reg;
 	data |= (data << 4);
@@ -782,24 +814,6 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	aic3x_write(codec, AIC3X_PLL_PROGD_REG,
 		    (pll_d & 0x3F) << PLLD_LSB_SHIFT);
 
-	/* select data word length */
-	data =
-	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
-		break;
-	case SNDRV_PCM_FORMAT_S20_3LE:
-		data |= (0x01 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		data |= (0x02 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		data |= (0x03 << 4);
-		break;
-	}
-	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
-
 	return 0;
 }
 
@@ -826,16 +840,8 @@ static int aic3x_set_dai_sysclk(struct snd_soc_codec_dai *codec_dai,
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 
-	switch (freq) {
-	case 12000000:
-	case 19200000:
-	case 22579200:
-	case 33868800:
-		aic3x->sysclk = freq;
-		return 0;
-	}
-
-	return -EINVAL;
+	aic3x->sysclk = freq;
+	return 0;
 }
 
 static int aic3x_set_dai_fmt(struct snd_soc_codec_dai *codec_dai,

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Daniel Mack]

[-- Attachment #1: Type: text/plain, Size: 404 bytes --]

On Thu, Apr 17, 2008 at 09:12:45PM +0200, Daniel Mack wrote:
> This patch makes the tlv320aic33 driver skip the initialisation of the
> PLL in case the sysclk is 256 * samplerate. Had to do some minor
> refactoring too - the check whether a sysclk set by set_sysclk() is
> valid is now done in set_hw_params().

Sorry, there was a small typo in the patch I just submitted.
Use this one instead.

Daniel


[-- Attachment #2: alsa-tlv320aic33-skip-pll.diff --]
[-- Type: text/x-diff, Size: 3009 bytes --]

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index 630684f..7d3f109 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -720,7 +720,7 @@ static inline int aic3x_get_divs(int mclk, int rate)
 			return i;
 	}
 
-	return 0;
+	return -1;
 }
 
 static int aic3x_hw_params(struct snd_pcm_substream *substream,
@@ -730,11 +730,40 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_device *socdev = rtd->socdev;
 	struct snd_soc_codec *codec = socdev->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
-	int i;
+	int i, skip_pll;
 	u8 data, pll_p, pll_r, pll_j;
 	u16 pll_d;
 
-	i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+	/* select data word length */
+	data =
+	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		data |= (0x01 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		data |= (0x02 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		data |= (0x03 << 4);
+		break;
+	}
+	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
+
+	/* Do not use the PLL in case our sysclk is 256 * sample rate.
+	 * In this case, use a fake entry in the dividers table to get
+	 * the reference freq */
+	skip_pll = (aic3x->sysclk == 256 * params_rate(params));
+
+	if (skip_pll)
+		i = aic3x_get_divs(aic3x_divs[0].mclk, params_rate(params));
+	else
+		i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+
+	if (i < 0)
+		return -EINVAL;
 
 	/* Route Left DAC to left channel input and
 	 * right DAC to right channel input */
@@ -755,6 +784,9 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	}
 	aic3x_write(codec, AIC3X_CODEC_DATAPATH_REG, data);
 
+	if (skip_pll)
+		return 0;
+
 	/* codec sample rate select */
 	data = aic3x_divs[i].sr_reg;
 	data |= (data << 4);
@@ -782,24 +814,6 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	aic3x_write(codec, AIC3X_PLL_PROGD_REG,
 		    (pll_d & 0x3F) << PLLD_LSB_SHIFT);
 
-	/* select data word length */
-	data =
-	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
-		break;
-	case SNDRV_PCM_FORMAT_S20_3LE:
-		data |= (0x01 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		data |= (0x02 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		data |= (0x03 << 4);
-		break;
-	}
-	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
-
 	return 0;
 }
 
@@ -826,16 +840,8 @@ static int aic3x_set_dai_sysclk(struct snd_soc_codec_dai *codec_dai,
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 
-	switch (freq) {
-	case 12000000:
-	case 19200000:
-	case 22579200:
-	case 33868800:
-		aic3x->sysclk = freq;
-		return 0;
-	}
-
-	return -EINVAL;
+	aic3x->sysclk = freq;
+	return 0;
 }
 
 static int aic3x_set_dai_fmt(struct snd_soc_codec_dai *codec_dai,

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Jarkko Nikula]

2008/4/17 Daniel Mack <daniel@caiaq.org>:

> On Thu, Apr 17, 2008 at 09:12:45PM +0200, Daniel Mack wrote:
> > This patch makes the tlv320aic33 driver skip the initialisation of the
> > PLL in case the sysclk is 256 * samplerate. Had to do some minor
> > refactoring too - the check whether a sysclk set by set_sysclk() is
> > valid is now done in set_hw_params().
>
> Sorry, there was a small typo in the patch I just submitted.
> Use this one instead.
>
>
I had a quick look to your patch and AIC33 spec.

Is this the same than 256-clock transfer mode? Should you set the bit 3 in
AIC3X_ASD_INTF_CTRLB in this case? Should you also still write the
AIC3X_SAMPLE_RATE_SEL_REG?


Jarkko

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Daniel Mack]

Hi Jarkko,

On Fri, Apr 18, 2008 at 10:59:08AM +0300, Jarkko Nikula wrote:
> I had a quick look to your patch and AIC33 spec.
>
> Is this the same than 256-clock transfer mode?

No, the 256-clock mode is for output only, while in my setup the TLV is
in slave mode. I attached this chip to the I2S output of an PXA270 which
always outputs sample rate * 256 as system clock. In this very case, the
PLL can be bypassed by selecting the left path described on page 27.

> Should you set the bit 3 in
> AIC3X_ASD_INTF_CTRLB in this case? Should you also still write the
> AIC3X_SAMPLE_RATE_SEL_REG?

AIC3X_SAMPLE_RATE_SEL_REG defaults to 0 which is what I want in this
case. Thus, I don't have to write it.

Daniel

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Jarkko Nikula]

On Fri, Apr 18, 2008 at 11:13 AM, Daniel Mack <daniel@caiaq.org> wrote:

> Hi Jarkko,
>
> No, the 256-clock mode is for output only, while in my setup the TLV is
> in slave mode. I attached this chip to the I2S output of an PXA270 which
> always outputs sample rate * 256 as system clock. In this very case, the
> PLL can be bypassed by selecting the left path described on page 27.
>

Ok, now I see. Probably you should refer it as, at least in comment, 128*Q
instead of 256 eventhough the driver is not currently touching the Q value
in AIC3X_PLL_PROGA_REG.


> AIC3X_SAMPLE_RATE_SEL_REG defaults to 0 which is what I want in this
> case. Thus, I don't have to write it.
>
>
Are you sure this is a general case?


Jarkko

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Daniel Mack]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

On Fri, Apr 18, 2008 at 11:58:49AM +0300, Jarkko Nikula wrote:
> > No, the 256-clock mode is for output only, while in my setup the TLV is
> > in slave mode. I attached this chip to the I2S output of an PXA270 which
> > always outputs sample rate * 256 as system clock. In this very case, the
> > PLL can be bypassed by selecting the left path described on page 27.
> >
> 
> Ok, now I see. Probably you should refer it as, at least in comment, 128*Q
> instead of 256 eventhough the driver is not currently touching the Q value
> in AIC3X_PLL_PROGA_REG.

Well, the constraint for that condition is that MCLK = 256*WCLK, and the
reason why that works for the chip without PLL is that Q=2. I stated
that a little better in the attached patch.

> > AIC3X_SAMPLE_RATE_SEL_REG defaults to 0 which is what I want in this
> > case. Thus, I don't have to write it.
> >
> >
> Are you sure this is a general case?

Well, the reset default is 0 (as stated on page 44) and set_hw_params()
is the only location where this value is written. So if it's never 
written with a different value, it should always be set to its default,
no?

Daniel


[-- Attachment #2: alsa-tlv320aic33-skip-pll.diff --]
[-- Type: text/x-diff, Size: 3111 bytes --]

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index 630684f..0f0bda2 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -720,7 +720,7 @@ static inline int aic3x_get_divs(int mclk, int rate)
 			return i;
 	}
 
-	return 0;
+	return -1;
 }
 
 static int aic3x_hw_params(struct snd_pcm_substream *substream,
@@ -730,11 +730,42 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_device *socdev = rtd->socdev;
 	struct snd_soc_codec *codec = socdev->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
-	int i;
+	int i, skip_pll;
 	u8 data, pll_p, pll_r, pll_j;
 	u16 pll_d;
 
-	i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+	/* select data word length */
+	data =
+	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		data |= (0x01 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		data |= (0x02 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		data |= (0x03 << 4);
+		break;
+	}
+	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
+
+	/* Do not use the PLL in case our MCLK is 256 * sample rate.
+	 * The Q value defaults to 2 so the codec sees MCLK as clock
+	 * input (refer to datasheet page 27).
+	 * In this case, use a fake entry in the dividers table to get
+	 * the reference freq */
+	skip_pll = (aic3x->sysclk == params_rate(params) * 256);
+
+	if (skip_pll)
+		i = aic3x_get_divs(aic3x_divs[0].mclk, params_rate(params));
+	else
+		i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+
+	if (i < 0)
+		return -EINVAL;
 
 	/* Route Left DAC to left channel input and
 	 * right DAC to right channel input */
@@ -755,6 +786,9 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	}
 	aic3x_write(codec, AIC3X_CODEC_DATAPATH_REG, data);
 
+	if (skip_pll)
+		return 0;
+
 	/* codec sample rate select */
 	data = aic3x_divs[i].sr_reg;
 	data |= (data << 4);
@@ -782,24 +816,6 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	aic3x_write(codec, AIC3X_PLL_PROGD_REG,
 		    (pll_d & 0x3F) << PLLD_LSB_SHIFT);
 
-	/* select data word length */
-	data =
-	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
-		break;
-	case SNDRV_PCM_FORMAT_S20_3LE:
-		data |= (0x01 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		data |= (0x02 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		data |= (0x03 << 4);
-		break;
-	}
-	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
-
 	return 0;
 }
 
@@ -826,16 +842,8 @@ static int aic3x_set_dai_sysclk(struct snd_soc_codec_dai *codec_dai,
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 
-	switch (freq) {
-	case 12000000:
-	case 19200000:
-	case 22579200:
-	case 33868800:
-		aic3x->sysclk = freq;
-		return 0;
-	}
-
-	return -EINVAL;
+	aic3x->sysclk = freq;
+	return 0;
 }
 
 static int aic3x_set_dai_fmt(struct snd_soc_codec_dai *codec_dai,

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Mark Brown]

On Fri, Apr 18, 2008 at 11:38:21AM +0200, Daniel Mack wrote:
> On Fri, Apr 18, 2008 at 11:58:49AM +0300, Jarkko Nikula wrote:

> > > AIC3X_SAMPLE_RATE_SEL_REG defaults to 0 which is what I want in this
> > > case. Thus, I don't have to write it.

> > Are you sure this is a general case?

> Well, the reset default is 0 (as stated on page 44) and set_hw_params()
> is the only location where this value is written. So if it's never 
> written with a different value, it should always be set to its default,
> no?

Not all systems use a static SYSCLK rate - sometimes systems will select
the rate based on their current usage.  If the system has previously had
to configure it then the register won't be at the default value any more.

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Daniel Mack]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

On Fri, Apr 18, 2008 at 11:08:59AM +0100, Mark Brown wrote:
> > Well, the reset default is 0 (as stated on page 44) and set_hw_params()
> > is the only location where this value is written. So if it's never 
> > written with a different value, it should always be set to its default,
> > no?
> 
> Not all systems use a static SYSCLK rate - sometimes systems will select
> the rate based on their current usage.  If the system has previously had
> to configure it then the register won't be at the default value any more.

Ok, I didn't consider that. New version of the patch attached which
resets AIC3X_PLL_PROGA_REG and AIC3X_SAMPLE_RATE_SEL_REG to appropriate
values in case the PLL setup is skipped.

Daniel


[-- Attachment #2: alsa-tlv320aic33-skip-pll.diff --]
[-- Type: text/x-diff, Size: 3762 bytes --]

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index 630684f..83a87f6 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -720,7 +720,7 @@ static inline int aic3x_get_divs(int mclk, int rate)
 			return i;
 	}
 
-	return 0;
+	return -1;
 }
 
 static int aic3x_hw_params(struct snd_pcm_substream *substream,
@@ -730,11 +730,44 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_device *socdev = rtd->socdev;
 	struct snd_soc_codec *codec = socdev->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
-	int i;
+	int i, skip_pll;
 	u8 data, pll_p, pll_r, pll_j;
 	u16 pll_d;
 
-	i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+	/* select data word length */
+	data =
+	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		data |= (0x01 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		data |= (0x02 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		data |= (0x03 << 4);
+		break;
+	}
+	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
+
+	/* Do not use the PLL in case our MCLK is 256 * sample rate.
+	 * The Q value is set to 2 so the codec sees MCLK as clock
+	 * input (refer to datasheet page 27).
+	 * In this case, use a fake entry in the dividers table to get
+	 * the reference freq */
+	skip_pll = (aic3x->sysclk == params_rate(params) * 256);
+
+	if (skip_pll) {
+		i = aic3x_get_divs(aic3x_divs[0].mclk, params_rate(params));
+		aic3x_write(codec, AIC3X_SAMPLE_RATE_SEL_REG, 0);
+		aic3x_write(codec, AIC3X_PLL_PROGA_REG, 2 << PLLQ_SHIFT);
+	} else
+		i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+
+	if (i < 0)
+		return -EINVAL;
 
 	/* Route Left DAC to left channel input and
 	 * right DAC to right channel input */
@@ -755,6 +788,9 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	}
 	aic3x_write(codec, AIC3X_CODEC_DATAPATH_REG, data);
 
+	if (skip_pll)
+		return 0;
+
 	/* codec sample rate select */
 	data = aic3x_divs[i].sr_reg;
 	data |= (data << 4);
@@ -782,24 +818,6 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	aic3x_write(codec, AIC3X_PLL_PROGD_REG,
 		    (pll_d & 0x3F) << PLLD_LSB_SHIFT);
 
-	/* select data word length */
-	data =
-	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
-		break;
-	case SNDRV_PCM_FORMAT_S20_3LE:
-		data |= (0x01 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		data |= (0x02 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		data |= (0x03 << 4);
-		break;
-	}
-	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
-
 	return 0;
 }
 
@@ -826,16 +844,8 @@ static int aic3x_set_dai_sysclk(struct snd_soc_codec_dai *codec_dai,
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 
-	switch (freq) {
-	case 12000000:
-	case 19200000:
-	case 22579200:
-	case 33868800:
-		aic3x->sysclk = freq;
-		return 0;
-	}
-
-	return -EINVAL;
+	aic3x->sysclk = freq;
+	return 0;
 }
 
 static int aic3x_set_dai_fmt(struct snd_soc_codec_dai *codec_dai,
diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
index d0cdeeb..e310374 100644
--- a/sound/soc/codecs/tlv320aic3x.h
+++ b/sound/soc/codecs/tlv320aic3x.h
@@ -108,6 +108,7 @@
 #define DACR1_2_RLOPM_VOL		92
 #define LLOPM_CTRL			86
 #define RLOPM_CTRL			93
+
 /* Clock generation control register */
 #define AIC3X_CLKGEN_CTRL_REG		102
 
@@ -128,6 +129,7 @@
 
 /* PLL registers bitfields */
 #define PLLP_SHIFT		0
+#define PLLQ_SHIFT		3
 #define PLLR_SHIFT		0
 #define PLLJ_SHIFT		2
 #define PLLD_MSB_SHIFT		0

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Jarkko Nikula]

On Fri, Apr 18, 2008 at 12:38 PM, Daniel Mack <daniel@caiaq.org> wrote:

> On Fri, Apr 18, 2008 at 11:58:49AM +0300, Jarkko Nikula wrote:
> > Ok, now I see. Probably you should refer it as, at least in comment,
> 128*Q
> > instead of 256 eventhough the driver is not currently touching the Q
> value
> > in AIC3X_PLL_PROGA_REG.
>
> Well, the constraint for that condition is that MCLK = 256*WCLK, and the
> reason why that works for the chip without PLL is that Q=2. I stated
> that a little better in the attached patch.
>

I would rather refer there just "Fsref = CLKDIV_IN / (128*Q)" as the
condition where PLL can be disabled (or is mandatory where it must be
disabled?).

Referring to data sheet page 27 is not good as it's correct only for certain
version of AIC33 spec and driver supports other AIC3x chips as well :-)


> > >
> > Are you sure this is a general case?
>
> Well, the reset default is 0 (as stated on page 44) and set_hw_params()
> is the only location where this value is written. So if it's never
> written with a different value, it should always be set to its default,
> no?
>
>
I mean even if the I2S interface Fs meets the MCLK/256 condition, the audio
sample rate may be lower and it may be still required to set ADC and DAC
rates.


Jarkko

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Daniel Mack]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

On Fri, Apr 18, 2008 at 01:47:33PM +0300, Jarkko Nikula wrote:
> > Well, the constraint for that condition is that MCLK = 256*WCLK, and the
> > reason why that works for the chip without PLL is that Q=2. I stated
> > that a little better in the attached patch.
> >
> 
> I would rather refer there just "Fsref = CLKDIV_IN / (128*Q)" as the
> condition where PLL can be disabled (or is mandatory where it must be
> disabled?).
> 
> Referring to data sheet page 27 is not good as it's correct only for certain
> version of AIC33 spec and driver supports other AIC3x chips as well :-)

Ok, I agree. I changed that to find an appropriate value for Q
programmatically. Have a look at the attached patch, please. I hope i
finally got it now ;)

Daniel


Subject: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

Try to find an appropriate value for Q during clock setup in order to
bypass the usage of the PLL in some cases. This saves some entries in
the dividers table.

Signed-off-by: Daniel Mack <daniel@caiaq.de>


[-- Attachment #2: alsa-tlv320aic33-skip-pll.diff --]
[-- Type: text/x-diff, Size: 4267 bytes --]

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index 630684f..4ddfe91 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -720,7 +720,7 @@ static inline int aic3x_get_divs(int mclk, int rate)
 			return i;
 	}
 
-	return 0;
+	return -1;
 }
 
 static int aic3x_hw_params(struct snd_pcm_substream *substream,
@@ -730,11 +730,52 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_device *socdev = rtd->socdev;
 	struct snd_soc_codec *codec = socdev->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
-	int i;
-	u8 data, pll_p, pll_r, pll_j;
+	int i, bypass_pll = 0;
+	u8 data, pll_p, pll_r, pll_j, pll_q;
 	u16 pll_d;
 
-	i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+	/* select data word length */
+	data =
+	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		data |= (0x01 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		data |= (0x02 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		data |= (0x03 << 4);
+		break;
+	}
+	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
+
+	/* Try to find a value for Q which allows us to bypass the PLL and
+	 * generate CODEC_CLK directly. This saves some entries in
+	 * aic3x_divs[]. */
+	for (pll_q = 2; pll_q < 18; pll_q++)
+		if (params_rate(params) == aic3x->sysclk / (128 * pll_q)) {
+			bypass_pll = 1;
+			break;
+		}
+
+	 /* If the PLL is bypassed, use first entry in the dividers table
+	 * to lookup fsref. */
+	if (bypass_pll) {
+		pll_q &= 0xf;
+		i = aic3x_get_divs(aic3x_divs[0].mclk, params_rate(params));
+		aic3x_write(codec, AIC3X_SAMPLE_RATE_SEL_REG, 0);
+		aic3x_write(codec, AIC3X_PLL_PROGA_REG, pll_q << PLLQ_SHIFT);
+		aic3x_write(codec, AIC3X_GPIOB_REG, CODEC_CLKIN_CLKDIV);
+	} else {
+		i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+		aic3x_write(codec, AIC3X_GPIOB_REG, CODEC_CLKIN_PLLDIV);
+	}
+
+	if (i < 0)
+		return -EINVAL;
 
 	/* Route Left DAC to left channel input and
 	 * right DAC to right channel input */
@@ -755,6 +796,9 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	}
 	aic3x_write(codec, AIC3X_CODEC_DATAPATH_REG, data);
 
+	if (bypass_pll)
+		return 0;
+
 	/* codec sample rate select */
 	data = aic3x_divs[i].sr_reg;
 	data |= (data << 4);
@@ -782,24 +826,6 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	aic3x_write(codec, AIC3X_PLL_PROGD_REG,
 		    (pll_d & 0x3F) << PLLD_LSB_SHIFT);
 
-	/* select data word length */
-	data =
-	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
-		break;
-	case SNDRV_PCM_FORMAT_S20_3LE:
-		data |= (0x01 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		data |= (0x02 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		data |= (0x03 << 4);
-		break;
-	}
-	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
-
 	return 0;
 }
 
@@ -826,16 +852,8 @@ static int aic3x_set_dai_sysclk(struct snd_soc_codec_dai *codec_dai,
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 
-	switch (freq) {
-	case 12000000:
-	case 19200000:
-	case 22579200:
-	case 33868800:
-		aic3x->sysclk = freq;
-		return 0;
-	}
-
-	return -EINVAL;
+	aic3x->sysclk = freq;
+	return 0;
 }
 
 static int aic3x_set_dai_fmt(struct snd_soc_codec_dai *codec_dai,
diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
index d0cdeeb..d49d001 100644
--- a/sound/soc/codecs/tlv320aic3x.h
+++ b/sound/soc/codecs/tlv320aic3x.h
@@ -109,6 +109,7 @@
 #define LLOPM_CTRL			86
 #define RLOPM_CTRL			93
 /* Clock generation control register */
+#define AIC3X_GPIOB_REG			101
 #define AIC3X_CLKGEN_CTRL_REG		102
 
 /* Page select register bits */
@@ -128,12 +129,15 @@
 
 /* PLL registers bitfields */
 #define PLLP_SHIFT		0
+#define PLLQ_SHIFT		3
 #define PLLR_SHIFT		0
 #define PLLJ_SHIFT		2
 #define PLLD_MSB_SHIFT		0
 #define PLLD_LSB_SHIFT		2
 
 /* Clock generation register bits */
+#define CODEC_CLKIN_PLLDIV	0
+#define CODEC_CLKIN_CLKDIV	1
 #define PLL_CLKIN_SHIFT		4
 #define MCLK_SOURCE		0x0
 #define PLL_CLKDIV_SHIFT	0

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Jarkko Nikula]

On Fri, Apr 18, 2008 at 2:41 PM, Daniel Mack <daniel@caiaq.org> wrote:

>
> Ok, I agree. I changed that to find an appropriate value for Q
> programmatically. Have a look at the attached patch, please. I hope i
> finally got it now ;)


That sounds a good idea. Then more cases will be covered.

What I noticed that instead of params_rate, I think we should compare here
the FSref of 44.1 and 48 kHz (how about dual-rate mode?) when defining can
the PLL be bypassed

if (params_rate(params) == aic3x->sysclk / (128 * pll_q))

---

Probably you forgot to move bypass case in this version after writing the
AIC3X_SAMPLE_RATE_SEL_REG?

Spec is also saying that when NDAC is 1.5, 2.5, ... 5.5, then odd values of
Q are not allowed.

Not the easiest chip... probably worth to ask from TI what's the case e.g.
when FSref is 2*48 kHz and ADC/DAC rate is 64 kHz (NDAC is 1.5).

And of course, over-designing the driver is not the purpose and probably
some special cases can be now covered just with -EINVAL and let the user who
needs them to send a patch :-)


Jarkko

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Daniel Mack]

On Fri, Apr 18, 2008 at 04:47:16PM +0300, Jarkko Nikula wrote:
> > Ok, I agree. I changed that to find an appropriate value for Q
> > programmatically. Have a look at the attached patch, please. I hope i
> > finally got it now ;)
> 
> 
> That sounds a good idea. Then more cases will be covered.
> 
> What I noticed that instead of params_rate, I think we should compare here
> the FSref of 44.1 and 48 kHz (how about dual-rate mode?) when defining can
> the PLL be bypassed
> 
> if (params_rate(params) == aic3x->sysclk / (128 * pll_q))

Hmm? Why do you think so? I'm afraid I don't get your point here.

> Probably you forgot to move bypass case in this version after writing the
> AIC3X_SAMPLE_RATE_SEL_REG?

No, actually not.

> Spec is also saying that when NDAC is 1.5, 2.5, ... 5.5, then odd values of
> Q are not allowed.

NDAC and NADC are both hard-coded to 0 (divider of 1) at the moment.
Support for more flexible settings could be done in another step.
Also, the PLL setup could be done by calculation of values rather
that by a lookup table.

I'd like to see this patch applied now as base for further refinements.
Do you agree?

Daniel

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Jarkko Nikula]

On Fri, Apr 18, 2008 at 5:03 PM, Daniel Mack <daniel@caiaq.org> wrote:

> On Fri, Apr 18, 2008 at 04:47:16PM +0300, Jarkko Nikula wrote:
> > if (params_rate(params) == aic3x->sysclk / (128 * pll_q))
>
> Hmm? Why do you think so? I'm afraid I don't get your point here.
>

I'm might be completely wrong here since I've used this codec only in
configuration where the interface rate or FSref equals to audio sampling
rate , but as far as I've interpreted the spec correctly we can have e.g.
the case where MCLK is 22.5792 MHz, FSref 44.1 kHz (i.e. PLL can be
disabled) but audio sampling rate is 11.025 kHz (== params_rate(params)).

And I think here equation "Fsref = CLKDIV_IN / (128 × Q)" Fsref refers to
44.1 or 48 kHz not the audio sampling rate?

But I don't want to speculate or do my own interpretation here. I'll ask
this from TI next week from my jarkko.nikula@nokia.com and can put you as a
cc as well.

Or do the driver author have any better knowledge here?


> > Probably you forgot to move bypass case in this version after writing
> the
> > AIC3X_SAMPLE_RATE_SEL_REG?
>
> No, actually not.
>


Your latest patch has:

+    if (bypass_pll)
+        return 0;
+
     /* codec sample rate select */
     data = aic3x_divs[i].sr_reg;
     data |= (data << 4);


>
> > Spec is also saying that when NDAC is 1.5, 2.5, ... 5.5, then odd values
> of
> > Q are not allowed.
>
> NDAC and NADC are both hard-coded to 0 (divider of 1) at the moment.
> Support for more flexible settings could be done in another step.
> Also, the PLL setup could be done by calculation of values rather
> that by a lookup table.


Again, I'm might be wrong but I NDAC and NADC refers to register
AIC3X_SAMPLE_RATE_SEL_REG which driver already takes into account properly.


> I'd like to see this patch applied now as base for further refinements.
> Do you agree?
>
>
I cannot prevent it to be applied, that's the driver author or maintainers
task to justify. I can only throw some discussion here if I see something to
be unclear (to me).

And don't take my comments as a nitpicking. Definitely patch like this is
worth to get in but we have to make sure that no possible bugs are
introduced. -EINVAL is much better alternative for some new configurations.


Jarkko

Re: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Daniel Mack]

On Fri, Apr 18, 2008 at 10:37:36PM +0300, Jarkko Nikula wrote:
> I'm might be completely wrong here since I've used this codec only in
> configuration where the interface rate or FSref equals to audio sampling
> rate , but as far as I've interpreted the spec correctly we can have e.g.
> the case where MCLK is 22.5792 MHz, FSref 44.1 kHz (i.e. PLL can be
> disabled) but audio sampling rate is 11.025 kHz (== params_rate(params)).

Of course. Fsref is a setting for the filter to tell it which internal
base-frequency it is operating on. But the setup of the sampling
frequency has different meaning. Also, the clocking diagram (page 27 in
the aic33 manual) doesn't even show that entity, or did I miss anything
after staring at this for some days? ;)

> But I don't want to speculate or do my own interpretation here. I'll ask
> this from TI next week from my jarkko.nikula@nokia.com and can put you as a
> cc as well.

Ok, thanks :)

> Your latest patch has:
> 
> +    if (bypass_pll)
> +        return 0;
> +

Yes, since all other setup can be skipped in case the PLL is bypassed.
Did you miss the question here? ;)

> Again, I'm might be wrong but I NDAC and NADC refers to register
> AIC3X_SAMPLE_RATE_SEL_REG which driver already takes into account properly.

Only in case the PLL is enabled. I agree that there could be some more
setup around all the possible settings in this clock generation, but I'd
my approach as a good first start.

> And don't take my comments as a nitpicking. Definitely patch like this is
> worth to get in but we have to make sure that no possible bugs are
> introduced. -EINVAL is much better alternative for some new configurations.

No, I don't take it as nitpicking :) I agree that a driver for such a
complex chip has to be modified with care to have any regression. But I
don't see such as far, so I'd take this patch as a step towards a better
solution. Unless someone comes up claiming that it's breaking anything.

Thanks and best rgeards,
Daniel