| Message ID | 20221121092517.225242-7-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add XU support to UVC Gadget | expand |
Hi Dan, Thank you for the patch. 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 ? > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > 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; > }; >
Hi Laurent On 29/12/2022 02:05, Laurent Pinchart wrote: > Hi Dan, > > Thank you for the patch. > > 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? > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> 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; >> }; >>
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@ideasonboard.com> > >> --- > >> 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; > >> }; > >>
Hi Laurent On 02/01/2023 12:25, Laurent Pinchart wrote: > 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 :-) Coming back to this after the break and other things got in the way...my expectation here would be to just move roughly the current implementation to the strings config group that's at the root of a USB gadget's configfs directory structure (where the manufacturer, product and serialnumber attributes currently live)...I.E. still to create a config item for the string with the id and string attributes and link to them from other parts of the config like the extension units to assign them. That puts them into the "core" part rather than UVC gadget...but the actual functionality wouldn't be changing much. Is that what you meant? > >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>>> --- >>>> 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; >>>> }; >>>>
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; };
Add .allow_link() and .drop_link() callbacks to allow users to link an extension unit descriptor to a string descriptor. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- 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(+)