On Fri, May 25, 2012 at 03:11:02PM +0800, Zhangfei Gao wrote:
The SSPA is a configurable multi-channel audio serial (TDM) interface. It's configurable at runtime to support up to 128 channels and the number of bits per sample: 8, 12, 16, 20, 24 and 32 bits. It also support stereo format: I2S, left-justified or right-justified.
Mostly looks good. A few fairly minor things...
+static int mmp_sspa_startup(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
+{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
- struct sspa_priv *sspa_priv = snd_soc_dai_get_drvdata(dai);
- struct ssp_device *sspa = sspa_priv->sspa;
- int ret = 0;
- if (!cpu_dai->active) {
clk_enable(sysclk);clk_enable(sspa->clk);- }
The clock API is refcounted so you shouldn't need to worry about multiple enables, you should just be able to unconditionally enable and disable. If this is needed we probably have a problem we should fix.
- switch (pll_id) {
- case MMP_SYSCLK:
clk_set_rate(sysclk, freq_out);break;- case MMP_SSPA_CLK:
clk_set_rate(sspa->clk, freq_out);break;
You're ignoring the return values here.
- priv->sspa->clk = clk_get(&pdev->dev, NULL);
- if (IS_ERR(priv->sspa->clk)) {
ret = PTR_ERR(priv->sspa->clk);goto err_free_clk;- }
devm_clk_get().
- res = request_mem_region(res->start, resource_size(res),
pdev->name);- if (res == NULL) {
dev_err(&pdev->dev, "failed to request memory resource\n");ret = -EBUSY;goto err_get_res;- }
- priv->sspa->mmio_base = ioremap(res->start, resource_size(res));
- if (priv->sspa->mmio_base == NULL) {
dev_err(&pdev->dev, "failed to ioremap() registers\n");ret = -ENODEV;goto err_free_mem;- }
devm_request_and_ioremap().
+err_free_clk:
- devm_kfree(&pdev->dev, priv->dma_params);
+err_priv_dma_params:
- devm_kfree(&pdev->dev, priv->sspa);
+err_priv_sspa:
- devm_kfree(&pdev->dev, priv);
The whole point of the devm_ stuff is that you don't need to do stuff like this.