On Mon, 14 Jul 2025 at 16:24, Hans de Goede [off-list ref] wrote:

Hi Ricardo,

On 5-Jun-25 19:53, Ricardo Ribalda wrote:

quoted

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 <redacted>

Thanks, this looks like a nice cleanup.

quoted

---
 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

quoted

+}
+
 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