[PATCH] ASoC: max98357a: Fix speaker pop when starting playback

[PATCH] ASoC: max98357a: Fix speaker pop when starting playback

[Ezequiel Garcia]

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

Re: [PATCH] ASoC: max98357a: Fix speaker pop when starting playback

[Mark Brown]

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

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 property
> +        specifies a delay for the SD_MODE pin assert, required to
> +        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.

> +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.

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

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

Re: [PATCH] ASoC: max98357a: Fix speaker pop when starting playback

[Ezequiel Garcia]

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 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.
> 
> 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

Re: [PATCH] ASoC: max98357a: Fix speaker pop when starting playback

[Ezequiel Garcia]

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 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.
> 
> 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

Re: [PATCH] ASoC: max98357a: Fix speaker pop when starting playback

[Mark Brown]

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

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 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.

> > 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...

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

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

Re: [PATCH] ASoC: max98357a: Fix speaker pop when starting playback

[Ezequiel Garcia]

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 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.
> > > 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

Re: [PATCH] ASoC: max98357a: Fix speaker pop when starting playback

[Ezequiel Garcia]

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 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.
> > > 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

Re: [PATCH] ASoC: max98357a: Fix speaker pop when starting playback

[Mark Brown]

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

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?

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

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