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

[Ezequiel Garcia <ezequiel@collabora.co.uk>]

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