Re: [PATCH v3 2/3] ALSA SoC: Add mpc5200-psc I2S driver

[Grant Likely <grant.likely@secretlab.ca>]

On Tue, Jul 22, 2008 at 11:09:52AM +0100, Mark Brown wrote:
> On Tue, Jul 22, 2008 at 12:53:58AM -0600, Grant Likely wrote:
> 
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> 
> There's a few issues that were raised on the previous review cycle that
> still need to be addressed but they should be fixable in incremental
> patches (and easier to review that way):
> 
> > +static int psc_i2s_trigger(struct snd_pcm_substream *substream, int cmd)
> 
> > +		spin_lock_irqsave(&psc_i2s->lock, flags);
> > +		/* first make sure it is low */
> > +		while ((in_8(&regs->ipcr_acr.ipcr) & 0x80) != 0)
> > +			;
> > +		/* then wait for the transition to high */
> > +		while ((in_8(&regs->ipcr_acr.ipcr) & 0x80) == 0)
> > +			;
> 
> These loops should really have some sort of time limit on them,
> otherwise they'll lock hard if the expected events don't happen.  Given
> that in slave mode they're synchronising with an externally generated
> clock this is something that might happen in practice and should produce
> better diagnostics.

Yes, I hope to rework these two lines entirely.  I'm not happy with the
current implementation either.

> > +	default:
> > +		dev_dbg(psc_i2s->dev, "invalid command\n");
> > +		return -EINVAL;
> > +	}
> 
> I'd really expect to see the other possible triggers handled, even if
> the appropriate action is to silently ignore them, rather than having
> them generate an error message.

Okay, I'll do that.

g.