[{"id":4508,"web_url":"https://patchwork.libcamera.org/comment/4508/","msgid":"<20200425124953.GA10975@pendragon.ideasonboard.com>","date":"2020-04-25T12:49:53","subject":"Re: [libcamera-devel] [PATCH v3 05/13] libcamera: camera_sensor:\n\tBreak out properties initialization","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Apr 24, 2020 at 11:52:56PM +0200, Jacopo Mondi wrote:\n> Refactor the CameraSensor properties initialization to a dedicated\n> function as it is expected to grow as we augment the number of\n> properties.\n> \n> While at it, move documentation of the properties() method to match the\n> declaration order in the class definition.\n> \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Niklas Söderlund \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/camera_sensor.cpp | 99 +++++++++++++++------------\n> src/libcamera/include/camera_sensor.h | 1 +\n> 2 files changed, 55 insertions(+), 45 deletions(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 2219a4307436..8d7abc7147a7 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -91,45 +91,6 @@ int CameraSensor::init()\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\t/* Retrieve and store the camera sensor properties. */\n> -\tconst ControlInfoMap &controls = subdev_->controls();\n> -\tint32_t propertyValue;\n> -\n> -\t/* Camera Location: default is front location. */\n> -\tconst auto &locationControl = controls.find(V4L2_CID_CAMERA_SENSOR_LOCATION);\n> -\tif (locationControl != controls.end()) {\n> -\t\tint32_t v4l2Location =\n> -\t\t\tlocationControl->second.def().get();\n> -\n> -\t\tswitch (v4l2Location) {\n> -\t\tdefault:\n> -\t\t\tLOG(CameraSensor, Warning)\n> -\t\t\t\t<< \"Unsupported camera location \"\n> -\t\t\t\t<< v4l2Location << \", setting to Front\";\n> -\t\t\t/* Fall-through */\n> -\t\tcase V4L2_LOCATION_FRONT:\n> -\t\t\tpropertyValue = properties::CameraLocationFront;\n> -\t\t\tbreak;\n> -\t\tcase V4L2_LOCATION_BACK:\n> -\t\t\tpropertyValue = properties::CameraLocationBack;\n> -\t\t\tbreak;\n> -\t\tcase V4L2_LOCATION_EXTERNAL:\n> -\t\t\tpropertyValue = properties::CameraLocationExternal;\n> -\t\t\tbreak;\n> -\t\t}\n> -\t} else {\n> -\t\tpropertyValue = properties::CameraLocationFront;\n> -\t}\n> -\tproperties_.set(properties::Location, propertyValue);\n> -\n> -\t/* Camera Rotation: default is 0 degrees. */\n> -\tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> -\tif (rotationControl != controls.end())\n> -\t\tpropertyValue = rotationControl->second.def().get();\n> -\telse\n> -\t\tpropertyValue = 0;\n> -\tproperties_.set(properties::Rotation, propertyValue);\n> -\n> \t/* Enumerate and cache media bus codes and sizes. */\n> \tconst ImageFormats formats = subdev_->formats(0);\n> \tif (formats.isEmpty()) {\n> @@ -160,6 +121,54 @@ int CameraSensor::init()\n> \tstd::sort(mbusCodes_.begin(), mbusCodes_.end());\n> \tstd::sort(sizes_.begin(), sizes_.end());\n> \n> +\treturn initProperties();\n\nOut of curiosity, is there a reason to have put the call at the end\ninstead of where the code was removed ?\n\n> +}\n> +\n> +/**\n> + * \\brief Initialize the camera sensor standard properties\n> + *\n> + * This method initializes the camera sensor standard properties, by inspecting\n\ns/,//\n\n> + * the control information reported by the sensor subdevice.\n> + *\n> + * \\return 0 on success, a negative error code otherwise\n> + */\n> +int CameraSensor::initProperties()\n> +{\n> +\tconst ControlInfoMap &controlMap = subdev_->controls();\n> +\tint32_t propertyValue;\n> +\n> +\t/* Camera Location: default is front location. */\n> +\tconst auto &locationControl = controlMap.find(V4L2_CID_CAMERA_SENSOR_LOCATION);\n> +\tif (locationControl != controlMap.end()) {\n> +\t\tint32_t v4l2Location =\n> +\t\t\tlocationControl->second.def().get();\n\nMaybe keep the blank line here for clarity ?\n\n> +\t\tswitch (v4l2Location) {\n> +\t\tcase V4L2_LOCATION_EXTERNAL:\n> +\t\t\tpropertyValue = properties::CameraLocationExternal;\n> +\t\t\tbreak;\n> +\t\tcase V4L2_LOCATION_FRONT:\n> +\t\t\tpropertyValue = properties::CameraLocationFront;\n> +\t\t\tbreak;\n> +\t\tcase V4L2_LOCATION_BACK:\n> +\t\t\tpropertyValue = properties::CameraLocationBack;\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tLOG(CameraSensor, Error)\n> +\t\t\t\t<< \"Unsupported camera location: \" << v4l2Location;\n> +\t\t\treturn -EINVAL;\n\nThis is a behavioural change, as we used to default to\nproperties::CameraLocationFront when the control had an unknown value.\nI'm not opposed to that, but it should be moved to a different patch,\nit's not even mentioned in the commit message. Let's keep patches that\nmove code around free of behavioural changes, that's easier to review.\n\n> +\t\t}\n> +\t} else {\n> +\t\tpropertyValue = properties::CameraLocationFront;\n> +\t}\n> +\tproperties_.set(properties::Location, propertyValue);\n> +\n> +\t/* Camera Rotation: default is 0 degrees. */\n> +\tpropertyValue = 0;\n> +\tconst auto &rotationControl = controlMap.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> +\tif (rotationControl != controlMap.end())\n> +\t\tpropertyValue = rotationControl->second.def().get();\n\nSame here, I'd keep the existing else branch (not a behavioural change\nthough).\n\n> +\tproperties_.set(properties::Rotation, propertyValue);\n> +\n> \treturn 0;\n> }\n> \n> @@ -325,12 +334,6 @@ int CameraSensor::getControls(ControlList *ctrls)\n> \treturn subdev_->getControls(ctrls);\n> }\n> \n> -/**\n> - * \\fn CameraSensor::properties()\n> - * \\brief Retrieve the camera sensor properties\n> - * \\return The list of camera sensor properties\n> - */\n> -\n> /**\n> * \\brief Write controls to the sensor\n> * \\param[in] ctrls The list of controls to write\n> @@ -361,6 +364,12 @@ int CameraSensor::setControls(ControlList *ctrls)\n> \treturn subdev_->setControls(ctrls);\n> }\n> \n> +/**\n> + * \\fn CameraSensor::properties()\n> + * \\brief Retrieve the camera sensor properties\n> + * \\return The list of camera sensor properties\n> + */\n> +\n> std::string CameraSensor::logPrefix() const\n> {\n> \treturn \"'\" + subdev_->entity()->name() + \"'\";\n> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> index 99cff98128dc..8fa4d450f959 100644\n> --- a/src/libcamera/include/camera_sensor.h\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -32,6 +32,7 @@ public:\n> \tCameraSensor &operator=(const CameraSensor &) = delete;\n> \n> \tint init();\n> +\tint initProperties();\n\nShouldn't this be a private function ?\n\nWith these small issues fixed,\n\nReviewed-by: Laurent Pinchart \n\n> \n> \tconst MediaEntity *entity() const { return entity_; }\n> \tconst std::vector &mbusCodes() const { return mbusCodes_; }","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F4167603FB\n\tfor ;\n\tSat, 25 Apr 2020 14:50:08 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 651F94F7;\n\tSat, 25 Apr 2020 14:50:08 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"EKP4ndx1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587819008;\n\tbh=bTXxUtWca4pdOjV9P202lt9G40sj9PzcaWnk8Pi/2lc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EKP4ndx1rcwAAHmsljjXv2yE1L1WJSvXF5RcHIJ//xdYK/n/i4PsL3TjRCsNJRL52\n\tF4f+C4nwRJ7cLyHTrVpyjOjqEggnEeVZnYJ16JTSp0XSIaH6yVzQNrA+yfDvX5FCh3\n\tQPXKWc9fLwq49mocAjQtUPSMlQFHfZO46RKrNHgw=","Date":"Sat, 25 Apr 2020 15:49:53 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425124953.GA10975@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200424215304.558317-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 05/13] libcamera: camera_sensor:\n\tBreak out properties initialization","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","X-List-Received-Date":"Sat, 25 Apr 2020 12:50:09 -0000"}},{"id":4511,"web_url":"https://patchwork.libcamera.org/comment/4511/","msgid":"<20200425134224.dei5m2evso32wgrq@uno.localdomain>","date":"2020-04-25T13:42:24","subject":"Re: [libcamera-devel] [PATCH v3 05/13] libcamera: camera_sensor:\n\tBreak out properties initialization","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Sat, Apr 25, 2020 at 03:49:53PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Apr 24, 2020 at 11:52:56PM +0200, Jacopo Mondi wrote:\n> > Refactor the CameraSensor properties initialization to a dedicated\n> > function as it is expected to grow as we augment the number of\n> > properties.\n> >\n> > While at it, move documentation of the properties() method to match the\n> > declaration order in the class definition.\n> >\n> > Reviewed-by: Kieran Bingham \n> > Reviewed-by: Niklas Söderlund \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/camera_sensor.cpp | 99 +++++++++++++++------------\n> > src/libcamera/include/camera_sensor.h | 1 +\n> > 2 files changed, 55 insertions(+), 45 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 2219a4307436..8d7abc7147a7 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -91,45 +91,6 @@ int CameraSensor::init()\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> >\n> > -\t/* Retrieve and store the camera sensor properties. */\n> > -\tconst ControlInfoMap &controls = subdev_->controls();\n> > -\tint32_t propertyValue;\n> > -\n> > -\t/* Camera Location: default is front location. */\n> > -\tconst auto &locationControl = controls.find(V4L2_CID_CAMERA_SENSOR_LOCATION);\n> > -\tif (locationControl != controls.end()) {\n> > -\t\tint32_t v4l2Location =\n> > -\t\t\tlocationControl->second.def().get();\n> > -\n> > -\t\tswitch (v4l2Location) {\n> > -\t\tdefault:\n> > -\t\t\tLOG(CameraSensor, Warning)\n> > -\t\t\t\t<< \"Unsupported camera location \"\n> > -\t\t\t\t<< v4l2Location << \", setting to Front\";\n> > -\t\t\t/* Fall-through */\n> > -\t\tcase V4L2_LOCATION_FRONT:\n> > -\t\t\tpropertyValue = properties::CameraLocationFront;\n> > -\t\t\tbreak;\n> > -\t\tcase V4L2_LOCATION_BACK:\n> > -\t\t\tpropertyValue = properties::CameraLocationBack;\n> > -\t\t\tbreak;\n> > -\t\tcase V4L2_LOCATION_EXTERNAL:\n> > -\t\t\tpropertyValue = properties::CameraLocationExternal;\n> > -\t\t\tbreak;\n> > -\t\t}\n> > -\t} else {\n> > -\t\tpropertyValue = properties::CameraLocationFront;\n> > -\t}\n> > -\tproperties_.set(properties::Location, propertyValue);\n> > -\n> > -\t/* Camera Rotation: default is 0 degrees. */\n> > -\tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> > -\tif (rotationControl != controls.end())\n> > -\t\tpropertyValue = rotationControl->second.def().get();\n> > -\telse\n> > -\t\tpropertyValue = 0;\n> > -\tproperties_.set(properties::Rotation, propertyValue);\n> > -\n> > \t/* Enumerate and cache media bus codes and sizes. */\n> > \tconst ImageFormats formats = subdev_->formats(0);\n> > \tif (formats.isEmpty()) {\n> > @@ -160,6 +121,54 @@ int CameraSensor::init()\n> > \tstd::sort(mbusCodes_.begin(), mbusCodes_.end());\n> > \tstd::sort(sizes_.begin(), sizes_.end());\n> >\n> > +\treturn initProperties();\n>\n> Out of curiosity, is there a reason to have put the call at the end\n> instead of where the code was removed ?\n>\n\nif we fail to find a suitable format, there is no reason to have\nproperties initialized, so let's first make sure we can produce the\nrequired format, then initialize properties\n\n\n> > +}\n> > +\n> > +/**\n> > + * \\brief Initialize the camera sensor standard properties\n> > + *\n> > + * This method initializes the camera sensor standard properties, by inspecting\n>\n> s/,//\n>\n> > + * the control information reported by the sensor subdevice.\n> > + *\n> > + * \\return 0 on success, a negative error code otherwise\n> > + */\n> > +int CameraSensor::initProperties()\n> > +{\n> > +\tconst ControlInfoMap &controlMap = subdev_->controls();\n> > +\tint32_t propertyValue;\n> > +\n> > +\t/* Camera Location: default is front location. */\n> > +\tconst auto &locationControl = controlMap.find(V4L2_CID_CAMERA_SENSOR_LOCATION);\n> > +\tif (locationControl != controlMap.end()) {\n> > +\t\tint32_t v4l2Location =\n> > +\t\t\tlocationControl->second.def().get();\n>\n> Maybe keep the blank line here for clarity ?\n>\n> > +\t\tswitch (v4l2Location) {\n> > +\t\tcase V4L2_LOCATION_EXTERNAL:\n> > +\t\t\tpropertyValue = properties::CameraLocationExternal;\n> > +\t\t\tbreak;\n> > +\t\tcase V4L2_LOCATION_FRONT:\n> > +\t\t\tpropertyValue = properties::CameraLocationFront;\n> > +\t\t\tbreak;\n> > +\t\tcase V4L2_LOCATION_BACK:\n> > +\t\t\tpropertyValue = properties::CameraLocationBack;\n> > +\t\t\tbreak;\n> > +\t\tdefault:\n> > +\t\t\tLOG(CameraSensor, Error)\n> > +\t\t\t\t<< \"Unsupported camera location: \" << v4l2Location;\n> > +\t\t\treturn -EINVAL;\n>\n> This is a behavioural change, as we used to default to\n> properties::CameraLocationFront when the control had an unknown value.\n> I'm not opposed to that, but it should be moved to a different patch,\n> it's not even mentioned in the commit message. Let's keep patches that\n> move code around free of behavioural changes, that's easier to review.\n\nthis was not intended, I think it got mixed up in between rebases,\ngood catch\n\n>\n> > +\t\t}\n> > +\t} else {\n> > +\t\tpropertyValue = properties::CameraLocationFront;\n> > +\t}\n> > +\tproperties_.set(properties::Location, propertyValue);\n> > +\n> > +\t/* Camera Rotation: default is 0 degrees. */\n> > +\tpropertyValue = 0;\n> > +\tconst auto &rotationControl = controlMap.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> > +\tif (rotationControl != controlMap.end())\n> > +\t\tpropertyValue = rotationControl->second.def().get();\n>\n> Same here, I'd keep the existing else branch (not a behavioural change\n> though).\n>\n\nthis was intended, but I can revert it back\n\n> > +\tproperties_.set(properties::Rotation, propertyValue);\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -325,12 +334,6 @@ int CameraSensor::getControls(ControlList *ctrls)\n> > \treturn subdev_->getControls(ctrls);\n> > }\n> >\n> > -/**\n> > - * \\fn CameraSensor::properties()\n> > - * \\brief Retrieve the camera sensor properties\n> > - * \\return The list of camera sensor properties\n> > - */\n> > -\n> > /**\n> > * \\brief Write controls to the sensor\n> > * \\param[in] ctrls The list of controls to write\n> > @@ -361,6 +364,12 @@ int CameraSensor::setControls(ControlList *ctrls)\n> > \treturn subdev_->setControls(ctrls);\n> > }\n> >\n> > +/**\n> > + * \\fn CameraSensor::properties()\n> > + * \\brief Retrieve the camera sensor properties\n> > + * \\return The list of camera sensor properties\n> > + */\n> > +\n> > std::string CameraSensor::logPrefix() const\n> > {\n> > \treturn \"'\" + subdev_->entity()->name() + \"'\";\n> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > index 99cff98128dc..8fa4d450f959 100644\n> > --- a/src/libcamera/include/camera_sensor.h\n> > +++ b/src/libcamera/include/camera_sensor.h\n> > @@ -32,6 +32,7 @@ public:\n> > \tCameraSensor &operator=(const CameraSensor &) = delete;\n> >\n> > \tint init();\n> > +\tint initProperties();\n>\n> Shouldn't this be a private function ?\n>\n> With these small issues fixed,\n\nIndeed, leftover from when this was a protected virtual\n\nThanks\n j\n\n>\n> Reviewed-by: Laurent Pinchart \n>\n> >\n> > \tconst MediaEntity *entity() const { return entity_; }\n> > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","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 EE7EC603FB\n\tfor ;\n\tSat, 25 Apr 2020 15:39:15 +0200 (CEST)","from uno.localdomain\n\t(host240-55-dynamic.3-87-r.retail.telecomitalia.it [87.3.55.240])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id DEB4C60003;\n\tSat, 25 Apr 2020 13:39:14 +0000 (UTC)"],"X-Originating-IP":"87.3.55.240","Date":"Sat, 25 Apr 2020 15:42:24 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425134224.dei5m2evso32wgrq@uno.localdomain>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-6-jacopo@jmondi.org>\n\t<20200425124953.GA10975@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200425124953.GA10975@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 05/13] libcamera: camera_sensor:\n\tBreak out properties initialization","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","X-List-Received-Date":"Sat, 25 Apr 2020 13:39:16 -0000"}},{"id":4516,"web_url":"https://patchwork.libcamera.org/comment/4516/","msgid":"<20200425165424.GC11157@pendragon.ideasonboard.com>","date":"2020-04-25T16:54:24","subject":"Re: [libcamera-devel] [PATCH v3 05/13] libcamera: camera_sensor:\n\tBreak out properties initialization","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sat, Apr 25, 2020 at 03:42:24PM +0200, Jacopo Mondi wrote:\n> On Sat, Apr 25, 2020 at 03:49:53PM +0300, Laurent Pinchart wrote:\n> > On Fri, Apr 24, 2020 at 11:52:56PM +0200, Jacopo Mondi wrote:\n> > > Refactor the CameraSensor properties initialization to a dedicated\n> > > function as it is expected to grow as we augment the number of\n> > > properties.\n> > >\n> > > While at it, move documentation of the properties() method to match the\n> > > declaration order in the class definition.\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > > Reviewed-by: Niklas Söderlund \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/camera_sensor.cpp | 99 +++++++++++++++------------\n> > > src/libcamera/include/camera_sensor.h | 1 +\n> > > 2 files changed, 55 insertions(+), 45 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > > index 2219a4307436..8d7abc7147a7 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -91,45 +91,6 @@ int CameraSensor::init()\n> > > \tif (ret < 0)\n> > > \t\treturn ret;\n> > >\n> > > -\t/* Retrieve and store the camera sensor properties. */\n> > > -\tconst ControlInfoMap &controls = subdev_->controls();\n> > > -\tint32_t propertyValue;\n> > > -\n> > > -\t/* Camera Location: default is front location. */\n> > > -\tconst auto &locationControl = controls.find(V4L2_CID_CAMERA_SENSOR_LOCATION);\n> > > -\tif (locationControl != controls.end()) {\n> > > -\t\tint32_t v4l2Location =\n> > > -\t\t\tlocationControl->second.def().get();\n> > > -\n> > > -\t\tswitch (v4l2Location) {\n> > > -\t\tdefault:\n> > > -\t\t\tLOG(CameraSensor, Warning)\n> > > -\t\t\t\t<< \"Unsupported camera location \"\n> > > -\t\t\t\t<< v4l2Location << \", setting to Front\";\n> > > -\t\t\t/* Fall-through */\n> > > -\t\tcase V4L2_LOCATION_FRONT:\n> > > -\t\t\tpropertyValue = properties::CameraLocationFront;\n> > > -\t\t\tbreak;\n> > > -\t\tcase V4L2_LOCATION_BACK:\n> > > -\t\t\tpropertyValue = properties::CameraLocationBack;\n> > > -\t\t\tbreak;\n> > > -\t\tcase V4L2_LOCATION_EXTERNAL:\n> > > -\t\t\tpropertyValue = properties::CameraLocationExternal;\n> > > -\t\t\tbreak;\n> > > -\t\t}\n> > > -\t} else {\n> > > -\t\tpropertyValue = properties::CameraLocationFront;\n> > > -\t}\n> > > -\tproperties_.set(properties::Location, propertyValue);\n> > > -\n> > > -\t/* Camera Rotation: default is 0 degrees. */\n> > > -\tconst auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> > > -\tif (rotationControl != controls.end())\n> > > -\t\tpropertyValue = rotationControl->second.def().get();\n> > > -\telse\n> > > -\t\tpropertyValue = 0;\n> > > -\tproperties_.set(properties::Rotation, propertyValue);\n> > > -\n> > > \t/* Enumerate and cache media bus codes and sizes. */\n> > > \tconst ImageFormats formats = subdev_->formats(0);\n> > > \tif (formats.isEmpty()) {\n> > > @@ -160,6 +121,54 @@ int CameraSensor::init()\n> > > \tstd::sort(mbusCodes_.begin(), mbusCodes_.end());\n> > > \tstd::sort(sizes_.begin(), sizes_.end());\n> > >\n> > > +\treturn initProperties();\n> >\n> > Out of curiosity, is there a reason to have put the call at the end\n> > instead of where the code was removed ?\n> \n> if we fail to find a suitable format, there is no reason to have\n> properties initialized, so let's first make sure we can produce the\n> required format, then initialize properties\n> \n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Initialize the camera sensor standard properties\n> > > + *\n> > > + * This method initializes the camera sensor standard properties, by inspecting\n> >\n> > s/,//\n> >\n> > > + * the control information reported by the sensor subdevice.\n> > > + *\n> > > + * \\return 0 on success, a negative error code otherwise\n> > > + */\n> > > +int CameraSensor::initProperties()\n> > > +{\n> > > +\tconst ControlInfoMap &controlMap = subdev_->controls();\n> > > +\tint32_t propertyValue;\n> > > +\n> > > +\t/* Camera Location: default is front location. */\n> > > +\tconst auto &locationControl = controlMap.find(V4L2_CID_CAMERA_SENSOR_LOCATION);\n> > > +\tif (locationControl != controlMap.end()) {\n> > > +\t\tint32_t v4l2Location =\n> > > +\t\t\tlocationControl->second.def().get();\n> >\n> > Maybe keep the blank line here for clarity ?\n> >\n> > > +\t\tswitch (v4l2Location) {\n> > > +\t\tcase V4L2_LOCATION_EXTERNAL:\n> > > +\t\t\tpropertyValue = properties::CameraLocationExternal;\n> > > +\t\t\tbreak;\n> > > +\t\tcase V4L2_LOCATION_FRONT:\n> > > +\t\t\tpropertyValue = properties::CameraLocationFront;\n> > > +\t\t\tbreak;\n> > > +\t\tcase V4L2_LOCATION_BACK:\n> > > +\t\t\tpropertyValue = properties::CameraLocationBack;\n> > > +\t\t\tbreak;\n> > > +\t\tdefault:\n> > > +\t\t\tLOG(CameraSensor, Error)\n> > > +\t\t\t\t<< \"Unsupported camera location: \" << v4l2Location;\n> > > +\t\t\treturn -EINVAL;\n> >\n> > This is a behavioural change, as we used to default to\n> > properties::CameraLocationFront when the control had an unknown value.\n> > I'm not opposed to that, but it should be moved to a different patch,\n> > it's not even mentioned in the commit message. Let's keep patches that\n> > move code around free of behavioural changes, that's easier to review.\n> \n> this was not intended, I think it got mixed up in between rebases,\n> good catch\n> \n> > > +\t\t}\n> > > +\t} else {\n> > > +\t\tpropertyValue = properties::CameraLocationFront;\n> > > +\t}\n> > > +\tproperties_.set(properties::Location, propertyValue);\n> > > +\n> > > +\t/* Camera Rotation: default is 0 degrees. */\n> > > +\tpropertyValue = 0;\n> > > +\tconst auto &rotationControl = controlMap.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> > > +\tif (rotationControl != controlMap.end())\n> > > +\t\tpropertyValue = rotationControl->second.def().get();\n> >\n> > Same here, I'd keep the existing else branch (not a behavioural change\n> > though).\n> \n> this was intended, but I can revert it back\n\nIf that's fine with you, let's minmize the changes in patches that move\ncode, and then perform them on top.\n\n> > > +\tproperties_.set(properties::Rotation, propertyValue);\n> > > +\n> > > \treturn 0;\n> > > }\n> > >\n> > > @@ -325,12 +334,6 @@ int CameraSensor::getControls(ControlList *ctrls)\n> > > \treturn subdev_->getControls(ctrls);\n> > > }\n> > >\n> > > -/**\n> > > - * \\fn CameraSensor::properties()\n> > > - * \\brief Retrieve the camera sensor properties\n> > > - * \\return The list of camera sensor properties\n> > > - */\n> > > -\n> > > /**\n> > > * \\brief Write controls to the sensor\n> > > * \\param[in] ctrls The list of controls to write\n> > > @@ -361,6 +364,12 @@ int CameraSensor::setControls(ControlList *ctrls)\n> > > \treturn subdev_->setControls(ctrls);\n> > > }\n> > >\n> > > +/**\n> > > + * \\fn CameraSensor::properties()\n> > > + * \\brief Retrieve the camera sensor properties\n> > > + * \\return The list of camera sensor properties\n> > > + */\n> > > +\n> > > std::string CameraSensor::logPrefix() const\n> > > {\n> > > \treturn \"'\" + subdev_->entity()->name() + \"'\";\n> > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > > index 99cff98128dc..8fa4d450f959 100644\n> > > --- a/src/libcamera/include/camera_sensor.h\n> > > +++ b/src/libcamera/include/camera_sensor.h\n> > > @@ -32,6 +32,7 @@ public:\n> > > \tCameraSensor &operator=(const CameraSensor &) = delete;\n> > >\n> > > \tint init();\n> > > +\tint initProperties();\n> >\n> > Shouldn't this be a private function ?\n> >\n> > With these small issues fixed,\n> \n> Indeed, leftover from when this was a protected virtual\n> \n> > Reviewed-by: Laurent Pinchart \n> >\n> > >\n> > > \tconst MediaEntity *entity() const { return entity_; }\n> > > \tconst std::vector &mbusCodes() const { return mbusCodes_; }","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 310F5603FC\n\tfor ;\n\tSat, 25 Apr 2020 18:54:39 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C2DC14F7;\n\tSat, 25 Apr 2020 18:54:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jjsVdWlo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587833679;\n\tbh=sEsuRmxtEsBW9WYrw9EVI7T7zcDXuIeNiaiUNGm3QQg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jjsVdWloZsMC5GFWwXcF5CCVkXsdAFKIYHcAUWC7beyXkwUJH9xdaqvhGE4TM2hcs\n\tBZCsi6Tph13BablpIozrIoUiYG3F3witdp5VSbsDpbV1sOJOmAEeq/eWGLRX9kdBYP\n\tjnsOxo0GZTzsSioqThSmJt5lhLmk2MrnB49AH8+o=","Date":"Sat, 25 Apr 2020 19:54:24 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200425165424.GC11157@pendragon.ideasonboard.com>","References":"<20200424215304.558317-1-jacopo@jmondi.org>\n\t<20200424215304.558317-6-jacopo@jmondi.org>\n\t<20200425124953.GA10975@pendragon.ideasonboard.com>\n\t<20200425134224.dei5m2evso32wgrq@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200425134224.dei5m2evso32wgrq@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 05/13] libcamera: camera_sensor:\n\tBreak out properties initialization","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","X-List-Received-Date":"Sat, 25 Apr 2020 16:54:39 -0000"}}]