On Mon, 19 Mar 2012 14:30:05 +0100, Nicolas Ferre <nicolas.ferre@xxxxxxxxx> wrote:
> On 03/17/2012 10:40 AM, Grant Likely :
> > On Thu, 15 Mar 2012 09:38:10 +0100, Nicolas Ferre <nicolas.ferre@xxxxxxxxx> wrote:
> >> +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.
>
> You mean, if there is no DMA controller phandle specified in the
> property?
No; I'm assuming that dma-channel properties will alwasy have a
phandle to the dma controller node.
> I think that it is not the purpose of this API to choose a DMA
> controller, Nor to provide a channel. The only purpose of this API is to
> give a HW request to be used by a DMA slave driver. This slave should
> already have a channel to use and a controller to talk to.
Then where is the function that finds the reference to the DMA
controller? I don't understand why it would be useful to decode that
separately.
> > The void *data pointer must be replaced with a typed structure so that
> > context can be returned.
>
> I am not sure to follow you entirely... How do I address the fact that
> several types of request value can be returned then?
>
> BTW, can we imagine a phandle property with a sting as a argument?
> should it be written this way?
> dma-request = <&testdmac1>, "slave-rx", "slave-tx";
No, I'm not suggesting that. Mixing phandles and strings in a single
property is possible but ugly. The phandle-args pattern which uses
zero or more cells as arguments should be used.
>
> If yes, the of_parse_phandle_with_args() is not working on this type...
Right; of_parse_phandle_with_args() should do the job.
>
> (I realize that there seems to be no way out for a generic API: maybe we
> should move to one or two cases to address and concentrate on them).
The way I read this patch, the xlate function returns a single
integer representing the DMA request number, but it doesn't provide
any data about *which* dma controller is associated with that channel.
The result of xlate needs to be something like a dma_chan reference
that identifies both the DMA engine and the channel/request on that
dma engine.
[...]
> >> +/**
> >> + * of_dma_xlate_onenumbercell() - Generic DMA xlate for direct one cell bindings
> >> + *
> >> + * Device Tree DMA translation function which works with one cell bindings
> >> + * where the cell values map directly to the hardware request number understood
> >> + * by the DMA controller.
> >> + */
> >> +int of_dma_xlate_onenumbercell(struct of_phandle_args *dma_spec, void *dma_req)
> >> +{
> >> + if (!dma_spec)
> >> + return -EINVAL;
> >> + if (WARN_ON(dma_spec->args_count != 1))
> >> + return -EINVAL;
> >> + *(int *)dma_req = dma_spec->args[0];
> >
> > Following on from comment above; the void *dma_req parameter is dangerous. How
> > does this function know that it has been passed an int* pointer?
>
> Well, that is a drawback that comes from having to address generic
> cases.
Not if you do it right. If a specific data structure is returned,
then there can be context attached as to what the data means and which
dma controller knows how to parse it.
> But anyway, if the DMA controller decide to register a .xlate()
> function that returns an integer, the slave driver that will ask a
> "request line" to this DMA controller will be aware that an integer has
> to be passed to of_get_dma_request().
The problem still remains; I don't see how the dma slave can figure
out *which* dma controller it needs to talk to.
g.
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies,Ltd.
--
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
