[alsa-devel] [PATCH V1 11/13] AC97 driver for mpc5200

[Mark Brown]

On Wed, May 13, 2009 at 09:59:22PM -0400, Jon Smirl wrote:

Looks mostly good but there's a few things that it'd be nice to address:

> +MODULE_AUTHOR("Jon Smirl <jonsmirl at gmail.com>");
> +MODULE_DESCRIPTION("mpc5200 AC97 module");
> +MODULE_LICENSE("GPL");

Normally this comes at the bottom of the file.

> +#define DRV_NAME "mpc5200-psc-ac97"
> +
> +static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg)
> +{
> +	struct psc_dma *psc_dma = ac97->private_data;
> +	int timeout;
> +	unsigned int val;
> +
> +	spin_lock(&psc_dma->lock);

Does this need to protect against interrupts?

> +static void psc_ac97_warm_reset(struct snd_ac97 *ac97)
> +{
> +	pr_info("psc_ac97_warm_reset\n");
> +}

Remove this - it's only going to confuse things to provide it if you
can't implement it, anything that actually needs the warm reset is going
to get upset anyway.

> +#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;
> +}

All the suspend and resume stuff can be removed if there's no
implementation.

> +static int psc_ac97_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int format)
> +{
> +	struct psc_dma *psc_dma = cpu_dai->private_data;
> +	dev_dbg(psc_dma->dev, "psc_ac97_set_fmt(cpu_dai=%p, format=%i)\n",
> +				cpu_dai, format);
> +
> +	return (format == SND_SOC_DAIFMT_AC97) ? 0 : -EINVAL;
> +}

Remove this, there's no need to provide a set_fmt operation for AC97
DAIs.