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 <radheys@xxxxxxxxxx>
> Signed-off-by: John Nichols <jnichol@xxxxxxxxxx>
> Signed-off-by: Jeffrey Mouroux <jmouroux@xxxxxxxxxx>
> Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
> Signed-off-by: Hyun Kwon <hyun.kwon@xxxxxxxxxx>
> Signed-off-by: Vishal Sagar <vsagar@xxxxxxxxxx>
> ---
> 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
> @@ -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 <radheys@xxxxxxxxxx>
> + * John Nichols <jnichol@xxxxxxxxxx>
> + * Jeffrey Mouroux <jmouroux@xxxxxxxxxx>
> + *
> + * 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..
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
