Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

On Wed, 7 Mar 2012, Russell King - ARM Linux wrote:

> On Wed, Mar 07, 2012 at 02:49:25PM +0100, Guennadi Liakhovetski wrote:
> > On Wed, 7 Mar 2012, Russell King - ARM Linux wrote:
> > > What
> > > would be useful is to have a helper function along these lines:
> > > 
> > > struct dma_chan *dma_request_channel_config(mask, fn, data, config)
> > > {
> > > 	struct dma_chan *c = dma_request_channel(mask, fn, data);
> > > 
> > > 	if (c) {
> > > 		if (dmaengine_slave_config(c, config)) {
> > > 			dma_release_channel(c);
> > > 			c = NULL;
> > > 		}
> > > 	}
> > > 	return c;
> > > }
> > 
> > Hm, yeah... That seems like an over-complication to me: to "just" allocae 
> > a channel you cann dma_request_channel(), which scans your devices and 
> > channels on them, calls your filter, calls the DMA controller driver's 
> > allocation method, only to eventually call dmaengine_slave_config() and 
> > see it fail, after which you release the channel and start anew...
> 
> The point is _not_ that this uses dmaengine_slave_config() to find a
> channel at all.  As I already said, there's nothing in dma_slave_config
> which _could_ be used to decide whether a channel is suitable or not.

I completely agree, that's why I wrote in my previous mail:

> > > However, I don't see anything in struct dma_slave_config which could be
> > > used to select an appropriate channel.
> > 
> > That's also my problem with it, and the reason, why I suggested, that it 
> > has to be embedded in a hardware-specific channel configuration type.

So, the question remains: which way should we go? If we don't come up with 
a generic solution, I'd be inclined to just do something as silly as

arch/arm/mach-shmobile/board-*.c

static const struct device *group1_dma_dev[] = {
	&dma0.device,
	&dma1.device,
};

static const struct device *group2_dma_dev[] = {
	&dma2.device,
	&dma3.device,
};

static struct my_platform_data = {
	.dma_dev_list = group1_dma_dev,
	.dma_dev_list_num = ARRAY_SIZE(group1_dma_dev),
};

drivers/.../sh_*.c

static bool filter(struct dma_chan *chan, void *arg)
{
	struct device *dev = chan->dev->device.parent;
	struct my_platform_data *pdata = arg;

	for (i = 0; i < pdata->dma_dev_list_num; i++)
		if (pdata->dma_dev_list[i] == dev)
			return true;

	return false;
}

even though I find it silly to have to do this on every platform and in 
every driver.

Thanks
Guennadi

> So to try to filter on the slave configuration is a pure red herring.
> None of the DMA controllers I have access to (whether they be working
> or not) could make any kind of decision about whether a particular
> channel is suitable for the peripheral by looking at the dma_slave_config
> structure - it's very much the case that the decision would be that
> 'any channel will do', which in reality it won't because there's other
> information required to make the decision about which virtual channel
> should be used.  Specifically, the request signal information.
> 
> Even more specifically, the request signal information may not be _just_
> the request signal on the DMA controller but also incorporate an
> external MUX like on the Realview boards (which pl08x handles itself.)
> 
> I do not see any milage in trying to select a channel based on "I want
> a DMA engine to access register X, width Y, burst size Z."  As far as
> I can see, the common situation is that there's nothing in that set of
> information which would be useful to chose a specific channel on a
> DMA engine.

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/