[libcamera-devel,v5,2/4] libcamera: Introduce V4L2Device base class

1470 diff mbox series

The V4L2 devices and subdevices share a few common operations,like
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 increase, as the control support
implementation is identical for the two derived classes.

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@jmondi.org>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/include/v4l2_device.h      |  43 +++++++
 src/libcamera/include/v4l2_subdevice.h   |   9 +-
 src/libcamera/include/v4l2_videodevice.h |  10 +-
 src/libcamera/meson.build                |   2 +
 src/libcamera/v4l2_device.cpp            | 144 +++++++++++++++++++++++
 src/libcamera/v4l2_subdevice.cpp         |  84 +++----------
 src/libcamera/v4l2_videodevice.cpp       | 103 +++++-----------
 7 files changed, 239 insertions(+), 156 deletions(-)
 create mode 100644 src/libcamera/include/v4l2_device.h
 create mode 100644 src/libcamera/v4l2_device.cpp

Hi Jacopo,

Thank you for the patch.

On Wed, Jun 19, 2019 at 01:05:46PM +0200, Jacopo Mondi wrote:
> The V4L2 devices and subdevices share a few common operations,like
> 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 increase, as the control support
> implementation is identical for the two derived classes.
> 
> 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@jmondi.org>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/include/v4l2_device.h      |  43 +++++++
>  src/libcamera/include/v4l2_subdevice.h   |   9 +-
>  src/libcamera/include/v4l2_videodevice.h |  10 +-
>  src/libcamera/meson.build                |   2 +
>  src/libcamera/v4l2_device.cpp            | 144 +++++++++++++++++++++++
>  src/libcamera/v4l2_subdevice.cpp         |  84 +++----------
>  src/libcamera/v4l2_videodevice.cpp       | 103 +++++-----------
>  7 files changed, 239 insertions(+), 156 deletions(-)
>  create mode 100644 src/libcamera/include/v4l2_device.h
>  create mode 100644 src/libcamera/v4l2_device.cpp
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> new file mode 100644
> index 000000000000..2c26f3eae2b4
> --- /dev/null
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_device.h - Common base for V4L2 video devices and subdevices
> + */
> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> +#define __LIBCAMERA_V4L2_DEVICE_H__
> +
> +#include <string>
> +
> +#include "log.h"
> +
> +namespace libcamera {
> +
> +class V4L2Device : protected Loggable
> +{
> +public:
> +	void close();
> +	bool isOpen() const { return fd_ != -1; }
> +	const std::string &deviceNode() const { return deviceNode_; }
> +
> +protected:
> +	V4L2Device(const std::string &deviceNode, const std::string &logTag);
> +	~V4L2Device() {}
> +
> +	int fd() { return fd_; }
> +
> +	int open(unsigned int flags);
> +
> +	int ioctl(unsigned long request, void *argp);
> +
> +	std::string logPrefix() const { return "'" + logTag_ + "'"; }

Let's try not to inline methods that are more complex than just
returning a member of the class.

I didn't notice in my previous review that V4L2Subdevice was using the
entity name as a prefix. I'm afraid my comment about moving logPrefix()
to V4L2Device was a bad one :-/ However, V4L2Device should still inherit
from Loggable. It will thus have a pure virtual logPrefix() inherited
from Loggable, requiring the derived classes to implement that method,
but allowing LOG() statements in V4L2Device to use the log prefix.

Here's the diff I came up with, it can be squashed with this patch if
you agree on the comments.

diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
index 2c26f3eae2b4..8c83adab4d43 100644
--- a/src/libcamera/include/v4l2_device.h
+++ b/src/libcamera/include/v4l2_device.h
@@ -21,7 +21,7 @@ public:
 	const std::string &deviceNode() const { return deviceNode_; }

 protected:
-	V4L2Device(const std::string &deviceNode, const std::string &logTag);
+	V4L2Device(const std::string &deviceNode);
 	~V4L2Device() {}

 	int fd() { return fd_; }
@@ -30,11 +30,8 @@ protected:

 	int ioctl(unsigned long request, void *argp);

-	std::string logPrefix() const { return "'" + logTag_ + "'"; }
-
 private:
 	std::string deviceNode_;
-	std::string logTag_;
 	int fd_;
 };

diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
index b1da4a87c553..9c077674f997 100644
--- a/src/libcamera/include/v4l2_subdevice.h
+++ b/src/libcamera/include/v4l2_subdevice.h
@@ -52,6 +52,9 @@ public:
 	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
 					     const std::string &entity);

+protected:
+	std::string logPrefix() const;
+
 private:
 	std::vector<unsigned int> enumPadCodes(unsigned int pad);
 	std::vector<SizeRange> enumPadSizes(unsigned int pad,
diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
index e73dec13f847..734b34f1dc53 100644
--- a/src/libcamera/include/v4l2_videodevice.h
+++ b/src/libcamera/include/v4l2_videodevice.h
@@ -147,6 +147,9 @@ public:
 	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
 					       const std::string &entity);

+protected:
+	std::string logPrefix() const;
+
 private:
 	int getFormatMeta(V4L2DeviceFormat *format);
 	int setFormatMeta(V4L2DeviceFormat *format);
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index eeed0a70fb0e..1f113780ba45 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -68,17 +68,15 @@ void V4L2Device::close()
 /**
  * \brief Construct a V4L2Device
  * \param[in] deviceNode The device node filesystem path
- * \param[in] logTag The tag to prefix log messages with. Helpful to identify
- * the device in the log output
  *
  * The V4L2Device constructor is protected and can only be accessed by the
  * V4L2VideoDevice and V4L2Subdevice derived classes.
  *
  * Initialize the file descriptor to -1 and store the \a deviceNode to be used
  * at open() time, and the \a logTag to prefix log messages with.
  */
-V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
-	: deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
+V4L2Device::V4L2Device(const std::string &deviceNode)
+	: deviceNode_(deviceNode), fd_(-1)
 {
 }

@@ -99,6 +94,7 @@ V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
  */
 int V4L2Device::open(unsigned int flags)
 {
+	LOG(V4L2, Info) << "Opening device";
 	if (isOpen()) {
 		LOG(V4L2, Error) << "Device already open";
 		return -EBUSY;
@@ -135,10 +131,4 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
 	return 0;
 }

-/**
- * \fn V4L2Device::logPrefix()
- * \brief Retrieve the tag to prefix log messages with
- * \return The tag to prefix log messages with
- */
-
 } /* namespace libcamera */
diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
index d0e1d717b26c..a188298de34c 100644
--- a/src/libcamera/v4l2_subdevice.cpp
+++ b/src/libcamera/v4l2_subdevice.cpp
@@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
  * path
  */
 V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
-	: V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
+	: V4L2Device(entity->deviceNode()), entity_(entity)
 {
 }

@@ -265,6 +265,11 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
 	return new V4L2Subdevice(mediaEntity);
 }

