[alsa-devel] [PATCH 7/7] S3C PCM: Added the CPU driver for PCM controllers

[jassi brar]

On Sat, Nov 7, 2009 at 2:50 AM, Mark Brown
<broonie at opensource.wolfsonmicro.com> wrote:
> On Thu, Nov 05, 2009 at 10:35:17AM +0900, Jassi Brar wrote:
>> Signed-off-by: Jassi Brar <jassi.brar at 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