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

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

[Robert Rosengren]

From: Danny Smith <dannys@axis.com>

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 <dannys@axis.com>
---
 sound/soc/codecs/adau17x1.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/adau17x1.c b/sound/soc/codecs/adau17x1.c
index 80c2a06285bb..5636b9522462 100644
--- a/sound/soc/codecs/adau17x1.c
+++ b/sound/soc/codecs/adau17x1.c
@@ -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;
 }
-- 
2.11.0

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

[Robert Rosengren]

From: Danny Smith <dannys@axis.com>

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 <dannys@axis.com>
---
 sound/soc/codecs/adau17x1.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/sound/soc/codecs/adau17x1.c b/sound/soc/codecs/adau17x1.c
index 5636b9522462..3c28b7191ecd 100644
--- a/sound/soc/codecs/adau17x1.c
+++ b/sound/soc/codecs/adau17x1.c
@@ -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;
 }
-- 
2.11.0

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

[Lars-Peter Clausen]

On 04/03/2018 10:05 AM, Robert Rosengren wrote:
> From: Danny Smith <dannys@axis.com>
> 
> 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.

> Signed-off-by: Danny Smith <dannys@axis.com>
> ---
>  sound/soc/codecs/adau17x1.c | 39 ++++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/sound/soc/codecs/adau17x1.c b/sound/soc/codecs/adau17x1.c
> index 5636b9522462..3c28b7191ecd 100644
> --- a/sound/soc/codecs/adau17x1.c
> +++ b/sound/soc/codecs/adau17x1.c
> @@ -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) {

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

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

[...]

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

[Lars-Peter Clausen]

On 04/03/2018 10:05 AM, Robert Rosengren wrote:
> From: Danny Smith <dannys@axis.com>
> 
> 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 <lgirdwood@gmail.com>
Mark Brown <broonie@kernel.org>


> Signed-off-by: Danny Smith <dannys@axis.com>
> ---
>  sound/soc/codecs/adau17x1.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/adau17x1.c b/sound/soc/codecs/adau17x1.c
> index 80c2a06285bb..5636b9522462 100644
> --- a/sound/soc/codecs/adau17x1.c
> +++ b/sound/soc/codecs/adau17x1.c
> @@ -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;
>  }
> 

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

[Robert Rosengren]

On 04/05/2018 05:44 PM, Lars-Peter Clausen wrote:
> On 04/03/2018 10:05 AM, Robert Rosengren wrote:
>> From: Danny Smith <dannys@axis.com>
>>
>> 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 <lgirdwood@gmail.com>
> Mark Brown <broonie@kernel.org>
> 

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

/ Robert

> 
>> Signed-off-by: Danny Smith <dannys@axis.com>
>> ---
>>  sound/soc/codecs/adau17x1.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)