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

[Mark Brown]

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.

> +	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.