[libcamera-devel] [PATCH v5 2/8] libcamera: Allow access to v4l2_query_ext_ctrl structure for a V4L2 control

[Jacopo Mondi]

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 at 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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel