On Fri, Jan 13, 2017 at 3:47 AM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> 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 <attila@xxxxxxxxx>
>> Cc: Marek Vasut <marex@xxxxxxx>
>> Cc: Luca Barbato <lu_zero@xxxxxxxxxx>
>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> ---
>> 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 <linux/delay.h>
>> +#include <linux/freezer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <linux/videodev2.h>
>> +#include <media/v4l2-common.h>
>> +#include <media/v4l2-ioctl.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-fh.h>
>> +#include <media/v4l2-event.h>
>
> Alphabetical order please.
>
>> +#include <media/videobuf2-v4l2.h>
>> +#include <media/videobuf2-vmalloc.h>
>
> [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.
Noted.
>
>> + 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 ?
Probably don't need the nested mutex and spinlock
>
>> + 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.
>
Noted
>> + 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 ?
Likely.
>
>> +
>> + 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.
>
Okay.
>> +}
>
> [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.
Noted.
>
>> +
>> + return 0;
>> +}
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
