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@xxxxxxxxxxxx> > > Signed-off-by: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> > > 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(®s->ipcr_acr.ipcr) & 0x80) != 0) > > + ; > > + /* then wait for the transition to high */ > > + while ((in_8(®s->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. _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
