| Message ID | 20241114-uvc-roi-v15-6-64cfeb56b6f8@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Implement UVC v1.5 ROI | expand |
Hi, On 14-Nov-24 8:10 PM, Ricardo Ribalda wrote: > Be consistent with uvc_get_le_value() and do the menu translation there. > > Note that in this case, the refactor does not provide much... but > consistency is a nice feature. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 77f7058ec966..c975e0ab479b 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -939,6 +939,8 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping, > int offset = mapping->offset; > u8 mask; > > + if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) > + value = uvc_mapping_get_menu_value(mapping, value); > /* > * According to the v4l2 spec, writing any value to a button control > * should result in the action belonging to the button control being There is a: if (mapping->v4l2_type == V4L2_CTRL_TYPE_BUTTON) below this comment block, maybe put the if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) below that as "else if (mapping->v4l2_type == V4L2_CTRL_TYPE_BUTTON) ... " Or maybe make this a switch-case on mapping->v4l2_type right away for future special handling of other types ? Otherwise this looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > @@ -1988,23 +1990,23 @@ int uvc_ctrl_set(struct uvc_fh *handle, > if (!test_bit(xctrl->value, &mapping->menu_mask)) > return -EINVAL; > > - value = uvc_mapping_get_menu_value(mapping, xctrl->value); > - > /* > * Valid menu indices are reported by the GET_RES request for > * UVC controls that support it. > */ > if (mapping->data_type == UVC_CTRL_DATA_TYPE_BITMASK) { > + int val = uvc_mapping_get_menu_value(mapping, > + xctrl->value); > if (!ctrl->cached) { > ret = uvc_ctrl_populate_cache(chain, ctrl); > if (ret < 0) > return ret; > } > > - if (!(uvc_get_ctrl_bitmap(ctrl, mapping) & value)) > + if (!(uvc_get_ctrl_bitmap(ctrl, mapping) & val)) > return -EINVAL; > } > - > + value = xctrl->value; > break; > > default: >
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 77f7058ec966..c975e0ab479b 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -939,6 +939,8 @@ static void uvc_set_le_value(struct uvc_control_mapping *mapping, int offset = mapping->offset; u8 mask; + if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) + value = uvc_mapping_get_menu_value(mapping, value); /* * According to the v4l2 spec, writing any value to a button control * should result in the action belonging to the button control being @@ -1988,23 +1990,23 @@ int uvc_ctrl_set(struct uvc_fh *handle, if (!test_bit(xctrl->value, &mapping->menu_mask)) return -EINVAL; - value = uvc_mapping_get_menu_value(mapping, xctrl->value); - /* * Valid menu indices are reported by the GET_RES request for * UVC controls that support it. */ if (mapping->data_type == UVC_CTRL_DATA_TYPE_BITMASK) { + int val = uvc_mapping_get_menu_value(mapping, + xctrl->value); if (!ctrl->cached) { ret = uvc_ctrl_populate_cache(chain, ctrl); if (ret < 0) return ret; } - if (!(uvc_get_ctrl_bitmap(ctrl, mapping) & value)) + if (!(uvc_get_ctrl_bitmap(ctrl, mapping) & val)) return -EINVAL; } - + value = xctrl->value; break; default:
Be consistent with uvc_get_le_value() and do the menu translation there. Note that in this case, the refactor does not provide much... but consistency is a nice feature. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)