On Thu, Nov 05, 2009 at 10:35:17AM +0900, Jassi Brar wrote:
> Signed-off-by: Jassi Brar <jassi.brar@xxxxxxxxxxx>
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.
> + 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.
> + 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.
> +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?
> + /* 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
