dma: xilinx: Add driver for Video Framebuffer IP
PATCH 2/2 dma: xilinx: Add driver for Video Framebuffer IP
Quoted 48 lines · Vishal Sagar
Hi Vinod
Thanks for your feedback.
Quoted 11 lines
-----Original Message-----
From: Vinod Koul [mailto:[email protected]]
Sent: Friday, December 22, 2017 3:25 PM
To: Vishal Sagar <[email protected]>
Cc: Vishal Sagar <[email protected]>; [email protected]; Dinesh Kumar <[email protected]>; [email protected]; Jeff Mouroux <[email protected]>; Radhey Shyam Pandey <[email protected]>; Hyun Kwon <[email protected]>; Maurice Penners <[email protected]>; John Nichols <[email protected]>
Subject: Re: [PATCH 2/2] dma: xilinx: Add driver for Video Framebuffer IPQuoted 8 lines
Quoted 4 lines · Vishal Sagar
Quoted 2 lines
The subsytem name is dmaengine!
[Vishal Sagar]
You don't need these tags when replying inline style
Ok noted.
Quoted 4 lines
Quoted 1 line
Apologies. This is my first instance of sending across patches to
community.
Quoted 1 line
I saw the precedence for this in other xilinx driver commit messages so
kept it as it is.
Quoted 9 lines
Quoted 1 line
I will make the changes as "dmaengine: xilinx: " in next version.
which precedence?
$ git log --online drivers/dma/xilinx/ |grep " dma:" |wc -l 2 $ git log --online drivers/dma/xilinx/ |grep " dmaengine: " |wc -l 52My mistake. It was in our git repo. I will take care in future.
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+static LIST_HEAD(frmbuf_chan_list); static +DEFINE_MUTEX(frmbuf_chan_list_lock);why do you need these globals?
Quoted 4 lines
Quoted 1 line
Please fix your MUA to wrap at 80 columns, I have reflowed your text to fit.
Ok thanks. I am using Outlook to send mails and have setup the client as per https://elinux.org/Mail_client_tips.
Did you follow that? Esp the wrapping part. Whatever tool you use, pls make sure replies are within 80chars
Quoted 10 lines
Quoted 7 lines
Quoted 5 lines
The frame buffer read and write are Soft IPs which can be configured to support different video formats. Please refer to PG278 Product Guide for Video Frame Buffer Read and Write v2.0.
huh why should I refer to that!
The product guide gives details about the memory layout of buffer for every format supported by these IPs.
Sorry but not my problem. I should understand and be able to review the patch. Please create and add comments to satisfy that
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
The client driver would want to
know what are the video formats supported and set the formataccordingly.
Quoted 1 line
For this there are APIs exposed by this driver.
You are *not* the first driver to support these kind of use cases.
80 char wrap issue again :(
Quoted 10 lines
Ok. We discussed internally about this. Instead of using custom APIs we could use
device_config()in the client and send custom data by wrapping it around the structdma_slave_config.=====================================================================
enum {
UNKNOWN,
XFRMBUF,
}xdma_type;
Quoted 8 lines
struct xilinx_dma_config {
struct dma_slave_config config;
enum xdma_type type;
union {
struct framebuffer_config;
/* Other future dma configs here */
} xdma_config;
};again, you are not the first ones proposing this and sorry we don't go that route anymore.
Please see other video drivers on how they set this
Quoted 7 lines
enum {
UNKNOWN,
V4L2,
DRM
} framework_type;Quoted 6 lines
struct framebuffer_config {
video_format; /* input param */
enum framework_type fwtype;
supported_v4l2_video_formats[]; /* output param */
supported_drm_video_formats[]; /* output param */
}this is all video specfic stuff, why should a dmaengine driver care about this. DMA care about transferring data.
As I said last time, you should split video stuff and dmaengine stuff to separate drivers. Dmaengine should only worry about transferring data and video driver should worry about video formats
Quoted 6 lines
=====================================================================
First time client sets type = UNKNOWN and callsdevice_config(). The driver sets the type back as XFRMBUF and populates the supported v4l2 and drm video formats and returns. Client calls it second time with type = XFRMBUF andvideo_formatand fwtype set to one of the supported formats. This will help framebuffer driver correctly configure the video format.
who is the client, video driver right?