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

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; }

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; }

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;
[...]

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;
}

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(-)
participants (2)
-
Lars-Peter Clausen
-
Robert Rosengren