Hi Edgar,
On 05/10/17 10:43, Kieran Bingham wrote:
> Hi Edgar,
>
> On 18/08/17 11:12, Edgar Thier wrote:
>>
>> Use flags the device exposes for UVC controls.
>> This allows the device to define which property flags are set.
>>
>> Since some cameras offer auto-adjustments for properties (e.g. auto-gain),
>> the values of other properties (e.g. gain) can change in the camera.
>> Examining the flags ensures that the driver is aware of such properties.
>>
>> Signed-off-by: Edgar Thier <info@xxxxxxxxxxxxxx>> ---
>> drivers/media/usb/uvc/uvc_ctrl.c | 58 +++++++++++++++++++++++++++-------------
>> 1 file changed, 40 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>> index c2ee6e3..6922c0cb 100644
>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>> @@ -1629,6 +1629,9 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
>> }
>> }
>>
>> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
>> + const struct uvc_control_info *info);
>
> Rather than forward declaring the function ... Could you put the function higher
> in the module please?
>
>> +
>> /*
>> * Query control information (size and flags) for XU controls.
>> */
>> @@ -1659,24 +1662,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
>>
>> info->size = le16_to_cpup((__le16 *)data);
>>
>> - /* Query the control information (GET_INFO) */
>> - ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
>> - info->selector, data, 1);
>> - if (ret < 0) {
>> - uvc_trace(UVC_TRACE_CONTROL,
>> - "GET_INFO failed on control %pUl/%u (%d).\n",
>> - info->entity, info->selector, ret);
>> - goto done;
>> - }
>> -
>> - info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
>> - | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
>> - | (data[0] & UVC_CONTROL_CAP_GET ?
>> - UVC_CTRL_FLAG_GET_CUR : 0)
>> - | (data[0] & UVC_CONTROL_CAP_SET ?
>> - UVC_CTRL_FLAG_SET_CUR : 0)
>> - | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
>> - UVC_CTRL_FLAG_AUTO_UPDATE : 0);
>> + info->flags = uvc_ctrl_get_flags(dev, ctrl, info);
>>
>> uvc_ctrl_fixup_xu_info(dev, ctrl, info);
>>
>> @@ -1884,6 +1870,40 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
>> */
>>
>> /*
>> + * Retrieve flags for a given control
>> + */
>> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl,
>> + const struct uvc_control_info *info)
>> +{
>> + u8 *data;
>> + int ret = 0;
>> + int flags = 0;
>> +
>> + data = kmalloc(2, GFP_KERNEL);
>
> kmalloc seems a bit of an overhead for 2 bytes (only one of which is used).
> Can this use local stack storage?
>
> (Laurent, looks like you originally wrote the code that did that, was there a
> reason for the kmalloc for 2 bytes?)
Aha - OK, Just spoke with Laurent and - yes this is needed, as we can't DMA to
the stack :) - I hadn't realised the 'data' was being DMA'd ..
>> + if (data == NULL)
>> + return -ENOMEM;
>
> This will set the callers 'flags' to -ENOMEM ? Is that desired?
>
> Of course removing the kmalloc will fix that anyway ...
Perhaps we have to return an empty flags here, which is what we will return if
the uvc_query_ctrl() fails anyway.
>> +
>> + ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum,
>> + info->selector, data, 1);
>> + if (ret < 0) {
>> + uvc_trace(UVC_TRACE_CONTROL,
>> + "GET_INFO failed on control %pUl/%u (%d).\n",
>> + info->entity, info->selector, ret);
>> + } else {
>> + flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
>> + | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF
>> + | (data[0] & UVC_CONTROL_CAP_GET ?
>> + UVC_CTRL_FLAG_GET_CUR : 0)
>> + | (data[0] & UVC_CONTROL_CAP_SET ?
>> + UVC_CTRL_FLAG_SET_CUR : 0)
>> + | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
>> + UVC_CTRL_FLAG_AUTO_UPDATE : 0);
>> + }
>> + kfree(data);
>> + return flags;
>> +}
>> +
>> +/*
>> * Add control information to a given control.
>> */
>> static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>> @@ -1902,6 +1922,8 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
>> goto done;
>> }
>>
>> + ctrl->info.flags = uvc_ctrl_get_flags(dev, ctrl, info);
>> +
>> ctrl->initialized = 1;
>>
>> uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
>>
