| Message ID | 20200829115429.30010-3-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for the patch. On Sat, Aug 29, 2020 at 12:54:23PM +0100, David Plowman wrote: > The V4L2Device::queryCtrl method simply returns a pointer to the > v4l2_query_ext_ctrl structure for the given control, which has already > been retrieved and stored. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > include/libcamera/internal/v4l2_device.h | 2 ++ > src/libcamera/v4l2_device.cpp | 15 +++++++++++++++ > 2 files changed, 17 insertions(+) > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > index 3b605aa..86dc05c 100644 > --- a/include/libcamera/internal/v4l2_device.h > +++ b/include/libcamera/internal/v4l2_device.h > @@ -29,6 +29,8 @@ public: > ControlList getControls(const std::vector<uint32_t> &ids); > int setControls(ControlList *ctrls); > > + const struct v4l2_query_ext_ctrl *queryControl(uint32_t id); > + > const std::string &deviceNode() const { return deviceNode_; } > std::string devicePath() const; > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 65830d4..af48107 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -353,6 +353,21 @@ int V4L2Device::setControls(ControlList *ctrls) > return ret; > } > > +/** > + * \brief Return the v4l2_query_ext_ctrl information for the given control. s/Return/Retrieve/ > + * \param[in] id The id number of the V4L2 control. Maybe just "The V4L2 control ID" ? > + * \return A pointer to the v4l2_query_ext_ctrl structure for the given > + * control, or a null pointer if not found. No need for trailing . at the end of one-line doxygen tags. > + */ > +const struct v4l2_query_ext_ctrl *V4L2Device::queryControl(uint32_t id) I'd name the function controlInfo() as queryControl() may be considered as implying that it issues a query control ioctl. With these small issues fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > +{ > + const auto it = controlInfo_.find(id); > + if (it == controlInfo_.end()) > + return nullptr; > + > + return &it->second; > +} > + > /** > * \brief Retrieve the device path in sysfs > *
Hello, quickly looking at this patch On Tue, Sep 01, 2020 at 03:47:35AM +0300, Laurent Pinchart wrote: > Hi David, > > Thank you for the patch. > > On Sat, Aug 29, 2020 at 12:54:23PM +0100, David Plowman wrote: > > The V4L2Device::queryCtrl method simply returns a pointer to the > > v4l2_query_ext_ctrl structure for the given control, which has already > > been retrieved and stored. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > include/libcamera/internal/v4l2_device.h | 2 ++ > > src/libcamera/v4l2_device.cpp | 15 +++++++++++++++ > > 2 files changed, 17 insertions(+) > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > index 3b605aa..86dc05c 100644 > > --- a/include/libcamera/internal/v4l2_device.h > > +++ b/include/libcamera/internal/v4l2_device.h > > @@ -29,6 +29,8 @@ public: > > ControlList getControls(const std::vector<uint32_t> &ids); > > int setControls(ControlList *ctrls); > > > > + const struct v4l2_query_ext_ctrl *queryControl(uint32_t id); > > + > > const std::string &deviceNode() const { return deviceNode_; } > > std::string devicePath() const; > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 65830d4..af48107 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -353,6 +353,21 @@ int V4L2Device::setControls(ControlList *ctrls) > > return ret; > > } > > > > +/** > > + * \brief Return the v4l2_query_ext_ctrl information for the given control. > > s/Return/Retrieve/ > > > + * \param[in] id The id number of the V4L2 control. > > Maybe just "The V4L2 control ID" ? > > > + * \return A pointer to the v4l2_query_ext_ctrl structure for the given > > + * control, or a null pointer if not found. > > No need for trailing . at the end of one-line doxygen tags. > > > + */ > > +const struct v4l2_query_ext_ctrl *V4L2Device::queryControl(uint32_t id) Should this be a const & ? > > I'd name the function controlInfo() as queryControl() may be considered > as implying that it issues a query control ioctl. > > With these small issues fixed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > +{ > > + const auto it = controlInfo_.find(id); > > + if (it == controlInfo_.end()) > > + return nullptr; > > + > > + return &it->second; > > +} > > + > > /** > > * \brief Retrieve the device path in sysfs > > * > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Tue, Sep 01, 2020 at 04:00:51PM +0200, Jacopo Mondi wrote: > Hello, > quickly looking at this patch > > On Tue, Sep 01, 2020 at 03:47:35AM +0300, Laurent Pinchart wrote: > > Hi David, > > > > Thank you for the patch. > > > > On Sat, Aug 29, 2020 at 12:54:23PM +0100, David Plowman wrote: > > > The V4L2Device::queryCtrl method simply returns a pointer to the > > > v4l2_query_ext_ctrl structure for the given control, which has already > > > been retrieved and stored. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > include/libcamera/internal/v4l2_device.h | 2 ++ > > > src/libcamera/v4l2_device.cpp | 15 +++++++++++++++ > > > 2 files changed, 17 insertions(+) > > > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > > index 3b605aa..86dc05c 100644 > > > --- a/include/libcamera/internal/v4l2_device.h > > > +++ b/include/libcamera/internal/v4l2_device.h > > > @@ -29,6 +29,8 @@ public: > > > ControlList getControls(const std::vector<uint32_t> &ids); > > > int setControls(ControlList *ctrls); > > > > > > + const struct v4l2_query_ext_ctrl *queryControl(uint32_t id); > > > + > > > const std::string &deviceNode() const { return deviceNode_; } > > > std::string devicePath() const; > > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index 65830d4..af48107 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -353,6 +353,21 @@ int V4L2Device::setControls(ControlList *ctrls) > > > return ret; > > > } > > > > > > +/** > > > + * \brief Return the v4l2_query_ext_ctrl information for the given control. > > > > s/Return/Retrieve/ > > > > > + * \param[in] id The id number of the V4L2 control. > > > > Maybe just "The V4L2 control ID" ? > > > > > + * \return A pointer to the v4l2_query_ext_ctrl structure for the given > > > + * control, or a null pointer if not found. > > > > No need for trailing . at the end of one-line doxygen tags. > > > > > + */ > > > +const struct v4l2_query_ext_ctrl *V4L2Device::queryControl(uint32_t id) > > Should this be a const & ? We wouldn't be able to return nullptr in that case. I think a pointer is appropriate. > > I'd name the function controlInfo() as queryControl() may be considered > > as implying that it issues a query control ioctl. > > > > With these small issues fixed, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > +{ > > > + const auto it = controlInfo_.find(id); > > > + if (it == controlInfo_.end()) > > > + return nullptr; > > > + > > > + return &it->second; > > > +} > > > + > > > /** > > > * \brief Retrieve the device path in sysfs > > > *
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h index 3b605aa..86dc05c 100644 --- a/include/libcamera/internal/v4l2_device.h +++ b/include/libcamera/internal/v4l2_device.h @@ -29,6 +29,8 @@ public: ControlList getControls(const std::vector<uint32_t> &ids); int setControls(ControlList *ctrls); + const struct v4l2_query_ext_ctrl *queryControl(uint32_t id); + const std::string &deviceNode() const { return deviceNode_; } std::string devicePath() const; diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 65830d4..af48107 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -353,6 +353,21 @@ int V4L2Device::setControls(ControlList *ctrls) return ret; } +/** + * \brief Return the v4l2_query_ext_ctrl information for the given control. + * \param[in] id The id number of the V4L2 control. + * \return A pointer to the v4l2_query_ext_ctrl structure for the given + * control, or a null pointer if not found. + */ +const struct v4l2_query_ext_ctrl *V4L2Device::queryControl(uint32_t id) +{ + const auto it = controlInfo_.find(id); + if (it == controlInfo_.end()) + return nullptr; + + return &it->second; +} + /** * \brief Retrieve the device path in sysfs *
The V4L2Device::queryCtrl method simply returns a pointer to the v4l2_query_ext_ctrl structure for the given control, which has already been retrieved and stored. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- include/libcamera/internal/v4l2_device.h | 2 ++ src/libcamera/v4l2_device.cpp | 15 +++++++++++++++ 2 files changed, 17 insertions(+)