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

[Mark Brown]

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...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20180327/7c108f5a/attachment.sig>