Re: [PATCH1/5] ASoC S3C24xx machine drivers - s3c2443-AC97

[Takashi Iwai <tiwai@suse.de>]

At Thu, 10 May 2007 14:00:13 +0100,
Liam Girdwood wrote:
> 
> Sorry for the lateness in the current merge window of this patch
> series.  

Well, I guess the patches will be unlikely merge to 2.6.22 at this
moment...  Anyway, I'll try to merge into ALSA tree.


> From: Graeme Gregory <gg@opensource.wolfsonmicro.com>
> 
> This patch adds AC97 support to the Samsung S3C2443 CPU. 
> 
> Signed-off-by: Graeme Gregory <gg@opensource.wolfsonmicro.com>
> Signed-off-by: Liam Girdwood <lg@opensource.wolfsonmicro.com>
> [2 s3c2443-ac97.patch <text/x-patch; UTF-8 (7bit)>]
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/soc/s3c24xx/s3c2443-ac97.c	Wed May 09 13:54:02 2007 +0100
> @@ -0,0 +1,405 @@
(snip)
> +static struct completion ac97_completion;

Use DECLARE_COMPLETION() rather than calling init_completion() at each
time.  Since this is accessed in the irq handler, it must be
initialized properly before anything happens.

> +	if (addr != reg)
> +		printk(KERN_ERR "s3c24xx-ac97: req addr = %02x, rep addr = %02x\n", reg, addr);

Try to keep within 80 chars.

> +static void s3c2443_ac97_warm_reset(struct snd_ac97 *ac97)
> +{
> +	u32 ac_glbctrl;
> +
> +	ac_glbctrl = readl(s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
> +	ac_glbctrl = S3C_AC97_GLBCTRL_WARMRESET;
> +	writel(ac_glbctrl, s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
> +	udelay(1000);

msleep() can be better here.

> +	ac_glbctrl = 0;
> +	writel(ac_glbctrl, s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);
> +	udelay(1000);

Ditto (also in s3c2443_ac97_cold_reset and s3c2443_ac97_probe()).

> +static irqreturn_t s3c2443_ac97_irq(int irq, void *dev_id)
> +{
> +	int status;
> +	u32 ac_glbctrl;
> +
> +	status = readl(s3c24xx_ac97.regs + S3C_AC97_GLBSTAT) & codec_ready;
> +
> +	if (status) {
> +	    ac_glbctrl = readl(s3c24xx_ac97.regs + S3C_AC97_GLBCTRL);

Keep the indentation correctly.


thanks,

Takashi