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@xxxxxxxxx>");
> +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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
