Hi Kieran, On Wednesday, 1 August 2018 13:07:20 EEST Kieran Bingham wrote: > On 01/08/18 01:29, Laurent Pinchart wrote: > > The video control and video streaming interface numbers are needed in > > the UVC gadget userspace stack to reply to UVC requests. They are > > hardcoded to fixed values at the moment, preventing configurations with > > multiple functions. > > > > To fix this, make them dynamically discoverable by userspace through > > read-only configfs attributes in <function>/control/bInterfaceNumber and > > <function>/streaming/bInterfaceNumber respectively. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > --- > > > > drivers/usb/gadget/function/f_uvc.c | 2 + > > drivers/usb/gadget/function/u_uvc.h | 3 ++ > > drivers/usb/gadget/function/uvc_configfs.c | 62 +++++++++++++++++++++++++ > It sounds like this is extending the userspace ABI. > > Do we need to document this anywhere? > Documentation/ABI/testing/configfs-usb-gadget-uvc ? You're right, I'll document it. > > 3 files changed, 67 insertions(+) [snip] > > diff --git a/drivers/usb/gadget/function/uvc_configfs.c > > b/drivers/usb/gadget/function/uvc_configfs.c index > > e019ea317c7a..801117686108 100644 > > --- a/drivers/usb/gadget/function/uvc_configfs.c > > +++ b/drivers/usb/gadget/function/uvc_configfs.c > > @@ -687,8 +687,39 @@ static const struct uvcg_config_group_type > > uvcg_control_class_grp_type = {> > > * control > > */ > > > > +static ssize_t uvcg_default_control_b_interface_number_show( > > + struct config_item *item, char *page) > > +{ > > + struct config_group *group = to_config_group(item); > > + struct mutex *su_mutex = &group->cg_subsys->su_mutex; > > + struct config_item *opts_item; > > + struct f_uvc_opts *opts; > > + int result = 0; > > + > > + mutex_lock(su_mutex); /* for navigating configfs hierarchy */ > > + > > + opts_item = item->ci_parent; > > + opts = to_f_uvc_opts(opts_item); > > + > > + mutex_lock(&opts->lock); > > + result += sprintf(page, "%u\n", opts->control_interface); > > + mutex_unlock(&opts->lock); > > + > > + mutex_unlock(su_mutex); > > + > > + return result; > > +} > > + > > +UVC_ATTR_RO(uvcg_default_control_, b_interface_number, bInterfaceNumber); > > + > > +static struct configfs_attribute *uvcg_default_control_attrs[] = { > > + &uvcg_default_control_attr_b_interface_number, > > + NULL, > > +}; [snip] > > +static ssize_t uvcg_default_streaming_b_interface_number_show( > > + struct config_item *item, char *page) > > +{ > > + struct config_group *group = to_config_group(item); > > + struct mutex *su_mutex = &group->cg_subsys->su_mutex; > > + struct config_item *opts_item; > > + struct f_uvc_opts *opts; > > + int result = 0; > > + > > + mutex_lock(su_mutex); /* for navigating configfs hierarchy */ > > + > > + opts_item = item->ci_parent; > > + opts = to_f_uvc_opts(opts_item); > > + > > + mutex_lock(&opts->lock); > > + result += sprintf(page, "%u\n", opts->streaming_interface); > > + mutex_unlock(&opts->lock); > > + > > + mutex_unlock(su_mutex); > > + > > + return result; > > +} > > That looks like a lot of common boilerplate code copied for each file > which prints a single %u. > > Perhaps we should convert those to a macro - but for now they look fine. Feel free to submit patches :-) Helper functions would be event better than macros to decrease the object size. > > + > > +UVC_ATTR_RO(uvcg_default_streaming_, b_interface_number, > > bInterfaceNumber); > > +static struct configfs_attribute > > *uvcg_default_streaming_attrs[] = { > > + &uvcg_default_streaming_attr_b_interface_number, > > + NULL, > > +}; [snip] -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html