[libcamera-devel] [PATCH v2 2/2] libcamera: Introduce V4L2Device base class

[Jacopo Mondi]

Hi Kieran,

On Wed, Jun 12, 2019 at 02:50:44PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 12/06/2019 14:38, Jacopo Mondi wrote:
> > he V4L2 devices and subdevices share a few common operations,like
>
> s/he/The/
>
> > opening and closing a device node, and perform IOCTLs on the device.
> >
> > With the forthcoming introduction of support for V4L2 controls, the
> > quantity of shared code will drastically increase, as the control
> > implementation is identical for the two.
> >
> > To maximize code re-use and avoid duplications, provide a V4L2Device
> > base class which groups the common operations and members.
> >
> > The newly introduced base class provides methods to open/close a device
> > node, access the file descriptor, and perform IOCTLs on the device.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
> Thank you - this is much easier to review now :-D
>
>
> A few comments below:
>
> > ---
> >  src/libcamera/include/v4l2_device.h      |  36 ++++++
> >  src/libcamera/include/v4l2_subdevice.h   |   6 +-
> >  src/libcamera/include/v4l2_videodevice.h |   5 +-
> >  src/libcamera/meson.build                |   2 +
> >  src/libcamera/v4l2_device.cpp            | 148 +++++++++++++++++++++++
> >  src/libcamera/v4l2_subdevice.cpp         |  66 +++-------
> >  src/libcamera/v4l2_videodevice.cpp       |  68 ++++-------
> >  7 files changed, 227 insertions(+), 104 deletions(-)
> >  create mode 100644 src/libcamera/include/v4l2_device.h
> >  create mode 100644 src/libcamera/v4l2_device.cpp
> >
>

[snip]

> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int V4L2Device::ioctl(unsigned long request, void *argp)
> > +{
> > +	/*
> > +	 * Printing out an error message is usually better performed
> > +	 * in the caller, which can provide more context.
> > +	 */
> > +	if (::ioctl(fd_, request, argp) < 0)
> > +		return -errno;
>
> I /really/ like this ... but note how many of the call sites for
> V4L2Device::ioctl() still work with the -errno fuss, which should now
> just deal directly with 'ret' where it's used.
>
> 'man ioctl' states:
>
> RETURN VALUE
>        Usually,  on  success zero is returned.  A few ioctl() requests
>        use the return value as an output parameter and return a
>        nonnegative value on success.  On error, -1 is returned, and
>        errno is set appropriately.
>
>
> I really like simplifying the -1, errno check pattern that is
> everywhere. But do we need to be sure we'll never need the return code
> of the ioctl?
>
> (I don't think we do - and we /could/ have a different call for that if
> necessary)
>

I agree, I think we're safe if we check for < 0 as it is done now.

>
>
> > +
> > +	return 0;
> > +}
> >

[snip]

> >  /**
> > @@ -462,7 +440,7 @@ int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> >  	int ret;
> >
> >  	v4l2Format.type = bufferType_;
> > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
>
>
> I really like that ioctl now knows about the FD internally and thus is
> simpler :)
>
> "do this action with this structure..."
>
>
> >  	if (ret) {
> >  		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
>
> This is an example of things that I think should now be:
>
> ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> if (ret) {
> 	LOG(V4L2, Error) << "Unable to get format: " << strerror(ret);
> 	...
> }
>
> That probably applies to all uses of V4L2Device::ioctl()

Yes indeed. I'll fix and resend.

Thanks
   j
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190612/d75de9ec/attachment.sig>