[libcamera-devel,3/5] libcamera: v4l2_controls: Use DataValue and DataInfo
diff mbox series

Message ID 20190918103424.14536-4-jacopo@jmondi.org
State Superseded
Delegated to: Jacopo Mondi
Headers show
Series
  • libcamera: Control framework backend rework
Related show

Commit Message

Jacopo Mondi Sept. 18, 2019, 10:34 a.m. UTC 
Use DataValue and DataInfo in the V4L2 control handling classes to
maximize code reuse with the libcamera controls classes.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/v4l2_controls.h | 22 ++++--------
 src/libcamera/include/v4l2_device.h   |  1 -
 src/libcamera/v4l2_controls.cpp       | 49 +++++++--------------------
 src/libcamera/v4l2_device.cpp         | 25 ++++++--------
 4 files changed, 30 insertions(+), 67 deletions(-)

--
2.23.0

Comments

Kieran Bingham Sept. 20, 2019, 11:15 a.m. UTC | #1 
Hi Jacopo,

On 18/09/2019 11:34, Jacopo Mondi wrote:
> Use DataValue and DataInfo in the V4L2 control handling classes to
> maximize code reuse with the libcamera controls classes.

This makes me happy :D

This patch makes a subtle change to the way "->type()" is used.

Could that be noted in the commit message please?
(Just so that it's clear)

Something like:

"The control type is now represented by the DataType from the DataValue
rather than the type_ previously stored in the V4L2ControlInfo"

Otherwise, I think this all looks fine.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/v4l2_controls.h | 22 ++++--------
>  src/libcamera/include/v4l2_device.h   |  1 -
>  src/libcamera/v4l2_controls.cpp       | 49 +++++++--------------------
>  src/libcamera/v4l2_device.cpp         | 25 ++++++--------
>  4 files changed, 30 insertions(+), 67 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> index 10b726504951..27b855f3407f 100644
> --- a/src/libcamera/include/v4l2_controls.h
> +++ b/src/libcamera/include/v4l2_controls.h
> @@ -16,29 +16,18 @@
>  #include <linux/v4l2-controls.h>
>  #include <linux/videodev2.h>
> 
> +#include <libcamera/data_value.h>
> +
>  namespace libcamera {
> 
> -class V4L2ControlInfo
> +class V4L2ControlInfo : public DataInfo
>  {
>  public:
>  	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> -
>  	unsigned int id() const { return id_; }
> -	unsigned int type() const { return type_; }
> -	size_t size() const { return size_; }
> -	const std::string &name() const { return name_; }
> -
> -	int64_t min() const { return min_; }
> -	int64_t max() const { return max_; }
> 
>  private:
>  	unsigned int id_;
> -	unsigned int type_;
> -	size_t size_;
> -	std::string name_;
> -
> -	int64_t min_;
> -	int64_t max_;
>  };
> 
>  using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;
> @@ -49,14 +38,15 @@ public:
>  	V4L2Control(unsigned int id, int value = 0)
>  		: id_(id), value_(value) {}
> 
> -	int64_t value() const { return value_; }
> +	DataType type() const { return value_.type(); }

Do you need to expose the type? Aren't they all int64_t here?

Ah - ok I see below we switch from examining the V4L2ControlInfo->type()
to examining the V4L2Control->type().

So this is fine.


> +	int64_t value() const { return value_.getInt64(); }
>  	void setValue(int64_t value) { value_ = value; }
> 
>  	unsigned int id() const { return id_; }
> 
>  private:
>  	unsigned int id_;
> -	int64_t value_;
> +	DataValue value_;
>  };
> 
>  class V4L2ControlList
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 75a52c33d821..3144adc26e36 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -44,7 +44,6 @@ protected:
>  private:
>  	void listControls();
>  	void updateControls(V4L2ControlList *ctrls,
> -			    const V4L2ControlInfo **controlInfo,
>  			    const struct v4l2_ext_control *v4l2Ctrls,
>  			    unsigned int count);
> 
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> index 84258d9954d0..9bc4929cbd76 100644
> --- a/src/libcamera/v4l2_controls.cpp
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -69,13 +69,10 @@ namespace libcamera {
>   * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
>   */
>  V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> +	: DataInfo(DataValue(static_cast<int64_t>(ctrl.minimum)),
> +		   DataValue(static_cast<int64_t>(ctrl.maximum))),
> +	  id_(ctrl.id)
>  {
> -	id_ = ctrl.id;
> -	type_ = ctrl.type;
> -	name_ = static_cast<const char *>(ctrl.name);
> -	size_ = ctrl.elem_size * ctrl.elems;
> -	min_ = ctrl.minimum;
> -	max_ = ctrl.maximum;
>  }
> 
>  /**
> @@ -84,36 +81,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_*

Oh - ok - so the types are subtly different now. But I think it's on a
path towards good reuse of the DataValue, DataInfo classes.


> - * \return The V4L2 control type
> - */
> -
> -/**
> - * \fn V4L2ControlInfo::size()
> - * \brief Retrieve the control value data size (in bytes)
> - * \return The V4L2 control value data size
> - */
> -
> -/**
> - * \fn V4L2ControlInfo::name()
> - * \brief Retrieve the control user readable name
> - * \return The V4L2 control user readable name
> - */
> -
> -/**
> - * \fn V4L2ControlInfo::min()
> - * \brief Retrieve the control minimum value
> - * \return The V4L2 control minimum value
> - */
> -
> -/**
> - * \fn V4L2ControlInfo::max()
> - * \brief Retrieve the control maximum value
> - * \return The V4L2 control maximum value
> - */
> -
>  /**
>   * \typedef V4L2ControlInfoMap
>   * \brief A map of control ID to V4L2ControlInfo
> @@ -134,6 +101,10 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
>   * to be directly used but are instead intended to be grouped in
>   * V4L2ControlList instances, which are then passed as parameters to
>   * V4L2Device::setControls() and V4L2Device::getControls() operations.
> + *
> + * \todo Currently all V4L2Controls are integers. For the sake of keeping the
> + * implementation as simpler as possible treat all values as int64. The value()
> + * and setValue() operations use that single data type for now.
>   */
> 
>  /**
> @@ -143,6 +114,12 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
>   * \param value The control value
>   */
> 
> +/**
> + * \fn V4L2Control::type()
> + * \brief Retrieve the type of the control
> + * \return The control type
> + */
> +
>  /**
>   * \fn V4L2Control::value()
>   * \brief Retrieve the value of the control
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 349bf2d29704..2b7e3b1993ca 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -210,7 +210,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
>  		ret = errorIdx;
>  	}
> 
> -	updateControls(ctrls, controlInfo, v4l2Ctrls, count);
> +	updateControls(ctrls, v4l2Ctrls, count);
> 
>  	return ret;
>  }
> @@ -262,8 +262,8 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
>  		v4l2Ctrls[i].id = info->id();
> 
>  		/* Set the v4l2_ext_control value for the write operation. */
> -		switch (info->type()) {
> -		case V4L2_CTRL_TYPE_INTEGER64:
> +		switch (ctrl->type()) {

Hrm. ... this feels like a hidden change between switching on the info
to the ctrl. I hope that's ok, it probably is.

<second pass edit, Yes I see the change throughout>



> +		case DataTypeInteger64:
>  			v4l2Ctrls[i].value64 = ctrl->value();
>  			break;
>  		default:
> @@ -299,7 +299,7 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
>  		ret = errorIdx;
>  	}
> 
> -	updateControls(ctrls, controlInfo, v4l2Ctrls, count);
> +	updateControls(ctrls, v4l2Ctrls, count);
> 
>  	return ret;
>  }
> @@ -352,8 +352,7 @@ void V4L2Device::listControls()
>  		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
>  			continue;
> 
> -		V4L2ControlInfo info(ctrl);
> -		switch (info.type()) {
> +		switch (ctrl.type) {
>  		case V4L2_CTRL_TYPE_INTEGER:
>  		case V4L2_CTRL_TYPE_BOOLEAN:
>  		case V4L2_CTRL_TYPE_MENU:
> @@ -364,11 +363,12 @@ void V4L2Device::listControls()
>  			break;
>  		/* \todo Support compound controls. */
>  		default:
> -			LOG(V4L2, Debug) << "Control type '" << info.type()
> +			LOG(V4L2, Debug) << "Control type '" << ctrl.type
>  					 << "' not supported";
>  			continue;
>  		}
> 
> +		V4L2ControlInfo info(ctrl);
>  		controls_.emplace(ctrl.id, info);
>  	}
>  }
> @@ -382,25 +382,22 @@ void V4L2Device::listControls()
>   * \param[in] count The number of controls to update
>   */
>  void V4L2Device::updateControls(V4L2ControlList *ctrls,
> -				const V4L2ControlInfo **controlInfo,
>  				const struct v4l2_ext_control *v4l2Ctrls,
>  				unsigned int count)
>  {
>  	for (unsigned int i = 0; i < count; ++i) {
> -		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> -		const V4L2ControlInfo *info = controlInfo[i];
>  		V4L2Control *ctrl = ctrls->getByIndex(i);
> 
> -		switch (info->type()) {
> -		case V4L2_CTRL_TYPE_INTEGER64:
> -			ctrl->setValue(v4l2Ctrl->value64);
> +		switch (ctrl->type()) {
> +		case DataTypeInteger64:
> +			ctrl->setValue(v4l2Ctrls[i].value64);
>  			break;
>  		default:
>  			/*
>  			 * \todo To be changed when support for string and
>  			 * compound controls will be added.
>  			 */
> -			ctrl->setValue(v4l2Ctrl->value);
> +			ctrl->setValue(v4l2Ctrls[i].value);
>  			break;
>  		}
>  	}
> --
> 2.23.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Jacopo Mondi Sept. 23, 2019, 11:07 a.m. UTC | #2 
Hi kieran,

On Fri, Sep 20, 2019 at 12:15:07PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 18/09/2019 11:34, Jacopo Mondi wrote:
> > Use DataValue and DataInfo in the V4L2 control handling classes to
> > maximize code reuse with the libcamera controls classes.
>
> This makes me happy :D
>
> This patch makes a subtle change to the way "->type()" is used.
>
> Could that be noted in the commit message please?
> (Just so that it's clear)
>
> Something like:
>
> "The control type is now represented by the DataType from the DataValue
> rather than the type_ previously stored in the V4L2ControlInfo"
>

Yes, good point. This needs to be tracked as it might require a
certain careful handling when we'll add support for compound types.

> Otherwise, I think this all looks fine.
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Thanks
  j

>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/include/v4l2_controls.h | 22 ++++--------
> >  src/libcamera/include/v4l2_device.h   |  1 -
> >  src/libcamera/v4l2_controls.cpp       | 49 +++++++--------------------
> >  src/libcamera/v4l2_device.cpp         | 25 ++++++--------
> >  4 files changed, 30 insertions(+), 67 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > index 10b726504951..27b855f3407f 100644
> > --- a/src/libcamera/include/v4l2_controls.h
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -16,29 +16,18 @@
> >  #include <linux/v4l2-controls.h>
> >  #include <linux/videodev2.h>
> >
> > +#include <libcamera/data_value.h>
> > +
> >  namespace libcamera {
> >
> > -class V4L2ControlInfo
> > +class V4L2ControlInfo : public DataInfo
> >  {
> >  public:
> >  	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> > -
> >  	unsigned int id() const { return id_; }
> > -	unsigned int type() const { return type_; }
> > -	size_t size() const { return size_; }
> > -	const std::string &name() const { return name_; }
> > -
> > -	int64_t min() const { return min_; }
> > -	int64_t max() const { return max_; }
> >
> >  private:
> >  	unsigned int id_;
> > -	unsigned int type_;
> > -	size_t size_;
> > -	std::string name_;
> > -
> > -	int64_t min_;
> > -	int64_t max_;
> >  };
> >
> >  using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;
> > @@ -49,14 +38,15 @@ public:
> >  	V4L2Control(unsigned int id, int value = 0)
> >  		: id_(id), value_(value) {}
> >
> > -	int64_t value() const { return value_; }
> > +	DataType type() const { return value_.type(); }
>
> Do you need to expose the type? Aren't they all int64_t here?
>
> Ah - ok I see below we switch from examining the V4L2ControlInfo->type()
> to examining the V4L2Control->type().
>
> So this is fine.
>
>
> > +	int64_t value() const { return value_.getInt64(); }
> >  	void setValue(int64_t value) { value_ = value; }
> >
> >  	unsigned int id() const { return id_; }
> >
> >  private:
> >  	unsigned int id_;
> > -	int64_t value_;
> > +	DataValue value_;
> >  };
> >
> >  class V4L2ControlList
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index 75a52c33d821..3144adc26e36 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -44,7 +44,6 @@ protected:
> >  private:
> >  	void listControls();
> >  	void updateControls(V4L2ControlList *ctrls,
> > -			    const V4L2ControlInfo **controlInfo,
> >  			    const struct v4l2_ext_control *v4l2Ctrls,
> >  			    unsigned int count);
> >
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > index 84258d9954d0..9bc4929cbd76 100644
> > --- a/src/libcamera/v4l2_controls.cpp
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -69,13 +69,10 @@ namespace libcamera {
> >   * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> >   */
> >  V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > +	: DataInfo(DataValue(static_cast<int64_t>(ctrl.minimum)),
> > +		   DataValue(static_cast<int64_t>(ctrl.maximum))),
> > +	  id_(ctrl.id)
> >  {
> > -	id_ = ctrl.id;
> > -	type_ = ctrl.type;
> > -	name_ = static_cast<const char *>(ctrl.name);
> > -	size_ = ctrl.elem_size * ctrl.elems;
> > -	min_ = ctrl.minimum;
> > -	max_ = ctrl.maximum;
> >  }
> >
> >  /**
> > @@ -84,36 +81,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_*
>
> Oh - ok - so the types are subtly different now. But I think it's on a
> path towards good reuse of the DataValue, DataInfo classes.
>
>
> > - * \return The V4L2 control type
> > - */
> > -
> > -/**
> > - * \fn V4L2ControlInfo::size()
> > - * \brief Retrieve the control value data size (in bytes)
> > - * \return The V4L2 control value data size
> > - */
> > -
> > -/**
> > - * \fn V4L2ControlInfo::name()
> > - * \brief Retrieve the control user readable name
> > - * \return The V4L2 control user readable name
> > - */
> > -
> > -/**
> > - * \fn V4L2ControlInfo::min()
> > - * \brief Retrieve the control minimum value
> > - * \return The V4L2 control minimum value
> > - */
> > -
> > -/**
> > - * \fn V4L2ControlInfo::max()
> > - * \brief Retrieve the control maximum value
> > - * \return The V4L2 control maximum value
> > - */
> > -
> >  /**
> >   * \typedef V4L2ControlInfoMap
> >   * \brief A map of control ID to V4L2ControlInfo
> > @@ -134,6 +101,10 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> >   * to be directly used but are instead intended to be grouped in
> >   * V4L2ControlList instances, which are then passed as parameters to
> >   * V4L2Device::setControls() and V4L2Device::getControls() operations.
> > + *
> > + * \todo Currently all V4L2Controls are integers. For the sake of keeping the
> > + * implementation as simpler as possible treat all values as int64. The value()
> > + * and setValue() operations use that single data type for now.
> >   */
> >
> >  /**
> > @@ -143,6 +114,12 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> >   * \param value The control value
> >   */
> >
> > +/**
> > + * \fn V4L2Control::type()
> > + * \brief Retrieve the type of the control
> > + * \return The control type
> > + */
> > +
> >  /**
> >   * \fn V4L2Control::value()
> >   * \brief Retrieve the value of the control
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 349bf2d29704..2b7e3b1993ca 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -210,7 +210,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)
> >  		ret = errorIdx;
> >  	}
> >
> > -	updateControls(ctrls, controlInfo, v4l2Ctrls, count);
> > +	updateControls(ctrls, v4l2Ctrls, count);
> >
> >  	return ret;
> >  }
> > @@ -262,8 +262,8 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
> >  		v4l2Ctrls[i].id = info->id();
> >
> >  		/* Set the v4l2_ext_control value for the write operation. */
> > -		switch (info->type()) {
> > -		case V4L2_CTRL_TYPE_INTEGER64:
> > +		switch (ctrl->type()) {
>
> Hrm. ... this feels like a hidden change between switching on the info
> to the ctrl. I hope that's ok, it probably is.
>
> <second pass edit, Yes I see the change throughout>
>
>
>
> > +		case DataTypeInteger64:
> >  			v4l2Ctrls[i].value64 = ctrl->value();
> >  			break;
> >  		default:
> > @@ -299,7 +299,7 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)
> >  		ret = errorIdx;
> >  	}
> >
> > -	updateControls(ctrls, controlInfo, v4l2Ctrls, count);
> > +	updateControls(ctrls, v4l2Ctrls, count);
> >
> >  	return ret;
> >  }
> > @@ -352,8 +352,7 @@ void V4L2Device::listControls()
> >  		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
> >  			continue;
> >
> > -		V4L2ControlInfo info(ctrl);
> > -		switch (info.type()) {
> > +		switch (ctrl.type) {
> >  		case V4L2_CTRL_TYPE_INTEGER:
> >  		case V4L2_CTRL_TYPE_BOOLEAN:
> >  		case V4L2_CTRL_TYPE_MENU:
> > @@ -364,11 +363,12 @@ void V4L2Device::listControls()
> >  			break;
> >  		/* \todo Support compound controls. */
> >  		default:
> > -			LOG(V4L2, Debug) << "Control type '" << info.type()
> > +			LOG(V4L2, Debug) << "Control type '" << ctrl.type
> >  					 << "' not supported";
> >  			continue;
> >  		}
> >
> > +		V4L2ControlInfo info(ctrl);
> >  		controls_.emplace(ctrl.id, info);
> >  	}
> >  }
> > @@ -382,25 +382,22 @@ void V4L2Device::listControls()
> >   * \param[in] count The number of controls to update
> >   */
> >  void V4L2Device::updateControls(V4L2ControlList *ctrls,
> > -				const V4L2ControlInfo **controlInfo,
> >  				const struct v4l2_ext_control *v4l2Ctrls,
> >  				unsigned int count)
> >  {
> >  	for (unsigned int i = 0; i < count; ++i) {
> > -		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> > -		const V4L2ControlInfo *info = controlInfo[i];
> >  		V4L2Control *ctrl = ctrls->getByIndex(i);
> >
> > -		switch (info->type()) {
> > -		case V4L2_CTRL_TYPE_INTEGER64:
> > -			ctrl->setValue(v4l2Ctrl->value64);
> > +		switch (ctrl->type()) {
> > +		case DataTypeInteger64:
> > +			ctrl->setValue(v4l2Ctrls[i].value64);
> >  			break;
> >  		default:
> >  			/*
> >  			 * \todo To be changed when support for string and
> >  			 * compound controls will be added.
> >  			 */
> > -			ctrl->setValue(v4l2Ctrl->value);
> > +			ctrl->setValue(v4l2Ctrls[i].value);
> >  			break;
> >  		}
> >  	}
> > --
> > 2.23.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran

Patch
diff mbox series 

diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
index 10b726504951..27b855f3407f 100644
--- a/src/libcamera/include/v4l2_controls.h
+++ b/src/libcamera/include/v4l2_controls.h
@@ -16,29 +16,18 @@ 
 #include <linux/v4l2-controls.h>
 #include <linux/videodev2.h>

+#include <libcamera/data_value.h>
+
 namespace libcamera {

-class V4L2ControlInfo
+class V4L2ControlInfo : public DataInfo
 {
 public:
 	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
-
 	unsigned int id() const { return id_; }
-	unsigned int type() const { return type_; }
-	size_t size() const { return size_; }
-	const std::string &name() const { return name_; }
-
-	int64_t min() const { return min_; }
-	int64_t max() const { return max_; }

 private:
 	unsigned int id_;
-	unsigned int type_;
-	size_t size_;
-	std::string name_;
-
-	int64_t min_;
-	int64_t max_;
 };

 using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;
@@ -49,14 +38,15 @@  public:
 	V4L2Control(unsigned int id, int value = 0)
 		: id_(id), value_(value) {}

-	int64_t value() const { return value_; }
+	DataType type() const { return value_.type(); }
+	int64_t value() const { return value_.getInt64(); }
 	void setValue(int64_t value) { value_ = value; }

 	unsigned int id() const { return id_; }

 private:
 	unsigned int id_;
-	int64_t value_;
+	DataValue value_;
 };

 class V4L2ControlList
diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
index 75a52c33d821..3144adc26e36 100644
--- a/src/libcamera/include/v4l2_device.h
+++ b/src/libcamera/include/v4l2_device.h
@@ -44,7 +44,6 @@  protected:
 private:
 	void listControls();
 	void updateControls(V4L2ControlList *ctrls,
-			    const V4L2ControlInfo **controlInfo,
 			    const struct v4l2_ext_control *v4l2Ctrls,
 			    unsigned int count);

diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
index 84258d9954d0..9bc4929cbd76 100644
--- a/src/libcamera/v4l2_controls.cpp
+++ b/src/libcamera/v4l2_controls.cpp
@@ -69,13 +69,10 @@  namespace libcamera {
  * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
  */
 V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
+	: DataInfo(DataValue(static_cast<int64_t>(ctrl.minimum)),
+		   DataValue(static_cast<int64_t>(ctrl.maximum))),
+	  id_(ctrl.id)
 {
-	id_ = ctrl.id;
-	type_ = ctrl.type;
-	name_ = static_cast<const char *>(ctrl.name);
-	size_ = ctrl.elem_size * ctrl.elems;
-	min_ = ctrl.minimum;
-	max_ = ctrl.maximum;
 }

 /**
@@ -84,36 +81,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)
- * \return The V4L2 control value data size
- */
-
-/**
- * \fn V4L2ControlInfo::name()
- * \brief Retrieve the control user readable name
- * \return The V4L2 control user readable name
- */
-
-/**
- * \fn V4L2ControlInfo::min()
- * \brief Retrieve the control minimum value
- * \return The V4L2 control minimum value
- */
-
-/**
- * \fn V4L2ControlInfo::max()
- * \brief Retrieve the control maximum value
- * \return The V4L2 control maximum value
- */
-
 /**
  * \typedef V4L2ControlInfoMap
  * \brief A map of control ID to V4L2ControlInfo
@@ -134,6 +101,10 @@  V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
  * to be directly used but are instead intended to be grouped in
  * V4L2ControlList instances, which are then passed as parameters to
  * V4L2Device::setControls() and V4L2Device::getControls() operations.
+ *
+ * \todo Currently all V4L2Controls are integers. For the sake of keeping the
+ * implementation as simpler as possible treat all values as int64. The value()
+ * and setValue() operations use that single data type for now.
  */

 /**
@@ -143,6 +114,12 @@  V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
  * \param value The control value
  */

+/**
+ * \fn V4L2Control::type()
+ * \brief Retrieve the type of the control
+ * \return The control type
+ */
+
 /**
  * \fn V4L2Control::value()
  * \brief Retrieve the value of the control
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 349bf2d29704..2b7e3b1993ca 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -210,7 +210,7 @@  int V4L2Device::getControls(V4L2ControlList *ctrls)
 		ret = errorIdx;
 	}

-	updateControls(ctrls, controlInfo, v4l2Ctrls, count);
+	updateControls(ctrls, v4l2Ctrls, count);

 	return ret;
 }
@@ -262,8 +262,8 @@  int V4L2Device::setControls(V4L2ControlList *ctrls)
 		v4l2Ctrls[i].id = info->id();

 		/* Set the v4l2_ext_control value for the write operation. */
-		switch (info->type()) {
-		case V4L2_CTRL_TYPE_INTEGER64:
+		switch (ctrl->type()) {
+		case DataTypeInteger64:
 			v4l2Ctrls[i].value64 = ctrl->value();
 			break;
 		default:
@@ -299,7 +299,7 @@  int V4L2Device::setControls(V4L2ControlList *ctrls)
 		ret = errorIdx;
 	}

-	updateControls(ctrls, controlInfo, v4l2Ctrls, count);
+	updateControls(ctrls, v4l2Ctrls, count);

 	return ret;
 }
@@ -352,8 +352,7 @@  void V4L2Device::listControls()
 		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
 			continue;

-		V4L2ControlInfo info(ctrl);
-		switch (info.type()) {
+		switch (ctrl.type) {
 		case V4L2_CTRL_TYPE_INTEGER:
 		case V4L2_CTRL_TYPE_BOOLEAN:
 		case V4L2_CTRL_TYPE_MENU:
@@ -364,11 +363,12 @@  void V4L2Device::listControls()
 			break;
 		/* \todo Support compound controls. */
 		default:
-			LOG(V4L2, Debug) << "Control type '" << info.type()
+			LOG(V4L2, Debug) << "Control type '" << ctrl.type
 					 << "' not supported";
 			continue;
 		}

+		V4L2ControlInfo info(ctrl);
 		controls_.emplace(ctrl.id, info);
 	}
 }
@@ -382,25 +382,22 @@  void V4L2Device::listControls()
  * \param[in] count The number of controls to update
  */
 void V4L2Device::updateControls(V4L2ControlList *ctrls,
-				const V4L2ControlInfo **controlInfo,
 				const struct v4l2_ext_control *v4l2Ctrls,
 				unsigned int count)
 {
 	for (unsigned int i = 0; i < count; ++i) {
-		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
-		const V4L2ControlInfo *info = controlInfo[i];
 		V4L2Control *ctrl = ctrls->getByIndex(i);

-		switch (info->type()) {
-		case V4L2_CTRL_TYPE_INTEGER64:
-			ctrl->setValue(v4l2Ctrl->value64);
+		switch (ctrl->type()) {
+		case DataTypeInteger64:
+			ctrl->setValue(v4l2Ctrls[i].value64);
 			break;
 		default:
 			/*
 			 * \todo To be changed when support for string and
 			 * compound controls will be added.
 			 */
-			ctrl->setValue(v4l2Ctrl->value);
+			ctrl->setValue(v4l2Ctrls[i].value);
 			break;
 		}
 	}