+std::string V4L2Subdevice::logPrefix() const
+{
+	return "'" + entity_->name() + "'";
+}
+
 std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
 {
 	std::vector<unsigned int> codes;
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 403e0b2e0653..2de3751f467e 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
  * \param[in] deviceNode The file-system path to the video device node
  */
 V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
-	: V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
+	: V4L2Device(deviceNode), bufferPool_(nullptr),
 	  queuedBuffersCount_(0), fdEvent_(nullptr)
 {
 	/*
@@ -389,6 +389,11 @@ void V4L2VideoDevice::close()
  * \return The string containing the device location
  */

+std::string V4L2VideoDevice::logPrefix() const
+{
+	return deviceNode() + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
+}
+
 /**
  * \brief Retrieve the image format set on the V4L2 device
  * \param[out] format The image format applied on the device

> +
> +private:
> +	std::string deviceNode_;
> +	std::string logTag_;
> +	int fd_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index 9afd28b6db02..b1da4a87c553 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -16,6 +16,7 @@
>  #include "formats.h"
>  #include "log.h"
>  #include "media_object.h"
> +#include "v4l2_device.h"
>  
>  namespace libcamera {
>  
> @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {
>  	const std::string toString() const;
>  };
>  
> -class V4L2Subdevice : protected Loggable
> +class V4L2Subdevice : public V4L2Device
>  {
>  public:
>  	explicit V4L2Subdevice(const MediaEntity *entity);
> @@ -37,8 +38,6 @@ public:
>  	~V4L2Subdevice();
>  
>  	int open();
> -	bool isOpen() const;
> -	void close();
>  
>  	const MediaEntity *entity() const { return entity_; }
>  
> @@ -53,9 +52,6 @@ public:
>  	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
>  					     const std::string &entity);
>  
> -protected:
> -	std::string logPrefix() const;
> -
>  private:
>  	std::vector<unsigned int> enumPadCodes(unsigned int pad);
>  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> @@ -65,7 +61,6 @@ private:
>  			 Rectangle *rect);
>  
>  	const MediaEntity *entity_;
> -	int fd_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 895658805b47..e73dec13f847 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -18,6 +18,7 @@
>  
>  #include "formats.h"
>  #include "log.h"
> +#include "v4l2_device.h"
>  
>  namespace libcamera {
>  
> @@ -112,7 +113,7 @@ public:
>  	const std::string toString() const;
>  };
>  
> -class V4L2VideoDevice : protected Loggable
> +class V4L2VideoDevice : public V4L2Device
>  {
>  public:
>  	explicit V4L2VideoDevice(const std::string &deviceNode);
> @@ -123,13 +124,11 @@ public:
>  	V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
>  
>  	int open();
> -	bool isOpen() const;
>  	void close();
>  
>  	const char *driverName() const { return caps_.driver(); }
>  	const char *deviceName() const { return caps_.card(); }
>  	const char *busName() const { return caps_.bus_info(); }
> -	const std::string &deviceNode() const { return deviceNode_; }
>  
>  	int getFormat(V4L2DeviceFormat *format);
>  	int setFormat(V4L2DeviceFormat *format);
> @@ -148,9 +147,6 @@ public:
>  	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
>  					       const std::string &entity);
>  
> -protected:
> -	std::string logPrefix() const;
> -
>  private:
>  	int getFormatMeta(V4L2DeviceFormat *format);
>  	int setFormatMeta(V4L2DeviceFormat *format);
> @@ -171,8 +167,6 @@ private:
>  	Buffer *dequeueBuffer();
>  	void bufferAvailable(EventNotifier *notifier);
>  
> -	std::string deviceNode_;
> -	int fd_;
>  	V4L2Capability caps_;
>  
>  	enum v4l2_buf_type bufferType_;
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 15ab53b1abbe..f26ad5b2dc57 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -23,6 +23,7 @@ libcamera_sources = files([
>      'stream.cpp',
>      'timer.cpp',
>      'utils.cpp',
> +    'v4l2_device.cpp',
>      'v4l2_subdevice.cpp',
>      'v4l2_videodevice.cpp',
>  ])
> @@ -41,6 +42,7 @@ libcamera_headers = files([
>      'include/media_object.h',
>      'include/pipeline_handler.h',
>      'include/utils.h',
> +    'include/v4l2_device.h',
>      'include/v4l2_subdevice.h',
>      'include/v4l2_videodevice.h',
>  ])
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> new file mode 100644
> index 000000000000..eeed0a70fb0e
> --- /dev/null
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -0,0 +1,144 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_device.cpp - Common base for V4L2 video devices and subdevices
> + */
> +
> +#include "v4l2_device.h"
> +
> +#include <fcntl.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +
> +#include "log.h"
> +
> +/**
> + * \file v4l2_device.h
> + * \brief Common base for V4L2 devices and subdevices
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(V4L2)
> +
> +/**
> + * \class V4L2Device
> + * \brief Base class for V4L2VideoDevice and V4L2Subdevice
> + *
> + * The V4L2Device class groups together the methods and fields common to
> + * both the V4L2VideoDevice and V4L2Subdevice classes, and provides a base
> + * class whith methods to open and close the device node associated with the
> + * device and to perform IOCTL system calls on it.
> + *
> + * The V4L2Device class cannot be instantiated directly, as its constructor
> + * is protected. Users should instead create instances of one the derived
> + * classes to model either a V4L2 video device or a V4L2 subdevice.
> + */
> +
> +/**
> + * \brief Close the device node
> + *
> + * Reset the file descriptor to -1
> + */
> +void V4L2Device::close()
> +{
> +	if (!isOpen())
> +		return;
> +
> +	if (::close(fd_) < 0)
> +		LOG(V4L2, Error) << "Failed to close V4L2 device: "
> +				 << strerror(errno);
> +	fd_ = -1;
> +}
> +
> +/**
> + * \fn V4L2Device::isOpen()
> + * \brief Check if the V4L2 device node is open
> + * \return True if the V4L2 device node is open, false otherwise
> + */
> +
> +/**
> + * \fn V4L2Device::deviceNode()
> + * \brief Retrieve the device node path
> + * \return The device node path
> + */
> +
> +/**
> + * \brief Construct a V4L2Device
> + * \param[in] deviceNode The device node filesystem path
> + * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> + * the device in the log output
> + *
> + * The V4L2Device constructor is protected and can only be accessed by the
> + * V4L2VideoDevice and V4L2Subdevice derived classes.

I would drop this sentence.

> + *
> + * Initialize the file descriptor to -1 and store the \a deviceNode to be used
> + * at open() time, and the \a logTag to prefix log messages with.
> + */
> +V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> +	: deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
> +{
> +}
> +
> +/**
> + * \fn V4L2Device::fd()
> + * \brief Retrieve the V4L2 device file descriptor
> + * \return The V4L2 device file descriptor, -1 if the device node is not open
> + */
> +
> +/**
> + * \brief Open a V4L2 device node
> + * \param[in] flags Access mode flags
> + *
> + * Open the device node path with the provided access mode \a flags and
> + * initialize the file descriptor, which was initially set to -1.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2Device::open(unsigned int flags)
> +{
> +	if (isOpen()) {
> +		LOG(V4L2, Error) << "Device already open";
> +		return -EBUSY;
> +	}
> +
> +	int ret = ::open(deviceNode_.c_str(), flags);
> +	if (ret < 0) {
> +		ret = -errno;
> +		LOG(V4L2, Error) << "Failed to open V4L2 device: "
> +				 << strerror(-ret);
> +		return ret;
> +	}
> +
> +	fd_ = ret;
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Perform an IOCTL system call on the device node
> + * \param[in] request The IOCTL request code
> + * \param[in] argp A pointer to the IOCTL argument
> + * \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;
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn V4L2Device::logPrefix()
> + * \brief Retrieve the tag to prefix log messages with
> + * \return The tag to prefix log messages with
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 48f1fcb30ed4..d0e1d717b26c 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -29,7 +29,7 @@
>  
>  namespace libcamera {
>  
> -LOG_DEFINE_CATEGORY(V4L2Subdev)
> +LOG_DECLARE_CATEGORY(V4L2)
>  
>  /**
>   * \struct V4L2SubdeviceFormat
> @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
>   * path
>   */
>  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> -	: entity_(entity), fd_(-1)
> +	: V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
>  {
>  }
>  
> @@ -117,45 +117,7 @@ V4L2Subdevice::~V4L2Subdevice()
>   */
>  int V4L2Subdevice::open()
>  {
> -	int ret;
> -
> -	if (isOpen()) {
> -		LOG(V4L2Subdev, Error) << "Device already open";
> -		return -EBUSY;
> -	}
> -
> -	ret = ::open(entity_->deviceNode().c_str(), O_RDWR);
> -	if (ret < 0) {
> -		ret = -errno;
> -		LOG(V4L2Subdev, Error)
> -			<< "Failed to open V4L2 subdevice '"
> -			<< entity_->deviceNode() << "': " << strerror(-ret);
> -		return ret;
> -	}
> -	fd_ = ret;
> -
> -	return 0;
> -}
> -
> -/**
> - * \brief Check if the subdevice is open
> - * \return True if the subdevice is open, false otherwise
> - */
> -bool V4L2Subdevice::isOpen() const
> -{
> -	return fd_ != -1;
> -}
> -
> -/**
> - * \brief Close the subdevice, releasing any resources acquired by open()
> - */
> -void V4L2Subdevice::close()
> -{
> -	if (!isOpen())
> -		return;
> -
> -	::close(fd_);
> -	fd_ = -1;
> +	return V4L2Device::open(O_RDWR);
>  }
>  
>  /**
> @@ -200,7 +162,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
>  	ImageFormats formats;
>  
>  	if (pad >= entity_->pads().size()) {
> -		LOG(V4L2Subdev, Error) << "Invalid pad: " << pad;
> +		LOG(V4L2, Error) << "Invalid pad: " << pad;
>  		return {};
>  	}
>  
> @@ -210,7 +172,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
>  			return {};
>  
>  		if (formats.addFormat(code, sizes)) {
> -			LOG(V4L2Subdev, Error)
> +			LOG(V4L2, Error)
>  				<< "Could not add sizes for media bus code "
>  				<< code << " on pad " << pad;
>  			return {};
> @@ -232,10 +194,9 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)
>  	subdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  	subdevFmt.pad = pad;
>  
> -	int ret = ioctl(fd_, VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> +	int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
>  	if (ret) {
> -		ret = -errno;
> -		LOG(V4L2Subdev, Error)
> +		LOG(V4L2, Error)
>  			<< "Unable to get format on pad " << pad
>  			<< ": " << strerror(-ret);
>  		return ret;
> @@ -268,10 +229,9 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
>  	subdevFmt.format.height = format->size.height;
>  	subdevFmt.format.code = format->mbus_code;
>  
> -	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> +	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
>  	if (ret) {
> -		ret = -errno;
> -		LOG(V4L2Subdev, Error)
> +		LOG(V4L2, Error)
>  			<< "Unable to set format on pad " << pad
>  			<< ": " << strerror(-ret);
>  		return ret;
> @@ -305,11 +265,6 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
>  	return new V4L2Subdevice(mediaEntity);
>  }
>  
> -std::string V4L2Subdevice::logPrefix() const
> -{
> -	return "'" + entity_->name() + "'";
> -}
> -
>  std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
>  {
>  	std::vector<unsigned int> codes;
> @@ -321,18 +276,17 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
>  		mbusEnum.index = index;
>  		mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  
> -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> +		ret = ioctl(VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
>  		if (ret)
>  			break;
>  
>  		codes.push_back(mbusEnum.code);
>  	}
>  
> -	if (ret && errno != EINVAL) {
> -		ret = errno;
> -		LOG(V4L2Subdev, Error)
> +	if (ret < 0 && ret != -EINVAL) {
> +		LOG(V4L2, Error)
>  			<< "Unable to enumerate formats on pad " << pad
> -			<< ": " << strerror(ret);
> +			<< ": " << strerror(-ret);
>  		return {};
>  	}
>  
> @@ -352,7 +306,7 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
>  		sizeEnum.code = code;
>  		sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  
> -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> +		ret = ioctl(VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
>  		if (ret)
>  			break;
>  
> @@ -360,9 +314,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
>  				   sizeEnum.max_width, sizeEnum.max_height);
>  	}
>  
> -	if (ret && (errno != EINVAL && errno != ENOTTY)) {
> -		ret = -errno;
> -		LOG(V4L2Subdev, Error)
> +	if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> +		LOG(V4L2, Error)
>  			<< "Unable to enumerate sizes on pad " << pad
>  			<< ": " << strerror(-ret);
>  		return {};
> @@ -386,10 +339,9 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
>  	sel.r.width = rect->w;
>  	sel.r.height = rect->h;
>  
> -	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);
> +	int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);
>  	if (ret < 0) {
> -		ret = -errno;
> -		LOG(V4L2Subdev, Error)
> +		LOG(V4L2, Error)
>  			<< "Unable to set rectangle " << target << " on pad "
>  			<< pad << ": " << strerror(-ret);
>  		return ret;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 0e70498c8bb1..403e0b2e0653 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -30,7 +30,7 @@
>   */
>  namespace libcamera {
>  
> -LOG_DEFINE_CATEGORY(V4L2)
> +LOG_DECLARE_CATEGORY(V4L2)
>  
>  /**
>   * \struct V4L2Capability
> @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
>   * \param[in] deviceNode The file-system path to the video device node
>   */
>  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> -	: deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),
> +	: V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
>  	  queuedBuffersCount_(0), fdEvent_(nullptr)
>  {
>  	/*
> @@ -305,23 +305,12 @@ int V4L2VideoDevice::open()
>  {
>  	int ret;
>  
> -	if (isOpen()) {
> -		LOG(V4L2, Error) << "Device already open";
> -		return -EBUSY;
> -	}
> -
> -	ret = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);
> -	if (ret < 0) {
> -		ret = -errno;
> -		LOG(V4L2, Error)
> -			<< "Failed to open V4L2 device: " << strerror(-ret);
> +	ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> +	if (ret < 0)
>  		return ret;
> -	}
> -	fd_ = ret;
>  
> -	ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
> +	ret = ioctl(VIDIOC_QUERYCAP, &caps_);
>  	if (ret < 0) {
> -		ret = -errno;
>  		LOG(V4L2, Error)
>  			<< "Failed to query device capabilities: "
>  			<< strerror(-ret);
> @@ -342,20 +331,20 @@ int V4L2VideoDevice::open()
>  	 * (POLLIN), and write notifications for OUTPUT devices (POLLOUT).
>  	 */
>  	if (caps_.isVideoCapture()) {
> -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
>  		bufferType_ = caps_.isMultiplanar()
>  			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
>  			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  	} else if (caps_.isVideoOutput()) {
> -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
>  		bufferType_ = caps_.isMultiplanar()
>  			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
>  			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
>  	} else if (caps_.isMetaCapture()) {
> -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
>  		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
>  	} else if (caps_.isMetaOutput()) {
> -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
>  		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
>  	} else {
>  		LOG(V4L2, Error) << "Device is not a supported type";
> @@ -368,28 +357,18 @@ int V4L2VideoDevice::open()
>  	return 0;
>  }
>  
> -/**
> - * \brief Check if device is successfully opened
> - * \return True if the device is open, false otherwise
> - */
> -bool V4L2VideoDevice::isOpen() const
> -{
> -	return fd_ != -1;
> -}
> -
>  /**
>   * \brief Close the device, releasing any resources acquired by open()
>   */
>  void V4L2VideoDevice::close()
>  {
> -	if (fd_ < 0)
> +	if (!isOpen())
>  		return;
>  
>  	releaseBuffers();
>  	delete fdEvent_;
>  
> -	::close(fd_);
> -	fd_ = -1;
> +	V4L2Device::close();
>  }
>  
>  /**
> @@ -410,17 +389,6 @@ void V4L2VideoDevice::close()
>   * \return The string containing the device location
>   */
>  
> -/**
> - * \fn V4L2VideoDevice::deviceNode()
> - * \brief Retrieve the video device node path
> - * \return The video device device node path
> - */
> -
> -std::string V4L2VideoDevice::logPrefix() const
> -{
> -	return deviceNode_ + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> -}
> -
>  /**
>   * \brief Retrieve the image format set on the V4L2 device
>   * \param[out] format The image format applied on the device
> @@ -462,9 +430,8 @@ int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
>  	int ret;
>  
>  	v4l2Format.type = bufferType_;
> -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
>  	if (ret) {
> -		ret = -errno;
>  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
>  		return ret;
>  	}
> @@ -488,9 +455,8 @@ int V4L2VideoDevice::setFormatMeta(V4L2DeviceFormat *format)
>  	v4l2Format.type = bufferType_;
>  	pix->dataformat = format->fourcc;
>  	pix->buffersize = format->planes[0].size;
> -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
>  	if (ret) {
> -		ret = -errno;
>  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
>  		return ret;
>  	}
> @@ -516,9 +482,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
>  	int ret;
>  
>  	v4l2Format.type = bufferType_;
> -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
>  	if (ret) {
> -		ret = -errno;
>  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
>  		return ret;
>  	}
> @@ -554,9 +519,8 @@ int V4L2VideoDevice::setFormatMultiplane(V4L2DeviceFormat *format)
>  		pix->plane_fmt[i].sizeimage = format->planes[i].size;
>  	}
>  
> -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
>  	if (ret) {
> -		ret = -errno;
>  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
>  		return ret;
>  	}
> @@ -584,9 +548,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
>  	int ret;
>  
>  	v4l2Format.type = bufferType_;
> -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
>  	if (ret) {
> -		ret = -errno;
>  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
>  		return ret;
>  	}
> @@ -613,9 +576,8 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)
>  	pix->pixelformat = format->fourcc;
>  	pix->bytesperline = format->planes[0].bpl;
>  	pix->field = V4L2_FIELD_NONE;
> -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
>  	if (ret) {
> -		ret = -errno;
>  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
>  		return ret;
>  	}
> @@ -670,9 +632,8 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)
>  	rb.type = bufferType_;
>  	rb.memory = memoryType_;
>  
> -	ret = ioctl(fd_, VIDIOC_REQBUFS, &rb);
> +	ret = ioctl(VIDIOC_REQBUFS, &rb);
>  	if (ret < 0) {
> -		ret = -errno;
>  		LOG(V4L2, Error)
>  			<< "Unable to request " << count << " buffers: "
>  			<< strerror(-ret);
> @@ -722,9 +683,8 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)
>  		buf.length = VIDEO_MAX_PLANES;
>  		buf.m.planes = planes;
>  
> -		ret = ioctl(fd_, VIDIOC_QUERYBUF, &buf);
> +		ret = ioctl(VIDIOC_QUERYBUF, &buf);
>  		if (ret < 0) {
> -			ret = -errno;
>  			LOG(V4L2, Error)
>  				<< "Unable to query buffer " << i << ": "
>  				<< strerror(-ret);
> @@ -775,9 +735,8 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,
>  	expbuf.plane = planeIndex;
>  	expbuf.flags = O_RDWR;
>  
> -	ret = ioctl(fd_, VIDIOC_EXPBUF, &expbuf);
> +	ret = ioctl(VIDIOC_EXPBUF, &expbuf);
>  	if (ret < 0) {
> -		ret = -errno;
>  		LOG(V4L2, Error)
>  			<< "Failed to export buffer: " << strerror(-ret);
>  		return ret;
> @@ -801,15 +760,14 @@ std::vector<unsigned int> V4L2VideoDevice::enumPixelformats()
>  		pixelformatEnum.index = index;
>  		pixelformatEnum.type = bufferType_;
>  
> -		ret = ioctl(fd_, VIDIOC_ENUM_FMT, &pixelformatEnum);
> +		ret = ioctl(VIDIOC_ENUM_FMT, &pixelformatEnum);
>  		if (ret)
>  			break;
>  
>  		formats.push_back(pixelformatEnum.pixelformat);
>  	}
>  
> -	if (ret && errno != EINVAL) {
> -		ret = -errno;
> +	if (ret && ret != -EINVAL) {
>  		LOG(V4L2, Error)
>  			<< "Unable to enumerate pixel formats: "
>  			<< strerror(-ret);
> @@ -829,7 +787,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
>  		frameSize.index = index;
>  		frameSize.pixel_format = pixelFormat;
>  
> -		ret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize);
> +		ret = ioctl(VIDIOC_ENUM_FRAMESIZES, &frameSize);
>  		if (ret)
>  			break;
>  
> @@ -867,8 +825,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
>  		}
>  	}
>  
> -	if (ret && errno != EINVAL) {
> -		ret = -errno;
> +	if (ret && ret != -EINVAL) {
>  		LOG(V4L2, Error)
>  			<< "Unable to enumerate frame sizes: "
>  			<< strerror(-ret);
> @@ -969,9 +926,8 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
>  
>  	LOG(V4L2, Debug) << "Queueing buffer " << buf.index;
>  
> -	ret = ioctl(fd_, VIDIOC_QBUF, &buf);
> +	ret = ioctl(VIDIOC_QBUF, &buf);
>  	if (ret < 0) {
> -		ret = -errno;
>  		LOG(V4L2, Error)
>  			<< "Failed to queue buffer " << buf.index << ": "
>  			<< strerror(-ret);
> @@ -1006,9 +962,8 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
>  		buf.m.planes = planes;
>  	}
>  
> -	ret = ioctl(fd_, VIDIOC_DQBUF, &buf);
> +	ret = ioctl(VIDIOC_DQBUF, &buf);
>  	if (ret < 0) {
> -		ret = -errno;
>  		LOG(V4L2, Error)
>  			<< "Failed to dequeue buffer: " << strerror(-ret);
>  		return nullptr;
> @@ -1066,9 +1021,8 @@ int V4L2VideoDevice::streamOn()
>  {
>  	int ret;
>  
> -	ret = ioctl(fd_, VIDIOC_STREAMON, &bufferType_);
> +	ret = ioctl(VIDIOC_STREAMON, &bufferType_);
>  	if (ret < 0) {
> -		ret = -errno;
>  		LOG(V4L2, Error)
>  			<< "Failed to start streaming: " << strerror(-ret);
>  		return ret;
> @@ -1089,9 +1043,8 @@ int V4L2VideoDevice::streamOff()
>  {
>  	int ret;
>  
> -	ret = ioctl(fd_, VIDIOC_STREAMOFF, &bufferType_);
> +	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
>  	if (ret < 0) {
> -		ret = -errno;
>  		LOG(V4L2, Error)
>  			<< "Failed to stop streaming: " << strerror(-ret);
>  		return ret;

Hi Laurent,

On Wed, Jun 19, 2019 at 03:20:29PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Jun 19, 2019 at 01:05:46PM +0200, Jacopo Mondi wrote:
> > The V4L2 devices and subdevices share a few common operations,like
> > 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 increase, as the control support
> > implementation is identical for the two derived classes.
> >
> > 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@jmondi.org>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/libcamera/include/v4l2_device.h      |  43 +++++++
> >  src/libcamera/include/v4l2_subdevice.h   |   9 +-
> >  src/libcamera/include/v4l2_videodevice.h |  10 +-
> >  src/libcamera/meson.build                |   2 +
> >  src/libcamera/v4l2_device.cpp            | 144 +++++++++++++++++++++++
> >  src/libcamera/v4l2_subdevice.cpp         |  84 +++----------
> >  src/libcamera/v4l2_videodevice.cpp       | 103 +++++-----------
> >  7 files changed, 239 insertions(+), 156 deletions(-)
> >  create mode 100644 src/libcamera/include/v4l2_device.h
> >  create mode 100644 src/libcamera/v4l2_device.cpp
> >
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > new file mode 100644
> > index 000000000000..2c26f3eae2b4
> > --- /dev/null
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -0,0 +1,43 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_device.h - Common base for V4L2 video devices and subdevices
> > + */
> > +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> > +#define __LIBCAMERA_V4L2_DEVICE_H__
> > +
> > +#include <string>
> > +
> > +#include "log.h"
> > +
> > +namespace libcamera {
> > +
> > +class V4L2Device : protected Loggable
> > +{
> > +public:
> > +	void close();
> > +	bool isOpen() const { return fd_ != -1; }
> > +	const std::string &deviceNode() const { return deviceNode_; }
> > +
> > +protected:
> > +	V4L2Device(const std::string &deviceNode, const std::string &logTag);
> > +	~V4L2Device() {}
> > +
> > +	int fd() { return fd_; }
> > +
> > +	int open(unsigned int flags);
> > +
> > +	int ioctl(unsigned long request, void *argp);
> > +
> > +	std::string logPrefix() const { return "'" + logTag_ + "'"; }
>
> Let's try not to inline methods that are more complex than just
> returning a member of the class.
>

Ok. I always assumed the compiler knows better, and for simple methods
like this one, implementing it here or in the cpp file it's a matter
of tastes

> I didn't notice in my previous review that V4L2Subdevice was using the
> entity name as a prefix. I'm afraid my comment about moving logPrefix()
> to V4L2Device was a bad one :-/ However, V4L2Device should still inherit
> from Loggable. It will thus have a pure virtual logPrefix() inherited
> from Loggable, requiring the derived classes to implement that method,
> but allowing LOG() statements in V4L2Device to use the log prefix.
>

Why is this (using the entity name as log prefix) bad for you? The
tag is passed to the base class constructor. Might be a bit of a
shortcut maybe, is this your concern ?

> Here's the diff I came up with, it can be squashed with this patch if
> you agree on the comments.
>
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 2c26f3eae2b4..8c83adab4d43 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -21,7 +21,7 @@ public:
>  	const std::string &deviceNode() const { return deviceNode_; }
>
>  protected:
> -	V4L2Device(const std::string &deviceNode, const std::string &logTag);
> +	V4L2Device(const std::string &deviceNode);
>  	~V4L2Device() {}
>
>  	int fd() { return fd_; }
> @@ -30,11 +30,8 @@ protected:
>
>  	int ioctl(unsigned long request, void *argp);
>
> -	std::string logPrefix() const { return "'" + logTag_ + "'"; }
> -
>  private:
>  	std::string deviceNode_;
> -	std::string logTag_;
>  	int fd_;
>  };
>
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index b1da4a87c553..9c077674f997 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -52,6 +52,9 @@ public:
>  	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
>  					     const std::string &entity);
>
> +protected:
> +	std::string logPrefix() const;
> +
>  private:
>  	std::vector<unsigned int> enumPadCodes(unsigned int pad);
>  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index e73dec13f847..734b34f1dc53 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -147,6 +147,9 @@ public:
>  	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
>  					       const std::string &entity);
>
> +protected:
> +	std::string logPrefix() const;
> +
>  private:
>  	int getFormatMeta(V4L2DeviceFormat *format);
>  	int setFormatMeta(V4L2DeviceFormat *format);
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index eeed0a70fb0e..1f113780ba45 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -68,17 +68,15 @@ void V4L2Device::close()
>  /**
>   * \brief Construct a V4L2Device
>   * \param[in] deviceNode The device node filesystem path
> - * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> - * the device in the log output
>   *
>   * The V4L2Device constructor is protected and can only be accessed by the
>   * V4L2VideoDevice and V4L2Subdevice derived classes.
>   *
>   * Initialize the file descriptor to -1 and store the \a deviceNode to be used
>   * at open() time, and the \a logTag to prefix log messages with.
>   */
> -V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> -	: deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
> +V4L2Device::V4L2Device(const std::string &deviceNode)
> +	: deviceNode_(deviceNode), fd_(-1)
>  {
>  }
>
> @@ -99,6 +94,7 @@ V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
>   */
>  int V4L2Device::open(unsigned int flags)
>  {
> +	LOG(V4L2, Info) << "Opening device";
>  	if (isOpen()) {
>  		LOG(V4L2, Error) << "Device already open";
>  		return -EBUSY;
> @@ -135,10 +131,4 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
>  	return 0;
>  }
>
> -/**
> - * \fn V4L2Device::logPrefix()
> - * \brief Retrieve the tag to prefix log messages with
> - * \return The tag to prefix log messages with
> - */
> -
>  } /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index d0e1d717b26c..a188298de34c 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
>   * path
>   */
>  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> -	: V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
> +	: V4L2Device(entity->deviceNode()), entity_(entity)
>  {
>  }
>
> @@ -265,6 +265,11 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
>  	return new V4L2Subdevice(mediaEntity);
>  }
>
> +std::string V4L2Subdevice::logPrefix() const
> +{
> +	return "'" + entity_->name() + "'";
> +}
> +
>  std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
>  {
>  	std::vector<unsigned int> codes;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 403e0b2e0653..2de3751f467e 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
>   * \param[in] deviceNode The file-system path to the video device node
>   */
>  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> -	: V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
> +	: V4L2Device(deviceNode), bufferPool_(nullptr),
>  	  queuedBuffersCount_(0), fdEvent_(nullptr)
>  {
>  	/*
> @@ -389,6 +389,11 @@ void V4L2VideoDevice::close()
>   * \return The string containing the device location
>   */
>
> +std::string V4L2VideoDevice::logPrefix() const
> +{
> +	return deviceNode() + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> +}
> +
>  /**
>   * \brief Retrieve the image format set on the V4L2 device
>   * \param[out] format The image format applied on the device
>
> > +
> > +private:
> > +	std::string deviceNode_;
> > +	std::string logTag_;
> > +	int fd_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index 9afd28b6db02..b1da4a87c553 100644
> > --- a/src/libcamera/include/v4l2_subdevice.h
> > +++ b/src/libcamera/include/v4l2_subdevice.h
> > @@ -16,6 +16,7 @@
> >  #include "formats.h"
> >  #include "log.h"
> >  #include "media_object.h"
> > +#include "v4l2_device.h"
> >
> >  namespace libcamera {
> >
> > @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {
> >  	const std::string toString() const;
> >  };
> >
> > -class V4L2Subdevice : protected Loggable
> > +class V4L2Subdevice : public V4L2Device
> >  {
> >  public:
> >  	explicit V4L2Subdevice(const MediaEntity *entity);
> > @@ -37,8 +38,6 @@ public:
> >  	~V4L2Subdevice();
> >
> >  	int open();
> > -	bool isOpen() const;
> > -	void close();
> >
> >  	const MediaEntity *entity() const { return entity_; }
> >
> > @@ -53,9 +52,6 @@ public:
> >  	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
> >  					     const std::string &entity);
> >
> > -protected:
> > -	std::string logPrefix() const;
> > -
> >  private:
> >  	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> >  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > @@ -65,7 +61,6 @@ private:
> >  			 Rectangle *rect);
> >
> >  	const MediaEntity *entity_;
> > -	int fd_;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > index 895658805b47..e73dec13f847 100644
> > --- a/src/libcamera/include/v4l2_videodevice.h
> > +++ b/src/libcamera/include/v4l2_videodevice.h
> > @@ -18,6 +18,7 @@
> >
> >  #include "formats.h"
> >  #include "log.h"
> > +#include "v4l2_device.h"
> >
> >  namespace libcamera {
> >
> > @@ -112,7 +113,7 @@ public:
> >  	const std::string toString() const;
> >  };
> >
> > -class V4L2VideoDevice : protected Loggable
> > +class V4L2VideoDevice : public V4L2Device
> >  {
> >  public:
> >  	explicit V4L2VideoDevice(const std::string &deviceNode);
> > @@ -123,13 +124,11 @@ public:
> >  	V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
> >
> >  	int open();
> > -	bool isOpen() const;
> >  	void close();
> >
> >  	const char *driverName() const { return caps_.driver(); }
> >  	const char *deviceName() const { return caps_.card(); }
> >  	const char *busName() const { return caps_.bus_info(); }
> > -	const std::string &deviceNode() const { return deviceNode_; }
> >
> >  	int getFormat(V4L2DeviceFormat *format);
> >  	int setFormat(V4L2DeviceFormat *format);
> > @@ -148,9 +147,6 @@ public:
> >  	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
> >  					       const std::string &entity);
> >
> > -protected:
> > -	std::string logPrefix() const;
> > -
> >  private:
> >  	int getFormatMeta(V4L2DeviceFormat *format);
> >  	int setFormatMeta(V4L2DeviceFormat *format);
> > @@ -171,8 +167,6 @@ private:
> >  	Buffer *dequeueBuffer();
> >  	void bufferAvailable(EventNotifier *notifier);
> >
> > -	std::string deviceNode_;
> > -	int fd_;
> >  	V4L2Capability caps_;
> >
> >  	enum v4l2_buf_type bufferType_;
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 15ab53b1abbe..f26ad5b2dc57 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -23,6 +23,7 @@ libcamera_sources = files([
> >      'stream.cpp',
> >      'timer.cpp',
> >      'utils.cpp',
> > +    'v4l2_device.cpp',
> >      'v4l2_subdevice.cpp',
> >      'v4l2_videodevice.cpp',
> >  ])
> > @@ -41,6 +42,7 @@ libcamera_headers = files([
> >      'include/media_object.h',
> >      'include/pipeline_handler.h',
> >      'include/utils.h',
> > +    'include/v4l2_device.h',
> >      'include/v4l2_subdevice.h',
> >      'include/v4l2_videodevice.h',
> >  ])
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > new file mode 100644
> > index 000000000000..eeed0a70fb0e
> > --- /dev/null
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -0,0 +1,144 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_device.cpp - Common base for V4L2 video devices and subdevices
> > + */
> > +
> > +#include "v4l2_device.h"
> > +
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <sys/ioctl.h>
> > +#include <unistd.h>
> > +
> > +#include "log.h"
> > +
> > +/**
> > + * \file v4l2_device.h
> > + * \brief Common base for V4L2 devices and subdevices
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(V4L2)
> > +
> > +/**
> > + * \class V4L2Device
> > + * \brief Base class for V4L2VideoDevice and V4L2Subdevice
> > + *
> > + * The V4L2Device class groups together the methods and fields common to
> > + * both the V4L2VideoDevice and V4L2Subdevice classes, and provides a base
> > + * class whith methods to open and close the device node associated with the
> > + * device and to perform IOCTL system calls on it.
> > + *
> > + * The V4L2Device class cannot be instantiated directly, as its constructor
> > + * is protected. Users should instead create instances of one the derived
> > + * classes to model either a V4L2 video device or a V4L2 subdevice.
> > + */
> > +
> > +/**
> > + * \brief Close the device node
> > + *
> > + * Reset the file descriptor to -1
> > + */
> > +void V4L2Device::close()
> > +{
> > +	if (!isOpen())
> > +		return;
> > +
> > +	if (::close(fd_) < 0)
> > +		LOG(V4L2, Error) << "Failed to close V4L2 device: "
> > +				 << strerror(errno);
> > +	fd_ = -1;
> > +}
> > +
> > +/**
> > + * \fn V4L2Device::isOpen()
> > + * \brief Check if the V4L2 device node is open
> > + * \return True if the V4L2 device node is open, false otherwise
> > + */
> > +
> > +/**
> > + * \fn V4L2Device::deviceNode()
> > + * \brief Retrieve the device node path
> > + * \return The device node path
> > + */
> > +
> > +/**
> > + * \brief Construct a V4L2Device
> > + * \param[in] deviceNode The device node filesystem path
> > + * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> > + * the device in the log output
> > + *
> > + * The V4L2Device constructor is protected and can only be accessed by the
> > + * V4L2VideoDevice and V4L2Subdevice derived classes.
>
> I would drop this sentence.
>
> > + *
> > + * Initialize the file descriptor to -1 and store the \a deviceNode to be used
> > + * at open() time, and the \a logTag to prefix log messages with.
> > + */
> > +V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> > +	: deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
> > +{
> > +}
> > +
> > +/**
> > + * \fn V4L2Device::fd()
> > + * \brief Retrieve the V4L2 device file descriptor
> > + * \return The V4L2 device file descriptor, -1 if the device node is not open
> > + */
> > +
> > +/**
> > + * \brief Open a V4L2 device node
> > + * \param[in] flags Access mode flags
> > + *
> > + * Open the device node path with the provided access mode \a flags and
> > + * initialize the file descriptor, which was initially set to -1.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int V4L2Device::open(unsigned int flags)
> > +{
> > +	if (isOpen()) {
> > +		LOG(V4L2, Error) << "Device already open";
> > +		return -EBUSY;
> > +	}
> > +
> > +	int ret = ::open(deviceNode_.c_str(), flags);
> > +	if (ret < 0) {
> > +		ret = -errno;
> > +		LOG(V4L2, Error) << "Failed to open V4L2 device: "
> > +				 << strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	fd_ = ret;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \brief Perform an IOCTL system call on the device node
> > + * \param[in] request The IOCTL request code
> > + * \param[in] argp A pointer to the IOCTL argument
> > + * \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;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \fn V4L2Device::logPrefix()
> > + * \brief Retrieve the tag to prefix log messages with
> > + * \return The tag to prefix log messages with
> > + */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 48f1fcb30ed4..d0e1d717b26c 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -29,7 +29,7 @@
> >
> >  namespace libcamera {
> >
> > -LOG_DEFINE_CATEGORY(V4L2Subdev)
> > +LOG_DECLARE_CATEGORY(V4L2)
> >
> >  /**
> >   * \struct V4L2SubdeviceFormat
> > @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
> >   * path
> >   */
> >  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> > -	: entity_(entity), fd_(-1)
> > +	: V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
> >  {
> >  }
> >
> > @@ -117,45 +117,7 @@ V4L2Subdevice::~V4L2Subdevice()
> >   */
> >  int V4L2Subdevice::open()
> >  {
> > -	int ret;
> > -
> > -	if (isOpen()) {
> > -		LOG(V4L2Subdev, Error) << "Device already open";
> > -		return -EBUSY;
> > -	}
> > -
> > -	ret = ::open(entity_->deviceNode().c_str(), O_RDWR);
> > -	if (ret < 0) {
> > -		ret = -errno;
> > -		LOG(V4L2Subdev, Error)
> > -			<< "Failed to open V4L2 subdevice '"
> > -			<< entity_->deviceNode() << "': " << strerror(-ret);
> > -		return ret;
> > -	}
> > -	fd_ = ret;
> > -
> > -	return 0;
> > -}
> > -
> > -/**
> > - * \brief Check if the subdevice is open
> > - * \return True if the subdevice is open, false otherwise
> > - */
> > -bool V4L2Subdevice::isOpen() const
> > -{
> > -	return fd_ != -1;
> > -}
> > -
> > -/**
> > - * \brief Close the subdevice, releasing any resources acquired by open()
> > - */
> > -void V4L2Subdevice::close()
> > -{
> > -	if (!isOpen())
> > -		return;
> > -
> > -	::close(fd_);
> > -	fd_ = -1;
> > +	return V4L2Device::open(O_RDWR);
> >  }
> >
> >  /**
> > @@ -200,7 +162,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> >  	ImageFormats formats;
> >
> >  	if (pad >= entity_->pads().size()) {
> > -		LOG(V4L2Subdev, Error) << "Invalid pad: " << pad;
> > +		LOG(V4L2, Error) << "Invalid pad: " << pad;
> >  		return {};
> >  	}
> >
> > @@ -210,7 +172,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> >  			return {};
> >
> >  		if (formats.addFormat(code, sizes)) {
> > -			LOG(V4L2Subdev, Error)
> > +			LOG(V4L2, Error)
> >  				<< "Could not add sizes for media bus code "
> >  				<< code << " on pad " << pad;
> >  			return {};
> > @@ -232,10 +194,9 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> >  	subdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >  	subdevFmt.pad = pad;
> >
> > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> > +	int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> >  	if (ret) {
> > -		ret = -errno;
> > -		LOG(V4L2Subdev, Error)
> > +		LOG(V4L2, Error)
> >  			<< "Unable to get format on pad " << pad
> >  			<< ": " << strerror(-ret);
> >  		return ret;
> > @@ -268,10 +229,9 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> >  	subdevFmt.format.height = format->size.height;
> >  	subdevFmt.format.code = format->mbus_code;
> >
> > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > +	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> >  	if (ret) {
> > -		ret = -errno;
> > -		LOG(V4L2Subdev, Error)
> > +		LOG(V4L2, Error)
> >  			<< "Unable to set format on pad " << pad
> >  			<< ": " << strerror(-ret);
> >  		return ret;
> > @@ -305,11 +265,6 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
> >  	return new V4L2Subdevice(mediaEntity);
> >  }
> >
> > -std::string V4L2Subdevice::logPrefix() const
> > -{
> > -	return "'" + entity_->name() + "'";
> > -}
> > -
> >  std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> >  {
> >  	std::vector<unsigned int> codes;
> > @@ -321,18 +276,17 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> >  		mbusEnum.index = index;
> >  		mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >
> > -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > +		ret = ioctl(VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> >  		if (ret)
> >  			break;
> >
> >  		codes.push_back(mbusEnum.code);
> >  	}
> >
> > -	if (ret && errno != EINVAL) {
> > -		ret = errno;
> > -		LOG(V4L2Subdev, Error)
> > +	if (ret < 0 && ret != -EINVAL) {
> > +		LOG(V4L2, Error)
> >  			<< "Unable to enumerate formats on pad " << pad
> > -			<< ": " << strerror(ret);
> > +			<< ": " << strerror(-ret);
> >  		return {};
> >  	}
> >
> > @@ -352,7 +306,7 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> >  		sizeEnum.code = code;
> >  		sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >
> > -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > +		ret = ioctl(VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> >  		if (ret)
> >  			break;
> >
> > @@ -360,9 +314,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> >  				   sizeEnum.max_width, sizeEnum.max_height);
> >  	}
> >
> > -	if (ret && (errno != EINVAL && errno != ENOTTY)) {
> > -		ret = -errno;
> > -		LOG(V4L2Subdev, Error)
> > +	if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> > +		LOG(V4L2, Error)
> >  			<< "Unable to enumerate sizes on pad " << pad
> >  			<< ": " << strerror(-ret);
> >  		return {};
> > @@ -386,10 +339,9 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> >  	sel.r.width = rect->w;
> >  	sel.r.height = rect->h;
> >
> > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);
> > +	int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);
> >  	if (ret < 0) {
> > -		ret = -errno;
> > -		LOG(V4L2Subdev, Error)
> > +		LOG(V4L2, Error)
> >  			<< "Unable to set rectangle " << target << " on pad "
> >  			<< pad << ": " << strerror(-ret);
> >  		return ret;
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 0e70498c8bb1..403e0b2e0653 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -30,7 +30,7 @@
> >   */
> >  namespace libcamera {
> >
> > -LOG_DEFINE_CATEGORY(V4L2)
> > +LOG_DECLARE_CATEGORY(V4L2)
> >
> >  /**
> >   * \struct V4L2Capability
> > @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
> >   * \param[in] deviceNode The file-system path to the video device node
> >   */
> >  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > -	: deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),
> > +	: V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
> >  	  queuedBuffersCount_(0), fdEvent_(nullptr)
> >  {
> >  	/*
> > @@ -305,23 +305,12 @@ int V4L2VideoDevice::open()
> >  {
> >  	int ret;
> >
> > -	if (isOpen()) {
> > -		LOG(V4L2, Error) << "Device already open";
> > -		return -EBUSY;
> > -	}
> > -
> > -	ret = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);
> > -	if (ret < 0) {
> > -		ret = -errno;
> > -		LOG(V4L2, Error)
> > -			<< "Failed to open V4L2 device: " << strerror(-ret);
> > +	ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> > +	if (ret < 0)
> >  		return ret;
> > -	}
> > -	fd_ = ret;
> >
> > -	ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
> > +	ret = ioctl(VIDIOC_QUERYCAP, &caps_);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Failed to query device capabilities: "
> >  			<< strerror(-ret);
> > @@ -342,20 +331,20 @@ int V4L2VideoDevice::open()
> >  	 * (POLLIN), and write notifications for OUTPUT devices (POLLOUT).
> >  	 */
> >  	if (caps_.isVideoCapture()) {
> > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >  		bufferType_ = caps_.isMultiplanar()
> >  			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> >  			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >  	} else if (caps_.isVideoOutput()) {
> > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >  		bufferType_ = caps_.isMultiplanar()
> >  			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> >  			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >  	} else if (caps_.isMetaCapture()) {
> > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >  		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> >  	} else if (caps_.isMetaOutput()) {
> > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >  		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> >  	} else {
> >  		LOG(V4L2, Error) << "Device is not a supported type";
> > @@ -368,28 +357,18 @@ int V4L2VideoDevice::open()
> >  	return 0;
> >  }
> >
> > -/**
> > - * \brief Check if device is successfully opened
> > - * \return True if the device is open, false otherwise
> > - */
> > -bool V4L2VideoDevice::isOpen() const
> > -{
> > -	return fd_ != -1;
> > -}
> > -
> >  /**
> >   * \brief Close the device, releasing any resources acquired by open()
> >   */
> >  void V4L2VideoDevice::close()
> >  {
> > -	if (fd_ < 0)
> > +	if (!isOpen())
> >  		return;
> >
> >  	releaseBuffers();
> >  	delete fdEvent_;
> >
> > -	::close(fd_);
> > -	fd_ = -1;
> > +	V4L2Device::close();
> >  }
> >
> >  /**
> > @@ -410,17 +389,6 @@ void V4L2VideoDevice::close()
> >   * \return The string containing the device location
> >   */
> >
> > -/**
> > - * \fn V4L2VideoDevice::deviceNode()
> > - * \brief Retrieve the video device node path
> > - * \return The video device device node path
> > - */
> > -
> > -std::string V4L2VideoDevice::logPrefix() const
> > -{
> > -	return deviceNode_ + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> > -}
> > -
> >  /**
> >   * \brief Retrieve the image format set on the V4L2 device
> >   * \param[out] format The image format applied on the device
> > @@ -462,9 +430,8 @@ int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> >  	int ret;
> >
> >  	v4l2Format.type = bufferType_;
> > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> >  	if (ret) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> >  		return ret;
> >  	}
> > @@ -488,9 +455,8 @@ int V4L2VideoDevice::setFormatMeta(V4L2DeviceFormat *format)
> >  	v4l2Format.type = bufferType_;
> >  	pix->dataformat = format->fourcc;
> >  	pix->buffersize = format->planes[0].size;
> > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> >  	if (ret) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> >  		return ret;
> >  	}
> > @@ -516,9 +482,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
> >  	int ret;
> >
> >  	v4l2Format.type = bufferType_;
> > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> >  	if (ret) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> >  		return ret;
> >  	}
> > @@ -554,9 +519,8 @@ int V4L2VideoDevice::setFormatMultiplane(V4L2DeviceFormat *format)
> >  		pix->plane_fmt[i].sizeimage = format->planes[i].size;
> >  	}
> >
> > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> >  	if (ret) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> >  		return ret;
> >  	}
> > @@ -584,9 +548,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
> >  	int ret;
> >
> >  	v4l2Format.type = bufferType_;
> > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> >  	if (ret) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> >  		return ret;
> >  	}
> > @@ -613,9 +576,8 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)
> >  	pix->pixelformat = format->fourcc;
> >  	pix->bytesperline = format->planes[0].bpl;
> >  	pix->field = V4L2_FIELD_NONE;
> > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> >  	if (ret) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> >  		return ret;
> >  	}
> > @@ -670,9 +632,8 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)
> >  	rb.type = bufferType_;
> >  	rb.memory = memoryType_;
> >
> > -	ret = ioctl(fd_, VIDIOC_REQBUFS, &rb);
> > +	ret = ioctl(VIDIOC_REQBUFS, &rb);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Unable to request " << count << " buffers: "
> >  			<< strerror(-ret);
> > @@ -722,9 +683,8 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)
> >  		buf.length = VIDEO_MAX_PLANES;
> >  		buf.m.planes = planes;
> >
> > -		ret = ioctl(fd_, VIDIOC_QUERYBUF, &buf);
> > +		ret = ioctl(VIDIOC_QUERYBUF, &buf);
> >  		if (ret < 0) {
> > -			ret = -errno;
> >  			LOG(V4L2, Error)
> >  				<< "Unable to query buffer " << i << ": "
> >  				<< strerror(-ret);
> > @@ -775,9 +735,8 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,
> >  	expbuf.plane = planeIndex;
> >  	expbuf.flags = O_RDWR;
> >
> > -	ret = ioctl(fd_, VIDIOC_EXPBUF, &expbuf);
> > +	ret = ioctl(VIDIOC_EXPBUF, &expbuf);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Failed to export buffer: " << strerror(-ret);
> >  		return ret;
> > @@ -801,15 +760,14 @@ std::vector<unsigned int> V4L2VideoDevice::enumPixelformats()
> >  		pixelformatEnum.index = index;
> >  		pixelformatEnum.type = bufferType_;
> >
> > -		ret = ioctl(fd_, VIDIOC_ENUM_FMT, &pixelformatEnum);
> > +		ret = ioctl(VIDIOC_ENUM_FMT, &pixelformatEnum);
> >  		if (ret)
> >  			break;
> >
> >  		formats.push_back(pixelformatEnum.pixelformat);
> >  	}
> >
> > -	if (ret && errno != EINVAL) {
> > -		ret = -errno;
> > +	if (ret && ret != -EINVAL) {
> >  		LOG(V4L2, Error)
> >  			<< "Unable to enumerate pixel formats: "
> >  			<< strerror(-ret);
> > @@ -829,7 +787,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
> >  		frameSize.index = index;
> >  		frameSize.pixel_format = pixelFormat;
> >
> > -		ret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize);
> > +		ret = ioctl(VIDIOC_ENUM_FRAMESIZES, &frameSize);
> >  		if (ret)
> >  			break;
> >
> > @@ -867,8 +825,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
> >  		}
> >  	}
> >
> > -	if (ret && errno != EINVAL) {
> > -		ret = -errno;
> > +	if (ret && ret != -EINVAL) {
> >  		LOG(V4L2, Error)
> >  			<< "Unable to enumerate frame sizes: "
> >  			<< strerror(-ret);
> > @@ -969,9 +926,8 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> >
> >  	LOG(V4L2, Debug) << "Queueing buffer " << buf.index;
> >
> > -	ret = ioctl(fd_, VIDIOC_QBUF, &buf);
> > +	ret = ioctl(VIDIOC_QBUF, &buf);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Failed to queue buffer " << buf.index << ": "
> >  			<< strerror(-ret);
> > @@ -1006,9 +962,8 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
> >  		buf.m.planes = planes;
> >  	}
> >
> > -	ret = ioctl(fd_, VIDIOC_DQBUF, &buf);
> > +	ret = ioctl(VIDIOC_DQBUF, &buf);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Failed to dequeue buffer: " << strerror(-ret);
> >  		return nullptr;
> > @@ -1066,9 +1021,8 @@ int V4L2VideoDevice::streamOn()
> >  {
> >  	int ret;
> >
> > -	ret = ioctl(fd_, VIDIOC_STREAMON, &bufferType_);
> > +	ret = ioctl(VIDIOC_STREAMON, &bufferType_);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Failed to start streaming: " << strerror(-ret);
> >  		return ret;
> > @@ -1089,9 +1043,8 @@ int V4L2VideoDevice::streamOff()
> >  {
> >  	int ret;
> >
> > -	ret = ioctl(fd_, VIDIOC_STREAMOFF, &bufferType_);
> > +	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Failed to stop streaming: " << strerror(-ret);
> >  		return ret;
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Wed, Jun 19, 2019 at 02:26:54PM +0200, Jacopo Mondi wrote:
> On Wed, Jun 19, 2019 at 03:20:29PM +0300, Laurent Pinchart wrote:
> > On Wed, Jun 19, 2019 at 01:05:46PM +0200, Jacopo Mondi wrote:
> > > The V4L2 devices and subdevices share a few common operations,like
> > > 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 increase, as the control support
> > > implementation is identical for the two derived classes.
> > >
> > > 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@jmondi.org>
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  src/libcamera/include/v4l2_device.h      |  43 +++++++
> > >  src/libcamera/include/v4l2_subdevice.h   |   9 +-
> > >  src/libcamera/include/v4l2_videodevice.h |  10 +-
> > >  src/libcamera/meson.build                |   2 +
> > >  src/libcamera/v4l2_device.cpp            | 144 +++++++++++++++++++++++
> > >  src/libcamera/v4l2_subdevice.cpp         |  84 +++----------
> > >  src/libcamera/v4l2_videodevice.cpp       | 103 +++++-----------
> > >  7 files changed, 239 insertions(+), 156 deletions(-)
> > >  create mode 100644 src/libcamera/include/v4l2_device.h
> > >  create mode 100644 src/libcamera/v4l2_device.cpp
> > >
> > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > > new file mode 100644
> > > index 000000000000..2c26f3eae2b4
> > > --- /dev/null
> > > +++ b/src/libcamera/include/v4l2_device.h
> > > @@ -0,0 +1,43 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_device.h - Common base for V4L2 video devices and subdevices
> > > + */
> > > +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> > > +#define __LIBCAMERA_V4L2_DEVICE_H__
> > > +
> > > +#include <string>
> > > +
> > > +#include "log.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +class V4L2Device : protected Loggable
> > > +{
> > > +public:
> > > +	void close();
> > > +	bool isOpen() const { return fd_ != -1; }
> > > +	const std::string &deviceNode() const { return deviceNode_; }
> > > +
> > > +protected:
> > > +	V4L2Device(const std::string &deviceNode, const std::string &logTag);
> > > +	~V4L2Device() {}
> > > +
> > > +	int fd() { return fd_; }
> > > +
> > > +	int open(unsigned int flags);
> > > +
> > > +	int ioctl(unsigned long request, void *argp);
> > > +
> > > +	std::string logPrefix() const { return "'" + logTag_ + "'"; }
> >
> > Let's try not to inline methods that are more complex than just
> > returning a member of the class.
> 
> Ok. I always assumed the compiler knows better, and for simple methods
> like this one, implementing it here or in the cpp file it's a matter
> of tastes
> 
> > I didn't notice in my previous review that V4L2Subdevice was using the
> > entity name as a prefix. I'm afraid my comment about moving logPrefix()
> > to V4L2Device was a bad one :-/ However, V4L2Device should still inherit
> > from Loggable. It will thus have a pure virtual logPrefix() inherited
> > from Loggable, requiring the derived classes to implement that method,
> > but allowing LOG() statements in V4L2Device to use the log prefix.
> 
> Why is this (using the entity name as log prefix) bad for you? The
> tag is passed to the base class constructor. Might be a bit of a
> shortcut maybe, is this your concern ?

My concern is two-fold. First of all, it requires passing a log tag to
the V4L2Device constructor, which isn't very nice. I thought both
derived classes used the device node name as a log tag, but that was not
correct.

Then, V4L2VideoDevice adds a suffix to the device node name to indicate
the queue direction. For mem-to-mem devices this is really useful, as a
single video device node exposes two queues, each of them handled by a
V4L2VideoDevice instance. I don't want to lose this debugging aid.

> > Here's the diff I came up with, it can be squashed with this patch if
> > you agree on the comments.
> >
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index 2c26f3eae2b4..8c83adab4d43 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -21,7 +21,7 @@ public:
> >  	const std::string &deviceNode() const { return deviceNode_; }
> >
> >  protected:
> > -	V4L2Device(const std::string &deviceNode, const std::string &logTag);
> > +	V4L2Device(const std::string &deviceNode);
> >  	~V4L2Device() {}
> >
> >  	int fd() { return fd_; }
> > @@ -30,11 +30,8 @@ protected:
> >
> >  	int ioctl(unsigned long request, void *argp);
> >
> > -	std::string logPrefix() const { return "'" + logTag_ + "'"; }
> > -
> >  private:
> >  	std::string deviceNode_;
> > -	std::string logTag_;
> >  	int fd_;
> >  };
> >
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index b1da4a87c553..9c077674f997 100644
> > --- a/src/libcamera/include/v4l2_subdevice.h
> > +++ b/src/libcamera/include/v4l2_subdevice.h
> > @@ -52,6 +52,9 @@ public:
> >  	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
> >  					     const std::string &entity);
> >
> > +protected:
> > +	std::string logPrefix() const;
> > +
> >  private:
> >  	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> >  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > index e73dec13f847..734b34f1dc53 100644
> > --- a/src/libcamera/include/v4l2_videodevice.h
> > +++ b/src/libcamera/include/v4l2_videodevice.h
> > @@ -147,6 +147,9 @@ public:
> >  	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
> >  					       const std::string &entity);
> >
> > +protected:
> > +	std::string logPrefix() const;
> > +
> >  private:
> >  	int getFormatMeta(V4L2DeviceFormat *format);
> >  	int setFormatMeta(V4L2DeviceFormat *format);
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index eeed0a70fb0e..1f113780ba45 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -68,17 +68,15 @@ void V4L2Device::close()
> >  /**
> >   * \brief Construct a V4L2Device
> >   * \param[in] deviceNode The device node filesystem path
> > - * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> > - * the device in the log output
> >   *
> >   * The V4L2Device constructor is protected and can only be accessed by the
> >   * V4L2VideoDevice and V4L2Subdevice derived classes.
> >   *
> >   * Initialize the file descriptor to -1 and store the \a deviceNode to be used
> >   * at open() time, and the \a logTag to prefix log messages with.
> >   */
> > -V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> > -	: deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
> > +V4L2Device::V4L2Device(const std::string &deviceNode)
> > +	: deviceNode_(deviceNode), fd_(-1)
> >  {
> >  }
> >
> > @@ -99,6 +94,7 @@ V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> >   */
> >  int V4L2Device::open(unsigned int flags)
> >  {
> > +	LOG(V4L2, Info) << "Opening device";
> >  	if (isOpen()) {
> >  		LOG(V4L2, Error) << "Device already open";
> >  		return -EBUSY;
> > @@ -135,10 +131,4 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> >  	return 0;
> >  }
> >
> > -/**
> > - * \fn V4L2Device::logPrefix()
> > - * \brief Retrieve the tag to prefix log messages with
> > - * \return The tag to prefix log messages with
> > - */
> > -
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index d0e1d717b26c..a188298de34c 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
> >   * path
> >   */
> >  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> > -	: V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
> > +	: V4L2Device(entity->deviceNode()), entity_(entity)
> >  {
> >  }
> >
> > @@ -265,6 +265,11 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
> >  	return new V4L2Subdevice(mediaEntity);
> >  }
> >
> > +std::string V4L2Subdevice::logPrefix() const
> > +{
> > +	return "'" + entity_->name() + "'";
> > +}
> > +
> >  std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> >  {
> >  	std::vector<unsigned int> codes;
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 403e0b2e0653..2de3751f467e 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
> >   * \param[in] deviceNode The file-system path to the video device node
> >   */
> >  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > -	: V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
> > +	: V4L2Device(deviceNode), bufferPool_(nullptr),
> >  	  queuedBuffersCount_(0), fdEvent_(nullptr)
> >  {
> >  	/*
> > @@ -389,6 +389,11 @@ void V4L2VideoDevice::close()
> >   * \return The string containing the device location
> >   */
> >
> > +std::string V4L2VideoDevice::logPrefix() const
> > +{
> > +	return deviceNode() + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> > +}
> > +
> >  /**
> >   * \brief Retrieve the image format set on the V4L2 device
> >   * \param[out] format The image format applied on the device
> >
> > > +
> > > +private:
> > > +	std::string deviceNode_;
> > > +	std::string logTag_;
> > > +	int fd_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
> > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > > index 9afd28b6db02..b1da4a87c553 100644
> > > --- a/src/libcamera/include/v4l2_subdevice.h
> > > +++ b/src/libcamera/include/v4l2_subdevice.h
> > > @@ -16,6 +16,7 @@
> > >  #include "formats.h"
> > >  #include "log.h"
> > >  #include "media_object.h"
> > > +#include "v4l2_device.h"
> > >
> > >  namespace libcamera {
> > >
> > > @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {
> > >  	const std::string toString() const;
> > >  };
> > >
> > > -class V4L2Subdevice : protected Loggable
> > > +class V4L2Subdevice : public V4L2Device
> > >  {
> > >  public:
> > >  	explicit V4L2Subdevice(const MediaEntity *entity);
> > > @@ -37,8 +38,6 @@ public:
> > >  	~V4L2Subdevice();
> > >
> > >  	int open();
> > > -	bool isOpen() const;
> > > -	void close();
> > >
> > >  	const MediaEntity *entity() const { return entity_; }
> > >
> > > @@ -53,9 +52,6 @@ public:
> > >  	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
> > >  					     const std::string &entity);
> > >
> > > -protected:
> > > -	std::string logPrefix() const;
> > > -
> > >  private:
> > >  	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> > >  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > > @@ -65,7 +61,6 @@ private:
> > >  			 Rectangle *rect);
> > >
> > >  	const MediaEntity *entity_;
> > > -	int fd_;
> > >  };
> > >
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > > index 895658805b47..e73dec13f847 100644
> > > --- a/src/libcamera/include/v4l2_videodevice.h
> > > +++ b/src/libcamera/include/v4l2_videodevice.h
> > > @@ -18,6 +18,7 @@
> > >
> > >  #include "formats.h"
> > >  #include "log.h"
> > > +#include "v4l2_device.h"
> > >
> > >  namespace libcamera {
> > >
> > > @@ -112,7 +113,7 @@ public:
> > >  	const std::string toString() const;
> > >  };
> > >
> > > -class V4L2VideoDevice : protected Loggable
> > > +class V4L2VideoDevice : public V4L2Device
> > >  {
> > >  public:
> > >  	explicit V4L2VideoDevice(const std::string &deviceNode);
> > > @@ -123,13 +124,11 @@ public:
> > >  	V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
> > >
> > >  	int open();
> > > -	bool isOpen() const;
> > >  	void close();
> > >
> > >  	const char *driverName() const { return caps_.driver(); }
> > >  	const char *deviceName() const { return caps_.card(); }
> > >  	const char *busName() const { return caps_.bus_info(); }
> > > -	const std::string &deviceNode() const { return deviceNode_; }
> > >
> > >  	int getFormat(V4L2DeviceFormat *format);
> > >  	int setFormat(V4L2DeviceFormat *format);
> > > @@ -148,9 +147,6 @@ public:
> > >  	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
> > >  					       const std::string &entity);
> > >
> > > -protected:
> > > -	std::string logPrefix() const;
> > > -
> > >  private:
> > >  	int getFormatMeta(V4L2DeviceFormat *format);
> > >  	int setFormatMeta(V4L2DeviceFormat *format);
> > > @@ -171,8 +167,6 @@ private:
> > >  	Buffer *dequeueBuffer();
> > >  	void bufferAvailable(EventNotifier *notifier);
> > >
> > > -	std::string deviceNode_;
> > > -	int fd_;
> > >  	V4L2Capability caps_;
> > >
> > >  	enum v4l2_buf_type bufferType_;
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index 15ab53b1abbe..f26ad5b2dc57 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -23,6 +23,7 @@ libcamera_sources = files([
> > >      'stream.cpp',
> > >      'timer.cpp',
> > >      'utils.cpp',
> > > +    'v4l2_device.cpp',
> > >      'v4l2_subdevice.cpp',
> > >      'v4l2_videodevice.cpp',
> > >  ])
> > > @@ -41,6 +42,7 @@ libcamera_headers = files([
> > >      'include/media_object.h',
> > >      'include/pipeline_handler.h',
> > >      'include/utils.h',
> > > +    'include/v4l2_device.h',
> > >      'include/v4l2_subdevice.h',
> > >      'include/v4l2_videodevice.h',
> > >  ])
> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > new file mode 100644
> > > index 000000000000..eeed0a70fb0e
> > > --- /dev/null
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -0,0 +1,144 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_device.cpp - Common base for V4L2 video devices and subdevices
> > > + */
> > > +
> > > +#include "v4l2_device.h"
> > > +
> > > +#include <fcntl.h>
> > > +#include <string.h>
> > > +#include <sys/ioctl.h>
> > > +#include <unistd.h>
> > > +
> > > +#include "log.h"
> > > +
> > > +/**
> > > + * \file v4l2_device.h
> > > + * \brief Common base for V4L2 devices and subdevices
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(V4L2)
> > > +
> > > +/**
> > > + * \class V4L2Device
> > > + * \brief Base class for V4L2VideoDevice and V4L2Subdevice
> > > + *
> > > + * The V4L2Device class groups together the methods and fields common to
> > > + * both the V4L2VideoDevice and V4L2Subdevice classes, and provides a base
> > > + * class whith methods to open and close the device node associated with the
> > > + * device and to perform IOCTL system calls on it.
> > > + *
> > > + * The V4L2Device class cannot be instantiated directly, as its constructor
> > > + * is protected. Users should instead create instances of one the derived
> > > + * classes to model either a V4L2 video device or a V4L2 subdevice.
> > > + */
> > > +
> > > +/**
> > > + * \brief Close the device node
> > > + *
> > > + * Reset the file descriptor to -1
> > > + */
> > > +void V4L2Device::close()
> > > +{
> > > +	if (!isOpen())
> > > +		return;
> > > +
> > > +	if (::close(fd_) < 0)
> > > +		LOG(V4L2, Error) << "Failed to close V4L2 device: "
> > > +				 << strerror(errno);
> > > +	fd_ = -1;
> > > +}
> > > +
> > > +/**
> > > + * \fn V4L2Device::isOpen()
> > > + * \brief Check if the V4L2 device node is open
> > > + * \return True if the V4L2 device node is open, false otherwise
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2Device::deviceNode()
> > > + * \brief Retrieve the device node path
> > > + * \return The device node path
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct a V4L2Device
> > > + * \param[in] deviceNode The device node filesystem path
> > > + * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> > > + * the device in the log output
> > > + *
> > > + * The V4L2Device constructor is protected and can only be accessed by the
> > > + * V4L2VideoDevice and V4L2Subdevice derived classes.
> >
> > I would drop this sentence.
> >
> > > + *
> > > + * Initialize the file descriptor to -1 and store the \a deviceNode to be used
> > > + * at open() time, and the \a logTag to prefix log messages with.
> > > + */
> > > +V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> > > +	: deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \fn V4L2Device::fd()
> > > + * \brief Retrieve the V4L2 device file descriptor
> > > + * \return The V4L2 device file descriptor, -1 if the device node is not open
> > > + */
> > > +
> > > +/**
> > > + * \brief Open a V4L2 device node
> > > + * \param[in] flags Access mode flags
> > > + *
> > > + * Open the device node path with the provided access mode \a flags and
> > > + * initialize the file descriptor, which was initially set to -1.
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int V4L2Device::open(unsigned int flags)
> > > +{
> > > +	if (isOpen()) {
> > > +		LOG(V4L2, Error) << "Device already open";
> > > +		return -EBUSY;
> > > +	}
> > > +
> > > +	int ret = ::open(deviceNode_.c_str(), flags);
> > > +	if (ret < 0) {
> > > +		ret = -errno;
> > > +		LOG(V4L2, Error) << "Failed to open V4L2 device: "
> > > +				 << strerror(-ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	fd_ = ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \brief Perform an IOCTL system call on the device node
> > > + * \param[in] request The IOCTL request code
> > > + * \param[in] argp A pointer to the IOCTL argument
> > > + * \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;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \fn V4L2Device::logPrefix()
> > > + * \brief Retrieve the tag to prefix log messages with
> > > + * \return The tag to prefix log messages with
> > > + */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index 48f1fcb30ed4..d0e1d717b26c 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -29,7 +29,7 @@
> > >
> > >  namespace libcamera {
> > >
> > > -LOG_DEFINE_CATEGORY(V4L2Subdev)
> > > +LOG_DECLARE_CATEGORY(V4L2)
> > >
> > >  /**
> > >   * \struct V4L2SubdeviceFormat
> > > @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
> > >   * path
> > >   */
> > >  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> > > -	: entity_(entity), fd_(-1)
> > > +	: V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
> > >  {
> > >  }
> > >
> > > @@ -117,45 +117,7 @@ V4L2Subdevice::~V4L2Subdevice()
> > >   */
> > >  int V4L2Subdevice::open()
> > >  {
> > > -	int ret;
> > > -
> > > -	if (isOpen()) {
> > > -		LOG(V4L2Subdev, Error) << "Device already open";
> > > -		return -EBUSY;
> > > -	}
> > > -
> > > -	ret = ::open(entity_->deviceNode().c_str(), O_RDWR);
> > > -	if (ret < 0) {
> > > -		ret = -errno;
> > > -		LOG(V4L2Subdev, Error)
> > > -			<< "Failed to open V4L2 subdevice '"
> > > -			<< entity_->deviceNode() << "': " << strerror(-ret);
> > > -		return ret;
> > > -	}
> > > -	fd_ = ret;
> > > -
> > > -	return 0;
> > > -}
> > > -
> > > -/**
> > > - * \brief Check if the subdevice is open
> > > - * \return True if the subdevice is open, false otherwise
> > > - */
> > > -bool V4L2Subdevice::isOpen() const
> > > -{
> > > -	return fd_ != -1;
> > > -}
> > > -
> > > -/**
> > > - * \brief Close the subdevice, releasing any resources acquired by open()
> > > - */
> > > -void V4L2Subdevice::close()
> > > -{
> > > -	if (!isOpen())
> > > -		return;
> > > -
> > > -	::close(fd_);
> > > -	fd_ = -1;
> > > +	return V4L2Device::open(O_RDWR);
> > >  }
> > >
> > >  /**
> > > @@ -200,7 +162,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> > >  	ImageFormats formats;
> > >
> > >  	if (pad >= entity_->pads().size()) {
> > > -		LOG(V4L2Subdev, Error) << "Invalid pad: " << pad;
> > > +		LOG(V4L2, Error) << "Invalid pad: " << pad;
> > >  		return {};
> > >  	}
> > >
> > > @@ -210,7 +172,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> > >  			return {};
> > >
> > >  		if (formats.addFormat(code, sizes)) {
> > > -			LOG(V4L2Subdev, Error)
> > > +			LOG(V4L2, Error)
> > >  				<< "Could not add sizes for media bus code "
> > >  				<< code << " on pad " << pad;
> > >  			return {};
> > > @@ -232,10 +194,9 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> > >  	subdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > >  	subdevFmt.pad = pad;
> > >
> > > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> > > +	int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> > >  	if (ret) {
> > > -		ret = -errno;
> > > -		LOG(V4L2Subdev, Error)
> > > +		LOG(V4L2, Error)
> > >  			<< "Unable to get format on pad " << pad
> > >  			<< ": " << strerror(-ret);
> > >  		return ret;
> > > @@ -268,10 +229,9 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> > >  	subdevFmt.format.height = format->size.height;
> > >  	subdevFmt.format.code = format->mbus_code;
> > >
> > > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > > +	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > >  	if (ret) {
> > > -		ret = -errno;
> > > -		LOG(V4L2Subdev, Error)
> > > +		LOG(V4L2, Error)
> > >  			<< "Unable to set format on pad " << pad
> > >  			<< ": " << strerror(-ret);
> > >  		return ret;
> > > @@ -305,11 +265,6 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
> > >  	return new V4L2Subdevice(mediaEntity);
> > >  }
> > >
> > > -std::string V4L2Subdevice::logPrefix() const
> > > -{
> > > -	return "'" + entity_->name() + "'";
> > > -}
> > > -
> > >  std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > >  {
> > >  	std::vector<unsigned int> codes;
> > > @@ -321,18 +276,17 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > >  		mbusEnum.index = index;
> > >  		mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > >
> > > -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > > +		ret = ioctl(VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > >  		if (ret)
> > >  			break;
> > >
> > >  		codes.push_back(mbusEnum.code);
> > >  	}
> > >
> > > -	if (ret && errno != EINVAL) {
> > > -		ret = errno;
> > > -		LOG(V4L2Subdev, Error)
> > > +	if (ret < 0 && ret != -EINVAL) {
> > > +		LOG(V4L2, Error)
> > >  			<< "Unable to enumerate formats on pad " << pad
> > > -			<< ": " << strerror(ret);
> > > +			<< ": " << strerror(-ret);
> > >  		return {};
> > >  	}
> > >
> > > @@ -352,7 +306,7 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > >  		sizeEnum.code = code;
> > >  		sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > >
> > > -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > > +		ret = ioctl(VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > >  		if (ret)
> > >  			break;
> > >
> > > @@ -360,9 +314,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > >  				   sizeEnum.max_width, sizeEnum.max_height);
> > >  	}
> > >
> > > -	if (ret && (errno != EINVAL && errno != ENOTTY)) {
> > > -		ret = -errno;
> > > -		LOG(V4L2Subdev, Error)
> > > +	if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> > > +		LOG(V4L2, Error)
> > >  			<< "Unable to enumerate sizes on pad " << pad
> > >  			<< ": " << strerror(-ret);
> > >  		return {};
> > > @@ -386,10 +339,9 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > >  	sel.r.width = rect->w;
> > >  	sel.r.height = rect->h;
> > >
> > > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);
> > > +	int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > > -		LOG(V4L2Subdev, Error)
> > > +		LOG(V4L2, Error)
> > >  			<< "Unable to set rectangle " << target << " on pad "
> > >  			<< pad << ": " << strerror(-ret);
> > >  		return ret;
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 0e70498c8bb1..403e0b2e0653 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -30,7 +30,7 @@
> > >   */
> > >  namespace libcamera {
> > >
> > > -LOG_DEFINE_CATEGORY(V4L2)
> > > +LOG_DECLARE_CATEGORY(V4L2)
> > >
> > >  /**
> > >   * \struct V4L2Capability
> > > @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
> > >   * \param[in] deviceNode The file-system path to the video device node
> > >   */
> > >  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > > -	: deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),
> > > +	: V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
> > >  	  queuedBuffersCount_(0), fdEvent_(nullptr)
> > >  {
> > >  	/*
> > > @@ -305,23 +305,12 @@ int V4L2VideoDevice::open()
> > >  {
> > >  	int ret;
> > >
> > > -	if (isOpen()) {
> > > -		LOG(V4L2, Error) << "Device already open";
> > > -		return -EBUSY;
> > > -	}
> > > -
> > > -	ret = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);
> > > -	if (ret < 0) {
> > > -		ret = -errno;
> > > -		LOG(V4L2, Error)
> > > -			<< "Failed to open V4L2 device: " << strerror(-ret);
> > > +	ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> > > +	if (ret < 0)
> > >  		return ret;
> > > -	}
> > > -	fd_ = ret;
> > >
> > > -	ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
> > > +	ret = ioctl(VIDIOC_QUERYCAP, &caps_);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Failed to query device capabilities: "
> > >  			<< strerror(-ret);
> > > @@ -342,20 +331,20 @@ int V4L2VideoDevice::open()
> > >  	 * (POLLIN), and write notifications for OUTPUT devices (POLLOUT).
> > >  	 */
> > >  	if (caps_.isVideoCapture()) {
> > > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> > > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> > >  		bufferType_ = caps_.isMultiplanar()
> > >  			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> > >  			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > >  	} else if (caps_.isVideoOutput()) {
> > > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> > > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> > >  		bufferType_ = caps_.isMultiplanar()
> > >  			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> > >  			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> > >  	} else if (caps_.isMetaCapture()) {
> > > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> > > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> > >  		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> > >  	} else if (caps_.isMetaOutput()) {
> > > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> > > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> > >  		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> > >  	} else {
> > >  		LOG(V4L2, Error) << "Device is not a supported type";
> > > @@ -368,28 +357,18 @@ int V4L2VideoDevice::open()
> > >  	return 0;
> > >  }
> > >
> > > -/**
> > > - * \brief Check if device is successfully opened
> > > - * \return True if the device is open, false otherwise
> > > - */
> > > -bool V4L2VideoDevice::isOpen() const
> > > -{
> > > -	return fd_ != -1;
> > > -}
> > > -
> > >  /**
> > >   * \brief Close the device, releasing any resources acquired by open()
> > >   */
> > >  void V4L2VideoDevice::close()
> > >  {
> > > -	if (fd_ < 0)
> > > +	if (!isOpen())
> > >  		return;
> > >
> > >  	releaseBuffers();
> > >  	delete fdEvent_;
> > >
> > > -	::close(fd_);
> > > -	fd_ = -1;
> > > +	V4L2Device::close();
> > >  }
> > >
> > >  /**
> > > @@ -410,17 +389,6 @@ void V4L2VideoDevice::close()
> > >   * \return The string containing the device location
> > >   */
> > >
> > > -/**
> > > - * \fn V4L2VideoDevice::deviceNode()
> > > - * \brief Retrieve the video device node path
> > > - * \return The video device device node path
> > > - */
> > > -
> > > -std::string V4L2VideoDevice::logPrefix() const
> > > -{
> > > -	return deviceNode_ + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> > > -}
> > > -
> > >  /**
> > >   * \brief Retrieve the image format set on the V4L2 device
> > >   * \param[out] format The image format applied on the device
> > > @@ -462,9 +430,8 @@ int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> > >  	int ret;
> > >
> > >  	v4l2Format.type = bufferType_;
> > > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> > >  	if (ret) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > > @@ -488,9 +455,8 @@ int V4L2VideoDevice::setFormatMeta(V4L2DeviceFormat *format)
> > >  	v4l2Format.type = bufferType_;
> > >  	pix->dataformat = format->fourcc;
> > >  	pix->buffersize = format->planes[0].size;
> > > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> > >  	if (ret) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > > @@ -516,9 +482,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
> > >  	int ret;
> > >
> > >  	v4l2Format.type = bufferType_;
> > > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> > >  	if (ret) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > > @@ -554,9 +519,8 @@ int V4L2VideoDevice::setFormatMultiplane(V4L2DeviceFormat *format)
> > >  		pix->plane_fmt[i].sizeimage = format->planes[i].size;
> > >  	}
> > >
> > > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> > >  	if (ret) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > > @@ -584,9 +548,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
> > >  	int ret;
> > >
> > >  	v4l2Format.type = bufferType_;
> > > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> > >  	if (ret) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > > @@ -613,9 +576,8 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)
> > >  	pix->pixelformat = format->fourcc;
> > >  	pix->bytesperline = format->planes[0].bpl;
> > >  	pix->field = V4L2_FIELD_NONE;
> > > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> > >  	if (ret) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > > @@ -670,9 +632,8 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)
> > >  	rb.type = bufferType_;
> > >  	rb.memory = memoryType_;
> > >
> > > -	ret = ioctl(fd_, VIDIOC_REQBUFS, &rb);
> > > +	ret = ioctl(VIDIOC_REQBUFS, &rb);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Unable to request " << count << " buffers: "
> > >  			<< strerror(-ret);
> > > @@ -722,9 +683,8 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)
> > >  		buf.length = VIDEO_MAX_PLANES;
> > >  		buf.m.planes = planes;
> > >
> > > -		ret = ioctl(fd_, VIDIOC_QUERYBUF, &buf);
> > > +		ret = ioctl(VIDIOC_QUERYBUF, &buf);
> > >  		if (ret < 0) {
> > > -			ret = -errno;
> > >  			LOG(V4L2, Error)
> > >  				<< "Unable to query buffer " << i << ": "
> > >  				<< strerror(-ret);
> > > @@ -775,9 +735,8 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,
> > >  	expbuf.plane = planeIndex;
> > >  	expbuf.flags = O_RDWR;
> > >
> > > -	ret = ioctl(fd_, VIDIOC_EXPBUF, &expbuf);
> > > +	ret = ioctl(VIDIOC_EXPBUF, &expbuf);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Failed to export buffer: " << strerror(-ret);
> > >  		return ret;
> > > @@ -801,15 +760,14 @@ std::vector<unsigned int> V4L2VideoDevice::enumPixelformats()
> > >  		pixelformatEnum.index = index;
> > >  		pixelformatEnum.type = bufferType_;
> > >
> > > -		ret = ioctl(fd_, VIDIOC_ENUM_FMT, &pixelformatEnum);
> > > +		ret = ioctl(VIDIOC_ENUM_FMT, &pixelformatEnum);
> > >  		if (ret)
> > >  			break;
> > >
> > >  		formats.push_back(pixelformatEnum.pixelformat);
> > >  	}
> > >
> > > -	if (ret && errno != EINVAL) {
> > > -		ret = -errno;
> > > +	if (ret && ret != -EINVAL) {
> > >  		LOG(V4L2, Error)
> > >  			<< "Unable to enumerate pixel formats: "
> > >  			<< strerror(-ret);
> > > @@ -829,7 +787,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
> > >  		frameSize.index = index;
> > >  		frameSize.pixel_format = pixelFormat;
> > >
> > > -		ret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize);
> > > +		ret = ioctl(VIDIOC_ENUM_FRAMESIZES, &frameSize);
> > >  		if (ret)
> > >  			break;
> > >
> > > @@ -867,8 +825,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
> > >  		}
> > >  	}
> > >
> > > -	if (ret && errno != EINVAL) {
> > > -		ret = -errno;
> > > +	if (ret && ret != -EINVAL) {
> > >  		LOG(V4L2, Error)
> > >  			<< "Unable to enumerate frame sizes: "
> > >  			<< strerror(-ret);
> > > @@ -969,9 +926,8 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> > >
> > >  	LOG(V4L2, Debug) << "Queueing buffer " << buf.index;
> > >
> > > -	ret = ioctl(fd_, VIDIOC_QBUF, &buf);
> > > +	ret = ioctl(VIDIOC_QBUF, &buf);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Failed to queue buffer " << buf.index << ": "
> > >  			<< strerror(-ret);
> > > @@ -1006,9 +962,8 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
> > >  		buf.m.planes = planes;
> > >  	}
> > >
> > > -	ret = ioctl(fd_, VIDIOC_DQBUF, &buf);
> > > +	ret = ioctl(VIDIOC_DQBUF, &buf);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Failed to dequeue buffer: " << strerror(-ret);
> > >  		return nullptr;
> > > @@ -1066,9 +1021,8 @@ int V4L2VideoDevice::streamOn()
> > >  {
> > >  	int ret;
> > >
> > > -	ret = ioctl(fd_, VIDIOC_STREAMON, &bufferType_);
> > > +	ret = ioctl(VIDIOC_STREAMON, &bufferType_);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Failed to start streaming: " << strerror(-ret);
> > >  		return ret;
> > > @@ -1089,9 +1043,8 @@ int V4L2VideoDevice::streamOff()
> > >  {
> > >  	int ret;
> > >
> > > -	ret = ioctl(fd_, VIDIOC_STREAMOFF, &bufferType_);
> > > +	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Failed to stop streaming: " << strerror(-ret);
> > >  		return ret;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

HI Laurent,

On Wed, Jun 19, 2019 at 03:29:08PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Jun 19, 2019 at 02:26:54PM +0200, Jacopo Mondi wrote:
> > On Wed, Jun 19, 2019 at 03:20:29PM +0300, Laurent Pinchart wrote:
> > > On Wed, Jun 19, 2019 at 01:05:46PM +0200, Jacopo Mondi wrote:
> > > > The V4L2 devices and subdevices share a few common operations,like
> > > > 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 increase, as the control support
> > > > implementation is identical for the two derived classes.
> > > >
> > > > 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@jmondi.org>
> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > ---
> > > >  src/libcamera/include/v4l2_device.h      |  43 +++++++
> > > >  src/libcamera/include/v4l2_subdevice.h   |   9 +-
> > > >  src/libcamera/include/v4l2_videodevice.h |  10 +-
> > > >  src/libcamera/meson.build                |   2 +
> > > >  src/libcamera/v4l2_device.cpp            | 144 +++++++++++++++++++++++
> > > >  src/libcamera/v4l2_subdevice.cpp         |  84 +++----------
> > > >  src/libcamera/v4l2_videodevice.cpp       | 103 +++++-----------
> > > >  7 files changed, 239 insertions(+), 156 deletions(-)
> > > >  create mode 100644 src/libcamera/include/v4l2_device.h
> > > >  create mode 100644 src/libcamera/v4l2_device.cpp
> > > >
> > > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > > > new file mode 100644
> > > > index 000000000000..2c26f3eae2b4
> > > > --- /dev/null
> > > > +++ b/src/libcamera/include/v4l2_device.h
> > > > @@ -0,0 +1,43 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2019, Google Inc.
> > > > + *
> > > > + * v4l2_device.h - Common base for V4L2 video devices and subdevices
> > > > + */
> > > > +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> > > > +#define __LIBCAMERA_V4L2_DEVICE_H__
> > > > +
> > > > +#include <string>
> > > > +
> > > > +#include "log.h"
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +class V4L2Device : protected Loggable
> > > > +{
> > > > +public:
> > > > +	void close();
> > > > +	bool isOpen() const { return fd_ != -1; }
> > > > +	const std::string &deviceNode() const { return deviceNode_; }
> > > > +
> > > > +protected:
> > > > +	V4L2Device(const std::string &deviceNode, const std::string &logTag);
> > > > +	~V4L2Device() {}
> > > > +
> > > > +	int fd() { return fd_; }
> > > > +
> > > > +	int open(unsigned int flags);
> > > > +
> > > > +	int ioctl(unsigned long request, void *argp);
> > > > +
> > > > +	std::string logPrefix() const { return "'" + logTag_ + "'"; }
> > >
> > > Let's try not to inline methods that are more complex than just
> > > returning a member of the class.
> >
> > Ok. I always assumed the compiler knows better, and for simple methods
> > like this one, implementing it here or in the cpp file it's a matter
> > of tastes
> >
> > > I didn't notice in my previous review that V4L2Subdevice was using the
> > > entity name as a prefix. I'm afraid my comment about moving logPrefix()
> > > to V4L2Device was a bad one :-/ However, V4L2Device should still inherit
> > > from Loggable. It will thus have a pure virtual logPrefix() inherited
> > > from Loggable, requiring the derived classes to implement that method,
> > > but allowing LOG() statements in V4L2Device to use the log prefix.
> >
> > Why is this (using the entity name as log prefix) bad for you? The
> > tag is passed to the base class constructor. Might be a bit of a
> > shortcut maybe, is this your concern ?
>
> My concern is two-fold. First of all, it requires passing a log tag to
> the V4L2Device constructor, which isn't very nice. I thought both
> derived classes used the device node name as a log tag, but that was not
> correct.

I agree it's a bit of a shortcut, yes

>
> Then, V4L2VideoDevice adds a suffix to the device node name to indicate
> the queue direction. For mem-to-mem devices this is really useful, as a
> single video device node exposes two queues, each of them handled by a
> V4L2VideoDevice instance. I don't want to lose this debugging aid.
>

That I considered not that relevant, but I ignore the case of devices
with multiple queues

I'll drop that part, I'll squash your patch on top.

Thanks
   j

> > > Here's the diff I came up with, it can be squashed with this patch if
> > > you agree on the comments.
> > >
> > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > > index 2c26f3eae2b4..8c83adab4d43 100644
> > > --- a/src/libcamera/include/v4l2_device.h
> > > +++ b/src/libcamera/include/v4l2_device.h
> > > @@ -21,7 +21,7 @@ public:
> > >  	const std::string &deviceNode() const { return deviceNode_; }
> > >
> > >  protected:
> > > -	V4L2Device(const std::string &deviceNode, const std::string &logTag);
> > > +	V4L2Device(const std::string &deviceNode);
> > >  	~V4L2Device() {}
> > >
> > >  	int fd() { return fd_; }
> > > @@ -30,11 +30,8 @@ protected:
> > >
> > >  	int ioctl(unsigned long request, void *argp);
> > >
> > > -	std::string logPrefix() const { return "'" + logTag_ + "'"; }
> > > -
> > >  private:
> > >  	std::string deviceNode_;
> > > -	std::string logTag_;
> > >  	int fd_;
> > >  };
> > >
> > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > > index b1da4a87c553..9c077674f997 100644
> > > --- a/src/libcamera/include/v4l2_subdevice.h
> > > +++ b/src/libcamera/include/v4l2_subdevice.h
> > > @@ -52,6 +52,9 @@ public:
> > >  	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
> > >  					     const std::string &entity);
> > >
> > > +protected:
> > > +	std::string logPrefix() const;
> > > +
> > >  private:
> > >  	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> > >  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > > index e73dec13f847..734b34f1dc53 100644
> > > --- a/src/libcamera/include/v4l2_videodevice.h
> > > +++ b/src/libcamera/include/v4l2_videodevice.h
> > > @@ -147,6 +147,9 @@ public:
> > >  	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
> > >  					       const std::string &entity);
> > >
> > > +protected:
> > > +	std::string logPrefix() const;
> > > +
> > >  private:
> > >  	int getFormatMeta(V4L2DeviceFormat *format);
> > >  	int setFormatMeta(V4L2DeviceFormat *format);
> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > index eeed0a70fb0e..1f113780ba45 100644
> > > --- a/src/libcamera/v4l2_device.cpp
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -68,17 +68,15 @@ void V4L2Device::close()
> > >  /**
> > >   * \brief Construct a V4L2Device
> > >   * \param[in] deviceNode The device node filesystem path
> > > - * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> > > - * the device in the log output
> > >   *
> > >   * The V4L2Device constructor is protected and can only be accessed by the
> > >   * V4L2VideoDevice and V4L2Subdevice derived classes.
> > >   *
> > >   * Initialize the file descriptor to -1 and store the \a deviceNode to be used
> > >   * at open() time, and the \a logTag to prefix log messages with.
> > >   */
> > > -V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> > > -	: deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
> > > +V4L2Device::V4L2Device(const std::string &deviceNode)
> > > +	: deviceNode_(deviceNode), fd_(-1)
> > >  {
> > >  }
> > >
> > > @@ -99,6 +94,7 @@ V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> > >   */
> > >  int V4L2Device::open(unsigned int flags)
> > >  {
> > > +	LOG(V4L2, Info) << "Opening device";
> > >  	if (isOpen()) {
> > >  		LOG(V4L2, Error) << "Device already open";
> > >  		return -EBUSY;
> > > @@ -135,10 +131,4 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> > >  	return 0;
> > >  }
> > >
> > > -/**
> > > - * \fn V4L2Device::logPrefix()
> > > - * \brief Retrieve the tag to prefix log messages with
> > > - * \return The tag to prefix log messages with
> > > - */
> > > -
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index d0e1d717b26c..a188298de34c 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
> > >   * path
> > >   */
> > >  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> > > -	: V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
> > > +	: V4L2Device(entity->deviceNode()), entity_(entity)
> > >  {
> > >  }
> > >
> > > @@ -265,6 +265,11 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
> > >  	return new V4L2Subdevice(mediaEntity);
> > >  }
> > >
> > > +std::string V4L2Subdevice::logPrefix() const
> > > +{
> > > +	return "'" + entity_->name() + "'";
> > > +}
> > > +
> > >  std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > >  {
> > >  	std::vector<unsigned int> codes;
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 403e0b2e0653..2de3751f467e 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
> > >   * \param[in] deviceNode The file-system path to the video device node
> > >   */
> > >  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > > -	: V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
> > > +	: V4L2Device(deviceNode), bufferPool_(nullptr),
> > >  	  queuedBuffersCount_(0), fdEvent_(nullptr)
> > >  {
> > >  	/*
> > > @@ -389,6 +389,11 @@ void V4L2VideoDevice::close()
> > >   * \return The string containing the device location
> > >   */
> > >
> > > +std::string V4L2VideoDevice::logPrefix() const
> > > +{
> > > +	return deviceNode() + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> > > +}
> > > +
> > >  /**
> > >   * \brief Retrieve the image format set on the V4L2 device
> > >   * \param[out] format The image format applied on the device
> > >
> > > > +
> > > > +private:
> > > > +	std::string deviceNode_;
> > > > +	std::string logTag_;
> > > > +	int fd_;
> > > > +};
> > > > +
> > > > +} /* namespace libcamera */
> > > > +
> > > > +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
> > > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > > > index 9afd28b6db02..b1da4a87c553 100644
> > > > --- a/src/libcamera/include/v4l2_subdevice.h
> > > > +++ b/src/libcamera/include/v4l2_subdevice.h
> > > > @@ -16,6 +16,7 @@
> > > >  #include "formats.h"
> > > >  #include "log.h"
> > > >  #include "media_object.h"
> > > > +#include "v4l2_device.h"
> > > >
> > > >  namespace libcamera {
> > > >
> > > > @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {
> > > >  	const std::string toString() const;
> > > >  };
> > > >
> > > > -class V4L2Subdevice : protected Loggable
> > > > +class V4L2Subdevice : public V4L2Device
> > > >  {
> > > >  public:
> > > >  	explicit V4L2Subdevice(const MediaEntity *entity);
> > > > @@ -37,8 +38,6 @@ public:
> > > >  	~V4L2Subdevice();
> > > >
> > > >  	int open();
> > > > -	bool isOpen() const;
> > > > -	void close();
> > > >
> > > >  	const MediaEntity *entity() const { return entity_; }
> > > >
> > > > @@ -53,9 +52,6 @@ public:
> > > >  	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
> > > >  					     const std::string &entity);
> > > >
> > > > -protected:
> > > > -	std::string logPrefix() const;
> > > > -
> > > >  private:
> > > >  	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> > > >  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > > > @@ -65,7 +61,6 @@ private:
> > > >  			 Rectangle *rect);
> > > >
> > > >  	const MediaEntity *entity_;
> > > > -	int fd_;
> > > >  };
> > > >
> > > >  } /* namespace libcamera */
> > > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > > > index 895658805b47..e73dec13f847 100644
> > > > --- a/src/libcamera/include/v4l2_videodevice.h
> > > > +++ b/src/libcamera/include/v4l2_videodevice.h
> > > > @@ -18,6 +18,7 @@
> > > >
> > > >  #include "formats.h"
> > > >  #include "log.h"
> > > > +#include "v4l2_device.h"
> > > >
> > > >  namespace libcamera {
> > > >
> > > > @@ -112,7 +113,7 @@ public:
> > > >  	const std::string toString() const;
> > > >  };
> > > >
> > > > -class V4L2VideoDevice : protected Loggable
> > > > +class V4L2VideoDevice : public V4L2Device
> > > >  {
> > > >  public:
> > > >  	explicit V4L2VideoDevice(const std::string &deviceNode);
> > > > @@ -123,13 +124,11 @@ public:
> > > >  	V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
> > > >
> > > >  	int open();
> > > > -	bool isOpen() const;
> > > >  	void close();
> > > >
> > > >  	const char *driverName() const { return caps_.driver(); }
> > > >  	const char *deviceName() const { return caps_.card(); }
> > > >  	const char *busName() const { return caps_.bus_info(); }
> > > > -	const std::string &deviceNode() const { return deviceNode_; }
> > > >
> > > >  	int getFormat(V4L2DeviceFormat *format);
> > > >  	int setFormat(V4L2DeviceFormat *format);
> > > > @@ -148,9 +147,6 @@ public:
> > > >  	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
> > > >  					       const std::string &entity);
> > > >
> > > > -protected:
> > > > -	std::string logPrefix() const;
> > > > -
> > > >  private:
> > > >  	int getFormatMeta(V4L2DeviceFormat *format);
> > > >  	int setFormatMeta(V4L2DeviceFormat *format);
> > > > @@ -171,8 +167,6 @@ private:
> > > >  	Buffer *dequeueBuffer();
> > > >  	void bufferAvailable(EventNotifier *notifier);
> > > >
> > > > -	std::string deviceNode_;
> > > > -	int fd_;
> > > >  	V4L2Capability caps_;
> > > >
> > > >  	enum v4l2_buf_type bufferType_;
> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > index 15ab53b1abbe..f26ad5b2dc57 100644
> > > > --- a/src/libcamera/meson.build
> > > > +++ b/src/libcamera/meson.build
> > > > @@ -23,6 +23,7 @@ libcamera_sources = files([
> > > >      'stream.cpp',
> > > >      'timer.cpp',
> > > >      'utils.cpp',
> > > > +    'v4l2_device.cpp',
> > > >      'v4l2_subdevice.cpp',
> > > >      'v4l2_videodevice.cpp',
> > > >  ])
> > > > @@ -41,6 +42,7 @@ libcamera_headers = files([
> > > >      'include/media_object.h',
> > > >      'include/pipeline_handler.h',
> > > >      'include/utils.h',
> > > > +    'include/v4l2_device.h',
> > > >      'include/v4l2_subdevice.h',
> > > >      'include/v4l2_videodevice.h',
> > > >  ])
> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > > new file mode 100644
> > > > index 000000000000..eeed0a70fb0e
> > > > --- /dev/null
> > > > +++ b/src/libcamera/v4l2_device.cpp
> > > > @@ -0,0 +1,144 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2019, Google Inc.
> > > > + *
> > > > + * v4l2_device.cpp - Common base for V4L2 video devices and subdevices
> > > > + */
> > > > +
> > > > +#include "v4l2_device.h"
> > > > +
> > > > +#include <fcntl.h>
> > > > +#include <string.h>
> > > > +#include <sys/ioctl.h>
> > > > +#include <unistd.h>
> > > > +
> > > > +#include "log.h"
> > > > +
> > > > +/**
> > > > + * \file v4l2_device.h
> > > > + * \brief Common base for V4L2 devices and subdevices
> > > > + */
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +LOG_DEFINE_CATEGORY(V4L2)
> > > > +
> > > > +/**
> > > > + * \class V4L2Device
> > > > + * \brief Base class for V4L2VideoDevice and V4L2Subdevice
> > > > + *
> > > > + * The V4L2Device class groups together the methods and fields common to
> > > > + * both the V4L2VideoDevice and V4L2Subdevice classes, and provides a base
> > > > + * class whith methods to open and close the device node associated with the
> > > > + * device and to perform IOCTL system calls on it.
> > > > + *
> > > > + * The V4L2Device class cannot be instantiated directly, as its constructor
> > > > + * is protected. Users should instead create instances of one the derived
> > > > + * classes to model either a V4L2 video device or a V4L2 subdevice.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Close the device node
> > > > + *
> > > > + * Reset the file descriptor to -1
> > > > + */
> > > > +void V4L2Device::close()
> > > > +{
> > > > +	if (!isOpen())
> > > > +		return;
> > > > +
> > > > +	if (::close(fd_) < 0)
> > > > +		LOG(V4L2, Error) << "Failed to close V4L2 device: "
> > > > +				 << strerror(errno);
> > > > +	fd_ = -1;
> > > > +}
> > > > +
> > > > +/**
> > > > + * \fn V4L2Device::isOpen()
> > > > + * \brief Check if the V4L2 device node is open
> > > > + * \return True if the V4L2 device node is open, false otherwise
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn V4L2Device::deviceNode()
> > > > + * \brief Retrieve the device node path
> > > > + * \return The device node path
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Construct a V4L2Device
> > > > + * \param[in] deviceNode The device node filesystem path
> > > > + * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> > > > + * the device in the log output
> > > > + *
> > > > + * The V4L2Device constructor is protected and can only be accessed by the
> > > > + * V4L2VideoDevice and V4L2Subdevice derived classes.
> > >
> > > I would drop this sentence.
> > >
> > > > + *
> > > > + * Initialize the file descriptor to -1 and store the \a deviceNode to be used
> > > > + * at open() time, and the \a logTag to prefix log messages with.
> > > > + */
> > > > +V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> > > > +	: deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
> > > > +{
> > > > +}
> > > > +
> > > > +/**
> > > > + * \fn V4L2Device::fd()
> > > > + * \brief Retrieve the V4L2 device file descriptor
> > > > + * \return The V4L2 device file descriptor, -1 if the device node is not open
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Open a V4L2 device node
> > > > + * \param[in] flags Access mode flags
> > > > + *
> > > > + * Open the device node path with the provided access mode \a flags and
> > > > + * initialize the file descriptor, which was initially set to -1.
> > > > + *
> > > > + * \return 0 on success or a negative error code otherwise
> > > > + */
> > > > +int V4L2Device::open(unsigned int flags)
> > > > +{
> > > > +	if (isOpen()) {
> > > > +		LOG(V4L2, Error) << "Device already open";
> > > > +		return -EBUSY;
> > > > +	}
> > > > +
> > > > +	int ret = ::open(deviceNode_.c_str(), flags);
> > > > +	if (ret < 0) {
> > > > +		ret = -errno;
> > > > +		LOG(V4L2, Error) << "Failed to open V4L2 device: "
> > > > +				 << strerror(-ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	fd_ = ret;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Perform an IOCTL system call on the device node
> > > > + * \param[in] request The IOCTL request code
> > > > + * \param[in] argp A pointer to the IOCTL argument
> > > > + * \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;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * \fn V4L2Device::logPrefix()
> > > > + * \brief Retrieve the tag to prefix log messages with
> > > > + * \return The tag to prefix log messages with
> > > > + */
> > > > +
> > > > +} /* namespace libcamera */
> > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > > index 48f1fcb30ed4..d0e1d717b26c 100644
> > > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > > @@ -29,7 +29,7 @@
> > > >
> > > >  namespace libcamera {
> > > >
> > > > -LOG_DEFINE_CATEGORY(V4L2Subdev)
> > > > +LOG_DECLARE_CATEGORY(V4L2)
> > > >
> > > >  /**
> > > >   * \struct V4L2SubdeviceFormat
> > > > @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
> > > >   * path
> > > >   */
> > > >  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> > > > -	: entity_(entity), fd_(-1)
> > > > +	: V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
> > > >  {
> > > >  }
> > > >
> > > > @@ -117,45 +117,7 @@ V4L2Subdevice::~V4L2Subdevice()
> > > >   */
> > > >  int V4L2Subdevice::open()
> > > >  {
> > > > -	int ret;
> > > > -
> > > > -	if (isOpen()) {
> > > > -		LOG(V4L2Subdev, Error) << "Device already open";
> > > > -		return -EBUSY;
> > > > -	}
> > > > -
> > > > -	ret = ::open(entity_->deviceNode().c_str(), O_RDWR);
> > > > -	if (ret < 0) {
> > > > -		ret = -errno;
> > > > -		LOG(V4L2Subdev, Error)
> > > > -			<< "Failed to open V4L2 subdevice '"
> > > > -			<< entity_->deviceNode() << "': " << strerror(-ret);
> > > > -		return ret;
> > > > -	}
> > > > -	fd_ = ret;
> > > > -
> > > > -	return 0;
> > > > -}
> > > > -
> > > > -/**
> > > > - * \brief Check if the subdevice is open
> > > > - * \return True if the subdevice is open, false otherwise
> > > > - */
> > > > -bool V4L2Subdevice::isOpen() const
> > > > -{
> > > > -	return fd_ != -1;
> > > > -}
> > > > -
> > > > -/**
> > > > - * \brief Close the subdevice, releasing any resources acquired by open()
> > > > - */
> > > > -void V4L2Subdevice::close()
> > > > -{
> > > > -	if (!isOpen())
> > > > -		return;
> > > > -
> > > > -	::close(fd_);
> > > > -	fd_ = -1;
> > > > +	return V4L2Device::open(O_RDWR);
> > > >  }
> > > >
> > > >  /**
> > > > @@ -200,7 +162,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> > > >  	ImageFormats formats;
> > > >
> > > >  	if (pad >= entity_->pads().size()) {
> > > > -		LOG(V4L2Subdev, Error) << "Invalid pad: " << pad;
> > > > +		LOG(V4L2, Error) << "Invalid pad: " << pad;
> > > >  		return {};
> > > >  	}
> > > >
> > > > @@ -210,7 +172,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> > > >  			return {};
> > > >
> > > >  		if (formats.addFormat(code, sizes)) {
> > > > -			LOG(V4L2Subdev, Error)
> > > > +			LOG(V4L2, Error)
> > > >  				<< "Could not add sizes for media bus code "
> > > >  				<< code << " on pad " << pad;
> > > >  			return {};
> > > > @@ -232,10 +194,9 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> > > >  	subdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > >  	subdevFmt.pad = pad;
> > > >
> > > > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> > > > +	int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> > > >  	if (ret) {
> > > > -		ret = -errno;
> > > > -		LOG(V4L2Subdev, Error)
> > > > +		LOG(V4L2, Error)
> > > >  			<< "Unable to get format on pad " << pad
> > > >  			<< ": " << strerror(-ret);
> > > >  		return ret;
> > > > @@ -268,10 +229,9 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> > > >  	subdevFmt.format.height = format->size.height;
> > > >  	subdevFmt.format.code = format->mbus_code;
> > > >
> > > > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > > > +	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > > >  	if (ret) {
> > > > -		ret = -errno;
> > > > -		LOG(V4L2Subdev, Error)
> > > > +		LOG(V4L2, Error)
> > > >  			<< "Unable to set format on pad " << pad
> > > >  			<< ": " << strerror(-ret);
> > > >  		return ret;
> > > > @@ -305,11 +265,6 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
> > > >  	return new V4L2Subdevice(mediaEntity);
> > > >  }
> > > >
> > > > -std::string V4L2Subdevice::logPrefix() const
> > > > -{
> > > > -	return "'" + entity_->name() + "'";
> > > > -}
> > > > -
> > > >  std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > > >  {
> > > >  	std::vector<unsigned int> codes;
> > > > @@ -321,18 +276,17 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > > >  		mbusEnum.index = index;
> > > >  		mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > >
> > > > -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > > > +		ret = ioctl(VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > > >  		if (ret)
> > > >  			break;
> > > >
> > > >  		codes.push_back(mbusEnum.code);
> > > >  	}
> > > >
> > > > -	if (ret && errno != EINVAL) {
> > > > -		ret = errno;
> > > > -		LOG(V4L2Subdev, Error)
> > > > +	if (ret < 0 && ret != -EINVAL) {
> > > > +		LOG(V4L2, Error)
> > > >  			<< "Unable to enumerate formats on pad " << pad
> > > > -			<< ": " << strerror(ret);
> > > > +			<< ": " << strerror(-ret);
> > > >  		return {};
> > > >  	}
> > > >
> > > > @@ -352,7 +306,7 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > > >  		sizeEnum.code = code;
> > > >  		sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > >
> > > > -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > > > +		ret = ioctl(VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > > >  		if (ret)
> > > >  			break;
> > > >
> > > > @@ -360,9 +314,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > > >  				   sizeEnum.max_width, sizeEnum.max_height);
> > > >  	}
> > > >
> > > > -	if (ret && (errno != EINVAL && errno != ENOTTY)) {
> > > > -		ret = -errno;
> > > > -		LOG(V4L2Subdev, Error)
> > > > +	if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> > > > +		LOG(V4L2, Error)
> > > >  			<< "Unable to enumerate sizes on pad " << pad
> > > >  			<< ": " << strerror(-ret);
> > > >  		return {};
> > > > @@ -386,10 +339,9 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > > >  	sel.r.width = rect->w;
> > > >  	sel.r.height = rect->h;
> > > >
> > > > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);
> > > > +	int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);
> > > >  	if (ret < 0) {
> > > > -		ret = -errno;
> > > > -		LOG(V4L2Subdev, Error)
> > > > +		LOG(V4L2, Error)
> > > >  			<< "Unable to set rectangle " << target << " on pad "
> > > >  			<< pad << ": " << strerror(-ret);
> > > >  		return ret;
> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > > index 0e70498c8bb1..403e0b2e0653 100644
> > > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > > @@ -30,7 +30,7 @@
> > > >   */
> > > >  namespace libcamera {
> > > >
> > > > -LOG_DEFINE_CATEGORY(V4L2)
> > > > +LOG_DECLARE_CATEGORY(V4L2)
> > > >
> > > >  /**
> > > >   * \struct V4L2Capability
> > > > @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
> > > >   * \param[in] deviceNode The file-system path to the video device node
> > > >   */
> > > >  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > > > -	: deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),
> > > > +	: V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
> > > >  	  queuedBuffersCount_(0), fdEvent_(nullptr)
> > > >  {
> > > >  	/*
> > > > @@ -305,23 +305,12 @@ int V4L2VideoDevice::open()
> > > >  {
> > > >  	int ret;
> > > >
> > > > -	if (isOpen()) {
> > > > -		LOG(V4L2, Error) << "Device already open";
> > > > -		return -EBUSY;
> > > > -	}
> > > > -
> > > > -	ret = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);
> > > > -	if (ret < 0) {
> > > > -		ret = -errno;
> > > > -		LOG(V4L2, Error)
> > > > -			<< "Failed to open V4L2 device: " << strerror(-ret);
> > > > +	ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> > > > +	if (ret < 0)
> > > >  		return ret;
> > > > -	}
> > > > -	fd_ = ret;
> > > >
> > > > -	ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
> > > > +	ret = ioctl(VIDIOC_QUERYCAP, &caps_);
> > > >  	if (ret < 0) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error)
> > > >  			<< "Failed to query device capabilities: "
> > > >  			<< strerror(-ret);
> > > > @@ -342,20 +331,20 @@ int V4L2VideoDevice::open()
> > > >  	 * (POLLIN), and write notifications for OUTPUT devices (POLLOUT).
> > > >  	 */
> > > >  	if (caps_.isVideoCapture()) {
> > > > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> > > > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> > > >  		bufferType_ = caps_.isMultiplanar()
> > > >  			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> > > >  			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > > >  	} else if (caps_.isVideoOutput()) {
> > > > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> > > > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> > > >  		bufferType_ = caps_.isMultiplanar()
> > > >  			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> > > >  			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> > > >  	} else if (caps_.isMetaCapture()) {
> > > > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> > > > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> > > >  		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> > > >  	} else if (caps_.isMetaOutput()) {
> > > > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> > > > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> > > >  		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> > > >  	} else {
> > > >  		LOG(V4L2, Error) << "Device is not a supported type";
> > > > @@ -368,28 +357,18 @@ int V4L2VideoDevice::open()
> > > >  	return 0;
> > > >  }
> > > >
> > > > -/**
> > > > - * \brief Check if device is successfully opened
> > > > - * \return True if the device is open, false otherwise
> > > > - */
> > > > -bool V4L2VideoDevice::isOpen() const
> > > > -{
> > > > -	return fd_ != -1;
> > > > -}
> > > > -
> > > >  /**
> > > >   * \brief Close the device, releasing any resources acquired by open()
> > > >   */
> > > >  void V4L2VideoDevice::close()
> > > >  {
> > > > -	if (fd_ < 0)
> > > > +	if (!isOpen())
> > > >  		return;
> > > >
> > > >  	releaseBuffers();
> > > >  	delete fdEvent_;
> > > >
> > > > -	::close(fd_);
> > > > -	fd_ = -1;
> > > > +	V4L2Device::close();
> > > >  }
> > > >
> > > >  /**
> > > > @@ -410,17 +389,6 @@ void V4L2VideoDevice::close()
> > > >   * \return The string containing the device location
> > > >   */
> > > >
> > > > -/**
> > > > - * \fn V4L2VideoDevice::deviceNode()
> > > > - * \brief Retrieve the video device node path
> > > > - * \return The video device device node path
> > > > - */
> > > > -
> > > > -std::string V4L2VideoDevice::logPrefix() const
> > > > -{
> > > > -	return deviceNode_ + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> > > > -}
> > > > -
> > > >  /**
> > > >   * \brief Retrieve the image format set on the V4L2 device
> > > >   * \param[out] format The image format applied on the device
> > > > @@ -462,9 +430,8 @@ int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> > > >  	int ret;
> > > >
> > > >  	v4l2Format.type = bufferType_;
> > > > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > > > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> > > >  	if (ret) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> > > >  		return ret;
> > > >  	}
> > > > @@ -488,9 +455,8 @@ int V4L2VideoDevice::setFormatMeta(V4L2DeviceFormat *format)
> > > >  	v4l2Format.type = bufferType_;
> > > >  	pix->dataformat = format->fourcc;
> > > >  	pix->buffersize = format->planes[0].size;
> > > > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > > > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> > > >  	if (ret) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> > > >  		return ret;
> > > >  	}
> > > > @@ -516,9 +482,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
> > > >  	int ret;
> > > >
> > > >  	v4l2Format.type = bufferType_;
> > > > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > > > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> > > >  	if (ret) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> > > >  		return ret;
> > > >  	}
> > > > @@ -554,9 +519,8 @@ int V4L2VideoDevice::setFormatMultiplane(V4L2DeviceFormat *format)
> > > >  		pix->plane_fmt[i].sizeimage = format->planes[i].size;
> > > >  	}
> > > >
> > > > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > > > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> > > >  	if (ret) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> > > >  		return ret;
> > > >  	}
> > > > @@ -584,9 +548,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
> > > >  	int ret;
> > > >
> > > >  	v4l2Format.type = bufferType_;
> > > > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > > > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> > > >  	if (ret) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> > > >  		return ret;
> > > >  	}
> > > > @@ -613,9 +576,8 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)
> > > >  	pix->pixelformat = format->fourcc;
> > > >  	pix->bytesperline = format->planes[0].bpl;
> > > >  	pix->field = V4L2_FIELD_NONE;
> > > > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > > > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> > > >  	if (ret) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> > > >  		return ret;
> > > >  	}
> > > > @@ -670,9 +632,8 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)
> > > >  	rb.type = bufferType_;
> > > >  	rb.memory = memoryType_;
> > > >
> > > > -	ret = ioctl(fd_, VIDIOC_REQBUFS, &rb);
> > > > +	ret = ioctl(VIDIOC_REQBUFS, &rb);
> > > >  	if (ret < 0) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error)
> > > >  			<< "Unable to request " << count << " buffers: "
> > > >  			<< strerror(-ret);
> > > > @@ -722,9 +683,8 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)
> > > >  		buf.length = VIDEO_MAX_PLANES;
> > > >  		buf.m.planes = planes;
> > > >
> > > > -		ret = ioctl(fd_, VIDIOC_QUERYBUF, &buf);
> > > > +		ret = ioctl(VIDIOC_QUERYBUF, &buf);
> > > >  		if (ret < 0) {
> > > > -			ret = -errno;
> > > >  			LOG(V4L2, Error)
> > > >  				<< "Unable to query buffer " << i << ": "
> > > >  				<< strerror(-ret);
> > > > @@ -775,9 +735,8 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,
> > > >  	expbuf.plane = planeIndex;
> > > >  	expbuf.flags = O_RDWR;
> > > >
> > > > -	ret = ioctl(fd_, VIDIOC_EXPBUF, &expbuf);
> > > > +	ret = ioctl(VIDIOC_EXPBUF, &expbuf);
> > > >  	if (ret < 0) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error)
> > > >  			<< "Failed to export buffer: " << strerror(-ret);
> > > >  		return ret;
> > > > @@ -801,15 +760,14 @@ std::vector<unsigned int> V4L2VideoDevice::enumPixelformats()
> > > >  		pixelformatEnum.index = index;
> > > >  		pixelformatEnum.type = bufferType_;
> > > >
> > > > -		ret = ioctl(fd_, VIDIOC_ENUM_FMT, &pixelformatEnum);
> > > > +		ret = ioctl(VIDIOC_ENUM_FMT, &pixelformatEnum);
> > > >  		if (ret)
> > > >  			break;
> > > >
> > > >  		formats.push_back(pixelformatEnum.pixelformat);
> > > >  	}
> > > >
> > > > -	if (ret && errno != EINVAL) {
> > > > -		ret = -errno;
> > > > +	if (ret && ret != -EINVAL) {
> > > >  		LOG(V4L2, Error)
> > > >  			<< "Unable to enumerate pixel formats: "
> > > >  			<< strerror(-ret);
> > > > @@ -829,7 +787,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
> > > >  		frameSize.index = index;
> > > >  		frameSize.pixel_format = pixelFormat;
> > > >
> > > > -		ret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize);
> > > > +		ret = ioctl(VIDIOC_ENUM_FRAMESIZES, &frameSize);
> > > >  		if (ret)
> > > >  			break;
> > > >
> > > > @@ -867,8 +825,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
> > > >  		}
> > > >  	}
> > > >
> > > > -	if (ret && errno != EINVAL) {
> > > > -		ret = -errno;
> > > > +	if (ret && ret != -EINVAL) {
> > > >  		LOG(V4L2, Error)
> > > >  			<< "Unable to enumerate frame sizes: "
> > > >  			<< strerror(-ret);
> > > > @@ -969,9 +926,8 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> > > >
> > > >  	LOG(V4L2, Debug) << "Queueing buffer " << buf.index;
> > > >
> > > > -	ret = ioctl(fd_, VIDIOC_QBUF, &buf);
> > > > +	ret = ioctl(VIDIOC_QBUF, &buf);
> > > >  	if (ret < 0) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error)
> > > >  			<< "Failed to queue buffer " << buf.index << ": "
> > > >  			<< strerror(-ret);
> > > > @@ -1006,9 +962,8 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
> > > >  		buf.m.planes = planes;
> > > >  	}
> > > >
> > > > -	ret = ioctl(fd_, VIDIOC_DQBUF, &buf);
> > > > +	ret = ioctl(VIDIOC_DQBUF, &buf);
> > > >  	if (ret < 0) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error)
> > > >  			<< "Failed to dequeue buffer: " << strerror(-ret);
> > > >  		return nullptr;
> > > > @@ -1066,9 +1021,8 @@ int V4L2VideoDevice::streamOn()
> > > >  {
> > > >  	int ret;
> > > >
> > > > -	ret = ioctl(fd_, VIDIOC_STREAMON, &bufferType_);
> > > > +	ret = ioctl(VIDIOC_STREAMON, &bufferType_);
> > > >  	if (ret < 0) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error)
> > > >  			<< "Failed to start streaming: " << strerror(-ret);
> > > >  		return ret;
> > > > @@ -1089,9 +1043,8 @@ int V4L2VideoDevice::streamOff()
> > > >  {
> > > >  	int ret;
> > > >
> > > > -	ret = ioctl(fd_, VIDIOC_STREAMOFF, &bufferType_);
> > > > +	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> > > >  	if (ret < 0) {
> > > > -		ret = -errno;
> > > >  		LOG(V4L2, Error)
> > > >  			<< "Failed to stop streaming: " << strerror(-ret);
> > > >  		return ret;
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
>
>
>
> --
> Regards,
>
> Laurent Pinchart

Hi
Laurent,
On Wed, Jun 19, 2019 at 03:20:29PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Jun 19, 2019 at 01:05:46PM +0200, Jacopo Mondi wrote:
> > The V4L2 devices and subdevices share a few common operations,like
> > 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 increase, as the control support
> > implementation is identical for the two derived classes.
> >
> > 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.
> >

With your patch squashed on top and the other comment addressed, can I
have your tag (and push) or would you like another iteration?

> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/libcamera/include/v4l2_device.h      |  43 +++++++
> >  src/libcamera/include/v4l2_subdevice.h   |   9 +-
> >  src/libcamera/include/v4l2_videodevice.h |  10 +-
> >  src/libcamera/meson.build                |   2 +
> >  src/libcamera/v4l2_device.cpp            | 144 +++++++++++++++++++++++
> >  src/libcamera/v4l2_subdevice.cpp         |  84 +++----------
> >  src/libcamera/v4l2_videodevice.cpp       | 103 +++++-----------
> >  7 files changed, 239 insertions(+), 156 deletions(-)
> >  create mode 100644 src/libcamera/include/v4l2_device.h
> >  create mode 100644 src/libcamera/v4l2_device.cpp
> >
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > new file mode 100644
> > index 000000000000..2c26f3eae2b4
> > --- /dev/null
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -0,0 +1,43 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_device.h - Common base for V4L2 video devices and subdevices
> > + */
> > +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> > +#define __LIBCAMERA_V4L2_DEVICE_H__
> > +
> > +#include <string>
> > +
> > +#include "log.h"
> > +
> > +namespace libcamera {
> > +
> > +class V4L2Device : protected Loggable
> > +{
> > +public:
> > +	void close();
> > +	bool isOpen() const { return fd_ != -1; }
> > +	const std::string &deviceNode() const { return deviceNode_; }
> > +
> > +protected:
> > +	V4L2Device(const std::string &deviceNode, const std::string &logTag);
> > +	~V4L2Device() {}
> > +
> > +	int fd() { return fd_; }
> > +
> > +	int open(unsigned int flags);
> > +
> > +	int ioctl(unsigned long request, void *argp);
> > +
> > +	std::string logPrefix() const { return "'" + logTag_ + "'"; }
>
> Let's try not to inline methods that are more complex than just
> returning a member of the class.
>
> I didn't notice in my previous review that V4L2Subdevice was using the
> entity name as a prefix. I'm afraid my comment about moving logPrefix()
> to V4L2Device was a bad one :-/ However, V4L2Device should still inherit
> from Loggable. It will thus have a pure virtual logPrefix() inherited
> from Loggable, requiring the derived classes to implement that method,
> but allowing LOG() statements in V4L2Device to use the log prefix.
>
> Here's the diff I came up with, it can be squashed with this patch if
> you agree on the comments.
>
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 2c26f3eae2b4..8c83adab4d43 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -21,7 +21,7 @@ public:
>  	const std::string &deviceNode() const { return deviceNode_; }
>
>  protected:
> -	V4L2Device(const std::string &deviceNode, const std::string &logTag);
> +	V4L2Device(const std::string &deviceNode);
>  	~V4L2Device() {}
>
>  	int fd() { return fd_; }
> @@ -30,11 +30,8 @@ protected:
>
>  	int ioctl(unsigned long request, void *argp);
>
> -	std::string logPrefix() const { return "'" + logTag_ + "'"; }
> -
>  private:
>  	std::string deviceNode_;
> -	std::string logTag_;
>  	int fd_;
>  };
>
> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> index b1da4a87c553..9c077674f997 100644
> --- a/src/libcamera/include/v4l2_subdevice.h
> +++ b/src/libcamera/include/v4l2_subdevice.h
> @@ -52,6 +52,9 @@ public:
>  	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
>  					     const std::string &entity);
>
> +protected:
> +	std::string logPrefix() const;
> +
>  private:
>  	std::vector<unsigned int> enumPadCodes(unsigned int pad);
>  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index e73dec13f847..734b34f1dc53 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -147,6 +147,9 @@ public:
>  	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
>  					       const std::string &entity);
>
> +protected:
> +	std::string logPrefix() const;
> +
>  private:
>  	int getFormatMeta(V4L2DeviceFormat *format);
>  	int setFormatMeta(V4L2DeviceFormat *format);
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index eeed0a70fb0e..1f113780ba45 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -68,17 +68,15 @@ void V4L2Device::close()
>  /**
>   * \brief Construct a V4L2Device
>   * \param[in] deviceNode The device node filesystem path
> - * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> - * the device in the log output
>   *
>   * The V4L2Device constructor is protected and can only be accessed by the
>   * V4L2VideoDevice and V4L2Subdevice derived classes.
>   *
>   * Initialize the file descriptor to -1 and store the \a deviceNode to be used
>   * at open() time, and the \a logTag to prefix log messages with.
>   */
> -V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> -	: deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
> +V4L2Device::V4L2Device(const std::string &deviceNode)
> +	: deviceNode_(deviceNode), fd_(-1)
>  {
>  }
>
> @@ -99,6 +94,7 @@ V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
>   */
>  int V4L2Device::open(unsigned int flags)
>  {
> +	LOG(V4L2, Info) << "Opening device";
>  	if (isOpen()) {
>  		LOG(V4L2, Error) << "Device already open";
>  		return -EBUSY;
> @@ -135,10 +131,4 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
>  	return 0;
>  }
>
> -/**
> - * \fn V4L2Device::logPrefix()
> - * \brief Retrieve the tag to prefix log messages with
> - * \return The tag to prefix log messages with
> - */
> -
>  } /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index d0e1d717b26c..a188298de34c 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
>   * path
>   */
>  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> -	: V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
> +	: V4L2Device(entity->deviceNode()), entity_(entity)
>  {
>  }
>
> @@ -265,6 +265,11 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
>  	return new V4L2Subdevice(mediaEntity);
>  }
>
> +std::string V4L2Subdevice::logPrefix() const
> +{
> +	return "'" + entity_->name() + "'";
> +}
> +
>  std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
>  {
>  	std::vector<unsigned int> codes;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 403e0b2e0653..2de3751f467e 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
>   * \param[in] deviceNode The file-system path to the video device node
>   */
>  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> -	: V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
> +	: V4L2Device(deviceNode), bufferPool_(nullptr),
>  	  queuedBuffersCount_(0), fdEvent_(nullptr)
>  {
>  	/*
> @@ -389,6 +389,11 @@ void V4L2VideoDevice::close()
>   * \return The string containing the device location
>   */
>
> +std::string V4L2VideoDevice::logPrefix() const
> +{
> +	return deviceNode() + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> +}
> +
>  /**
>   * \brief Retrieve the image format set on the V4L2 device
>   * \param[out] format The image format applied on the device
>
> > +
> > +private:
> > +	std::string deviceNode_;
> > +	std::string logTag_;
> > +	int fd_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index 9afd28b6db02..b1da4a87c553 100644
> > --- a/src/libcamera/include/v4l2_subdevice.h
> > +++ b/src/libcamera/include/v4l2_subdevice.h
> > @@ -16,6 +16,7 @@
> >  #include "formats.h"
> >  #include "log.h"
> >  #include "media_object.h"
> > +#include "v4l2_device.h"
> >
> >  namespace libcamera {
> >
> > @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {
> >  	const std::string toString() const;
> >  };
> >
> > -class V4L2Subdevice : protected Loggable
> > +class V4L2Subdevice : public V4L2Device
> >  {
> >  public:
> >  	explicit V4L2Subdevice(const MediaEntity *entity);
> > @@ -37,8 +38,6 @@ public:
> >  	~V4L2Subdevice();
> >
> >  	int open();
> > -	bool isOpen() const;
> > -	void close();
> >
> >  	const MediaEntity *entity() const { return entity_; }
> >
> > @@ -53,9 +52,6 @@ public:
> >  	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
> >  					     const std::string &entity);
> >
> > -protected:
> > -	std::string logPrefix() const;
> > -
> >  private:
> >  	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> >  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > @@ -65,7 +61,6 @@ private:
> >  			 Rectangle *rect);
> >
> >  	const MediaEntity *entity_;
> > -	int fd_;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > index 895658805b47..e73dec13f847 100644
> > --- a/src/libcamera/include/v4l2_videodevice.h
> > +++ b/src/libcamera/include/v4l2_videodevice.h
> > @@ -18,6 +18,7 @@
> >
> >  #include "formats.h"
> >  #include "log.h"
> > +#include "v4l2_device.h"
> >
> >  namespace libcamera {
> >
> > @@ -112,7 +113,7 @@ public:
> >  	const std::string toString() const;
> >  };
> >
> > -class V4L2VideoDevice : protected Loggable
> > +class V4L2VideoDevice : public V4L2Device
> >  {
> >  public:
> >  	explicit V4L2VideoDevice(const std::string &deviceNode);
> > @@ -123,13 +124,11 @@ public:
> >  	V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
> >
> >  	int open();
> > -	bool isOpen() const;
> >  	void close();
> >
> >  	const char *driverName() const { return caps_.driver(); }
> >  	const char *deviceName() const { return caps_.card(); }
> >  	const char *busName() const { return caps_.bus_info(); }
> > -	const std::string &deviceNode() const { return deviceNode_; }
> >
> >  	int getFormat(V4L2DeviceFormat *format);
> >  	int setFormat(V4L2DeviceFormat *format);
> > @@ -148,9 +147,6 @@ public:
> >  	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
> >  					       const std::string &entity);
> >
> > -protected:
> > -	std::string logPrefix() const;
> > -
> >  private:
> >  	int getFormatMeta(V4L2DeviceFormat *format);
> >  	int setFormatMeta(V4L2DeviceFormat *format);
> > @@ -171,8 +167,6 @@ private:
> >  	Buffer *dequeueBuffer();
> >  	void bufferAvailable(EventNotifier *notifier);
> >
> > -	std::string deviceNode_;
> > -	int fd_;
> >  	V4L2Capability caps_;
> >
> >  	enum v4l2_buf_type bufferType_;
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 15ab53b1abbe..f26ad5b2dc57 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -23,6 +23,7 @@ libcamera_sources = files([
> >      'stream.cpp',
> >      'timer.cpp',
> >      'utils.cpp',
> > +    'v4l2_device.cpp',
> >      'v4l2_subdevice.cpp',
> >      'v4l2_videodevice.cpp',
> >  ])
> > @@ -41,6 +42,7 @@ libcamera_headers = files([
> >      'include/media_object.h',
> >      'include/pipeline_handler.h',
> >      'include/utils.h',
> > +    'include/v4l2_device.h',
> >      'include/v4l2_subdevice.h',
> >      'include/v4l2_videodevice.h',
> >  ])
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > new file mode 100644
> > index 000000000000..eeed0a70fb0e
> > --- /dev/null
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -0,0 +1,144 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_device.cpp - Common base for V4L2 video devices and subdevices
> > + */
> > +
> > +#include "v4l2_device.h"
> > +
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <sys/ioctl.h>
> > +#include <unistd.h>
> > +
> > +#include "log.h"
> > +
> > +/**
> > + * \file v4l2_device.h
> > + * \brief Common base for V4L2 devices and subdevices
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(V4L2)
> > +
> > +/**
> > + * \class V4L2Device
> > + * \brief Base class for V4L2VideoDevice and V4L2Subdevice
> > + *
> > + * The V4L2Device class groups together the methods and fields common to
> > + * both the V4L2VideoDevice and V4L2Subdevice classes, and provides a base
> > + * class whith methods to open and close the device node associated with the
> > + * device and to perform IOCTL system calls on it.
> > + *
> > + * The V4L2Device class cannot be instantiated directly, as its constructor
> > + * is protected. Users should instead create instances of one the derived
> > + * classes to model either a V4L2 video device or a V4L2 subdevice.
> > + */
> > +
> > +/**
> > + * \brief Close the device node
> > + *
> > + * Reset the file descriptor to -1
> > + */
> > +void V4L2Device::close()
> > +{
> > +	if (!isOpen())
> > +		return;
> > +
> > +	if (::close(fd_) < 0)
> > +		LOG(V4L2, Error) << "Failed to close V4L2 device: "
> > +				 << strerror(errno);
> > +	fd_ = -1;
> > +}
> > +
> > +/**
> > + * \fn V4L2Device::isOpen()
> > + * \brief Check if the V4L2 device node is open
> > + * \return True if the V4L2 device node is open, false otherwise
> > + */
> > +
> > +/**
> > + * \fn V4L2Device::deviceNode()
> > + * \brief Retrieve the device node path
> > + * \return The device node path
> > + */
> > +
> > +/**
> > + * \brief Construct a V4L2Device
> > + * \param[in] deviceNode The device node filesystem path
> > + * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> > + * the device in the log output
> > + *
> > + * The V4L2Device constructor is protected and can only be accessed by the
> > + * V4L2VideoDevice and V4L2Subdevice derived classes.
>
> I would drop this sentence.
>
> > + *
> > + * Initialize the file descriptor to -1 and store the \a deviceNode to be used
> > + * at open() time, and the \a logTag to prefix log messages with.
> > + */
> > +V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> > +	: deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
> > +{
> > +}
> > +
> > +/**
> > + * \fn V4L2Device::fd()
> > + * \brief Retrieve the V4L2 device file descriptor
> > + * \return The V4L2 device file descriptor, -1 if the device node is not open
> > + */
> > +
> > +/**
> > + * \brief Open a V4L2 device node
> > + * \param[in] flags Access mode flags
> > + *
> > + * Open the device node path with the provided access mode \a flags and
> > + * initialize the file descriptor, which was initially set to -1.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int V4L2Device::open(unsigned int flags)
> > +{
> > +	if (isOpen()) {
> > +		LOG(V4L2, Error) << "Device already open";
> > +		return -EBUSY;
> > +	}
> > +
> > +	int ret = ::open(deviceNode_.c_str(), flags);
> > +	if (ret < 0) {
> > +		ret = -errno;
> > +		LOG(V4L2, Error) << "Failed to open V4L2 device: "
> > +				 << strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	fd_ = ret;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \brief Perform an IOCTL system call on the device node
> > + * \param[in] request The IOCTL request code
> > + * \param[in] argp A pointer to the IOCTL argument
> > + * \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;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \fn V4L2Device::logPrefix()
> > + * \brief Retrieve the tag to prefix log messages with
> > + * \return The tag to prefix log messages with
> > + */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 48f1fcb30ed4..d0e1d717b26c 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -29,7 +29,7 @@
> >
> >  namespace libcamera {
> >
> > -LOG_DEFINE_CATEGORY(V4L2Subdev)
> > +LOG_DECLARE_CATEGORY(V4L2)
> >
> >  /**
> >   * \struct V4L2SubdeviceFormat
> > @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
> >   * path
> >   */
> >  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> > -	: entity_(entity), fd_(-1)
> > +	: V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
> >  {
> >  }
> >
> > @@ -117,45 +117,7 @@ V4L2Subdevice::~V4L2Subdevice()
> >   */
> >  int V4L2Subdevice::open()
> >  {
> > -	int ret;
> > -
> > -	if (isOpen()) {
> > -		LOG(V4L2Subdev, Error) << "Device already open";
> > -		return -EBUSY;
> > -	}
> > -
> > -	ret = ::open(entity_->deviceNode().c_str(), O_RDWR);
> > -	if (ret < 0) {
> > -		ret = -errno;
> > -		LOG(V4L2Subdev, Error)
> > -			<< "Failed to open V4L2 subdevice '"
> > -			<< entity_->deviceNode() << "': " << strerror(-ret);
> > -		return ret;
> > -	}
> > -	fd_ = ret;
> > -
> > -	return 0;
> > -}
> > -
> > -/**
> > - * \brief Check if the subdevice is open
> > - * \return True if the subdevice is open, false otherwise
> > - */
> > -bool V4L2Subdevice::isOpen() const
> > -{
> > -	return fd_ != -1;
> > -}
> > -
> > -/**
> > - * \brief Close the subdevice, releasing any resources acquired by open()
> > - */
> > -void V4L2Subdevice::close()
> > -{
> > -	if (!isOpen())
> > -		return;
> > -
> > -	::close(fd_);
> > -	fd_ = -1;
> > +	return V4L2Device::open(O_RDWR);
> >  }
> >
> >  /**
> > @@ -200,7 +162,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> >  	ImageFormats formats;
> >
> >  	if (pad >= entity_->pads().size()) {
> > -		LOG(V4L2Subdev, Error) << "Invalid pad: " << pad;
> > +		LOG(V4L2, Error) << "Invalid pad: " << pad;
> >  		return {};
> >  	}
> >
> > @@ -210,7 +172,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> >  			return {};
> >
> >  		if (formats.addFormat(code, sizes)) {
> > -			LOG(V4L2Subdev, Error)
> > +			LOG(V4L2, Error)
> >  				<< "Could not add sizes for media bus code "
> >  				<< code << " on pad " << pad;
> >  			return {};
> > @@ -232,10 +194,9 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> >  	subdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >  	subdevFmt.pad = pad;
> >
> > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> > +	int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> >  	if (ret) {
> > -		ret = -errno;
> > -		LOG(V4L2Subdev, Error)
> > +		LOG(V4L2, Error)
> >  			<< "Unable to get format on pad " << pad
> >  			<< ": " << strerror(-ret);
> >  		return ret;
> > @@ -268,10 +229,9 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> >  	subdevFmt.format.height = format->size.height;
> >  	subdevFmt.format.code = format->mbus_code;
> >
> > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > +	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> >  	if (ret) {
> > -		ret = -errno;
> > -		LOG(V4L2Subdev, Error)
> > +		LOG(V4L2, Error)
> >  			<< "Unable to set format on pad " << pad
> >  			<< ": " << strerror(-ret);
> >  		return ret;
> > @@ -305,11 +265,6 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
> >  	return new V4L2Subdevice(mediaEntity);
> >  }
> >
> > -std::string V4L2Subdevice::logPrefix() const
> > -{
> > -	return "'" + entity_->name() + "'";
> > -}
> > -
> >  std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> >  {
> >  	std::vector<unsigned int> codes;
> > @@ -321,18 +276,17 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> >  		mbusEnum.index = index;
> >  		mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >
> > -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > +		ret = ioctl(VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> >  		if (ret)
> >  			break;
> >
> >  		codes.push_back(mbusEnum.code);
> >  	}
> >
> > -	if (ret && errno != EINVAL) {
> > -		ret = errno;
> > -		LOG(V4L2Subdev, Error)
> > +	if (ret < 0 && ret != -EINVAL) {
> > +		LOG(V4L2, Error)
> >  			<< "Unable to enumerate formats on pad " << pad
> > -			<< ": " << strerror(ret);
> > +			<< ": " << strerror(-ret);
> >  		return {};
> >  	}
> >
> > @@ -352,7 +306,7 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> >  		sizeEnum.code = code;
> >  		sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >
> > -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > +		ret = ioctl(VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> >  		if (ret)
> >  			break;
> >
> > @@ -360,9 +314,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> >  				   sizeEnum.max_width, sizeEnum.max_height);
> >  	}
> >
> > -	if (ret && (errno != EINVAL && errno != ENOTTY)) {
> > -		ret = -errno;
> > -		LOG(V4L2Subdev, Error)
> > +	if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> > +		LOG(V4L2, Error)
> >  			<< "Unable to enumerate sizes on pad " << pad
> >  			<< ": " << strerror(-ret);
> >  		return {};
> > @@ -386,10 +339,9 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> >  	sel.r.width = rect->w;
> >  	sel.r.height = rect->h;
> >
> > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);
> > +	int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);
> >  	if (ret < 0) {
> > -		ret = -errno;
> > -		LOG(V4L2Subdev, Error)
> > +		LOG(V4L2, Error)
> >  			<< "Unable to set rectangle " << target << " on pad "
> >  			<< pad << ": " << strerror(-ret);
> >  		return ret;
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 0e70498c8bb1..403e0b2e0653 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -30,7 +30,7 @@
> >   */
> >  namespace libcamera {
> >
> > -LOG_DEFINE_CATEGORY(V4L2)
> > +LOG_DECLARE_CATEGORY(V4L2)
> >
> >  /**
> >   * \struct V4L2Capability
> > @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
> >   * \param[in] deviceNode The file-system path to the video device node
> >   */
> >  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > -	: deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),
> > +	: V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
> >  	  queuedBuffersCount_(0), fdEvent_(nullptr)
> >  {
> >  	/*
> > @@ -305,23 +305,12 @@ int V4L2VideoDevice::open()
> >  {
> >  	int ret;
> >
> > -	if (isOpen()) {
> > -		LOG(V4L2, Error) << "Device already open";
> > -		return -EBUSY;
> > -	}
> > -
> > -	ret = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);
> > -	if (ret < 0) {
> > -		ret = -errno;
> > -		LOG(V4L2, Error)
> > -			<< "Failed to open V4L2 device: " << strerror(-ret);
> > +	ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> > +	if (ret < 0)
> >  		return ret;
> > -	}
> > -	fd_ = ret;
> >
> > -	ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
> > +	ret = ioctl(VIDIOC_QUERYCAP, &caps_);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Failed to query device capabilities: "
> >  			<< strerror(-ret);
> > @@ -342,20 +331,20 @@ int V4L2VideoDevice::open()
> >  	 * (POLLIN), and write notifications for OUTPUT devices (POLLOUT).
> >  	 */
> >  	if (caps_.isVideoCapture()) {
> > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >  		bufferType_ = caps_.isMultiplanar()
> >  			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> >  			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >  	} else if (caps_.isVideoOutput()) {
> > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >  		bufferType_ = caps_.isMultiplanar()
> >  			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> >  			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >  	} else if (caps_.isMetaCapture()) {
> > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> >  		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> >  	} else if (caps_.isMetaOutput()) {
> > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> >  		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> >  	} else {
> >  		LOG(V4L2, Error) << "Device is not a supported type";
> > @@ -368,28 +357,18 @@ int V4L2VideoDevice::open()
> >  	return 0;
> >  }
> >
> > -/**
> > - * \brief Check if device is successfully opened
> > - * \return True if the device is open, false otherwise
> > - */
> > -bool V4L2VideoDevice::isOpen() const
> > -{
> > -	return fd_ != -1;
> > -}
> > -
> >  /**
> >   * \brief Close the device, releasing any resources acquired by open()
> >   */
> >  void V4L2VideoDevice::close()
> >  {
> > -	if (fd_ < 0)
> > +	if (!isOpen())
> >  		return;
> >
> >  	releaseBuffers();
> >  	delete fdEvent_;
> >
> > -	::close(fd_);
> > -	fd_ = -1;
> > +	V4L2Device::close();
> >  }
> >
> >  /**
> > @@ -410,17 +389,6 @@ void V4L2VideoDevice::close()
> >   * \return The string containing the device location
> >   */
> >
> > -/**
> > - * \fn V4L2VideoDevice::deviceNode()
> > - * \brief Retrieve the video device node path
> > - * \return The video device device node path
> > - */
> > -
> > -std::string V4L2VideoDevice::logPrefix() const
> > -{
> > -	return deviceNode_ + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> > -}
> > -
> >  /**
> >   * \brief Retrieve the image format set on the V4L2 device
> >   * \param[out] format The image format applied on the device
> > @@ -462,9 +430,8 @@ int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> >  	int ret;
> >
> >  	v4l2Format.type = bufferType_;
> > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> >  	if (ret) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> >  		return ret;
> >  	}
> > @@ -488,9 +455,8 @@ int V4L2VideoDevice::setFormatMeta(V4L2DeviceFormat *format)
> >  	v4l2Format.type = bufferType_;
> >  	pix->dataformat = format->fourcc;
> >  	pix->buffersize = format->planes[0].size;
> > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> >  	if (ret) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> >  		return ret;
> >  	}
> > @@ -516,9 +482,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
> >  	int ret;
> >
> >  	v4l2Format.type = bufferType_;
> > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> >  	if (ret) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> >  		return ret;
> >  	}
> > @@ -554,9 +519,8 @@ int V4L2VideoDevice::setFormatMultiplane(V4L2DeviceFormat *format)
> >  		pix->plane_fmt[i].sizeimage = format->planes[i].size;
> >  	}
> >
> > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> >  	if (ret) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> >  		return ret;
> >  	}
> > @@ -584,9 +548,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
> >  	int ret;
> >
> >  	v4l2Format.type = bufferType_;
> > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> >  	if (ret) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> >  		return ret;
> >  	}
> > @@ -613,9 +576,8 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)
> >  	pix->pixelformat = format->fourcc;
> >  	pix->bytesperline = format->planes[0].bpl;
> >  	pix->field = V4L2_FIELD_NONE;
> > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> >  	if (ret) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> >  		return ret;
> >  	}
> > @@ -670,9 +632,8 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)
> >  	rb.type = bufferType_;
> >  	rb.memory = memoryType_;
> >
> > -	ret = ioctl(fd_, VIDIOC_REQBUFS, &rb);
> > +	ret = ioctl(VIDIOC_REQBUFS, &rb);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Unable to request " << count << " buffers: "
> >  			<< strerror(-ret);
> > @@ -722,9 +683,8 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)
> >  		buf.length = VIDEO_MAX_PLANES;
> >  		buf.m.planes = planes;
> >
> > -		ret = ioctl(fd_, VIDIOC_QUERYBUF, &buf);
> > +		ret = ioctl(VIDIOC_QUERYBUF, &buf);
> >  		if (ret < 0) {
> > -			ret = -errno;
> >  			LOG(V4L2, Error)
> >  				<< "Unable to query buffer " << i << ": "
> >  				<< strerror(-ret);
> > @@ -775,9 +735,8 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,
> >  	expbuf.plane = planeIndex;
> >  	expbuf.flags = O_RDWR;
> >
> > -	ret = ioctl(fd_, VIDIOC_EXPBUF, &expbuf);
> > +	ret = ioctl(VIDIOC_EXPBUF, &expbuf);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Failed to export buffer: " << strerror(-ret);
> >  		return ret;
> > @@ -801,15 +760,14 @@ std::vector<unsigned int> V4L2VideoDevice::enumPixelformats()
> >  		pixelformatEnum.index = index;
> >  		pixelformatEnum.type = bufferType_;
> >
> > -		ret = ioctl(fd_, VIDIOC_ENUM_FMT, &pixelformatEnum);
> > +		ret = ioctl(VIDIOC_ENUM_FMT, &pixelformatEnum);
> >  		if (ret)
> >  			break;
> >
> >  		formats.push_back(pixelformatEnum.pixelformat);
> >  	}
> >
> > -	if (ret && errno != EINVAL) {
> > -		ret = -errno;
> > +	if (ret && ret != -EINVAL) {
> >  		LOG(V4L2, Error)
> >  			<< "Unable to enumerate pixel formats: "
> >  			<< strerror(-ret);
> > @@ -829,7 +787,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
> >  		frameSize.index = index;
> >  		frameSize.pixel_format = pixelFormat;
> >
> > -		ret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize);
> > +		ret = ioctl(VIDIOC_ENUM_FRAMESIZES, &frameSize);
> >  		if (ret)
> >  			break;
> >
> > @@ -867,8 +825,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
> >  		}
> >  	}
> >
> > -	if (ret && errno != EINVAL) {
> > -		ret = -errno;
> > +	if (ret && ret != -EINVAL) {
> >  		LOG(V4L2, Error)
> >  			<< "Unable to enumerate frame sizes: "
> >  			<< strerror(-ret);
> > @@ -969,9 +926,8 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> >
> >  	LOG(V4L2, Debug) << "Queueing buffer " << buf.index;
> >
> > -	ret = ioctl(fd_, VIDIOC_QBUF, &buf);
> > +	ret = ioctl(VIDIOC_QBUF, &buf);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Failed to queue buffer " << buf.index << ": "
> >  			<< strerror(-ret);
> > @@ -1006,9 +962,8 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
> >  		buf.m.planes = planes;
> >  	}
> >
> > -	ret = ioctl(fd_, VIDIOC_DQBUF, &buf);
> > +	ret = ioctl(VIDIOC_DQBUF, &buf);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Failed to dequeue buffer: " << strerror(-ret);
> >  		return nullptr;
> > @@ -1066,9 +1021,8 @@ int V4L2VideoDevice::streamOn()
> >  {
> >  	int ret;
> >
> > -	ret = ioctl(fd_, VIDIOC_STREAMON, &bufferType_);
> > +	ret = ioctl(VIDIOC_STREAMON, &bufferType_);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Failed to start streaming: " << strerror(-ret);
> >  		return ret;
> > @@ -1089,9 +1043,8 @@ int V4L2VideoDevice::streamOff()
> >  {
> >  	int ret;
> >
> > -	ret = ioctl(fd_, VIDIOC_STREAMOFF, &bufferType_);
> > +	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> >  	if (ret < 0) {
> > -		ret = -errno;
> >  		LOG(V4L2, Error)
> >  			<< "Failed to stop streaming: " << strerror(-ret);
> >  		return ret;
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Wed, Jun 19, 2019 at 03:07:27PM +0200, Jacopo Mondi wrote:
> On Wed, Jun 19, 2019 at 03:20:29PM +0300, Laurent Pinchart wrote:
> > On Wed, Jun 19, 2019 at 01:05:46PM +0200, Jacopo Mondi wrote:
> > > The V4L2 devices and subdevices share a few common operations,like
> > > 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 increase, as the control support
> > > implementation is identical for the two derived classes.
> > >
> > > 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.
> > >
> 
> With your patch squashed on top and the other comment addressed, can I
> have your tag (and push) or would you like another iteration?

I was thinking on commenting about the order of methods in the
v4l2_device.cpp file, but that can be addressed later, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  src/libcamera/include/v4l2_device.h      |  43 +++++++
> > >  src/libcamera/include/v4l2_subdevice.h   |   9 +-
> > >  src/libcamera/include/v4l2_videodevice.h |  10 +-
> > >  src/libcamera/meson.build                |   2 +
> > >  src/libcamera/v4l2_device.cpp            | 144 +++++++++++++++++++++++
> > >  src/libcamera/v4l2_subdevice.cpp         |  84 +++----------
> > >  src/libcamera/v4l2_videodevice.cpp       | 103 +++++-----------
> > >  7 files changed, 239 insertions(+), 156 deletions(-)
> > >  create mode 100644 src/libcamera/include/v4l2_device.h
> > >  create mode 100644 src/libcamera/v4l2_device.cpp
> > >
> > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > > new file mode 100644
> > > index 000000000000..2c26f3eae2b4
> > > --- /dev/null
> > > +++ b/src/libcamera/include/v4l2_device.h
> > > @@ -0,0 +1,43 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_device.h - Common base for V4L2 video devices and subdevices
> > > + */
> > > +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> > > +#define __LIBCAMERA_V4L2_DEVICE_H__
> > > +
> > > +#include <string>
> > > +
> > > +#include "log.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +class V4L2Device : protected Loggable
> > > +{
> > > +public:
> > > +	void close();
> > > +	bool isOpen() const { return fd_ != -1; }
> > > +	const std::string &deviceNode() const { return deviceNode_; }
> > > +
> > > +protected:
> > > +	V4L2Device(const std::string &deviceNode, const std::string &logTag);
> > > +	~V4L2Device() {}
> > > +
> > > +	int fd() { return fd_; }
> > > +
> > > +	int open(unsigned int flags);
> > > +
> > > +	int ioctl(unsigned long request, void *argp);
> > > +
> > > +	std::string logPrefix() const { return "'" + logTag_ + "'"; }
> >
> > Let's try not to inline methods that are more complex than just
> > returning a member of the class.
> >
> > I didn't notice in my previous review that V4L2Subdevice was using the
> > entity name as a prefix. I'm afraid my comment about moving logPrefix()
> > to V4L2Device was a bad one :-/ However, V4L2Device should still inherit
> > from Loggable. It will thus have a pure virtual logPrefix() inherited
> > from Loggable, requiring the derived classes to implement that method,
> > but allowing LOG() statements in V4L2Device to use the log prefix.
> >
> > Here's the diff I came up with, it can be squashed with this patch if
> > you agree on the comments.
> >
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index 2c26f3eae2b4..8c83adab4d43 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -21,7 +21,7 @@ public:
> >  	const std::string &deviceNode() const { return deviceNode_; }
> >
> >  protected:
> > -	V4L2Device(const std::string &deviceNode, const std::string &logTag);
> > +	V4L2Device(const std::string &deviceNode);
> >  	~V4L2Device() {}
> >
> >  	int fd() { return fd_; }
> > @@ -30,11 +30,8 @@ protected:
> >
> >  	int ioctl(unsigned long request, void *argp);
> >
> > -	std::string logPrefix() const { return "'" + logTag_ + "'"; }
> > -
> >  private:
> >  	std::string deviceNode_;
> > -	std::string logTag_;
> >  	int fd_;
> >  };
> >
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index b1da4a87c553..9c077674f997 100644
> > --- a/src/libcamera/include/v4l2_subdevice.h
> > +++ b/src/libcamera/include/v4l2_subdevice.h
> > @@ -52,6 +52,9 @@ public:
> >  	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
> >  					     const std::string &entity);
> >
> > +protected:
> > +	std::string logPrefix() const;
> > +
> >  private:
> >  	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> >  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > index e73dec13f847..734b34f1dc53 100644
> > --- a/src/libcamera/include/v4l2_videodevice.h
> > +++ b/src/libcamera/include/v4l2_videodevice.h
> > @@ -147,6 +147,9 @@ public:
> >  	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
> >  					       const std::string &entity);
> >
> > +protected:
> > +	std::string logPrefix() const;
> > +
> >  private:
> >  	int getFormatMeta(V4L2DeviceFormat *format);
> >  	int setFormatMeta(V4L2DeviceFormat *format);
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index eeed0a70fb0e..1f113780ba45 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -68,17 +68,15 @@ void V4L2Device::close()
> >  /**
> >   * \brief Construct a V4L2Device
> >   * \param[in] deviceNode The device node filesystem path
> > - * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> > - * the device in the log output
> >   *
> >   * The V4L2Device constructor is protected and can only be accessed by the
> >   * V4L2VideoDevice and V4L2Subdevice derived classes.
> >   *
> >   * Initialize the file descriptor to -1 and store the \a deviceNode to be used
> >   * at open() time, and the \a logTag to prefix log messages with.
> >   */
> > -V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> > -	: deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
> > +V4L2Device::V4L2Device(const std::string &deviceNode)
> > +	: deviceNode_(deviceNode), fd_(-1)
> >  {
> >  }
> >
> > @@ -99,6 +94,7 @@ V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> >   */
> >  int V4L2Device::open(unsigned int flags)
> >  {
> > +	LOG(V4L2, Info) << "Opening device";
> >  	if (isOpen()) {
> >  		LOG(V4L2, Error) << "Device already open";
> >  		return -EBUSY;
> > @@ -135,10 +131,4 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> >  	return 0;
> >  }
> >
> > -/**
> > - * \fn V4L2Device::logPrefix()
> > - * \brief Retrieve the tag to prefix log messages with
> > - * \return The tag to prefix log messages with
> > - */
> > -
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index d0e1d717b26c..a188298de34c 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
> >   * path
> >   */
> >  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> > -	: V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
> > +	: V4L2Device(entity->deviceNode()), entity_(entity)
> >  {
> >  }
> >
> > @@ -265,6 +265,11 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
> >  	return new V4L2Subdevice(mediaEntity);
> >  }
> >
> > +std::string V4L2Subdevice::logPrefix() const
> > +{
> > +	return "'" + entity_->name() + "'";
> > +}
> > +
> >  std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> >  {
> >  	std::vector<unsigned int> codes;
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 403e0b2e0653..2de3751f467e 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
> >   * \param[in] deviceNode The file-system path to the video device node
> >   */
> >  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > -	: V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
> > +	: V4L2Device(deviceNode), bufferPool_(nullptr),
> >  	  queuedBuffersCount_(0), fdEvent_(nullptr)
> >  {
> >  	/*
> > @@ -389,6 +389,11 @@ void V4L2VideoDevice::close()
> >   * \return The string containing the device location
> >   */
> >
> > +std::string V4L2VideoDevice::logPrefix() const
> > +{
> > +	return deviceNode() + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> > +}
> > +
> >  /**
> >   * \brief Retrieve the image format set on the V4L2 device
> >   * \param[out] format The image format applied on the device
> >
> > > +
> > > +private:
> > > +	std::string deviceNode_;
> > > +	std::string logTag_;
> > > +	int fd_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
> > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > > index 9afd28b6db02..b1da4a87c553 100644
> > > --- a/src/libcamera/include/v4l2_subdevice.h
> > > +++ b/src/libcamera/include/v4l2_subdevice.h
> > > @@ -16,6 +16,7 @@
> > >  #include "formats.h"
> > >  #include "log.h"
> > >  #include "media_object.h"
> > > +#include "v4l2_device.h"
> > >
> > >  namespace libcamera {
> > >
> > > @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {
> > >  	const std::string toString() const;
> > >  };
> > >
> > > -class V4L2Subdevice : protected Loggable
> > > +class V4L2Subdevice : public V4L2Device
> > >  {
> > >  public:
> > >  	explicit V4L2Subdevice(const MediaEntity *entity);
> > > @@ -37,8 +38,6 @@ public:
> > >  	~V4L2Subdevice();
> > >
> > >  	int open();
> > > -	bool isOpen() const;
> > > -	void close();
> > >
> > >  	const MediaEntity *entity() const { return entity_; }
> > >
> > > @@ -53,9 +52,6 @@ public:
> > >  	static V4L2Subdevice *fromEntityName(const MediaDevice *media,
> > >  					     const std::string &entity);
> > >
> > > -protected:
> > > -	std::string logPrefix() const;
> > > -
> > >  private:
> > >  	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> > >  	std::vector<SizeRange> enumPadSizes(unsigned int pad,
> > > @@ -65,7 +61,6 @@ private:
> > >  			 Rectangle *rect);
> > >
> > >  	const MediaEntity *entity_;
> > > -	int fd_;
> > >  };
> > >
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > > index 895658805b47..e73dec13f847 100644
> > > --- a/src/libcamera/include/v4l2_videodevice.h
> > > +++ b/src/libcamera/include/v4l2_videodevice.h
> > > @@ -18,6 +18,7 @@
> > >
> > >  #include "formats.h"
> > >  #include "log.h"
> > > +#include "v4l2_device.h"
> > >
> > >  namespace libcamera {
> > >
> > > @@ -112,7 +113,7 @@ public:
> > >  	const std::string toString() const;
> > >  };
> > >
> > > -class V4L2VideoDevice : protected Loggable
> > > +class V4L2VideoDevice : public V4L2Device
> > >  {
> > >  public:
> > >  	explicit V4L2VideoDevice(const std::string &deviceNode);
> > > @@ -123,13 +124,11 @@ public:
> > >  	V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
> > >
> > >  	int open();
> > > -	bool isOpen() const;
> > >  	void close();
> > >
> > >  	const char *driverName() const { return caps_.driver(); }
> > >  	const char *deviceName() const { return caps_.card(); }
> > >  	const char *busName() const { return caps_.bus_info(); }
> > > -	const std::string &deviceNode() const { return deviceNode_; }
> > >
> > >  	int getFormat(V4L2DeviceFormat *format);
> > >  	int setFormat(V4L2DeviceFormat *format);
> > > @@ -148,9 +147,6 @@ public:
> > >  	static V4L2VideoDevice *fromEntityName(const MediaDevice *media,
> > >  					       const std::string &entity);
> > >
> > > -protected:
> > > -	std::string logPrefix() const;
> > > -
> > >  private:
> > >  	int getFormatMeta(V4L2DeviceFormat *format);
> > >  	int setFormatMeta(V4L2DeviceFormat *format);
> > > @@ -171,8 +167,6 @@ private:
> > >  	Buffer *dequeueBuffer();
> > >  	void bufferAvailable(EventNotifier *notifier);
> > >
> > > -	std::string deviceNode_;
> > > -	int fd_;
> > >  	V4L2Capability caps_;
> > >
> > >  	enum v4l2_buf_type bufferType_;
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index 15ab53b1abbe..f26ad5b2dc57 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -23,6 +23,7 @@ libcamera_sources = files([
> > >      'stream.cpp',
> > >      'timer.cpp',
> > >      'utils.cpp',
> > > +    'v4l2_device.cpp',
> > >      'v4l2_subdevice.cpp',
> > >      'v4l2_videodevice.cpp',
> > >  ])
> > > @@ -41,6 +42,7 @@ libcamera_headers = files([
> > >      'include/media_object.h',
> > >      'include/pipeline_handler.h',
> > >      'include/utils.h',
> > > +    'include/v4l2_device.h',
> > >      'include/v4l2_subdevice.h',
> > >      'include/v4l2_videodevice.h',
> > >  ])
> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > new file mode 100644
> > > index 000000000000..eeed0a70fb0e
> > > --- /dev/null
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -0,0 +1,144 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * v4l2_device.cpp - Common base for V4L2 video devices and subdevices
> > > + */
> > > +
> > > +#include "v4l2_device.h"
> > > +
> > > +#include <fcntl.h>
> > > +#include <string.h>
> > > +#include <sys/ioctl.h>
> > > +#include <unistd.h>
> > > +
> > > +#include "log.h"
> > > +
> > > +/**
> > > + * \file v4l2_device.h
> > > + * \brief Common base for V4L2 devices and subdevices
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(V4L2)
> > > +
> > > +/**
> > > + * \class V4L2Device
> > > + * \brief Base class for V4L2VideoDevice and V4L2Subdevice
> > > + *
> > > + * The V4L2Device class groups together the methods and fields common to
> > > + * both the V4L2VideoDevice and V4L2Subdevice classes, and provides a base
> > > + * class whith methods to open and close the device node associated with the
> > > + * device and to perform IOCTL system calls on it.
> > > + *
> > > + * The V4L2Device class cannot be instantiated directly, as its constructor
> > > + * is protected. Users should instead create instances of one the derived
> > > + * classes to model either a V4L2 video device or a V4L2 subdevice.
> > > + */
> > > +
> > > +/**
> > > + * \brief Close the device node
> > > + *
> > > + * Reset the file descriptor to -1
> > > + */
> > > +void V4L2Device::close()
> > > +{
> > > +	if (!isOpen())
> > > +		return;
> > > +
> > > +	if (::close(fd_) < 0)
> > > +		LOG(V4L2, Error) << "Failed to close V4L2 device: "
> > > +				 << strerror(errno);
> > > +	fd_ = -1;
> > > +}
> > > +
> > > +/**
> > > + * \fn V4L2Device::isOpen()
> > > + * \brief Check if the V4L2 device node is open
> > > + * \return True if the V4L2 device node is open, false otherwise
> > > + */
> > > +
> > > +/**
> > > + * \fn V4L2Device::deviceNode()
> > > + * \brief Retrieve the device node path
> > > + * \return The device node path
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct a V4L2Device
> > > + * \param[in] deviceNode The device node filesystem path
> > > + * \param[in] logTag The tag to prefix log messages with. Helpful to identify
> > > + * the device in the log output
> > > + *
> > > + * The V4L2Device constructor is protected and can only be accessed by the
> > > + * V4L2VideoDevice and V4L2Subdevice derived classes.
> >
> > I would drop this sentence.
> >
> > > + *
> > > + * Initialize the file descriptor to -1 and store the \a deviceNode to be used
> > > + * at open() time, and the \a logTag to prefix log messages with.
> > > + */
> > > +V4L2Device::V4L2Device(const std::string &deviceNode, const std::string &logTag)
> > > +	: deviceNode_(deviceNode), logTag_(logTag), fd_(-1)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \fn V4L2Device::fd()
> > > + * \brief Retrieve the V4L2 device file descriptor
> > > + * \return The V4L2 device file descriptor, -1 if the device node is not open
> > > + */
> > > +
> > > +/**
> > > + * \brief Open a V4L2 device node
> > > + * \param[in] flags Access mode flags
> > > + *
> > > + * Open the device node path with the provided access mode \a flags and
> > > + * initialize the file descriptor, which was initially set to -1.
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int V4L2Device::open(unsigned int flags)
> > > +{
> > > +	if (isOpen()) {
> > > +		LOG(V4L2, Error) << "Device already open";
> > > +		return -EBUSY;
> > > +	}
> > > +
> > > +	int ret = ::open(deviceNode_.c_str(), flags);
> > > +	if (ret < 0) {
> > > +		ret = -errno;
> > > +		LOG(V4L2, Error) << "Failed to open V4L2 device: "
> > > +				 << strerror(-ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	fd_ = ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \brief Perform an IOCTL system call on the device node
> > > + * \param[in] request The IOCTL request code
> > > + * \param[in] argp A pointer to the IOCTL argument
> > > + * \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;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \fn V4L2Device::logPrefix()
> > > + * \brief Retrieve the tag to prefix log messages with
> > > + * \return The tag to prefix log messages with
> > > + */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index 48f1fcb30ed4..d0e1d717b26c 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -29,7 +29,7 @@
> > >
> > >  namespace libcamera {
> > >
> > > -LOG_DEFINE_CATEGORY(V4L2Subdev)
> > > +LOG_DECLARE_CATEGORY(V4L2)
> > >
> > >  /**
> > >   * \struct V4L2SubdeviceFormat
> > > @@ -102,7 +102,7 @@ const std::string V4L2SubdeviceFormat::toString() const
> > >   * path
> > >   */
> > >  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)
> > > -	: entity_(entity), fd_(-1)
> > > +	: V4L2Device(entity->deviceNode(), entity->name()), entity_(entity)
> > >  {
> > >  }
> > >
> > > @@ -117,45 +117,7 @@ V4L2Subdevice::~V4L2Subdevice()
> > >   */
> > >  int V4L2Subdevice::open()
> > >  {
> > > -	int ret;
> > > -
> > > -	if (isOpen()) {
> > > -		LOG(V4L2Subdev, Error) << "Device already open";
> > > -		return -EBUSY;
> > > -	}
> > > -
> > > -	ret = ::open(entity_->deviceNode().c_str(), O_RDWR);
> > > -	if (ret < 0) {
> > > -		ret = -errno;
> > > -		LOG(V4L2Subdev, Error)
> > > -			<< "Failed to open V4L2 subdevice '"
> > > -			<< entity_->deviceNode() << "': " << strerror(-ret);
> > > -		return ret;
> > > -	}
> > > -	fd_ = ret;
> > > -
> > > -	return 0;
> > > -}
> > > -
> > > -/**
> > > - * \brief Check if the subdevice is open
> > > - * \return True if the subdevice is open, false otherwise
> > > - */
> > > -bool V4L2Subdevice::isOpen() const
> > > -{
> > > -	return fd_ != -1;
> > > -}
> > > -
> > > -/**
> > > - * \brief Close the subdevice, releasing any resources acquired by open()
> > > - */
> > > -void V4L2Subdevice::close()
> > > -{
> > > -	if (!isOpen())
> > > -		return;
> > > -
> > > -	::close(fd_);
> > > -	fd_ = -1;
> > > +	return V4L2Device::open(O_RDWR);
> > >  }
> > >
> > >  /**
> > > @@ -200,7 +162,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> > >  	ImageFormats formats;
> > >
> > >  	if (pad >= entity_->pads().size()) {
> > > -		LOG(V4L2Subdev, Error) << "Invalid pad: " << pad;
> > > +		LOG(V4L2, Error) << "Invalid pad: " << pad;
> > >  		return {};
> > >  	}
> > >
> > > @@ -210,7 +172,7 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> > >  			return {};
> > >
> > >  		if (formats.addFormat(code, sizes)) {
> > > -			LOG(V4L2Subdev, Error)
> > > +			LOG(V4L2, Error)
> > >  				<< "Could not add sizes for media bus code "
> > >  				<< code << " on pad " << pad;
> > >  			return {};
> > > @@ -232,10 +194,9 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> > >  	subdevFmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > >  	subdevFmt.pad = pad;
> > >
> > > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> > > +	int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> > >  	if (ret) {
> > > -		ret = -errno;
> > > -		LOG(V4L2Subdev, Error)
> > > +		LOG(V4L2, Error)
> > >  			<< "Unable to get format on pad " << pad
> > >  			<< ": " << strerror(-ret);
> > >  		return ret;
> > > @@ -268,10 +229,9 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> > >  	subdevFmt.format.height = format->size.height;
> > >  	subdevFmt.format.code = format->mbus_code;
> > >
> > > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > > +	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> > >  	if (ret) {
> > > -		ret = -errno;
> > > -		LOG(V4L2Subdev, Error)
> > > +		LOG(V4L2, Error)
> > >  			<< "Unable to set format on pad " << pad
> > >  			<< ": " << strerror(-ret);
> > >  		return ret;
> > > @@ -305,11 +265,6 @@ V4L2Subdevice *V4L2Subdevice::fromEntityName(const MediaDevice *media,
> > >  	return new V4L2Subdevice(mediaEntity);
> > >  }
> > >
> > > -std::string V4L2Subdevice::logPrefix() const
> > > -{
> > > -	return "'" + entity_->name() + "'";
> > > -}
> > > -
> > >  std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > >  {
> > >  	std::vector<unsigned int> codes;
> > > @@ -321,18 +276,17 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > >  		mbusEnum.index = index;
> > >  		mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > >
> > > -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > > +		ret = ioctl(VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > >  		if (ret)
> > >  			break;
> > >
> > >  		codes.push_back(mbusEnum.code);
> > >  	}
> > >
> > > -	if (ret && errno != EINVAL) {
> > > -		ret = errno;
> > > -		LOG(V4L2Subdev, Error)
> > > +	if (ret < 0 && ret != -EINVAL) {
> > > +		LOG(V4L2, Error)
> > >  			<< "Unable to enumerate formats on pad " << pad
> > > -			<< ": " << strerror(ret);
> > > +			<< ": " << strerror(-ret);
> > >  		return {};
> > >  	}
> > >
> > > @@ -352,7 +306,7 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > >  		sizeEnum.code = code;
> > >  		sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > >
> > > -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > > +		ret = ioctl(VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum);
> > >  		if (ret)
> > >  			break;
> > >
> > > @@ -360,9 +314,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> > >  				   sizeEnum.max_width, sizeEnum.max_height);
> > >  	}
> > >
> > > -	if (ret && (errno != EINVAL && errno != ENOTTY)) {
> > > -		ret = -errno;
> > > -		LOG(V4L2Subdev, Error)
> > > +	if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> > > +		LOG(V4L2, Error)
> > >  			<< "Unable to enumerate sizes on pad " << pad
> > >  			<< ": " << strerror(-ret);
> > >  		return {};
> > > @@ -386,10 +339,9 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > >  	sel.r.width = rect->w;
> > >  	sel.r.height = rect->h;
> > >
> > > -	int ret = ioctl(fd_, VIDIOC_SUBDEV_S_SELECTION, &sel);
> > > +	int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > > -		LOG(V4L2Subdev, Error)
> > > +		LOG(V4L2, Error)
> > >  			<< "Unable to set rectangle " << target << " on pad "
> > >  			<< pad << ": " << strerror(-ret);
> > >  		return ret;
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 0e70498c8bb1..403e0b2e0653 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -30,7 +30,7 @@
> > >   */
> > >  namespace libcamera {
> > >
> > > -LOG_DEFINE_CATEGORY(V4L2)
> > > +LOG_DECLARE_CATEGORY(V4L2)
> > >
> > >  /**
> > >   * \struct V4L2Capability
> > > @@ -270,7 +270,7 @@ const std::string V4L2DeviceFormat::toString() const
> > >   * \param[in] deviceNode The file-system path to the video device node
> > >   */
> > >  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > > -	: deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),
> > > +	: V4L2Device(deviceNode, deviceNode), bufferPool_(nullptr),
> > >  	  queuedBuffersCount_(0), fdEvent_(nullptr)
> > >  {
> > >  	/*
> > > @@ -305,23 +305,12 @@ int V4L2VideoDevice::open()
> > >  {
> > >  	int ret;
> > >
> > > -	if (isOpen()) {
> > > -		LOG(V4L2, Error) << "Device already open";
> > > -		return -EBUSY;
> > > -	}
> > > -
> > > -	ret = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);
> > > -	if (ret < 0) {
> > > -		ret = -errno;
> > > -		LOG(V4L2, Error)
> > > -			<< "Failed to open V4L2 device: " << strerror(-ret);
> > > +	ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> > > +	if (ret < 0)
> > >  		return ret;
> > > -	}
> > > -	fd_ = ret;
> > >
> > > -	ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
> > > +	ret = ioctl(VIDIOC_QUERYCAP, &caps_);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Failed to query device capabilities: "
> > >  			<< strerror(-ret);
> > > @@ -342,20 +331,20 @@ int V4L2VideoDevice::open()
> > >  	 * (POLLIN), and write notifications for OUTPUT devices (POLLOUT).
> > >  	 */
> > >  	if (caps_.isVideoCapture()) {
> > > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> > > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> > >  		bufferType_ = caps_.isMultiplanar()
> > >  			    ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> > >  			    : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > >  	} else if (caps_.isVideoOutput()) {
> > > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> > > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> > >  		bufferType_ = caps_.isMultiplanar()
> > >  			    ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> > >  			    : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> > >  	} else if (caps_.isMetaCapture()) {
> > > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> > > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> > >  		bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> > >  	} else if (caps_.isMetaOutput()) {
> > > -		fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> > > +		fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> > >  		bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> > >  	} else {
> > >  		LOG(V4L2, Error) << "Device is not a supported type";
> > > @@ -368,28 +357,18 @@ int V4L2VideoDevice::open()
> > >  	return 0;
> > >  }
> > >
> > > -/**
> > > - * \brief Check if device is successfully opened
> > > - * \return True if the device is open, false otherwise
> > > - */
> > > -bool V4L2VideoDevice::isOpen() const
> > > -{
> > > -	return fd_ != -1;
> > > -}
> > > -
> > >  /**
> > >   * \brief Close the device, releasing any resources acquired by open()
> > >   */
> > >  void V4L2VideoDevice::close()
> > >  {
> > > -	if (fd_ < 0)
> > > +	if (!isOpen())
> > >  		return;
> > >
> > >  	releaseBuffers();
> > >  	delete fdEvent_;
> > >
> > > -	::close(fd_);
> > > -	fd_ = -1;
> > > +	V4L2Device::close();
> > >  }
> > >
> > >  /**
> > > @@ -410,17 +389,6 @@ void V4L2VideoDevice::close()
> > >   * \return The string containing the device location
> > >   */
> > >
> > > -/**
> > > - * \fn V4L2VideoDevice::deviceNode()
> > > - * \brief Retrieve the video device node path
> > > - * \return The video device device node path
> > > - */
> > > -
> > > -std::string V4L2VideoDevice::logPrefix() const
> > > -{
> > > -	return deviceNode_ + (V4L2_TYPE_IS_OUTPUT(bufferType_) ? "[out]" : "[cap]");
> > > -}
> > > -
> > >  /**
> > >   * \brief Retrieve the image format set on the V4L2 device
> > >   * \param[out] format The image format applied on the device
> > > @@ -462,9 +430,8 @@ int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> > >  	int ret;
> > >
> > >  	v4l2Format.type = bufferType_;
> > > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> > >  	if (ret) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > > @@ -488,9 +455,8 @@ int V4L2VideoDevice::setFormatMeta(V4L2DeviceFormat *format)
> > >  	v4l2Format.type = bufferType_;
> > >  	pix->dataformat = format->fourcc;
> > >  	pix->buffersize = format->planes[0].size;
> > > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> > >  	if (ret) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > > @@ -516,9 +482,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
> > >  	int ret;
> > >
> > >  	v4l2Format.type = bufferType_;
> > > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> > >  	if (ret) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > > @@ -554,9 +519,8 @@ int V4L2VideoDevice::setFormatMultiplane(V4L2DeviceFormat *format)
> > >  		pix->plane_fmt[i].sizeimage = format->planes[i].size;
> > >  	}
> > >
> > > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> > >  	if (ret) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > > @@ -584,9 +548,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
> > >  	int ret;
> > >
> > >  	v4l2Format.type = bufferType_;
> > > -	ret = ioctl(fd_, VIDIOC_G_FMT, &v4l2Format);
> > > +	ret = ioctl(VIDIOC_G_FMT, &v4l2Format);
> > >  	if (ret) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error) << "Unable to get format: " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > > @@ -613,9 +576,8 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)
> > >  	pix->pixelformat = format->fourcc;
> > >  	pix->bytesperline = format->planes[0].bpl;
> > >  	pix->field = V4L2_FIELD_NONE;
> > > -	ret = ioctl(fd_, VIDIOC_S_FMT, &v4l2Format);
> > > +	ret = ioctl(VIDIOC_S_FMT, &v4l2Format);
> > >  	if (ret) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error) << "Unable to set format: " << strerror(-ret);
> > >  		return ret;
> > >  	}
> > > @@ -670,9 +632,8 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)
> > >  	rb.type = bufferType_;
> > >  	rb.memory = memoryType_;
> > >
> > > -	ret = ioctl(fd_, VIDIOC_REQBUFS, &rb);
> > > +	ret = ioctl(VIDIOC_REQBUFS, &rb);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Unable to request " << count << " buffers: "
> > >  			<< strerror(-ret);
> > > @@ -722,9 +683,8 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)
> > >  		buf.length = VIDEO_MAX_PLANES;
> > >  		buf.m.planes = planes;
> > >
> > > -		ret = ioctl(fd_, VIDIOC_QUERYBUF, &buf);
> > > +		ret = ioctl(VIDIOC_QUERYBUF, &buf);
> > >  		if (ret < 0) {
> > > -			ret = -errno;
> > >  			LOG(V4L2, Error)
> > >  				<< "Unable to query buffer " << i << ": "
> > >  				<< strerror(-ret);
> > > @@ -775,9 +735,8 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,
> > >  	expbuf.plane = planeIndex;
> > >  	expbuf.flags = O_RDWR;
> > >
> > > -	ret = ioctl(fd_, VIDIOC_EXPBUF, &expbuf);
> > > +	ret = ioctl(VIDIOC_EXPBUF, &expbuf);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Failed to export buffer: " << strerror(-ret);
> > >  		return ret;
> > > @@ -801,15 +760,14 @@ std::vector<unsigned int> V4L2VideoDevice::enumPixelformats()
> > >  		pixelformatEnum.index = index;
> > >  		pixelformatEnum.type = bufferType_;
> > >
> > > -		ret = ioctl(fd_, VIDIOC_ENUM_FMT, &pixelformatEnum);
> > > +		ret = ioctl(VIDIOC_ENUM_FMT, &pixelformatEnum);
> > >  		if (ret)
> > >  			break;
> > >
> > >  		formats.push_back(pixelformatEnum.pixelformat);
> > >  	}
> > >
> > > -	if (ret && errno != EINVAL) {
> > > -		ret = -errno;
> > > +	if (ret && ret != -EINVAL) {
> > >  		LOG(V4L2, Error)
> > >  			<< "Unable to enumerate pixel formats: "
> > >  			<< strerror(-ret);
> > > @@ -829,7 +787,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
> > >  		frameSize.index = index;
> > >  		frameSize.pixel_format = pixelFormat;
> > >
> > > -		ret = ioctl(fd_, VIDIOC_ENUM_FRAMESIZES, &frameSize);
> > > +		ret = ioctl(VIDIOC_ENUM_FRAMESIZES, &frameSize);
> > >  		if (ret)
> > >  			break;
> > >
> > > @@ -867,8 +825,7 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
> > >  		}
> > >  	}
> > >
> > > -	if (ret && errno != EINVAL) {
> > > -		ret = -errno;
> > > +	if (ret && ret != -EINVAL) {
> > >  		LOG(V4L2, Error)
> > >  			<< "Unable to enumerate frame sizes: "
> > >  			<< strerror(-ret);
> > > @@ -969,9 +926,8 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> > >
> > >  	LOG(V4L2, Debug) << "Queueing buffer " << buf.index;
> > >
> > > -	ret = ioctl(fd_, VIDIOC_QBUF, &buf);
> > > +	ret = ioctl(VIDIOC_QBUF, &buf);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Failed to queue buffer " << buf.index << ": "
> > >  			<< strerror(-ret);
> > > @@ -1006,9 +962,8 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
> > >  		buf.m.planes = planes;
> > >  	}
> > >
> > > -	ret = ioctl(fd_, VIDIOC_DQBUF, &buf);
> > > +	ret = ioctl(VIDIOC_DQBUF, &buf);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Failed to dequeue buffer: " << strerror(-ret);
> > >  		return nullptr;
> > > @@ -1066,9 +1021,8 @@ int V4L2VideoDevice::streamOn()
> > >  {
> > >  	int ret;
> > >
> > > -	ret = ioctl(fd_, VIDIOC_STREAMON, &bufferType_);
> > > +	ret = ioctl(VIDIOC_STREAMON, &bufferType_);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Failed to start streaming: " << strerror(-ret);
> > >  		return ret;
> > > @@ -1089,9 +1043,8 @@ int V4L2VideoDevice::streamOff()
> > >  {
> > >  	int ret;
> > >
> > > -	ret = ioctl(fd_, VIDIOC_STREAMOFF, &bufferType_);
> > > +	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> > >  	if (ret < 0) {
> > > -		ret = -errno;
> > >  		LOG(V4L2, Error)
> > >  			<< "Failed to stop streaming: " << strerror(-ret);
> > >  		return ret;