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

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

On Tue, Jul 12, 2011 at 11:04:37PM +0800, Dong Aisheng wrote:

Looks pretty good, a few small issues below.

> +	/* The SAIF clock should be either 384*fs or 512*fs */
> +	if (saif->mclk_in_use) {
> +		if (mclk % 32 == 0) {
> +			scr &= ~BM_SAIF_CTRL_BITCLK_BASE_RATE;
> +			ret = clk_set_rate(saif->clk, 512 * rate);
> +		} else if (mclk % 48 == 0) {
> +			scr |= BM_SAIF_CTRL_BITCLK_BASE_RATE;
> +			ret = clk_set_rate(saif->clk, 384 * rate);
> +		}

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

> +	if (ret)
> +		return  -EINVAL;

Should pass through any error codes we get.

In hw_params()...
> +	stat = __raw_readl(saif->base + SAIF_STAT);
> +	if (stat & BM_SAIF_STAT_BUSY) {
> +		dev_err(cpu_dai->dev, "error: busy\n");
> +		return -EBUSY;
> +	}

Does this work for simultaneous playback and record?  Perhaps you need
to set symmetric_rates in the DAI (if the hardware needs the same sample
rate for playback and record) and have a check here to see if we're
trying to apply the same configuration as we already have.

> +	struct mxs_saif *saif = dev_id;
> +	unsigned int stat;
> +
> +	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()?

> +	__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.

> +	if (pdev->id >= 2)
> +		return -EINVAL;
> +	mxs_saif[pdev->id] = saif;

The id check should check for ARRAY_SIZE() mxs_saif[] to make updates
easier if a new chip has more ports.

> +	saif->dma_param.chan_irq = platform_get_irq(pdev, 1);
> +	if (saif->dma_param.chan_irq < 0) {

Does the IRQ requesting need to happen after you've set the base
address?  Otherwise there's a potential race if for some reason the IRQ
fires before we've got the data structures set up.