Re: [PATCH v3 2/3] ALSA SoC: Add mpc5200-psc I2S driver
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 <[EMAIL PROTECTED]> > > Signed-off-by: Mark Brown <[EMAIL PROTECTED]> > > 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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v3 2/3] ALSA SoC: Add mpc5200-psc I2S driver
What about the mpc5200b?
+/* Match table for of_platform binding */
+static struct of_device_id psc_i2s_match[] __devinitdata = {
+ { .compatible = "fsl,mpc5200-psc-i2s", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, psc_i2s_match);
+
I'm just being grumpy because updating to linus/master made me fix
over a hundred merge conflicts, now I have to test everything again.
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v3 2/3] ALSA SoC: Add mpc5200-psc I2S driver
On Tue, Jul 22, 2008 at 12:53:58AM -0600, Grant Likely wrote: > Signed-off-by: Grant Likely <[EMAIL PROTECTED]> Signed-off-by: Mark Brown <[EMAIL PROTECTED]> 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. > + 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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
