From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lars-Peter Clausen Subject: Re: [PATCH v2] dma: Add Xilinx AXI Video Direct Memory Access Engine driver support Date: Sun, 26 Jan 2014 18:39:21 +0100 Message-ID: <52E54849.2000208@metafoo.de> References: <1390409565-4200-1-git-send-email-sthokal@xilinx.com> <1390409565-4200-2-git-send-email-sthokal@xilinx.com> <52E0FC22.8060903@metafoo.de> <52E2698B.6070001@metafoo.de> <20140126135933.GD10628@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140126135933.GD10628@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Vinod Koul Cc: devicetree@vger.kernel.org, "linux-kernel@vger.kernel.org" , michal.simek@xilinx.com, Srikanth Thokala , dmaengine@vger.kernel.org, robh+dt@kernel.org, Grant Likely , dan.j.williams@intel.com, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org On 01/26/2014 02:59 PM, Vinod Koul wrote: > On Fri, Jan 24, 2014 at 02:24:27PM +0100, Lars-Peter Clausen wrote: >> On 01/24/2014 12:16 PM, Srikanth Thokala wrote: >>> Hi Lars, >>> >>> On Thu, Jan 23, 2014 at 4:55 PM, Lars-Peter Clausen wrote: >>>> On 01/22/2014 05:52 PM, Srikanth Thokala wrote: >>>> [...] >>>>> +/** >>>>> + * 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. - Lars