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

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Wed, Mar 07, 2012 at 01:30:23PM +0100, Guennadi Liakhovetski wrote:
> 1. The current scheme is:
> 
> (a) client issues
> 	dma_request_channel()
>     with an optional filter function as parameter
> (b) the core picks up a suitable from its PoV DMA controller device and a 
>     channel on it and calls the filter function with that channel as an 
>     argument
> (c) the filter function can verify, whether that channel is suitable or 
>     not (*)
> (d) the client driver then can call
> 	dmaengine_slave_config()
>     to provide any additional channel configuration information to the DMA 
>     controller driver (**)
> (e) if the filter has rejected this channel, the core jumps to the next 
>     DMA controller instance (***)

No - if the filter function rejects the first free channel, the next free
channel on the same controller will be tried.  When all channels have
been tried, the next DMA controller is checked.

> 2. (goal: eliminate filter function look-ups) proposed by Linus W
> 
> (a) client issues
> 	dma_request_slave_channel(dev, "MMC-RX")
> (b) the dmaengine core scans a platform-provided list of channel mappings 
>     and picks up _the_ correct channel (****)

That doesn't work if you have multiple DMA controllers supporting the
same client.

> 3. Jassi's idea with capabilities has been rejected by Russell
>
> 4. (goal: simplify the allocation and configuration procedure) proposed by 
>    myself
> 
> (a) as in (1) client issues
> 	dma_request_channel()
>     with an additional slave configuration parameter
> (b) the core picks up a suitable from its PoV DMA controller device and a 
>     channel on it, (optionally) calls the filter

How can it work out what's a suitable DMA controller device?  Even knowing
where the DMA register is, the burst size and width doesn't really narrow
down the selection of the DMA controller.

> (c) the core calls DMA controller driver's
> 	.device_alloc_chan_resources()
>     method, which verifies, whether the channel can be configured for the 
>     requesting slave, if not, an error is returned and the next DMA 
>     controller instance is checked by the core

And this effectively prevents a channel being reconfigured to target a
different burst size or different transfer width without freeing and
re-requesting it.

> Naturally, my preference goes for (4) because (a) I think, it is the DMA 
> controller driver, that has to decide, whether the channel is suitable for 
> a specific slave,

We already effectively do that with many of the DMA engine drivers.  The
DMA engine drivers export their filter function which should be used when
requesting a channel (if you care about the channel you end up with.)

> (b) changes to the core are minimal, simple and 
> trivially backwards-compatible, (c) the core is not cluttered with 
> hw-specific channel mappings, (d) the additional call to 
> dmaengine_slave_config() can be eliminated.

The call to dmaengine_slave_config() actually simplifies the DMA engine
support for some drivers though, so eliminating it doesn't help.  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;
}

which would simplify some of the DMA engine users.  There'll still be
some though which would want to call dmaengine_slave_config() to change
the channels configuration when the mode of the device switches.

However, I don't see anything in struct dma_slave_config which could be
used to select an appropriate channel.