diff mbox series

[v10,03/11] media: uvcvideo: introduce __uvc_ctrl_get_std()

Message ID 20221109060621.704531-4-yunkec@google.com (mailing list archive)
State Superseded
Delegated to: Laurent Pinchart
Headers
Series media: Implement UVC v1.5 ROI |

Commit Message

Yunke Cao Nov. 9, 2022, 6:06 a.m. UTC 
  Refactor uvc_ctrl to make adding compound control easier.

Currently uvc_ctrl_get() only work for non-compound controls.
Move the logic into uvc_ctrl_std(), return error for compound
controls.

Signed-off-by: Yunke Cao <yunkec@google.com>
---
Changelog since v9:
- No change.
Changelog since v8:
- No change.
Changelog since v7:
- Newly added patch. Split the refactoring of uvc_ctrl_get from v7 3/7.

 drivers/media/usb/uvc/uvc_ctrl.c | 40 +++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 13 deletions(-)

Comments

Dan Scally Dec. 15, 2022, 9:59 a.m. UTC | #1 
Hi Yunke - thanks for the patches

On 09/11/2022 06:06, Yunke Cao wrote:
> Refactor uvc_ctrl to make adding compound control easier.
>
> Currently uvc_ctrl_get() only work for non-compound controls.
> Move the logic into uvc_ctrl_std(), return error for compound
> controls.


s/uvc_ctrl_std/__uvc_ctrl_std/. This patch does a bit more than the 
commit message outlines, so I think it could do with some fleshing out.

