On Thu, May 12, 2016 at 10:02:07AM +0100, Jon Hunter wrote:

quoted

quoted

+	ret = request_irq(tdc->irq, tegra_adma_isr, 0, dma_chan_name(dc), tdc);
+	if (ret) {
+		dev_err(tdc2dev(tdc), "failed to get interrupt for %s\n",
+			dma_chan_name(dc));
+		return ret;
+	}

Okay why are we requesting and freeing up irq while allocating channels..?
Btw since he usage is cyclic the sound core grabs the channel at start and
never releases it

The interrupt controller used by this DMA is a 2nd level interrupt
controller in a different power-domain. I am currently in the process of
adding support for this interrupt controller and it requires runtime
power-management and in discussions with tglx the only way to allow the
interrupt controller to idle is by releasing the interrupt. Hence, here
we will acquire and release the interrupts as they are used. Please note
that probing the DMA controller will be deferred until the interrupt
controller is present because we still map the interrupt in the probe.

Okay, lets keep it

quoted

quoted

+	dma_cap_set(DMA_SLAVE, tdma->dma_dev.cap_mask);
+	dma_cap_set(DMA_PRIVATE, tdma->dma_dev.cap_mask);
+	dma_cap_set(DMA_CYCLIC, tdma->dma_dev.cap_mask);

You should set only cyclic, btw why do you support only one. Cyclic is
actually a special case for SLAVE, so ideally you should support slave
too...

Only CYCLIC is supported for now because this DMA is only used for
audio. There is another DMA for Tegra210 (the APB-DMA) which is a more
general purpose DMA and should be used for all other DMA activity.
Hence, I am not sure if it is best to keep the PRIVATE flag as I assume
that means that not anyone can request a channel. We only should use
this DMA for audio.

We discussed the setting of the DMA_SLAVE flag before [0] and I need to
set DMA_SLAVE because I am using dma_get_any_slave_channel() in the
xlate. May be you forgot? I know you would prefer we don't, but as I
explained in this case (and as for other DMAs) this is the most
appropriate API to use. We agreed in that discussion with Arnd Bergmann
that this is ok.

I will drop the tegra_adma_prep_slave_sg() function. Please let me know
if you are ok with the other items given my explanation and I will send
a V6. I would like to get this merged for v4.7 if possible.

Sure, looks fine to me

-- 
~Vinod