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 <[email protected]>
---
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(-)
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;
--
2.34.1
On 23. 04. 24 16:09, Chris Wulff wrote:
> 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 <[email protected]>
> ---
> 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(-)
>
> 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
Nacked-by: Pavel Hofman <[email protected]>
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://lore.kernel.org/linux-usb/[email protected]/
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.
Thanks a lot for considering,
Pavel.
> + 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;
> From:?Pavel Hofman <[email protected]>
> 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 <[email protected]>
>
> 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/[email protected]/__;!!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.
-- Chris Wulff
On 23. 04. 24 19:22, Chris Wulff wrote:
>> From: Pavel Hofman <[email protected]>
>> 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 <[email protected]>
>>
>> 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/[email protected]/__;!!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/[email protected]/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 <[email protected]>
>Sent:?Wednesday, April 24, 2024 3:55 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 <[email protected]>
>>>
>>> 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://lore.kernel.org/linux-usb/[email protected]/
>>> 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/[email protected]/T/ ?
>
>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 am in favor of the subdirs for alt mode settings, with the main config options acting as the default/single
configuration as it is today.
I can change these patches from c/p_alt1_name to c/p_altx_name if nobody objects to that, or I could remove
the alt name from this patch set if anyone thinks this needs more discussion. I don't actually need to set
the alt name for my use case, but included it for completeness.
-- Chris Wulff
On 27. 04. 24 18:27, Chris Wulff wrote:
>> From: Pavel Hofman <[email protected]>
>>>>> + 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 <[email protected]>
> ...
>> 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.