Re: [PATCH v2] dma: Add Xilinx AXI Video Direct Memory Access Enginedriver support

[Lars-Peter Clausen]

On 01/31/2014 06:44 PM, Andy Gross 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 <lars@xxxxxxxxxx> 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 comments in the include/linux/dmaengine.h state that if you have
> non-generic, non-fixed configuration then you can just create your own
> structure and embed the dma_slave_config.  Using the container_of you can get
> back your structure.

We should probably revise that, since it is not going to work that well.

> I agree that we should always use the generic structure if possible, but
> sometimes there are some non-standard things that you have to do for your
> hardware.  I am currently in a bind for adding some quirky features that are
> required by peripherals who want to use the QCOM DMA devices.

Well there are two types of extensions to the API. The first type changes the 
semantics of the API so it is no longer possible to use the API without knowing 
about the extension. This is in my opinion a complete no-go since goes against 
the very idea of a common API. If you implement the common API with custom 
semantics you have a custom API. It's just better hidden since you use the same 
function names. My opinion on this is if you want/need a custom API make it a 
custom API with custom function names. This on one hand avoids confusion about 
the behavior and on the other hand reduces the maintenance burden for the 
common API (e.g. if somebody makes changes to the common API they don't have to 
bother to update your driver and don't have to try to understand the custom 
semantics). The other kind of extensions are those that add additional 
functionality on top of the common API, while keeping the normal semantics for 
the common API. Which means a user that does not know about the extensions is 
still able to function. A user that knows about the extension can make use of 
the additional features.

That said, everybody always thinks their hardware is special and requires 
special extensions. Usually this is not the case, there will always sooner or 
later somebody else who needs the same extensions. The dmaengine API is not set 
in stone, so if you think something is missing to properly to support your 
hardware it is worth investigating if it makes sense to add the missing parts 
to the common API. As I said before the whole point of the exercise of having a 
common API is that we want to abstract away (hardware) implementation specific 
details. This allows the upper layers to have platform independent common code 
to take care of setting up the DMA transfers. E.g. in ALSA subsystem we went 
from 10+ custom implementations of a PCM driver build on top of dmaengine to 1 
generic implementation that is shared between platforms. All those custom PCM 
drivers had hardcoded assumptions about the behavior and features of the 
underlying dmaengine driver. To be able to have one generic PCM driver it was 
necessarily to extend the dmaengine API to be able to expose these differences 
in features and behavior. So as I said the API is not set in stone if it is 
necessary to extend or modify it to support something properly do it. Other 
subsystems also want to go the direction of having more shared code that makes 
use of the dmaengine API at the subsytem level rather than having every driver 
basically implement the same stuff (with slight variations) over and over 
again. Having custom extensions in your dmaengine will not make it possible to 
write a generic user.

> If the context field in prep_slave_sg and prep_dma_cyclic was exposed to
> everyone, that would allow an easy way to pass in hardware specific
> configuration without bastardizing the slave_config.  I noticed that rapidio is
> the only consumer of that field and that they have their own prep function.
>
> If we are not going to allow people to do their own slave_config when they need
> to, then we need to remove the comments from the include file and expose the
> context to the dmaengine_prep_slave_sg and dmaengine_prep_dma_cyclic.

I don't think the way the rapidio stuff is handled is good for the reasons 
stated above. It uses the same names, but has different semantics. A user of 
the dmaengine interface that does not know that the underlying dmaengine driver 
expects rapidio semantics does not work.

- Lars

--
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/