| Message ID | 1458062592-27981-3-git-send-email-appanad@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 15, 2016 at 10:53:07PM +0530, Kedareswara rao Appana wrote:
> This patch adds quirks support in the driver to differentiate differnet IP cores.
Wouldn't it help to explain why quirks are needed for these cores in
changelog?
Also limit your changelogs properly. Am sure checkpatch would have warned
you!
Hi Vinod, > -----Original Message----- > From: dmaengine-owner@vger.kernel.org [mailto:dmaengine- > owner@vger.kernel.org] On Behalf Of Vinod Koul > Sent: Wednesday, March 16, 2016 8:44 AM > To: Appana Durga Kedareswara Rao > Cc: dan.j.williams@intel.com; Michal Simek; Soren Brinkmann; Appana Durga > Kedareswara Rao; moritz.fischer@ettus.com; > laurent.pinchart@ideasonboard.com; luis@debethencourt.com; Anirudha > Sarangi; dmaengine@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH 2/7] dmaengine: xilinx_vdma: Add quirks support to > differentiate differnet IP cores > > On Tue, Mar 15, 2016 at 10:53:07PM +0530, Kedareswara rao Appana wrote: > > This patch adds quirks support in the driver to differentiate differnet IP cores. > > Wouldn't it help to explain why quirks are needed for these cores in changelog? Will fix in the next version. > > Also limit your changelogs properly. Am sure checkpatch would have warned > you! Sure will fix in the next version sorry for the noise. Thanks, Kedar. > > -- > ~Vinod > -- > To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body > of a message to majordomo@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html
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/ (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. > cores. > > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> > --- > 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). > + 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. > }; > > /* 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 ? As the xdma_platform_data structure contains a single integer, you could store it in the .data field directly. > + {} > +}; > +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. > + if (match && match->data) { I don't think this condition can ever be false. > + 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",
Hi Laurent Pinchart, Thanks for reviewing the patch. > -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com] > Sent: Thursday, March 17, 2016 12:49 PM > To: Appana Durga Kedareswara Rao > Cc: dan.j.williams@intel.com; vinod.koul@intel.com; Michal Simek; Soren > Brinkmann; Appana Durga Kedareswara Rao; moritz.fischer@ettus.com; > luis@debethencourt.com; Anirudha Sarangi; dmaengine@vger.kernel.org; linux- > arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > 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@xilinx.com> > > --- > > 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
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 { + u32 quirks; }; /* 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}, + {} +}; +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); + if (match && match->data) { + 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",
This patch adds quirks support in the driver to differentiate differnet IP cores. Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com> --- drivers/dma/xilinx/xilinx_vdma.c | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-)