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