ASoC: adau17x1: Handling of DSP_RUN register during fw setup

PATCH Superseded Robert Rosengren 2 participants 5 messages Apr 3, 2018 – Apr 11, 2018

Subsystems (3)

Sound ASoC Analog devices inc ASoC codec drivers

lore

Share & tools

lore

Robert Rosengren

author

2018-04-03 08:05 UTC #0 lore raw

PATCH 1/2 ASoC: adau17x1: Handling of DSP_RUN register during fw setup

From: Danny Smith <[email protected]>

DSP_RUN needs to be disabled during firmware write otherwise we can end up with undefined behavior if writing to a dsp which is already running firmware.

Signed-off-by Danny Smith <[email protected]>

---
 sound/soc/codecs/adau17x1.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

sound/soc/codecs/adau17x1.c +7 -1

index 80c2a06285bb..5636b9522462 100644
@@ -838,14 +838,19 @@ EXPORT_SYMBOL_GPL(adau17x1_volatile_register);
 int adau17x1_setup_firmware(struct adau *adau, unsigned int rate)
 {
 	int ret;
-	int dspsr;
+	int dspsr, dsp_run;
 
 	ret = regmap_read(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE, &dspsr);
 	if (ret)
 		return ret;
 
+	ret = regmap_read(adau->regmap, ADAU17X1_DSP_RUN, &dsp_run);
+	if (ret)
+		return ret;
+
 	regmap_write(adau->regmap, ADAU17X1_DSP_ENABLE, 1);
 	regmap_write(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE, 0xf);
+	regmap_write(adau->regmap, ADAU17X1_DSP_RUN, 0);
 
 	ret = sigmadsp_setup(adau->sigmadsp, rate);
 	if (ret) {

@@ -853,6 +858,7 @@ int adau17x1_setup_firmware(struct adau *adau, unsigned int rate)
 		return ret;
 	}
 	regmap_write(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE, dspsr);
+	regmap_write(adau->regmap, ADAU17X1_DSP_RUN, dsp_run);
 
 	return 0;
 }

Signature · 1 line

2.11.0

Robert Rosengren

author

2018-04-03 08:05 UTC #1 lore raw

PATCH 2/2 ASoC: adau17x1: Do not reload dsp-fw if samplerate has not changed

From: Danny Smith <[email protected]>

Reloading fw causes an audiable popping sound, we can avoid this by not reloading if the samplerate is the same as before.

Signed-off-by Danny Smith <[email protected]>

---
 sound/soc/codecs/adau17x1.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

sound/soc/codecs/adau17x1.c +24 ~~-15~~

index 5636b9522462..3c28b7191ecd 100644
@@ -840,25 +840,34 @@ int adau17x1_setup_firmware(struct adau *adau, unsigned int rate)
 	int ret;
 	int dspsr, dsp_run;
 
-	ret = regmap_read(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE, &dspsr);
-	if (ret)
-		return ret;
+	/* Check if sample rate is the same as before. If it is there is no
+	 * point in performing the below steps as the call to
+	 * sigmadsp_setup(...) will return directly when it finds the sample
+	 * rate to be the same as before. By checking this we can prevent an
+	 * audiable popping noise which occours when toggling DSP_RUN.
+	 */
+	if (adau->sigmadsp->current_samplerate != rate) {
+		ret = regmap_read(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE,
+			&dspsr);
+		if (ret)
+			return ret;
 
-	ret = regmap_read(adau->regmap, ADAU17X1_DSP_RUN, &dsp_run);
-	if (ret)
-		return ret;
+		ret = regmap_read(adau->regmap, ADAU17X1_DSP_RUN, &dsp_run);
+		if (ret)
+			return ret;
 
-	regmap_write(adau->regmap, ADAU17X1_DSP_ENABLE, 1);
-	regmap_write(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE, 0xf);
-	regmap_write(adau->regmap, ADAU17X1_DSP_RUN, 0);
+		regmap_write(adau->regmap, ADAU17X1_DSP_ENABLE, 1);
+		regmap_write(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE, 0xf);
+		regmap_write(adau->regmap, ADAU17X1_DSP_RUN, 0);
 
-	ret = sigmadsp_setup(adau->sigmadsp, rate);
-	if (ret) {
-		regmap_write(adau->regmap, ADAU17X1_DSP_ENABLE, 0);
-		return ret;
+		ret = sigmadsp_setup(adau->sigmadsp, rate);
+		if (ret) {
+			regmap_write(adau->regmap, ADAU17X1_DSP_ENABLE, 0);
+			return ret;
+		}
+		regmap_write(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE, dspsr);
+		regmap_write(adau->regmap, ADAU17X1_DSP_RUN, dsp_run);
 	}
-	regmap_write(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE, dspsr);
-	regmap_write(adau->regmap, ADAU17X1_DSP_RUN, dsp_run);
 
 	return 0;
 }

Signature · 1 line

2.11.0

Lars-Peter Clausen

maintainer

2018-04-05 15:33 UTC #2 lore raw

Re: [PATCH 2/2] ASoC: adau17x1: Do not reload dsp-fw if samplerate has not changed

