[alsa-devel] [PATCH V2 02/10] ASoc: mxs: add mxs-saif driver
broonie at opensource.wolfsonmicro.com
Thu Jul 14 05:06:35 CEST 2011
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.
More information about the Alsa-devel