[alsa-devel] [PATCH] ASoC: max98357a: Fix speaker pop when starting playback
From: Mac Chiang mac.chiang@intel.com
Currently, speaker pop/crack noise can happen during PCM playback. This is due to the LRCLK being removed while the BCLK being present, which as per the datasheet can cause large DC output voltage.
While on some platforms simply controlling the SD_MODE pin is enough to remove the noise, others have a significant delay between the BCLK and the LRCLK activation.
Therefore, this commit adds a delay in the codec driver to guarantee clocks are both active by the time SD_MODE is asserted.
Link: https://github.com/raspberrypi/linux/issues/2212 Signed-off-by: Mac Chiang mac.chiang@intel.com Signed-off-by: Stephen Barber smbarber@chromium.org Signed-off-by: Ezequiel Garcia ezequiel@collabora.co.uk --- .../devicetree/bindings/sound/max98357a.txt | 6 ++ sound/soc/codecs/max98357a.c | 69 +++++++++++++++++++--- 2 files changed, 66 insertions(+), 9 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/max98357a.txt b/Documentation/devicetree/bindings/sound/max98357a.txt index 28645a2ff885..de42c9f372ed 100644 --- a/Documentation/devicetree/bindings/sound/max98357a.txt +++ b/Documentation/devicetree/bindings/sound/max98357a.txt @@ -9,10 +9,16 @@ Optional properties: - sdmode-gpios : GPIO specifier for the chip's SD_MODE pin. If this option is not specified then driver does not manage the pin state (e.g. chip is always on). +- sdmode-delay : specify a delay time for SD_MODE pin. According + to the DAC datasheet, if LRCLK is removed while BCLK is present, + the DAC output can cause loud pop/crack noises. This property + specifies a delay for the SD_MODE pin assert, required to + eliminate the noise.
Example:
max98357a { compatible = "maxim,max98357a"; sdmode-gpios = <&qcom_pinmux 25 0>; + sdmode-delay = <5>; }; diff --git a/sound/soc/codecs/max98357a.c b/sound/soc/codecs/max98357a.c index 426ed2dae6ca..9666fa3602a5 100644 --- a/sound/soc/codecs/max98357a.c +++ b/sound/soc/codecs/max98357a.c @@ -27,27 +27,57 @@ #include <sound/soc-dai.h> #include <sound/soc-dapm.h>
+struct max98357a_priv { + struct delayed_work enable_sdmode_work; + struct gpio_desc *sdmode; + unsigned int sdmode_delay; + int sdmode_enabled; + spinlock_t sdmode_lock; +}; + +static void max98357a_enable_sdmode_work(struct work_struct *work) +{ + struct max98357a_priv *max98357a = container_of(work, + struct max98357a_priv, enable_sdmode_work.work); + unsigned long flags; + + spin_lock_irqsave(&max98357a->sdmode_lock, flags); + gpiod_set_value(max98357a->sdmode, max98357a->sdmode_enabled); + spin_unlock_irqrestore(&max98357a->sdmode_lock, flags); +} + static int max98357a_daiops_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { - struct gpio_desc *sdmode = snd_soc_dai_get_drvdata(dai); + struct snd_soc_codec *codec = dai->codec; + struct max98357a_priv *max98357a = snd_soc_codec_get_drvdata(codec); + unsigned long flags;
- if (!sdmode) + if (!max98357a->sdmode) return 0;
switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - gpiod_set_value(sdmode, 1); + max98357a->sdmode_enabled = 1; + if (max98357a->sdmode_delay) { + queue_delayed_work(system_power_efficient_wq, + &max98357a->enable_sdmode_work, + msecs_to_jiffies(max98357a->sdmode_delay)); + return 0; + } break; case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - gpiod_set_value(sdmode, 0); + max98357a->sdmode_enabled = 0; break; }
+ spin_lock_irqsave(&max98357a->sdmode_lock, flags); + gpiod_set_value(max98357a->sdmode, max98357a->sdmode_enabled); + spin_unlock_irqrestore(&max98357a->sdmode_lock, flags); return 0; }
@@ -61,19 +91,30 @@ static const struct snd_soc_dapm_route max98357a_dapm_routes[] = {
static int max98357a_codec_probe(struct snd_soc_codec *codec) { - struct gpio_desc *sdmode; + struct max98357a_priv *max98357a = snd_soc_codec_get_drvdata(codec);
- sdmode = devm_gpiod_get_optional(codec->dev, "sdmode", GPIOD_OUT_LOW); - if (IS_ERR(sdmode)) - return PTR_ERR(sdmode); + max98357a->sdmode = devm_gpiod_get_optional(codec->dev, + "sdmode", GPIOD_OUT_LOW); + if (IS_ERR(max98357a->sdmode)) + return PTR_ERR(max98357a->sdmode);
- snd_soc_codec_set_drvdata(codec, sdmode); + spin_lock_init(&max98357a->sdmode_lock); + INIT_DELAYED_WORK(&max98357a->enable_sdmode_work, + max98357a_enable_sdmode_work); + return 0; +} + +static int max98357a_codec_remove(struct snd_soc_codec *codec) +{ + struct max98357a_priv *max98357a = snd_soc_codec_get_drvdata(codec);
+ cancel_delayed_work_sync(&max98357a->enable_sdmode_work); return 0; }
static const struct snd_soc_codec_driver max98357a_codec_driver = { .probe = max98357a_codec_probe, + .remove = max98357a_codec_remove, .component_driver = { .dapm_widgets = max98357a_dapm_widgets, .num_dapm_widgets = ARRAY_SIZE(max98357a_dapm_widgets), @@ -107,6 +148,16 @@ static struct snd_soc_dai_driver max98357a_dai_driver = {
static int max98357a_platform_probe(struct platform_device *pdev) { + struct max98357a_priv *max98357a; + + max98357a = devm_kzalloc(&pdev->dev, sizeof(*max98357a), GFP_KERNEL); + if (!max98357a) + return -ENOMEM; + dev_set_drvdata(&pdev->dev, max98357a); + + device_property_read_u32(&pdev->dev, "sdmode-delay", + &max98357a->sdmode_delay); + return snd_soc_register_codec(&pdev->dev, &max98357a_codec_driver, &max98357a_dai_driver, 1); }
On Wed, Mar 21, 2018 at 07:30:15PM -0300, Ezequiel Garcia wrote:
+- sdmode-delay : specify a delay time for SD_MODE pin. According
to the DAC datasheet, if LRCLK is removed while BCLK is present,the DAC output can cause loud pop/crack noises. This propertyspecifies a delay for the SD_MODE pin assert, required toeliminate the noise.
Why is this configurable? This sounds like something entirely within the digital domain of the device rather than something that depends on board configuration and it's hard to see how someone would configure this.
+static void max98357a_enable_sdmode_work(struct work_struct *work) +{
- struct max98357a_priv *max98357a = container_of(work,
struct max98357a_priv, enable_sdmode_work.work);- unsigned long flags;
- spin_lock_irqsave(&max98357a->sdmode_lock, flags);
- gpiod_set_value(max98357a->sdmode, max98357a->sdmode_enabled);
- spin_unlock_irqrestore(&max98357a->sdmode_lock, flags);
+}
What is this lock supposed to accomplish? We perform a single action under the lock which itself has internal locking, it's not going to have any meaningful effect.
Hi Mark,
Thanks for reviewing this so quickly.
On Thu, 2018-03-22 at 09:54 +0800, Mark Brown wrote:
On Wed, Mar 21, 2018 at 07:30:15PM -0300, Ezequiel Garcia wrote:
+- sdmode-delay : specify a delay time for SD_MODE pin. According
to the DAC datasheet, if LRCLK is removed while BCLK ispresent,
the DAC output can cause loud pop/crack noises. Thisproperty
specifies a delay for the SD_MODE pin assert, required toeliminate the noise.Why is this configurable? This sounds like something entirely within the digital domain of the device rather than something that depends on board configuration and it's hard to see how someone would configure this.
The amount of delay needed seems specific to the CPU DAI, not specific to the DAC. The CPU DAIs or the machine could specify this as a parameter, but I can't don't see how.
Perhaps it could be a driver parameter?
+static void max98357a_enable_sdmode_work(struct work_struct *work) +{
- struct max98357a_priv *max98357a = container_of(work,
struct max98357a_priv, enable_sdmode_work.work);- unsigned long flags;
- spin_lock_irqsave(&max98357a->sdmode_lock, flags);
- gpiod_set_value(max98357a->sdmode, max98357a-
sdmode_enabled);
- spin_unlock_irqrestore(&max98357a->sdmode_lock, flags);
+}
What is this lock supposed to accomplish? We perform a single action under the lock which itself has internal locking, it's not going to have any meaningful effect.
I am under the impression it removes the race condition between the gpiod_set_value() happening in the different contexts.
Thanks, Eze
Hi Mark,
Thanks for reviewing this so quickly.
On Thu, 2018-03-22 at 09:54 +0800, Mark Brown wrote:
On Wed, Mar 21, 2018 at 07:30:15PM -0300, Ezequiel Garcia wrote:
+- sdmode-delay : specify a delay time for SD_MODE pin. According
to the DAC datasheet, if LRCLK is removed while BCLK ispresent,
the DAC output can cause loud pop/crack noises. Thisproperty
specifies a delay for the SD_MODE pin assert, required toeliminate the noise.Why is this configurable? This sounds like something entirely within the digital domain of the device rather than something that depends on board configuration and it's hard to see how someone would configure this.
The amount of delay needed seems specific to the CPU DAI, not specific to the DAC. The CPU DAIs or the machine could specify this as a parameter, but I can't don't see how.
Perhaps it could be a driver parameter?
+static void max98357a_enable_sdmode_work(struct work_struct *work) +{
- struct max98357a_priv *max98357a = container_of(work,
struct max98357a_priv, enable_sdmode_work.work);- unsigned long flags;
- spin_lock_irqsave(&max98357a->sdmode_lock, flags);
- gpiod_set_value(max98357a->sdmode, max98357a-
sdmode_enabled);
- spin_unlock_irqrestore(&max98357a->sdmode_lock, flags);
+}
What is this lock supposed to accomplish? We perform a single action under the lock which itself has internal locking, it's not going to have any meaningful effect.
I am under the impression it removes the race condition between the gpiod_set_value() happening in the different contexts.
Thanks, Eze
On Thu, Mar 22, 2018 at 10:01:32AM -0300, Ezequiel Garcia wrote:
On Thu, 2018-03-22 at 09:54 +0800, Mark Brown wrote:
On Wed, Mar 21, 2018 at 07:30:15PM -0300, Ezequiel Garcia wrote:
+- sdmode-delay : specify a delay time for SD_MODE pin. According
to the DAC datasheet, if LRCLK is removed while BCLK ispresent,
the DAC output can cause loud pop/crack noises. Thisproperty
specifies a delay for the SD_MODE pin assert, required toeliminate the noise.
Why is this configurable? This sounds like something entirely within the digital domain of the device rather than something that depends on board configuration and it's hard to see how someone would configure this.
The amount of delay needed seems specific to the CPU DAI, not specific to the DAC. The CPU DAIs or the machine could specify this as a parameter, but I can't don't see how.
This sounds like it's just trying to reimplement the digital mute feature we already have, it sounds more like what you're seeing is noise playing out of the SoC at the start of playback due to startup issues or non-flushed FIFOs. Can you try implementing there please?
Perhaps it could be a driver parameter?
Driver parameters are almost never a good idea. Nobody turns them on and the mechanisms for doing so are bad.
+static void max98357a_enable_sdmode_work(struct work_struct *work) +{
- struct max98357a_priv *max98357a = container_of(work,
struct max98357a_priv, enable_sdmode_work.work);- unsigned long flags;
- spin_lock_irqsave(&max98357a->sdmode_lock, flags);
- gpiod_set_value(max98357a->sdmode, max98357a-
sdmode_enabled);
- spin_unlock_irqrestore(&max98357a->sdmode_lock, flags);
+}
What is this lock supposed to accomplish? We perform a single action under the lock which itself has internal locking, it's not going to have any meaningful effect.
I am under the impression it removes the race condition between the gpiod_set_value() happening in the different contexts.
Can you articulate what this race is and how this prevents it - bear in mind that there's a single operation within each of those locks...
On Tue, 2018-03-27 at 19:36 +0800, Mark Brown wrote:
On Thu, Mar 22, 2018 at 10:01:32AM -0300, Ezequiel Garcia wrote:
On Thu, 2018-03-22 at 09:54 +0800, Mark Brown wrote:
On Wed, Mar 21, 2018 at 07:30:15PM -0300, Ezequiel Garcia wrote:
+- sdmode-delay : specify a delay time for SD_MODE pin. According
to the DAC datasheet, if LRCLK is removed while BCLKis present,
the DAC output can cause loud pop/crack noises. Thisproperty
specifies a delay for the SD_MODE pin assert, requiredto
eliminate the noise.Why is this configurable? This sounds like something entirely within the digital domain of the device rather than something that depends on board configuration and it's hard to see how someone would configure this.
The amount of delay needed seems specific to the CPU DAI, not specific to the DAC. The CPU DAIs or the machine could specify this as a parameter, but I can't don't see how.
This sounds like it's just trying to reimplement the digital mute feature we already have, it sounds more like what you're seeing is noise playing out of the SoC at the start of playback due to startup issues or non-flushed FIFOs. Can you try implementing there please?
OK, I'll see how it goes using digital mute. Please discard v2 then.
Thanks, Eze
On Tue, 2018-03-27 at 19:36 +0800, Mark Brown wrote:
On Thu, Mar 22, 2018 at 10:01:32AM -0300, Ezequiel Garcia wrote:
On Thu, 2018-03-22 at 09:54 +0800, Mark Brown wrote:
On Wed, Mar 21, 2018 at 07:30:15PM -0300, Ezequiel Garcia wrote:
+- sdmode-delay : specify a delay time for SD_MODE pin. According
to the DAC datasheet, if LRCLK is removed while BCLKis present,
the DAC output can cause loud pop/crack noises. Thisproperty
specifies a delay for the SD_MODE pin assert, requiredto
eliminate the noise.Why is this configurable? This sounds like something entirely within the digital domain of the device rather than something that depends on board configuration and it's hard to see how someone would configure this.
The amount of delay needed seems specific to the CPU DAI, not specific to the DAC. The CPU DAIs or the machine could specify this as a parameter, but I can't don't see how.
This sounds like it's just trying to reimplement the digital mute feature we already have, it sounds more like what you're seeing is noise playing out of the SoC at the start of playback due to startup issues or non-flushed FIFOs. Can you try implementing there please?
On a second look, I don't think digital mute helps here. And I don't think the problem is fixable at the CPU DAI level.
The noise has been reported on bcm2835-i2s and rockchip-i2s, which made me think most SoCs were unable to guarantee the clocks could be stopped properly.
As the commit explains, the root of the noise is that this amplifier specifies that the LRCLK cannot be disabled with the BCLK being enabled.
I don't see any mechanism at the SoC level to guarantee that the LRCLK and BCLK clocks will be shutdown in order.
Hopefully, I'm making sense here.
Perhaps it could be a driver parameter?
Driver parameters are almost never a good idea. Nobody turns them on and the mechanisms for doing so are bad.
Well, from the above description, it seems this noise arises from a limitation of the amplifier, so it doesn't seem too bad to apply a small delay and make it configurable.
The default value (no delay) will work for most users, and the delay parameter would be turned on by those having noise issues with this driver.
That is what patch v2 does, let me know if you still think it's horrible, or if you think I am still missing anything.
Thanks, Eze
On Tue, Mar 27, 2018 at 06:57:44PM -0300, Ezequiel Garcia wrote:
On a second look, I don't think digital mute helps here. And I don't think the problem is fixable at the CPU DAI level.
The noise has been reported on bcm2835-i2s and rockchip-i2s, which made me think most SoCs were unable to guarantee the clocks could be stopped properly.
As the commit explains, the root of the noise is that this amplifier specifies that the LRCLK cannot be disabled with the BCLK being enabled.
I don't see any mechanism at the SoC level to guarantee that the LRCLK and BCLK clocks will be shutdown in order.
Honestly I'm surprised that there's a big issue here - most of the SoCs I've seen shut everything down at pretty much the same time. It sounds like this CODEC is *extremely* fragile somehow and probably can't really tolerate having the clocks stopped at all.
Well, from the above description, it seems this noise arises from a limitation of the amplifier, so it doesn't seem too bad to apply a small delay and make it configurable.
The default value (no delay) will work for most users, and the delay parameter would be turned on by those having noise issues with this driver.
That is what patch v2 does, let me know if you still think it's horrible, or if you think I am still missing anything.
I'm still not super sure why you need to assert SD_MODE with an extra delay here (for the benefit of the archives that's shutdown mode which powers the chip off) - if anything I'd have thought it would be better to assert it before stopping any clock and then deassert it after the clocks are running again which would be what mute would do. Why is that not sufficient?
participants (4)
-
Ezequiel Garcia -
Ezequiel Garcia -
Ezequiel Garcia -
Mark Brown