From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galahad.ideasonboard.com ([185.26.127.97]:51961 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751365AbdAMLqt (ORCPT ); Fri, 13 Jan 2017 06:46:49 -0500 From: Laurent Pinchart To: Matt Ranostay Cc: linux-media@vger.kernel.org, Attila Kinali , Marek Vasut , Luca Barbato Subject: Re: [PATCH v5] media: video-i2c: add video-i2c driver Date: Fri, 13 Jan 2017 13:47 +0200 Message-ID: <4891099.8VPSOPnGff@avalon> In-Reply-To: <1482548666-25272-1-git-send-email-matt@ranostay.consulting> References: <1482548666-25272-1-git-send-email-matt@ranostay.consulting> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: Hi Matt, Thank you for the patch. On Friday 23 Dec 2016 19:04:26 Matt Ranostay wrote: > There are several thermal sensors that only have a low-speed bus > interface but output valid video data. This patchset enables support > for the AMG88xx "Grid-Eye" sensor family. > > Cc: Attila Kinali > Cc: Marek Vasut > Cc: Luca Barbato > Cc: Laurent Pinchart > Signed-off-by: Matt Ranostay > --- > Changes from v1: > * correct i2c_polling_remove() operations > * fixed delay calcuation in buffer_queue() > * add include linux/slab.h > > Changes from v2: > * fix build error due to typo in include of slab.h > > Changes from v3: > * switch data transport to a kthread to avoid to .buf_queue that can't sleep > * change naming from i2c-polling to video-i2c > * make the driver for single chipset under another uses the driver > > Changes from v4: > * fix wraparound issue with jiffies and schedule_timeout_interruptible() > > drivers/media/i2c/Kconfig | 9 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/video-i2c.c | 569 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 579 insertions(+) > create mode 100644 drivers/media/i2c/video-i2c.c [snip] > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c > new file mode 100644 > index 000000000000..9390560bd117 > --- /dev/null > +++ b/drivers/media/i2c/video-i2c.c [snip] > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Alphabetical order please. > +#include > +#include [snip] > +static struct v4l2_fmtdesc amg88xx_format = { > + .description = "12-bit Greyscale", If I'm not mistaken the V4L2 core fills that for you nowadays, you don't have to set it. > + .pixelformat = V4L2_PIX_FMT_Y12, > +}; > + > +static struct v4l2_frmsize_discrete amg88xx_size = { > + .width = 8, > + .height = 8, > +}; > + > +struct video_i2c_chip { > + /* video dimensions */ > + struct v4l2_fmtdesc *format; > + struct v4l2_frmsize_discrete *size; You can make those two pointers (and the variables they point to) const. > + > + /* max frames per second */ > + unsigned int max_fps; > + > + /* pixel buffer size */ > + unsigned int buffer_size; > + > + /* pixel size in bits */ > + unsigned int bpp; > + > + /* xfer function */ > + int (*xfer)(struct video_i2c_data *data, char *buf); > +}; [snip] > +static int video_i2c_thread_vid_cap(void *priv) > +{ > + struct video_i2c_data *data = priv; > + struct video_i2c_buffer *vid_cap_buf = NULL; > + > + set_freezable(); > + > + do { > + unsigned int start_jiffies = jiffies; jiffies is an unsigned long. > + unsigned int delay = msecs_to_jiffies(1000 / data->chip- >max_fps); > + int schedule_delay; > + > + try_to_freeze(); > + > + mutex_lock(&data->lock); Why do you need the mutex here ? > + spin_lock(&data->slock); > + > + if (!list_empty(&data->vid_cap_active)) { > + vid_cap_buf = list_entry(data->vid_cap_active.next, > + struct video_i2c_buffer, list); > + list_del(&vid_cap_buf->list); > + } > + > + if (vid_cap_buf) { vid_cap_buf will only be non-NULL in all but the first iteration of the loop, even if the list is empty. You should declare the variable inside the loop, not at the function level. > + struct vb2_buffer *vb2_buf = &vid_cap_buf->vb.vb2_buf; > + void *vbuf = vb2_plane_vaddr(vb2_buf, 0); > + int ret = data->chip->xfer(data, vbuf); > + > + vb2_buf->timestamp = ktime_get_ns(); > + vb2_buffer_done(vb2_buf, ret ? > + VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE); > + } > + > + spin_unlock(&data->slock); The spinlock must be unlocked before data->chip->xfer. You can't hold it and sleep. > + mutex_unlock(&data->lock); > + > + schedule_delay = delay - (jiffies - start_jiffies); Does this still work when jiffies wraps around ? > + if (schedule_delay < 0) > + schedule_delay = delay; > + > + schedule_timeout_interruptible(schedule_delay); > + } while (!kthread_should_stop()); > + > + return 0; > +} > + > +static int start_streaming(struct vb2_queue *vq, unsigned int count) > +{ > + struct video_i2c_data *data = vb2_get_drv_priv(vq); > + > + if (data->kthread_vid_cap) > + return 0; > + > + data->kthread_vid_cap = kthread_run(video_i2c_thread_vid_cap, data, > + "%s-vid-cap", data- >v4l2_dev.name); > + No need for a blank line here. > + if (IS_ERR(data->kthread_vid_cap)) { > + struct video_i2c_buffer *buf, *tmp; > + > + list_for_each_entry_safe(buf, tmp, &data->vid_cap_active, list) { > + list_del(&buf->list); > + vb2_buffer_done(&buf->vb.vb2_buf, > + VB2_BUF_STATE_QUEUED); > + } Shouldn't you protect the whole loop with the data->slock spinlock ? > + > + return PTR_ERR(data->kthread_vid_cap); > + } > + > + return 0; > +} > + > +static void stop_streaming(struct vb2_queue *vq) > +{ > + struct video_i2c_data *data = vb2_get_drv_priv(vq); > + > + if (data->kthread_vid_cap == NULL) > + return; > + > + mutex_lock(&data->lock); Do you need the mutex here ? > + spin_lock(&data->slock); > + > + while (!list_empty(&data->vid_cap_active)) { > + struct video_i2c_buffer *buf; > + > + buf = list_entry(data->vid_cap_active.next, > + struct video_i2c_buffer, list); > + list_del(&buf->list); > + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR); > + } > + spin_unlock(&data->slock); > + mutex_unlock(&data->lock); > + > + kthread_stop(data->kthread_vid_cap); Shouldn't you stop the thread first ? > + data->kthread_vid_cap = NULL; > +} [snip] > + > +static int video_i2c_querycap(struct file *file, void *priv, > + struct v4l2_capability *vcap) > +{ > + struct video_i2c_data *data = video_drvdata(file); > + struct i2c_client *client = data->client; > + > + strlcpy(vcap->driver, data->v4l2_dev.name, sizeof(vcap->driver)); > + strlcpy(vcap->card, "I2C Transport Video", sizeof(vcap->card)); Shouldn't this include the I2C device name ? > + > + sprintf(vcap->bus_info, "I2C:%d-%d", client->adapter->nr, client- >addr); > + return 0; > +} [snip] > +static int video_i2c_enum_frameintervals(struct file *file, void *priv, > + struct v4l2_frmivalenum *fe) > +{ > + struct video_i2c_data *data = video_drvdata(file); > + struct v4l2_frmsize_discrete *size = data->chip->size; > + > + if (fe->index > 0) > + return -EINVAL; > + > + if ((fe->width != size->width) || (fe->height != size->height)) No need for the inner parentheses. > + return -EINVAL; > + > + fe->type = V4L2_FRMIVAL_TYPE_DISCRETE; > + fe->discrete.numerator = 1; > + fe->discrete.denominator = data->chip->max_fps; > + > + return 0; > +} [snip] > +static int video_i2c_fmt_vid_cap(struct file *file, void *fh, > + struct v4l2_format *fmt) Should you call the function video_i2c_s_fmt_vid_cap ? > +{ > + struct video_i2c_data *data = video_drvdata(file); > + int ret = video_i2c_try_fmt_vid_cap(file, fh, fmt); > + > + if (ret < 0) > + return ret; video_i2c_try_fmt_vid_cap() always returns 0, so you can drop the check (and the ret variable). > + if (vb2_is_busy(&data->vb_vidq)) > + return -EBUSY; > + > + return 0; > +} > + > +static int video_i2c_g_parm(struct file *filp, void *priv, > + struct v4l2_streamparm *parm) > +{ > + struct video_i2c_data *data = video_drvdata(filp); > + > + if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) > + return -EINVAL; > + > + parm->parm.capture.readbuffers = 1; > + parm->parm.capture.capability = V4L2_CAP_TIMEPERFRAME; > + parm->parm.capture.timeperframe.numerator = 1; > + parm->parm.capture.timeperframe.denominator = data->chip->max_fps; > + > + return 0; > +} > + > +static int video_i2c_s_parm(struct file *filp, void *priv, > + struct v4l2_streamparm *parm) > +{ > + if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) > + return -EINVAL; > + > + return video_i2c_g_parm(filp, priv, parm); You can just use the video_i2c_g_parm() function as the .vidioc_s_parm handler, there's no need to declare a separate function. > +} [snip] > +static int video_i2c_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct video_i2c_data *data; > + struct v4l2_device *v4l2_dev; > + struct vb2_queue *queue; > + int ret; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->chip = &video_i2c_chip; > + data->client = client; > + v4l2_dev = &data->v4l2_dev; > + strlcpy(v4l2_dev->name, VIDEO_I2C_DRIVER, sizeof(v4l2_dev->name)); > + > + ret = v4l2_device_register(&client->dev, v4l2_dev); > + if (ret < 0) > + goto error_free_device; > + > + mutex_init(&data->lock); > + mutex_init(&data->queue_lock); > + > + queue = &data->vb_vidq; > + queue->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > + queue->io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ; > + queue->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC | > + V4L2_BUF_FLAG_TSTAMP_SRC_EOF; > + queue->drv_priv = data; > + queue->buf_struct_size = sizeof(struct video_i2c_buffer); > + queue->min_buffers_needed = 1; > + queue->ops = &video_i2c_video_qops; > + queue->mem_ops = &vb2_vmalloc_memops; > + > + ret = vb2_queue_init(queue); > + if (ret < 0) > + goto error_free_device; This should be error_unregister_device. > + > + data->vdev.queue = queue; > + data->vdev.queue->lock = &data->queue_lock; > + > + strlcpy(data->vdev.name, "I2C Transport Video", sizeof(data- >vdev.name)); Shouldn't this be called after the I2C device name ? > + data->vdev.v4l2_dev = v4l2_dev; > + data->vdev.fops = &video_i2c_fops; > + data->vdev.lock = &data->lock; > + data->vdev.ioctl_ops = &video_i2c_ioctl_ops; > + data->vdev.release = video_i2c_release; > + data->vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE | > + V4L2_CAP_READWRITE | V4L2_CAP_STREAMING; > + > + spin_lock_init(&data->slock); > + INIT_LIST_HEAD(&data->vid_cap_active); > + > + video_set_drvdata(&data->vdev, data); > + i2c_set_clientdata(client, data); > + > + ret = video_register_device(&data->vdev, VFL_TYPE_GRABBER, -1); > + if (ret < 0) > + goto error_unregister_device; > + > + return 0; > + > +error_unregister_device: > + v4l2_device_unregister(v4l2_dev); > + > +error_free_device: > + kfree(data); > + > + return ret; > +} > + > +static int video_i2c_remove(struct i2c_client *client) > +{ > + struct video_i2c_data *data = i2c_get_clientdata(client); > + > + v4l2_device_unregister(&data->v4l2_dev); > + video_unregister_device(&data->vdev); I believe you have to swap those two calls, as vdev references v4l2_dev. > + > + return 0; > +} [snip] -- Regards, Laurent Pinchart