| Message ID | 20221121092517.225242-9-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add XU support to UVC Gadget | expand |
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@ideasonboard.com> > --- > 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. > + > + 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) \
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@ideasonboard.com> > > --- > > 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) \
On Thu, Dec 29, 2022 at 04:08:11AM +0200, Laurent Pinchart wrote: > 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. Absolutely there should be support for arbitrary strings in the gadget configfs core. After all, it already supports strings for the vendor and product names. Alan Stern
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; + + 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) \
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@ideasonboard.com> --- 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(+)