On Sat, Nov 7, 2009 at 2:50 AM, Mark Brown
<broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> 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.
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
