RE: [PATCH 2/7] dmaengine: xilinx_vdma: Add quirks support to differentiate differnet IP cores

[Appana Durga Kedareswara Rao]

Hi Laurent Pinchart,

	Thanks for reviewing the patch.

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx]
> Sent: Thursday, March 17, 2016 12:49 PM
> To: Appana Durga Kedareswara Rao
> Cc: dan.j.williams@xxxxxxxxx; vinod.koul@xxxxxxxxx; Michal Simek; Soren
> Brinkmann; Appana Durga Kedareswara Rao; moritz.fischer@xxxxxxxxx;
> luis@xxxxxxxxxxxxxxxxx; Anirudha Sarangi; dmaengine@xxxxxxxxxxxxxxx; linux-
> arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/7] dmaengine: xilinx_vdma: Add quirks support to
> differentiate differnet IP cores
> 
> Hello,
> 
> On Tuesday 15 March 2016 22:53:07 Kedareswara rao Appana wrote:
> > This patch adds quirks support in the driver to differentiate
> > differnet IP
> 
> s/differnet/different/

Ok will modify In the V2.

> 
> (and in the subject line too)
> 
> With this series applied the driver will not be vdma-specific anymore. The
> xilinx_vdma_ prefix will thus be confusing. I'd bite the bullet and apply a global
> rename to functions that are not shared between the three DMA IP core types
> before patch 2/7.

Ok will modify the General API's that will be shared across the DMA's to the dma instead of vdma in the v2.

> 
> > cores.
> >
> > Signed-off-by: Kedareswara rao Appana <appanad@xxxxxxxxxx>
> > ---
> >  drivers/dma/xilinx/xilinx_vdma.c | 36
> > ++++++++++++++++++++++++++++++------
> >  1 file changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/dma/xilinx/xilinx_vdma.c
> > b/drivers/dma/xilinx/xilinx_vdma.c index 7ab6793..f682bef 100644
> > --- a/drivers/dma/xilinx/xilinx_vdma.c
> > +++ b/drivers/dma/xilinx/xilinx_vdma.c
> > @@ -139,6 +139,8 @@
> >  /* Delay loop counter to prevent hardware failure */
> >  #define XILINX_VDMA_LOOP_COUNT		1000000
> >
> > +#define AXIVDMA_SUPPORT		BIT(0)
> > +
> >  /**
> >   * struct xilinx_vdma_desc_hw - Hardware Descriptor
> >   * @next_desc: Next Descriptor Pointer @0x00 @@ -240,6 +242,7 @@
> > struct xilinx_vdma_chan {
> >   * @chan: Driver specific VDMA channel
> >   * @has_sg: Specifies whether Scatter-Gather is present or not
> >   * @flush_on_fsync: Flush on frame sync
> > + * @quirks: Needed for different IP cores
> >   */
> >  struct xilinx_vdma_device {
> >  	void __iomem *regs;
> > @@ -248,6 +251,15 @@ struct xilinx_vdma_device {
> >  	struct xilinx_vdma_chan
> *chan[XILINX_VDMA_MAX_CHANS_PER_DEVICE];
> >  	bool has_sg;
> >  	u32 flush_on_fsync;
> > +	u32 quirks;
> > +};
> > +
> > +/**
> > + * struct xdma_platform_data - DMA platform structure
> > + * @quirks: quirks for platform specific data.
> > + */
> > +struct xdma_platform_data {
> 
> This isn't platform data but device information, I'd rename the structure to
> xdma_device_info (and update the description accordingly).

Ok will modify in v2.

> 
> > +	u32 quirks;
> 
> I don't think you should call this field quirks as it stores the IP core type.
> Quirks usually refer to non-standard behaviour that requires specific handling in
> the driver, possibly in the form of work-arounds. I would thus call the field type
> and document it as "DMA IP core type". Similarly I'd rename AXIVDMA_SUPPORT
> to XDMA_TYPE_VDMA.

Sure will modify in the v2.

> 
> >  };
> >
> >  /* Macros */
> > @@ -1239,6 +1251,16 @@ static struct dma_chan
> > *of_dma_xilinx_xlate(struct of_phandle_args *dma_spec, return
> > dma_get_slave_channel(&xdev->chan[chan_id]->common);
> >  }
> >
> > +static const struct xdma_platform_data xvdma_def = {
> > +	.quirks = AXIVDMA_SUPPORT,
> > +};
> > +
> > +static const struct of_device_id xilinx_vdma_of_ids[] = {
> > +	{ .compatible = "xlnx,axi-vdma-1.00.a", .data = &xvdma_def},
> 
> Missing space before }, did you run checkpatch ?

Yes I ran check patch it didn't troughed any warning ok will fix in v2.
 
> 
> As the xdma_platform_data structure contains a single integer, you could store
> it in the .data field directly.

Ok will fix in v2.

> 
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, xilinx_vdma_of_ids);
> > +
> >  /**
> >   * xilinx_vdma_probe - Driver probe function
> >   * @pdev: Pointer to the platform_device structure @@ -1251,6 +1273,7
> > @@ static int xilinx_vdma_probe(struct platform_device
> > *pdev) struct xilinx_vdma_device *xdev;
> >  	struct device_node *child;
> >  	struct resource *io;
> > +	const struct of_device_id *match;
> >  	u32 num_frames;
> >  	int i, err;
> >
> > @@ -1259,6 +1282,13 @@ static int xilinx_vdma_probe(struct
> > platform_device
> > *pdev) if (!xdev)
> >  		return -ENOMEM;
> >
> > +	match = of_match_node(xilinx_vdma_of_ids, pdev->dev.of_node);
> 
> You can use of_device_get_match_data() to get the data directly.

Ok will fix in v2

> 
> > +	if (match && match->data) {
> 
> I don't think this condition can ever be false.

OK will fix in v2.

Regards,
Kedar.

> 
> > +		const struct xdma_platform_data *data = match->data;
> > +
> > +		xdev->quirks = data->quirks;
> > +	}
> > +
> >  	xdev->dev = &pdev->dev;
> >
> >  	/* Request and map I/O memory */
> > @@ -1356,12 +1386,6 @@ static int xilinx_vdma_remove(struct
> > platform_device
> > *pdev) return 0;
> >  }
> >
> > -static const struct of_device_id xilinx_vdma_of_ids[] = {
> > -	{ .compatible = "xlnx,axi-vdma-1.00.a",},
> > -	{}
> > -};
> > -MODULE_DEVICE_TABLE(of, xilinx_vdma_of_ids);
> > -
> >  static struct platform_driver xilinx_vdma_driver = {
> >  	.driver = {
> >  		.name = "xilinx-vdma",
> 
> --
> Regards,
> 
> Laurent Pinchart