[{"id":12238,"web_url":"https://patchwork.libcamera.org/comment/12238/","msgid":"<20200901004735.GE16155@pendragon.ideasonboard.com>","date":"2020-09-01T00:47:35","subject":"Re: [libcamera-devel] [PATCH v5 2/8] libcamera: Allow access to\n\tv4l2_query_ext_ctrl structure for a V4L2 control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Sat, Aug 29, 2020 at 12:54:23PM +0100, David Plowman wrote:\n> The V4L2Device::queryCtrl method simply returns a pointer to the\n> v4l2_query_ext_ctrl structure for the given control, which has already\n> been retrieved and stored.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/v4l2_device.h | 2 ++\n> src/libcamera/v4l2_device.cpp | 15 +++++++++++++++\n> 2 files changed, 17 insertions(+)\n> \n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 3b605aa..86dc05c 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -29,6 +29,8 @@ public:\n> \tControlList getControls(const std::vector &ids);\n> \tint setControls(ControlList *ctrls);\n> \n> +\tconst struct v4l2_query_ext_ctrl *queryControl(uint32_t id);\n> +\n> \tconst std::string &deviceNode() const { return deviceNode_; }\n> \tstd::string devicePath() const;\n> \n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 65830d4..af48107 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -353,6 +353,21 @@ int V4L2Device::setControls(ControlList *ctrls)\n> \treturn ret;\n> }\n> \n> +/**\n> + * \\brief Return the v4l2_query_ext_ctrl information for the given control.\n\ns/Return/Retrieve/\n\n> + * \\param[in] id The id number of the V4L2 control.\n\nMaybe just \"The V4L2 control ID\" ?\n\n> + * \\return A pointer to the v4l2_query_ext_ctrl structure for the given\n> + * control, or a null pointer if not found.\n\nNo need for trailing . at the end of one-line doxygen tags.\n\n> + */\n> +const struct v4l2_query_ext_ctrl *V4L2Device::queryControl(uint32_t id)\n\nI'd name the function controlInfo() as queryControl() may be considered\nas implying that it issues a query control ioctl.\n\nWith these small issues fixed,\n\nReviewed-by: Laurent Pinchart \n\n> +{\n> +\tconst auto it = controlInfo_.find(id);\n> +\tif (it == controlInfo_.end())\n> +\t\treturn nullptr;\n> +\n> +\treturn &it->second;\n> +}\n> +\n> /**\n> * \\brief Retrieve the device path in sysfs\n> *","headers":{"Return-Path":"","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 EC308BF019\n\tfor ;\n\tTue, 1 Sep 2020 00:48:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 832DE628D4;\n\tTue, 1 Sep 2020 02:48:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49ACA60375\n\tfor ;\n\tTue, 1 Sep 2020 02:48:16 +0200 (CEST)","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 659D6277;\n\tTue, 1 Sep 2020 02:47:56 +0200 (CEST)"],"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=\"O/s4yVub\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598921276;\n\tbh=8LhsFT1Ei5nuT02jUive6DuwmvNUqIyTNcQcxiiYaxM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=O/s4yVubiwGUSRUMERZ3tzrCRZHJlvJUCl4cKS/F9/5yDuyREKLmN7HaDFff8ONKn\n\tRN/zABbijqKYa6Pm3+J0iYDxyL4DkhH1WwVKV7Ahfp+Ny3juoPHJe2kWYtWRJGenF7\n\t4ZrRf7CX4Hzoe849azpzE2sp5Gy2xRsyBbINRSW8=","Date":"Tue, 1 Sep 2020 03:47:35 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200901004735.GE16155@pendragon.ideasonboard.com>","References":"<20200829115429.30010-1-david.plowman@raspberrypi.com>\n\t<20200829115429.30010-3-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200829115429.30010-3-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v5 2/8] libcamera: Allow access to\n\tv4l2_query_ext_ctrl structure for a V4L2 control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12239,"web_url":"https://patchwork.libcamera.org/comment/12239/","msgid":"<20200901140051.eb5quvd67timwxho@uno.localdomain>","date":"2020-09-01T14:00:51","subject":"Re: [libcamera-devel] [PATCH v5 2/8] libcamera: Allow access to\n\tv4l2_query_ext_ctrl structure for a V4L2 control","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n quickly looking at this patch\n\nOn Tue, Sep 01, 2020 at 03:47:35AM +0300, Laurent Pinchart wrote:\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Sat, Aug 29, 2020 at 12:54:23PM +0100, David Plowman wrote:\n> > The V4L2Device::queryCtrl method simply returns a pointer to the\n> > v4l2_query_ext_ctrl structure for the given control, which has already\n> > been retrieved and stored.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/internal/v4l2_device.h | 2 ++\n> > src/libcamera/v4l2_device.cpp | 15 +++++++++++++++\n> > 2 files changed, 17 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > index 3b605aa..86dc05c 100644\n> > --- a/include/libcamera/internal/v4l2_device.h\n> > +++ b/include/libcamera/internal/v4l2_device.h\n> > @@ -29,6 +29,8 @@ public:\n> > \tControlList getControls(const std::vector &ids);\n> > \tint setControls(ControlList *ctrls);\n> >\n> > +\tconst struct v4l2_query_ext_ctrl *queryControl(uint32_t id);\n> > +\n> > \tconst std::string &deviceNode() const { return deviceNode_; }\n> > \tstd::string devicePath() const;\n> >\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 65830d4..af48107 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -353,6 +353,21 @@ int V4L2Device::setControls(ControlList *ctrls)\n> > \treturn ret;\n> > }\n> >\n> > +/**\n> > + * \\brief Return the v4l2_query_ext_ctrl information for the given control.\n>\n> s/Return/Retrieve/\n>\n> > + * \\param[in] id The id number of the V4L2 control.\n>\n> Maybe just \"The V4L2 control ID\" ?\n>\n> > + * \\return A pointer to the v4l2_query_ext_ctrl structure for the given\n> > + * control, or a null pointer if not found.\n>\n> No need for trailing . at the end of one-line doxygen tags.\n>\n> > + */\n> > +const struct v4l2_query_ext_ctrl *V4L2Device::queryControl(uint32_t id)\n\nShould this be a const & ?\n\n>\n> I'd name the function controlInfo() as queryControl() may be considered\n> as implying that it issues a query control ioctl.\n>\n> With these small issues fixed,\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > +{\n> > +\tconst auto it = controlInfo_.find(id);\n> > +\tif (it == controlInfo_.end())\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn &it->second;\n> > +}\n> > +\n> > /**\n> > * \\brief Retrieve the device path in sysfs\n> > *\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","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 84415BE174\n\tfor ;\n\tTue, 1 Sep 2020 13:57:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13B83628D4;\n\tTue, 1 Sep 2020 15:57:09 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 307D960376\n\tfor ;\n\tTue, 1 Sep 2020 15:57:07 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 43629100016;\n\tTue, 1 Sep 2020 13:57:05 +0000 (UTC)"],"Date":"Tue, 1 Sep 2020 16:00:51 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200901140051.eb5quvd67timwxho@uno.localdomain>","References":"<20200829115429.30010-1-david.plowman@raspberrypi.com>\n\t<20200829115429.30010-3-david.plowman@raspberrypi.com>\n\t<20200901004735.GE16155@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200901004735.GE16155@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 2/8] libcamera: Allow access to\n\tv4l2_query_ext_ctrl structure for a V4L2 control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12240,"web_url":"https://patchwork.libcamera.org/comment/12240/","msgid":"<20200901151200.GA6148@pendragon.ideasonboard.com>","date":"2020-09-01T15:12:00","subject":"Re: [libcamera-devel] [PATCH v5 2/8] libcamera: Allow access to\n\tv4l2_query_ext_ctrl structure for a V4L2 control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Sep 01, 2020 at 04:00:51PM +0200, Jacopo Mondi wrote:\n> Hello,\n> quickly looking at this patch\n> \n> On Tue, Sep 01, 2020 at 03:47:35AM +0300, Laurent Pinchart wrote:\n> > Hi David,\n> >\n> > Thank you for the patch.\n> >\n> > On Sat, Aug 29, 2020 at 12:54:23PM +0100, David Plowman wrote:\n> > > The V4L2Device::queryCtrl method simply returns a pointer to the\n> > > v4l2_query_ext_ctrl structure for the given control, which has already\n> > > been retrieved and stored.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/internal/v4l2_device.h | 2 ++\n> > > src/libcamera/v4l2_device.cpp | 15 +++++++++++++++\n> > > 2 files changed, 17 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > index 3b605aa..86dc05c 100644\n> > > --- a/include/libcamera/internal/v4l2_device.h\n> > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > @@ -29,6 +29,8 @@ public:\n> > > \tControlList getControls(const std::vector &ids);\n> > > \tint setControls(ControlList *ctrls);\n> > >\n> > > +\tconst struct v4l2_query_ext_ctrl *queryControl(uint32_t id);\n> > > +\n> > > \tconst std::string &deviceNode() const { return deviceNode_; }\n> > > \tstd::string devicePath() const;\n> > >\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index 65830d4..af48107 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -353,6 +353,21 @@ int V4L2Device::setControls(ControlList *ctrls)\n> > > \treturn ret;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Return the v4l2_query_ext_ctrl information for the given control.\n> >\n> > s/Return/Retrieve/\n> >\n> > > + * \\param[in] id The id number of the V4L2 control.\n> >\n> > Maybe just \"The V4L2 control ID\" ?\n> >\n> > > + * \\return A pointer to the v4l2_query_ext_ctrl structure for the given\n> > > + * control, or a null pointer if not found.\n> >\n> > No need for trailing . at the end of one-line doxygen tags.\n> >\n> > > + */\n> > > +const struct v4l2_query_ext_ctrl *V4L2Device::queryControl(uint32_t id)\n> \n> Should this be a const & ?\n\nWe wouldn't be able to return nullptr in that case. I think a pointer is\nappropriate.\n\n> > I'd name the function controlInfo() as queryControl() may be considered\n> > as implying that it issues a query control ioctl.\n> >\n> > With these small issues fixed,\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > +{\n> > > +\tconst auto it = controlInfo_.find(id);\n> > > +\tif (it == controlInfo_.end())\n> > > +\t\treturn nullptr;\n> > > +\n> > > +\treturn &it->second;\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Retrieve the device path in sysfs\n> > > *","headers":{"Return-Path":"","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 B798FBE174\n\tfor ;\n\tTue, 1 Sep 2020 15:12:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 34D8C62929;\n\tTue, 1 Sep 2020 17:12:27 +0200 (CEST)","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 2C63A60376\n\tfor ;\n\tTue, 1 Sep 2020 17:12:26 +0200 (CEST)","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 09CE9277;\n\tTue, 1 Sep 2020 17:12:21 +0200 (CEST)"],"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=\"WBuP5NxR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598973142;\n\tbh=EsI7s3Z23aqcIOX6MPlAAXptVApuyHXip/EYmzYWyY4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WBuP5NxRCZIiFe9d4EwAkrRyIiOhfoObu/GCk259xzEmc7IvFUd5Hjy7AP5jQVVIG\n\tc9XwjkgCWcPTOkyAyekLjbIPKb4CLCuWMbTZWWM2Jvuytl+/839Q9sgwZDM/yJRZJs\n\tvATPlIeOBK+Y9hGj4UnIQNuesNSv0+N7kMGA6oXU=","Date":"Tue, 1 Sep 2020 18:12:00 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200901151200.GA6148@pendragon.ideasonboard.com>","References":"<20200829115429.30010-1-david.plowman@raspberrypi.com>\n\t<20200829115429.30010-3-david.plowman@raspberrypi.com>\n\t<20200901004735.GE16155@pendragon.ideasonboard.com>\n\t<20200901140051.eb5quvd67timwxho@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200901140051.eb5quvd67timwxho@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v5 2/8] libcamera: Allow access to\n\tv4l2_query_ext_ctrl structure for a V4L2 control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]