[PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.

[PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.

[Chris Wulff]

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(-)

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

Re: [PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.

[Pavel Hofman]

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 <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(-)
> 
> 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 <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://lore.kernel.org/linux-usb/35be4668-58d3-894a-72cf-de1afaacae45@ivitera.com/
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;

Re: [PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.

[Chris Wulff]

> 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.

  -- Chris Wulff

Re: [PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.

[Pavel Hofman]

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.

Re: [PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.

[Chris Wulff]

>From: Pavel Hofman <pavel.hofman@ivitera.com>
>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 <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://lore.kernel.org/linux-usb/35be4668-58d3-894a-72cf-de1afaacae45@ivitera.com/
>>> 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/ ?
>
>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

Re: [PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.

[Chris Wulff]

>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

Re: [PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.

[Pavel Hofman]

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.