Re: [alsa-devel] [PATCH V2 02/10] ASoc: mxs: add mxs-saif driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Wed, Jul 13, 2011 at 08:14:09AM +0000, Dong Aisheng-B29396 wrote:

> > What if MCLK is not set corectly for some reason?  We'll just silently do
> > nothing.

> We will just return an error if MCLK is not correct (not 32x or 48x).
>         /* SAIF MCLK should be either 32*fs or 48*fs */
>         if (saif->mclk_in_use && (mclk % 32 != 0) && (mclk % 48 != 0))
>                 return -EINVAL;
> Machine driver should guarantee to set a correct value since it's HW limitation.
> Is that ok?

Please fix the code so you only check if the ratio is valid in one
place, the code currently is too hard to read.

> So it seems, as you said, we may need to set symmetric_rates in the DAI.

Not if there are two separate SAIFs used for each direction - this is
only relevant if one interface provides both playback and capture.

> > > +	stat = __raw_readl(saif->base + SAIF_STAT);
> > > +	if (stat & BM_SAIF_STAT_FIFO_UNDERFLOW_IRQ)
> > > +		dev_dbg(saif->dev, "underrun!!! %d\n", ++saif->fifo_underrun);

> > > +	if (stat & BM_SAIF_STAT_FIFO_OVERFLOW_IRQ)
> > > +		dev_dbg(saif->dev, "overrun!!! %d\n", ++saif->fifo_overrun);

> > Probably dev_err()?
> I met an issue that during each playback when I pressed CTRL+C to stop,
> I would always see a line of "underrun!!!" message.
> This might because we actually do not stop SAIF (clear HW_SAIF_CTRL RUN bit)
> When SNDRV_PCM_TRIGGER_STOP due to codec still needs the clock.
> (clear HW_SAIF_CTRL RUN bit may cause mclk stop)
> So I observed that there was at least one underrun happened.
> It's limitation that sgtl5000 codec needs SAIF mclk.
> Bothered by this "underrun!!!", i use the dev_dbg only for debug purpose.

Hrm, OK.  I guess that's reasonable.

> > > +	__raw_writel(BM_SAIF_STAT_FIFO_UNDERFLOW_IRQ |
> > > +			BM_SAIF_STAT_FIFO_OVERFLOW_IRQ,
> > > +			saif->base + SAIF_STAT + MXS_CLR_ADDR);

> > > +	return IRQ_HANDLED;

> > Should really check to see if underflow or overflow occurred and only
> > report handled if they did - otherwise we might have trouble with new
> > hardware blocks that have other interrupts.

> I may clear those two interrupts separately.
> But I'm not quite understand how it may cause trouble with new hardware
> Block that have other interrupts since it only clear underrun&overrun irq.

Because you return IRQ_HANDLED you're telling the IRQ infrastructure
that you handled the interrupt but you didn't.  If you didn't handle an
interrupt you should return IRQ_NONE instead.