Re: [PATCH 4/5] dmaengine: sprd: Add Spreadtrum DMA configuration

[Vinod Koul <vinod.koul@intel.com>]

On Tue, Apr 10, 2018 at 03:46:06PM +0800, Baolin Wang wrote:

> +/*
> + * struct sprd_dma_config - DMA configuration structure
> + * @config: dma slave channel config
> + * @fragment_len: specify one fragment transfer length
> + * @block_len: specify one block transfer length
> + * @transcation_len: specify one transcation transfer length
> + * @wrap_ptr: wrap pointer address, once the transfer address reaches the
> + * 'wrap_ptr', the next transfer address will jump to the 'wrap_to' address.
> + * @wrap_to: wrap jump to address
> + * @req_mode: specify the DMA request mode
> + * @int_mode: specify the DMA interrupt type
> + */
> +struct sprd_dma_config {
> +	struct dma_slave_config config;
> +	u32 fragment_len;

why not use _maxburst?

> +	u32 block_len;
> +	u32 transcation_len;

what does block and transaction len refer to here

> +	phys_addr_t wrap_ptr;
> +	phys_addr_t wrap_to;

this sound sg_list to me, why are we not using that here

> +	enum sprd_dma_req_mode req_mode;

Looking at definition of request mode we have frag, block, transaction list
etc.. That should depend upon dma request. If you have been asked to
transfer a list, you shall configure list mode. if it is a single
transaction then it should be transaction mode!

> +	enum sprd_dma_int_type int_mode;

Here again I think driver needs to take a call based on dma_ctrl_flags.

Okay I dont think we are proceeding in right direction on this one. This
seems to be fairly generic dma controller and in line with other IP blocks
and you should take reference from those one. I dont think we need this
configuration and can do with generic api and configuration provided.

Thanks
-- 
~Vinod