Re: [PATCH V5 2/3] dmaengine: tegra-adma: Add support for Tegra210 ADMA

[Vinod Koul <vinod.koul-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Thu, May 12, 2016 at 10:02:07AM +0100, Jon Hunter wrote:
> >> +	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

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