Hi Dan,
On Sun, Jan 01, 2023 at 09:09:43PM +0000, Dan Scally wrote:
> On 29/12/2022 02:05, Laurent Pinchart wrote:
> > On Mon, Nov 21, 2022 at 09:25:14AM +0000, Daniel Scally wrote:
> >> Add .allow_link() and .drop_link() callbacks to allow users to link
> >> an extension unit descriptor to a string descriptor.
> >
> > A link seems weird to me for this. Wouldn't it be better to store the
> > name in uvcg_extension in a similar way that device or config strings
> > are handled in drievrs/usb/gadget/configfs.c ?
>
> I think it's _easier_ that way but it conceptually makes more sense to
> me like this. The primary problem I had with that method though is that
> I couldn't see a good way to specify the language, if we do it that way.
> Or do we just say we only support US English?
Good question. As mentioned in a separate e-mail, I think string
handling should be implemented in core gadget configfs helpers. I hope
I'll be able to use my free get out of jail card and defer the decision
on how to implement it to the USB gadget maintainers :-)
> >> Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> >> ---
> >> Changes in v2:
> >>
> >> - New patch
> >>
> >> drivers/usb/gadget/function/uvc_configfs.c | 60 ++++++++++++++++++++++
> >> drivers/usb/gadget/function/uvc_configfs.h | 1 +
> >> 2 files changed, 61 insertions(+)
> >>
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> >> index da2f70036993..5c51862150c4 100644
> >> --- a/drivers/usb/gadget/function/uvc_configfs.c
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >> @@ -1053,8 +1053,68 @@ static void uvcg_extension_release(struct config_item *item)
> >> kfree(xu);
> >> }
> >>
> >> +static int uvcg_extension_allow_link(struct config_item *src, struct config_item *tgt)
> >> +{
> >> + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> >> + struct uvcg_extension *xu = to_uvcg_extension(src);
> >> + struct config_item *strings;
> >> + struct uvcg_string *string;
> >> + struct config_item *opts_item;
> >> + struct f_uvc_opts *opts;
> >> + int ret = 0;
> >> +
> >> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> >> +
> >> + /* Validate that the target of the link is an entry in strings/<langid> */
> >> + opts_item = src->ci_parent->ci_parent->ci_parent;
> >> +
> >> + strings = config_group_find_item(to_config_group(opts_item), "strings");
> >> + if (!strings || tgt->ci_parent->ci_parent != strings) {
> >> + ret = -EINVAL;
> >> + goto put_strings;
> >> + }
> >> +
> >> + string = to_uvcg_string(tgt);
> >> + opts = to_f_uvc_opts(opts_item);
> >> +
> >> + mutex_lock(&opts->lock);
> >> +
> >> + xu->string_descriptor_index = string->usb_string.id;
> >> +
> >> + mutex_unlock(&opts->lock);
> >> +
> >> +put_strings:
> >> + config_item_put(strings);
> >> + mutex_unlock(su_mutex);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void uvcg_extension_drop_link(struct config_item *src, struct config_item *tgt)
> >> +{
> >> + struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
> >> + struct uvcg_extension *xu = to_uvcg_extension(src);
> >> + struct config_item *opts_item;
> >> + struct f_uvc_opts *opts;
> >> +
> >> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> >> +
> >> + opts_item = src->ci_parent->ci_parent->ci_parent;
> >> + opts = to_f_uvc_opts(opts_item);
> >> +
> >> + mutex_lock(&opts->lock);
> >> +
> >> + xu->string_descriptor_index = 0;
> >> +
> >> + mutex_unlock(&opts->lock);
> >> +
> >> + mutex_unlock(su_mutex);
> >> +}
> >> +
> >> static struct configfs_item_operations uvcg_extension_item_ops = {
> >> .release = uvcg_extension_release,
> >> + .allow_link = uvcg_extension_allow_link,
> >> + .drop_link = uvcg_extension_drop_link,
> >> };
> >>
> >> static const struct config_item_type uvcg_extension_type = {
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
> >> index a714426a174a..e1308026aed6 100644
> >> --- a/drivers/usb/gadget/function/uvc_configfs.h
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> >> @@ -183,6 +183,7 @@ struct uvcg_extension_unit_descriptor {
> >> struct uvcg_extension {
> >> struct config_item item;
> >> struct list_head list;
> >> + u8 string_descriptor_index;
> >> struct uvcg_extension_unit_descriptor desc;
> >> };
> >>
--
Regards,
Laurent Pinchart
