Re: [PATCH v3] media: i2c-polling: add i2c-polling driver

[Laurent Pinchart]

Hi Matt,

On Wednesday 23 Nov 2016 18:37:29 Matt Ranostay wrote:
> On Wed, Nov 23, 2016 at 8:30 AM, Laurent Pinchart wrote:
> > On Tuesday 22 Nov 2016 17:18:40 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>
> >> Signed-off-by: Matt Ranostay <matt@xxxxxxxxxxxxxxxxxxx>
> >> ---
> >> 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
> >> 
> >>  drivers/media/i2c/Kconfig       |   8 +
> >>  drivers/media/i2c/Makefile      |   1 +
> >>  drivers/media/i2c/i2c-polling.c | 469 ++++++++++++++++++++++++++++++++++
> > 
> > Just looking at the driver name I believe a rename is needed. i2c-polling
> > is a very generic name and would mislead many people into thinking about
> > an I2C subsystem core feature instead of a video driver. "video-i2c" is
> > one option, I'm open to other ideas.
> > 
> >>  3 files changed, 478 insertions(+)
> >>  create mode 100644 drivers/media/i2c/i2c-polling.c

[snip]

> >> diff --git a/drivers/media/i2c/i2c-polling.c
> >> b/drivers/media/i2c/i2c-polling.c new file mode 100644
> >> index 000000000000..46a4eecde2d2
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/i2c-polling.c

[snip]

> >> +static const struct v4l2_ioctl_ops i2c_polling_ioctl_ops = {
> >> +     .vidioc_querycap                = i2c_polling_querycap,
> >> +     .vidioc_g_input                 = i2c_polling_g_input,
> >> +     .vidioc_s_input                 = i2c_polling_s_input,
> >> +     .vidioc_enum_input              = i2c_polling_enum_input,
> >> +     .vidioc_enum_fmt_vid_cap        = i2c_polling_enum_fmt_vid_cap,
> >> +     .vidioc_enum_framesizes         = i2c_polling_enum_framesizes,
> >> +     .vidioc_enum_frameintervals     = i2c_polling_enum_frameintervals,
> >> +     .vidioc_g_fmt_vid_cap           = i2c_polling_fmt_vid_cap,
> >> +     .vidioc_s_fmt_vid_cap           = i2c_polling_fmt_vid_cap,
> >> +     .vidioc_g_parm                  = i2c_polling_g_parm,
> >> +     .vidioc_s_parm                  = i2c_polling_s_parm,
> >> +     .vidioc_try_fmt_vid_cap         = i2c_polling_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,
> > 
> > No need to set the buffer-related .vidioc_* pointers to vb2_ioctl_*
> > explicitly, the core will use vb2 if the fields are left unset.
> 
> Not so sure about that from getting these ioctl errors with those removed:
> 
> avconv -f video4linux2 -s 8x8 -r 10 -i /dev/video0 test%3d.png
> avconv version 8cd084d, Copyright (c) 2000-2016 the Libav developers
>   built on Nov  8 2016 02:26:17 with gcc 4.7.3 (Linaro GCC
> 4.7-2013.02-01) 20130205 (prerelease)
> [video4linux2 @ 0x10390c0] ioctl(VIDIOC_QUERYBUF)
> /dev/video0: Inappropriate ioctl for device

My comment was wrong, I've mistaken it with the control-related ioctls. I'm 
very sorry about that.

> >> +     .vidioc_log_status              = v4l2_ctrl_log_status,
> >> +     .vidioc_subscribe_event         = v4l2_ctrl_subscribe_event,
> >> +     .vidioc_unsubscribe_event       = v4l2_event_unsubscribe,
> >> +};

-- 
Regards,

Laurent Pinchart