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.