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

[Mark Brown]

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).