[libcamera-devel,8/9] libcamera: v4l2_controls: Remove V4L2ControlInfo type field
diff mbox series

Message ID 20191007224642.6597-9-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Use ControlList for both libcamera and V4L2 controls
Related show

Commit Message

Laurent Pinchart Oct. 7, 2019, 10:46 p.m. UTC 
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@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(-)

Comments

Jacopo Mondi Oct. 9, 2019, 9:23 a.m. UTC | #1 
Hi Laurent,

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@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(); }
?

Either ways:
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j


>  	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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Oct. 9, 2019, 9:41 a.m. UTC | #2 
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@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@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:

Patch
diff mbox series 

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_; }
 	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: