Hi Dan,
Thank you for the patch.
On Mon, Nov 21, 2022 at 09:57:38AM +0000, Dan Scally wrote:
> Hi all - apologies, meant to add this discussion point before sending
>
> On 21/11/2022 09:25, Daniel Scally wrote:
> > Currently the string descriptors for the IAD, VideoControl Interface
> > and VideoStreaming Interfaces are hardcoded into f_uvc. Now that we
> > can create arbitrary string descriptors, add a mechanism to define
> > string descriptors for the IAD, VC and VS interfaces by linking to
> > the appropriate directory at function level.
> >
> > Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> >
> > - New patch
> >
> > drivers/usb/gadget/function/u_uvc.h | 8 +++
> > drivers/usb/gadget/function/uvc_configfs.c | 59 ++++++++++++++++++++++
> > 2 files changed, 67 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/function/u_uvc.h b/drivers/usb/gadget/function/u_uvc.h
> > index c1c9ea5931d3..331cdf5ba9c8 100644
> > --- a/drivers/usb/gadget/function/u_uvc.h
> > +++ b/drivers/usb/gadget/function/u_uvc.h
> > @@ -88,6 +88,14 @@ struct f_uvc_opts {
> > struct list_head languages;
> > unsigned int nlangs;
> >
> > + /*
> > + * Indexes into the function's string descriptors allowing users to set
> > + * custom descriptions rather than the hard-coded defaults.
> > + */
> > + u8 iad_index;
> > + u8 vs0_index;
> > + u8 vs1_index;
> > +
> > /*
> > * Read/write access to configfs attributes is handled by configfs.
> > *
> > diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> > index 5c51862150c4..e8faa31998fa 100644
> > --- a/drivers/usb/gadget/function/uvc_configfs.c
> > +++ b/drivers/usb/gadget/function/uvc_configfs.c
> > @@ -3197,8 +3197,67 @@ static void uvc_func_item_release(struct config_item *item)
> > usb_put_function_instance(&opts->func_inst);
> > }
> >
> > +static int uvc_func_allow_link(struct config_item *src, struct config_item *tgt)
> > +{
> > + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> > + struct config_item *strings;
> > + struct uvcg_string *string;
> > + struct f_uvc_opts *opts;
> > + int ret = 0;
> > +
> > + mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> > +
> > + /* Validate that the target is an entry in strings/<langid> */
> > + strings = config_group_find_item(to_config_group(src), "strings");
> > + if (!strings || tgt->ci_parent->ci_parent != strings) {
> > + ret = -EINVAL;
> > + goto put_strings;
> > + }
> > +
> > + string = to_uvcg_string(tgt);
> > +
> > + opts = to_f_uvc_opts(src);
> > + mutex_lock(&opts->lock);
> > +
> > + if (!strcmp(tgt->ci_name, "iad_desc"))
> > + opts->iad_index = string->usb_string.id;
> > + else if (!strcmp(tgt->ci_name, "vs0_desc"))
> > + opts->vs0_index = string->usb_string.id;
> > + else if (!strcmp(tgt->ci_name, "vs1_desc"))
> > + opts->vs1_index = string->usb_string.id;
> > + else
> > + ret = -EINVAL;
>
> Is this reliance on the name of the target to set the right internal
> index an acceptable method? I wasn't quite sure, but it seemed like the
> simplest way to do it.
I haven't dug in the USB gadget configfs implementation, but I think
string support is something that shouldn't be specific to the UVC
gadget. I think we should be able to create a config item of "type"
string, and have gadget helpers handle the rest.
Feedback from the USB gadget maintainers would be useful.
> > +
> > + mutex_unlock(&opts->lock);
> > +
> > +put_strings:
> > + config_item_put(strings);
> > + mutex_unlock(su_mutex);
> > +
> > + return ret;
> > +}
> > +
> > +static void uvc_func_drop_link(struct config_item *src, struct config_item *tgt)
> > +{
> > + struct f_uvc_opts *opts;
> > +
> > + opts = to_f_uvc_opts(src);
> > + mutex_lock(&opts->lock);
> > +
> > + if (!strcmp(tgt->ci_name, "iad_desc"))
> > + opts->iad_index = 0;
> > + else if (!strcmp(tgt->ci_name, "vs0_desc"))
> > + opts->vs0_index = 0;
> > + else if (!strcmp(tgt->ci_name, "vs1_desc"))
> > + opts->vs1_index = 0;
> > +
> > + mutex_unlock(&opts->lock);
> > +}
> > +
> > static struct configfs_item_operations uvc_func_item_ops = {
> > .release = uvc_func_item_release,
> > + .allow_link = uvc_func_allow_link,
> > + .drop_link = uvc_func_drop_link,
> > };
> >
> > #define UVCG_OPTS_ATTR(cname, aname, limit) \
--
Regards,
Laurent Pinchart
