Re: [PATCH 1/7 v2] dmaengine: add a simple dma library

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

Hi Vinod

Thanks for your email.

On Mon, 30 Jan 2012, Vinod Koul wrote:

> On Thu, 2012-01-26 at 15:56 +0100, Guennadi Liakhovetski wrote:
> > This patch adds a library of functions, helping to implement dmaengine
> > drivers for hardware, unable to handle scatter-gather lists natively.
> > The first version of this driver only supports memcpy and slave DMA
> > operation.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> > v2:
> > 
> > 1. switch from using a tasklet to threaded IRQ, which allowed to
> > 2. remove lock / unlock inline functions
> > 3. remove __devinit, __devexit annotations
> Sorry to join the discussion late, was on vacation, travel, long
> weekend...
> 
> I don't still comprehend the need for a library on top of dmaengine
> which gain is just a library between clients and dmacs. Surely we don't
> want to write another abstraction on top of one provided?
> 
> If the question is to handle scatter-gather even if the hardware doesn't
> have the capability, then why don't add that in dmaengine itself rather
> than one more layer?

Well, yes, adding new abstraction layers is always a decision, that has to 
be well justified. In this case it does at least make the life easier for 
two sh-mobile drivers: shdma and the new SUDMAC driver.

However, I did name the library in a generic way without reference to sh, 
assuming, that it might with time become useful for other architectures 
too. The reasons why I prefered to keep it as an optional addition to 
dmaengine core, instead of tightly integrating it with it are, that (1) I 
did not want to add useless code to drivers, that do not need it, (2) I am 
not sure if and when this library will become useful for other drivers: 
apart from sh I am only familiar with one more dmaengine driver: 
ipu/ipu_idmac.c, and that one supports scatter-gather lists in a limited 
way and has some further peculiarities, that would likely make it a bad 
match for the simple DMA library, (3) keeping it separate makes its 
further development easier.

OTOH, I'm certainly fine with a tighter library integration with the 
dmaengine core. I think, it still would be better to keep it in a separate 
file and only build it if needed, right? This woult also simplify code 
debugging and further development. I can remove the "simple" notation, 
which does make it look like an additional abstraction layer, and replace 
it with, say, sgsoft (scatter-gather software implementation)? A more 
interesting question is what to do with struct dma_simple_dev, struct 
dma_simple_chan, struct dma_simple_desc, that embed struct dma_device, 
struct dma_chan and struct dma_async_tx_descriptor respectively. I don't 
think we want to merge all the additions from those wrapping structs back 
into their dmaengine counterparts?

How would you like to do this? Don't you think, it would be good to allow 
both: either implement a dmaengine driver directly, exactly as all drivers 
are doing now, or use the additional helper library for suitable (simple) 
hardware types? I see it similar to I2C, where you either implement an I2C 
driver directly, or you use the bitbanging abstraction for simpler 
hardware.

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