On Wed, Dec 20, 2017 at 02:00:18PM +0530, Vishal Sagar wrote:

The Video Framebuffer IP is available in two forms: read or write.
This driver supports either form of the IP.
Each IP is a single channel DMA and is video format aware.
It can read/write video data arranged from/to memory as packed or
semi-planar way based on the selected video format.
To get list of supported video formats, clients can call certain
APIs exposed by the driver. This driver introduces support for
these IPs and integrates with the DMA Engine Framework.

The subsytem name is dmaengine!

Signed-off-by: Radhey Shyam Pandey <redacted>
Signed-off-by: John Nichols <redacted>
Signed-off-by: Jeffrey Mouroux <redacted>
Signed-off-by: Michal Simek <redacted>
Signed-off-by: Hyun Kwon <redacted>
Signed-off-by: Vishal Sagar <redacted>
---
 drivers/dma/Kconfig                |   14 +-
 drivers/dma/xilinx/Makefile        |    1 +
 drivers/dma/xilinx/xilinx_frmbuf.c | 1155 ++++++++++++++++++++++++++++++++++++

Thats a very large file for review, do consider splitting stuff

quoted hunk

@@ -342,7 +342,7 @@ config MOXART_DMA
 	select DMA_VIRTUAL_CHANNELS
 	help
 	  Enable support for the MOXA ART SoC DMA controller.
-
+

why this noise, this patch has some many sign-off's but didnt anyone see this
noise?

+// SPDX-License-Identifier: GPL-2.0

Copyright needs to follow this in C99 style

+/*
+ * DMAEngine driver for Xilinx Framebuffer IP
+ *
+ * Copyright (C) 2016 - 2017 Xilinx, Inc. All rights reserved.
+ *
+ * Authors: Radhey Shyam Pandey [off-list ref]
+ *          John Nichols [off-list ref]
+ *          Jeffrey Mouroux [off-list ref]
+ *
+ * Based on the Freescale DMA driver.
+ *
+ * Description:
+ * The AXI Framebuffer core is a soft Xilinx IP core that
+ * provides high-bandwidth direct memory access between memory
+ * and AXI4-Stream.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.

why do you need this license text? With SPDX tag this is implied..

+#include <linux/bitops.h>
+#include <linux/dma/xilinx_frmbuf.h>
+#include <linux/dmapool.h>
+#include <linux/gpio/consumer.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_dma.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include <linux/videodev2.h>

do you need so many headers?

+struct xilinx_frmbuf_chan {
+	struct xilinx_frmbuf_device *xdev;
+	/* Descriptor operation lock */

this is duplicate comment

+static LIST_HEAD(frmbuf_chan_list);
+static DEFINE_MUTEX(frmbuf_chan_list_lock);

why do you need these globals?

