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

[Laurent Pinchart]

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 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 & ?

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 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