On Sun, May 16, 2010 at 11:34:01PM +0800, Wan ZongShun wrote:
This all looks pretty good. A few comments below but they're all fairly
minor and should be easy to address.
> index b1749bc..81d4848 100644
> --- a/sound/soc/Kconfig
> +++ b/sound/soc/Kconfig
> @@ -36,6 +36,7 @@ source "sound/soc/s3c24xx/Kconfig"
> source "sound/soc/s6000/Kconfig"
> source "sound/soc/sh/Kconfig"
> source "sound/soc/txx9/Kconfig"
> +source "sound/soc/nuc900/Kconfig"
Please keep the Kconfig and Makefile sorted, this helps avoid merge
issues.
> +#define NUC900_AC97_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\
> + SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
> + SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000)
SNDRV_PCM_RATE_8000_48000.
> + if (!timeout)
> + dev_err(nuc900_audio->dev, "AC97 read register time out !\n");
> +
> + val = AUDIO_READ(nuc900_audio->mmio + ACTL_ACOS0) ;
If the read timed out shouldn't we be returning rather than continuing?
> + val = AUDIO_READ(nuc900_audio->mmio + ACTL_ACCON);
> + val &= (~AC_C_RES);
> + AUDIO_WRITE(nuc900_audio->mmio + ACTL_ACCON, val);
> + udelay(1000);
> + if (!(AUDIO_READ(nuc900_audio->mmio + ACTL_ACIS0) & CODEC_READY))
> + dev_err(nuc900_audio->dev, "AC97 codec cold reset failed!\n");
What is this actually checking in the hardware? Not all CODECs enable
the AC97 link by default after a cold reset, the standard allows them to
power up in a low power state which will
On the other hand, given that there's no warm reset operation perhaps
this isn't a big deal.
> +static void nuc900_ac97_remove(struct platform_device *pdev,
> + struct snd_soc_dai *dai)
> +{
> + struct nuc900_audio *nuc900_audio = nuc900_ac97_data;
> +
> + /* Enable unit clock */
> + clk_disable(nuc900_audio->clk);
Bit rot in the comments here.
> +/* bit definition of REG_ACTL_CON register */
> +#define AUDCLK_EN 0x8000
> +#define PFIFO_EN 0x4000
> +#define RFIFO_EN 0x2000
These constants (and the rest) really should be namespaced - they're
likely to collide with other definitions in, for example, CODEC drivers
used by machines.
> +#define IIS_EN 0x0002
Looks like there's I2S support to come?
> +static irqreturn_t nuc900_dma_interrupt(int irq, void *dev_id)
> +{
> +
> + snd_pcm_period_elapsed(substream);
> +
> + return IRQ_HANDLED;
This is done unconditionally - are you sure there can't be any spurious
interrupts (eg, error interrupts). It shouldn't cause any harm, of
course.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
