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 writeback >> + connector. > > 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
