Re: [PATCH RFC 0/2] staging: Support Avalon-MM DMA Interface for PCIe

[Vinod Koul]

On 19-09-19, 13:37, Greg KH wrote:
> On Thu, Sep 19, 2019 at 11:59:11AM +0200, Alexander Gordeev wrote:
> > The Avalon-MM DMA Interface for PCIe is a design found in hard IPs for
> > Intel Arria, Cyclone or Stratix FPGAs. It transfers data between on-chip
> > memory and system memory. This RFC is an attempt to provide a generic API:
> > 
> > 	typedef void (*avalon_dma_xfer_callback)(void *dma_async_param);
> >  
> > 	int avalon_dma_submit_xfer(
> > 		struct avalon_dma *avalon_dma,
> > 		enum dma_data_direction direction,
> > 		dma_addr_t dev_addr, dma_addr_t host_addr,
> > 		unsigned int size,
> > 		avalon_dma_xfer_callback callback,
> > 		void *callback_param);
> >  
> > 	int avalon_dma_submit_xfer_sg(struct avalon_dma *avalon_dma,
> > 		enum dma_data_direction direction,
> > 		dma_addr_t dev_addr,
> > 		struct sg_table *sg_table,
> > 		avalon_dma_xfer_callback callback,
> > 		void *callback_param);
> >  
> > 	int avalon_dma_issue_pending(struct avalon_dma *avalon_dma);

Why wrap the *existing* kernel APIs with you own!

A quick glance at the code submitted tells me that it mimcks kernel
APIs. But why you folks didnt use the kernel dmaengine APIs in not clear
to me. So please convert it (should be relatively easy as you seem to
have wrappers for dmaengine callbacks)

> > 
> > Patch 1 introduces "avalon-dma" driver that provides the above-mentioned
> > generic interface.
> > 
> > Patch 2 adds "avalon-drv" driver using "avalon-dma" to transfer user-
> > provided data. This driver was used to debug and stress "avalon-dma"
> > and could be used as a code base for other implementations. Strictly
> > speaking, it does not need to be part of the kernel tree.
> > A companion tool using "avalon-drv" to DMA files (not part of this
> > patchset) is located at git@xxxxxxxxxx:a-gordeev/avalon-drv-tool.git

Heh! We do have a dmatest driver which does this and much more! why not
use that one instead of adding you own!

> > The suggested interface is developed with the standard "dmaengine"
> > in mind and could be reworked to suit it. I would appreciate, however
> > gathering some feedback on the implemenation first - as the hardware-
> > specific code would persist. It is also a call for testing - I only
> > have access to a single Arria 10 device to try on.

Why not use dmaengine in first place?

> > This series is against v5.3 and could be found at
> > git@xxxxxxxxxx:a-gordeev/linux.git avalon-dma-engine
> 
> Why is this being submitted for drivers/staging/ and not the "real" part
> of the kernel tree?
> 
> All staging code must have a TODO file listing what needs to be done in
> order to get it out of staging, and be self-contained (i.e. no files
> include/linux/)
> 
> Please fix that up when resending this series.

-- 
~Vinod