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

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

Hi Linus

On Fri, 30 Mar 2012, Linus Walleij wrote:

> On Fri, Mar 16, 2012 at 3:28 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > On Fri, 16 Mar 2012, Linus Walleij wrote:
> 
> >> Oh I was not thinking of relying on config to sort out channels.
> >>
> >> I was thinking of internalizing the dma_filter_fn and make it an
> >> (optional, maybe?) part of dmaengine.
> >
> > Yessss!!! Let's do that! :-D Now, you're proposing exactly the same, as
> > what I was proposing! :-)
> 
> No.

Well, ok, not _exactly_, but in essence. Maybe I didn't explain my 
proposal well enough, so, the analogy is not that clear. My main 
requirement was: we need a way for DMA slaves to pass information about 
the required DMA channel to the dmaengine framework already with the 
request_channel() call, because just requesting _a_ channel and then 
looking at it and either accepting or rejecting it is not good enough. My 
proposal was more generic - pass an opaque parameter to be matched 
against. You are proposing a very specific channel description, but see 
more below.

> > Now you just have to remove the filter function
> > parameter from dma_request_channel() - it is anyway the same for all and
> > implemented in the dmaengine core - and you get
> >
> > dma_request_channel(mask, slave_desc)
> 
> What is the point of mask on slave channels?
> I was more thinking introduce a new call:
> 
> dma_request_slave_channel(struct device *dev);
> 
> >From this the core looks up a suitable channel for that device.
> 
> However you're right (in some later mail) that we need to distinguish
> between RX/TX channels at this point, so I can agree we need some
> additional parameter, but that should be very abstract, not containing
> any custom stuff or any void * or something like that.
> 
> If the device and direction is really all we need to distinguish a suitable
> channel (which I imagine) the signature may very well be:
> 
> dma_request_slave_channel(struct device *dev, enum dma_transfer_direction dir);
> 
> But I'm not sure. (Keep beating me about it... but at this point
> I think code speaks more than words.)

Ok, I looked through a datasheet of one of SoCs, that we're working with 
and most devices require no more than two channels. But then I hit a weird 
example: the sh7372 SoC has 3 SDHI (SD-card) controller instances. SDHI1 
and SDHI2 each can use 2 DMA channels - Tx and Rx, this is also what is 
currently used. However, SDHI0 has 4 (!) DMA channels listed for it in the 
datasheet: 2 Rx and 2 Tx. I have no idea whether it is a bug in the 
documentation (not very likely, this is later confirmed at a different 
location), or whether those channels are completely identical (what would 
be the point then???), or whether we'll ever support that in software (ATM 
we only use two of them), but - a precedent is there... So, we can either 
pretend, that we don't know about it or decide, that we'll never use it 
and go ahead with just device/direction, or we can make it more 
future-proof immediately. But, well, this is a kernel internal API, so, we 
can change it at any time in the future.

Thanks
Guennadi

> > which is exactly what I was proposing! :-)
> 
> Sorry, not at all AFAICT.
> 
> Yours,
> Linus Walleij
> 

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