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
