Thanks Paul. Some comments embedded and for the rest I will update the code accordingly.
A few nits follow.
On Wed, 2015-04-01 at 17:12 -0400, Jilai Wang wrote:
--- a/drivers/gpu/drm/msm/Kconfig +++ b/drivers/gpu/drm/msm/Kconfig
+config DRM_MSM_WB
- bool "Enable writeback support for MSM modesetting driver"
- depends on DRM_MSM
- depends on VIDEO_V4L2
- select VIDEOBUF2_CORE
- default y
- help
Choose this option if you have a need to support writebackconnector.DRM_MSM_WB is a bool symbol...
--- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile
+msm-$(CONFIG_DRM_MSM_WB) += \
- mdp/mdp5/mdp5_wb_encoder.o \
- mdp/mdp_wb/mdp_wb.o \
- mdp/mdp_wb/mdp_wb_connector.o \
- mdp/mdp_wb/mdp_wb_v4l2.o
so mdp_wb_v4l2.o will never be part of a module.
If CONFIG-DRM_MSM_WB is y, then all wb related files (including mdp_wb_v4l2.o) will be added to msm-y, then be linked to msm.o obj-$(CONFIG_DRM_MSM) += msm.o Refer to document http://lwn.net/Articles/21835/ (section 3.3), it should be able to be built-in to kernel or as a module.
--- /dev/null +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb_v4l2.c
+#include <linux/module.h>
This include is needed mostly for module_param(), right?
+#define MSM_WB_MODULE_NAME "msm_wb"
MSM_WB_DRIVER_NAME? But see below.
+static unsigned debug; +module_param(debug, uint, 0644);
debug is basically a boolean type of flag. Would static bool debug; module_param(debug, bool, 0644);
work?
+MODULE_PARM_DESC(debug, "activates debug info");
No one is ever going to see this description.
+#define dprintk(dev, level, fmt, arg...) \
- v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
All instances of dprintk() that I found had level set to 1, so the above could be simplified a bit: #define dprintk(dev, fmt, arg...) \ v4l2_dbg(1, debug, &dev->v4l2_dev, fmt, ## arg)
But perhaps pr_debug() and the dynamic debug facility could be used instead of module_param(), dprintk() and v4l2_dbg(). Not sure which approach is easier.
+static const struct v4l2_file_operations msm_wb_v4l2_fops = {
- .owner = THIS_MODULE,
THIS_MODULE will basically be equivalent to NULL.
- .open = v4l2_fh_open,
- .release = vb2_fop_release,
- .poll = vb2_fop_poll,
- .unlocked_ioctl = video_ioctl2,
+};
+int msm_wb_v4l2_init(struct msm_wb *wb) +{
- struct msm_wb_v4l2_dev *dev;
- struct video_device *vfd;
- struct vb2_queue *q;
- int ret;
- dev = kzalloc(sizeof(*dev), GFP_KERNEL);
- if (!dev)
return -ENOMEM;- snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name),
"%s", MSM_WB_MODULE_NAME);It looks like you can actually drop the #define and merge the last two arguments to snprintf() into just "msm_wb".
- ret = v4l2_device_register(NULL, &dev->v4l2_dev);
- if (ret)
goto free_dev;- /* default ARGB8888 640x480 */
- dev->fmt = get_format(V4L2_PIX_FMT_RGB32);
- dev->width = 640;
- dev->height = 480;
- /* initialize queue */
- q = &dev->vb_vidq;
- q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
- q->io_modes = VB2_DMABUF;
- q->drv_priv = dev;
- q->buf_struct_size = sizeof(struct msm_wb_v4l2_buffer);
- q->ops = &msm_wb_vb2_ops;
- q->mem_ops = &msm_wb_vb2_mem_ops;
- q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
- ret = vb2_queue_init(q);
- if (ret)
goto unreg_dev;- mutex_init(&dev->mutex);
- vfd = &dev->vdev;
- *vfd = msm_wb_v4l2_template;
- vfd->debug = debug;
I couldn't find a member of struct video_device named debug. Where does that come from? Because, as far as I can see, this won't compile.
yes, it's there: http://lxr.free-electrons.com/source/include/media/v4l2-dev.h#L127
- vfd->v4l2_dev = &dev->v4l2_dev;
- vfd->queue = q;
- /*
* Provide a mutex to v4l2 core. It will be used to protect* all fops and v4l2 ioctls.*/- vfd->lock = &dev->mutex;
- video_set_drvdata(vfd, dev);
- ret = video_register_device(vfd, VFL_TYPE_GRABBER, -1);
- if (ret < 0)
goto unreg_dev;- dev->wb = wb;
- wb->wb_v4l2 = dev;
- v4l2_info(&dev->v4l2_dev, "V4L2 device registered as %s\n",
video_device_node_name(vfd));- return 0;
+unreg_dev:
- v4l2_device_unregister(&dev->v4l2_dev);
+free_dev:
- kfree(dev);
- return ret;
+}
Paul Bolle