Create a helper function to query a control. The new function reduces
the number of arguments, calculates the length of the operation and
redirects the operation to the hardware or to the entity private
functions.
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_ctrl.c | 81 ++++++++++++++++++++--------------------
1 file changed, 41 insertions(+), 40 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index b2768080c08aafa85acb9b7f318672c043d84e55..21ec7b978bc7aca21db7cb8fd5d135d876f3330c 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -576,6 +576,34 @@ static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = {
V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
};
+static int uvc_ctrl_query_entity(struct uvc_device *dev,
+ const struct uvc_control *ctrl, u8 query,
+ void *data)
+{
+ u16 len;
+
+ switch (query) {
+ case UVC_GET_INFO:
+ len = 1;
+ break;
+ case UVC_GET_LEN:
+ len = 2;
+ break;
+ default:
+ len = ctrl->info.size;
+ }
+
+ if (query == UVC_GET_CUR && ctrl->entity->get_cur)
+ return ctrl->entity->get_cur(dev, ctrl->entity,
+ ctrl->info.selector, data, len);
+ if (query == UVC_GET_INFO && ctrl->entity->get_info)
+ return ctrl->entity->get_info(dev, ctrl->entity,
+ ctrl->info.selector, data);
+
+ return uvc_query_ctrl(dev, query, ctrl->entity->id, dev->intfnum,
+ ctrl->info.selector, data, len);
+}
+
static const struct uvc_control_mapping *uvc_ctrl_filter_plf_mapping(
struct uvc_video_chain *chain, struct uvc_control *ctrl)
{
@@ -1222,35 +1250,27 @@ static int uvc_ctrl_populate_cache(struct uvc_video_chain *chain,
int ret;
if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) {
- ret = uvc_query_ctrl(chain->dev, UVC_GET_DEF, ctrl->entity->id,
- chain->dev->intfnum, ctrl->info.selector,
- uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF),
- ctrl->info.size);
+ ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_DEF,
+ uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF));
if (ret < 0)
return ret;
}
if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) {
- ret = uvc_query_ctrl(chain->dev, UVC_GET_MIN, ctrl->entity->id,
- chain->dev->intfnum, ctrl->info.selector,
- uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN),
- ctrl->info.size);
+ ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_MIN,
+ uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
if (ret < 0)
return ret;
}
if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) {
- ret = uvc_query_ctrl(chain->dev, UVC_GET_MAX, ctrl->entity->id,
- chain->dev->intfnum, ctrl->info.selector,
- uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX),
- ctrl->info.size);
+ ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_MAX,
+ uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
if (ret < 0)
return ret;
}
if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES) {
- ret = uvc_query_ctrl(chain->dev, UVC_GET_RES, ctrl->entity->id,
- chain->dev->intfnum, ctrl->info.selector,
- uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES),
- ctrl->info.size);
+ ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_RES,
+ uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
if (ret < 0) {
if (UVC_ENTITY_TYPE(ctrl->entity) !=
UVC_VC_EXTENSION_UNIT)
@@ -1291,16 +1311,7 @@ static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
return 0;
}
- if (ctrl->entity->get_cur)
- ret = ctrl->entity->get_cur(chain->dev, ctrl->entity,
- ctrl->info.selector, data,
- ctrl->info.size);
- else
- ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
- ctrl->entity->id, chain->dev->intfnum,
- ctrl->info.selector, data,
- ctrl->info.size);
-
+ ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_CUR, data);
if (ret < 0)
return ret;
@@ -2164,11 +2175,8 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
continue;
if (!rollback)
- ret = uvc_query_ctrl(dev, UVC_SET_CUR, ctrl->entity->id,
- dev->intfnum, ctrl->info.selector,
- uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
- ctrl->info.size);
-
+ ret = uvc_ctrl_query_entity(dev, ctrl, UVC_SET_CUR,
+ uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
if (!ret)
processed_ctrls++;
@@ -2570,13 +2578,7 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
if (data == NULL)
return -ENOMEM;
- if (ctrl->entity->get_info)
- ret = ctrl->entity->get_info(dev, ctrl->entity,
- ctrl->info.selector, data);
- else
- ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id,
- dev->intfnum, info->selector, data, 1);
-
+ ret = uvc_ctrl_query_entity(dev, ctrl, UVC_GET_INFO, data);
if (!ret) {
info->flags &= ~(UVC_CTRL_FLAG_GET_CUR |
UVC_CTRL_FLAG_SET_CUR |
@@ -2654,8 +2656,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
info->selector = ctrl->index + 1;
/* Query and verify the control length (GET_LEN) */
- ret = uvc_query_ctrl(dev, UVC_GET_LEN, ctrl->entity->id, dev->intfnum,
- info->selector, data, 2);
+ ret = uvc_ctrl_query_entity(dev, ctrl, UVC_GET_LEN, data);
if (ret < 0) {
uvc_dbg(dev, CONTROL,
"GET_LEN failed on control %pUl/%u (%d)\n",
--
2.50.0.rc0.642.g800a2b2222-googHi Ricardo,
On 5-Jun-25 19:53, Ricardo Ribalda wrote:
> Create a helper function to query a control. The new function reduces
> the number of arguments, calculates the length of the operation and
> redirects the operation to the hardware or to the entity private
> functions.
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Thanks, this looks like a nice cleanup.
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 81 ++++++++++++++++++++--------------------
> 1 file changed, 41 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index b2768080c08aafa85acb9b7f318672c043d84e55..21ec7b978bc7aca21db7cb8fd5d135d876f3330c 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -576,6 +576,34 @@ static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = {
> V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
> };
>
> +static int uvc_ctrl_query_entity(struct uvc_device *dev,
> + const struct uvc_control *ctrl, u8 query,
> + void *data)
> +{
> + u16 len;
> +
> + switch (query) {
> + case UVC_GET_INFO:
> + len = 1;
> + break;
> + case UVC_GET_LEN:
> + len = 2;
> + break;
> + default:
> + len = ctrl->info.size;
> + }
> +
> + if (query == UVC_GET_CUR && ctrl->entity->get_cur)
> + return ctrl->entity->get_cur(dev, ctrl->entity,
> + ctrl->info.selector, data, len);
> + if (query == UVC_GET_INFO && ctrl->entity->get_info)
> + return ctrl->entity->get_info(dev, ctrl->entity,
> + ctrl->info.selector, data);
> +
> + return uvc_query_ctrl(dev, query, ctrl->entity->id, dev->intfnum,
> + ctrl->info.selector, data, len);
Maybe:
if (query == UVC_GET_CUR && ctrl->entity->get_cur)
return ctrl->entity->get_cur(dev, ctrl->entity,
ctrl->info.selector, data, len);
else if (query == UVC_GET_INFO && ctrl->entity->get_info)
return ctrl->entity->get_info(dev, ctrl->entity,
ctrl->info.selector, data);
else
return uvc_query_ctrl(dev, query, ctrl->entity->id, dev->intfnum,
ctrl->info.selector, data, len);
?
That + Laurent's well observed remark about info->selector vs
ctrl->info.selector which I would probably have missed...
About Laurent's remark about one case of this pre-existing,
please fix this in a separate patch (I guess you would have done so
anyways, but just to be sure).
Regards,
Hans
> +}
> +
> static const struct uvc_control_mapping *uvc_ctrl_filter_plf_mapping(
> struct uvc_video_chain *chain, struct uvc_control *ctrl)
> {
> @@ -1222,35 +1250,27 @@ static int uvc_ctrl_populate_cache(struct uvc_video_chain *chain,
> int ret;
>
> if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) {
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_DEF, ctrl->entity->id,
> - chain->dev->intfnum, ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF),
> - ctrl->info.size);
> + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_DEF,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF));
> if (ret < 0)
> return ret;
> }
>
> if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) {
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_MIN, ctrl->entity->id,
> - chain->dev->intfnum, ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN),
> - ctrl->info.size);
> + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_MIN,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
> if (ret < 0)
> return ret;
> }
> if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) {
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_MAX, ctrl->entity->id,
> - chain->dev->intfnum, ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX),
> - ctrl->info.size);
> + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_MAX,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
> if (ret < 0)
> return ret;
> }
> if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES) {
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_RES, ctrl->entity->id,
> - chain->dev->intfnum, ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES),
> - ctrl->info.size);
> + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_RES,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
> if (ret < 0) {
> if (UVC_ENTITY_TYPE(ctrl->entity) !=
> UVC_VC_EXTENSION_UNIT)
> @@ -1291,16 +1311,7 @@ static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> return 0;
> }
>
> - if (ctrl->entity->get_cur)
> - ret = ctrl->entity->get_cur(chain->dev, ctrl->entity,
> - ctrl->info.selector, data,
> - ctrl->info.size);
> - else
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> - ctrl->entity->id, chain->dev->intfnum,
> - ctrl->info.selector, data,
> - ctrl->info.size);
> -
> + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_CUR, data);
> if (ret < 0)
> return ret;
>
> @@ -2164,11 +2175,8 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> continue;
>
> if (!rollback)
> - ret = uvc_query_ctrl(dev, UVC_SET_CUR, ctrl->entity->id,
> - dev->intfnum, ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> - ctrl->info.size);
> -
> + ret = uvc_ctrl_query_entity(dev, ctrl, UVC_SET_CUR,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> if (!ret)
> processed_ctrls++;
>
> @@ -2570,13 +2578,7 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> if (data == NULL)
> return -ENOMEM;
>
> - if (ctrl->entity->get_info)
> - ret = ctrl->entity->get_info(dev, ctrl->entity,
> - ctrl->info.selector, data);
> - else
> - ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id,
> - dev->intfnum, info->selector, data, 1);
> -
> + ret = uvc_ctrl_query_entity(dev, ctrl, UVC_GET_INFO, data);
> if (!ret) {
> info->flags &= ~(UVC_CTRL_FLAG_GET_CUR |
> UVC_CTRL_FLAG_SET_CUR |
> @@ -2654,8 +2656,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
> info->selector = ctrl->index + 1;
>
> /* Query and verify the control length (GET_LEN) */
> - ret = uvc_query_ctrl(dev, UVC_GET_LEN, ctrl->entity->id, dev->intfnum,
> - info->selector, data, 2);
> + ret = uvc_ctrl_query_entity(dev, ctrl, UVC_GET_LEN, data);
> if (ret < 0) {
> uvc_dbg(dev, CONTROL,
> "GET_LEN failed on control %pUl/%u (%d)\n",
>
On Mon, 14 Jul 2025 at 16:24, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi Ricardo,
>
> On 5-Jun-25 19:53, Ricardo Ribalda wrote:
> > Create a helper function to query a control. The new function reduces
> > the number of arguments, calculates the length of the operation and
> > redirects the operation to the hardware or to the entity private
> > functions.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>
> Thanks, this looks like a nice cleanup.
>
> > ---
> > drivers/media/usb/uvc/uvc_ctrl.c | 81 ++++++++++++++++++++--------------------
> > 1 file changed, 41 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index b2768080c08aafa85acb9b7f318672c043d84e55..21ec7b978bc7aca21db7cb8fd5d135d876f3330c 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -576,6 +576,34 @@ static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = {
> > V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
> > };
> >
> > +static int uvc_ctrl_query_entity(struct uvc_device *dev,
> > + const struct uvc_control *ctrl, u8 query,
> > + void *data)
> > +{
> > + u16 len;
> > +
> > + switch (query) {
> > + case UVC_GET_INFO:
> > + len = 1;
> > + break;
> > + case UVC_GET_LEN:
> > + len = 2;
> > + break;
> > + default:
> > + len = ctrl->info.size;
> > + }
> > +
> > + if (query == UVC_GET_CUR && ctrl->entity->get_cur)
> > + return ctrl->entity->get_cur(dev, ctrl->entity,
> > + ctrl->info.selector, data, len);
> > + if (query == UVC_GET_INFO && ctrl->entity->get_info)
> > + return ctrl->entity->get_info(dev, ctrl->entity,
> > + ctrl->info.selector, data);
> > +
> > + return uvc_query_ctrl(dev, query, ctrl->entity->id, dev->intfnum,
> > + ctrl->info.selector, data, len);
>
> Maybe:
>
> if (query == UVC_GET_CUR && ctrl->entity->get_cur)
> return ctrl->entity->get_cur(dev, ctrl->entity,
> ctrl->info.selector, data, len);
> else if (query == UVC_GET_INFO && ctrl->entity->get_info)
> return ctrl->entity->get_info(dev, ctrl->entity,
> ctrl->info.selector, data);
> else
> return uvc_query_ctrl(dev, query, ctrl->entity->id, dev->intfnum,
> ctrl->info.selector, data, len);
>
> ?
>
> That + Laurent's well observed remark about info->selector vs
> ctrl->info.selector which I would probably have missed...
>
> About Laurent's remark about one case of this pre-existing,
> please fix this in a separate patch (I guess you would have done so
> anyways, but just to be sure).
I have changed the code a bit so we can always rely on
ctrl->info.selector. I have made a small "preparation patch" getting
ready for it. Keep an eye on the next version ;)
There is no need to send a new patch fixing the current code because
the only controls that could have invalid ctrl->info.selector are xu
and software entities are not xu.
Regards!
>
> Regards,
>
> Hans
>
>
>
>
>
> > +}
> > +
> > static const struct uvc_control_mapping *uvc_ctrl_filter_plf_mapping(
> > struct uvc_video_chain *chain, struct uvc_control *ctrl)
> > {
> > @@ -1222,35 +1250,27 @@ static int uvc_ctrl_populate_cache(struct uvc_video_chain *chain,
> > int ret;
> >
> > if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) {
> > - ret = uvc_query_ctrl(chain->dev, UVC_GET_DEF, ctrl->entity->id,
> > - chain->dev->intfnum, ctrl->info.selector,
> > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF),
> > - ctrl->info.size);
> > + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_DEF,
> > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF));
> > if (ret < 0)
> > return ret;
> > }
> >
> > if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) {
> > - ret = uvc_query_ctrl(chain->dev, UVC_GET_MIN, ctrl->entity->id,
> > - chain->dev->intfnum, ctrl->info.selector,
> > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN),
> > - ctrl->info.size);
> > + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_MIN,
> > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
> > if (ret < 0)
> > return ret;
> > }
> > if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) {
> > - ret = uvc_query_ctrl(chain->dev, UVC_GET_MAX, ctrl->entity->id,
> > - chain->dev->intfnum, ctrl->info.selector,
> > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX),
> > - ctrl->info.size);
> > + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_MAX,
> > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
> > if (ret < 0)
> > return ret;
> > }
> > if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES) {
> > - ret = uvc_query_ctrl(chain->dev, UVC_GET_RES, ctrl->entity->id,
> > - chain->dev->intfnum, ctrl->info.selector,
> > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES),
> > - ctrl->info.size);
> > + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_RES,
> > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
> > if (ret < 0) {
> > if (UVC_ENTITY_TYPE(ctrl->entity) !=
> > UVC_VC_EXTENSION_UNIT)
> > @@ -1291,16 +1311,7 @@ static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> > return 0;
> > }
> >
> > - if (ctrl->entity->get_cur)
> > - ret = ctrl->entity->get_cur(chain->dev, ctrl->entity,
> > - ctrl->info.selector, data,
> > - ctrl->info.size);
> > - else
> > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > - ctrl->entity->id, chain->dev->intfnum,
> > - ctrl->info.selector, data,
> > - ctrl->info.size);
> > -
> > + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_CUR, data);
> > if (ret < 0)
> > return ret;
> >
> > @@ -2164,11 +2175,8 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> > continue;
> >
> > if (!rollback)
> > - ret = uvc_query_ctrl(dev, UVC_SET_CUR, ctrl->entity->id,
> > - dev->intfnum, ctrl->info.selector,
> > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > - ctrl->info.size);
> > -
> > + ret = uvc_ctrl_query_entity(dev, ctrl, UVC_SET_CUR,
> > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > if (!ret)
> > processed_ctrls++;
> >
> > @@ -2570,13 +2578,7 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> > if (data == NULL)
> > return -ENOMEM;
> >
> > - if (ctrl->entity->get_info)
> > - ret = ctrl->entity->get_info(dev, ctrl->entity,
> > - ctrl->info.selector, data);
> > - else
> > - ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id,
> > - dev->intfnum, info->selector, data, 1);
> > -
> > + ret = uvc_ctrl_query_entity(dev, ctrl, UVC_GET_INFO, data);
> > if (!ret) {
> > info->flags &= ~(UVC_CTRL_FLAG_GET_CUR |
> > UVC_CTRL_FLAG_SET_CUR |
> > @@ -2654,8 +2656,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
> > info->selector = ctrl->index + 1;
> >
> > /* Query and verify the control length (GET_LEN) */
> > - ret = uvc_query_ctrl(dev, UVC_GET_LEN, ctrl->entity->id, dev->intfnum,
> > - info->selector, data, 2);
> > + ret = uvc_ctrl_query_entity(dev, ctrl, UVC_GET_LEN, data);
> > if (ret < 0) {
> > uvc_dbg(dev, CONTROL,
> > "GET_LEN failed on control %pUl/%u (%d)\n",
> >
>
--
Ricardo Ribalda
Hi Ricardo,
On Thu, Jun 05, 2025 at 05:53:02PM +0000, Ricardo Ribalda wrote:
> Create a helper function to query a control. The new function reduces
> the number of arguments, calculates the length of the operation and
> redirects the operation to the hardware or to the entity private
> functions.
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 81 ++++++++++++++++++++--------------------
> 1 file changed, 41 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index b2768080c08aafa85acb9b7f318672c043d84e55..21ec7b978bc7aca21db7cb8fd5d135d876f3330c 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -576,6 +576,34 @@ static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = {
> V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
> };
>
> +static int uvc_ctrl_query_entity(struct uvc_device *dev,
> + const struct uvc_control *ctrl, u8 query,
> + void *data)
> +{
> + u16 len;
> +
> + switch (query) {
> + case UVC_GET_INFO:
> + len = 1;
> + break;
> + case UVC_GET_LEN:
> + len = 2;
> + break;
> + default:
> + len = ctrl->info.size;
break;
> + }
> +
> + if (query == UVC_GET_CUR && ctrl->entity->get_cur)
> + return ctrl->entity->get_cur(dev, ctrl->entity,
> + ctrl->info.selector, data, len);
> + if (query == UVC_GET_INFO && ctrl->entity->get_info)
> + return ctrl->entity->get_info(dev, ctrl->entity,
> + ctrl->info.selector, data);
> +
> + return uvc_query_ctrl(dev, query, ctrl->entity->id, dev->intfnum,
> + ctrl->info.selector, data, len);
> +}
> +
> static const struct uvc_control_mapping *uvc_ctrl_filter_plf_mapping(
> struct uvc_video_chain *chain, struct uvc_control *ctrl)
> {
> @@ -1222,35 +1250,27 @@ static int uvc_ctrl_populate_cache(struct uvc_video_chain *chain,
> int ret;
>
> if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) {
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_DEF, ctrl->entity->id,
> - chain->dev->intfnum, ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF),
> - ctrl->info.size);
> + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_DEF,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF));
> if (ret < 0)
> return ret;
> }
>
> if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) {
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_MIN, ctrl->entity->id,
> - chain->dev->intfnum, ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN),
> - ctrl->info.size);
> + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_MIN,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN));
> if (ret < 0)
> return ret;
> }
> if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) {
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_MAX, ctrl->entity->id,
> - chain->dev->intfnum, ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX),
> - ctrl->info.size);
> + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_MAX,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX));
> if (ret < 0)
> return ret;
> }
> if (ctrl->info.flags & UVC_CTRL_FLAG_GET_RES) {
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_RES, ctrl->entity->id,
> - chain->dev->intfnum, ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES),
> - ctrl->info.size);
> + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_RES,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES));
> if (ret < 0) {
> if (UVC_ENTITY_TYPE(ctrl->entity) !=
> UVC_VC_EXTENSION_UNIT)
> @@ -1291,16 +1311,7 @@ static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> return 0;
> }
>
> - if (ctrl->entity->get_cur)
> - ret = ctrl->entity->get_cur(chain->dev, ctrl->entity,
> - ctrl->info.selector, data,
> - ctrl->info.size);
> - else
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> - ctrl->entity->id, chain->dev->intfnum,
> - ctrl->info.selector, data,
> - ctrl->info.size);
> -
> + ret = uvc_ctrl_query_entity(chain->dev, ctrl, UVC_GET_CUR, data);
> if (ret < 0)
> return ret;
>
> @@ -2164,11 +2175,8 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> continue;
>
> if (!rollback)
> - ret = uvc_query_ctrl(dev, UVC_SET_CUR, ctrl->entity->id,
> - dev->intfnum, ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> - ctrl->info.size);
> -
> + ret = uvc_ctrl_query_entity(dev, ctrl, UVC_SET_CUR,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> if (!ret)
> processed_ctrls++;
>
> @@ -2570,13 +2578,7 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> if (data == NULL)
> return -ENOMEM;
>
> - if (ctrl->entity->get_info)
> - ret = ctrl->entity->get_info(dev, ctrl->entity,
> - ctrl->info.selector, data);
> - else
> - ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id,
> - dev->intfnum, info->selector, data, 1);
Here ctrl->info isn't filled yet, so replacing info->selector with
ctrl->info.selector won't work. Usage of ctrl->info.selector in the
->get_info() branch looks like a bug.
> -
> + ret = uvc_ctrl_query_entity(dev, ctrl, UVC_GET_INFO, data);
> if (!ret) {
> info->flags &= ~(UVC_CTRL_FLAG_GET_CUR |
> UVC_CTRL_FLAG_SET_CUR |
> @@ -2654,8 +2656,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
> info->selector = ctrl->index + 1;
>
> /* Query and verify the control length (GET_LEN) */
> - ret = uvc_query_ctrl(dev, UVC_GET_LEN, ctrl->entity->id, dev->intfnum,
> - info->selector, data, 2);
> + ret = uvc_ctrl_query_entity(dev, ctrl, UVC_GET_LEN, data);
Same here.
> if (ret < 0) {
> uvc_dbg(dev, CONTROL,
> "GET_LEN failed on control %pUl/%u (%d)\n",
--
Regards,
Laurent Pinchart
© 2016 - 2025 Red Hat, Inc.