On Sun, Jan 26, 2014 at 06:39:21PM +0100, Lars-Peter Clausen wrote:

On 01/26/2014 02:59 PM, Vinod Koul wrote:

quoted

On Fri, Jan 24, 2014 at 02:24:27PM +0100, Lars-Peter Clausen wrote:

quoted

On 01/24/2014 12:16 PM, Srikanth Thokala wrote:

quoted

Hi Lars,

On Thu, Jan 23, 2014 at 4:55 PM, Lars-Peter Clausen [off-list ref] wrote:

quoted

On 01/22/2014 05:52 PM, Srikanth Thokala wrote:
[...]

quoted

+/**
+ * xilinx_vdma_device_control - Configure DMA channel of the device
+ * @dchan: DMA Channel pointer
+ * @cmd: DMA control command
+ * @arg: Channel configuration
+ *
+ * Return: '0' on success and failure value on error
+ */
+static int xilinx_vdma_device_control(struct dma_chan *dchan,
+                                   enum dma_ctrl_cmd cmd, unsigned long arg)
+{
+     struct xilinx_vdma_chan *chan = to_xilinx_chan(dchan);
+
+     switch (cmd) {
+     case DMA_TERMINATE_ALL:
+             xilinx_vdma_terminate_all(chan);
+             return 0;
+     case DMA_SLAVE_CONFIG:
+             return xilinx_vdma_slave_config(chan,
+                                     (struct xilinx_vdma_config *)arg);

You really shouldn't be overloading the generic API with your own semantics.
DMA_SLAVE_CONFIG should take a dma_slave_config and nothing else.

Ok.  The driver needs few additional configuration from the slave
device like Vertical
Size, Horizontal Size,  Stride etc., for the DMA transfers, in that case do you
suggest me to define a separate dma_ctrl_cmd like the one FSLDMA_EXTERNAL_START
defined for Freescale drivers?

In my opinion it is not a good idea to have driver implement a generic API,
but at the same time let the driver have custom semantics for those API
calls. It's a bit like having a gpio driver that expects 23 and 42 as the
values passed to gpio_set_value instead of 0 and 1. It completely defeats
the purpose of a generic API, namely that you are able to write generic code
that makes use of the API without having to know about which implementation
API it is talking to. The dmaengine framework provides the
dmaengine_prep_interleaved_dma() function to setup two dimensional
transfers, e.g. take a look at sirf-dma.c or imx-dma.c.

The question here i think would be waht this device supports? Is the hardware
capable of doing interleaved transfers, then would make sense.

The hardware does 2D transfers. The parameters for a transfer are height,
width and stride. That's only a subset of what interleaved transfers can be
(xt->num_frames must be one for 2d transfers). But if I remember correctly
there has been some discussion on this in the past and the result of that
discussion was that using interleaved transfers for 2D transfers is
preferred over adding a custom API for 2D transfers.

Yup that would be my recomendation. Moving this driver to interleaved API seems
right to me

-- 
~Vinod