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 <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.
> + 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
