Re: [PATCH v2] dma: Add Xilinx AXI Video Direct Memory AccessEngine driver support

[Vinod Koul]

On Fri, Jan 31, 2014 at 12:22:52PM +0530, 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.
> >>
> >> While we do try to get users use dma_slave_config, but there will always be
> >> someone who have specfic params. If we can generalize then we might want to add
> >> to the dma_slave_config as well
> >
> > There are many configuration parameters which are specific to IP and I
> > would like to
> > give an overview of some of parameteres here:
> >
> > 1) Park Mode ('cfg->park'): In Park mode, engine will park on frame
> > referenced by
> >     'cfg->park_frm', so user will have control on each frame in this mode.
> >
> > 2) Interrupt Coalesce ('cfg->coalesce'):  Used for setting interrupt
> > threshold. This value
> >    determines the number of frame buffers to process. To use this feature,
> >    'cfg->frm_cnt_en' should be set.
> >
> > 3) Frame Synchronization Source ('cfg->ext_fsync'):  Can be an
> > external/internal frame
> >     synchronization source. Used to synchronize one channel (MM2S/S2MM) with
> >     another (S2MM/MM2S) channel.
> >
> > 4) Genlock Synchronization ('cfg->genlock'): Used to avoid mismatch rate between
> >     master and slave.  In master mode (cfg->master), frames are not dropped and
> >     slave can drop frames to adjust to master frame rate.
> >
> > And in future, this Engine being a soft IP, we could expect some more additional
> > parameters.  Isn't a good idea to have a private member in dma_slave_config for
> > sharing additional configuration between slave device and dma engine? Or a new
> > dma_ctrl_cmd like FSLDMA_EXTERNAL_START?

The idea of a generic API is that we can use it for most of the controllers. Even
if you are planning to support a family of controllers

ATM, lets not discuss the possiblity of private member and try to exhanust all
possible options. Worst case you can embed the dma_slave_config in
xilinx_dma_slave_config and retrieve it in dmac driver

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/