[PATCH] ASoC: adau1701: Reset codec based on sample rate changes

[PATCH] ASoC: adau1701: Reset codec based on sample rate changes

[pascal.huerst]

From: Pascal Huerst <pascal.huerst@gmail.com>

Instead of checking if mclk/lrclk ratio has changed, check if
sample rate has changed. In certain cases, the mclk might be
changed in the machine driver, which can lead to the same
mclk/lrclk ration, eventhow the sample rate has changed.

Since the codec has to be programmed differently for every
sample rate, its better to check for samplerate changes instead
of mclk/lrclk ration changes.

Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
---
 sound/soc/codecs/adau1701.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/adau1701.c b/sound/soc/codecs/adau1701.c
index 202dea1..1dbbcda 100644
--- a/sound/soc/codecs/adau1701.c
+++ b/sound/soc/codecs/adau1701.c
@@ -111,6 +111,7 @@ struct adau1701 {
 	unsigned int dai_fmt;
 	unsigned int pll_clkdiv;
 	unsigned int sysclk;
+	unsigned int current_rate;
 	struct regmap *regmap;
 	struct i2c_client *client;
 	u8 pin_config[12];
@@ -436,22 +437,24 @@ static int adau1701_hw_params(struct snd_pcm_substream *substream,
 {
 	struct snd_soc_codec *codec = dai->codec;
 	struct adau1701 *adau1701 = snd_soc_codec_get_drvdata(codec);
-	unsigned int clkdiv = adau1701->sysclk / params_rate(params);
+	unsigned int rate = params_rate(params);
+	unsigned int clkdiv = adau1701->sysclk / rate;
 	unsigned int val;
 	int ret;

 	/*
-	 * If the mclk/lrclk ratio changes, the chip needs updated PLL
+	 * If the sample rate changes, the chip needs updated PLL
 	 * mode GPIO settings, and a full reset cycle, including a new
 	 * firmware upload.
 	 */
-	if (clkdiv != adau1701->pll_clkdiv) {
-		ret = adau1701_reset(codec, clkdiv, params_rate(params));
+	if (adau1701->current_rate != rate) {
+		adau1701->current_rate = rate;
+		ret = adau1701_reset(codec, clkdiv, rate);
 		if (ret < 0)
 			return ret;
 	}

-	switch (params_rate(params)) {
+	switch (rate) {
 	case 192000:
 		val = ADAU1701_DSPCTRL_SR_192;
 		break;
--
2.5.0

Re: [PATCH] ASoC: adau1701: Reset codec based on sample rate changes

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 562 bytes --]

On Wed, Mar 23, 2016 at 12:58:23PM +0100, pascal.huerst@gmail.com wrote:

> Instead of checking if mclk/lrclk ratio has changed, check if
> sample rate has changed. In certain cases, the mclk might be
> changed in the machine driver, which can lead to the same
> mclk/lrclk ration, eventhow the sample rate has changed.

> Since the codec has to be programmed differently for every
> sample rate, its better to check for samplerate changes instead
> of mclk/lrclk ration changes.

Why does this mean we have to reset the CODEC rather than just
reprogramming it?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: adau1701: Reset codec based on sample rate changes

[Lars-Peter Clausen]

[-- Attachment #1.1: Type: text/plain, Size: 929 bytes --]

On 03/23/2016 01:48 PM, Mark Brown wrote:
> On Wed, Mar 23, 2016 at 12:58:23PM +0100, pascal.huerst@gmail.com wrote:
> 
>> Instead of checking if mclk/lrclk ratio has changed, check if
>> sample rate has changed. In certain cases, the mclk might be
>> changed in the machine driver, which can lead to the same
>> mclk/lrclk ration, eventhow the sample rate has changed.
> 
>> Since the codec has to be programmed differently for every
>> sample rate, its better to check for samplerate changes instead
>> of mclk/lrclk ration changes.
> 
> Why does this mean we have to reset the CODEC rather than just
> reprogramming it?

Quoting from the datasheet:

	The clock mode should not be changed without also resetting
	the ADAU1701. If the mode is changed during operation, a
	click or pop can result in the output signals. The state of the
	PLL_MODEx pins should be changed while RESET is held low.

- Lars


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: adau1701: Reset codec based on sample rate changes

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 598 bytes --]

On Wed, Mar 23, 2016 at 02:37:10PM +0100, Lars-Peter Clausen wrote:
> On 03/23/2016 01:48 PM, Mark Brown wrote:

> > Why does this mean we have to reset the CODEC rather than just
> > reprogramming it?

> Quoting from the datasheet:

> 	The clock mode should not be changed without also resetting
> 	the ADAU1701. If the mode is changed during operation, a
> 	click or pop can result in the output signals. The state of the
> 	PLL_MODEx pins should be changed while RESET is held low.

That should be in the commit log and/or code since that's *extremely*
unusual and is likely to surprise people.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: adau1701: Reset codec based on sample rate changes

[Lars-Peter Clausen]

On 03/23/2016 12:58 PM, pascal.huerst@gmail.com wrote:
> From: Pascal Huerst <pascal.huerst@gmail.com>
> 
> Instead of checking if mclk/lrclk ratio has changed, check if
> sample rate has changed. In certain cases, the mclk might be
> changed in the machine driver, which can lead to the same
> mclk/lrclk ration, eventhow the sample rate has changed.
> 
> Since the codec has to be programmed differently for every
> sample rate, its better to check for samplerate changes instead
> of mclk/lrclk ration changes.

Mark's comment made me give this some additional though. Do we actually
need to reset the device if the clkdiv did not change. Stopping the DSP,
uploading the new firmware and then restarting it should be sufficient.
But on the other hand the time the reset takes should be negligible
compared to programming the firmware, so it might be ok to always do it.
Let me know what you think.

Re: [PATCH] ASoC: adau1701: Reset codec based on sample rate changes

[Pascal Huerst]

On 23.03.2016 14:44, Lars-Peter Clausen wrote:
> On 03/23/2016 12:58 PM, pascal.huerst@gmail.com wrote:
>> From: Pascal Huerst <pascal.huerst@gmail.com>
>>
>> Instead of checking if mclk/lrclk ratio has changed, check if
>> sample rate has changed. In certain cases, the mclk might be
>> changed in the machine driver, which can lead to the same
>> mclk/lrclk ration, eventhow the sample rate has changed.
>>
>> Since the codec has to be programmed differently for every
>> sample rate, its better to check for samplerate changes instead
>> of mclk/lrclk ration changes.
> 
> Mark's comment made me give this some additional though. Do we actually
> need to reset the device if the clkdiv did not change. Stopping the DSP,
> uploading the new firmware and then restarting it should be sufficient.
> But on the other hand the time the reset takes should be negligible
> compared to programming the firmware, so it might be ok to always do it.
> Let me know what you think.

Ok, I see your point. So I did some measurements.

On our devices,

a firmware download takes about: 844ms
Resetting the pll settings takes about: 87ms

I'm not sure, if this worth the effort, but certainly it could be done.
I would probably leave it for now, be a bit more precise in the comment
above and add a note to the commit message, as Mark suggested (?)

While at it I also saw, that we should keep the reset line low, while
changing the pll settings. (Which we don't right now)

The datasheet states:

... "The state of the PLL_MODEx pins should be changed while RESET is
held low." ...