On Sat, Feb 23, 2013 at 10:16:21PM +0000, Arnd Bergmann wrote:
> On Saturday 23 February 2013, Markus Pargmann wrote:
>
> > +Required properties:
> > +- compatible : Should be "fsl,<chip>-dma". chip can be imx1, imx21 or imx27
> > +- reg : Should contain DMA registers location and length
> > +- interrupts : First item should be DMA interrupt, second one is optional
> > and
> > + should contain DMA Error interrupt
> > +- #dma-cells : Has to be 1. imx-dma does not support anything else.
>
> Hmm, so #dma-cells is 1
>
> > @@ -996,13 +1020,33 @@ static void imxdma_issue_pending(struct dma_chan
> > *chan)
> > spin_unlock_irqrestore(&imxdma->lock, flags);
> > }
> >
> > +bool imxdma_filter_fn(struct dma_chan *chan, void *param)
> > +{
> > + struct imx_dma_data *data = kzalloc(sizeof(*data), GFP_KERNEL);
> > + printk("%s\n", __func__);
> > +
> > + if (!data)
> > + return false;
> > +
> > + data->dma_request = *(unsigned *) param;
> > + data->alloc_ctl_filter = true;
> > + chan->private = data;
> > +
> > + return true;
> > +}
>
> which matches the usage here, but
>
> > diff --git a/include/linux/platform_data/dma-imx.h
> > b/include/linux/platform_data/dma-imx.h
> > index f6d30cc..762a7d0 100644
> > --- a/include/linux/platform_data/dma-imx.h
> > +++ b/include/linux/platform_data/dma-imx.h
> > @@ -51,6 +51,9 @@ struct imx_dma_data {
> > int dma_request; /* DMA request line */
> > enum sdma_peripheral_type peripheral_type;
> > int priority;
> > +
> > + /* Did the controller's filter function allocated this object? */
> > + bool alloc_ctl_filter;
> > };
>
> There are actually two more members in the imx_dma_data structure. Shouldn't
> those
> be encoded in the dma specifier as well?
imx_dma_data is used by imx-dma and imx-sdma, but imx-dma does not use
peripheral_type and priority. When not loaded from devicetree
imx_dma_data is constructed by imx drivers without knowledge about the
dma driver. This patch moves the initialization to the driver, so I
think there is no need to fill all fields. I didn't want to use a new
struct because the imx drivers could still use this old way.
>
> > static inline int imx_dma_is_ipu(struct dma_chan *chan)
> > @@ -63,7 +66,8 @@ static inline int imx_dma_is_general_purpose(struct
> > dma_chan *chan)
> > return strstr(dev_name(chan->device->dev), "sdma") ||
> > !strcmp(dev_name(chan->device->dev), "imx1-dma") ||
> > !strcmp(dev_name(chan->device->dev), "imx21-dma") ||
> > - !strcmp(dev_name(chan->device->dev), "imx27-dma");
> > + !strcmp(dev_name(chan->device->dev), "imx27-dma") ||
> > + !strcmp(chan->device->dev->driver->name, "imx-dma");
> > }
>
> Also, your filter function does not actually check
> imx_dma_is_general_purpose() as the old style filter functions
> in the slave drivers do, which breaks when you have more than one dma engine
> in the system.
Oh yes, in the filter function should be a device driver comparison. But
I would prefer a direct check instead of imx_dma_is_general_purpose because
I think the driver should not define a filter function that initializes data
for another driver (sdma).
Thanks
Markus
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
