Re: [alsa-devel] [PATCH V2 7/9] AC97 driver for mpc5200

On Sat, May 23, 2009 at 07:13:09PM -0400, Jon Smirl wrote:

+#ifdef CONFIG_PM +static int psc_ac97_suspend(struct snd_soc_dai *dai) +{

• return 0;

+}

+static int psc_ac97_resume(struct snd_soc_dai *dai) +{

• return 0;

+}

+#else +#define psc_ac97_suspend NULL +#define psc_ac97_resume NULL +#endif

These can all just be removed, they can be added later if they are implemented.

+static int psc_ac97_hw_analog_params(struct snd_pcm_substream *substream,

• 		 struct snd_pcm_hw_params *params,
  

• 		 struct snd_soc_dai *dai)
  

+{

• struct snd_soc_pcm_runtime *rtd = substream->private_data;
• struct psc_dma *psc_dma = rtd->dai->cpu_dai->private_data;

Note that cpu_dai is passed in as an argument here - this didn't used to be the case which is why most drivers fish it out of rtd but that's not required any more.

+static int psc_ac97_trigger(struct snd_pcm_substream *substream, int cmd,

• 						 struct snd_soc_dai *dai)
  

Again with the odd indentation.

+static int __devinit psc_ac97_of_probe(struct of_device *op,

• 		      const struct of_device_id *match)
  

+{

• rc = snd_soc_register_dais(psc_ac97_dai, ARRAY_SIZE(psc_ac97_dai));
• if (rc != 0) {

• pr_err("Failed to register DAI\n");
  

• return 0;
  

• }

I'd expect to see the error passed back to the caller here? I'd also expect to see this happening right at the end of this function after you're happy everything else has worked, otherwise the core might try to start using the half-initialised driver.

• /* AC97 clock is generated by the codec.

• * Ensure that it starts ticking after codec reset.
  

• */
  

• max_reset = 0;

+reset:

• if (max_reset++ > 5) {

• dev_err(&op->dev, "AC97 codec failed to reset\n");
  

• mpc5200_audio_dma_destroy(op);
  

• return -ENODEV;
  

• }
• psc_ac97_cold_reset(&ac97);
• psc_ac97_warm_reset(&ac97);
• /* first make sure it is low */
• timeout = 0;
• while ((in_8(&regs->ipcr_acr.ipcr) & 0x80) != 0) {

• udelay(1);
  

• if (timeout++ > 1000)
  

• 	goto reset;
  

• }

This looks awfully similar to the logic in the resume function of your CODEC driver... In general an ASoC AC97 DAI driver shouldn't be trying to bring the CODEC up like this, it should normally leave that up to the CODEC driver. This is mostly because some system designers do strange and wonderful things with clocking which require some system specific code to either bring up the master clock for the codec before it'll reset or reconfigure the clocking to something standard.

The issue with the use of goto rather than a real loop also applies here.

• /* then wait for the transition to high */
• timeout = 0;
• while ((in_8(&regs->ipcr_acr.ipcr) & 0x80) == 0) {

• udelay(1);
  

• if (timeout++ > 1000)
  

• 	psc_ac97_warm_reset(&ac97);
  

• }

Shouldn't this be integrated into the warm reset operation (or to put it another way, why wouldn't a warm reset want to do this)?

• /* Go */
• out_8(&regs->command, MPC52xx_PSC_TX_ENABLE | MPC52xx_PSC_RX_ENABLE);
• id1 = psc_ac97_read(&ac97, AC97_VENDOR_ID1);
• id2 = psc_ac97_read(&ac97, AC97_VENDOR_ID2);
• dev_info(&op->dev, "Codec ID is %04x %04x\n", id1, id2);

You really can't rely on this working in general system designs - like I say, some platforms will require additional work to bring the CODEC up. I'd just remove it, it's purely for information anyway. The out_8() probably ought to be moved into the ASoC probe() function (called once the card is coming up).