Re: [PATCH] dma: tegra: Use runtime_pm for clk enable/disable

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 12/11/2013 01:49 AM, Chaitanya Bandi wrote:
> Used runtime_pm APIs for clock enabling/disabling.
> Made changes such that clock is not enabled during
> idle. Also moved the usage of clk prepare/unprepare
> such that they are not called in isr context.

Hmm. This is going to cause conflicts with the patch I'm taking through
the Tegra tree which converts the driver to support the standard DMA DT
bindings. Perhaps this can wait until 3.15, or perhaps we can merge the
Tegra branch back into the DMA tree to resolve the conflict.

> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c

> - * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + * Copyright (c) 2012-13, NVIDIA CORPORATION.  All rights reserved.

s/13/2013/

> @@ -580,6 +580,11 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
>  	list_add_tail(&sgreq->node, &tdc->free_sg_req);
>  
>  	/* Do not start DMA if it is going to be terminate */
> +	if (list_empty(&tdc->pending_sg_req) && (!to_terminate)) {
> +		clk_disable(tdc->tdma->dma_clk);
> +		pm_runtime_put(tdc->tdma->dev);
> +	}
> +
>  	if (to_terminate || list_empty(&tdc->pending_sg_req))
>  		return;

Don't you want to insert the new code before the comment? Otherwise,
you're separating the existing comment and code.

Here and many other places, both pm_runtime_get/put *and*
clk_enable/disable are called. Why doesn't the code *just* call
pm_runtime_get/put, and let the implementation of those APIs perform the
clock_enable/disable? That'd be a lot more typical.

Most of the new calls to pm_runtime_*()/clk_{en,dis}able() are missing
error checking.

> @@ -682,12 +687,21 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)

> +	pm_runtime_get(tdc->tdma->dev);

I think you need pm_runtime_get_sync() here to make sure the clock gets
turned on immediately? Perhaps that why...

> +	ret = clk_enable(tdc->tdma->dma_clk);

... there's also direct manipulation of the clock everywhere.

Also, shouldn't the DMA core be calling pm_runtime_get(), rather than
each individual DMA driver?