| Message ID | CO1PR17MB54195BE778868AFDFE2DCB36E1112@CO1PR17MB5419.namprd17.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] usb: gadget: f_uac2: Expose all string descriptors through configfs. | expand |
On 23. 04. 24 19:22, Chris Wulff wrote: >> From: Pavel Hofman <pavel.hofman@ivitera.com> >> Sent: Tuesday, April 23, 2024 11:38 AM > >>> + p_it_name playback input terminal name >>> + p_ot_name playback output terminal name >>> + p_fu_name playback function unit name >>> + p_alt0_name playback alt mode 0 name >>> + p_alt1_name playback alt mode 1 name >> >> Nacked-by: Pavel Hofman <pavel.hofman@ivitera.com> >> >> I am not sure adding a numbered parameter for every additional alt mode >> is a way to go for the future. I am not that much concerned about UAC1, >> but IMO (at least) in UAC2 the configuration method should be flexible >> for more alt setttings. I can see use cases with many more altsettings. >> >> My proposal for adding more alt settings >> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/35be4668-58d3-894a-72cf-de1afaacae45@ivitera.com/__;!!HBnMciuwfVSXJQ!TYg7j7-fh3eZAzPfiONi2lo54mf2qsWtpG0nwdaQwSqd1nGdKkTDN8o6_lSIWlWPtHoc-2Nz1KCbRhiXJnzXO8Ku1w$ >> suggested using lists to existing parameters where each item would >> correspond to the alt setting of the same index (+1). That would allow >> using more altsettings easily, without having to add parameters to the >> source code and adding configfs params. I received no feedback. I do not >> push the param list proposal, but I am convinced an acceptable solution >> should be discussed thoroughly by the UAC2 gadget stakeholders. >> >> I am afraid that once p_alt1_name/c_alt1_name params are accepted, there >> will be no way back because subsequent removal of configfs params could >> be viewed as a regression for users. > > I have been thinking about this as well. The alt names are slightly different than the rest of the settings > since they also include alt mode 0. I was thinking p/c_alt1_name could be expanded to the array so > that the entries line up with the other settings and don't have an extra entry for alt 0. Perhaps a different > name would make more sense. > > Along those lines, I didn't see any gadget drivers using an array of strings for anything, which is also why > I didn't try to do anything here that merged alt0/1 names into an array. If we were to do an array of strings > I'm not sure what the best separator would be. Maybe ";"? The rates array uses ",". > > This patch only exposes the existing strings to make them configurable, but I don't want to do anything > that would preclude a nice interface for extra alt modes. > Thanks a lot for your response. Please can you take a look at https://lore.kernel.org/linux-usb/72e9b581-4a91-2319-cb9f-0bcb370f34a1@ivitera.com/T/#m68560853b0c7bc2478942d1f953caa2ac95512bd ? If the params in the upper level were to stand as defaults for the altsettings (and for the existing altsetting 1 if no specific altset subdir configs were given), maybe the naming xxx_alt1_xxx could become a bit confusing. E.g. p_altx_name or p_alt_non0_name? Thanks a lot, Pavel.
>From: Pavel Hofman <pavel.hofman@ivitera.com> >>>> + p_it_name playback input terminal name >>>> + p_ot_name playback output terminal name >>>> + p_fu_name playback function unit name >>>> + p_alt0_name playback alt mode 0 name >>>> + p_alt1_name playback alt mode 1 name >>> >>> Nacked-by: Pavel Hofman <pavel.hofman@ivitera.com> ... >If the params in the upper level were to stand as defaults for the >altsettings (and for the existing altsetting 1 if no specific altset >subdir configs were given), maybe the naming xxx_alt1_xxx could become a >bit confusing. E.g. p_altx_name or p_alt_non0_name? I've been prototyping this a bit to see how it will work. My current configfs structure looks something like: (all existing properties) c_it_name c_it_ch_name c_fu_name c_ot_name p_it_name p_it_ch_name p_fu_name p_ot_name num_alt_modes (settable to 2..5 in my prototype) alt.0 c_alt_name p_alt_name alt.1 (for alt.1, alt.2, etc.) c_alt_name p_alt_name c_ssize p_ssize (Additional properties here for other things that are settable for each alt mode, but the only one I've implemented in my prototype so far is sample size.) This brings up a few questions: Is a property for setting the number of alt modes preferred, or being able to make directories like some other things (eg, "mkdir alt.5" would add alt mode 5)? The former is what I started with, but I am leaning towards the latter as it is a bit more flexible (you could create alt modes of any index, though I'm not entirely sure why you'd want to.) It does involve a bit more dynamic memory allocation, but nothing crazy. And second, should the alt.x directories go back to the defaults if you remove and re-create them? I'm assuming it makes sense to do that, just putting it out there since my current prototype doesn't work that way. I appreciate your thoughts on this. -- Chris Wulff
On 27. 04. 24 18:27, Chris Wulff wrote: >> From: Pavel Hofman <pavel.hofman@ivitera.com> >>>>> + p_it_name playback input terminal name >>>>> + p_ot_name playback output terminal name >>>>> + p_fu_name playback function unit name >>>>> + p_alt0_name playback alt mode 0 name >>>>> + p_alt1_name playback alt mode 1 name >>>> >>>> Nacked-by: Pavel Hofman <pavel.hofman@ivitera.com> > ... >> If the params in the upper level were to stand as defaults for the >> altsettings (and for the existing altsetting 1 if no specific altset >> subdir configs were given), maybe the naming xxx_alt1_xxx could become a >> bit confusing. E.g. p_altx_name or p_alt_non0_name? > > I've been prototyping this a bit to see how it will work. My current configfs > structure looks something like: > > (all existing properties) > c_it_name > c_it_ch_name > c_fu_name > c_ot_name > p_it_name > p_it_ch_name > p_fu_name > p_ot_name > num_alt_modes (settable to 2..5 in my prototype) > > alt.0 > c_alt_name > p_alt_name > alt.1 (for alt.1, alt.2, etc.) > c_alt_name > p_alt_name > c_ssize > p_ssize > (Additional properties here for other things that are settable for each alt mode, > but the only one I've implemented in my prototype so far is sample size.) > Hats off to your speed, that's amazing. IMO this is a perfect config layout, logical, extensible, easy to generate manually as well as with a script. > This brings up a few questions: > > Is a property for setting the number of alt modes preferred, or being able to > make directories like some other things (eg, "mkdir alt.5" would add alt mode 5)? > The former is what I started with, but I am leaning towards the latter as it is a bit > more flexible (you could create alt modes of any index, though I'm not entirely > sure why you'd want to.) It does involve a bit more dynamic memory allocation, > but nothing crazy. I am not sure the arbitrary index of alt mode would be useful (is it even allowed in USB specs?). But I may not have understood your question properly. The num_alt_modes property - can the number be perhaps aquired from the number of created directories? Or would that number of alt modes be created automatically (all same with default values), and the properties in alt.X dirs would subsequently only modify their respective values? > > And second, should the alt.x directories go back to the defaults if you remove > and re-create them? I'm assuming it makes sense to do that, just putting it out > there since my current prototype doesn't work that way. IIUC just creating the alt.X directory would create the alt X mode, with default properties from the top-level configs or with the source-code defaults. Thanks a lot, Pavel.
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uac2 b/Documentation/ABI/testing/configfs-usb-gadget-uac2 index a2bf4fd82a5b..250f8eeb8eab 100644 --- a/Documentation/ABI/testing/configfs-usb-gadget-uac2 +++ b/Documentation/ABI/testing/configfs-usb-gadget-uac2 @@ -35,6 +35,19 @@ Description: req_number the number of pre-allocated requests for both capture and playback function_name name of the interface + if_ctrl_name topology control name + clksrc_in_name input clock name + clksrc_out_name output clock name + p_it_name playback input terminal name + p_ot_name playback output terminal name + p_fu_name playback function unit name + p_alt0_name playback alt mode 0 name + p_alt1_name playback alt mode 1 name + c_it_name capture input terminal name + c_ot_name capture output terminal name + c_fu_name capture functional unit name + c_alt0_name capture alt mode 0 name + c_alt1_name capture alt mode 1 name c_terminal_type code of the capture terminal type p_terminal_type code of the playback terminal type ===================== ======================================= diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst index b086c7ab72f0..1a11d3b3bb88 100644 --- a/Documentation/usb/gadget-testing.rst +++ b/Documentation/usb/gadget-testing.rst @@ -765,6 +765,19 @@ The uac2 function provides these attributes in its function directory: req_number the number of pre-allocated request for both capture and playback function_name name of the interface + if_ctrl_name topology control name + clksrc_in_name input clock name + clksrc_out_name output clock name + p_it_name playback input terminal name + p_ot_name playback output terminal name + p_fu_name playback function unit name + p_alt0_name playback alt mode 0 name + p_alt1_name playback alt mode 1 name + c_it_name capture input terminal name + c_ot_name capture output terminal name + c_fu_name capture functional unit name + c_alt0_name capture alt mode 0 name + c_alt1_name capture alt mode 1 name c_terminal_type code of the capture terminal type p_terminal_type code of the playback terminal type ================ ==================================================== diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c index 383f6854cfec..5ca7ecdb6e60 100644 --- a/drivers/usb/gadget/function/f_uac2.c +++ b/drivers/usb/gadget/function/f_uac2.c @@ -104,25 +104,10 @@ enum { STR_AS_OUT_ALT1, STR_AS_IN_ALT0, STR_AS_IN_ALT1, + NUM_STR_DESCRIPTORS, }; -static struct usb_string strings_fn[] = { - /* [STR_ASSOC].s = DYNAMIC, */ - [STR_IF_CTRL].s = "Topology Control", - [STR_CLKSRC_IN].s = "Input Clock", - [STR_CLKSRC_OUT].s = "Output Clock", - [STR_USB_IT].s = "USBH Out", - [STR_IO_IT].s = "USBD Out", - [STR_USB_OT].s = "USBH In", - [STR_IO_OT].s = "USBD In", - [STR_FU_IN].s = "Capture Volume", - [STR_FU_OUT].s = "Playback Volume", - [STR_AS_OUT_ALT0].s = "Playback Inactive", - [STR_AS_OUT_ALT1].s = "Playback Active", - [STR_AS_IN_ALT0].s = "Capture Inactive", - [STR_AS_IN_ALT1].s = "Capture Active", - { }, -}; +static struct usb_string strings_fn[NUM_STR_DESCRIPTORS + 1] = {}; static const char *const speed_names[] = { [USB_SPEED_UNKNOWN] = "UNKNOWN", @@ -1049,6 +1034,21 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn) return ret; strings_fn[STR_ASSOC].s = uac2_opts->function_name; + strings_fn[STR_IF_CTRL].s = uac2_opts->if_ctrl_name; + strings_fn[STR_CLKSRC_IN].s = uac2_opts->clksrc_in_name; + strings_fn[STR_CLKSRC_OUT].s = uac2_opts->clksrc_out_name; + + strings_fn[STR_USB_IT].s = uac2_opts->p_it_name; + strings_fn[STR_IO_OT].s = uac2_opts->p_ot_name; + strings_fn[STR_FU_OUT].s = uac2_opts->p_fu_name; + strings_fn[STR_AS_OUT_ALT0].s = uac2_opts->p_alt0_name; + strings_fn[STR_AS_OUT_ALT1].s = uac2_opts->p_alt1_name; + + strings_fn[STR_IO_IT].s = uac2_opts->c_it_name; + strings_fn[STR_USB_OT].s = uac2_opts->c_ot_name; + strings_fn[STR_FU_IN].s = uac2_opts->c_fu_name; + strings_fn[STR_AS_IN_ALT0].s = uac2_opts->c_alt0_name; + strings_fn[STR_AS_IN_ALT1].s = uac2_opts->c_alt1_name; us = usb_gstrings_attach(cdev, fn_strings, ARRAY_SIZE(strings_fn)); if (IS_ERR(us)) @@ -2097,10 +2097,26 @@ UAC2_ATTRIBUTE(s16, c_volume_max); UAC2_ATTRIBUTE(s16, c_volume_res); UAC2_ATTRIBUTE(u32, fb_max); UAC2_ATTRIBUTE_STRING(function_name); +UAC2_ATTRIBUTE_STRING(if_ctrl_name); +UAC2_ATTRIBUTE_STRING(clksrc_in_name); +UAC2_ATTRIBUTE_STRING(clksrc_out_name); + +UAC2_ATTRIBUTE_STRING(p_it_name); +UAC2_ATTRIBUTE_STRING(p_ot_name); +UAC2_ATTRIBUTE_STRING(p_fu_name); +UAC2_ATTRIBUTE_STRING(p_alt0_name); +UAC2_ATTRIBUTE_STRING(p_alt1_name); + +UAC2_ATTRIBUTE_STRING(c_it_name); +UAC2_ATTRIBUTE_STRING(c_ot_name); +UAC2_ATTRIBUTE_STRING(c_fu_name); +UAC2_ATTRIBUTE_STRING(c_alt0_name); +UAC2_ATTRIBUTE_STRING(c_alt1_name); UAC2_ATTRIBUTE(s16, p_terminal_type); UAC2_ATTRIBUTE(s16, c_terminal_type); + static struct configfs_attribute *f_uac2_attrs[] = { &f_uac2_opts_attr_p_chmask, &f_uac2_opts_attr_p_srate, @@ -2127,6 +2143,21 @@ static struct configfs_attribute *f_uac2_attrs[] = { &f_uac2_opts_attr_c_volume_res, &f_uac2_opts_attr_function_name, + &f_uac2_opts_attr_if_ctrl_name, + &f_uac2_opts_attr_clksrc_in_name, + &f_uac2_opts_attr_clksrc_out_name, + + &f_uac2_opts_attr_p_it_name, + &f_uac2_opts_attr_p_ot_name, + &f_uac2_opts_attr_p_fu_name, + &f_uac2_opts_attr_p_alt0_name, + &f_uac2_opts_attr_p_alt1_name, + + &f_uac2_opts_attr_c_it_name, + &f_uac2_opts_attr_c_ot_name, + &f_uac2_opts_attr_c_fu_name, + &f_uac2_opts_attr_c_alt0_name, + &f_uac2_opts_attr_c_alt1_name, &f_uac2_opts_attr_p_terminal_type, &f_uac2_opts_attr_c_terminal_type, @@ -2188,6 +2219,21 @@ static struct usb_function_instance *afunc_alloc_inst(void) opts->fb_max = FBACK_FAST_MAX; scnprintf(opts->function_name, sizeof(opts->function_name), "Source/Sink"); + scnprintf(opts->if_ctrl_name, sizeof(opts->if_ctrl_name), "Topology Control"); + scnprintf(opts->clksrc_in_name, sizeof(opts->clksrc_in_name), "Input Clock"); + scnprintf(opts->clksrc_out_name, sizeof(opts->clksrc_out_name), "Output Clock"); + + scnprintf(opts->p_it_name, sizeof(opts->p_it_name), "USBH Out"); + scnprintf(opts->p_ot_name, sizeof(opts->p_ot_name), "USBD In"); + scnprintf(opts->p_fu_name, sizeof(opts->p_fu_name), "Playback Volume"); + scnprintf(opts->p_alt0_name, sizeof(opts->p_alt0_name), "Playback Inactive"); + scnprintf(opts->p_alt1_name, sizeof(opts->p_alt1_name), "Playback Active"); + + scnprintf(opts->c_it_name, sizeof(opts->c_it_name), "USBD Out"); + scnprintf(opts->c_ot_name, sizeof(opts->c_ot_name), "USBH In"); + scnprintf(opts->c_fu_name, sizeof(opts->c_fu_name), "Capture Volume"); + scnprintf(opts->c_alt0_name, sizeof(opts->c_alt0_name), "Capture Inactive"); + scnprintf(opts->c_alt1_name, sizeof(opts->c_alt1_name), "Capture Active"); opts->p_terminal_type = UAC2_DEF_P_TERM_TYPE; opts->c_terminal_type = UAC2_DEF_C_TERM_TYPE; diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h index 5e81bdd6c5fb..e697d35a1759 100644 --- a/drivers/usb/gadget/function/u_uac2.h +++ b/drivers/usb/gadget/function/u_uac2.h @@ -68,7 +68,22 @@ struct f_uac2_opts { int fb_max; bool bound; - char function_name[32]; + char function_name[USB_MAX_STRING_LEN]; + char if_ctrl_name[USB_MAX_STRING_LEN]; + char clksrc_in_name[USB_MAX_STRING_LEN]; + char clksrc_out_name[USB_MAX_STRING_LEN]; + + char p_it_name[USB_MAX_STRING_LEN]; + char p_ot_name[USB_MAX_STRING_LEN]; + char p_fu_name[USB_MAX_STRING_LEN]; + char p_alt0_name[USB_MAX_STRING_LEN]; + char p_alt1_name[USB_MAX_STRING_LEN]; + + char c_it_name[USB_MAX_STRING_LEN]; + char c_ot_name[USB_MAX_STRING_LEN]; + char c_fu_name[USB_MAX_STRING_LEN]; + char c_alt0_name[USB_MAX_STRING_LEN]; + char c_alt1_name[USB_MAX_STRING_LEN]; s16 p_terminal_type; s16 c_terminal_type;
This makes all string descriptors configurable for the UAC2 gadget so the user can configure names of terminals/controls/alt modes. Signed-off-by: Chris Wulff <chris.wulff@biamp.com> --- v2: Improved naming of parameters to be mode user friendly. Added documentation. v1: https://lore.kernel.org/linux-usb/CO1PR17MB5419B50F94A0014647542931E10D2@CO1PR17MB5419.namprd17.prod.outlook.com/ .../ABI/testing/configfs-usb-gadget-uac2 | 13 +++ Documentation/usb/gadget-testing.rst | 13 +++ drivers/usb/gadget/function/f_uac2.c | 80 +++++++++++++++---- drivers/usb/gadget/function/u_uac2.h | 17 +++- 4 files changed, 105 insertions(+), 18 deletions(-)