diff mbox series

[v2,03/11] usb: gadget: f_uac2: Support multiple sampling rates

Message ID 20211220211130.88590-4-pavel.hofman@ivitera.com
State New
Headers show
Series usb: gadget: audio: Multiple rates, dyn. bInterval | expand

Commit Message

Pavel Hofman Dec. 20, 2021, 9:11 p.m. UTC 
From: Julian Scheel <julian@jusst.de>

A list of sampling rates can be specified via configfs. All enabled
sampling rates are sent to the USB host on request. When the host
selects a sampling rate the internal active rate is updated.

Config strings with single value stay compatible with the previous version.

Multiple samplerates passed as configuration arrays to g_audio module
when built for f_uac2.

Signed-off-by: Julian Scheel <julian@jusst.de>
Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>
---
 .../ABI/testing/configfs-usb-gadget-uac2      |   4 +-
 Documentation/usb/gadget-testing.rst          |   4 +-
 drivers/usb/gadget/function/f_uac2.c          | 118 ++++++++++++++----
 drivers/usb/gadget/function/u_uac2.h          |  62 +++++++++
 drivers/usb/gadget/legacy/audio.c             |  28 +++--
 5 files changed, 177 insertions(+), 39 deletions(-)

Comments

John Keeping Dec. 21, 2021, 11:59 a.m. UTC | #1 
On Mon, Dec 20, 2021 at 10:11:22PM +0100, Pavel Hofman wrote:
> From: Julian Scheel <julian@jusst.de>
> 
> A list of sampling rates can be specified via configfs. All enabled
> sampling rates are sent to the USB host on request. When the host
> selects a sampling rate the internal active rate is updated.
> 
> Config strings with single value stay compatible with the previous version.
> 
> Multiple samplerates passed as configuration arrays to g_audio module
> when built for f_uac2.
> 
> Signed-off-by: Julian Scheel <julian@jusst.de>
> Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>
> ---
>  .../ABI/testing/configfs-usb-gadget-uac2      |   4 +-
>  Documentation/usb/gadget-testing.rst          |   4 +-
>  drivers/usb/gadget/function/f_uac2.c          | 118 ++++++++++++++----
>  drivers/usb/gadget/function/u_uac2.h          |  62 +++++++++
>  drivers/usb/gadget/legacy/audio.c             |  28 +++--
>  5 files changed, 177 insertions(+), 39 deletions(-)
> 
> diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
> index c18113077889..928f60a31544 100644
> --- a/Documentation/usb/gadget-testing.rst
> +++ b/Documentation/usb/gadget-testing.rst
> @@ -726,7 +726,7 @@ The uac2 function provides these attributes in its function directory:
>  
>  	================ ====================================================
>  	c_chmask         capture channel mask
> -	c_srate          capture sampling rate
> +	c_srate          list of capture sampling rates (comma-separated)
>  	c_ssize          capture sample size (bytes)
>  	c_sync           capture synchronization type (async/adaptive)
>  	c_mute_present   capture mute control enable
> @@ -736,7 +736,7 @@ The uac2 function provides these attributes in its function directory:
>  	c_volume_res     capture volume control resolution (in 1/256 dB)
>  	fb_max           maximum extra bandwidth in async mode
>  	p_chmask         playback channel mask
> -	p_srate          playback sampling rate
> +	p_srate          list of playback sampling rates (comma-separated)
>  	p_ssize          playback sample size (bytes)
>  	p_mute_present   playback mute control enable
>  	p_volume_present playback volume control enable
> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
> index 1d6e426e5078..74e32bb146c7 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -70,6 +70,7 @@ struct f_uac2 {
>  	/* Interrupt IN endpoint of AC interface */
>  	struct usb_ep	*int_ep;
>  	atomic_t	int_count;
> +	int ctl_id;

Control for what?  This is assigned from a variable called clock_id, so
shouldn't that be the name here?

I think this needs a comment to explain that it's transient state that
is only valid during the handling of a single control request.

>  };
>  
>  static inline struct f_uac2 *func_to_uac2(struct usb_function *f)
> @@ -166,7 +167,7 @@ static struct uac_clock_source_descriptor in_clk_src_desc = {
>  	.bDescriptorSubtype = UAC2_CLOCK_SOURCE,
>  	/* .bClockID = DYNAMIC */
>  	.bmAttributes = UAC_CLOCK_SOURCE_TYPE_INT_FIXED,
> -	.bmControls = (CONTROL_RDONLY << CLK_FREQ_CTRL),
> +	.bmControls = (CONTROL_RDWR << CLK_FREQ_CTRL),
>  	.bAssocTerminal = 0,
>  };
>  
> @@ -178,7 +179,7 @@ static struct uac_clock_source_descriptor out_clk_src_desc = {
>  	.bDescriptorSubtype = UAC2_CLOCK_SOURCE,
>  	/* .bClockID = DYNAMIC */
>  	.bmAttributes = UAC_CLOCK_SOURCE_TYPE_INT_FIXED,
> -	.bmControls = (CONTROL_RDONLY << CLK_FREQ_CTRL),
> +	.bmControls = (CONTROL_RDWR << CLK_FREQ_CTRL),
>  	.bAssocTerminal = 0,
>  };
>  
> @@ -635,12 +636,32 @@ struct cntrl_cur_lay3 {
>  };
>  
>  struct cntrl_range_lay3 {
> -	__le16	wNumSubRanges;
>  	__le32	dMIN;
>  	__le32	dMAX;
>  	__le32	dRES;
>  } __packed;
>  
> +#define ranges_size(c) (sizeof(c.wNumSubRanges) + c.wNumSubRanges \
> +		* sizeof(struct cntrl_ranges_lay3))
> +
> +struct cntrl_ranges_lay3 {
> +	__u16	wNumSubRanges;
> +	struct cntrl_range_lay3 r[UAC_MAX_RATES];
> +} __packed;

These structures are now inconsistent between cntrl_range_lay2 and
cntrl_range_lay3.  Would it be better to make these flex arrays?  I
guess that will make the code that uses it more complicated, but at the
moment it looks like these are trying to be generic while in reality
being quite specific to the one place that uses them at the moment.

