[{"id":21161,"web_url":"https://patchwork.libcamera.org/comment/21161/","msgid":"<163768744015.3059017.10171555261547304064@Monstersaurus>","date":"2021-11-23T17:10:40","subject":"Re: [libcamera-devel] [PATCH v4 3/4] libcamera: camera_lens: Add a\n\tnew class to model a camera lens","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Han-Lin Chen (2021-11-23 12:37:50)\n> The CameraLens class abstracts camera lens and provides helper\n> functions to ease interactions with them.\n> \n> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  Documentation/index.rst                    |   1 +\n>  Documentation/lens_driver_requirements.rst |  28 ++++\n>  Documentation/meson.build                  |   1 +\n>  include/libcamera/internal/camera_lens.h   |  48 +++++++\n>  include/libcamera/internal/meson.build     |   1 +\n>  src/libcamera/camera_lens.cpp              | 150 +++++++++++++++++++++\n>  src/libcamera/meson.build                  |   1 +\n>  7 files changed, 230 insertions(+)\n>  create mode 100644 Documentation/lens_driver_requirements.rst\n>  create mode 100644 include/libcamera/internal/camera_lens.h\n>  create mode 100644 src/libcamera/camera_lens.cpp\n> \n> diff --git a/Documentation/index.rst b/Documentation/index.rst\n> index 1f4fc485..0ee10044 100644\n> --- a/Documentation/index.rst\n> +++ b/Documentation/index.rst\n> @@ -21,3 +21,4 @@\n>     Tracing guide <guides/tracing>\n>     Environment variables <environment_variables>\n>     Sensor driver requirements <sensor_driver_requirements>\n> +   Lens driver requirements <lens_driver_requirements>\n> diff --git a/Documentation/lens_driver_requirements.rst b/Documentation/lens_driver_requirements.rst\n> new file mode 100644\n> index 00000000..afc300cf\n> --- /dev/null\n> +++ b/Documentation/lens_driver_requirements.rst\n> @@ -0,0 +1,28 @@\n> +.. SPDX-License-Identifier: CC-BY-SA-4.0\n> +\n> +.. _lens-driver-requirements:\n> +\n> +Lens Driver Requirements\n> +========================\n> +\n> +libcamera handles lens devices in the CameraLens class and defines\n> +a consistent interface through its API towards other library components.\n> +\n> +The CameraLens class uses the V4L2 subdev kernel API to interface with the\n> +camera lens through one or multiple sub-devices exposed in userspace by\n> +the lens driver.\n> +\n> +In order for libcamera to be fully operational and provide all the required\n> +information to interface with the camera lens to applications and pipeline\n> +handlers, a set of mandatory features the driver has to support has been defined.\n> +\n> +Mandatory Requirements\n> +----------------------\n> +\n> +The lens driver is assumed to be fully compliant with the V4L2 specification.\n> +\n> +The lens driver shall support the following V4L2 controls:\n> +\n> +* `V4L2_CID_FOCUS_ABSOLUTE`_\n> +\n> +.. _V4L2_CID_FOCUS_ABSOLUTE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-camera.html\n> diff --git a/Documentation/meson.build b/Documentation/meson.build\n> index 4c972675..33af82aa 100644\n> --- a/Documentation/meson.build\n> +++ b/Documentation/meson.build\n> @@ -66,6 +66,7 @@ if sphinx.found()\n>          'guides/pipeline-handler.rst',\n>          'guides/tracing.rst',\n>          'index.rst',\n> +        'lens_driver_requirements.rst',\n>          'sensor_driver_requirements.rst',\n>         '../README.rst',\n>      ]\n> diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h\n> new file mode 100644\n> index 00000000..fe83d4ad\n> --- /dev/null\n> +++ b/include/libcamera/internal/camera_lens.h\n> @@ -0,0 +1,48 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_lens.h - A camera lens\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_CAMERA_LENS_H__\n> +#define __LIBCAMERA_INTERNAL_CAMERA_LENS_H__\n> +\n> +#include <libcamera/base/class.h>\n> +#include <libcamera/base/log.h>\n> +\n> +#include <libcamera/controls.h>\n> +\n> +#include \"libcamera/internal/v4l2_subdevice.h\"\n> +\n> +namespace libcamera {\n> +\n> +class MediaEntity;\n> +\n> +class CameraLens : protected Loggable\n> +{\n> +public:\n> +       explicit CameraLens(const MediaEntity *entity);\n> +       ~CameraLens();\n> +\n> +       int init();\n> +       int setFocusPostion(int32_t position);\n> +\n> +       const std::string &model() const { return model_; }\n> +\n> +protected:\n> +       std::string logPrefix() const override;\n> +\n> +private:\n> +       LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraLens)\n> +\n> +       int validateLensDriver();\n> +\n> +       const MediaEntity *entity_;\n> +       std::unique_ptr<V4L2Subdevice> subdev_;\n> +\n> +       std::string model_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_CAMERA_LENS_H__ */\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index de8630fe..7e9b9998 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -14,6 +14,7 @@ libcamera_internal_headers = files([\n>      'byte_stream_buffer.h',\n>      'camera.h',\n>      'camera_controls.h',\n> +    'camera_lens.h',\n>      'camera_sensor.h',\n>      'camera_sensor_properties.h',\n>      'camera_utils.h',\n> diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp\n> new file mode 100644\n> index 00000000..cb7159fe\n> --- /dev/null\n> +++ b/src/libcamera/camera_lens.cpp\n> @@ -0,0 +1,150 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_lens.cpp - A camera lens\n> + */\n> +\n> +#include \"libcamera/internal/camera_lens.h\"\n> +\n> +#include <regex>\n\nI don't think regex is used anymore.\n\n> +#include <libcamera/property_ids.h>\n> +\n> +#include \"libcamera/internal/camera_utils.h\"\n> +#include \"libcamera/internal/media_device.h\"\n> +#include \"libcamera/internal/sysfs.h\"\n\nnor sysfs.h...\n\n> +\n> +/**\n> + * \\file camera_lens.h\n> + * \\brief A camera lens\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(CameraLens)\n> +\n> +/**\n> + * \\class CameraLens\n> + * \\brief A camera lens based on V4L2 subdevices\n> + *\n> + * The CameraLens class eases handling of lens for pipeline handlers by\n> + * hiding the details of the V4L2 subdevice kernel API and caching lens\n> + * information.\n> + *\n> + * The implementation is currently limited to lens that expose a single V4L2\n> + * subdevice. It will be extended to support more complex devices as the needs\n> + * arise.\n> + */\n> +\n> +/**\n> + * \\brief Construct a CameraLens\n> + * \\param[in] entity The media entity backing the camera lens\n> + *\n> + * Once constructed the instance must be initialized with init().\n> + */\n> +CameraLens::CameraLens(const MediaEntity *entity)\n> +       : entity_(entity)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Destroy a CameraLens\n> + */\n> +CameraLens::~CameraLens() = default;\n> +\n> +/**\n> + * \\brief Initialize the camera lens instance\n> + *\n> + * This function performs the initialisation steps of the CameraLens that may\n> + * fail. It shall be called once and only once after constructing the instance.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int CameraLens::init()\n> +{\n> +       if (entity_->function() != MEDIA_ENT_F_LENS) {\n> +               LOG(CameraLens, Error)\n> +                       << \"Invalid lens function \"\n> +                       << utils::hex(entity_->function());\n> +               return -EINVAL;\n> +       }\n> +\n> +       model_ = extractModelFromEntityName(entity_->name());\n> +\n> +       /* Create and open the subdev. */\n> +       subdev_ = std::make_unique<V4L2Subdevice>(entity_);\n> +       int ret = subdev_->open();\n> +       if (ret < 0)\n> +               return ret;\n\nThis is probably why I think I'd make the model handled by the\nV4L2Subdevice.\n\nWe /know/ we're creating a V4L2Subdevice here, so we could assign\nsomething like\n\n\tmodel_ = subdev_->model();\n\nOr simply have model() call directly onto the subdev which would have\nthe implementation...\n\n\n> +\n> +       ret = validateLensDriver();\n> +       if (ret)\n> +               return ret;\n> +\n> +       return 0;\n> +}\n> +\n> +/**\n> + * \\brief This function sets the focal point of the lens to a specific position.\n> + * \\param[in] position The focal point of the lens\n> + *\n> + * This function sets the value of focal point of the lens as in \\a position.\n> + *\n> + * \\return 0 on success or -EINVAL otherwise\n> + */\n> +int CameraLens::setFocusPostion(int32_t position)\n> +{\n> +       ControlList lensCtrls(subdev_->controls());\n> +       lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, static_cast<int32_t>(position));\n> +\n> +       if (subdev_->setControls(&lensCtrls))\n> +               return -EINVAL;\n> +\n> +       return 0;\n> +}\n\nI fear this is going the same way that the TestPatternMode is going on\nthe CameraSensor.\n\nI don't think we should expose a function for each control and setting\non the devices.\n\nThe device should accept the controls, parse it and apply it.\n\nThe setFocusPosition helper function is probably good to keep as it is -\nbut I think a CameraLens should be taking a ControlList (of libcamera\ncontrols) and doing something like:\n\n\n+/**\n+ * \\brief Write libcamera controls to the lens\n+ * \\param[in] list The list of controls to write\n+ *\n+ * Supported libcamera controls are interpreted and converted to V4L2\n+ * controls to write to the device.\n+ *\n+ * \\return 0 on success or an error code otherwise\n+ * \\retval -EINVAL One of the control is not supported or not accessible\n+ */\n+int CameraLens::setControls(ControlList *list)\n+{\n+\tif (!list)\n+\t\treturn -ENODATA;\n+\n+\tconst ControlIdMap *idmap = list->idMap();\n+\tif (idmap != &controls::controls) {\n+\t\tLOG(CameraLens, Error) << \"Invalid Control List\";\n+\t\treturn -EINVAL;\n+\t}\n+\n+\tControlList subdevControls;\n+\n+\tfor (const auto &ctrl : *list) {\n+\t\tunsigned int id = ctrl.first;\n+\t\tconst ControlValue &value = ctrl.second;\n+\n+\t\t// switch case would be nicer here.... but isn't\n+\t\t// currently possible\n+\n+\t\t// Also not a libcamera control yet\n+\t\tif (id == controls::FocusAbsolute) {\n+\t\t\tint position = value.get<int>();\n+\t\t\tsubdevControls.set(V4L2_CID_FOCUS_ABSOLUTE, position);\n+\t\t}\n+\n+\t\t// Not a real libcamera control currently\n+\t\tif (id == controls::FocusRelative) {\n+\t\t\t// Not implemented, showing other control\n+\t\t\t// handling only\n+\t\t}\n+\t}\n+\n+\treturn subdev_->setControls(&subdevControls);\n+}\n+\n\nI think this is what CameraSensor should be doing too, but we have this\nawkward precedent that it can already take a V4L2ControlList directly...\n\n\nGiven that a lens can only currently set a position anyway, it's not\nsuch a big deal if this is considered if/when we need more controls\ninstead, and we keep the direct and simple setting function.\n\n--\nKieran\n\n\n> +\n> +int CameraLens::validateLensDriver()\n> +{\n> +       int err = 0;\n> +       static constexpr uint32_t mandatoryControls[] = {\n> +               V4L2_CID_FOCUS_ABSOLUTE\n> +       };\n> +\n> +       const ControlInfoMap &controls = subdev_->controls();\n> +       for (uint32_t ctrl : mandatoryControls) {\n> +               if (!controls.count(ctrl)) {\n> +                       LOG(CameraLens, Error)\n> +                               << \"Mandatory V4L2 control \" << utils::hex(ctrl)\n> +                               << \" not available\";\n> +                       err = -EINVAL;\n> +               }\n> +       }\n> +\n> +       if (err) {\n> +               LOG(CameraLens, Error)\n> +                       << \"The lens kernel driver needs to be fixed\";\n> +               LOG(CameraLens, Error)\n> +                       << \"See Documentation/lens_driver_requirements.rst in\"\n> +                       << \" the libcamera sources for more information\";\n> +               return err;\n> +       }\n> +\n> +       return 0;\n> +}\n> +\n> +/**\n> + * \\fn CameraLens::model()\n> + * \\brief Retrieve the lens model name\n> + *\n> + * The lens model name is a free-formed string that uniquely identifies the\n> + * lens model.\n> + *\n> + * \\return The lens model name\n> + */\n> +\n> +std::string CameraLens::logPrefix() const\n> +{\n> +       return \"'\" + entity_->name() + \"'\";\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 563ed861..2100805d 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -5,6 +5,7 @@ libcamera_sources = files([\n>      'byte_stream_buffer.cpp',\n>      'camera.cpp',\n>      'camera_controls.cpp',\n> +    'camera_lens.cpp',\n>      'camera_manager.cpp',\n>      'camera_sensor.cpp',\n>      'camera_sensor_properties.cpp',\n> -- \n> 2.34.0.rc2.393.gf8c9666880-goog\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E29FCBF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 23 Nov 2021 17:10:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B6B860230;\n\tTue, 23 Nov 2021 18:10:44 +0100 (CET)","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 6FCEB60121\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 23 Nov 2021 18:10:42 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 057B6A1B;\n\tTue, 23 Nov 2021 18:10:41 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bFbxtV95\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637687442;\n\tbh=YFiahe1NjpdAEJsfp2c4Q0zDiuFzOfJsaxCJwanfGpU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=bFbxtV95+vqBFSgB1lu4GIe32DG46UJSEKNm8linEHYpKNEXyGQ/hmXuWXAinbAb+\n\t+heKjrgwPgQM6iJSctBngy0bl4pDohnyYAMMSgUdWUtQPkLY8Y7DEK2vwBbQNoC9ol\n\taQfsuyiJZU9iQJXMWJ1ALYGl/QHHxqSgjeBBTSic=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211123123751.3194696-4-hanlinchen@chromium.org>","References":"<20211123123751.3194696-1-hanlinchen@chromium.org>\n\t<20211123123751.3194696-4-hanlinchen@chromium.org>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Han-Lin Chen <hanlinchen@chromium.org>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 23 Nov 2021 17:10:40 +0000","Message-ID":"<163768744015.3059017.10171555261547304064@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 3/4] libcamera: camera_lens: Add a\n\tnew class to model a camera lens","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21175,"web_url":"https://patchwork.libcamera.org/comment/21175/","msgid":"<YZ3SyFKYjxcfa2TE@pendragon.ideasonboard.com>","date":"2021-11-24T05:51:04","subject":"Re: [libcamera-devel] [PATCH v4 3/4] libcamera: camera_lens: Add a\n\tnew class to model a camera lens","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Tue, Nov 23, 2021 at 05:10:40PM +0000, Kieran Bingham wrote:\n> Quoting Han-Lin Chen (2021-11-23 12:37:50)\n> > The CameraLens class abstracts camera lens and provides helper\n> > functions to ease interactions with them.\n> > \n> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>\n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > ---\n> >  Documentation/index.rst                    |   1 +\n> >  Documentation/lens_driver_requirements.rst |  28 ++++\n> >  Documentation/meson.build                  |   1 +\n> >  include/libcamera/internal/camera_lens.h   |  48 +++++++\n> >  include/libcamera/internal/meson.build     |   1 +\n> >  src/libcamera/camera_lens.cpp              | 150 +++++++++++++++++++++\n> >  src/libcamera/meson.build                  |   1 +\n> >  7 files changed, 230 insertions(+)\n> >  create mode 100644 Documentation/lens_driver_requirements.rst\n> >  create mode 100644 include/libcamera/internal/camera_lens.h\n> >  create mode 100644 src/libcamera/camera_lens.cpp\n> > \n> > diff --git a/Documentation/index.rst b/Documentation/index.rst\n> > index 1f4fc485..0ee10044 100644\n> > --- a/Documentation/index.rst\n> > +++ b/Documentation/index.rst\n> > @@ -21,3 +21,4 @@\n> >     Tracing guide <guides/tracing>\n> >     Environment variables <environment_variables>\n> >     Sensor driver requirements <sensor_driver_requirements>\n> > +   Lens driver requirements <lens_driver_requirements>\n> > diff --git a/Documentation/lens_driver_requirements.rst b/Documentation/lens_driver_requirements.rst\n> > new file mode 100644\n> > index 00000000..afc300cf\n> > --- /dev/null\n> > +++ b/Documentation/lens_driver_requirements.rst\n> > @@ -0,0 +1,28 @@\n> > +.. SPDX-License-Identifier: CC-BY-SA-4.0\n> > +\n> > +.. _lens-driver-requirements:\n> > +\n> > +Lens Driver Requirements\n> > +========================\n> > +\n> > +libcamera handles lens devices in the CameraLens class and defines\n> > +a consistent interface through its API towards other library components.\n> > +\n> > +The CameraLens class uses the V4L2 subdev kernel API to interface with the\n> > +camera lens through one or multiple sub-devices exposed in userspace by\n\n\"... through a sub-device exposed to userspace ...\"\n\nCamera sensors may use multiple subdevs, but VCMs use a single subdev\nonly.\n\n> > +the lens driver.\n> > +\n> > +In order for libcamera to be fully operational and provide all the required\n> > +information to interface with the camera lens to applications and pipeline\n> > +handlers, a set of mandatory features the driver has to support has been defined.\n> > +\n> > +Mandatory Requirements\n> > +----------------------\n> > +\n> > +The lens driver is assumed to be fully compliant with the V4L2 specification.\n> > +\n> > +The lens driver shall support the following V4L2 controls:\n> > +\n> > +* `V4L2_CID_FOCUS_ABSOLUTE`_\n> > +\n> > +.. _V4L2_CID_FOCUS_ABSOLUTE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-camera.html\n> > diff --git a/Documentation/meson.build b/Documentation/meson.build\n> > index 4c972675..33af82aa 100644\n> > --- a/Documentation/meson.build\n> > +++ b/Documentation/meson.build\n> > @@ -66,6 +66,7 @@ if sphinx.found()\n> >          'guides/pipeline-handler.rst',\n> >          'guides/tracing.rst',\n> >          'index.rst',\n> > +        'lens_driver_requirements.rst',\n> >          'sensor_driver_requirements.rst',\n> >         '../README.rst',\n> >      ]\n> > diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h\n> > new file mode 100644\n> > index 00000000..fe83d4ad\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/camera_lens.h\n> > @@ -0,0 +1,48 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * camera_lens.h - A camera lens\n> > + */\n> > +#ifndef __LIBCAMERA_INTERNAL_CAMERA_LENS_H__\n> > +#define __LIBCAMERA_INTERNAL_CAMERA_LENS_H__\n\nYou need to include <memory> and <string>.\n\n> > +\n> > +#include <libcamera/base/class.h>\n> > +#include <libcamera/base/log.h>\n> > +\n> > +#include <libcamera/controls.h>\n> > +\n> > +#include \"libcamera/internal/v4l2_subdevice.h\"\n\nYou could forward-declare V4L2Subdevice instead.\n\n> > +\n> > +namespace libcamera {\n> > +\n> > +class MediaEntity;\n> > +\n> > +class CameraLens : protected Loggable\n> > +{\n> > +public:\n> > +       explicit CameraLens(const MediaEntity *entity);\n> > +       ~CameraLens();\n> > +\n> > +       int init();\n> > +       int setFocusPostion(int32_t position);\n\nCan the position be negative ?\n\n> > +\n> > +       const std::string &model() const { return model_; }\n> > +\n> > +protected:\n> > +       std::string logPrefix() const override;\n> > +\n> > +private:\n> > +       LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraLens)\n> > +\n> > +       int validateLensDriver();\n> > +\n> > +       const MediaEntity *entity_;\n> > +       std::unique_ptr<V4L2Subdevice> subdev_;\n> > +\n> > +       std::string model_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_INTERNAL_CAMERA_LENS_H__ */\n> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index de8630fe..7e9b9998 100644\n> > --- a/include/libcamera/internal/meson.build\n> > +++ b/include/libcamera/internal/meson.build\n> > @@ -14,6 +14,7 @@ libcamera_internal_headers = files([\n> >      'byte_stream_buffer.h',\n> >      'camera.h',\n> >      'camera_controls.h',\n> > +    'camera_lens.h',\n> >      'camera_sensor.h',\n> >      'camera_sensor_properties.h',\n> >      'camera_utils.h',\n> > diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp\n> > new file mode 100644\n> > index 00000000..cb7159fe\n> > --- /dev/null\n> > +++ b/src/libcamera/camera_lens.cpp\n> > @@ -0,0 +1,150 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * camera_lens.cpp - A camera lens\n> > + */\n> > +\n> > +#include \"libcamera/internal/camera_lens.h\"\n> > +\n> > +#include <regex>\n> \n> I don't think regex is used anymore.\n> \n> > +#include <libcamera/property_ids.h>\n> > +\n> > +#include \"libcamera/internal/camera_utils.h\"\n> > +#include \"libcamera/internal/media_device.h\"\n> > +#include \"libcamera/internal/sysfs.h\"\n> \n> nor sysfs.h...\n\nOn the other hand, utils.h is needed.\n\n> > +\n> > +/**\n> > + * \\file camera_lens.h\n> > + * \\brief A camera lens\n\nMaybe \"A camera lens controller\" ? Up to you.\n\nWe will also need to support focus lens. I'm not sure if we'll need a\nseparate class, we'll figure it out then.\n\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(CameraLens)\n> > +\n> > +/**\n> > + * \\class CameraLens\n> > + * \\brief A camera lens based on V4L2 subdevices\n> > + *\n> > + * The CameraLens class eases handling of lens for pipeline handlers by\n> > + * hiding the details of the V4L2 subdevice kernel API and caching lens\n> > + * information.\n> > + *\n> > + * The implementation is currently limited to lens that expose a single V4L2\n> > + * subdevice. It will be extended to support more complex devices as the needs\n> > + * arise.\n\nI'd drop this paragraph for the reason explained above.\n\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a CameraLens\n> > + * \\param[in] entity The media entity backing the camera lens\n\ns/lens/lens controller/\n\n> > + *\n> > + * Once constructed the instance must be initialized with init().\n> > + */\n> > +CameraLens::CameraLens(const MediaEntity *entity)\n> > +       : entity_(entity)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Destroy a CameraLens\n> > + */\n> > +CameraLens::~CameraLens() = default;\n> > +\n> > +/**\n> > + * \\brief Initialize the camera lens instance\n> > + *\n> > + * This function performs the initialisation steps of the CameraLens that may\n> > + * fail. It shall be called once and only once after constructing the instance.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int CameraLens::init()\n> > +{\n> > +       if (entity_->function() != MEDIA_ENT_F_LENS) {\n> > +               LOG(CameraLens, Error)\n> > +                       << \"Invalid lens function \"\n> > +                       << utils::hex(entity_->function());\n> > +               return -EINVAL;\n> > +       }\n> > +\n> > +       model_ = extractModelFromEntityName(entity_->name());\n> > +\n> > +       /* Create and open the subdev. */\n> > +       subdev_ = std::make_unique<V4L2Subdevice>(entity_);\n> > +       int ret = subdev_->open();\n> > +       if (ret < 0)\n> > +               return ret;\n> \n> This is probably why I think I'd make the model handled by the\n> V4L2Subdevice.\n> \n> We /know/ we're creating a V4L2Subdevice here, so we could assign\n> something like\n> \n> \tmodel_ = subdev_->model();\n> \n> Or simply have model() call directly onto the subdev which would have\n> the implementation...\n\nAgreed. I'd however like to drop use of model() in patch 4/4, in favour\nof ancillary links that Daniel Scally is working on. This doesn't\nprevent moving model() to V4L2Subdevice though.\n\n> > +\n> > +       ret = validateLensDriver();\n> > +       if (ret)\n> > +               return ret;\n> > +\n> > +       return 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief This function sets the focal point of the lens to a specific position.\n> > + * \\param[in] position The focal point of the lens\n> > + *\n> > + * This function sets the value of focal point of the lens as in \\a position.\n> > + *\n> > + * \\return 0 on success or -EINVAL otherwise\n> > + */\n> > +int CameraLens::setFocusPostion(int32_t position)\n> > +{\n> > +       ControlList lensCtrls(subdev_->controls());\n> > +       lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, static_cast<int32_t>(position));\n> > +\n> > +       if (subdev_->setControls(&lensCtrls))\n> > +               return -EINVAL;\n> > +\n> > +       return 0;\n> > +}\n> \n> I fear this is going the same way that the TestPatternMode is going on\n> the CameraSensor.\n> \n> I don't think we should expose a function for each control and setting\n> on the devices.\n> \n> The device should accept the controls, parse it and apply it.\n> \n> The setFocusPosition helper function is probably good to keep as it is -\n> but I think a CameraLens should be taking a ControlList (of libcamera\n> controls) and doing something like:\n> \n> +/**\n> + * \\brief Write libcamera controls to the lens\n> + * \\param[in] list The list of controls to write\n> + *\n> + * Supported libcamera controls are interpreted and converted to V4L2\n> + * controls to write to the device.\n> + *\n> + * \\return 0 on success or an error code otherwise\n> + * \\retval -EINVAL One of the control is not supported or not accessible\n> + */\n> +int CameraLens::setControls(ControlList *list)\n> +{\n> +\tif (!list)\n> +\t\treturn -ENODATA;\n\nIf it can't be null, we can pass a reference.\n\n> +\n> +\tconst ControlIdMap *idmap = list->idMap();\n> +\tif (idmap != &controls::controls) {\n> +\t\tLOG(CameraLens, Error) << \"Invalid Control List\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tControlList subdevControls;\n> +\n> +\tfor (const auto &ctrl : *list) {\n> +\t\tunsigned int id = ctrl.first;\n> +\t\tconst ControlValue &value = ctrl.second;\n> +\n> +\t\t// switch case would be nicer here.... but isn't\n> +\t\t// currently possible\n> +\n> +\t\t// Also not a libcamera control yet\n\nIf we go in this direction (and I think it's a good idea), then I\nbelieve we'll have to create internal controls for the camera sensor and\nthe lens controller. Some of the libcamera public controls may match,\nbut we'll most probably need internal controls that don't make sense in\nthe public API. We should have all the infrastructure we need for this,\nso it shouldn't be too difficult technically (but may result in\nnitpicking).\n\nThis being said, as we currently expose a single control in this class,\nwe could also start with an ad-hoc function.\n\n> +\t\tif (id == controls::FocusAbsolute) {\n> +\t\t\tint position = value.get<int>();\n> +\t\t\tsubdevControls.set(V4L2_CID_FOCUS_ABSOLUTE, position);\n> +\t\t}\n> +\n> +\t\t// Not a real libcamera control currently\n> +\t\tif (id == controls::FocusRelative) {\n> +\t\t\t// Not implemented, showing other control\n> +\t\t\t// handling only\n> +\t\t}\n> +\t}\n> +\n> +\treturn subdev_->setControls(&subdevControls);\n> +}\n> +\n> \n> I think this is what CameraSensor should be doing too, but we have this\n> awkward precedent that it can already take a V4L2ControlList directly...\n> \n> Given that a lens can only currently set a position anyway, it's not\n> such a big deal if this is considered if/when we need more controls\n> instead, and we keep the direct and simple setting function.\n\nSeems we agree :-)\n\n> > +\n> > +int CameraLens::validateLensDriver()\n> > +{\n> > +       int err = 0;\n> > +       static constexpr uint32_t mandatoryControls[] = {\n> > +               V4L2_CID_FOCUS_ABSOLUTE\n> > +       };\n> > +\n> > +       const ControlInfoMap &controls = subdev_->controls();\n> > +       for (uint32_t ctrl : mandatoryControls) {\n> > +               if (!controls.count(ctrl)) {\n> > +                       LOG(CameraLens, Error)\n> > +                               << \"Mandatory V4L2 control \" << utils::hex(ctrl)\n> > +                               << \" not available\";\n> > +                       err = -EINVAL;\n> > +               }\n> > +       }\n> > +\n> > +       if (err) {\n> > +               LOG(CameraLens, Error)\n> > +                       << \"The lens kernel driver needs to be fixed\";\n> > +               LOG(CameraLens, Error)\n> > +                       << \"See Documentation/lens_driver_requirements.rst in\"\n> > +                       << \" the libcamera sources for more information\";\n> > +               return err;\n> > +       }\n> > +\n> > +       return 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn CameraLens::model()\n> > + * \\brief Retrieve the lens model name\n> > + *\n> > + * The lens model name is a free-formed string that uniquely identifies the\n> > + * lens model.\n> > + *\n> > + * \\return The lens model name\n> > + */\n> > +\n> > +std::string CameraLens::logPrefix() const\n> > +{\n> > +       return \"'\" + entity_->name() + \"'\";\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 563ed861..2100805d 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -5,6 +5,7 @@ libcamera_sources = files([\n> >      'byte_stream_buffer.cpp',\n> >      'camera.cpp',\n> >      'camera_controls.cpp',\n> > +    'camera_lens.cpp',\n> >      'camera_manager.cpp',\n> >      'camera_sensor.cpp',\n> >      'camera_sensor_properties.cpp',","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DF089BDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Nov 2021 05:51:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4247760121;\n\tWed, 24 Nov 2021 06:51:29 +0100 (CET)","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 EC9B060121\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Nov 2021 06:51:27 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 67C50993;\n\tWed, 24 Nov 2021 06:51:27 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UVKs2cvn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637733087;\n\tbh=bDt8IxPCNCxoB9QScbiiKbvlGdbOhq68a0W6LFEIN3g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UVKs2cvnqEoX5fVCR17k9Ov0JEfdXVpUSAK+UlQd5u0GgG8lbpo9qB8Rdn8deJnSw\n\tXrmGdHDL/HrdC4w915zwQPD9Bsd1J+ahYr/IqJbuYXHJjONNxkeW5h/PxwZ5QlEAVy\n\tTxbuZ4pE/e/TT7EtLEE9Cf6xKawjrczdU4B34n+Y=","Date":"Wed, 24 Nov 2021 07:51:04 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YZ3SyFKYjxcfa2TE@pendragon.ideasonboard.com>","References":"<20211123123751.3194696-1-hanlinchen@chromium.org>\n\t<20211123123751.3194696-4-hanlinchen@chromium.org>\n\t<163768744015.3059017.10171555261547304064@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163768744015.3059017.10171555261547304064@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 3/4] libcamera: camera_lens: Add a\n\tnew class to model a camera lens","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21178,"web_url":"https://patchwork.libcamera.org/comment/21178/","msgid":"<f1b8f40b-dc14-3c80-f779-00bf51b3affd@ideasonboard.com>","date":"2021-11-24T06:03:22","subject":"Re: [libcamera-devel] [PATCH v4 3/4] libcamera: camera_lens: Add a\n\tnew class to model a camera lens","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hello,\n\nOn 11/23/21 10:40 PM, Kieran Bingham wrote:\n> Quoting Han-Lin Chen (2021-11-23 12:37:50)\n>> The CameraLens class abstracts camera lens and provides helper\n>> functions to ease interactions with them.\n>>\n>> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>\n>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>> ---\n>>   Documentation/index.rst                    |   1 +\n>>   Documentation/lens_driver_requirements.rst |  28 ++++\n>>   Documentation/meson.build                  |   1 +\n>>   include/libcamera/internal/camera_lens.h   |  48 +++++++\n>>   include/libcamera/internal/meson.build     |   1 +\n>>   src/libcamera/camera_lens.cpp              | 150 +++++++++++++++++++++\n>>   src/libcamera/meson.build                  |   1 +\n>>   7 files changed, 230 insertions(+)\n>>   create mode 100644 Documentation/lens_driver_requirements.rst\n>>   create mode 100644 include/libcamera/internal/camera_lens.h\n>>   create mode 100644 src/libcamera/camera_lens.cpp\n>>\n>> diff --git a/Documentation/index.rst b/Documentation/index.rst\n>> index 1f4fc485..0ee10044 100644\n>> --- a/Documentation/index.rst\n>> +++ b/Documentation/index.rst\n>> @@ -21,3 +21,4 @@\n>>      Tracing guide <guides/tracing>\n>>      Environment variables <environment_variables>\n>>      Sensor driver requirements <sensor_driver_requirements>\n>> +   Lens driver requirements <lens_driver_requirements>\n>> diff --git a/Documentation/lens_driver_requirements.rst b/Documentation/lens_driver_requirements.rst\n>> new file mode 100644\n>> index 00000000..afc300cf\n>> --- /dev/null\n>> +++ b/Documentation/lens_driver_requirements.rst\n>> @@ -0,0 +1,28 @@\n>> +.. SPDX-License-Identifier: CC-BY-SA-4.0\n>> +\n>> +.. _lens-driver-requirements:\n>> +\n>> +Lens Driver Requirements\n>> +========================\n>> +\n>> +libcamera handles lens devices in the CameraLens class and defines\n>> +a consistent interface through its API towards other library components.\n>> +\n>> +The CameraLens class uses the V4L2 subdev kernel API to interface with the\n>> +camera lens through one or multiple sub-devices exposed in userspace by\n>> +the lens driver.\n>> +\n>> +In order for libcamera to be fully operational and provide all the required\n>> +information to interface with the camera lens to applications and pipeline\n>> +handlers, a set of mandatory features the driver has to support has been defined.\n>> +\n>> +Mandatory Requirements\n>> +----------------------\n>> +\n>> +The lens driver is assumed to be fully compliant with the V4L2 specification.\n>> +\n>> +The lens driver shall support the following V4L2 controls:\n>> +\n>> +* `V4L2_CID_FOCUS_ABSOLUTE`_\n>> +\n>> +.. _V4L2_CID_FOCUS_ABSOLUTE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-camera.html\n>> diff --git a/Documentation/meson.build b/Documentation/meson.build\n>> index 4c972675..33af82aa 100644\n>> --- a/Documentation/meson.build\n>> +++ b/Documentation/meson.build\n>> @@ -66,6 +66,7 @@ if sphinx.found()\n>>           'guides/pipeline-handler.rst',\n>>           'guides/tracing.rst',\n>>           'index.rst',\n>> +        'lens_driver_requirements.rst',\n>>           'sensor_driver_requirements.rst',\n>>          '../README.rst',\n>>       ]\n>> diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h\n>> new file mode 100644\n>> index 00000000..fe83d4ad\n>> --- /dev/null\n>> +++ b/include/libcamera/internal/camera_lens.h\n>> @@ -0,0 +1,48 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * camera_lens.h - A camera lens\n>> + */\n>> +#ifndef __LIBCAMERA_INTERNAL_CAMERA_LENS_H__\n>> +#define __LIBCAMERA_INTERNAL_CAMERA_LENS_H__\n>> +\n>> +#include <libcamera/base/class.h>\n>> +#include <libcamera/base/log.h>\n>> +\n>> +#include <libcamera/controls.h>\n>> +\n>> +#include \"libcamera/internal/v4l2_subdevice.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +class MediaEntity;\n>> +\n>> +class CameraLens : protected Loggable\n>> +{\n>> +public:\n>> +       explicit CameraLens(const MediaEntity *entity);\n>> +       ~CameraLens();\n>> +\n>> +       int init();\n>> +       int setFocusPostion(int32_t position);\n>> +\n>> +       const std::string &model() const { return model_; }\n>> +\n>> +protected:\n>> +       std::string logPrefix() const override;\n>> +\n>> +private:\n>> +       LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraLens)\n>> +\n>> +       int validateLensDriver();\n>> +\n>> +       const MediaEntity *entity_;\n>> +       std::unique_ptr<V4L2Subdevice> subdev_;\n>> +\n>> +       std::string model_;\n>> +};\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_INTERNAL_CAMERA_LENS_H__ */\n>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n>> index de8630fe..7e9b9998 100644\n>> --- a/include/libcamera/internal/meson.build\n>> +++ b/include/libcamera/internal/meson.build\n>> @@ -14,6 +14,7 @@ libcamera_internal_headers = files([\n>>       'byte_stream_buffer.h',\n>>       'camera.h',\n>>       'camera_controls.h',\n>> +    'camera_lens.h',\n>>       'camera_sensor.h',\n>>       'camera_sensor_properties.h',\n>>       'camera_utils.h',\n>> diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp\n>> new file mode 100644\n>> index 00000000..cb7159fe\n>> --- /dev/null\n>> +++ b/src/libcamera/camera_lens.cpp\n>> @@ -0,0 +1,150 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * camera_lens.cpp - A camera lens\n>> + */\n>> +\n>> +#include \"libcamera/internal/camera_lens.h\"\n>> +\n>> +#include <regex>\n> I don't think regex is used anymore.\n>\n>> +#include <libcamera/property_ids.h>\n>> +\n>> +#include \"libcamera/internal/camera_utils.h\"\n>> +#include \"libcamera/internal/media_device.h\"\n>> +#include \"libcamera/internal/sysfs.h\"\n> nor sysfs.h...\n>\n>> +\n>> +/**\n>> + * \\file camera_lens.h\n>> + * \\brief A camera lens\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(CameraLens)\n>> +\n>> +/**\n>> + * \\class CameraLens\n>> + * \\brief A camera lens based on V4L2 subdevices\n>> + *\n>> + * The CameraLens class eases handling of lens for pipeline handlers by\n>> + * hiding the details of the V4L2 subdevice kernel API and caching lens\n>> + * information.\n>> + *\n>> + * The implementation is currently limited to lens that expose a single V4L2\n>> + * subdevice. It will be extended to support more complex devices as the needs\n>> + * arise.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a CameraLens\n>> + * \\param[in] entity The media entity backing the camera lens\n>> + *\n>> + * Once constructed the instance must be initialized with init().\n>> + */\n>> +CameraLens::CameraLens(const MediaEntity *entity)\n>> +       : entity_(entity)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Destroy a CameraLens\n>> + */\n>> +CameraLens::~CameraLens() = default;\n>> +\n>> +/**\n>> + * \\brief Initialize the camera lens instance\n>> + *\n>> + * This function performs the initialisation steps of the CameraLens that may\n>> + * fail. It shall be called once and only once after constructing the instance.\n>> + *\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +int CameraLens::init()\n>> +{\n>> +       if (entity_->function() != MEDIA_ENT_F_LENS) {\n>> +               LOG(CameraLens, Error)\n>> +                       << \"Invalid lens function \"\n>> +                       << utils::hex(entity_->function());\n>> +               return -EINVAL;\n>> +       }\n>> +\n>> +       model_ = extractModelFromEntityName(entity_->name());\n>> +\n>> +       /* Create and open the subdev. */\n>> +       subdev_ = std::make_unique<V4L2Subdevice>(entity_);\n>> +       int ret = subdev_->open();\n>> +       if (ret < 0)\n>> +               return ret;\n> This is probably why I think I'd make the model handled by the\n> V4L2Subdevice.\n>\n> We /know/ we're creating a V4L2Subdevice here, so we could assign\n> something like\n>\n> \tmodel_ = subdev_->model();\n>\n> Or simply have model() call directly onto the subdev which would have\n> the implementation...\n>\n>\n>> +\n>> +       ret = validateLensDriver();\n>> +       if (ret)\n>> +               return ret;\n>> +\n>> +       return 0;\n>> +}\n>> +\n>> +/**\n>> + * \\brief This function sets the focal point of the lens to a specific position.\n>> + * \\param[in] position The focal point of the lens\n>> + *\n>> + * This function sets the value of focal point of the lens as in \\a position.\n>> + *\n>> + * \\return 0 on success or -EINVAL otherwise\n>> + */\n>> +int CameraLens::setFocusPostion(int32_t position)\n>> +{\n>> +       ControlList lensCtrls(subdev_->controls());\n>> +       lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, static_cast<int32_t>(position));\n>> +\n>> +       if (subdev_->setControls(&lensCtrls))\n>> +               return -EINVAL;\n>> +\n>> +       return 0;\n>> +}\n> I fear this is going the same way that the TestPatternMode is going on\n> the CameraSensor.\n>\n> I don't think we should expose a function for each control and setting\n> on the devices.\n>\n> The device should accept the controls, parse it and apply it.\n>\n> The setFocusPosition helper function is probably good to keep as it is -\n> but I think a CameraLens should be taking a ControlList (of libcamera\n> controls) and doing something like:\n>\n>\n> +/**\n> + * \\brief Write libcamera controls to the lens\n> + * \\param[in] list The list of controls to write\n> + *\n> + * Supported libcamera controls are interpreted and converted to V4L2\n> + * controls to write to the device.\n> + *\n> + * \\return 0 on success or an error code otherwise\n> + * \\retval -EINVAL One of the control is not supported or not accessible\n> + */\n> +int CameraLens::setControls(ControlList *list)\n> +{\n> +\tif (!list)\n> +\t\treturn -ENODATA;\n> +\n> +\tconst ControlIdMap *idmap = list->idMap();\n> +\tif (idmap != &controls::controls) {\n> +\t\tLOG(CameraLens, Error) << \"Invalid Control List\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tControlList subdevControls;\n> +\n> +\tfor (const auto &ctrl : *list) {\n> +\t\tunsigned int id = ctrl.first;\n> +\t\tconst ControlValue &value = ctrl.second;\n> +\n> +\t\t// switch case would be nicer here.... but isn't\n> +\t\t// currently possible\n> +\n> +\t\t// Also not a libcamera control yet\n> +\t\tif (id == controls::FocusAbsolute) {\n> +\t\t\tint position = value.get<int>();\n> +\t\t\tsubdevControls.set(V4L2_CID_FOCUS_ABSOLUTE, position);\n> +\t\t}\n> +\n> +\t\t// Not a real libcamera control currently\n> +\t\tif (id == controls::FocusRelative) {\n> +\t\t\t// Not implemented, showing other control\n> +\t\t\t// handling only\n> +\t\t}\n> +\t}\n> +\n> +\treturn subdev_->setControls(&subdevControls);\n> +}\n> +\n>\n> I think this is what CameraSensor should be doing too, but we have this\n> awkward precedent that it can already take a V4L2ControlList directly...\n>\n>\n> Given that a lens can only currently set a position anyway, it's not\n> such a big deal if this is considered if/when we need more controls\n> instead, and we keep the direct and simple setting function.\n\n\nI think it's a good design to set controls via a ControlList. It makes \nsense when seen in conjunction with validateLensDriver() below, which \n(will) iterate over multiple controls deemed mandatory. Same should \nfollow for setting controls, I think.\n\nWe can capture the design idea somewhere in the codebase in fact, if we \ndecide to implement it later.\n\n>\n> --\n> Kieran\n>\n>\n>> +\n>> +int CameraLens::validateLensDriver()\n>> +{\n>> +       int err = 0;\n\n\nCan we make this, for simplicity\n\n             int ret = 0;\n\n>> +       static constexpr uint32_t mandatoryControls[] = {\n>> +               V4L2_CID_FOCUS_ABSOLUTE\n\n\ns/V4L2_CID_FOCUS_ABSOLUTE/V4L2_CID_FOCUS_ABSOLUTE,/  to ease off diffs later\n\n>> +       };\n>> +\n>> +       const ControlInfoMap &controls = subdev_->controls();\n>> +       for (uint32_t ctrl : mandatoryControls) {\n>> +               if (!controls.count(ctrl)) {\n>> +                       LOG(CameraLens, Error)\n>> +                               << \"Mandatory V4L2 control \" << utils::hex(ctrl)\n>> +                               << \" not available\";\n>> +                       err = -EINVAL;\n             ret = -EINVAL\n>> +               }\n>> +       }\n>> +\n>> +       if (err) {\n             if (ret <0) {\n>> +               LOG(CameraLens, Error)\n>> +                       << \"The lens kernel driver needs to be fixed\";\n>> +               LOG(CameraLens, Error)\n>> +                       << \"See Documentation/lens_driver_requirements.rst in\"\n>> +                       << \" the libcamera sources for more information\";\n\n\nShould we log this as one Error message instead of two?\n\n>> +               return err;\nremove this line\n>> +       }\n>> +\n>> +       return 0;\n\n         return ret;\n\n>> +}\n>> +\n>> +/**\n>> + * \\fn CameraLens::model()\n>> + * \\brief Retrieve the lens model name\n>> + *\n>> + * The lens model name is a free-formed string that uniquely identifies the\n>> + * lens model.\n>> + *\n>> + * \\return The lens model name\n>> + */\n>> +\n>> +std::string CameraLens::logPrefix() const\n>> +{\n>> +       return \"'\" + entity_->name() + \"'\";\n>> +}\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index 563ed861..2100805d 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -5,6 +5,7 @@ libcamera_sources = files([\n>>       'byte_stream_buffer.cpp',\n>>       'camera.cpp',\n>>       'camera_controls.cpp',\n>> +    'camera_lens.cpp',\n>>       'camera_manager.cpp',\n>>       'camera_sensor.cpp',\n>>       'camera_sensor_properties.cpp',\n>> -- \n>> 2.34.0.rc2.393.gf8c9666880-goog\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 84D38BDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Nov 2021 06:03:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 437AC6022F;\n\tWed, 24 Nov 2021 07:03:30 +0100 (CET)","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 C7A6F60121\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Nov 2021 07:03:28 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.81])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E75E993;\n\tWed, 24 Nov 2021 07:03:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IvKb9TTL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637733808;\n\tbh=uA0VXgGkNyOfsaa/MF5sipyAKPu0+s6k2FpcDy3u2RM=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=IvKb9TTLyWem7wYhsie3QuOIss+uPU7wrbK5IwakQFK9MZ7ikY52Hcxz3TX7fFo3h\n\tctIuGWNnAaHpqgLSsh+REpKiXwFOEgcn9QyRIKFBR8i2IiAae40PehAbUXhDiUNLkd\n\tO9UEIllMh7RTxJJf3iA7HXruKDPnnOSmZyjA7/yc=","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tHan-Lin Chen <hanlinchen@chromium.org>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211123123751.3194696-1-hanlinchen@chromium.org>\n\t<20211123123751.3194696-4-hanlinchen@chromium.org>\n\t<163768744015.3059017.10171555261547304064@Monstersaurus>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<f1b8f40b-dc14-3c80-f779-00bf51b3affd@ideasonboard.com>","Date":"Wed, 24 Nov 2021 11:33:22 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<163768744015.3059017.10171555261547304064@Monstersaurus>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v4 3/4] libcamera: camera_lens: Add a\n\tnew class to model a camera lens","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]