On 05/04/18 09:39, Sakari Ailus wrote:
> Hi Matt,
>
> Thanks for the patch. It's a very nicely written, small driver. :-)
>
> Please see more comments below.
>
> On Sat, Mar 31, 2018 at 05:59:26PM -0700, 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.
>>
>> Signed-off-by: Matt Ranostay <matt.ranostay@xxxxxxxxxxxx>
>
> Are there plans to implement support for more of these chips?
>
>> ---
>> MAINTAINERS | 6 +
>> drivers/media/i2c/Kconfig | 9 +
>> drivers/media/i2c/Makefile | 1 +
>> drivers/media/i2c/video-i2c.c | 562 ++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 578 insertions(+)
>> create mode 100644 drivers/media/i2c/video-i2c.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 49236216a871..f0262b751d5b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -14863,6 +14863,12 @@ L: linux-media@xxxxxxxxxxxxxxx
>> S: Maintained
>> F: drivers/media/platform/video-mux.c
>>
>> +VIDEO I2C POLLING DRIVER
>> +M: Matt Ranostay <matt.ranostay@xxxxxxxxxxxx>
>> +L: linux-media@xxxxxxxxxxxxxxx
>> +S: Maintained
>> +F: drivers/media/i2c/video-i2c.c
>> +
>> VIDEOBUF2 FRAMEWORK
>> M: Pawel Osciak <pawel@xxxxxxxxxx>
>> M: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 541f0d28afd8..122e5047a01e 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -974,6 +974,15 @@ config VIDEO_M52790
>>
>> To compile this driver as a module, choose M here: the
>> module will be called m52790.
>> +
>> +config VIDEO_I2C
>> + tristate "I2C transport video support"
>> + depends on VIDEO_V4L2 && I2C
>> + select VIDEOBUF2_VMALLOC
>> + ---help---
>> + Enable the I2C transport video support which supports the
>> + following:
>> + * Panasonic AMG88xx Grid-Eye Sensors
>
> You could add the name of the module here if it's compiled as a module.
>
>> endmenu
>>
>> menu "Sensors used on soc_camera driver"
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index ea34aee1a85a..84cc472238ef 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -96,6 +96,7 @@ obj-$(CONFIG_VIDEO_LM3646) += lm3646.o
>> obj-$(CONFIG_VIDEO_SMIAPP_PLL) += smiapp-pll.o
>> obj-$(CONFIG_VIDEO_AK881X) += ak881x.o
>> obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o
>> +obj-$(CONFIG_VIDEO_I2C) += video-i2c.o
>> obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o
>> obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
>> obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
>> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
>> new file mode 100644
>> index 000000000000..9fba8a2767c5
>> --- /dev/null
>> +++ b/drivers/media/i2c/video-i2c.c
>> @@ -0,0 +1,562 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * video-i2c.c - Support for I2C transport video devices
>> + *
>> + * Copyright (C) 2018 Matt Ranostay <matt.ranostay@xxxxxxxxxxxx>
>> + *
>> + * Supported:
>> + * - Panasonic AMG88xx Grid-Eye Sensors
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/freezer.h>
>> +#include <linux/kthread.h>
>> +#include <linux/i2c.h>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of_device.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <linux/videodev2.h>
>> +#include <media/v4l2-common.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-event.h>
>> +#include <media/v4l2-fh.h>
>> +#include <media/v4l2-ioctl.h>
>> +#include <media/videobuf2-v4l2.h>
>> +#include <media/videobuf2-vmalloc.h>
>> +
>> +#define VIDEO_I2C_DRIVER "video-i2c"
>> +
>> +struct video_i2c_chip;
>> +
>> +struct video_i2c_buffer {
>> + struct vb2_v4l2_buffer vb;
>> + struct list_head list;
>> +};
>> +
>> +struct video_i2c_data {
>> + struct i2c_client *client;
>> + const struct video_i2c_chip *chip;
>> + struct mutex lock;
>> + spinlock_t slock;
>> + unsigned int sequence;
>> + struct mutex queue_lock;
>> +
>> + struct v4l2_device v4l2_dev;
>> + struct video_device vdev;
>> + struct vb2_queue vb_vidq;
>> +
>> + struct task_struct *kthread_vid_cap;
>> + struct list_head vid_cap_active;
>> +};
>> +
>> +static struct v4l2_fmtdesc amg88xx_format = {
>> + .pixelformat = V4L2_PIX_FMT_Y12,
>> +};
>> +
>> +static struct v4l2_frmsize_discrete amg88xx_size = {
>> + .width = 8,
>> + .height = 8,
>> +};
>> +
>> +struct video_i2c_chip {
>> + /* video dimensions */
>> + const struct v4l2_fmtdesc *format;
>> + const struct v4l2_frmsize_discrete *size;
>> +
>> + /* 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);
>> +};
>> +
>> +static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
>> +{
>> + struct i2c_client *client = data->client;
>> + struct i2c_msg msg[2];
>> + u8 reg = 0x80;
>> + int ret;
>> +
>> + msg[0].addr = client->addr;
>> + msg[0].flags = 0;
>> + msg[0].len = 1;
>> + msg[0].buf = (char *) ®
>> +
>> + msg[1].addr = client->addr;
>> + msg[1].flags = I2C_M_RD;
>> + msg[1].len = data->chip->buffer_size;
>> + msg[1].buf = (char *) buf;
>
> No space before "buf" (nor "®" above).
>
>> +
>> + ret = i2c_transfer(client->adapter, msg, 2);
>> +
>> + return (ret == 2) ? 0 : -EIO;
>> +}
>> +
>> +#define AMG88XX 0
>> +
>> +static const struct video_i2c_chip video_i2c_chip[] = {
>> + [AMG88XX] = {
>> + .size = &amg88xx_size,
>> + .format = &amg88xx_format,
>> + .max_fps = 10,
>> + .buffer_size = 128,
>> + .bpp = 16,
>> + .xfer = &amg88xx_xfer,
>> + },
>> +};
>> +
>> +static const struct v4l2_file_operations video_i2c_fops = {
>> + .owner = THIS_MODULE,
>> + .open = v4l2_fh_open,
>> + .release = vb2_fop_release,
>> + .poll = vb2_fop_poll,
>> + .read = vb2_fop_read,
>> + .mmap = vb2_fop_mmap,
>> + .unlocked_ioctl = video_ioctl2,
>> +};
>> +
>> +static int queue_setup(struct vb2_queue *vq,
>> + unsigned int *nbuffers, unsigned int *nplanes,
>> + unsigned int sizes[], struct device *alloc_devs[])
>> +{
>> + struct video_i2c_data *data = vb2_get_drv_priv(vq);
>> + unsigned int size = data->chip->buffer_size;
>> +
>> + if (vq->num_buffers + *nbuffers < 2)
>> + *nbuffers = 2;
>> +
>> + if (*nplanes)
>> + return sizes[0] < size ? -EINVAL : 0;
>> +
>> + *nplanes = 1;
>> + sizes[0] = size;
>> +
>> + return 0;
>> +}
>> +
>> +static int buffer_prepare(struct vb2_buffer *vb)
>> +{
>> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> + struct video_i2c_data *data = vb2_get_drv_priv(vb->vb2_queue);
>> + unsigned int size = data->chip->buffer_size;
>> +
>> + if (vb2_plane_size(vb, 0) < size)
>> + return -EINVAL;
>> +
>> + vbuf->field = V4L2_FIELD_NONE;
>> + vb2_set_plane_payload(vb, 0, size);
>> +
>> + return 0;
>> +}
>> +
>> +static void buffer_queue(struct vb2_buffer *vb)
>> +{
>> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> + struct video_i2c_data *data = vb2_get_drv_priv(vb->vb2_queue);
>> + struct video_i2c_buffer *buf =
>> + container_of(vbuf, struct video_i2c_buffer, vb);
>> +
>> + spin_lock(&data->slock);
>> + list_add_tail(&buf->list, &data->vid_cap_active);
>> + spin_unlock(&data->slock);
>> +}
>> +
>> +static int video_i2c_thread_vid_cap(void *priv)
>> +{
>> + struct video_i2c_data *data = priv;
>> +
>> + set_freezable();
>> +
>> + do {
>> + unsigned long start_jiffies = jiffies;
>> + unsigned int delay = msecs_to_jiffies(1000 / data->chip->max_fps);
>> + struct video_i2c_buffer *vid_cap_buf = NULL;
>> + int schedule_delay;
>> +
>> + try_to_freeze();
>> +
>> + 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);
>> + }
>> +
>> + spin_unlock(&data->slock);
>> +
>> + if (vid_cap_buf) {
>> + 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();
>> + vid_cap_buf->vb.sequence = data->sequence++;
>> + vb2_buffer_done(vb2_buf, ret ?
>> + VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
>> + }
>> +
>> + schedule_delay = delay - (jiffies - start_jiffies);
>> +
>> + if (time_after(jiffies, start_jiffies + delay))
>> + 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);
>> + struct video_i2c_buffer *buf, *tmp;
>> +
>> + if (data->kthread_vid_cap)
>> + return 0;
>> +
>> + data->sequence = 0;
>> + data->kthread_vid_cap = kthread_run(video_i2c_thread_vid_cap, data,
>> + "%s-vid-cap", data->v4l2_dev.name);
>> + if (!IS_ERR(data->kthread_vid_cap))
>> + return 0;
>> +
>> + spin_lock(&data->slock);
>> +
>> + 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);
>
> What's the purpose of this?
This is the error path (kthread_run failed), so all buffers need to be
returned to vb2.
>
>> + }
>> +
>> + spin_unlock(&data->slock);
>> +
>> + return PTR_ERR(data->kthread_vid_cap);
>
> How about 0 instead?
This is the error path, so the error should be returned here. This code
is correct.
>
>> +}
>> +
>> +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;
>> +
>> + kthread_stop(data->kthread_vid_cap);
>> +
>> + spin_lock(&data->slock);
>> +
>> + while (!list_empty(&data->vid_cap_active)) {
>> + struct video_i2c_buffer *buf;
>> +
>> + buf = list_entry(data->vid_cap_active.next,
>
> list_last_entry(&data->vid_cap_active, ...)?
Why? You're deleting the list, so whether you pick the first
or last element really doesn't matter.
>
>> + struct video_i2c_buffer, list);
>> + list_del(&buf->list);
>> + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>> + }
>> + spin_unlock(&data->slock);
>> +
>> + data->kthread_vid_cap = NULL;
>
> How about moving the assignment right after kthread_stop()? The pointer is
> no longer valid after that anyway.
>
>> +}
>> +
>> +static struct vb2_ops video_i2c_video_qops = {
>> + .queue_setup = queue_setup,
>> + .buf_prepare = buffer_prepare,
>> + .buf_queue = buffer_queue,
>> + .start_streaming = start_streaming,
>> + .stop_streaming = stop_streaming,
>> + .wait_prepare = vb2_ops_wait_prepare,
>> + .wait_finish = vb2_ops_wait_finish,
>> +};
>> +
>> +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));
>> + sprintf(vcap->card, "I2C %d-%d Transport Video",
>> + client->adapter->nr, client->addr);
>
> I wonder if the chip name would be nice here. Up to you.
>
>> +
>> + sprintf(vcap->bus_info, "I2C:%d-%d", client->adapter->nr, client->addr);
>> + return 0;
>> +}
>> +
>> +static int video_i2c_g_input(struct file *file, void *fh, unsigned int *inp)
>> +{
>> + *inp = 0;
>> +
>> + return 0;
>> +}
>> +
>> +static int video_i2c_s_input(struct file *file, void *fh, unsigned int inp)
>> +{
>> + return (inp > 0) ? -EINVAL : 0;
>> +}
>> +
>> +static int video_i2c_enum_input(struct file *file, void *fh,
>> + struct v4l2_input *vin)
>> +{
>> + if (vin->index > 0)
>> + return -EINVAL;
>> +
>> + strlcpy(vin->name, "Camera", sizeof(vin->name));
>> +
>> + vin->type = V4L2_INPUT_TYPE_CAMERA;
>> +
>> + return 0;
>> +}
>> +
>> +static int video_i2c_enum_fmt_vid_cap(struct file *file, void *fh,
>> + struct v4l2_fmtdesc *fmt)
>> +{
>> + struct video_i2c_data *data = video_drvdata(file);
>> + enum v4l2_buf_type type = fmt->type;
>> +
>> + if (fmt->index > 0)
>> + return -EINVAL;
>> +
>> + *fmt = *data->chip->format;
>> + fmt->type = type;
>> +
>> + return 0;
>> +}
>> +
>> +static int video_i2c_enum_framesizes(struct file *file, void *fh,
>> + struct v4l2_frmsizeenum *fsize)
>> +{
>> + const struct video_i2c_data *data = video_drvdata(file);
>> + const struct v4l2_frmsize_discrete *size = data->chip->size;
>> +
>> + /* currently only one frame size is allowed */
>> + if (fsize->index > 0)
>> + return -EINVAL;
>> +
>> + if (fsize->pixel_format != data->chip->format->pixelformat)
>> + return -EINVAL;
>> +
>> + fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
>> + fsize->discrete.width = size->width;
>> + fsize->discrete.height = size->height;
>> +
>> + return 0;
>> +}
>> +
>> +static int video_i2c_enum_frameintervals(struct file *file, void *priv,
>> + struct v4l2_frmivalenum *fe)
>> +{
>> + const struct video_i2c_data *data = video_drvdata(file);
>> + const struct v4l2_frmsize_discrete *size = data->chip->size;
>> +
>> + if (fe->index > 0)
>> + return -EINVAL;
>> +
>> + if (fe->width != size->width || fe->height != size->height)
>> + return -EINVAL;
>> +
>> + fe->type = V4L2_FRMIVAL_TYPE_DISCRETE;
>> + fe->discrete.numerator = 1;
>> + fe->discrete.denominator = data->chip->max_fps;
>> +
>> + return 0;
>> +}
>> +
>> +static int video_i2c_try_fmt_vid_cap(struct file *file, void *fh,
>> + struct v4l2_format *fmt)
>> +{
>> + const struct video_i2c_data *data = video_drvdata(file);
>> + const struct v4l2_frmsize_discrete *size = data->chip->size;
>> + struct v4l2_pix_format *pix = &fmt->fmt.pix;
>> + unsigned int bpp = data->chip->bpp / 8;
>> +
>> + pix->width = size->width;
>> + pix->height = size->height;
>> + pix->pixelformat = data->chip->format->pixelformat;
>> + pix->field = V4L2_FIELD_NONE;
>> + pix->bytesperline = pix->width * bpp;
>> + pix->sizeimage = pix->bytesperline * pix->height;
>> + pix->colorspace = V4L2_COLORSPACE_RAW;
>> +
>> + return 0;
>> +}
>> +
>> +static int video_i2c_s_fmt_vid_cap(struct file *file, void *fh,
>> + struct v4l2_format *fmt)
>> +{
>> + struct video_i2c_data *data = video_drvdata(file);
>> +
>> + if (vb2_is_busy(&data->vb_vidq))
>> + return -EBUSY;
>> +
>> + return video_i2c_try_fmt_vid_cap(file, fh, fmt);
>> +}
>> +
>> +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 const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
>> + .vidioc_querycap = video_i2c_querycap,
>> + .vidioc_g_input = video_i2c_g_input,
>> + .vidioc_s_input = video_i2c_s_input,
>> + .vidioc_enum_input = video_i2c_enum_input,
>> + .vidioc_enum_fmt_vid_cap = video_i2c_enum_fmt_vid_cap,
>> + .vidioc_enum_framesizes = video_i2c_enum_framesizes,
>> + .vidioc_enum_frameintervals = video_i2c_enum_frameintervals,
>> + .vidioc_g_fmt_vid_cap = video_i2c_try_fmt_vid_cap,
>> + .vidioc_s_fmt_vid_cap = video_i2c_s_fmt_vid_cap,
>> + .vidioc_g_parm = video_i2c_g_parm,
>> + .vidioc_s_parm = video_i2c_g_parm,
>> + .vidioc_try_fmt_vid_cap = video_i2c_try_fmt_vid_cap,
>> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
>> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
>> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
>> + .vidioc_querybuf = vb2_ioctl_querybuf,
>> + .vidioc_qbuf = vb2_ioctl_qbuf,
>> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
>> + .vidioc_streamon = vb2_ioctl_streamon,
>> + .vidioc_streamoff = vb2_ioctl_streamoff,
>> +};
>> +
>> +static void video_i2c_release(struct video_device *vdev)
>> +{
>> + kfree(video_get_drvdata(vdev));
>> +}
>> +
>> +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 chip_id, ret;
>> +
>> + data = kzalloc(sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + if (client->dev.of_node)
>> + chip_id = (long) of_device_get_match_data(&client->dev);
>
> We now have device_get_match_data(). Please use that instead. It's also
> safe for NULL:
>
> chip_id = device_get_match_data(&client->dev);
> if (!chip_id)
> chip_id = id->driver_data;
>
> I wonder if it'd be actually cleaner (and easier to maintain) if you used
> pointer to the chip specific struct here instead of giving the chips
> special IDs. You could omit the chip IDs altogether.
>
>> + else
>> + chip_id = id->driver_data;
>
> Do you need to support the non-OF case? If not, you could switch to
> .probe_new callback and omit the I²C ID table.
>
>> +
>> + data->chip = &video_i2c_chip[chip_id];
>> + 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_DMABUF | VB2_MMAP | VB2_USERPTR | VB2_READ;
>> + queue->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> + 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_unregister_device;
>> +
>> + data->vdev.queue = queue;
>> + data->vdev.queue->lock = &data->queue_lock;
>> +
>> + snprintf(data->vdev.name, sizeof(data->vdev.name),
>> + "I2C %d-%d Transport Video",
>> + client->adapter->nr, client->addr);
>
> You could use this string in querycap implementation as it's the same.
>
>> +
>> + 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);
>
> mutex_destroy() for both mutexes. Same in remove below.
>
>> +
>> +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);
>> +
>> + video_unregister_device(&data->vdev);
>> + v4l2_device_unregister(&data->v4l2_dev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id video_i2c_id_table[] = {
>> + { "amg88xx", AMG88XX },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, video_i2c_id_table);
>> +
>> +static const struct of_device_id video_i2c_of_match[] = {
>> + { .compatible = "panasonic,amg88xx", .data = (void *) AMG88XX },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, video_i2c_of_match);
>> +
>> +static struct i2c_driver video_i2c_driver = {
>> + .driver = {
>> + .name = VIDEO_I2C_DRIVER,
>> + .of_match_table = of_match_ptr(video_i2c_of_match),
>
> No need to use of_match_ptr macro, a direct assignment will do.
>
>> + },
>> + .probe = video_i2c_probe,
>> + .remove = video_i2c_remove,
>> + .id_table = video_i2c_id_table,
>> +};
>> +
>> +module_i2c_driver(video_i2c_driver);
>> +
>> +MODULE_AUTHOR("Matt Ranostay <matt.ranostay@xxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("I2C transport video support");
>> +MODULE_LICENSE("GPL");
>
> "GPL v2"? Or adjust the SPDX header accordingly.
>
Regards,
Hans
