[alsa-devel] [PATCH v2 2/3] ALSA SoC: Add mpc5200-psc I2S driver

[Mark Brown]

On Sat, Jul 12, 2008 at 02:39:34AM -0600, Grant Likely wrote:

> This is an I2S bus driver for the MPC5200 PSC device.  It is probably
> will not be merged as-is because it uses v1 of the ASoC API, but I want
> to get it out there for comments.

Looks basically good.  A few things below, plus you probably want to run
checkpatch over it.

> +/* Bestcomm DMA irq handler */
> +static irqreturn_t psc_i2s_bcom_irq(int irq, void *_psc_i2s_stream)
> +{
> +	struct psc_i2s_stream *s = _psc_i2s_stream;
> +
> +	//spin_lock(&s->psc_i2s->lock);

Either the lock is needed or it isn't :) .

> + * If this is the first stream open, then grab the IRQ and program most of
> + * the PSC registers.

> +static int psc_i2s_startup(struct snd_pcm_substream *substream)
> +{

The comment here doesn't seem to reflect what the function does - it
looks to just unconditionally reinitialise the controller with things
like this:

> +	/* Disable all interrupts and reset the PSC */
> +	out_be16(&regs->mpc52xx_psc_imr, 0);
> +	out_8(&regs->command, 3 << 4); /* reset transmitter */
> +	out_8(&regs->command, 2 << 4); /* reset receiver */
> +	out_8(&regs->command, 1 << 4); /* reset mode */
> +	out_8(&regs->command, 4 << 4); /* reset error */

which I'd imagine might upset running streams?

> +static int psc_i2s_trigger(struct snd_pcm_substream *substream, int cmd)
> +{

> +       case SNDRV_PCM_TRIGGER_STOP:

> +       case SNDRV_PCM_TRIGGER_STOP:

> +	default:
> +		dev_dbg(psc_i2s->dev, "invalid command\n");
> +		return -EINVAL;
> +	}

This doesn't handle the pause, suspend, resume or pause_release
triggers.  If there's really nothing to do for those it should ignore
them, especially given the default: behaviour.