+static const struct xilinx_frmbuf_format_desc xilinx_frmbuf_formats[] = {
+	{
+		.dts_name = "xbgr8888",
+		.id = XILINX_FRMBUF_FMT_RGBX8,
+		.bpp = 4,
+		.num_planes = 1,
+		.drm_fmt = DRM_FORMAT_XBGR8888,
+		.v4l2_fmt = 0,
+		.fmt_bitmask = BIT(0),
+	},
+	{
+		.dts_name = "unsupported",
+		.id = XILINX_FRMBUF_FMT_YUVX8,
+		.bpp = 4,
+		.num_planes = 1,
+		.drm_fmt = 0,
+		.v4l2_fmt = 0,
+		.fmt_bitmask = BIT(1),
+	},

am no DT expert, but this doesn't look right to me. Care to explain what
this struct does?

+static const struct of_device_id xilinx_frmbuf_of_ids[] = {
+	{ .compatible = "xlnx,axi-frmbuf-wr-v2",
+		.data = (void *)DMA_DEV_TO_MEM},
+	{ .compatible = "xlnx,axi-frmbuf-rd-v2",
+		.data = (void *)DMA_MEM_TO_DEV},

is the direction a hw property or configurable?

+static inline void frmbuf_write(struct xilinx_frmbuf_chan *chan, u32 reg,
+				u32 value)

right justfied pls, here and other places

+{
+	iowrite32(value, chan->xdev->regs + reg);
+}
+
+static inline void frmbuf_writeq(struct xilinx_frmbuf_chan *chan, u32 reg,
+				 u64 value)
+{
+	iowrite32(lower_32_bits(value), chan->xdev->regs + reg);
+	iowrite32(upper_32_bits(value), chan->xdev->regs + reg + 4);
+}
+
+static void writeq_addr(struct xilinx_frmbuf_chan *chan, u32 reg,
+			dma_addr_t addr)
+{
+	frmbuf_writeq(chan, reg, (u64)addr);

why the cast?

+static struct xilinx_frmbuf_device *frmbuf_find_dev(struct dma_chan *chan)
+{
+	struct xilinx_frmbuf_chan *xchan, *temp;
+	struct xilinx_frmbuf_device *xdev;
+	bool is_frmbuf_chan = false;
+
+	list_for_each_entry_safe(xchan, temp, &frmbuf_chan_list, chan_node) {
+		if (chan == &xchan->common)
+			is_frmbuf_chan = true;
+	}
+
+	if (!is_frmbuf_chan)
+		return ERR_PTR(-ENODEV);
+
+	xchan = to_xilinx_chan(chan);
+	xdev = container_of(xchan, struct xilinx_frmbuf_device, chan);
+
+	return xdev;

hmm this sounds strange to me, care to explain this pls?

+static int frmbuf_verify_format(struct dma_chan *chan, u32 fourcc, u32 type)
+{
+	struct xilinx_frmbuf_chan *xil_chan = to_xilinx_chan(chan);
+	u32 i, sz = ARRAY_SIZE(xilinx_frmbuf_formats);
+
+	for (i = 0; i < sz; i++) {
+		if ((type == XDMA_DRM &&
+		     fourcc != xilinx_frmbuf_formats[i].drm_fmt) ||
+		   (type == XDMA_V4L2 &&
+		    fourcc != xilinx_frmbuf_formats[i].v4l2_fmt))

this is very hard to read..

+static void xilinx_xdma_set_config(struct dma_chan *chan, u32 fourcc, u32 type)
+{
+	struct xilinx_frmbuf_chan *xil_chan;
+	bool found_xchan = false;
+	int ret;
+
+	mutex_lock(&frmbuf_chan_list_lock);
+	list_for_each_entry(xil_chan, &frmbuf_chan_list, chan_node) {
+		if (chan == &xil_chan->common) {
+			found_xchan = true;
+			break;
+		}
+	}

again this seaching and am not sure why?

+void xilinx_xdma_drm_config(struct dma_chan *chan, u32 drm_fourcc)
+{
+	xilinx_xdma_set_config(chan, drm_fourcc, XDMA_DRM);
+
+} EXPORT_SYMBOL_GPL(xilinx_xdma_drm_config);

first EXPORT_SYMBOL_GPL should be in different line, second what is being
set? some documentation for EXPORT_SYMBOLs is required..

+
+void xilinx_xdma_v4l2_config(struct dma_chan *chan, u32 v4l2_fourcc)
+{
+	xilinx_xdma_set_config(chan, v4l2_fourcc, XDMA_V4L2);
+
+} EXPORT_SYMBOL_GPL(xilinx_xdma_v4l2_config);

here too..

+
+int xilinx_xdma_get_drm_vid_fmts(struct dma_chan *chan, u32 *fmt_cnt,
+				 u32 **fmts)
+{
+	struct xilinx_frmbuf_device *xdev;
+
+	xdev = frmbuf_find_dev(chan);
+
+	if (IS_ERR(xdev))
+		return PTR_ERR(xdev);
+
+	*fmt_cnt = xdev->drm_fmt_cnt;
+	*fmts = xdev->drm_memory_fmts;
+
+	return 0;
+}
+EXPORT_SYMBOL(xilinx_xdma_get_drm_vid_fmts);

and here

+
+int xilinx_xdma_get_v4l2_vid_fmts(struct dma_chan *chan, u32 *fmt_cnt,
+				  u32 **fmts)
+{
+	struct xilinx_frmbuf_device *xdev;
+
+	xdev = frmbuf_find_dev(chan);
+
+	if (IS_ERR(xdev))
+		return PTR_ERR(xdev);
+
+	*fmt_cnt = xdev->v4l2_fmt_cnt;
+	*fmts = xdev->v4l2_memory_fmts;
+
+	return 0;
+}
+EXPORT_SYMBOL(xilinx_xdma_get_v4l2_vid_fmts);

and this

+/* -----------------------------------------------------------------------------

do you mind getting rid of these, at best they are distraction

+static struct xilinx_frmbuf_tx_descriptor *
+xilinx_frmbuf_alloc_tx_descriptor(struct xilinx_frmbuf_chan *chan)
+{
+	struct xilinx_frmbuf_tx_descriptor *desc;
+
+	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return NULL;

this can be skipped here!

+static void xilinx_frmbuf_free_descriptors(struct xilinx_frmbuf_chan *chan)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&chan->lock, flags);
+
+	xilinx_frmbuf_free_desc_list(chan, &chan->pending_list);
+	xilinx_frmbuf_free_desc_list(chan, &chan->done_list);
+	kfree(chan->active_desc);
+	kfree(chan->staged_desc);
+
+	chan->staged_desc = NULL;
+	chan->active_desc = NULL;
+	INIT_LIST_HEAD(&chan->pending_list);
+	INIT_LIST_HEAD(&chan->done_list);

initializing lists in free?

+static int xilinx_frmbuf_alloc_chan_resources(struct dma_chan *dchan)
+{
+	dma_cookie_init(dchan);

this can be done in your probe and not on alloc.. and consequently you can
get rid of alloc..

+static enum dma_status xilinx_frmbuf_tx_status(struct dma_chan *dchan,
+					       dma_cookie_t cookie,
+					       struct dma_tx_state *txstate)
+{
+	return dma_cookie_status(dchan, cookie, txstate);

no residue caln?

+static int xilinx_frmbuf_chan_probe(struct xilinx_frmbuf_device *xdev,
+				    struct device_node *node)
+{
+	struct xilinx_frmbuf_chan *chan;
+	int err;
+	u32 dma_addr_size;
+
+	chan = &xdev->chan;
+
+	chan->dev = xdev->dev;
+	chan->xdev = xdev;
+	chan->idle = true;
+
+	err = of_property_read_u32(node, "xlnx,dma-addr-width",

why not device_property_read_u32

+static int xilinx_frmbuf_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct xilinx_frmbuf_device *xdev;
+	struct resource *io;
+	enum dma_transfer_direction dma_dir;
+	const struct of_device_id *match;
+	int err;
+	u32 i, j;
+	int hw_vid_fmt_cnt;
+	const char *vid_fmts[ARRAY_SIZE(xilinx_frmbuf_formats)];

inverted x-mas tree helps in readablity

+	INIT_LIST_HEAD(&xdev->common.channels);
+	dma_cap_set(DMA_SLAVE, xdev->common.cap_mask);

why slave and why not DMA_INTERLEAVE?

+	dma_cap_set(DMA_PRIVATE, xdev->common.cap_mask);
+
+	/* Initialize the channels */
+	err = xilinx_frmbuf_chan_probe(xdev, node);
+	if (err < 0)
+		return err;
+
+	xdev->chan.direction = dma_dir;
+
+	if (xdev->chan.direction == DMA_DEV_TO_MEM) {
+		xdev->common.directions = BIT(DMA_DEV_TO_MEM);
+		dev_info(&pdev->dev, "Xilinx AXI frmbuf DMA_DEV_TO_MEM\n");
+	} else if (xdev->chan.direction == DMA_MEM_TO_DEV) {
+		xdev->common.directions = BIT(DMA_MEM_TO_DEV);
+		dev_info(&pdev->dev, "Xilinx AXI frmbuf DMA_MEM_TO_DEV\n");
+	} else {
+		xilinx_frmbuf_chan_remove(&xdev->chan);

why not do the checks first!

+MODULE_AUTHOR("Xilinx, Inc.");
+MODULE_DESCRIPTION("Xilinx Framebuffer driver");
+MODULE_LICENSE("GPL v2");

no MODULE_ALIAS, how did you test this?

+#if IS_ENABLED(CONFIG_XILINX_FRMBUF)
+/**
+ * xilinx_xdma_drm_config - configure video format in video aware DMA
+ * @chan: dma channel instance
+ * @drm_fourcc: DRM fourcc code describing the memory layout of video data
+ *
+ * This routine is used when utilizing "video format aware" Xilinx DMA IP
+ * (such as Video Framebuffer Read or Video Framebuffer Write).  This call
+ * must be made prior to dma_async_issue_pending() to enstablish the video
+ * data memory format within the hardware DMA.
+ */

pls move this to src..