On Sat, Nov 7, 2009 at 2:50 AM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Thu, Nov 05, 2009 at 10:35:17AM +0900, Jassi Brar wrote:
Signed-off-by: Jassi Brar jassi.brar@samsung.com
This looks basically good. A few comments but they're pretty much nitpicks:
- clk = (clkctl & S3C_PCM_CLKCTL_SERCLKSEL_PCLK) ?
- pcm->pclk : pcm->cclk;
This would be clearer as an if statement - the ternery operator isn't massively legible at the best of times, especially when statements end up spanning multiple lines.
okay.
- case SND_SOC_DAIFMT_CBS_CFS:
- /* Nothing to do, Master by default */
- break;
- default:
- spin_unlock_irq(&pcm->lock);
- dev_err(pcm->dev, "Unsupported master/slave format!\n");
- return -EINVAL;
- }
A goto to handle the lock unwinding might be appropriate.
Yes, that wud be better.
- case S3C_PCM_CLKSRC_MUX:
- clkctl &= ~S3C_PCM_CLKCTL_SERCLKSEL_PCLK;
- if (clk_get_rate(pcm->cclk) != freq)
- clk_set_rate(pcm->cclk, freq);
May as well just clk_set_rate() unconditionally, it'll do no harm to do a null change.
I believe the clock sources should be touched only when we can't do without it. clk_get_rate doesn't touch any register, but clk_set_rate does even if overwrite the same value.
+struct clk *s3c_pcm_get_clock(struct snd_soc_dai *cpu_dai) +{
- struct s3c_pcm_info *pcm = to_info(cpu_dai);
- void __iomem *regs = pcm->regs;
- u32 clkctl = readl(regs + S3C_PCM_CLKCTL);
- if (clkctl & S3C_PCM_CLKCTL_SERCLKSEL_PCLK)
- return pcm->pclk;
- else
- return pcm->cclk;
+} +EXPORT_SYMBOL_GPL(s3c_pcm_get_clock);
How will this be used?
Hmmm, can't think of it's role with this revised driver.
- /* Check for valid device index */
- if (pdev->id >= ARRAY_SIZE(s3c_pcm)) {
- dev_err(&pdev->dev, "id %d out of range\n", pdev->id);
- return -EINVAL;
- }
id could be less than zero too.
okay. Though i wonder how did it pass review in s3c64xx-i2s.c