From: Mark Brown <broonie@opensource.wolfsonmicro.com> To: jassisinghbrar@gmail.com Cc: alsa-devel@alsa-project.org, linux-arm-kernel@lists.infradead.org, ben-linux@fluff.org, lrg@slimlogic.co.uk, linux-samsung-soc@vger.kernel.org, Jassi Brar <jassi.brar@samsung.com> Subject: Re: [PATCH 2/3] ASoC: AC97: S3C: Add controller driver Date: Tue, 26 Jan 2010 10:47:23 +0000 [thread overview] Message-ID: <20100126104723.GF15759@rakim.wolfsonmicro.main> (raw) In-Reply-To: <1264485101-13782-2-git-send-email-jassisinghbrar@gmail.com> On Tue, Jan 26, 2010 at 02:51:40PM +0900, jassisinghbrar@gmail.com wrote: This looks good overall, just a few smallish issues: > +static void s3c_ac97_activate(struct snd_ac97 *ac97) > +{ > + u32 ac_glbctrl, stat; > + > + stat = readl(s3c_ac97.regs + S3C_AC97_GLBSTAT) & 0x7; > + switch (stat) { > + case S3C_AC97_GLBSTAT_MAINSTATE_ACTIVE: > + return; > + case S3C_AC97_GLBSTAT_MAINSTATE_READY: > + case S3C_AC97_GLBSTAT_MAINSTATE_INIT: > + break; > + default: > + s3c_ac97_cold_reset(ac97); > + s3c_ac97_warm_reset(ac97); > + break; > + } This automatic cold and warm reset looks a bit fishy - obviously if this code path ever gets hit in normal operation then it's going to seriously disrupt things since it'll reset the CODEC registers. A warm reset by itself wouldn't be a problem but I'd rather see explicit cold resets in the callers where that's required. > + ac_glbctrl = readl(s3c_ac97.regs + S3C_AC97_GLBCTRL); > + ac_glbctrl = S3C_AC97_GLBCTRL_ACLINKON; > + writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL); > + msleep(1); This also looks a bit odd, ACLINKON sounds like bringing up the link which is what a warm reset does. > + INIT_COMPLETION(s3c_ac97.done); > + > + if (!wait_for_completion_timeout(&s3c_ac97.done, HZ)) > + printk(KERN_ERR "AC97: Unable to activate!"); This looks racy - the INIT_COMPLETION() happens after all the hardware configuration which suggests that if you're unlucky the event which should trigger the completion will have happened before the init. A bunch of interrupts arriving at an inconvenient time could trigger this, for example. The same idiom appears in the register reads and writes. > + if (addr != reg) > + printk(KERN_ERR "s3c-ac97: req addr = %02x," > + " rep addr = %02x\n", reg, addr); Please don't split error messages over multiple lines, it makes grepping for them harder. > +static irqreturn_t s3c_ac97_irq(int irq, void *dev_id) > +{ > + u32 ac_glbctrl, ac_glbstat; > + > + ac_glbstat = readl(s3c_ac97.regs + S3C_AC97_GLBSTAT); > + > + if (ac_glbstat & S3C_AC97_GLBSTAT_CODECREADY) { > + > + ac_glbctrl = readl(s3c_ac97.regs + S3C_AC97_GLBCTRL); > + ac_glbctrl &= ~S3C_AC97_GLBCTRL_CODECREADYIE; > + writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL); > + > + ac_glbctrl = readl(s3c_ac97.regs + S3C_AC97_GLBCTRL); > + ac_glbctrl |= (1<<30); /* Clear interrupt */ > + writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL); > + > + complete(&s3c_ac97.done); > + } > + > + return IRQ_HANDLED; > +} You should only be returning IRQ_HANDLED if you actually handled an interrupt here. > +#define S3C_AC97_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\ > + SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | \ > + SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | \ > + SNDRV_PCM_RATE_48000) SNDRV_PCM_RATE_8000_48000. > + .capture = { > + .stream_name = "AC97 Capture", > + /* NOTE: If the codec ouputs just one slot, > + * it *seems* our AC97 controller reads the only > + * valid slot(if either 3 or 4) for PCM-In. > + * For such cases, we record Mono. > + */ This seems like unsurprising behaviour for an AC97 controller - the slot validity information is there for just this purpose. I'd just remove the comment as a result. > + .capture = { > + .stream_name = "AC97 Mic Capture", > + .channels_min = 1, > + /* NOTE: If the codec(like WM9713) can't ouput just > + * one slot, it *seems* our AC97 controller reads > + * two slots(if one of them is Slot-6) for MIC also. > + * For such cases, we record Stereo. > + */ Similarly here. > + if (ac97_pdata->cfg_gpio(pdev)) { > + dev_err(&pdev->dev, "Unable to configure gpio\n"); > + ret = -EINVAL; > + goto lb3; > + } Should really check for the function being non-NULL here. > +lb5: > + free_irq(irq_res->start, NULL); Perhaps a better name than 'lb' - err, or fail or something. lb means nothing to me at least. > + irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + if (irq_res) > + free_irq(irq_res->start, NULL); This should never get called if the resources aren't allocated.
WARNING: multiple messages have this Message-ID (diff)
From: broonie@opensource.wolfsonmicro.com (Mark Brown) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/3] ASoC: AC97: S3C: Add controller driver Date: Tue, 26 Jan 2010 10:47:23 +0000 [thread overview] Message-ID: <20100126104723.GF15759@rakim.wolfsonmicro.main> (raw) In-Reply-To: <1264485101-13782-2-git-send-email-jassisinghbrar@gmail.com> On Tue, Jan 26, 2010 at 02:51:40PM +0900, jassisinghbrar at gmail.com wrote: This looks good overall, just a few smallish issues: > +static void s3c_ac97_activate(struct snd_ac97 *ac97) > +{ > + u32 ac_glbctrl, stat; > + > + stat = readl(s3c_ac97.regs + S3C_AC97_GLBSTAT) & 0x7; > + switch (stat) { > + case S3C_AC97_GLBSTAT_MAINSTATE_ACTIVE: > + return; > + case S3C_AC97_GLBSTAT_MAINSTATE_READY: > + case S3C_AC97_GLBSTAT_MAINSTATE_INIT: > + break; > + default: > + s3c_ac97_cold_reset(ac97); > + s3c_ac97_warm_reset(ac97); > + break; > + } This automatic cold and warm reset looks a bit fishy - obviously if this code path ever gets hit in normal operation then it's going to seriously disrupt things since it'll reset the CODEC registers. A warm reset by itself wouldn't be a problem but I'd rather see explicit cold resets in the callers where that's required. > + ac_glbctrl = readl(s3c_ac97.regs + S3C_AC97_GLBCTRL); > + ac_glbctrl = S3C_AC97_GLBCTRL_ACLINKON; > + writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL); > + msleep(1); This also looks a bit odd, ACLINKON sounds like bringing up the link which is what a warm reset does. > + INIT_COMPLETION(s3c_ac97.done); > + > + if (!wait_for_completion_timeout(&s3c_ac97.done, HZ)) > + printk(KERN_ERR "AC97: Unable to activate!"); This looks racy - the INIT_COMPLETION() happens after all the hardware configuration which suggests that if you're unlucky the event which should trigger the completion will have happened before the init. A bunch of interrupts arriving at an inconvenient time could trigger this, for example. The same idiom appears in the register reads and writes. > + if (addr != reg) > + printk(KERN_ERR "s3c-ac97: req addr = %02x," > + " rep addr = %02x\n", reg, addr); Please don't split error messages over multiple lines, it makes grepping for them harder. > +static irqreturn_t s3c_ac97_irq(int irq, void *dev_id) > +{ > + u32 ac_glbctrl, ac_glbstat; > + > + ac_glbstat = readl(s3c_ac97.regs + S3C_AC97_GLBSTAT); > + > + if (ac_glbstat & S3C_AC97_GLBSTAT_CODECREADY) { > + > + ac_glbctrl = readl(s3c_ac97.regs + S3C_AC97_GLBCTRL); > + ac_glbctrl &= ~S3C_AC97_GLBCTRL_CODECREADYIE; > + writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL); > + > + ac_glbctrl = readl(s3c_ac97.regs + S3C_AC97_GLBCTRL); > + ac_glbctrl |= (1<<30); /* Clear interrupt */ > + writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL); > + > + complete(&s3c_ac97.done); > + } > + > + return IRQ_HANDLED; > +} You should only be returning IRQ_HANDLED if you actually handled an interrupt here. > +#define S3C_AC97_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\ > + SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | \ > + SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | \ > + SNDRV_PCM_RATE_48000) SNDRV_PCM_RATE_8000_48000. > + .capture = { > + .stream_name = "AC97 Capture", > + /* NOTE: If the codec ouputs just one slot, > + * it *seems* our AC97 controller reads the only > + * valid slot(if either 3 or 4) for PCM-In. > + * For such cases, we record Mono. > + */ This seems like unsurprising behaviour for an AC97 controller - the slot validity information is there for just this purpose. I'd just remove the comment as a result. > + .capture = { > + .stream_name = "AC97 Mic Capture", > + .channels_min = 1, > + /* NOTE: If the codec(like WM9713) can't ouput just > + * one slot, it *seems* our AC97 controller reads > + * two slots(if one of them is Slot-6) for MIC also. > + * For such cases, we record Stereo. > + */ Similarly here. > + if (ac97_pdata->cfg_gpio(pdev)) { > + dev_err(&pdev->dev, "Unable to configure gpio\n"); > + ret = -EINVAL; > + goto lb3; > + } Should really check for the function being non-NULL here. > +lb5: > + free_irq(irq_res->start, NULL); Perhaps a better name than 'lb' - err, or fail or something. lb means nothing to me at least. > + irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + if (irq_res) > + free_irq(irq_res->start, NULL); This should never get called if the resources aren't allocated.
next prev parent reply other threads:[~2010-01-26 10:47 UTC|newest] Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-01-26 5:51 [PATCH 1/3] ARM: S3C64XX: Add AC97 platform resources jassisinghbrar 2010-01-26 5:51 ` jassisinghbrar at gmail.com 2010-01-26 5:51 ` [PATCH 2/3] ASoC: AC97: S3C: Add controller driver jassisinghbrar 2010-01-26 5:51 ` jassisinghbrar at gmail.com 2010-01-26 5:51 ` [PATCH 3/3] ARM: SMDK64XX: Enable AC97 device jassisinghbrar 2010-01-26 5:51 ` jassisinghbrar at gmail.com 2010-01-26 10:19 ` Mark Brown 2010-01-26 10:19 ` Mark Brown 2010-01-26 10:52 ` jassi brar 2010-01-26 10:52 ` jassi brar 2010-01-26 10:59 ` Mark Brown 2010-01-26 10:59 ` Mark Brown 2010-01-26 7:32 ` [PATCH 2/3] ASoC: AC97: S3C: Add controller driver jassi brar 2010-01-26 7:32 ` jassi brar 2010-01-26 10:53 ` Mark Brown 2010-01-26 10:53 ` Mark Brown 2010-01-26 11:49 ` jassi brar 2010-01-26 11:49 ` jassi brar 2010-01-26 10:23 ` Ben Dooks 2010-01-26 10:23 ` Ben Dooks 2010-01-26 11:03 ` jassi brar 2010-01-26 11:03 ` jassi brar 2010-01-26 11:09 ` Mark Brown 2010-01-26 11:09 ` Mark Brown 2010-01-26 11:57 ` jassi brar 2010-01-26 11:57 ` jassi brar 2010-01-26 12:04 ` Mark Brown 2010-01-26 12:04 ` Mark Brown 2010-01-27 2:45 ` jassi brar 2010-01-27 2:45 ` jassi brar 2010-01-27 10:25 ` Mark Brown 2010-01-27 10:25 ` Mark Brown 2010-01-26 10:47 ` Mark Brown [this message] 2010-01-26 10:47 ` Mark Brown 2010-01-26 11:17 ` jassi brar 2010-01-26 11:17 ` jassi brar 2010-01-26 11:52 ` Mark Brown 2010-01-26 11:52 ` Mark Brown 2010-01-26 12:11 ` jassi brar 2010-01-26 12:11 ` jassi brar 2010-01-26 13:00 ` Mark Brown 2010-01-26 13:00 ` Mark Brown 2010-01-26 10:12 ` [PATCH 1/3] ARM: S3C64XX: Add AC97 platform resources Mark Brown 2010-01-26 10:12 ` Mark Brown
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20100126104723.GF15759@rakim.wolfsonmicro.main \ --to=broonie@opensource.wolfsonmicro.com \ --cc=alsa-devel@alsa-project.org \ --cc=ben-linux@fluff.org \ --cc=jassi.brar@samsung.com \ --cc=jassisinghbrar@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-samsung-soc@vger.kernel.org \ --cc=lrg@slimlogic.co.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.