| Message ID | 20220216094301.2448-1-3090101217@zju.edu.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: f_uac1: add set requests support | expand |
On Wed, Feb 16, 2022 at 05:43:01PM +0800, 3090101217@zju.edu.cn wrote: > From: Jing Leng <jleng@ambarella.com> > > Currently the f_uac1 driver only supports UAC_SET_CUR request. > > But when uac1 device is plugged to Ubuntu 20.04 PC, at the stage > of setup, the PC will send UAC_SET_RES request, If the device > doesn't respond to the request, the PC will abort the setup process > and uac1 device can't be recognized on Ubuntu 20.04 PC. So is this a bug in the Host side to not do stuff like this? Why not fix it there instead? Where is the requirement that this command must be handled by the device? thanks, greg k-h
Hi Greg KH, > So is this a bug in the Host side to not do stuff like this? Why not > fix it there instead? > > Where is the requirement that this command must be handled by the > device? > First we need to clarify two issues. 1. Does the Ubuntu go beyond the UAC1 specification? No. On page 66 of the UAC1 specification ( https://www.usb.org/sites/default/files/audio10.pdf): The bRequest can be SET_CUR, SET_MIN, SET_MAX, SET_RES or SET_MEM. In most cases, only the CUR and MEM attribute will be supported for the Set request. However, this specification does not prevent a designer from making other attributes programmable. Supplement: Windows 10 only sends SET_CUR request. 2. Does the old version kernel have the problem on the Ubuntu? NO. (e.g. linux-5.10) The problem is introduced by the following modification: commit 0356e6283c7177391d144612f4b12986ed5c4f6e Author: Ruslan Bilovol <ruslan.bilovol@gmail.com> Date: Mon Jul 12 14:55:29 2021 +0200 usb: gadget: f_uac1: add volume and mute support Since Ubuntu doesn't go beyond the UAC1 specification and the problem is introduced by new version kernel, Why don't we perfect it on kernel side? Thanks Jing Leng
On Thu, Feb 17, 2022 at 09:42:00AM +0800, Jing Leng wrote: > Hi Greg KH, > > > So is this a bug in the Host side to not do stuff like this? Why not > > fix it there instead? > > > > Where is the requirement that this command must be handled by the > > device? > > > > First we need to clarify two issues. > > 1. Does the Ubuntu go beyond the UAC1 specification? > No. > On page 66 of the UAC1 specification ( > https://www.usb.org/sites/default/files/audio10.pdf): > The bRequest can be SET_CUR, SET_MIN, SET_MAX, SET_RES or SET_MEM. > In most cases, only the CUR and MEM attribute will be supported for > the Set request. However, this specification does not prevent a > designer from making other attributes programmable. > Supplement: Windows 10 only sends SET_CUR request. > > 2. Does the old version kernel have the problem on the Ubuntu? > NO. (e.g. linux-5.10) > The problem is introduced by the following modification: > commit 0356e6283c7177391d144612f4b12986ed5c4f6e > Author: Ruslan Bilovol <ruslan.bilovol@gmail.com> > Date: Mon Jul 12 14:55:29 2021 +0200 > > usb: gadget: f_uac1: add volume and mute support Then please add this commit id as a "Fixes:" tag in the changelog area. thanks, greg k-h
diff --git a/drivers/usb/gadget/function/f_uac1.c b/drivers/usb/gadget/function/f_uac1.c index 03f50643fbba..c9d8ec4fdf22 100644 --- a/drivers/usb/gadget/function/f_uac1.c +++ b/drivers/usb/gadget/function/f_uac1.c @@ -589,7 +589,7 @@ in_rq_res(struct usb_function *fn, const struct usb_ctrlrequest *cr) } static void -out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req) +out_rq_complete(struct usb_ep *ep, struct usb_request *req) { struct g_audio *audio = req->context; struct usb_composite_dev *cdev = audio->func.config->cdev; @@ -614,9 +614,11 @@ out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req) is_playback = 1; if (control_selector == UAC_FU_MUTE) { - u8 mute = *(u8 *)req->buf; + if (cr->bRequest == UAC_SET_CUR) { + u8 mute = *(u8 *)req->buf; - u_audio_set_mute(audio, is_playback, mute); + u_audio_set_mute(audio, is_playback, mute); + } return; } else if (control_selector == UAC_FU_VOLUME) { @@ -624,7 +626,34 @@ out_rq_cur_complete(struct usb_ep *ep, struct usb_request *req) s16 volume; volume = le16_to_cpu(*c); - u_audio_set_volume(audio, is_playback, volume); + + switch (cr->bRequest) { + case UAC_SET_CUR: + u_audio_set_volume(audio, is_playback, volume); + break; + case UAC_SET_MIN: + if (is_playback) + opts->p_volume_min = volume; + else + opts->c_volume_min = volume; + break; + case UAC_SET_MAX: + if (is_playback) + opts->p_volume_max = volume; + else + opts->c_volume_max = volume; + break; + case UAC_SET_RES: + if (is_playback) + opts->p_volume_res = volume; + else + opts->c_volume_res = volume; + break; + case UAC_SET_MEM: + break; + default: + break; + } return; } else { @@ -643,7 +672,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) +ac_rq_out(struct usb_function *fn, const struct usb_ctrlrequest *cr) { struct usb_request *req = fn->config->cdev->req; struct g_audio *audio = func_to_g_audio(fn); @@ -659,7 +688,7 @@ out_rq_cur(struct usb_function *fn, const struct usb_ctrlrequest *cr) (FUOUT_EN(opts) && (entity_id == USB_OUT_FU_ID))) { memcpy(&uac1->setup_cr, cr, sizeof(*cr)); req->context = audio; - req->complete = out_rq_cur_complete; + req->complete = out_rq_complete; return w_length; } else { @@ -789,8 +818,7 @@ f_audio_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) value = audio_get_endpoint_req(f, ctrl); break; case USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE: - if (ctrl->bRequest == UAC_SET_CUR) - value = out_rq_cur(f, ctrl); + value = ac_rq_out(f, ctrl); break; case USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE: value = ac_rq_in(f, ctrl);