Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751215AbbDBJkT (ORCPT ); Thu, 2 Apr 2015 05:40:19 -0400 Received: from lb2-smtp-cloud2.xs4all.net ([194.109.24.25]:44481 "EHLO lb2-smtp-cloud2.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750714AbbDBJkP (ORCPT ); Thu, 2 Apr 2015 05:40:15 -0400 Message-ID: <1427967609.10518.33.camel@x220> Subject: Re: [PATCH 2/3] drm:msm: Initial Add Writeback Support From: Paul Bolle To: Jilai Wang Cc: dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, robdclark@gmail.com Date: Thu, 02 Apr 2015 11:40:09 +0200 In-Reply-To: <1427922770-13721-1-git-send-email-jilaiw@codeaurora.org> References: <1427922770-13721-1-git-send-email-jilaiw@codeaurora.org> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4 (3.10.4-4.fc20) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4169 Lines: 161 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. > --- /dev/null > +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb_v4l2.c > +#include 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. > + 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/