> +static int get_max_srate(const int *srates)
> +{
> +	int i, max_srate = 0;
> +
> +	for (i = 0; i < UAC_MAX_RATES; i++) {
> +		if (srates[i] == 0)
> +			break;
> +		if (srates[i] > max_srate)
> +			max_srate = srates[i];
> +	}
> +	return max_srate;
> +}
> +
>  static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>  	struct usb_endpoint_descriptor *ep_desc,
>  	enum usb_device_speed speed, bool is_playback)
> @@ -667,11 +688,11 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>  
>  	if (is_playback) {
>  		chmask = uac2_opts->p_chmask;
> -		srate = uac2_opts->p_srate;
> +		srate = get_max_srate(uac2_opts->p_srates);
>  		ssize = uac2_opts->p_ssize;
>  	} else {
>  		chmask = uac2_opts->c_chmask;
> -		srate = uac2_opts->c_srate;
> +		srate = get_max_srate(uac2_opts->c_srates);
>  		ssize = uac2_opts->c_ssize;
>  	}
>  
> @@ -912,10 +933,10 @@ static int afunc_validate_opts(struct g_audio *agdev, struct device *dev)
>  	} else if ((opts->c_ssize < 1) || (opts->c_ssize > 4)) {
>  		dev_err(dev, "Error: incorrect capture sample size\n");
>  		return -EINVAL;
> -	} else if (!opts->p_srate) {
> +	} else if (!opts->p_srates[0]) {
>  		dev_err(dev, "Error: incorrect playback sampling rate\n");
>  		return -EINVAL;
> -	} else if (!opts->c_srate) {
> +	} else if (!opts->c_srates[0]) {
>  		dev_err(dev, "Error: incorrect capture sampling rate\n");
>  		return -EINVAL;
>  	}
> @@ -1210,7 +1231,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
>  
>  	agdev->params.p_chmask = uac2_opts->p_chmask;
>  	agdev->params.p_srate = uac2_opts->p_srate;
> -	agdev->params.p_srates[0] = uac2_opts->p_srate;
> +	memcpy(agdev->params.p_srates, uac2_opts->p_srates,
> +			sizeof(agdev->params.p_srates));
>  	agdev->params.p_ssize = uac2_opts->p_ssize;
>  	if (FUIN_EN(uac2_opts)) {
>  		agdev->params.p_fu.id = USB_IN_FU_ID;
> @@ -1222,7 +1244,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
>  	}
>  	agdev->params.c_chmask = uac2_opts->c_chmask;
>  	agdev->params.c_srate = uac2_opts->c_srate;
> -	agdev->params.c_srates[0] = uac2_opts->c_srate;
> +	memcpy(agdev->params.c_srates, uac2_opts->c_srates,
> +			sizeof(agdev->params.c_srates));
>  	agdev->params.c_ssize = uac2_opts->c_ssize;
>  	if (FUOUT_EN(uac2_opts)) {
>  		agdev->params.c_fu.id = USB_OUT_FU_ID;
> @@ -1502,28 +1525,39 @@ in_rq_range(struct usb_function *fn, const struct usb_ctrlrequest *cr)
>  	u8 entity_id = (w_index >> 8) & 0xff;
>  	u8 control_selector = w_value >> 8;
>  	int value = -EOPNOTSUPP;
> -	int p_srate, c_srate;
> -
> -	p_srate = opts->p_srate;
> -	c_srate = opts->c_srate;
>  
>  	if ((entity_id == USB_IN_CLK_ID) || (entity_id == USB_OUT_CLK_ID)) {
>  		if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) {
> -			struct cntrl_range_lay3 r;
> +			struct cntrl_ranges_lay3 rs;
> +			int i;
> +			int wNumSubRanges = 0;
> +			int srate;
> +			int *srates;
>  
>  			if (entity_id == USB_IN_CLK_ID)
> -				r.dMIN = cpu_to_le32(p_srate);
> +				srates = opts->p_srates;
>  			else if (entity_id == USB_OUT_CLK_ID)
> -				r.dMIN = cpu_to_le32(c_srate);
> +				srates = opts->c_srates;
>  			else
>  				return -EOPNOTSUPP;
> -
> -			r.dMAX = r.dMIN;
> -			r.dRES = 0;
> -			r.wNumSubRanges = cpu_to_le16(1);
> -
> -			value = min_t(unsigned int, w_length, sizeof(r));
> -			memcpy(req->buf, &r, value);
> +			for (i = 0; i < UAC_MAX_RATES; i++) {
> +				srate = srates[i];
> +				if (srate == 0)
> +					break;
> +
> +				rs.r[wNumSubRanges].dMIN = cpu_to_le32(srate);
> +				rs.r[wNumSubRanges].dMAX = cpu_to_le32(srate);
> +				rs.r[wNumSubRanges].dRES = 0;
> +				wNumSubRanges++;
> +				dev_dbg(&agdev->gadget->dev,
> +					"%s(): clk %d: rate ID %d: %d\n",
> +					__func__, entity_id, wNumSubRanges, srate);
> +			}
> +			rs.wNumSubRanges = cpu_to_le16(wNumSubRanges);
> +			value = min_t(unsigned int, w_length, ranges_size(rs));
> +			dev_dbg(&agdev->gadget->dev, "%s(): sending %d rates, size %d\n",
> +				__func__, rs.wNumSubRanges, value);
> +			memcpy(req->buf, &rs, value);
>  		} else {
>  			dev_err(&agdev->gadget->dev,
>  				"%s:%d control_selector=%d TODO!\n",
> @@ -1582,6 +1616,28 @@ ac_rq_in(struct usb_function *fn, const struct usb_ctrlrequest *cr)
>  		return -EOPNOTSUPP;
>  }
>  
> +static void uac2_cs_control_sam_freq(struct usb_ep *ep, struct usb_request *req)
> +{
> +	struct usb_function *fn = ep->driver_data;
> +	struct g_audio *agdev = func_to_g_audio(fn);
> +	struct f_uac2 *uac2 = func_to_uac2(fn);
> +	struct f_uac2_opts *opts = g_audio_to_uac2_opts(agdev);
> +	u32 val;
> +
> +	if (req->actual != 4)
> +		return;
> +
> +	val = le32_to_cpu(*((u32 *)req->buf));
> +	dev_dbg(&agdev->gadget->dev, "%s val: %d.\n", __func__, val);
> +	if (uac2->ctl_id == USB_IN_CLK_ID) {
> +		opts->p_srate = val;

Don't you need to hold opts->lock to change this?

I'm not sure opts should be changed here though - that's the setup phase
and this is "current state", so shouldn't it move to struct f_uac2?

> +		u_audio_set_playback_srate(agdev, opts->p_srate);
> +	} else if (uac2->ctl_id == USB_OUT_CLK_ID) {
> +		opts->c_srate = val;
> +		u_audio_set_capture_srate(agdev, opts->c_srate);
> +	}
> +}
> +
>  static void
>  out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req)
>  {
> @@ -1633,6 +1689,7 @@ out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req)
>  static int
>  out_rq_cur(struct usb_function *fn, const struct usb_ctrlrequest *cr)
>  {
> +	struct usb_composite_dev *cdev = fn->config->cdev;
>  	struct usb_request *req = fn->config->cdev->req;
>  	struct g_audio *agdev = func_to_g_audio(fn);
>  	struct f_uac2_opts *opts = g_audio_to_uac2_opts(agdev);
> @@ -1642,10 +1699,17 @@ out_rq_cur(struct usb_function *fn, const struct usb_ctrlrequest *cr)
>  	u16 w_value = le16_to_cpu(cr->wValue);
>  	u8 entity_id = (w_index >> 8) & 0xff;
>  	u8 control_selector = w_value >> 8;
> +	u8 clock_id = w_index >> 8;
>  
>  	if ((entity_id == USB_IN_CLK_ID) || (entity_id == USB_OUT_CLK_ID)) {
> -		if (control_selector == UAC2_CS_CONTROL_SAM_FREQ)
> +		if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) {
> +			dev_dbg(&agdev->gadget->dev,
> +				"control_selector UAC2_CS_CONTROL_SAM_FREQ, clock: %d\n", clock_id);
> +			cdev->gadget->ep0->driver_data = fn;
> +			uac2->ctl_id = clock_id;
> +			req->complete = uac2_cs_control_sam_freq;
>  			return w_length;
> +		}
>  	} else if ((FUIN_EN(opts) && (entity_id == USB_IN_FU_ID)) ||
>  			(FUOUT_EN(opts) && (entity_id == USB_OUT_FU_ID))) {
>  		memcpy(&uac2->setup_cr, cr, sizeof(*cr));
> @@ -1839,10 +1903,10 @@ end:									\
>  CONFIGFS_ATTR(f_uac2_opts_, name)
>  
>  UAC2_ATTRIBUTE(u32, p_chmask);
> -UAC2_ATTRIBUTE(u32, p_srate);
> +UAC_RATE2_ATTRIBUTE(p_srate);

UAC2_RATE_ATTRIBUTE() to match the pattern of the other values here?

>  UAC2_ATTRIBUTE(u32, p_ssize);
>  UAC2_ATTRIBUTE(u32, c_chmask);
> -UAC2_ATTRIBUTE(u32, c_srate);
> +UAC_RATE2_ATTRIBUTE(c_srate);
>  UAC2_ATTRIBUTE_SYNC(c_sync);
>  UAC2_ATTRIBUTE(u32, c_ssize);
>  UAC2_ATTRIBUTE(u32, req_number);
> @@ -1915,9 +1979,11 @@ static struct usb_function_instance *afunc_alloc_inst(void)
>  				    &f_uac2_func_type);
>  
>  	opts->p_chmask = UAC2_DEF_PCHMASK;
> +	opts->p_srates[0] = UAC2_DEF_PSRATE;
>  	opts->p_srate = UAC2_DEF_PSRATE;
>  	opts->p_ssize = UAC2_DEF_PSSIZE;
>  	opts->c_chmask = UAC2_DEF_CCHMASK;
> +	opts->c_srates[0] = UAC2_DEF_CSRATE;
>  	opts->c_srate = UAC2_DEF_CSRATE;
>  	opts->c_ssize = UAC2_DEF_CSSIZE;
>  	opts->c_sync = UAC2_DEF_CSYNC;
> diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h
> index e0c8e3513bfd..8058217322f8 100644
> --- a/drivers/usb/gadget/function/u_uac2.h
> +++ b/drivers/usb/gadget/function/u_uac2.h
> @@ -14,6 +14,7 @@
>  #define U_UAC2_H
>  
>  #include <linux/usb/composite.h>
> +#include "uac_common.h"
>  
>  #define UAC2_DEF_PCHMASK 0x3
>  #define UAC2_DEF_PSRATE 48000
> @@ -35,9 +36,11 @@
>  struct f_uac2_opts {
>  	struct usb_function_instance	func_inst;
>  	int				p_chmask;
> +	int				p_srates[UAC_MAX_RATES];
>  	int				p_srate;
>  	int				p_ssize;
>  	int				c_chmask;
> +	int				c_srates[UAC_MAX_RATES];
>  	int				c_srate;
>  	int				c_ssize;
>  	int				c_sync;
> @@ -62,4 +65,63 @@ struct f_uac2_opts {
>  	int				refcnt;
>  };
>  
> +#define UAC_RATE2_ATTRIBUTE(name)					\

Why define this in the header?  UAC2_ATTRIBUTE() is in the .c file and
this is only used in one place so no need for it to be in the .h.
Pavel Hofman Dec. 22, 2021, 10:01 a.m. UTC | #2 
Dne 21. 12. 21 v 12:59 John Keeping napsal(a):
> On Mon, Dec 20, 2021 at 10:11:22PM +0100, Pavel Hofman wrote:
>> From: Julian Scheel <julian@jusst.de>
>>
>> A list of sampling rates can be specified via configfs. All enabled
>> sampling rates are sent to the USB host on request. When the host
>> selects a sampling rate the internal active rate is updated.
>>
>> Config strings with single value stay compatible with the previous version.
>>
>> Multiple samplerates passed as configuration arrays to g_audio module
>> when built for f_uac2.
>>
>> Signed-off-by: Julian Scheel <julian@jusst.de>
>> Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>
>> ---
>>   .../ABI/testing/configfs-usb-gadget-uac2      |   4 +-
>>   Documentation/usb/gadget-testing.rst          |   4 +-
>>   drivers/usb/gadget/function/f_uac2.c          | 118 ++++++++++++++----
>>   drivers/usb/gadget/function/u_uac2.h          |  62 +++++++++
>>   drivers/usb/gadget/legacy/audio.c             |  28 +++--
>>   5 files changed, 177 insertions(+), 39 deletions(-)
>>
>> diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
>> index c18113077889..928f60a31544 100644
>> --- a/Documentation/usb/gadget-testing.rst
>> +++ b/Documentation/usb/gadget-testing.rst
>> @@ -726,7 +726,7 @@ The uac2 function provides these attributes in its function directory:
>>   
>>   	================ ====================================================
>>   	c_chmask         capture channel mask
>> -	c_srate          capture sampling rate
>> +	c_srate          list of capture sampling rates (comma-separated)
>>   	c_ssize          capture sample size (bytes)
>>   	c_sync           capture synchronization type (async/adaptive)
>>   	c_mute_present   capture mute control enable
>> @@ -736,7 +736,7 @@ The uac2 function provides these attributes in its function directory:
>>   	c_volume_res     capture volume control resolution (in 1/256 dB)
>>   	fb_max           maximum extra bandwidth in async mode
>>   	p_chmask         playback channel mask
>> -	p_srate          playback sampling rate
>> +	p_srate          list of playback sampling rates (comma-separated)
>>   	p_ssize          playback sample size (bytes)
>>   	p_mute_present   playback mute control enable
>>   	p_volume_present playback volume control enable
>> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
>> index 1d6e426e5078..74e32bb146c7 100644
>> --- a/drivers/usb/gadget/function/f_uac2.c
>> +++ b/drivers/usb/gadget/function/f_uac2.c
>> @@ -70,6 +70,7 @@ struct f_uac2 {
>>   	/* Interrupt IN endpoint of AC interface */
>>   	struct usb_ep	*int_ep;
>>   	atomic_t	int_count;
>> +	int ctl_id;
> 
> Control for what?  This is assigned from a variable called clock_id, so
> shouldn't that be the name here?
> 
> I think this needs a comment to explain that it's transient state that
> is only valid during the handling of a single control request.

Fixed

> 
>>   };
>>   
>>   static inline struct f_uac2 *func_to_uac2(struct usb_function *f)
>> @@ -166,7 +167,7 @@ static struct uac_clock_source_descriptor in_clk_src_desc = {
>>   	.bDescriptorSubtype = UAC2_CLOCK_SOURCE,
>>   	/* .bClockID = DYNAMIC */
>>   	.bmAttributes = UAC_CLOCK_SOURCE_TYPE_INT_FIXED,
>> -	.bmControls = (CONTROL_RDONLY << CLK_FREQ_CTRL),
>> +	.bmControls = (CONTROL_RDWR << CLK_FREQ_CTRL),
>>   	.bAssocTerminal = 0,
>>   };
>>   
>> @@ -178,7 +179,7 @@ static struct uac_clock_source_descriptor out_clk_src_desc = {
>>   	.bDescriptorSubtype = UAC2_CLOCK_SOURCE,
>>   	/* .bClockID = DYNAMIC */
>>   	.bmAttributes = UAC_CLOCK_SOURCE_TYPE_INT_FIXED,
>> -	.bmControls = (CONTROL_RDONLY << CLK_FREQ_CTRL),
>> +	.bmControls = (CONTROL_RDWR << CLK_FREQ_CTRL),
>>   	.bAssocTerminal = 0,
>>   };
>>   
>> @@ -635,12 +636,32 @@ struct cntrl_cur_lay3 {
>>   };
>>   
>>   struct cntrl_range_lay3 {
>> -	__le16	wNumSubRanges;
>>   	__le32	dMIN;
>>   	__le32	dMAX;
>>   	__le32	dRES;
>>   } __packed;
>>   
>> +#define ranges_size(c) (sizeof(c.wNumSubRanges) + c.wNumSubRanges \
>> +		* sizeof(struct cntrl_ranges_lay3))
>> +
>> +struct cntrl_ranges_lay3 {
>> +	__u16	wNumSubRanges;
>> +	struct cntrl_range_lay3 r[UAC_MAX_RATES];
>> +} __packed;
> 
> These structures are now inconsistent between cntrl_range_lay2 and
> cntrl_range_lay3.  Would it be better to make these flex arrays?  I
> guess that will make the code that uses it more complicated, but at the
> moment it looks like these are trying to be generic while in reality
> being quite specific to the one place that uses them at the moment.

I am afraid I do not know exactly how to do that. Please can you post an 
example? The rate control requires u32 (u16 is too small). Thanks a lot.

> 
>> +static int get_max_srate(const int *srates)
>> +{
>> +	int i, max_srate = 0;
>> +
>> +	for (i = 0; i < UAC_MAX_RATES; i++) {
>> +		if (srates[i] == 0)
>> +			break;
>> +		if (srates[i] > max_srate)
>> +			max_srate = srates[i];
>> +	}
>> +	return max_srate;
>> +}
>> +
>>   static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>>   	struct usb_endpoint_descriptor *ep_desc,
>>   	enum usb_device_speed speed, bool is_playback)
>> @@ -667,11 +688,11 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>>   
>>   	if (is_playback) {
>>   		chmask = uac2_opts->p_chmask;
>> -		srate = uac2_opts->p_srate;
>> +		srate = get_max_srate(uac2_opts->p_srates);
>>   		ssize = uac2_opts->p_ssize;
>>   	} else {
>>   		chmask = uac2_opts->c_chmask;
>> -		srate = uac2_opts->c_srate;
>> +		srate = get_max_srate(uac2_opts->c_srates);
>>   		ssize = uac2_opts->c_ssize;
>>   	}
>>   
>> @@ -912,10 +933,10 @@ static int afunc_validate_opts(struct g_audio *agdev, struct device *dev)
>>   	} else if ((opts->c_ssize < 1) || (opts->c_ssize > 4)) {
>>   		dev_err(dev, "Error: incorrect capture sample size\n");
>>   		return -EINVAL;
>> -	} else if (!opts->p_srate) {
>> +	} else if (!opts->p_srates[0]) {
>>   		dev_err(dev, "Error: incorrect playback sampling rate\n");
>>   		return -EINVAL;
>> -	} else if (!opts->c_srate) {
>> +	} else if (!opts->c_srates[0]) {
>>   		dev_err(dev, "Error: incorrect capture sampling rate\n");
>>   		return -EINVAL;
>>   	}
>> @@ -1210,7 +1231,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
>>   
>>   	agdev->params.p_chmask = uac2_opts->p_chmask;
>>   	agdev->params.p_srate = uac2_opts->p_srate;
>> -	agdev->params.p_srates[0] = uac2_opts->p_srate;
>> +	memcpy(agdev->params.p_srates, uac2_opts->p_srates,
>> +			sizeof(agdev->params.p_srates));
>>   	agdev->params.p_ssize = uac2_opts->p_ssize;
>>   	if (FUIN_EN(uac2_opts)) {
>>   		agdev->params.p_fu.id = USB_IN_FU_ID;
>> @@ -1222,7 +1244,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
>>   	}
>>   	agdev->params.c_chmask = uac2_opts->c_chmask;
>>   	agdev->params.c_srate = uac2_opts->c_srate;
>> -	agdev->params.c_srates[0] = uac2_opts->c_srate;
>> +	memcpy(agdev->params.c_srates, uac2_opts->c_srates,
>> +			sizeof(agdev->params.c_srates));
>>   	agdev->params.c_ssize = uac2_opts->c_ssize;
>>   	if (FUOUT_EN(uac2_opts)) {
>>   		agdev->params.c_fu.id = USB_OUT_FU_ID;
>> @@ -1502,28 +1525,39 @@ in_rq_range(struct usb_function *fn, const struct usb_ctrlrequest *cr)
>>   	u8 entity_id = (w_index >> 8) & 0xff;
>>   	u8 control_selector = w_value >> 8;
>>   	int value = -EOPNOTSUPP;
>> -	int p_srate, c_srate;
>> -
>> -	p_srate = opts->p_srate;
>> -	c_srate = opts->c_srate;
>>   
>>   	if ((entity_id == USB_IN_CLK_ID) || (entity_id == USB_OUT_CLK_ID)) {
>>   		if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) {
>> -			struct cntrl_range_lay3 r;
>> +			struct cntrl_ranges_lay3 rs;
>> +			int i;
>> +			int wNumSubRanges = 0;
>> +			int srate;
>> +			int *srates;
>>   
>>   			if (entity_id == USB_IN_CLK_ID)
>> -				r.dMIN = cpu_to_le32(p_srate);
>> +				srates = opts->p_srates;
>>   			else if (entity_id == USB_OUT_CLK_ID)
>> -				r.dMIN = cpu_to_le32(c_srate);
>> +				srates = opts->c_srates;
>>   			else
>>   				return -EOPNOTSUPP;
>> -
>> -			r.dMAX = r.dMIN;
>> -			r.dRES = 0;
>> -			r.wNumSubRanges = cpu_to_le16(1);
>> -
>> -			value = min_t(unsigned int, w_length, sizeof(r));
>> -			memcpy(req->buf, &r, value);
>> +			for (i = 0; i < UAC_MAX_RATES; i++) {
>> +				srate = srates[i];
>> +				if (srate == 0)
>> +					break;
>> +
>> +				rs.r[wNumSubRanges].dMIN = cpu_to_le32(srate);
>> +				rs.r[wNumSubRanges].dMAX = cpu_to_le32(srate);
>> +				rs.r[wNumSubRanges].dRES = 0;
>> +				wNumSubRanges++;
>> +				dev_dbg(&agdev->gadget->dev,
>> +					"%s(): clk %d: rate ID %d: %d\n",
>> +					__func__, entity_id, wNumSubRanges, srate);
>> +			}
>> +			rs.wNumSubRanges = cpu_to_le16(wNumSubRanges);
>> +			value = min_t(unsigned int, w_length, ranges_size(rs));
>> +			dev_dbg(&agdev->gadget->dev, "%s(): sending %d rates, size %d\n",
>> +				__func__, rs.wNumSubRanges, value);
>> +			memcpy(req->buf, &rs, value);
>>   		} else {
>>   			dev_err(&agdev->gadget->dev,
>>   				"%s:%d control_selector=%d TODO!\n",
>> @@ -1582,6 +1616,28 @@ ac_rq_in(struct usb_function *fn, const struct usb_ctrlrequest *cr)
>>   		return -EOPNOTSUPP;
>>   }
>>   
>> +static void uac2_cs_control_sam_freq(struct usb_ep *ep, struct usb_request *req)
>> +{
>> +	struct usb_function *fn = ep->driver_data;
>> +	struct g_audio *agdev = func_to_g_audio(fn);
>> +	struct f_uac2 *uac2 = func_to_uac2(fn);
>> +	struct f_uac2_opts *opts = g_audio_to_uac2_opts(agdev);
>> +	u32 val;
>> +
>> +	if (req->actual != 4)
>> +		return;
>> +
>> +	val = le32_to_cpu(*((u32 *)req->buf));
>> +	dev_dbg(&agdev->gadget->dev, "%s val: %d.\n", __func__, val);
>> +	if (uac2->ctl_id == USB_IN_CLK_ID) {
>> +		opts->p_srate = val;
> 
> Don't you need to hold opts->lock to change this?
> I'm not sure opts should be changed here though - that's the setup phase
> and this is "current state", so shouldn't it move to struct f_uac2?

OK. I moved the current p_srate/c_srate from struct opts to f_uac2, 
initialized with first value of opts->p_srates/c_srates[0] in 
afunc_bind. The struct f_uac2 has no lock yet. Should I add the lock 
mutex to f_uac2 and be locking f_uac2 access here in 
uac2_cs_control_sam_freq?

> 
>> +		u_audio_set_playback_srate(agdev, opts->p_srate);
>> +	} else if (uac2->ctl_id == USB_OUT_CLK_ID) {
>> +		opts->c_srate = val;
>> +		u_audio_set_capture_srate(agdev, opts->c_srate);
>> +	}
>> +}
>> +
>>   static void
>>   out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req)
>>   {
>> @@ -1633,6 +1689,7 @@ out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req)
>>   static int
>>   out_rq_cur(struct usb_function *fn, const struct usb_ctrlrequest *cr)
>>   {
>> +	struct usb_composite_dev *cdev = fn->config->cdev;
>>   	struct usb_request *req = fn->config->cdev->req;
>>   	struct g_audio *agdev = func_to_g_audio(fn);
>>   	struct f_uac2_opts *opts = g_audio_to_uac2_opts(agdev);
>> @@ -1642,10 +1699,17 @@ out_rq_cur(struct usb_function *fn, const struct usb_ctrlrequest *cr)
>>   	u16 w_value = le16_to_cpu(cr->wValue);
>>   	u8 entity_id = (w_index >> 8) & 0xff;
>>   	u8 control_selector = w_value >> 8;
>> +	u8 clock_id = w_index >> 8;
>>   
>>   	if ((entity_id == USB_IN_CLK_ID) || (entity_id == USB_OUT_CLK_ID)) {
>> -		if (control_selector == UAC2_CS_CONTROL_SAM_FREQ)
>> +		if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) {
>> +			dev_dbg(&agdev->gadget->dev,
>> +				"control_selector UAC2_CS_CONTROL_SAM_FREQ, clock: %d\n", clock_id);
>> +			cdev->gadget->ep0->driver_data = fn;
>> +			uac2->ctl_id = clock_id;
>> +			req->complete = uac2_cs_control_sam_freq;
>>   			return w_length;
>> +		}
>>   	} else if ((FUIN_EN(opts) && (entity_id == USB_IN_FU_ID)) ||
>>   			(FUOUT_EN(opts) && (entity_id == USB_OUT_FU_ID))) {
>>   		memcpy(&uac2->setup_cr, cr, sizeof(*cr));
>> @@ -1839,10 +1903,10 @@ end:									\
>>   CONFIGFS_ATTR(f_uac2_opts_, name)
>>   
>>   UAC2_ATTRIBUTE(u32, p_chmask);
>> -UAC2_ATTRIBUTE(u32, p_srate);
>> +UAC_RATE2_ATTRIBUTE(p_srate);
> 
> UAC2_RATE_ATTRIBUTE() to match the pattern of the other values here?

Fixed

> 
>>   UAC2_ATTRIBUTE(u32, p_ssize);
>>   UAC2_ATTRIBUTE(u32, c_chmask);
>> -UAC2_ATTRIBUTE(u32, c_srate);
>> +UAC_RATE2_ATTRIBUTE(c_srate);
>>   UAC2_ATTRIBUTE_SYNC(c_sync);
>>   UAC2_ATTRIBUTE(u32, c_ssize);
>>   UAC2_ATTRIBUTE(u32, req_number);
>> @@ -1915,9 +1979,11 @@ static struct usb_function_instance *afunc_alloc_inst(void)
>>   				    &f_uac2_func_type);
>>   
>>   	opts->p_chmask = UAC2_DEF_PCHMASK;
>> +	opts->p_srates[0] = UAC2_DEF_PSRATE;
>>   	opts->p_srate = UAC2_DEF_PSRATE;
>>   	opts->p_ssize = UAC2_DEF_PSSIZE;
>>   	opts->c_chmask = UAC2_DEF_CCHMASK;
>> +	opts->c_srates[0] = UAC2_DEF_CSRATE;
>>   	opts->c_srate = UAC2_DEF_CSRATE;
>>   	opts->c_ssize = UAC2_DEF_CSSIZE;
>>   	opts->c_sync = UAC2_DEF_CSYNC;
>> diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h
>> index e0c8e3513bfd..8058217322f8 100644
>> --- a/drivers/usb/gadget/function/u_uac2.h
>> +++ b/drivers/usb/gadget/function/u_uac2.h
>> @@ -14,6 +14,7 @@
>>   #define U_UAC2_H
>>   
>>   #include <linux/usb/composite.h>
>> +#include "uac_common.h"
>>   
>>   #define UAC2_DEF_PCHMASK 0x3
>>   #define UAC2_DEF_PSRATE 48000
>> @@ -35,9 +36,11 @@
>>   struct f_uac2_opts {
>>   	struct usb_function_instance	func_inst;
>>   	int				p_chmask;
>> +	int				p_srates[UAC_MAX_RATES];
>>   	int				p_srate;
>>   	int				p_ssize;
>>   	int				c_chmask;
>> +	int				c_srates[UAC_MAX_RATES];
>>   	int				c_srate;
>>   	int				c_ssize;
>>   	int				c_sync;
>> @@ -62,4 +65,63 @@ struct f_uac2_opts {
>>   	int				refcnt;
>>   };
>>   
>> +#define UAC_RATE2_ATTRIBUTE(name)					\
> 
> Why define this in the header?  UAC2_ATTRIBUTE() is in the .c file and
> this is only used in one place so no need for it to be in the .h.

Moved, renamed to UAC2_RATE_ATTRIBUTE.

Thanks,

Pavel.
John Keeping Jan. 5, 2022, 12:44 p.m. UTC | #3 
On Wed, Jan 05, 2022 at 01:20:01PM +0100, Pavel Hofman wrote:
> Dne 04. 01. 22 v 16:33 John Keeping napsal(a):
> > On Wed, Dec 22, 2021 at 11:01:16AM +0100, Pavel Hofman wrote:
> > > 
> > > Dne 21. 12. 21 v 12:59 John Keeping napsal(a):
> > > > On Mon, Dec 20, 2021 at 10:11:22PM +0100, Pavel Hofman wrote:
> > > > > From: Julian Scheel <julian@jusst.de>
> > > > > 
> > > > > A list of sampling rates can be specified via configfs. All enabled
> > > > > sampling rates are sent to the USB host on request. When the host
> > > > > selects a sampling rate the internal active rate is updated.
> > > > > 
> > > > > Config strings with single value stay compatible with the previous version.
> > > > > 
> > > > > Multiple samplerates passed as configuration arrays to g_audio module
> > > > > when built for f_uac2.
> > > > > 
> > > > > Signed-off-by: Julian Scheel <julian@jusst.de>
> > > > > Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>
> > > > > ---
> > [snip]
> > > > >    };
> > > > >    static inline struct f_uac2 *func_to_uac2(struct usb_function *f)
> > > > > @@ -166,7 +167,7 @@ static struct uac_clock_source_descriptor in_clk_src_desc = {
> > > > >    	.bDescriptorSubtype = UAC2_CLOCK_SOURCE,
> > > > >    	/* .bClockID = DYNAMIC */
> > > > >    	.bmAttributes = UAC_CLOCK_SOURCE_TYPE_INT_FIXED,
> > > > > -	.bmControls = (CONTROL_RDONLY << CLK_FREQ_CTRL),
> > > > > +	.bmControls = (CONTROL_RDWR << CLK_FREQ_CTRL),
> > > > >    	.bAssocTerminal = 0,
> > > > >    };
> > > > > @@ -178,7 +179,7 @@ static struct uac_clock_source_descriptor out_clk_src_desc = {
> > > > >    	.bDescriptorSubtype = UAC2_CLOCK_SOURCE,
> > > > >    	/* .bClockID = DYNAMIC */
> > > > >    	.bmAttributes = UAC_CLOCK_SOURCE_TYPE_INT_FIXED,
> > > > > -	.bmControls = (CONTROL_RDONLY << CLK_FREQ_CTRL),
> > > > > +	.bmControls = (CONTROL_RDWR << CLK_FREQ_CTRL),
> > > > >    	.bAssocTerminal = 0,
> > > > >    };
> > > > > @@ -635,12 +636,32 @@ struct cntrl_cur_lay3 {
> > > > >    };
> > > > >    struct cntrl_range_lay3 {
> > > > > -	__le16	wNumSubRanges;
> > > > >    	__le32	dMIN;
> > > > >    	__le32	dMAX;
> > > > >    	__le32	dRES;
> > > > >    } __packed;
> > > > > +#define ranges_size(c) (sizeof(c.wNumSubRanges) + c.wNumSubRanges \
> > > > > +		* sizeof(struct cntrl_ranges_lay3))
> > > > > +
> > > > > +struct cntrl_ranges_lay3 {
> > > > > +	__u16	wNumSubRanges;
> > > > > +	struct cntrl_range_lay3 r[UAC_MAX_RATES];
> > > > > +} __packed;
> > > > 
> > > > These structures are now inconsistent between cntrl_range_lay2 and
> > > > cntrl_range_lay3.  Would it be better to make these flex arrays?  I
> > > > guess that will make the code that uses it more complicated, but at the
> > > > moment it looks like these are trying to be generic while in reality
> > > > being quite specific to the one place that uses them at the moment.
> > > 
> > > I am afraid I do not know exactly how to do that. Please can you post an
> > > example? The rate control requires u32 (u16 is too small). Thanks a lot.
> > 
> > After the change in this patch, we end up with:
> > 
> > 	struct cntrl_range_lay2 {
> > 		__le16	wNumSubRanges;
> > 		__le16	wMIN;
> > 		__le16	wMAX;
> > 		__le16	wRES;
> > 	} __packed;
> > 
> > 	struct cntrl_range_lay3 {
> > 		__le32	dMIN;
> > 		__le32	dMAX;
> > 		__le32	dRES;
> > 	} __packed;
> > 
> > so there are two structures with similar names but totally different
> > structure, which I think risks confusion in the future.
> > 
> > I wonder if DECLARE_UAC2_FEATURE_UNIT_DESCRIPTOR in linux/usb/audio-v2.h
> > provides inspiration here, so potentially something like:
> > 
> > 	#define DECLARE_UAC2_CNTRL_RANGE_LAY3(n)	\
> > 		struct uac2_cntrl_range_lay3_##n {	\
> > 			__le16 wNumSubRanges;		\
> > 			struct cntrl_range_le32 r[n];	\
> > 		} __packed;
> > 
> > 	DECLARE_UAC2_CNTRL_RANGE_LAY3(UAC_MAX_RATES);
> 
> Thanks, I will try to follow your suggestion in the next patchset version.
> 
> > 
> > > > > +static int get_max_srate(const int *srates)
> > > > > +{
> > > > > +	int i, max_srate = 0;
> > > > > +
> > > > > +	for (i = 0; i < UAC_MAX_RATES; i++) {
> > > > > +		if (srates[i] == 0)
> > > > > +			break;
> > > > > +		if (srates[i] > max_srate)
> > > > > +			max_srate = srates[i];
> > > > > +	}
> > > > > +	return max_srate;
> > > > > +}
> > > > > +
> > > > >    static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
> > > > >    	struct usb_endpoint_descriptor *ep_desc,
> > > > >    	enum usb_device_speed speed, bool is_playback)
> > > > > @@ -667,11 +688,11 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
> > > > >    	if (is_playback) {
> > > > >    		chmask = uac2_opts->p_chmask;
> > > > > -		srate = uac2_opts->p_srate;
> > > > > +		srate = get_max_srate(uac2_opts->p_srates);
> > > > >    		ssize = uac2_opts->p_ssize;
> > > > >    	} else {
> > > > >    		chmask = uac2_opts->c_chmask;
> > > > > -		srate = uac2_opts->c_srate;
> > > > > +		srate = get_max_srate(uac2_opts->c_srates);
> > > > >    		ssize = uac2_opts->c_ssize;
> > > > >    	}
> > > > > @@ -912,10 +933,10 @@ static int afunc_validate_opts(struct g_audio *agdev, struct device *dev)
> > > > >    	} else if ((opts->c_ssize < 1) || (opts->c_ssize > 4)) {
> > > > >    		dev_err(dev, "Error: incorrect capture sample size\n");
> > > > >    		return -EINVAL;
> > > > > -	} else if (!opts->p_srate) {
> > > > > +	} else if (!opts->p_srates[0]) {
> > > > >    		dev_err(dev, "Error: incorrect playback sampling rate\n");
> > > > >    		return -EINVAL;
> > > > > -	} else if (!opts->c_srate) {
> > > > > +	} else if (!opts->c_srates[0]) {
> > > > >    		dev_err(dev, "Error: incorrect capture sampling rate\n");
> > > > >    		return -EINVAL;
> > > > >    	}
> > > > > @@ -1210,7 +1231,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
> > > > >    	agdev->params.p_chmask = uac2_opts->p_chmask;
> > > > >    	agdev->params.p_srate = uac2_opts->p_srate;
> > > > > -	agdev->params.p_srates[0] = uac2_opts->p_srate;
> > > > > +	memcpy(agdev->params.p_srates, uac2_opts->p_srates,
> > > > > +			sizeof(agdev->params.p_srates));
> > > > >    	agdev->params.p_ssize = uac2_opts->p_ssize;
> > > > >    	if (FUIN_EN(uac2_opts)) {
> > > > >    		agdev->params.p_fu.id = USB_IN_FU_ID;
> > > > > @@ -1222,7 +1244,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
> > > > >    	}
> > > > >    	agdev->params.c_chmask = uac2_opts->c_chmask;
> > > > >    	agdev->params.c_srate = uac2_opts->c_srate;
> > > > > -	agdev->params.c_srates[0] = uac2_opts->c_srate;
> > > > > +	memcpy(agdev->params.c_srates, uac2_opts->c_srates,
> > > > > +			sizeof(agdev->params.c_srates));
> > > > >    	agdev->params.c_ssize = uac2_opts->c_ssize;
> > > > >    	if (FUOUT_EN(uac2_opts)) {
> > > > >    		agdev->params.c_fu.id = USB_OUT_FU_ID;
> > > > > @@ -1502,28 +1525,39 @@ in_rq_range(struct usb_function *fn, const struct usb_ctrlrequest *cr)
> > > > >    	u8 entity_id = (w_index >> 8) & 0xff;
> > > > >    	u8 control_selector = w_value >> 8;
> > > > >    	int value = -EOPNOTSUPP;
> > > > > -	int p_srate, c_srate;
> > > > > -
> > > > > -	p_srate = opts->p_srate;
> > > > > -	c_srate = opts->c_srate;
> > > > >    	if ((entity_id == USB_IN_CLK_ID) || (entity_id == USB_OUT_CLK_ID)) {
> > > > >    		if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) {
> > > > > -			struct cntrl_range_lay3 r;
> > > > > +			struct cntrl_ranges_lay3 rs;
> > > > > +			int i;
> > > > > +			int wNumSubRanges = 0;
> > > > > +			int srate;
> > > > > +			int *srates;
> > > > >    			if (entity_id == USB_IN_CLK_ID)
> > > > > -				r.dMIN = cpu_to_le32(p_srate);
> > > > > +				srates = opts->p_srates;
> > > > >    			else if (entity_id == USB_OUT_CLK_ID)
> > > > > -				r.dMIN = cpu_to_le32(c_srate);
> > > > > +				srates = opts->c_srates;
> > > > >    			else
> > > > >    				return -EOPNOTSUPP;
> > > > > -
> > > > > -			r.dMAX = r.dMIN;
> > > > > -			r.dRES = 0;
> > > > > -			r.wNumSubRanges = cpu_to_le16(1);
> > > > > -
> > > > > -			value = min_t(unsigned int, w_length, sizeof(r));
> > > > > -			memcpy(req->buf, &r, value);
> > > > > +			for (i = 0; i < UAC_MAX_RATES; i++) {
> > > > > +				srate = srates[i];
> > > > > +				if (srate == 0)
> > > > > +					break;
> > > > > +
> > > > > +				rs.r[wNumSubRanges].dMIN = cpu_to_le32(srate);
> > > > > +				rs.r[wNumSubRanges].dMAX = cpu_to_le32(srate);
> > > > > +				rs.r[wNumSubRanges].dRES = 0;
> > > > > +				wNumSubRanges++;
> > > > > +				dev_dbg(&agdev->gadget->dev,
> > > > > +					"%s(): clk %d: rate ID %d: %d\n",
> > > > > +					__func__, entity_id, wNumSubRanges, srate);
> > > > > +			}
> > > > > +			rs.wNumSubRanges = cpu_to_le16(wNumSubRanges);
> > > > > +			value = min_t(unsigned int, w_length, ranges_size(rs));
> > > > > +			dev_dbg(&agdev->gadget->dev, "%s(): sending %d rates, size %d\n",
> > > > > +				__func__, rs.wNumSubRanges, value);
> > > > > +			memcpy(req->buf, &rs, value);
> > > > >    		} else {
> > > > >    			dev_err(&agdev->gadget->dev,
> > > > >    				"%s:%d control_selector=%d TODO!\n",
> > > > > @@ -1582,6 +1616,28 @@ ac_rq_in(struct usb_function *fn, const struct usb_ctrlrequest *cr)
> > > > >    		return -EOPNOTSUPP;
> > > > >    }
> > > > > +static void uac2_cs_control_sam_freq(struct usb_ep *ep, struct usb_request *req)
> > > > > +{
> > > > > +	struct usb_function *fn = ep->driver_data;
> > > > > +	struct g_audio *agdev = func_to_g_audio(fn);
> > > > > +	struct f_uac2 *uac2 = func_to_uac2(fn);
> > > > > +	struct f_uac2_opts *opts = g_audio_to_uac2_opts(agdev);
> > > > > +	u32 val;
> > > > > +
> > > > > +	if (req->actual != 4)
> > > > > +		return;
> > > > > +
> > > > > +	val = le32_to_cpu(*((u32 *)req->buf));
> > > > > +	dev_dbg(&agdev->gadget->dev, "%s val: %d.\n", __func__, val);
> > > > > +	if (uac2->ctl_id == USB_IN_CLK_ID) {
> > > > > +		opts->p_srate = val;
> > > > 
> > > > Don't you need to hold opts->lock to change this?
> > > > I'm not sure opts should be changed here though - that's the setup phase
> > > > and this is "current state", so shouldn't it move to struct f_uac2?
> > > 
> > > OK. I moved the current p_srate/c_srate from struct opts to f_uac2,
> > > initialized with first value of opts->p_srates/c_srates[0] in afunc_bind.
> > > The struct f_uac2 has no lock yet. Should I add the lock mutex to f_uac2 and
> > > be locking f_uac2 access here in uac2_cs_control_sam_freq?
> > 
> > Could we move this into struct uac_rtd_params and use the existing lock
> > there to guard it?
> > 
> > It would need accessor functions as that structure's local to u_audio.c,
> > but there's already u_audio_set_playback_srate() so that isn't a big
> > change.
> 
> I have already moved p_/c_srate from uac_params to uac_rtd_params in
> u_audio.c in the next version of the patchset. But IIUC the currently
> selected playback/capture rate is required within f_uac2 too, in in_rq_cur()
> in:
> 
> if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) {
> 	...
> 	if (entity_id == USB_IN_CLK_ID)
> 		c.dCUR = cpu_to_le32(p_srate);
> 	else if (entity_id == USB_OUT_CLK_ID)
> 		c.dCUR = cpu_to_le32(c_srate);
> 	...
> }

Can this can be u_audio_get_playback_srate(agdev) (and equivalent for
capture)?


John
Pavel Hofman Jan. 5, 2022, 2:05 p.m. UTC | #4 
Dne 05. 01. 22 v 13:44 John Keeping napsal(a):
> On Wed, Jan 05, 2022 at 01:20:01PM +0100, Pavel Hofman wrote:
>> Dne 04. 01. 22 v 16:33 John Keeping napsal(a):
>>> On Wed, Dec 22, 2021 at 11:01:16AM +0100, Pavel Hofman wrote:
>>>>
>>>> Dne 21. 12. 21 v 12:59 John Keeping napsal(a):
>>>>> On Mon, Dec 20, 2021 at 10:11:22PM +0100, Pavel Hofman wrote:
>>>>>> From: Julian Scheel <julian@jusst.de>
>>>>>>
>>>>>> A list of sampling rates can be specified via configfs. All enabled
>>>>>> sampling rates are sent to the USB host on request. When the host
>>>>>> selects a sampling rate the internal active rate is updated.
>>>>>>
>>>>>> Config strings with single value stay compatible with the previous version.
>>>>>>
>>>>>> Multiple samplerates passed as configuration arrays to g_audio module
>>>>>> when built for f_uac2.
>>>>>>
>>>>>> Signed-off-by: Julian Scheel <julian@jusst.de>
>>>>>> Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>
>>>>>> ---
>>> [snip]
>>>>>>     };
>>>>>>     static inline struct f_uac2 *func_to_uac2(struct usb_function *f)
>>>>>> @@ -166,7 +167,7 @@ static struct uac_clock_source_descriptor in_clk_src_desc = {
>>>>>>     	.bDescriptorSubtype = UAC2_CLOCK_SOURCE,
>>>>>>     	/* .bClockID = DYNAMIC */
>>>>>>     	.bmAttributes = UAC_CLOCK_SOURCE_TYPE_INT_FIXED,
>>>>>> -	.bmControls = (CONTROL_RDONLY << CLK_FREQ_CTRL),
>>>>>> +	.bmControls = (CONTROL_RDWR << CLK_FREQ_CTRL),
>>>>>>     	.bAssocTerminal = 0,
>>>>>>     };
>>>>>> @@ -178,7 +179,7 @@ static struct uac_clock_source_descriptor out_clk_src_desc = {
>>>>>>     	.bDescriptorSubtype = UAC2_CLOCK_SOURCE,
>>>>>>     	/* .bClockID = DYNAMIC */
>>>>>>     	.bmAttributes = UAC_CLOCK_SOURCE_TYPE_INT_FIXED,
>>>>>> -	.bmControls = (CONTROL_RDONLY << CLK_FREQ_CTRL),
>>>>>> +	.bmControls = (CONTROL_RDWR << CLK_FREQ_CTRL),
>>>>>>     	.bAssocTerminal = 0,
>>>>>>     };
>>>>>> @@ -635,12 +636,32 @@ struct cntrl_cur_lay3 {
>>>>>>     };
>>>>>>     struct cntrl_range_lay3 {
>>>>>> -	__le16	wNumSubRanges;
>>>>>>     	__le32	dMIN;
>>>>>>     	__le32	dMAX;
>>>>>>     	__le32	dRES;
>>>>>>     } __packed;
>>>>>> +#define ranges_size(c) (sizeof(c.wNumSubRanges) + c.wNumSubRanges \
>>>>>> +		* sizeof(struct cntrl_ranges_lay3))
>>>>>> +
>>>>>> +struct cntrl_ranges_lay3 {
>>>>>> +	__u16	wNumSubRanges;
>>>>>> +	struct cntrl_range_lay3 r[UAC_MAX_RATES];
>>>>>> +} __packed;
>>>>>
>>>>> These structures are now inconsistent between cntrl_range_lay2 and
>>>>> cntrl_range_lay3.  Would it be better to make these flex arrays?  I
>>>>> guess that will make the code that uses it more complicated, but at the
>>>>> moment it looks like these are trying to be generic while in reality
>>>>> being quite specific to the one place that uses them at the moment.
>>>>
>>>> I am afraid I do not know exactly how to do that. Please can you post an
>>>> example? The rate control requires u32 (u16 is too small). Thanks a lot.
>>>
>>> After the change in this patch, we end up with:
>>>
>>> 	struct cntrl_range_lay2 {
>>> 		__le16	wNumSubRanges;
>>> 		__le16	wMIN;
>>> 		__le16	wMAX;
>>> 		__le16	wRES;
>>> 	} __packed;
>>>
>>> 	struct cntrl_range_lay3 {
>>> 		__le32	dMIN;
>>> 		__le32	dMAX;
>>> 		__le32	dRES;
>>> 	} __packed;
>>>
>>> so there are two structures with similar names but totally different
>>> structure, which I think risks confusion in the future.
>>>
>>> I wonder if DECLARE_UAC2_FEATURE_UNIT_DESCRIPTOR in linux/usb/audio-v2.h
>>> provides inspiration here, so potentially something like:
>>>
>>> 	#define DECLARE_UAC2_CNTRL_RANGE_LAY3(n)	\
>>> 		struct uac2_cntrl_range_lay3_##n {	\
>>> 			__le16 wNumSubRanges;		\
>>> 			struct cntrl_range_le32 r[n];	\
>>> 		} __packed;
>>>
>>> 	DECLARE_UAC2_CNTRL_RANGE_LAY3(UAC_MAX_RATES);
>>
>> Thanks, I will try to follow your suggestion in the next patchset version.
>>
>>>
>>>>>> +static int get_max_srate(const int *srates)
>>>>>> +{
>>>>>> +	int i, max_srate = 0;
>>>>>> +
>>>>>> +	for (i = 0; i < UAC_MAX_RATES; i++) {
>>>>>> +		if (srates[i] == 0)
>>>>>> +			break;
>>>>>> +		if (srates[i] > max_srate)
>>>>>> +			max_srate = srates[i];
>>>>>> +	}
>>>>>> +	return max_srate;
>>>>>> +}
>>>>>> +
>>>>>>     static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>>>>>>     	struct usb_endpoint_descriptor *ep_desc,
>>>>>>     	enum usb_device_speed speed, bool is_playback)
>>>>>> @@ -667,11 +688,11 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>>>>>>     	if (is_playback) {
>>>>>>     		chmask = uac2_opts->p_chmask;
>>>>>> -		srate = uac2_opts->p_srate;
>>>>>> +		srate = get_max_srate(uac2_opts->p_srates);
>>>>>>     		ssize = uac2_opts->p_ssize;
>>>>>>     	} else {
>>>>>>     		chmask = uac2_opts->c_chmask;
>>>>>> -		srate = uac2_opts->c_srate;
>>>>>> +		srate = get_max_srate(uac2_opts->c_srates);
>>>>>>     		ssize = uac2_opts->c_ssize;
>>>>>>     	}
>>>>>> @@ -912,10 +933,10 @@ static int afunc_validate_opts(struct g_audio *agdev, struct device *dev)
>>>>>>     	} else if ((opts->c_ssize < 1) || (opts->c_ssize > 4)) {
>>>>>>     		dev_err(dev, "Error: incorrect capture sample size\n");
>>>>>>     		return -EINVAL;
>>>>>> -	} else if (!opts->p_srate) {
>>>>>> +	} else if (!opts->p_srates[0]) {
>>>>>>     		dev_err(dev, "Error: incorrect playback sampling rate\n");
>>>>>>     		return -EINVAL;
>>>>>> -	} else if (!opts->c_srate) {
>>>>>> +	} else if (!opts->c_srates[0]) {
>>>>>>     		dev_err(dev, "Error: incorrect capture sampling rate\n");
>>>>>>     		return -EINVAL;
>>>>>>     	}
>>>>>> @@ -1210,7 +1231,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
>>>>>>     	agdev->params.p_chmask = uac2_opts->p_chmask;
>>>>>>     	agdev->params.p_srate = uac2_opts->p_srate;
>>>>>> -	agdev->params.p_srates[0] = uac2_opts->p_srate;
>>>>>> +	memcpy(agdev->params.p_srates, uac2_opts->p_srates,
>>>>>> +			sizeof(agdev->params.p_srates));
>>>>>>     	agdev->params.p_ssize = uac2_opts->p_ssize;
>>>>>>     	if (FUIN_EN(uac2_opts)) {
>>>>>>     		agdev->params.p_fu.id = USB_IN_FU_ID;
>>>>>> @@ -1222,7 +1244,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
>>>>>>     	}
>>>>>>     	agdev->params.c_chmask = uac2_opts->c_chmask;
>>>>>>     	agdev->params.c_srate = uac2_opts->c_srate;
>>>>>> -	agdev->params.c_srates[0] = uac2_opts->c_srate;
>>>>>> +	memcpy(agdev->params.c_srates, uac2_opts->c_srates,
>>>>>> +			sizeof(agdev->params.c_srates));
>>>>>>     	agdev->params.c_ssize = uac2_opts->c_ssize;
>>>>>>     	if (FUOUT_EN(uac2_opts)) {
>>>>>>     		agdev->params.c_fu.id = USB_OUT_FU_ID;
>>>>>> @@ -1502,28 +1525,39 @@ in_rq_range(struct usb_function *fn, const struct usb_ctrlrequest *cr)
>>>>>>     	u8 entity_id = (w_index >> 8) & 0xff;
>>>>>>     	u8 control_selector = w_value >> 8;
>>>>>>     	int value = -EOPNOTSUPP;
>>>>>> -	int p_srate, c_srate;
>>>>>> -
>>>>>> -	p_srate = opts->p_srate;
>>>>>> -	c_srate = opts->c_srate;
>>>>>>     	if ((entity_id == USB_IN_CLK_ID) || (entity_id == USB_OUT_CLK_ID)) {
>>>>>>     		if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) {
>>>>>> -			struct cntrl_range_lay3 r;
>>>>>> +			struct cntrl_ranges_lay3 rs;
>>>>>> +			int i;
>>>>>> +			int wNumSubRanges = 0;
>>>>>> +			int srate;
>>>>>> +			int *srates;
>>>>>>     			if (entity_id == USB_IN_CLK_ID)
>>>>>> -				r.dMIN = cpu_to_le32(p_srate);
>>>>>> +				srates = opts->p_srates;
>>>>>>     			else if (entity_id == USB_OUT_CLK_ID)
>>>>>> -				r.dMIN = cpu_to_le32(c_srate);
>>>>>> +				srates = opts->c_srates;
>>>>>>     			else
>>>>>>     				return -EOPNOTSUPP;
>>>>>> -
>>>>>> -			r.dMAX = r.dMIN;
>>>>>> -			r.dRES = 0;
>>>>>> -			r.wNumSubRanges = cpu_to_le16(1);
>>>>>> -
>>>>>> -			value = min_t(unsigned int, w_length, sizeof(r));
>>>>>> -			memcpy(req->buf, &r, value);
>>>>>> +			for (i = 0; i < UAC_MAX_RATES; i++) {
>>>>>> +				srate = srates[i];
>>>>>> +				if (srate == 0)
>>>>>> +					break;
>>>>>> +
>>>>>> +				rs.r[wNumSubRanges].dMIN = cpu_to_le32(srate);
>>>>>> +				rs.r[wNumSubRanges].dMAX = cpu_to_le32(srate);
>>>>>> +				rs.r[wNumSubRanges].dRES = 0;
>>>>>> +				wNumSubRanges++;
>>>>>> +				dev_dbg(&agdev->gadget->dev,
>>>>>> +					"%s(): clk %d: rate ID %d: %d\n",
>>>>>> +					__func__, entity_id, wNumSubRanges, srate);
>>>>>> +			}
>>>>>> +			rs.wNumSubRanges = cpu_to_le16(wNumSubRanges);
>>>>>> +			value = min_t(unsigned int, w_length, ranges_size(rs));
>>>>>> +			dev_dbg(&agdev->gadget->dev, "%s(): sending %d rates, size %d\n",
>>>>>> +				__func__, rs.wNumSubRanges, value);
>>>>>> +			memcpy(req->buf, &rs, value);
>>>>>>     		} else {
>>>>>>     			dev_err(&agdev->gadget->dev,
>>>>>>     				"%s:%d control_selector=%d TODO!\n",
>>>>>> @@ -1582,6 +1616,28 @@ ac_rq_in(struct usb_function *fn, const struct usb_ctrlrequest *cr)
>>>>>>     		return -EOPNOTSUPP;
>>>>>>     }
>>>>>> +static void uac2_cs_control_sam_freq(struct usb_ep *ep, struct usb_request *req)
>>>>>> +{
>>>>>> +	struct usb_function *fn = ep->driver_data;
>>>>>> +	struct g_audio *agdev = func_to_g_audio(fn);
>>>>>> +	struct f_uac2 *uac2 = func_to_uac2(fn);
>>>>>> +	struct f_uac2_opts *opts = g_audio_to_uac2_opts(agdev);
>>>>>> +	u32 val;
>>>>>> +
>>>>>> +	if (req->actual != 4)
>>>>>> +		return;
>>>>>> +
>>>>>> +	val = le32_to_cpu(*((u32 *)req->buf));
>>>>>> +	dev_dbg(&agdev->gadget->dev, "%s val: %d.\n", __func__, val);
>>>>>> +	if (uac2->ctl_id == USB_IN_CLK_ID) {
>>>>>> +		opts->p_srate = val;
>>>>>
>>>>> Don't you need to hold opts->lock to change this?
>>>>> I'm not sure opts should be changed here though - that's the setup phase
>>>>> and this is "current state", so shouldn't it move to struct f_uac2?
>>>>
>>>> OK. I moved the current p_srate/c_srate from struct opts to f_uac2,
>>>> initialized with first value of opts->p_srates/c_srates[0] in afunc_bind.
>>>> The struct f_uac2 has no lock yet. Should I add the lock mutex to f_uac2 and
>>>> be locking f_uac2 access here in uac2_cs_control_sam_freq?
>>>
>>> Could we move this into struct uac_rtd_params and use the existing lock
>>> there to guard it?
>>>
>>> It would need accessor functions as that structure's local to u_audio.c,
>>> but there's already u_audio_set_playback_srate() so that isn't a big
>>> change.
>>
>> I have already moved p_/c_srate from uac_params to uac_rtd_params in
>> u_audio.c in the next version of the patchset. But IIUC the currently
>> selected playback/capture rate is required within f_uac2 too, in in_rq_cur()
>> in:
>>
>> if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) {
>> 	...
>> 	if (entity_id == USB_IN_CLK_ID)
>> 		c.dCUR = cpu_to_le32(p_srate);
>> 	else if (entity_id == USB_OUT_CLK_ID)
>> 		c.dCUR = cpu_to_le32(c_srate);
>> 	...
>> }
> 
> Can this can be u_audio_get_playback_srate(agdev) (and equivalent for
> capture)?
> 

We certainly can add the rate getter API call. But I do not know whether 
master should store and query slave for data required for proper 
operation of the master. IMO f_uac2 should be in control of the params 
requested by the real master - the USB host.

Currently the gadget-side app can open the gadget alsa device at the 
last-used rate (or channel count if/when multiple channels config is 
implemented) at any time, without knowing what params the user will 
actually request. IMO in some use cases the gadget alsa device should 
not allow opening before the host starts playback/capture, i.e. before 
the user actually requests what params to use. This would require more 
patches, nothing critical now and not in my pipeline. I (will) handle 
this issue with the side-channel gaudio_ctl application controller which 
receives the currently required params (srate as of now) via ctl 
notifications.

The question is whether it's correct for f_uac2 to ask u_audio about 
current rate at any time, even when u_audio is inactive and the only 
party doing something with the rate is actually f_uac2.

Maybe the ultimate data flow should be the other way round. But IIUC 
u_audio currently does not have any hooks back to f_uac1/2. The USB 
functions store all the params to uac_param for u_audio to use.

But I can add the u_audio_get_playback/capture_srate API method (in a 
separate patch, I guess) to avoid keeping the same values in two places, 
if found convenient.

I think we all should discuss our use cases in order to delineate the 
path along which the audio gadget should evolve.

Thanks,

Pavel.
Pavel Hofman Jan. 6, 2022, 8:45 a.m. UTC | #5 
Dne 05. 01. 22 v 15:05 Pavel Hofman napsal(a):
> Dne 05. 01. 22 v 13:44 John Keeping napsal(a):
>> On Wed, Jan 05, 2022 at 01:20:01PM +0100, Pavel Hofman wrote:
>>> Dne 04. 01. 22 v 16:33 John Keeping napsal(a):
>>>> On Wed, Dec 22, 2021 at 11:01:16AM +0100, Pavel Hofman wrote:
>>>>> 
>>>>> Dne 21. 12. 21 v 12:59 John Keeping napsal(a):
>>>>>> On Mon, Dec 20, 2021 at 10:11:22PM +0100, Pavel Hofman
>>>>>> wrote:
>>>>>>> From: Julian Scheel <julian@jusst.de>
>>>>>>> 
>>>>>>> A list of sampling rates can be specified via configfs.
>>>>>>> All enabled sampling rates are sent to the USB host on
>>>>>>> request. When the host selects a sampling rate the
>>>>>>> internal active rate is updated.
>>>>>>> 
>>>>>>> Config strings with single value stay compatible with the
>>>>>>>  previous version.
>>>>>>> 
>>>>>>> Multiple samplerates passed as configuration arrays to
>>>>>>> g_audio module when built for f_uac2.
>>>>>>> 
>>>>>>> Signed-off-by: Julian Scheel <julian@jusst.de> 
>>>>>>> Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com> 
>>>>>>> ---
>>>> [snip]
>>>>>>> }; static inline struct f_uac2 *func_to_uac2(struct
>>>>>>> usb_function *f) @@ -166,7 +167,7 @@ static struct
>>>>>>> uac_clock_source_descriptor in_clk_src_desc = { 
>>>>>>> .bDescriptorSubtype = UAC2_CLOCK_SOURCE, /* .bClockID =
>>>>>>> DYNAMIC */ .bmAttributes =
>>>>>>> UAC_CLOCK_SOURCE_TYPE_INT_FIXED, -    .bmControls =
>>>>>>> (CONTROL_RDONLY << CLK_FREQ_CTRL), +    .bmControls =
>>>>>>> (CONTROL_RDWR << CLK_FREQ_CTRL), .bAssocTerminal = 0, }; 
>>>>>>> @@ -178,7 +179,7 @@ static struct
>>>>>>> uac_clock_source_descriptor out_clk_src_desc = { 
>>>>>>> .bDescriptorSubtype = UAC2_CLOCK_SOURCE, /* .bClockID =
>>>>>>> DYNAMIC */ .bmAttributes =
>>>>>>> UAC_CLOCK_SOURCE_TYPE_INT_FIXED, -    .bmControls =
>>>>>>> (CONTROL_RDONLY << CLK_FREQ_CTRL), +    .bmControls =
>>>>>>> (CONTROL_RDWR << CLK_FREQ_CTRL), .bAssocTerminal = 0, }; 
>>>>>>> @@ -635,12 +636,32 @@ struct cntrl_cur_lay3 { }; struct
>>>>>>> cntrl_range_lay3 { -    __le16    wNumSubRanges; __le32
>>>>>>> dMIN; __le32    dMAX; __le32    dRES; } __packed; 
>>>>>>> +#define ranges_size(c) (sizeof(c.wNumSubRanges) +
>>>>>>> c.wNumSubRanges \ +        * sizeof(struct
>>>>>>> cntrl_ranges_lay3)) + +struct cntrl_ranges_lay3 { +
>>>>>>> __u16    wNumSubRanges; +    struct cntrl_range_lay3
>>>>>>> r[UAC_MAX_RATES]; +} __packed;
>>>>>> 
>>>>>> These structures are now inconsistent between
>>>>>> cntrl_range_lay2 and cntrl_range_lay3.  Would it be better
>>>>>> to make these flex arrays?  I guess that will make the code
>>>>>> that uses it more complicated, but at the moment it looks
>>>>>> like these are trying to be generic while in reality being
>>>>>> quite specific to the one place that uses them at the
>>>>>> moment.
>>>>> 
>>>>> I am afraid I do not know exactly how to do that. Please can
>>>>> you post an example? The rate control requires u32 (u16 is
>>>>> too small). Thanks a lot.
>>>> 
>>>> After the change in this patch, we end up with:
>>>> 
>>>> struct cntrl_range_lay2 { __le16    wNumSubRanges; __le16
>>>> wMIN; __le16    wMAX; __le16    wRES; } __packed;
>>>> 
>>>> struct cntrl_range_lay3 { __le32    dMIN; __le32    dMAX; 
>>>> __le32    dRES; } __packed;
>>>> 
>>>> so there are two structures with similar names but totally
>>>> different structure, which I think risks confusion in the
>>>> future.
>>>> 
>>>> I wonder if DECLARE_UAC2_FEATURE_UNIT_DESCRIPTOR in 
>>>> linux/usb/audio-v2.h provides inspiration here, so potentially
>>>> something like:
>>>> 
>>>> #define DECLARE_UAC2_CNTRL_RANGE_LAY3(n)    \ struct
>>>> uac2_cntrl_range_lay3_##n {    \ __le16 wNumSubRanges;
>>>> \ struct cntrl_range_le32 r[n];    \ } __packed;
>>>> 
>>>> DECLARE_UAC2_CNTRL_RANGE_LAY3(UAC_MAX_RATES);
>>> 
>>> Thanks, I will try to follow your suggestion in the next patchset
>>>  version.
>>> 
>>>> 
>>>>>>> +static int get_max_srate(const int *srates) +{ +    int
>>>>>>> i, max_srate = 0; + +    for (i = 0; i < UAC_MAX_RATES;
>>>>>>> i++) { +        if (srates[i] == 0) +            break; +
>>>>>>> if (srates[i] > max_srate) +            max_srate =
>>>>>>> srates[i]; +    } +    return max_srate; +} + static int
>>>>>>> set_ep_max_packet_size(const struct f_uac2_opts 
>>>>>>> *uac2_opts, struct usb_endpoint_descriptor *ep_desc, enum
>>>>>>> usb_device_speed speed, bool is_playback) @@ -667,11
>>>>>>> +688,11 @@ static int set_ep_max_packet_size(const struct
>>>>>>> f_uac2_opts *uac2_opts, if (is_playback) { chmask =
>>>>>>> uac2_opts->p_chmask; -        srate =
>>>>>>> uac2_opts->p_srate; +        srate =
>>>>>>> get_max_srate(uac2_opts->p_srates); ssize =
>>>>>>> uac2_opts->p_ssize; } else { chmask =
>>>>>>> uac2_opts->c_chmask; -        srate =
>>>>>>> uac2_opts->c_srate; +        srate =
>>>>>>> get_max_srate(uac2_opts->c_srates); ssize =
>>>>>>> uac2_opts->c_ssize; } @@ -912,10 +933,10 @@ static int
>>>>>>> afunc_validate_opts(struct g_audio *agdev, struct device
>>>>>>> *dev) } else if ((opts->c_ssize < 1) || (opts->c_ssize >
>>>>>>> 4)) { dev_err(dev, "Error: incorrect capture sample
>>>>>>> size\n"); return -EINVAL; -    } else if (!opts->p_srate)
>>>>>>> { +    } else if (!opts->p_srates[0]) { dev_err(dev,
>>>>>>> "Error: incorrect playback sampling rate\n"); return
>>>>>>> -EINVAL; -    } else if (!opts->c_srate) { +    } else if
>>>>>>> (!opts->c_srates[0]) { dev_err(dev, "Error: incorrect
>>>>>>> capture sampling rate\n"); return -EINVAL; } @@ -1210,7
>>>>>>> +1231,8 @@ afunc_bind(struct usb_configuration *cfg, 
>>>>>>> struct usb_function *fn) agdev->params.p_chmask =
>>>>>>> uac2_opts->p_chmask; agdev->params.p_srate =
>>>>>>> uac2_opts->p_srate; -    agdev->params.p_srates[0] =
>>>>>>> uac2_opts->p_srate; +    memcpy(agdev->params.p_srates,
>>>>>>> uac2_opts->p_srates, +
>>>>>>> sizeof(agdev->params.p_srates)); agdev->params.p_ssize =
>>>>>>> uac2_opts->p_ssize; if (FUIN_EN(uac2_opts)) { 
>>>>>>> agdev->params.p_fu.id = USB_IN_FU_ID; @@ -1222,7 +1244,8
>>>>>>> @@ afunc_bind(struct usb_configuration *cfg, struct
>>>>>>> usb_function *fn) } agdev->params.c_chmask =
>>>>>>> uac2_opts->c_chmask; agdev->params.c_srate =
>>>>>>> uac2_opts->c_srate; -    agdev->params.c_srates[0] =
>>>>>>> uac2_opts->c_srate; +    memcpy(agdev->params.c_srates,
>>>>>>> uac2_opts->c_srates, +
>>>>>>> sizeof(agdev->params.c_srates)); agdev->params.c_ssize =
>>>>>>> uac2_opts->c_ssize; if (FUOUT_EN(uac2_opts)) { 
>>>>>>> agdev->params.c_fu.id = USB_OUT_FU_ID; @@ -1502,28
>>>>>>> +1525,39 @@ in_rq_range(struct usb_function *fn, const
>>>>>>> struct usb_ctrlrequest *cr) u8 entity_id = (w_index >> 8)
>>>>>>> & 0xff; u8 control_selector = w_value >> 8; int value =
>>>>>>> -EOPNOTSUPP; -    int p_srate, c_srate; - -    p_srate =
>>>>>>> opts->p_srate; -    c_srate = opts->c_srate; if
>>>>>>> ((entity_id == USB_IN_CLK_ID) || (entity_id == 
>>>>>>> USB_OUT_CLK_ID)) { if (control_selector ==
>>>>>>> UAC2_CS_CONTROL_SAM_FREQ) { -            struct
>>>>>>> cntrl_range_lay3 r; +            struct cntrl_ranges_lay3
>>>>>>> rs; +            int i; +            int wNumSubRanges =
>>>>>>> 0; +            int srate; +            int *srates; if
>>>>>>> (entity_id == USB_IN_CLK_ID) -                r.dMIN =
>>>>>>> cpu_to_le32(p_srate); +                srates =
>>>>>>> opts->p_srates; else if (entity_id == USB_OUT_CLK_ID) -
>>>>>>> r.dMIN = cpu_to_le32(c_srate); +                srates =
>>>>>>> opts->c_srates; else return -EOPNOTSUPP; - -
>>>>>>> r.dMAX = r.dMIN; -            r.dRES = 0; -
>>>>>>> r.wNumSubRanges = cpu_to_le16(1); - -            value =
>>>>>>> min_t(unsigned int, w_length, sizeof(r)); -
>>>>>>> memcpy(req->buf, &r, value); +            for (i = 0; i <
>>>>>>> UAC_MAX_RATES; i++) { +                srate =
>>>>>>> srates[i]; +                if (srate == 0) +
>>>>>>> break; + +                rs.r[wNumSubRanges].dMIN =
>>>>>>> cpu_to_le32(srate); +
>>>>>>> rs.r[wNumSubRanges].dMAX = cpu_to_le32(srate); +
>>>>>>> rs.r[wNumSubRanges].dRES = 0; +
>>>>>>> wNumSubRanges++; +
>>>>>>> dev_dbg(&agdev->gadget->dev, +                    "%s():
>>>>>>> clk %d: rate ID %d: %d\n", +                    __func__,
>>>>>>> entity_id, wNumSubRanges, srate); +            } +
>>>>>>> rs.wNumSubRanges = cpu_to_le16(wNumSubRanges); +
>>>>>>> value = min_t(unsigned int, w_length, ranges_size(rs)); +
>>>>>>> dev_dbg(&agdev->gadget->dev, "%s(): sending %d rates,
>>>>>>> size %d\n", +                __func__, rs.wNumSubRanges,
>>>>>>> value); +            memcpy(req->buf, &rs, value); } else
>>>>>>> { dev_err(&agdev->gadget->dev, "%s:%d control_selector=%d
>>>>>>> TODO!\n", @@ -1582,6 +1616,28 @@ ac_rq_in(struct
>>>>>>> usb_function *fn, const struct usb_ctrlrequest *cr) 
>>>>>>> return -EOPNOTSUPP; } +static void
>>>>>>> uac2_cs_control_sam_freq(struct usb_ep *ep, struct 
>>>>>>> usb_request *req) +{ +    struct usb_function *fn =
>>>>>>> ep->driver_data; +    struct g_audio *agdev =
>>>>>>> func_to_g_audio(fn); +    struct f_uac2 *uac2 =
>>>>>>> func_to_uac2(fn); +    struct f_uac2_opts *opts =
>>>>>>> g_audio_to_uac2_opts(agdev); +    u32 val; + +    if
>>>>>>> (req->actual != 4) +        return; + +    val =
>>>>>>> le32_to_cpu(*((u32 *)req->buf)); +
>>>>>>> dev_dbg(&agdev->gadget->dev, "%s val: %d.\n", __func__,
>>>>>>> val); +    if (uac2->ctl_id == USB_IN_CLK_ID) { +
>>>>>>> opts->p_srate = val;
>>>>>> 
>>>>>> Don't you need to hold opts->lock to change this? I'm not
>>>>>> sure opts should be changed here though - that's the setup
>>>>>>  phase and this is "current state", so shouldn't it move to
>>>>>> struct f_uac2?
>>>>> 
>>>>> OK. I moved the current p_srate/c_srate from struct opts to
>>>>> f_uac2, initialized with first value of
>>>>> opts->p_srates/c_srates[0] in afunc_bind. The struct f_uac2
>>>>> has no lock yet. Should I add the lock mutex to f_uac2 and be
>>>>> locking f_uac2 access here in uac2_cs_control_sam_freq?
>>>> 
>>>> Could we move this into struct uac_rtd_params and use the
>>>> existing lock there to guard it?
>>>> 
>>>> It would need accessor functions as that structure's local to 
>>>> u_audio.c, but there's already u_audio_set_playback_srate() so
>>>> that isn't a big change.
>>> 
>>> I have already moved p_/c_srate from uac_params to uac_rtd_params
>>> in u_audio.c in the next version of the patchset. But IIUC the
>>> currently selected playback/capture rate is required within
>>> f_uac2 too, in in_rq_cur() in:
>>> 
>>> if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) { ... if
>>> (entity_id == USB_IN_CLK_ID) c.dCUR = cpu_to_le32(p_srate); else
>>> if (entity_id == USB_OUT_CLK_ID) c.dCUR = cpu_to_le32(c_srate); 
>>> ... }
>> 
>> Can this can be u_audio_get_playback_srate(agdev) (and equivalent
>> for capture)?
>> 
> 
> We certainly can add the rate getter API call. But I do not know
> whether master should store and query slave for data required for
> proper operation of the master. IMO f_uac2 should be in control of
> the params requested by the real master - the USB host.
> 
> Currently the gadget-side app can open the gadget alsa device at the
>  last-used rate (or channel count if/when multiple channels config is
>  implemented) at any time, without knowing what params the user will
>  actually request. IMO in some use cases the gadget alsa device
> should not allow opening before the host starts playback/capture,
> i.e. before the user actually requests what params to use. This would
> require more patches, nothing critical now and not in my pipeline. I
> (will) handle this issue with the side-channel gaudio_ctl application
> controller which receives the currently required params (srate as of
> now) via ctl notifications.
> 
> The question is whether it's correct for f_uac2 to ask u_audio about
>  current rate at any time, even when u_audio is inactive and the only
>  party doing something with the rate is actually f_uac2.
> 
> Maybe the ultimate data flow should be the other way round. But IIUC
>  u_audio currently does not have any hooks back to f_uac1/2. The USB
>  functions store all the params to uac_param for u_audio to use.
> 
> But I can add the u_audio_get_playback/capture_srate API method (in a
>  separate patch, I guess) to avoid keeping the same values in two
> places, if found convenient.
> 
> I think we all should discuss our use cases in order to delineate the
>  path along which the audio gadget should evolve.
> 

Actually the host is not always the ultimate master. E.g. should the
gadget capture an SPDIF stream and pass it to the host, the u_audio part
should limit the available samplerates for f_uacX down to the currently
receiving samplerate. That suggests the
u_audio_get_playback/capture_srate API is probably in the right
direction. I do so and remove the c_srate/p_srate fields from f_uacX struct.


Thanks,

Pavel.
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uac2 b/Documentation/ABI/testing/configfs-usb-gadget-uac2
index 244d96650123..b20b0471d48a 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uac2
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uac2
@@ -6,7 +6,7 @@  Description:
 
 		=====================	=======================================
 		c_chmask		capture channel mask
-		c_srate			capture sampling rate
+		c_srate			list of capture sampling rates (comma-separated)
 		c_ssize			capture sample size (bytes)
 		c_sync			capture synchronization type
 					(async/adaptive)
@@ -20,7 +20,7 @@  Description:
 					(in 1/256 dB)
 		fb_max			maximum extra bandwidth in async mode
 		p_chmask		playback channel mask
-		p_srate			playback sampling rate
+		p_srate			list of playback sampling rates (comma-separated)
 		p_ssize			playback sample size (bytes)
 		p_mute_present		playback mute control enable
 		p_volume_present	playback volume control enable
diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
index c18113077889..928f60a31544 100644
--- a/Documentation/usb/gadget-testing.rst
+++ b/Documentation/usb/gadget-testing.rst
@@ -726,7 +726,7 @@  The uac2 function provides these attributes in its function directory:
 
 	================ ====================================================
 	c_chmask         capture channel mask
-	c_srate          capture sampling rate
+	c_srate          list of capture sampling rates (comma-separated)
 	c_ssize          capture sample size (bytes)
 	c_sync           capture synchronization type (async/adaptive)
 	c_mute_present   capture mute control enable
@@ -736,7 +736,7 @@  The uac2 function provides these attributes in its function directory:
 	c_volume_res     capture volume control resolution (in 1/256 dB)
 	fb_max           maximum extra bandwidth in async mode
 	p_chmask         playback channel mask
-	p_srate          playback sampling rate
+	p_srate          list of playback sampling rates (comma-separated)
 	p_ssize          playback sample size (bytes)
 	p_mute_present   playback mute control enable
 	p_volume_present playback volume control enable
diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index 1d6e426e5078..74e32bb146c7 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -70,6 +70,7 @@  struct f_uac2 {
 	/* Interrupt IN endpoint of AC interface */
 	struct usb_ep	*int_ep;
 	atomic_t	int_count;
+	int ctl_id;
 };
 
 static inline struct f_uac2 *func_to_uac2(struct usb_function *f)
@@ -166,7 +167,7 @@  static struct uac_clock_source_descriptor in_clk_src_desc = {
 	.bDescriptorSubtype = UAC2_CLOCK_SOURCE,
 	/* .bClockID = DYNAMIC */
 	.bmAttributes = UAC_CLOCK_SOURCE_TYPE_INT_FIXED,
-	.bmControls = (CONTROL_RDONLY << CLK_FREQ_CTRL),
+	.bmControls = (CONTROL_RDWR << CLK_FREQ_CTRL),
 	.bAssocTerminal = 0,
 };
 
@@ -178,7 +179,7 @@  static struct uac_clock_source_descriptor out_clk_src_desc = {
 	.bDescriptorSubtype = UAC2_CLOCK_SOURCE,
 	/* .bClockID = DYNAMIC */
 	.bmAttributes = UAC_CLOCK_SOURCE_TYPE_INT_FIXED,
-	.bmControls = (CONTROL_RDONLY << CLK_FREQ_CTRL),
+	.bmControls = (CONTROL_RDWR << CLK_FREQ_CTRL),
 	.bAssocTerminal = 0,
 };
 
@@ -635,12 +636,32 @@  struct cntrl_cur_lay3 {
 };
 
 struct cntrl_range_lay3 {
-	__le16	wNumSubRanges;
 	__le32	dMIN;
 	__le32	dMAX;
 	__le32	dRES;
 } __packed;
 
+#define ranges_size(c) (sizeof(c.wNumSubRanges) + c.wNumSubRanges \
+		* sizeof(struct cntrl_ranges_lay3))
+
+struct cntrl_ranges_lay3 {
+	__u16	wNumSubRanges;
+	struct cntrl_range_lay3 r[UAC_MAX_RATES];
+} __packed;
+
+static int get_max_srate(const int *srates)
+{
+	int i, max_srate = 0;
+
+	for (i = 0; i < UAC_MAX_RATES; i++) {
+		if (srates[i] == 0)
+			break;
+		if (srates[i] > max_srate)
+			max_srate = srates[i];
+	}
+	return max_srate;
+}
+
 static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
 	struct usb_endpoint_descriptor *ep_desc,
 	enum usb_device_speed speed, bool is_playback)
@@ -667,11 +688,11 @@  static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
 
 	if (is_playback) {
 		chmask = uac2_opts->p_chmask;
-		srate = uac2_opts->p_srate;
+		srate = get_max_srate(uac2_opts->p_srates);
 		ssize = uac2_opts->p_ssize;
 	} else {
 		chmask = uac2_opts->c_chmask;
-		srate = uac2_opts->c_srate;
+		srate = get_max_srate(uac2_opts->c_srates);
 		ssize = uac2_opts->c_ssize;
 	}
 
@@ -912,10 +933,10 @@  static int afunc_validate_opts(struct g_audio *agdev, struct device *dev)
 	} else if ((opts->c_ssize < 1) || (opts->c_ssize > 4)) {
 		dev_err(dev, "Error: incorrect capture sample size\n");
 		return -EINVAL;
-	} else if (!opts->p_srate) {
+	} else if (!opts->p_srates[0]) {
 		dev_err(dev, "Error: incorrect playback sampling rate\n");
 		return -EINVAL;
-	} else if (!opts->c_srate) {
+	} else if (!opts->c_srates[0]) {
 		dev_err(dev, "Error: incorrect capture sampling rate\n");
 		return -EINVAL;
 	}
@@ -1210,7 +1231,8 @@  afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 
 	agdev->params.p_chmask = uac2_opts->p_chmask;
 	agdev->params.p_srate = uac2_opts->p_srate;
-	agdev->params.p_srates[0] = uac2_opts->p_srate;
+	memcpy(agdev->params.p_srates, uac2_opts->p_srates,
+			sizeof(agdev->params.p_srates));
 	agdev->params.p_ssize = uac2_opts->p_ssize;
 	if (FUIN_EN(uac2_opts)) {
 		agdev->params.p_fu.id = USB_IN_FU_ID;
@@ -1222,7 +1244,8 @@  afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 	}
 	agdev->params.c_chmask = uac2_opts->c_chmask;
 	agdev->params.c_srate = uac2_opts->c_srate;
-	agdev->params.c_srates[0] = uac2_opts->c_srate;
+	memcpy(agdev->params.c_srates, uac2_opts->c_srates,
+			sizeof(agdev->params.c_srates));
 	agdev->params.c_ssize = uac2_opts->c_ssize;
 	if (FUOUT_EN(uac2_opts)) {
 		agdev->params.c_fu.id = USB_OUT_FU_ID;
@@ -1502,28 +1525,39 @@  in_rq_range(struct usb_function *fn, const struct usb_ctrlrequest *cr)
 	u8 entity_id = (w_index >> 8) & 0xff;
 	u8 control_selector = w_value >> 8;
 	int value = -EOPNOTSUPP;
-	int p_srate, c_srate;
-
-	p_srate = opts->p_srate;
-	c_srate = opts->c_srate;
 
 	if ((entity_id == USB_IN_CLK_ID) || (entity_id == USB_OUT_CLK_ID)) {
 		if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) {
-			struct cntrl_range_lay3 r;
+			struct cntrl_ranges_lay3 rs;
+			int i;
+			int wNumSubRanges = 0;
+			int srate;
+			int *srates;
 
 			if (entity_id == USB_IN_CLK_ID)
-				r.dMIN = cpu_to_le32(p_srate);
+				srates = opts->p_srates;
 			else if (entity_id == USB_OUT_CLK_ID)
-				r.dMIN = cpu_to_le32(c_srate);
+				srates = opts->c_srates;
 			else
 				return -EOPNOTSUPP;
-
-			r.dMAX = r.dMIN;
-			r.dRES = 0;
-			r.wNumSubRanges = cpu_to_le16(1);
-
-			value = min_t(unsigned int, w_length, sizeof(r));
-			memcpy(req->buf, &r, value);
+			for (i = 0; i < UAC_MAX_RATES; i++) {
+				srate = srates[i];
+				if (srate == 0)
+					break;
+
+				rs.r[wNumSubRanges].dMIN = cpu_to_le32(srate);
+				rs.r[wNumSubRanges].dMAX = cpu_to_le32(srate);
+				rs.r[wNumSubRanges].dRES = 0;
+				wNumSubRanges++;
+				dev_dbg(&agdev->gadget->dev,
+					"%s(): clk %d: rate ID %d: %d\n",
+					__func__, entity_id, wNumSubRanges, srate);
+			}
+			rs.wNumSubRanges = cpu_to_le16(wNumSubRanges);
+			value = min_t(unsigned int, w_length, ranges_size(rs));
+			dev_dbg(&agdev->gadget->dev, "%s(): sending %d rates, size %d\n",
+				__func__, rs.wNumSubRanges, value);
+			memcpy(req->buf, &rs, value);
 		} else {
 			dev_err(&agdev->gadget->dev,
 				"%s:%d control_selector=%d TODO!\n",
@@ -1582,6 +1616,28 @@  ac_rq_in(struct usb_function *fn, const struct usb_ctrlrequest *cr)
 		return -EOPNOTSUPP;
 }
 
+static void uac2_cs_control_sam_freq(struct usb_ep *ep, struct usb_request *req)
+{
+	struct usb_function *fn = ep->driver_data;
+	struct g_audio *agdev = func_to_g_audio(fn);
+	struct f_uac2 *uac2 = func_to_uac2(fn);
+	struct f_uac2_opts *opts = g_audio_to_uac2_opts(agdev);
+	u32 val;
+
+	if (req->actual != 4)
+		return;
+
+	val = le32_to_cpu(*((u32 *)req->buf));
+	dev_dbg(&agdev->gadget->dev, "%s val: %d.\n", __func__, val);
+	if (uac2->ctl_id == USB_IN_CLK_ID) {
+		opts->p_srate = val;
+		u_audio_set_playback_srate(agdev, opts->p_srate);
+	} else if (uac2->ctl_id == USB_OUT_CLK_ID) {
+		opts->c_srate = val;
+		u_audio_set_capture_srate(agdev, opts->c_srate);
+	}
+}
+
 static void
 out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req)
 {
@@ -1633,6 +1689,7 @@  out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req)
 static int
 out_rq_cur(struct usb_function *fn, const struct usb_ctrlrequest *cr)
 {
+	struct usb_composite_dev *cdev = fn->config->cdev;
 	struct usb_request *req = fn->config->cdev->req;
 	struct g_audio *agdev = func_to_g_audio(fn);
 	struct f_uac2_opts *opts = g_audio_to_uac2_opts(agdev);
@@ -1642,10 +1699,17 @@  out_rq_cur(struct usb_function *fn, const struct usb_ctrlrequest *cr)
 	u16 w_value = le16_to_cpu(cr->wValue);
 	u8 entity_id = (w_index >> 8) & 0xff;
 	u8 control_selector = w_value >> 8;
+	u8 clock_id = w_index >> 8;
 
 	if ((entity_id == USB_IN_CLK_ID) || (entity_id == USB_OUT_CLK_ID)) {
-		if (control_selector == UAC2_CS_CONTROL_SAM_FREQ)
+		if (control_selector == UAC2_CS_CONTROL_SAM_FREQ) {
+			dev_dbg(&agdev->gadget->dev,
+				"control_selector UAC2_CS_CONTROL_SAM_FREQ, clock: %d\n", clock_id);
+			cdev->gadget->ep0->driver_data = fn;
+			uac2->ctl_id = clock_id;
+			req->complete = uac2_cs_control_sam_freq;
 			return w_length;
+		}
 	} else if ((FUIN_EN(opts) && (entity_id == USB_IN_FU_ID)) ||
 			(FUOUT_EN(opts) && (entity_id == USB_OUT_FU_ID))) {
 		memcpy(&uac2->setup_cr, cr, sizeof(*cr));
@@ -1839,10 +1903,10 @@  end:									\
 CONFIGFS_ATTR(f_uac2_opts_, name)
 
 UAC2_ATTRIBUTE(u32, p_chmask);
-UAC2_ATTRIBUTE(u32, p_srate);
+UAC_RATE2_ATTRIBUTE(p_srate);
 UAC2_ATTRIBUTE(u32, p_ssize);
 UAC2_ATTRIBUTE(u32, c_chmask);
-UAC2_ATTRIBUTE(u32, c_srate);
+UAC_RATE2_ATTRIBUTE(c_srate);
 UAC2_ATTRIBUTE_SYNC(c_sync);
 UAC2_ATTRIBUTE(u32, c_ssize);
 UAC2_ATTRIBUTE(u32, req_number);
@@ -1915,9 +1979,11 @@  static struct usb_function_instance *afunc_alloc_inst(void)
 				    &f_uac2_func_type);
 
 	opts->p_chmask = UAC2_DEF_PCHMASK;
+	opts->p_srates[0] = UAC2_DEF_PSRATE;
 	opts->p_srate = UAC2_DEF_PSRATE;
 	opts->p_ssize = UAC2_DEF_PSSIZE;
 	opts->c_chmask = UAC2_DEF_CCHMASK;
+	opts->c_srates[0] = UAC2_DEF_CSRATE;
 	opts->c_srate = UAC2_DEF_CSRATE;
 	opts->c_ssize = UAC2_DEF_CSSIZE;
 	opts->c_sync = UAC2_DEF_CSYNC;
diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h
index e0c8e3513bfd..8058217322f8 100644
--- a/drivers/usb/gadget/function/u_uac2.h
+++ b/drivers/usb/gadget/function/u_uac2.h
@@ -14,6 +14,7 @@ 
 #define U_UAC2_H
 
 #include <linux/usb/composite.h>
+#include "uac_common.h"
 
 #define UAC2_DEF_PCHMASK 0x3
 #define UAC2_DEF_PSRATE 48000
@@ -35,9 +36,11 @@ 
 struct f_uac2_opts {
 	struct usb_function_instance	func_inst;
 	int				p_chmask;
+	int				p_srates[UAC_MAX_RATES];
 	int				p_srate;
 	int				p_ssize;
 	int				c_chmask;
+	int				c_srates[UAC_MAX_RATES];
 	int				c_srate;
 	int				c_ssize;
 	int				c_sync;
@@ -62,4 +65,63 @@  struct f_uac2_opts {
 	int				refcnt;
 };
 
+#define UAC_RATE2_ATTRIBUTE(name)					\
+static ssize_t f_uac2_opts_##name##_show(struct config_item *item,	\
+					 char *page)			\
+{									\
+	struct f_uac2_opts *opts = to_f_uac2_opts(item);			\
+	int result = 0;							\
+	int i;								\
+									\
+	mutex_lock(&opts->lock);					\
+	page[0] = '\0';							\
+	for (i = 0; i < UAC_MAX_RATES; i++) {				\
+		if (opts->name##s[i] == 0)					\
+			break;					\
+		result += sprintf(page + strlen(page), "%u,",		\
+				opts->name##s[i]);				\
+	}								\
+	if (strlen(page) > 0)						\
+		page[strlen(page) - 1] = '\n';				\
+	mutex_unlock(&opts->lock);					\
+									\
+	return result;							\
+}									\
+									\
+static ssize_t f_uac2_opts_##name##_store(struct config_item *item,	\
+					  const char *page, size_t len)	\
+{									\
+	struct f_uac2_opts *opts = to_f_uac2_opts(item);			\
+	char *split_page = NULL;					\
+	int ret = -EINVAL;						\
+	char *token;							\
+	u32 num;							\
+	int i;								\
+									\
+	mutex_lock(&opts->lock);					\
+	if (opts->refcnt) {						\
+		ret = -EBUSY;						\
+		goto end;						\
+	}								\
+									\
+	i = 0;								\
+	memset(opts->name##s, 0x00, sizeof(opts->name##s));			\
+	split_page = kstrdup(page, GFP_KERNEL);				\
+	while ((token = strsep(&split_page, ",")) != NULL) {		\
+		ret = kstrtou32(token, 0, &num);			\
+		if (ret)						\
+			goto end;					\
+									\
+		opts->name##s[i++] = num;					\
+		opts->name = num;				\
+		ret = len;						\
+	};								\
+									\
+end:									\
+	kfree(split_page);						\
+	mutex_unlock(&opts->lock);					\
+	return ret;							\
+}									\
+									\
+CONFIGFS_ATTR(f_uac2_opts_, name)
 #endif
diff --git a/drivers/usb/gadget/legacy/audio.c b/drivers/usb/gadget/legacy/audio.c
index a748ed0842e8..58bcb26c7854 100644
--- a/drivers/usb/gadget/legacy/audio.c
+++ b/drivers/usb/gadget/legacy/audio.c
@@ -26,9 +26,10 @@  module_param(p_chmask, uint, S_IRUGO);
 MODULE_PARM_DESC(p_chmask, "Playback Channel Mask");
 
 /* Playback Default 48 KHz */
-static int p_srate = UAC2_DEF_PSRATE;
-module_param(p_srate, uint, S_IRUGO);
-MODULE_PARM_DESC(p_srate, "Playback Sampling Rate");
+static int p_srates[UAC_MAX_RATES] = {UAC2_DEF_PSRATE};
+static int p_srates_cnt = 1;
+module_param_array_named(p_srate, p_srates, uint, &p_srates_cnt, S_IRUGO);
+MODULE_PARM_DESC(p_srate, "Playback Sampling Rates (array)");
 
 /* Playback Default 16bits/sample */
 static int p_ssize = UAC2_DEF_PSSIZE;
@@ -41,9 +42,10 @@  module_param(c_chmask, uint, S_IRUGO);
 MODULE_PARM_DESC(c_chmask, "Capture Channel Mask");
 
 /* Capture Default 64 KHz */
-static int c_srate = UAC2_DEF_CSRATE;
-module_param(c_srate, uint, S_IRUGO);
-MODULE_PARM_DESC(c_srate, "Capture Sampling Rate");
+static int c_srates[UAC_MAX_RATES] = {UAC2_DEF_CSRATE};
+static int c_srates_cnt = 1;
+module_param_array_named(c_srate, c_srates, uint, &c_srates_cnt, S_IRUGO);
+MODULE_PARM_DESC(c_srate, "Capture Sampling Rates (array)");
 
 /* Capture Default 16bits/sample */
 static int c_ssize = UAC2_DEF_CSSIZE;
@@ -244,7 +246,7 @@  static int audio_bind(struct usb_composite_dev *cdev)
 	struct f_uac1_legacy_opts	*uac1_opts;
 #endif
 #endif
-	int			status;
+	int status, i;
 
 #ifndef CONFIG_GADGET_UAC1
 	fi_uac2 = usb_get_function_instance("uac2");
@@ -263,10 +265,18 @@  static int audio_bind(struct usb_composite_dev *cdev)
 #ifndef CONFIG_GADGET_UAC1
 	uac2_opts = container_of(fi_uac2, struct f_uac2_opts, func_inst);
 	uac2_opts->p_chmask = p_chmask;
-	uac2_opts->p_srate = p_srate;
+
+	for (i = 0; i < p_srates_cnt; ++i)
+		uac2_opts->p_srates[i] = p_srates[i];
+	uac2_opts->p_srate = p_srates[0];
+
 	uac2_opts->p_ssize = p_ssize;
 	uac2_opts->c_chmask = c_chmask;
-	uac2_opts->c_srate = c_srate;
+
+	for (i = 0; i < c_srates_cnt; ++i)
+		uac2_opts->c_srates[i] = c_srates[i];
+	uac2_opts->c_srate = c_srates[0];
+
 	uac2_opts->c_ssize = c_ssize;
 	uac2_opts->req_number = UAC2_DEF_REQ_NUM;
 #else