[libcamera-devel] [PATCH 8/9] libcamera: v4l2_controls: Remove V4L2ControlInfo type field

[Laurent Pinchart]

Hi Jacopo,

On Wed, Oct 09, 2019 at 11:23:50AM +0200, Jacopo Mondi wrote:
> On Tue, Oct 08, 2019 at 01:46:41AM +0300, Laurent Pinchart wrote:
> > The V4L2ControlInfo type field stores the V4L2 control type. It partly
> > duplicates the V4L2ControlInfo id().type() that stores the corresponding
> > libcamera control type. The two fields are not strictly identical, but
> > having two types doesn't provide us with any extra value. As this is
> > confusing, remove the V4L2ControlInfo type field.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/include/v4l2_controls.h |  2 --
> >  src/libcamera/v4l2_controls.cpp       |  7 -------
> >  src/libcamera/v4l2_device.cpp         | 10 +++++-----
> >  3 files changed, 5 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > index 1d85ecf9864a..133ab9ec208b 100644
> > --- a/src/libcamera/include/v4l2_controls.h
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -31,14 +31,12 @@ public:
> >  	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> >
> >  	const ControlId &id() const { return id_; }
> > -	unsigned int type() const { return type_; }
> 
> Have you considered
> 	unsigned int type() const { return id_.type(); }
> ?

I think that would be the worst of both worlds :-) If the type can be
retrieved from id(), I don't think we should create an alias here. It
would be slightly shorter for callers, but quite confusing I think as a
type() method on a V4L2ControlInfo can be expected to return the V4L2
control type.

> Either ways:
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> >  	size_t size() const { return size_; }
> >
> >  	const ControlRange &range() const { return range_; }
> >
> >  private:
> >  	V4L2ControlId id_;
> > -	unsigned int type_;
> >  	size_t size_;
> >
> >  	ControlRange range_;
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > index 98b2b3fe9d0a..d3f94709e821 100644
> > --- a/src/libcamera/v4l2_controls.cpp
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -127,7 +127,6 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
> >  V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> >  	: id_(ctrl)
> >  {
> > -	type_ = ctrl.type;
> >  	size_ = ctrl.elem_size * ctrl.elems;
> >
> >  	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
> > @@ -144,12 +143,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> >   * \return The V4L2 control ID
> >   */
> >
> > -/**
> > - * \fn V4L2ControlInfo::type()
> > - * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_*
> > - * \return The V4L2 control type
> > - */
> > -
> >  /**
> >   * \fn V4L2ControlInfo::size()
> >   * \brief Retrieve the control value data size (in bytes)
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 50616571dcef..b64e5068a671 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -269,8 +269,8 @@ int V4L2Device::setControls(ControlList *ctrls)
> >
> >  		/* Set the v4l2_ext_control value for the write operation. */
> >  		const ControlValue &value = ctrl.second;
> > -		switch (info->type()) {
> > -		case V4L2_CTRL_TYPE_INTEGER64:
> > +		switch (info->id().type()) {
> > +		case ControlTypeInteger64:
> >  			v4l2Ctrls[i].value64 = value.get<int64_t>();
> >  			break;
> >  		default:
> > @@ -278,7 +278,7 @@ int V4L2Device::setControls(ControlList *ctrls)
> >  			 * \todo To be changed when support for string and
> >  			 * compound controls will be added.
> >  			 */
> > -			v4l2Ctrls[i].value64 = value.get<int32_t>();
> > +			v4l2Ctrls[i].value = value.get<int32_t>();
> >  			break;
> >  		}
> >
> > @@ -402,8 +402,8 @@ void V4L2Device::updateControls(ControlList *ctrls,
> >  		const V4L2ControlInfo *info = controlInfo[i];
> >  		ControlValue &value = ctrl.second;
> >
> > -		switch (info->type()) {
> > -		case V4L2_CTRL_TYPE_INTEGER64:
> > +		switch (info->id().type()) {
> > +		case ControlTypeInteger64:
> >  			value.set<int64_t>(v4l2Ctrl->value64);
> >  			break;
> >  		default:

-- 
Regards,

Laurent Pinchart