> Signed-off-by: Yunke Cao <yunkec@google.com>
> ---
> Changelog since v9:
> - No change.
> Changelog since v8:
> - No change.
> Changelog since v7:
> - Newly added patch. Split the refactoring of uvc_ctrl_get from v7 3/7.
>
>   drivers/media/usb/uvc/uvc_ctrl.c | 40 +++++++++++++++++++++-----------
>   1 file changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index dfb9d1daece6..93ae7ba5d0cc 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1028,15 +1028,15 @@ static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
>   	return ret;
>   }
>   
> -static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> -			  struct uvc_control *ctrl,
> -			  struct uvc_control_mapping *mapping,
> -			  s32 *value)
> +static int __uvc_ctrl_get_std(struct uvc_video_chain *chain,
> +			      struct uvc_control *ctrl,
> +			      struct uvc_control_mapping *mapping,
> +			      s32 *value)
>   {
>   	int ret;
>   
> -	if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> -		return -EACCES;


Why is this check being dropped here? Won't it still be needed when the 
function's called for non-compound controls?

> +	if (uvc_ctrl_mapping_is_compound(mapping))
> +		return -EINVAL;
>   
>   	ret = __uvc_ctrl_load_cur(chain, ctrl);
>   	if (ret < 0)
> @@ -1153,8 +1153,13 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>   		__uvc_find_control(ctrl->entity, mapping->master_id,
>   				   &master_map, &master_ctrl, 0);
>   	if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> -		s32 val;
> -		int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> +		s32 val = 0;
> +		int ret;
> +
> +		if (uvc_ctrl_mapping_is_compound(master_map))
> +			return -EINVAL;
> +
> +		ret = __uvc_ctrl_get_std(chain, master_ctrl, master_map, &val);
>   		if (ret < 0)
>   			return ret;
>   
> @@ -1399,7 +1404,8 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
>   	if (ctrl == NULL)
>   		return;
>   
> -	if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0)
> +	if (uvc_ctrl_mapping_is_compound(mapping) ||
> +	    __uvc_ctrl_get_std(chain, ctrl, mapping, &val) == 0)
>   		changes |= V4L2_EVENT_CTRL_CH_VALUE;
>   
>   	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
> @@ -1566,7 +1572,8 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
>   		u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
>   		s32 val = 0;
>   
> -		if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
> +		if (uvc_ctrl_mapping_is_compound(mapping) ||
> +		    __uvc_ctrl_get_std(handle->chain, ctrl, mapping, &val) == 0)
>   			changes |= V4L2_EVENT_CTRL_CH_VALUE;
>   
>   		uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, val,
> @@ -1746,7 +1753,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
>   	if (ctrl == NULL)
>   		return -EINVAL;
>   
> -	return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
> +	if (uvc_ctrl_mapping_is_compound(mapping))
> +		return -EINVAL;
> +	else
> +		return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value);
>   }
>   
>   static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain,
> @@ -1893,8 +1903,12 @@ int uvc_ctrl_set(struct uvc_fh *handle,
>   		       ctrl->info.size);
>   	}
>   
> -	mapping->set(mapping, value,
> -		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> +	if (!uvc_ctrl_mapping_is_compound(mapping))
> +		mapping->set(mapping, value,
> +			     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> +	else
> +		return -EINVAL;
> +
>   
>   	if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
>   		ctrl->handle = handle;
Yunke Cao Dec. 16, 2022, 1:28 a.m. UTC | #2 
Hi Dan,

Thanks for the review.

On Thu, Dec 15, 2022 at 6:59 PM Dan Scally <dan.scally@ideasonboard.com> wrote:
>
> Hi Yunke - thanks for the patches
>
> On 09/11/2022 06:06, Yunke Cao wrote:
> > Refactor uvc_ctrl to make adding compound control easier.
> >
> > Currently uvc_ctrl_get() only work for non-compound controls.
> > Move the logic into uvc_ctrl_std(), return error for compound
> > controls.
>
>
> s/uvc_ctrl_std/__uvc_ctrl_std/. This patch does a bit more than the
> commit message outlines, so I think it could do with some fleshing out.
>
> > Signed-off-by: Yunke Cao <yunkec@google.com>
> > ---
> > Changelog since v9:
> > - No change.
> > Changelog since v8:
> > - No change.
> > Changelog since v7:
> > - Newly added patch. Split the refactoring of uvc_ctrl_get from v7 3/7.
> >
> >   drivers/media/usb/uvc/uvc_ctrl.c | 40 +++++++++++++++++++++-----------
> >   1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index dfb9d1daece6..93ae7ba5d0cc 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1028,15 +1028,15 @@ static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> >       return ret;
> >   }
> >
> > -static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > -                       struct uvc_control *ctrl,
> > -                       struct uvc_control_mapping *mapping,
> > -                       s32 *value)
> > +static int __uvc_ctrl_get_std(struct uvc_video_chain *chain,
> > +                           struct uvc_control *ctrl,
> > +                           struct uvc_control_mapping *mapping,
> > +                           s32 *value)
> >   {
> >       int ret;
> >
> > -     if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > -             return -EACCES;
>
>
> Why is this check being dropped here? Won't it still be needed when the
> function's called for non-compound controls?
>

I'm dropping it here because __uvc_ctrl_load_cur() checks the same thing.
I think this is duplicated.

> > +     if (uvc_ctrl_mapping_is_compound(mapping))
> > +             return -EINVAL;
> >
> >       ret = __uvc_ctrl_load_cur(chain, ctrl);
> >       if (ret < 0)
> > @@ -1153,8 +1153,13 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> >               __uvc_find_control(ctrl->entity, mapping->master_id,
> >                                  &master_map, &master_ctrl, 0);
> >       if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> > -             s32 val;
> > -             int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> > +             s32 val = 0;
> > +             int ret;
> > +
> > +             if (uvc_ctrl_mapping_is_compound(master_map))
> > +                     return -EINVAL;
> > +
> > +             ret = __uvc_ctrl_get_std(chain, master_ctrl, master_map, &val);
> >               if (ret < 0)
> >                       return ret;
> >
> > @@ -1399,7 +1404,8 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
> >       if (ctrl == NULL)
> >               return;
> >
> > -     if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0)
> > +     if (uvc_ctrl_mapping_is_compound(mapping) ||
> > +         __uvc_ctrl_get_std(chain, ctrl, mapping, &val) == 0)
> >               changes |= V4L2_EVENT_CTRL_CH_VALUE;
> >
> >       uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
> > @@ -1566,7 +1572,8 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
> >               u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
> >               s32 val = 0;
> >
> > -             if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
> > +             if (uvc_ctrl_mapping_is_compound(mapping) ||
> > +                 __uvc_ctrl_get_std(handle->chain, ctrl, mapping, &val) == 0)
> >                       changes |= V4L2_EVENT_CTRL_CH_VALUE;
> >
> >               uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, val,
> > @@ -1746,7 +1753,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
> >       if (ctrl == NULL)
> >               return -EINVAL;
> >
> > -     return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
> > +     if (uvc_ctrl_mapping_is_compound(mapping))
> > +             return -EINVAL;
> > +     else
> > +             return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value);
> >   }
> >
> >   static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain,
> > @@ -1893,8 +1903,12 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> >                      ctrl->info.size);
> >       }
> >
> > -     mapping->set(mapping, value,
> > -             uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > +     if (!uvc_ctrl_mapping_is_compound(mapping))
> > +             mapping->set(mapping, value,
> > +                          uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > +     else
> > +             return -EINVAL;
> > +
> >
> >       if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> >               ctrl->handle = handle;

Best,
Yunke
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index dfb9d1daece6..93ae7ba5d0cc 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1028,15 +1028,15 @@  static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
 	return ret;
 }
 
-static int __uvc_ctrl_get(struct uvc_video_chain *chain,
-			  struct uvc_control *ctrl,
-			  struct uvc_control_mapping *mapping,
-			  s32 *value)
+static int __uvc_ctrl_get_std(struct uvc_video_chain *chain,
+			      struct uvc_control *ctrl,
+			      struct uvc_control_mapping *mapping,
+			      s32 *value)
 {
 	int ret;
 
-	if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
-		return -EACCES;
+	if (uvc_ctrl_mapping_is_compound(mapping))
+		return -EINVAL;
 
 	ret = __uvc_ctrl_load_cur(chain, ctrl);
 	if (ret < 0)
@@ -1153,8 +1153,13 @@  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 		__uvc_find_control(ctrl->entity, mapping->master_id,
 				   &master_map, &master_ctrl, 0);
 	if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
-		s32 val;
-		int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
+		s32 val = 0;
+		int ret;
+
+		if (uvc_ctrl_mapping_is_compound(master_map))
+			return -EINVAL;
+
+		ret = __uvc_ctrl_get_std(chain, master_ctrl, master_map, &val);
 		if (ret < 0)
 			return ret;
 
@@ -1399,7 +1404,8 @@  static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
 	if (ctrl == NULL)
 		return;
 
-	if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0)
+	if (uvc_ctrl_mapping_is_compound(mapping) ||
+	    __uvc_ctrl_get_std(chain, ctrl, mapping, &val) == 0)
 		changes |= V4L2_EVENT_CTRL_CH_VALUE;
 
 	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
@@ -1566,7 +1572,8 @@  static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
 		u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
 		s32 val = 0;
 
-		if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
+		if (uvc_ctrl_mapping_is_compound(mapping) ||
+		    __uvc_ctrl_get_std(handle->chain, ctrl, mapping, &val) == 0)
 			changes |= V4L2_EVENT_CTRL_CH_VALUE;
 
 		uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, val,
@@ -1746,7 +1753,10 @@  int uvc_ctrl_get(struct uvc_video_chain *chain,
 	if (ctrl == NULL)
 		return -EINVAL;
 
-	return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
+	if (uvc_ctrl_mapping_is_compound(mapping))
+		return -EINVAL;
+	else
+		return __uvc_ctrl_get_std(chain, ctrl, mapping, &xctrl->value);
 }
 
 static int __uvc_ctrl_get_boundary_std(struct uvc_video_chain *chain,
@@ -1893,8 +1903,12 @@  int uvc_ctrl_set(struct uvc_fh *handle,
 		       ctrl->info.size);
 	}
 
-	mapping->set(mapping, value,
-		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
+	if (!uvc_ctrl_mapping_is_compound(mapping))
+		mapping->set(mapping, value,
+			     uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
+	else
+		return -EINVAL;
+
 
 	if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
 		ctrl->handle = handle;