On Mon, May 28, 2012 at 10:59 PM, Mark Brown
[off-list ref] wrote:

On Fri, May 25, 2012 at 03:11:02PM +0800, Zhangfei Gao wrote:

quoted

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

Thanks Mark for kind review.

quoted

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

Will update.

quoted

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

will update.

quoted

+ ? ? 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().

Unfortunately, not find devm_clk_get.

quoted

+ ? ? 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().

will update.

quoted

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

Thanks confirmation, not sure before, also find devm_kfree is used.
Will remove the devm_kfree etc in err handle as well as remove function.

_______________________________________________
Alsa-devel mailing list
Alsa-devel at alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel