Hi Jilai, On Thu, 2015-04-02 at 17:58 +0000, jilaiw@xxxxxxxxxxxxxx wrote: > Thanks Paul. Some comments embedded and for the rest I will update the > code accordingly. Most of my comments appear to be ill informed, so I wouldn't mind if you'd specify which updates you actually plan to do. > >> --- 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 (A tell tale was that I didn't quote that line.) > 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. It's hard typing with a brown paper bag for headware: I still have trouble speaking Makefile, even after all these years, but I'm afraid you're right. > >> --- /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 Probably in some tree I'm not aware of. I only did: $ git cat-file blob v4.0-rc6:include/media/v4l2-dev.h | grep debug /* Internal device debug flags, not for use by drivers */ int dev_debug; and $ git cat-file blob next-20150402:include/media/v4l2-dev.h | grep debug /* Internal device debug flags, not for use by drivers */ int dev_debug; Which tree does debug come from? Thanks, Paul Bolle _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
