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(®s->mpc52xx_psc_imr, 0);
- out_8(®s->command, 3 << 4); /* reset transmitter */
- out_8(®s->command, 2 << 4); /* reset receiver */
- out_8(®s->command, 1 << 4); /* reset mode */
- out_8(®s->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.