[2/2] usb: gadget: uvc: configfs: Add bFrameIndex attributes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Joel and Paul,

On Friday, 8 June 2018 22:12:56 EEST Laurent Pinchart wrote:
> From: Paul Elder <paul.elder@ideasonboard.com>
> 
> The UVC frame descriptions are numbered using the descriptor's
> bFrameIndex field. The index is used in UVC requests, and is thus
> needed to handle requests in userspace. Make it dynamically discoverable
> by exposing it in a bFrameIndex configfs attribute of the frame
> config item.
> 
> However, the bFrameIndex value is never set to begin with. Set these
> values when linking the stream class header in the configfs hierarchy,
> and store it in the config item private data.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  drivers/usb/gadget/function/uvc_configfs.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c index
> 0ad6ea57d0b7..882d9963b244 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -1152,6 +1152,8 @@ UVC_ATTR(uvcg_frame_, cname, aname);
> 
>  #define noop_conversion(x) (x)
> 
> +UVCG_FRAME_ATTR_RO(b_frame_index, bFrameIndex, noop_conversion,
> +		noop_conversion, 8);
>  UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
>  		noop_conversion, 8);
>  UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
> @@ -1298,6 +1300,7 @@ static ssize_t
> uvcg_frame_dw_frame_interval_store(struct config_item *item,
> UVC_ATTR(uvcg_frame_, dw_frame_interval, dwFrameInterval);
> 
>  static struct configfs_attribute *uvcg_frame_attrs[] = {
> +	&uvcg_frame_attr_b_frame_index,
>  	&uvcg_frame_attr_bm_capabilities,
>  	&uvcg_frame_attr_w_width,
>  	&uvcg_frame_attr_w_height,
> @@ -2130,12 +2133,15 @@ static int __uvcg_fill_strm(void *priv1, void
> *priv2, void *priv3, int n, sizeof(*frm->dw_frame_interval);
>  		memcpy(*dest, frm->dw_frame_interval, sz);
>  		*dest += sz;
> -		if (frm->fmt_type == UVCG_UNCOMPRESSED)
> +		if (frm->fmt_type == UVCG_UNCOMPRESSED) {
>  			h->bLength = UVC_DT_FRAME_UNCOMPRESSED_SIZE(
>  				frm->frame.b_frame_interval_type);
> -		else if (frm->fmt_type == UVCG_MJPEG)
> +			frm->frame.b_frame_index = n + 1;
> +		} else if (frm->fmt_type == UVCG_MJPEG) {
>  			h->bLength = UVC_DT_FRAME_MJPEG_SIZE(
>  				frm->frame.b_frame_interval_type);
> +			frm->frame.b_frame_index = n + 1;
> +		}
>  	}
>  	break;
>  	}

This approach looks simpler to me than the patch that Joel submitted. However, 
this patch lacks ABI documentation, which is included in Joel's patch.

Joel, as you were first, I propose taking your patch and modifying it with the 
approach followed here, using the UVCG_FRAME_ATTR_RO macro and setting 
b_frame_index in __uvcg_fill_strm(). Would that be OK with you ? Or is there a 
reason why the approach taken in your patch would be better ?