On Saturday 17 March 2012, Grant Likely wrote: > > +static LIST_HEAD(of_dma_list); > > + > > +struct of_dma { > > + struct list_head of_dma_controllers; > > + struct device_node *of_node; > > + int of_dma_n_cells; > > + int (*of_dma_xlate)(struct of_phandle_args *dma_spec, void *data); > > +}; > > This _xlate is nearly useless as a generic API. It solves the problem for > the specific case where the driver is hard-coded to know which DMA engine > to talk to, but since the returned data doesn't provide any context, it > isn't useful if there are multiple DMA controllers to choose from. > > The void *data pointer must be replaced with a typed structure so that > context can be returned. I've read up a bit more on how the existing drivers use the filter functions, it seems there are multiple classes of them, the classes that I've encountered are: 1. matches on chan->device pointer and/or chan_id (8 drivers) 2. will match anything (6 drivers) 3. requires specific dma engine driver, then behaves like 1 or 2 (8 drivers, almost all freescale) 4. one of a kind, matches resource name string or device->dev_id (two drivers) 5. filter function and data both provided by platform code, platform picks dmaengine driver. (4 amba pl* drivers, used on ARM, ux500, ...) The last category is interesting because here, the dmaengine driver (pl330, coh901318, sirf-dma, ste_dma40) provides the filter function while in the other cases that is provided by the device driver! Out of these, the ste_dma40 is special because it's the only one where the data is a complex data structure describing the constraints on the driver, while all others just find the right channel. Some drivers also pass assign driver specific data to chan->private. I would hope that we can all make them use something like struct dma_channel *of_dma_request_channel(struct of_node*, int index, void *driver_data); with an appropriate common definition behind it. In the cases where the driver can just match anything, I'd assume that all channels are equal, so #dma-cells would be 0. For the ste_dma40, #dma-cells needs to cover all of stedma40_chan_cfg. In most other cases, #dma-cells can be 1 and just enumerate the channels, unless we want to simplify the cases that Russell mentioned where we want to keep a two stage mapping channel identifiers and physical channel numbers. How about an implementation like this:? typedef bool dma_filter_simple(struct dma_chan *chan, void *filter_param) { /* zero #dma-cells, accept anything */ return true; } struct dma_channel *of_dma_request_channel(struct of_node*, int index, dma_cap_mask_t *mask, void *driver_data) { struct of_phandle_args dma_spec; struct dma_device *device; struct dma_chan *chan = NULL; dma_filter_fn *filter; ret = of_parse_phandle_with_args(np, "dma-request", "#dma-cells", index, &dma_spec); device = dma_find_device(dma_spec->np); if (!device) goto out; if (dma_spec->args_count == 0) filter = dma_filter_simple; else filter = device->dma_dt_filter; /* new member */ chan = private_candidate(mask, device, filter, dma_spec->args); if (chan && !chan->private) chan->private = driver_data; out: return chan; } Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html