[{"id":1736,"web_url":"https://patchwork.libcamera.org/comment/1736/","msgid":"<995d4b50-cc86-eed3-6b83-7f28e4253430@ideasonboard.com>","date":"2019-06-03T09:22:29","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: Provide a V4L2Base\n\tclass","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nJust some overview comments so far ...\n\nOn 02/06/2019 14:04, Jacopo Mondi wrote:\n> Provide a base class for V4L2 Devices and Subdevices to group common\n> methods and fields.\n> \n> At the moment the code shared between V4L2Device and V4L2Subdevice is\n> quite limited, but with the forthcoming introduction of control it will\n> grow consistently.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/include/v4l2_base.h      | 31 +++++++++++++\n>  src/libcamera/include/v4l2_device.h    |  9 ++--\n>  src/libcamera/include/v4l2_subdevice.h |  9 ++--\n>  src/libcamera/meson.build              |  2 +\n\n>  src/libcamera/v4l2_base.cpp            | 64 ++++++++++++++++++++++++++\n>  src/libcamera/v4l2_device.cpp          | 13 ++----\n>  src/libcamera/v4l2_subdevice.cpp       | 12 +----\n\nPerhaps we should have a v4l2/ subdir in here now?\n\n\n>  7 files changed, 110 insertions(+), 30 deletions(-)\n>  create mode 100644 src/libcamera/include/v4l2_base.h\n>  create mode 100644 src/libcamera/v4l2_base.cpp\n> \n> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> new file mode 100644\n> index 000000000000..2fda81a960d2\n> --- /dev/null\n> +++ b/src/libcamera/include/v4l2_base.h\n> @@ -0,0 +1,31 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_base.h - Common base for V4L2 devices and subdevices\n> + */\n> +#ifndef __LIBCAMERA_V4L2_BASE__\n> +#define __LIBCAMERA_V4L2_BASE__\n\nI wonder if we could just call this 'v4l2'?\n\nEqually it might be better to keep the base too ... It should be clear\nthat we can't instantiate the base object of course.\n\n> +\n> +#include <vector>\n> +\n> +namespace libcamera {\n> +\n> +class V4L2Base\n> +{\n> +public:\n\nAre you avoiding a constructor here to initialise the fd_? (for\nsomething I haven't yet thought of?)\n\n> +\tvirtual ~V4L2Base()\n> +\t{\n> +\t}\n> +\n> +\tvirtual int open() = 0;\n> +\tvirtual void close() = 0;\n> +\tbool isOpen() const;\n> +\n> +protected:\n> +\tint fd_;\n> +};\n> +\n> +}; /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_V4L2_BASE__ */\n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index bdecc087fe5a..d9bcdb921b8c 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -17,6 +17,7 @@\n>  #include <libcamera/signal.h>\n>  \n>  #include \"log.h\"\n> +#include \"v4l2_base.h\"\n>  \n>  namespace libcamera {\n>  \n> @@ -111,7 +112,7 @@ public:\n>  \tconst std::string toString() const;\n>  };\n>  \n> -class V4L2Device : protected Loggable\n> +class V4L2Device : public V4L2Base, protected Loggable\n>  {\n>  public:\n>  \texplicit V4L2Device(const std::string &deviceNode);\n> @@ -121,9 +122,8 @@ public:\n>  \n>  \tV4L2Device &operator=(const V4L2Device &) = delete;\n>  \n> -\tint open();\n> -\tbool isOpen() const;\n> -\tvoid close();\n> +\tint open() override;\n> +\tvoid close() override;\n>  \n>  \tconst char *driverName() const { return caps_.driver(); }\n>  \tconst char *deviceName() const { return caps_.card(); }\n> @@ -167,7 +167,6 @@ private:\n>  \tvoid bufferAvailable(EventNotifier *notifier);\n>  \n>  \tstd::string deviceNode_;\n> -\tint fd_;\n>  \tV4L2Capability caps_;\n>  \n>  \tenum v4l2_buf_type bufferType_;\n> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> index 3e4e5107aebe..bdef3e69dd8c 100644\n> --- a/src/libcamera/include/v4l2_subdevice.h\n> +++ b/src/libcamera/include/v4l2_subdevice.h\n> @@ -16,6 +16,7 @@\n>  #include \"formats.h\"\n>  #include \"log.h\"\n>  #include \"media_object.h\"\n> +#include \"v4l2_base.h\"\n>  \n>  namespace libcamera {\n>  \n> @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {\n>  \tconst std::string toString() const;\n>  };\n>  \n> -class V4L2Subdevice : protected Loggable\n> +class V4L2Subdevice : public V4L2Base, protected Loggable\n>  {\n>  public:\n>  \texplicit V4L2Subdevice(const MediaEntity *entity);\n> @@ -36,9 +37,8 @@ public:\n>  \tV4L2Subdevice &operator=(const V4L2Subdevice &) = delete;\n>  \t~V4L2Subdevice();\n>  \n> -\tint open();\n> -\tbool isOpen() const;\n> -\tvoid close();\n> +\tint open() override;\n> +\tvoid close() override;\n>  \n>  \tconst MediaEntity *entity() const { return entity_; }\n>  \n> @@ -64,7 +64,6 @@ private:\n>  \t\t\t Rectangle *rect);\n>  \n>  \tconst MediaEntity *entity_;\n> -\tint fd_;\n>  };\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 6a73580d71f5..6d858a22531e 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -21,6 +21,7 @@ libcamera_sources = files([\n>      'stream.cpp',\n>      'timer.cpp',\n>      'utils.cpp',\n> +    'v4l2_base.cpp',\n>      'v4l2_device.cpp',\n>      'v4l2_subdevice.cpp',\n>  ])\n> @@ -38,6 +39,7 @@ libcamera_headers = files([\n>      'include/media_object.h',\n>      'include/pipeline_handler.h',\n>      'include/utils.h',\n> +    'include/v4l2_base.h',\n>      'include/v4l2_device.h',\n>      'include/v4l2_subdevice.h',\n>  ])\n> diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp\n> new file mode 100644\n> index 000000000000..7d05a3c82e4d\n> --- /dev/null\n> +++ b/src/libcamera/v4l2_base.cpp\n> @@ -0,0 +1,64 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * v4l2_base.cpp - Common base for V4L2 devices and subdevices\n> + */\n> +\n> +#include \"v4l2_base.h\"\n> +\n> +/**\n> + * \\file v4l2_base.h\n> + * \\brief Common base for V4L2 devices and subdevices\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class V4L2Base\n> + * \\brief Base class for V4L2Device and V4L2Subdevice\n> + *\n> + * The V4L2Base class groups together the methods and fields common to\n> + * both V4L2Device and V4L2Subdevice, and provide a base abstract class which\n> + * defines a streamlined interface that both the derived class have to implement.\n> + *\n> + * The interface defined by V4L2Base only requires derived classes to implement\n> + * methods that modifies the status of the file descriptor associated with\n> + * the video device or subdevice, such as \\a open() and \\a close().\n> + *\n> + * Methods common to V4L2Device and V4L2Subdevice, such as V4L2 controls\n> + * support are implemented in the base class to maximize code re-use.\n> + */\n> +\n> +/**\n> + * \\brief Open a V4L2 device or subdevice\n> + *\n> + * Pure virtual method to be implemented by the derived classes.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\brief Close the device or subdevice\n> + *\n> + * Pure virtual method to be implemented by the derived classes.\n> + */\n> +\n> +/**\n> + * \\brief Check if the V4L2 device or subdevice is open\n> + * \\return True if the V4L2 device or subdevice is open, false otherwise\n> + */\n> +bool V4L2Base::isOpen() const\n> +{\n> +\treturn fd_ != -1;\n> +}\n> +\n> +/**\n> + * \\var V4L2Base::fd_\n> + * \\brief The V4L2 device or subdevice device node file descriptor\n> + *\n> + * The file descriptor is initialized to -1 and reset to this value once\n> + * the device or subdevice gets closed.\n> + */\n> +\n> +}; /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 0821bd75fb42..64533e88a512 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -270,9 +270,11 @@ const std::string V4L2DeviceFormat::toString() const\n>   * \\param[in] deviceNode The file-system path to the video device node\n>   */\n>  V4L2Device::V4L2Device(const std::string &deviceNode)\n> -\t: deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),\n> +\t: deviceNode_(deviceNode), bufferPool_(nullptr),\n>  \t  queuedBuffersCount_(0), fdEvent_(nullptr)\n>  {\n> +\tfd_ = -1;\n\nShouldn't this be initialised in the base class?\n\n> +\n>  \t/*\n>  \t * We default to an MMAP based CAPTURE device, however this will be\n>  \t * updated based upon the device capabilities.\n> @@ -368,15 +370,6 @@ int V4L2Device::open()\n>  \treturn 0;\n>  }\n>  \n> -/**\n> - * \\brief Check if device is successfully opened\n> - * \\return True if the device is open, false otherwise\n> - */\n> -bool V4L2Device::isOpen() const\n> -{\n> -\treturn fd_ != -1;\n> -}\n> -\n>  /**\n>   * \\brief Close the device, releasing any resources acquired by open()\n>   */\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index fceee33156e9..3a053fadb3f6 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -102,8 +102,9 @@ const std::string V4L2SubdeviceFormat::toString() const\n>   * path\n>   */\n>  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)\n> -\t: entity_(entity), fd_(-1)\n> +\t: entity_(entity)\n>  {\n> +\tfd_ = -1;\n>  }\n>  \n>  V4L2Subdevice::~V4L2Subdevice()\n> @@ -137,15 +138,6 @@ int V4L2Subdevice::open()\n>  \treturn 0;\n>  }\n>  \n> -/**\n> - * \\brief Check if the subdevice is open\n> - * \\return True if the subdevice is open, false otherwise\n> - */\n> -bool V4L2Subdevice::isOpen() const\n> -{\n> -\treturn fd_ != -1;\n> -}\n> -\n>  /**\n>   * \\brief Close the subdevice, releasing any resources acquired by open()\n>   */\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A62160BD8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  3 Jun 2019 11:22:32 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9DAA22D2;\n\tMon,  3 Jun 2019 11:22:31 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559553751;\n\tbh=I4k8DeS7YGeKP4wBBMXpdaky6ofbcZx802VALuo+G2s=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=Z5kw+yK8OcNvg+NdZd8PZTESssJBnzq1Wv1lJEs9CmEDZims/yg/lQu5L5+UDEil5\n\t3cG5DHf9JfSKC/8xqL6rU/89C2kCZfcAgFE4Ziad1MUtbkbKYyTg2Xsrd2jN1cIEZD\n\ttUnmDLQK4NXjMtoC5I+JWj/p/WAlaGOeGfs4/aAk=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>, libcamera-devel@lists.libcamera.org","References":"<20190602130435.18780-1-jacopo@jmondi.org>\n\t<20190602130435.18780-3-jacopo@jmondi.org>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<995d4b50-cc86-eed3-6b83-7f28e4253430@ideasonboard.com>","Date":"Mon, 3 Jun 2019 10:22:29 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190602130435.18780-3-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: Provide a V4L2Base\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 03 Jun 2019 09:22:32 -0000"}},{"id":1790,"web_url":"https://patchwork.libcamera.org/comment/1790/","msgid":"<20190608154805.GF4786@pendragon.ideasonboard.com>","date":"2019-06-08T15:48:05","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: Provide a V4L2Base\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Jun 03, 2019 at 10:22:29AM +0100, Kieran Bingham wrote:\n> Hi Jacopo,\n> \n> Just some overview comments so far ...\n> \n> On 02/06/2019 14:04, Jacopo Mondi wrote:\n> > Provide a base class for V4L2 Devices and Subdevices to group common\n> > methods and fields.\n> > \n> > At the moment the code shared between V4L2Device and V4L2Subdevice is\n> > quite limited, but with the forthcoming introduction of control it will\n> > grow consistently.\n> > \n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/libcamera/include/v4l2_base.h      | 31 +++++++++++++\n> >  src/libcamera/include/v4l2_device.h    |  9 ++--\n> >  src/libcamera/include/v4l2_subdevice.h |  9 ++--\n> >  src/libcamera/meson.build              |  2 +\n> >  src/libcamera/v4l2_base.cpp            | 64 ++++++++++++++++++++++++++\n> >  src/libcamera/v4l2_device.cpp          | 13 ++----\n> >  src/libcamera/v4l2_subdevice.cpp       | 12 +----\n> \n> Perhaps we should have a v4l2/ subdir in here now?\n> \n> >  7 files changed, 110 insertions(+), 30 deletions(-)\n> >  create mode 100644 src/libcamera/include/v4l2_base.h\n> >  create mode 100644 src/libcamera/v4l2_base.cpp\n> > \n> > diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> > new file mode 100644\n> > index 000000000000..2fda81a960d2\n> > --- /dev/null\n> > +++ b/src/libcamera/include/v4l2_base.h\n> > @@ -0,0 +1,31 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * v4l2_base.h - Common base for V4L2 devices and subdevices\n> > + */\n> > +#ifndef __LIBCAMERA_V4L2_BASE__\n> > +#define __LIBCAMERA_V4L2_BASE__\n> \n> I wonder if we could just call this 'v4l2'?\n> \n> Equally it might be better to keep the base too ... It should be clear\n> that we can't instantiate the base object of course.\n\nWe could also rename V4L2Device to V4L2VideoDevice and name the base\nclass V4L2Device (or V4L2DeviceBase). I don't mind either way.\n\n> > +\n> > +#include <vector>\n> > +\n> > +namespace libcamera {\n> > +\n> > +class V4L2Base\n> > +{\n> > +public:\n> \n> Are you avoiding a constructor here to initialise the fd_? (for\n> something I haven't yet thought of?)\n> \n> > +\tvirtual ~V4L2Base()\n> > +\t{\n> > +\t}\n> > +\n> > +\tvirtual int open() = 0;\n> > +\tvirtual void close() = 0;\n\nThere's no need to make the open() and close() functions virtual, or\neven to declare them in the base class, as you never call them on a\nV4L2Base instance. All you need is an fd_ in the base. It could also\npossibly make sense to add an ioctl() function in the base class.\n\nHowever, I'm not too fond of storing fd_ in the base class and\nimplementing open() and close() in derived classes. One option would be\nto implement protected open() and close() functions in the base class\nthat would just open and close the fd, and call them from the open() and\nclose() function of the derived classes. You wouldn't need to make the\nbase methods virtual in this case, and I think I would make fd_ private\nand provide a protected fd() accessor.\n\n> > +\tbool isOpen() const;\n> > +\n> > +protected:\n> > +\tint fd_;\n> > +};\n> > +\n> > +}; /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_V4L2_BASE__ */\n> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > index bdecc087fe5a..d9bcdb921b8c 100644\n> > --- a/src/libcamera/include/v4l2_device.h\n> > +++ b/src/libcamera/include/v4l2_device.h\n> > @@ -17,6 +17,7 @@\n> >  #include <libcamera/signal.h>\n> >  \n> >  #include \"log.h\"\n> > +#include \"v4l2_base.h\"\n> >  \n> >  namespace libcamera {\n> >  \n> > @@ -111,7 +112,7 @@ public:\n> >  \tconst std::string toString() const;\n> >  };\n> >  \n> > -class V4L2Device : protected Loggable\n> > +class V4L2Device : public V4L2Base, protected Loggable\n> >  {\n> >  public:\n> >  \texplicit V4L2Device(const std::string &deviceNode);\n> > @@ -121,9 +122,8 @@ public:\n> >  \n> >  \tV4L2Device &operator=(const V4L2Device &) = delete;\n> >  \n> > -\tint open();\n> > -\tbool isOpen() const;\n> > -\tvoid close();\n> > +\tint open() override;\n> > +\tvoid close() override;\n> >  \n> >  \tconst char *driverName() const { return caps_.driver(); }\n> >  \tconst char *deviceName() const { return caps_.card(); }\n> > @@ -167,7 +167,6 @@ private:\n> >  \tvoid bufferAvailable(EventNotifier *notifier);\n> >  \n> >  \tstd::string deviceNode_;\n> > -\tint fd_;\n> >  \tV4L2Capability caps_;\n> >  \n> >  \tenum v4l2_buf_type bufferType_;\n> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > index 3e4e5107aebe..bdef3e69dd8c 100644\n> > --- a/src/libcamera/include/v4l2_subdevice.h\n> > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > @@ -16,6 +16,7 @@\n> >  #include \"formats.h\"\n> >  #include \"log.h\"\n> >  #include \"media_object.h\"\n> > +#include \"v4l2_base.h\"\n> >  \n> >  namespace libcamera {\n> >  \n> > @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {\n> >  \tconst std::string toString() const;\n> >  };\n> >  \n> > -class V4L2Subdevice : protected Loggable\n> > +class V4L2Subdevice : public V4L2Base, protected Loggable\n> >  {\n> >  public:\n> >  \texplicit V4L2Subdevice(const MediaEntity *entity);\n> > @@ -36,9 +37,8 @@ public:\n> >  \tV4L2Subdevice &operator=(const V4L2Subdevice &) = delete;\n> >  \t~V4L2Subdevice();\n> >  \n> > -\tint open();\n> > -\tbool isOpen() const;\n> > -\tvoid close();\n> > +\tint open() override;\n> > +\tvoid close() override;\n> >  \n> >  \tconst MediaEntity *entity() const { return entity_; }\n> >  \n> > @@ -64,7 +64,6 @@ private:\n> >  \t\t\t Rectangle *rect);\n> >  \n> >  \tconst MediaEntity *entity_;\n> > -\tint fd_;\n> >  };\n> >  \n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 6a73580d71f5..6d858a22531e 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -21,6 +21,7 @@ libcamera_sources = files([\n> >      'stream.cpp',\n> >      'timer.cpp',\n> >      'utils.cpp',\n> > +    'v4l2_base.cpp',\n> >      'v4l2_device.cpp',\n> >      'v4l2_subdevice.cpp',\n> >  ])\n> > @@ -38,6 +39,7 @@ libcamera_headers = files([\n> >      'include/media_object.h',\n> >      'include/pipeline_handler.h',\n> >      'include/utils.h',\n> > +    'include/v4l2_base.h',\n> >      'include/v4l2_device.h',\n> >      'include/v4l2_subdevice.h',\n> >  ])\n> > diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp\n> > new file mode 100644\n> > index 000000000000..7d05a3c82e4d\n> > --- /dev/null\n> > +++ b/src/libcamera/v4l2_base.cpp\n> > @@ -0,0 +1,64 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * v4l2_base.cpp - Common base for V4L2 devices and subdevices\n> > + */\n> > +\n> > +#include \"v4l2_base.h\"\n> > +\n> > +/**\n> > + * \\file v4l2_base.h\n> > + * \\brief Common base for V4L2 devices and subdevices\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class V4L2Base\n> > + * \\brief Base class for V4L2Device and V4L2Subdevice\n> > + *\n> > + * The V4L2Base class groups together the methods and fields common to\n> > + * both V4L2Device and V4L2Subdevice, and provide a base abstract class which\n> > + * defines a streamlined interface that both the derived class have to implement.\n> > + *\n> > + * The interface defined by V4L2Base only requires derived classes to implement\n> > + * methods that modifies the status of the file descriptor associated with\n> > + * the video device or subdevice, such as \\a open() and \\a close().\n> > + *\n> > + * Methods common to V4L2Device and V4L2Subdevice, such as V4L2 controls\n> > + * support are implemented in the base class to maximize code re-use.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Open a V4L2 device or subdevice\n> > + *\n> > + * Pure virtual method to be implemented by the derived classes.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\brief Close the device or subdevice\n> > + *\n> > + * Pure virtual method to be implemented by the derived classes.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Check if the V4L2 device or subdevice is open\n> > + * \\return True if the V4L2 device or subdevice is open, false otherwise\n> > + */\n> > +bool V4L2Base::isOpen() const\n> > +{\n> > +\treturn fd_ != -1;\n> > +}\n> > +\n> > +/**\n> > + * \\var V4L2Base::fd_\n> > + * \\brief The V4L2 device or subdevice device node file descriptor\n> > + *\n> > + * The file descriptor is initialized to -1 and reset to this value once\n> > + * the device or subdevice gets closed.\n> > + */\n> > +\n> > +}; /* namespace libcamera */\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 0821bd75fb42..64533e88a512 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -270,9 +270,11 @@ const std::string V4L2DeviceFormat::toString() const\n> >   * \\param[in] deviceNode The file-system path to the video device node\n> >   */\n> >  V4L2Device::V4L2Device(const std::string &deviceNode)\n> > -\t: deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),\n> > +\t: deviceNode_(deviceNode), bufferPool_(nullptr),\n> >  \t  queuedBuffersCount_(0), fdEvent_(nullptr)\n> >  {\n> > +\tfd_ = -1;\n> \n> Shouldn't this be initialised in the base class?\n> \n> > +\n> >  \t/*\n> >  \t * We default to an MMAP based CAPTURE device, however this will be\n> >  \t * updated based upon the device capabilities.\n> > @@ -368,15 +370,6 @@ int V4L2Device::open()\n> >  \treturn 0;\n> >  }\n> >  \n> > -/**\n> > - * \\brief Check if device is successfully opened\n> > - * \\return True if the device is open, false otherwise\n> > - */\n> > -bool V4L2Device::isOpen() const\n> > -{\n> > -\treturn fd_ != -1;\n> > -}\n> > -\n> >  /**\n> >   * \\brief Close the device, releasing any resources acquired by open()\n> >   */\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index fceee33156e9..3a053fadb3f6 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -102,8 +102,9 @@ const std::string V4L2SubdeviceFormat::toString() const\n> >   * path\n> >   */\n> >  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)\n> > -\t: entity_(entity), fd_(-1)\n> > +\t: entity_(entity)\n> >  {\n> > +\tfd_ = -1;\n> >  }\n> >  \n> >  V4L2Subdevice::~V4L2Subdevice()\n> > @@ -137,15 +138,6 @@ int V4L2Subdevice::open()\n> >  \treturn 0;\n> >  }\n> >  \n> > -/**\n> > - * \\brief Check if the subdevice is open\n> > - * \\return True if the subdevice is open, false otherwise\n> > - */\n> > -bool V4L2Subdevice::isOpen() const\n> > -{\n> > -\treturn fd_ != -1;\n> > -}\n> > -\n> >  /**\n> >   * \\brief Close the subdevice, releasing any resources acquired by open()\n> >   */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBF956969A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat,  8 Jun 2019 17:48:19 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [109.132.30.162])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 65C455D;\n\tSat,  8 Jun 2019 17:48:19 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560008899;\n\tbh=CMoixvhIzibIujNOfg6gPbVLx/H1po9tDvXx3gMDW8o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IhcNrQU3Lm0c3YeB7cDv3KGU2SpxKFqhmGi75WtfYVbRIM61gtXD93Dt4DbrSAEFE\n\tnHGF61RrLYSdTgN6y/8Wyxh0FUk34TsXKSxhlzahLjACrkhZ1/OcgIbvmwv1VygwGP\n\tOixUNTrIi1eOQEd6y1Q8JwoI9hYW0hyn1kFPdWUg=","Date":"Sat, 8 Jun 2019 18:48:05 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo@jmondi.org>, libcamera-devel@lists.libcamera.org","Message-ID":"<20190608154805.GF4786@pendragon.ideasonboard.com>","References":"<20190602130435.18780-1-jacopo@jmondi.org>\n\t<20190602130435.18780-3-jacopo@jmondi.org>\n\t<995d4b50-cc86-eed3-6b83-7f28e4253430@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<995d4b50-cc86-eed3-6b83-7f28e4253430@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: Provide a V4L2Base\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 08 Jun 2019 15:48:20 -0000"}},{"id":1813,"web_url":"https://patchwork.libcamera.org/comment/1813/","msgid":"<20190610070605.lnldvqpxeum54eyt@uno.localdomain>","date":"2019-06-10T07:06:05","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: Provide a V4L2Base\n\tclass","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent/Kieran,\n\nOn Sat, Jun 08, 2019 at 06:48:05PM +0300, Laurent Pinchart wrote:\n> Hello,\n>\n> On Mon, Jun 03, 2019 at 10:22:29AM +0100, Kieran Bingham wrote:\n> > Hi Jacopo,\n> >\n> > Just some overview comments so far ...\n> >\n> > On 02/06/2019 14:04, Jacopo Mondi wrote:\n> > > Provide a base class for V4L2 Devices and Subdevices to group common\n> > > methods and fields.\n> > >\n> > > At the moment the code shared between V4L2Device and V4L2Subdevice is\n> > > quite limited, but with the forthcoming introduction of control it will\n> > > grow consistently.\n> > >\n> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > >  src/libcamera/include/v4l2_base.h      | 31 +++++++++++++\n> > >  src/libcamera/include/v4l2_device.h    |  9 ++--\n> > >  src/libcamera/include/v4l2_subdevice.h |  9 ++--\n> > >  src/libcamera/meson.build              |  2 +\n> > >  src/libcamera/v4l2_base.cpp            | 64 ++++++++++++++++++++++++++\n> > >  src/libcamera/v4l2_device.cpp          | 13 ++----\n> > >  src/libcamera/v4l2_subdevice.cpp       | 12 +----\n> >\n> > Perhaps we should have a v4l2/ subdir in here now?\n> >\n\nI like the idea. Anyone is against this?\n\n> > >  7 files changed, 110 insertions(+), 30 deletions(-)\n> > >  create mode 100644 src/libcamera/include/v4l2_base.h\n> > >  create mode 100644 src/libcamera/v4l2_base.cpp\n> > >\n> > > diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> > > new file mode 100644\n> > > index 000000000000..2fda81a960d2\n> > > --- /dev/null\n> > > +++ b/src/libcamera/include/v4l2_base.h\n> > > @@ -0,0 +1,31 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * v4l2_base.h - Common base for V4L2 devices and subdevices\n> > > + */\n> > > +#ifndef __LIBCAMERA_V4L2_BASE__\n> > > +#define __LIBCAMERA_V4L2_BASE__\n> >\n> > I wonder if we could just call this 'v4l2'?\n> >\n> > Equally it might be better to keep the base too ... It should be clear\n> > that we can't instantiate the base object of course.\n>\n> We could also rename V4L2Device to V4L2VideoDevice and name the base\n> class V4L2Device (or V4L2DeviceBase). I don't mind either way.\n>\n\nWhat if we have V4L2Device as base class and V4l2Videodev and\nV4L2Subdev as derived ones ?\n\n> > > +\n> > > +#include <vector>\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class V4L2Base\n> > > +{\n> > > +public:\n> >\n> > Are you avoiding a constructor here to initialise the fd_? (for\n> > something I haven't yet thought of?)\n> >\n\nIt's (wrongly?) implemented in the derived classes.\n\n> > > +\tvirtual ~V4L2Base()\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\tvirtual int open() = 0;\n> > > +\tvirtual void close() = 0;\n>\n> There's no need to make the open() and close() functions virtual, or\n> even to declare them in the base class, as you never call them on a\n> V4L2Base instance. All you need is an fd_ in the base. It could also\n> possibly make sense to add an ioctl() function in the base class.\n>\n\n+1 for ioctl wrapper. It would save some code.\n\n> However, I'm not too fond of storing fd_ in the base class and\n> implementing open() and close() in derived classes. One option would be\n> to implement protected open() and close() functions in the base class\n> that would just open and close the fd, and call them from the open() and\n> close() function of the derived classes. You wouldn't need to make the\n> base methods virtual in this case, and I think I would make fd_ private\n> and provide a protected fd() accessor.\n\nIsn't this over-engineered? I mean, the derived classes would have to\ncall the base class methods that just wrap open/close without doing\nmuch more, and have to call a method to access the fd_, without any\nreal benefit in my opinion. I would rather move fd_ to the derived\nclasses and have open()/close() implemented there independently.\n\nUnless I'm missing some obvious reason not to do so...\n\n>\n> > > +\tbool isOpen() const;\n> > > +\n> > > +protected:\n> > > +\tint fd_;\n> > > +};\n> > > +\n> > > +}; /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_V4L2_BASE__ */\n> > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > > index bdecc087fe5a..d9bcdb921b8c 100644\n> > > --- a/src/libcamera/include/v4l2_device.h\n> > > +++ b/src/libcamera/include/v4l2_device.h\n> > > @@ -17,6 +17,7 @@\n> > >  #include <libcamera/signal.h>\n> > >\n> > >  #include \"log.h\"\n> > > +#include \"v4l2_base.h\"\n> > >\n> > >  namespace libcamera {\n> > >\n> > > @@ -111,7 +112,7 @@ public:\n> > >  \tconst std::string toString() const;\n> > >  };\n> > >\n> > > -class V4L2Device : protected Loggable\n> > > +class V4L2Device : public V4L2Base, protected Loggable\n> > >  {\n> > >  public:\n> > >  \texplicit V4L2Device(const std::string &deviceNode);\n> > > @@ -121,9 +122,8 @@ public:\n> > >\n> > >  \tV4L2Device &operator=(const V4L2Device &) = delete;\n> > >\n> > > -\tint open();\n> > > -\tbool isOpen() const;\n> > > -\tvoid close();\n> > > +\tint open() override;\n> > > +\tvoid close() override;\n> > >\n> > >  \tconst char *driverName() const { return caps_.driver(); }\n> > >  \tconst char *deviceName() const { return caps_.card(); }\n> > > @@ -167,7 +167,6 @@ private:\n> > >  \tvoid bufferAvailable(EventNotifier *notifier);\n> > >\n> > >  \tstd::string deviceNode_;\n> > > -\tint fd_;\n> > >  \tV4L2Capability caps_;\n> > >\n> > >  \tenum v4l2_buf_type bufferType_;\n> > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> > > index 3e4e5107aebe..bdef3e69dd8c 100644\n> > > --- a/src/libcamera/include/v4l2_subdevice.h\n> > > +++ b/src/libcamera/include/v4l2_subdevice.h\n> > > @@ -16,6 +16,7 @@\n> > >  #include \"formats.h\"\n> > >  #include \"log.h\"\n> > >  #include \"media_object.h\"\n> > > +#include \"v4l2_base.h\"\n> > >\n> > >  namespace libcamera {\n> > >\n> > > @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {\n> > >  \tconst std::string toString() const;\n> > >  };\n> > >\n> > > -class V4L2Subdevice : protected Loggable\n> > > +class V4L2Subdevice : public V4L2Base, protected Loggable\n> > >  {\n> > >  public:\n> > >  \texplicit V4L2Subdevice(const MediaEntity *entity);\n> > > @@ -36,9 +37,8 @@ public:\n> > >  \tV4L2Subdevice &operator=(const V4L2Subdevice &) = delete;\n> > >  \t~V4L2Subdevice();\n> > >\n> > > -\tint open();\n> > > -\tbool isOpen() const;\n> > > -\tvoid close();\n> > > +\tint open() override;\n> > > +\tvoid close() override;\n> > >\n> > >  \tconst MediaEntity *entity() const { return entity_; }\n> > >\n> > > @@ -64,7 +64,6 @@ private:\n> > >  \t\t\t Rectangle *rect);\n> > >\n> > >  \tconst MediaEntity *entity_;\n> > > -\tint fd_;\n> > >  };\n> > >\n> > >  } /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 6a73580d71f5..6d858a22531e 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -21,6 +21,7 @@ libcamera_sources = files([\n> > >      'stream.cpp',\n> > >      'timer.cpp',\n> > >      'utils.cpp',\n> > > +    'v4l2_base.cpp',\n> > >      'v4l2_device.cpp',\n> > >      'v4l2_subdevice.cpp',\n> > >  ])\n> > > @@ -38,6 +39,7 @@ libcamera_headers = files([\n> > >      'include/media_object.h',\n> > >      'include/pipeline_handler.h',\n> > >      'include/utils.h',\n> > > +    'include/v4l2_base.h',\n> > >      'include/v4l2_device.h',\n> > >      'include/v4l2_subdevice.h',\n> > >  ])\n> > > diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp\n> > > new file mode 100644\n> > > index 000000000000..7d05a3c82e4d\n> > > --- /dev/null\n> > > +++ b/src/libcamera/v4l2_base.cpp\n> > > @@ -0,0 +1,64 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Google Inc.\n> > > + *\n> > > + * v4l2_base.cpp - Common base for V4L2 devices and subdevices\n> > > + */\n> > > +\n> > > +#include \"v4l2_base.h\"\n> > > +\n> > > +/**\n> > > + * \\file v4l2_base.h\n> > > + * \\brief Common base for V4L2 devices and subdevices\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\class V4L2Base\n> > > + * \\brief Base class for V4L2Device and V4L2Subdevice\n> > > + *\n> > > + * The V4L2Base class groups together the methods and fields common to\n> > > + * both V4L2Device and V4L2Subdevice, and provide a base abstract class which\n> > > + * defines a streamlined interface that both the derived class have to implement.\n> > > + *\n> > > + * The interface defined by V4L2Base only requires derived classes to implement\n> > > + * methods that modifies the status of the file descriptor associated with\n> > > + * the video device or subdevice, such as \\a open() and \\a close().\n> > > + *\n> > > + * Methods common to V4L2Device and V4L2Subdevice, such as V4L2 controls\n> > > + * support are implemented in the base class to maximize code re-use.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Open a V4L2 device or subdevice\n> > > + *\n> > > + * Pure virtual method to be implemented by the derived classes.\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Close the device or subdevice\n> > > + *\n> > > + * Pure virtual method to be implemented by the derived classes.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Check if the V4L2 device or subdevice is open\n> > > + * \\return True if the V4L2 device or subdevice is open, false otherwise\n> > > + */\n> > > +bool V4L2Base::isOpen() const\n> > > +{\n> > > +\treturn fd_ != -1;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\var V4L2Base::fd_\n> > > + * \\brief The V4L2 device or subdevice device node file descriptor\n> > > + *\n> > > + * The file descriptor is initialized to -1 and reset to this value once\n> > > + * the device or subdevice gets closed.\n> > > + */\n> > > +\n> > > +}; /* namespace libcamera */\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index 0821bd75fb42..64533e88a512 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -270,9 +270,11 @@ const std::string V4L2DeviceFormat::toString() const\n> > >   * \\param[in] deviceNode The file-system path to the video device node\n> > >   */\n> > >  V4L2Device::V4L2Device(const std::string &deviceNode)\n> > > -\t: deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),\n> > > +\t: deviceNode_(deviceNode), bufferPool_(nullptr),\n> > >  \t  queuedBuffersCount_(0), fdEvent_(nullptr)\n> > >  {\n> > > +\tfd_ = -1;\n> >\n> > Shouldn't this be initialised in the base class?\n> >\n> > > +\n> > >  \t/*\n> > >  \t * We default to an MMAP based CAPTURE device, however this will be\n> > >  \t * updated based upon the device capabilities.\n> > > @@ -368,15 +370,6 @@ int V4L2Device::open()\n> > >  \treturn 0;\n> > >  }\n> > >\n> > > -/**\n> > > - * \\brief Check if device is successfully opened\n> > > - * \\return True if the device is open, false otherwise\n> > > - */\n> > > -bool V4L2Device::isOpen() const\n> > > -{\n> > > -\treturn fd_ != -1;\n> > > -}\n> > > -\n> > >  /**\n> > >   * \\brief Close the device, releasing any resources acquired by open()\n> > >   */\n> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > index fceee33156e9..3a053fadb3f6 100644\n> > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > @@ -102,8 +102,9 @@ const std::string V4L2SubdeviceFormat::toString() const\n> > >   * path\n> > >   */\n> > >  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)\n> > > -\t: entity_(entity), fd_(-1)\n> > > +\t: entity_(entity)\n> > >  {\n> > > +\tfd_ = -1;\n> > >  }\n> > >\n> > >  V4L2Subdevice::~V4L2Subdevice()\n> > > @@ -137,15 +138,6 @@ int V4L2Subdevice::open()\n> > >  \treturn 0;\n> > >  }\n> > >\n> > > -/**\n> > > - * \\brief Check if the subdevice is open\n> > > - * \\return True if the subdevice is open, false otherwise\n> > > - */\n> > > -bool V4L2Subdevice::isOpen() const\n> > > -{\n> > > -\treturn fd_ != -1;\n> > > -}\n> > > -\n> > >  /**\n> > >   * \\brief Close the subdevice, releasing any resources acquired by open()\n> > >   */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C01706362E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2019 09:04:52 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 227906000A;\n\tMon, 10 Jun 2019 07:04:51 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 10 Jun 2019 09:06:05 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190610070605.lnldvqpxeum54eyt@uno.localdomain>","References":"<20190602130435.18780-1-jacopo@jmondi.org>\n\t<20190602130435.18780-3-jacopo@jmondi.org>\n\t<995d4b50-cc86-eed3-6b83-7f28e4253430@ideasonboard.com>\n\t<20190608154805.GF4786@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"gszfyzsrc23wa4fr\"","Content-Disposition":"inline","In-Reply-To":"<20190608154805.GF4786@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: Provide a V4L2Base\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 10 Jun 2019 07:04:53 -0000"}},{"id":1820,"web_url":"https://patchwork.libcamera.org/comment/1820/","msgid":"<20190610082252.GJ4806@pendragon.ideasonboard.com>","date":"2019-06-10T08:22:52","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: Provide a V4L2Base\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jun 10, 2019 at 09:06:05AM +0200, Jacopo Mondi wrote:\n> On Sat, Jun 08, 2019 at 06:48:05PM +0300, Laurent Pinchart wrote:\n> > On Mon, Jun 03, 2019 at 10:22:29AM +0100, Kieran Bingham wrote:\n> >> On 02/06/2019 14:04, Jacopo Mondi wrote:\n> >>> Provide a base class for V4L2 Devices and Subdevices to group common\n> >>> methods and fields.\n> >>>\n> >>> At the moment the code shared between V4L2Device and V4L2Subdevice is\n> >>> quite limited, but with the forthcoming introduction of control it will\n> >>> grow consistently.\n> >>>\n> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> >>> ---\n> >>>  src/libcamera/include/v4l2_base.h      | 31 +++++++++++++\n> >>>  src/libcamera/include/v4l2_device.h    |  9 ++--\n> >>>  src/libcamera/include/v4l2_subdevice.h |  9 ++--\n> >>>  src/libcamera/meson.build              |  2 +\n> >>>  src/libcamera/v4l2_base.cpp            | 64 ++++++++++++++++++++++++++\n> >>>  src/libcamera/v4l2_device.cpp          | 13 ++----\n> >>>  src/libcamera/v4l2_subdevice.cpp       | 12 +----\n> >>\n> >> Perhaps we should have a v4l2/ subdir in here now?\n> \n> I like the idea. Anyone is against this?\n\nI don't think it's necessary yet, but I'm not against it either. The\nfiles will need to start with the v4l2_ prefix anyway.\n\n> >>>  7 files changed, 110 insertions(+), 30 deletions(-)\n> >>>  create mode 100644 src/libcamera/include/v4l2_base.h\n> >>>  create mode 100644 src/libcamera/v4l2_base.cpp\n> >>>\n> >>> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> >>> new file mode 100644\n> >>> index 000000000000..2fda81a960d2\n> >>> --- /dev/null\n> >>> +++ b/src/libcamera/include/v4l2_base.h\n> >>> @@ -0,0 +1,31 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2019, Google Inc.\n> >>> + *\n> >>> + * v4l2_base.h - Common base for V4L2 devices and subdevices\n> >>> + */\n> >>> +#ifndef __LIBCAMERA_V4L2_BASE__\n> >>> +#define __LIBCAMERA_V4L2_BASE__\n> >>\n> >> I wonder if we could just call this 'v4l2'?\n> >>\n> >> Equally it might be better to keep the base too ... It should be clear\n> >> that we can't instantiate the base object of course.\n> >\n> > We could also rename V4L2Device to V4L2VideoDevice and name the base\n> > class V4L2Device (or V4L2DeviceBase). I don't mind either way.\n> \n> What if we have V4L2Device as base class and V4l2Videodev and\n> V4L2Subdev as derived ones ?\n\nThat works for me.\n\n> >>> +\n> >>> +#include <vector>\n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +class V4L2Base\n> >>> +{\n> >>> +public:\n> >>\n> >> Are you avoiding a constructor here to initialise the fd_? (for\n> >> something I haven't yet thought of?)\n> \n> It's (wrongly?) implemented in the derived classes.\n> \n> >>> +\tvirtual ~V4L2Base()\n> >>> +\t{\n> >>> +\t}\n> >>> +\n> >>> +\tvirtual int open() = 0;\n> >>> +\tvirtual void close() = 0;\n> >\n> > There's no need to make the open() and close() functions virtual, or\n> > even to declare them in the base class, as you never call them on a\n> > V4L2Base instance. All you need is an fd_ in the base. It could also\n> > possibly make sense to add an ioctl() function in the base class.\n> \n> +1 for ioctl wrapper. It would save some code.\n> \n> > However, I'm not too fond of storing fd_ in the base class and\n> > implementing open() and close() in derived classes. One option would be\n> > to implement protected open() and close() functions in the base class\n> > that would just open and close the fd, and call them from the open() and\n> > close() function of the derived classes. You wouldn't need to make the\n> > base methods virtual in this case, and I think I would make fd_ private\n> > and provide a protected fd() accessor.\n> \n> Isn't this over-engineered? I mean, the derived classes would have to\n> call the base class methods that just wrap open/close without doing\n> much more, and have to call a method to access the fd_, without any\n> real benefit in my opinion. I would rather move fd_ to the derived\n> classes and have open()/close() implemented there independently.\n> \n> Unless I'm missing some obvious reason not to do so...\n\nYou can't move fd_ to the derived classes as you need it in the ioctl()\nand control functions in the base class. I don't think an accessor for\nthe fd would be a big deal, as it can be inlined and would thus be free.\nopen() and close() would indeed be a bit small, but I think that\ngrouping them with fd_ in the same class would be a better design.\n\n> >>> +\tbool isOpen() const;\n> >>> +\n> >>> +protected:\n> >>> +\tint fd_;\n> >>> +};\n> >>> +\n> >>> +}; /* namespace libcamera */\n> >>> +\n> >>> +#endif /* __LIBCAMERA_V4L2_BASE__ */\n> >>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> >>> index bdecc087fe5a..d9bcdb921b8c 100644\n> >>> --- a/src/libcamera/include/v4l2_device.h\n> >>> +++ b/src/libcamera/include/v4l2_device.h\n> >>> @@ -17,6 +17,7 @@\n> >>>  #include <libcamera/signal.h>\n> >>>\n> >>>  #include \"log.h\"\n> >>> +#include \"v4l2_base.h\"\n> >>>\n> >>>  namespace libcamera {\n> >>>\n> >>> @@ -111,7 +112,7 @@ public:\n> >>>  \tconst std::string toString() const;\n> >>>  };\n> >>>\n> >>> -class V4L2Device : protected Loggable\n> >>> +class V4L2Device : public V4L2Base, protected Loggable\n> >>>  {\n> >>>  public:\n> >>>  \texplicit V4L2Device(const std::string &deviceNode);\n> >>> @@ -121,9 +122,8 @@ public:\n> >>>\n> >>>  \tV4L2Device &operator=(const V4L2Device &) = delete;\n> >>>\n> >>> -\tint open();\n> >>> -\tbool isOpen() const;\n> >>> -\tvoid close();\n> >>> +\tint open() override;\n> >>> +\tvoid close() override;\n> >>>\n> >>>  \tconst char *driverName() const { return caps_.driver(); }\n> >>>  \tconst char *deviceName() const { return caps_.card(); }\n> >>> @@ -167,7 +167,6 @@ private:\n> >>>  \tvoid bufferAvailable(EventNotifier *notifier);\n> >>>\n> >>>  \tstd::string deviceNode_;\n> >>> -\tint fd_;\n> >>>  \tV4L2Capability caps_;\n> >>>\n> >>>  \tenum v4l2_buf_type bufferType_;\n> >>> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h\n> >>> index 3e4e5107aebe..bdef3e69dd8c 100644\n> >>> --- a/src/libcamera/include/v4l2_subdevice.h\n> >>> +++ b/src/libcamera/include/v4l2_subdevice.h\n> >>> @@ -16,6 +16,7 @@\n> >>>  #include \"formats.h\"\n> >>>  #include \"log.h\"\n> >>>  #include \"media_object.h\"\n> >>> +#include \"v4l2_base.h\"\n> >>>\n> >>>  namespace libcamera {\n> >>>\n> >>> @@ -28,7 +29,7 @@ struct V4L2SubdeviceFormat {\n> >>>  \tconst std::string toString() const;\n> >>>  };\n> >>>\n> >>> -class V4L2Subdevice : protected Loggable\n> >>> +class V4L2Subdevice : public V4L2Base, protected Loggable\n> >>>  {\n> >>>  public:\n> >>>  \texplicit V4L2Subdevice(const MediaEntity *entity);\n> >>> @@ -36,9 +37,8 @@ public:\n> >>>  \tV4L2Subdevice &operator=(const V4L2Subdevice &) = delete;\n> >>>  \t~V4L2Subdevice();\n> >>>\n> >>> -\tint open();\n> >>> -\tbool isOpen() const;\n> >>> -\tvoid close();\n> >>> +\tint open() override;\n> >>> +\tvoid close() override;\n> >>>\n> >>>  \tconst MediaEntity *entity() const { return entity_; }\n> >>>\n> >>> @@ -64,7 +64,6 @@ private:\n> >>>  \t\t\t Rectangle *rect);\n> >>>\n> >>>  \tconst MediaEntity *entity_;\n> >>> -\tint fd_;\n> >>>  };\n> >>>\n> >>>  } /* namespace libcamera */\n> >>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >>> index 6a73580d71f5..6d858a22531e 100644\n> >>> --- a/src/libcamera/meson.build\n> >>> +++ b/src/libcamera/meson.build\n> >>> @@ -21,6 +21,7 @@ libcamera_sources = files([\n> >>>      'stream.cpp',\n> >>>      'timer.cpp',\n> >>>      'utils.cpp',\n> >>> +    'v4l2_base.cpp',\n> >>>      'v4l2_device.cpp',\n> >>>      'v4l2_subdevice.cpp',\n> >>>  ])\n> >>> @@ -38,6 +39,7 @@ libcamera_headers = files([\n> >>>      'include/media_object.h',\n> >>>      'include/pipeline_handler.h',\n> >>>      'include/utils.h',\n> >>> +    'include/v4l2_base.h',\n> >>>      'include/v4l2_device.h',\n> >>>      'include/v4l2_subdevice.h',\n> >>>  ])\n> >>> diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp\n> >>> new file mode 100644\n> >>> index 000000000000..7d05a3c82e4d\n> >>> --- /dev/null\n> >>> +++ b/src/libcamera/v4l2_base.cpp\n> >>> @@ -0,0 +1,64 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2019, Google Inc.\n> >>> + *\n> >>> + * v4l2_base.cpp - Common base for V4L2 devices and subdevices\n> >>> + */\n> >>> +\n> >>> +#include \"v4l2_base.h\"\n> >>> +\n> >>> +/**\n> >>> + * \\file v4l2_base.h\n> >>> + * \\brief Common base for V4L2 devices and subdevices\n> >>> + */\n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +/**\n> >>> + * \\class V4L2Base\n> >>> + * \\brief Base class for V4L2Device and V4L2Subdevice\n> >>> + *\n> >>> + * The V4L2Base class groups together the methods and fields common to\n> >>> + * both V4L2Device and V4L2Subdevice, and provide a base abstract class which\n> >>> + * defines a streamlined interface that both the derived class have to implement.\n> >>> + *\n> >>> + * The interface defined by V4L2Base only requires derived classes to implement\n> >>> + * methods that modifies the status of the file descriptor associated with\n> >>> + * the video device or subdevice, such as \\a open() and \\a close().\n> >>> + *\n> >>> + * Methods common to V4L2Device and V4L2Subdevice, such as V4L2 controls\n> >>> + * support are implemented in the base class to maximize code re-use.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\brief Open a V4L2 device or subdevice\n> >>> + *\n> >>> + * Pure virtual method to be implemented by the derived classes.\n> >>> + *\n> >>> + * \\return 0 on success or a negative error code otherwise\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\brief Close the device or subdevice\n> >>> + *\n> >>> + * Pure virtual method to be implemented by the derived classes.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\brief Check if the V4L2 device or subdevice is open\n> >>> + * \\return True if the V4L2 device or subdevice is open, false otherwise\n> >>> + */\n> >>> +bool V4L2Base::isOpen() const\n> >>> +{\n> >>> +\treturn fd_ != -1;\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\var V4L2Base::fd_\n> >>> + * \\brief The V4L2 device or subdevice device node file descriptor\n> >>> + *\n> >>> + * The file descriptor is initialized to -1 and reset to this value once\n> >>> + * the device or subdevice gets closed.\n> >>> + */\n> >>> +\n> >>> +}; /* namespace libcamera */\n> >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >>> index 0821bd75fb42..64533e88a512 100644\n> >>> --- a/src/libcamera/v4l2_device.cpp\n> >>> +++ b/src/libcamera/v4l2_device.cpp\n> >>> @@ -270,9 +270,11 @@ const std::string V4L2DeviceFormat::toString() const\n> >>>   * \\param[in] deviceNode The file-system path to the video device node\n> >>>   */\n> >>>  V4L2Device::V4L2Device(const std::string &deviceNode)\n> >>> -\t: deviceNode_(deviceNode), fd_(-1), bufferPool_(nullptr),\n> >>> +\t: deviceNode_(deviceNode), bufferPool_(nullptr),\n> >>>  \t  queuedBuffersCount_(0), fdEvent_(nullptr)\n> >>>  {\n> >>> +\tfd_ = -1;\n> >>\n> >> Shouldn't this be initialised in the base class?\n> >>\n> >>> +\n> >>>  \t/*\n> >>>  \t * We default to an MMAP based CAPTURE device, however this will be\n> >>>  \t * updated based upon the device capabilities.\n> >>> @@ -368,15 +370,6 @@ int V4L2Device::open()\n> >>>  \treturn 0;\n> >>>  }\n> >>>\n> >>> -/**\n> >>> - * \\brief Check if device is successfully opened\n> >>> - * \\return True if the device is open, false otherwise\n> >>> - */\n> >>> -bool V4L2Device::isOpen() const\n> >>> -{\n> >>> -\treturn fd_ != -1;\n> >>> -}\n> >>> -\n> >>>  /**\n> >>>   * \\brief Close the device, releasing any resources acquired by open()\n> >>>   */\n> >>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> >>> index fceee33156e9..3a053fadb3f6 100644\n> >>> --- a/src/libcamera/v4l2_subdevice.cpp\n> >>> +++ b/src/libcamera/v4l2_subdevice.cpp\n> >>> @@ -102,8 +102,9 @@ const std::string V4L2SubdeviceFormat::toString() const\n> >>>   * path\n> >>>   */\n> >>>  V4L2Subdevice::V4L2Subdevice(const MediaEntity *entity)\n> >>> -\t: entity_(entity), fd_(-1)\n> >>> +\t: entity_(entity)\n> >>>  {\n> >>> +\tfd_ = -1;\n> >>>  }\n> >>>\n> >>>  V4L2Subdevice::~V4L2Subdevice()\n> >>> @@ -137,15 +138,6 @@ int V4L2Subdevice::open()\n> >>>  \treturn 0;\n> >>>  }\n> >>>\n> >>> -/**\n> >>> - * \\brief Check if the subdevice is open\n> >>> - * \\return True if the subdevice is open, false otherwise\n> >>> - */\n> >>> -bool V4L2Subdevice::isOpen() const\n> >>> -{\n> >>> -\treturn fd_ != -1;\n> >>> -}\n> >>> -\n> >>>  /**\n> >>>   * \\brief Close the subdevice, releasing any resources acquired by open()\n> >>>   */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 435C263751\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2019 10:23:08 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(233.56-78-194.adsl-static.isp.belgacom.be [194.78.56.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3D229CB;\n\tMon, 10 Jun 2019 10:23:07 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560154987;\n\tbh=TtaIN1CLZJVj3g3pTOnvVADpqFOSU4RLy3/W086jwSw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=njY35hlqsq+nu94iL8rhGhzHBMkW2tQB2XHK/Kc6qILc+3AAlsGLRGM/cWEr2rSof\n\tFZKRoP4P9mT2NxHQqxy/EBjysXwchtgXFL2lc68q/uYdheB2auXzUrJiGnDRsTan6L\n\tRKsLm3Q1gNagsBDs2WhE2JgtOPU039Lh1dBnders=","Date":"Mon, 10 Jun 2019 11:22:52 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190610082252.GJ4806@pendragon.ideasonboard.com>","References":"<20190602130435.18780-1-jacopo@jmondi.org>\n\t<20190602130435.18780-3-jacopo@jmondi.org>\n\t<995d4b50-cc86-eed3-6b83-7f28e4253430@ideasonboard.com>\n\t<20190608154805.GF4786@pendragon.ideasonboard.com>\n\t<20190610070605.lnldvqpxeum54eyt@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190610070605.lnldvqpxeum54eyt@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: Provide a V4L2Base\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 10 Jun 2019 08:23:08 -0000"}}]