Quoted 5 lines · Robert Rosengren

From: Danny Smith <[email protected]>
Reloading fw causes an audiable popping sound, we can avoid this by not reloading if the samplerate is the same as before.

Seems like a sensible idea thanks.

Quoted 13 lines

Signed-off-by Danny Smith <[email protected]>

---
 sound/soc/codecs/adau17x1.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

sound/soc/codecs/adau17x1.c +0 -0

index 5636b9522462..3c28b7191ecd 100644
@@ -840,25 +840,34 @@ int adau17x1_setup_firmware(struct adau *adau, unsigned int rate)
 	int ret;
 	int dspsr, dsp_run;

sound/soc/codecs/adau17x1.c int adau17x1_setup_firmware(struct adau *adau, unsigned int rate) · line 849

Quoted 10 lines

sound/soc/codecs/adau17x1.c +7 -3

-	ret = regmap_read(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE, &dspsr);
-	if (ret)
-		return ret;
+	/* Check if sample rate is the same as before. If it is there is no
+	 * point in performing the below steps as the call to
+	 * sigmadsp_setup(...) will return directly when it finds the sample
+	 * rate to be the same as before. By checking this we can prevent an
+	 * audiable popping noise which occours when toggling DSP_RUN.
+	 */
+	if (adau->sigmadsp->current_samplerate != rate) {

I think in order to avoid all that re-indention you could just

	if (adau->sigmadsp->current_samplerate == rate)
		return 0;

[...]

Lars-Peter Clausen

maintainer

2018-04-05 15:44 UTC #3 lore raw

In reply to Robert Rosengren

Quoted 6 lines · Robert Rosengren

From: Danny Smith <[email protected]>
DSP_RUN needs to be disabled during firmware write otherwise we can end up with undefined behavior if writing to a dsp which is already running firmware.

Good point, thanks.

I believe there is a possible (but rare under normal circumstances) race condition here though, where DAPM could run at the same time and enable/disable the DSP while the firmware is being loaded.

To avoid this the firmware load sequence should be encapsulated in snd_soc_dapm_mutex_lock()/snd_soc_dapm_mutex_unlock().

Pass struct snd_soc_component instead of struct adau to this function and then use snd_soc_component_get_dapm() to get the DAPM context.

For the next version of the patches please also add the ASoC maintainers to the reciver list since they have to apply the patch. The maintainers are:

Liam Girdwood <[email protected]>
Mark Brown <[email protected]>

Quoted 39 lines

Signed-off-by Danny Smith <[email protected]>

---
 sound/soc/codecs/adau17x1.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

sound/soc/codecs/adau17x1.c +7 -1

index 80c2a06285bb..5636b9522462 100644
@@ -838,14 +838,19 @@ EXPORT_SYMBOL_GPL(adau17x1_volatile_register);
 int adau17x1_setup_firmware(struct adau *adau, unsigned int rate)
 {
 	int ret;
-	int dspsr;
+	int dspsr, dsp_run;
 
 	ret = regmap_read(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE, &dspsr);
 	if (ret)
 		return ret;
 
+	ret = regmap_read(adau->regmap, ADAU17X1_DSP_RUN, &dsp_run);
+	if (ret)
+		return ret;
+
 	regmap_write(adau->regmap, ADAU17X1_DSP_ENABLE, 1);
 	regmap_write(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE, 0xf);
+	regmap_write(adau->regmap, ADAU17X1_DSP_RUN, 0);
 
 	ret = sigmadsp_setup(adau->sigmadsp, rate);
 	if (ret) {

@@ -853,6 +858,7 @@ int adau17x1_setup_firmware(struct adau *adau, unsigned int rate)
 		return ret;
 	}
 	regmap_write(adau->regmap, ADAU17X1_DSP_SAMPLING_RATE, dspsr);
+	regmap_write(adau->regmap, ADAU17X1_DSP_RUN, dsp_run);
 
 	return 0;
 }

Robert Rosengren

author

2018-04-11 05:16 UTC #4 lore raw

Quoted 17 lines · Lars-Peter Clausen

Quoted 6 lines · Robert Rosengren
From: Danny Smith <[email protected]>
DSP_RUN needs to be disabled during firmware write otherwise we can end up with undefined behavior if writing to a dsp which is already running firmware.
Good point, thanks.
I believe there is a possible (but rare under normal circumstances) race condition here though, where DAPM could run at the same time and enable/disable the DSP while the firmware is being loaded.
To avoid this the firmware load sequence should be encapsulated in snd_soc_dapm_mutex_lock()/snd_soc_dapm_mutex_unlock().

Quoted 9 lines

Pass struct snd_soc_component instead of struct adau to this function and then use snd_soc_component_get_dapm() to get the DAPM context.
For the next version of the patches please also add the ASoC maintainers to the reciver list since they have to apply the patch. The maintainers are:
Liam Girdwood <[email protected]>
Mark Brown <[email protected]>

Thanks for your comments! New patches are already sent for your review :)

/ Robert

Quoted 5 lines

Quoted 5 lines
Signed-off-by Danny Smith <[email protected]>
---
 sound/soc/codecs/adau17x1.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)