| Message ID | 20220622085757.23437-1-chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: f_uac1: add IAD descriptor | expand |
On Wed, Jun 22, 2022 at 04:57:57PM +0800, Chunfeng Yun wrote: > From: xin lin <xin.lin@mediatek.com> > > Win10 can not enumerate composite device of UVC+UAC1+ADB without IAD descriptor > in uac1.0, so add it. I do not know what this means at all, sorry. Can you please provide a better changelog text that describes what all of this is in more detail? > > Signed-off-by: xin lin <xin.lin@mediatek.com> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > --- > drivers/usb/gadget/function/f_uac1.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/usb/gadget/function/f_uac1.c b/drivers/usb/gadget/function/f_uac1.c > index 6f0e1d803dc2..8390207bc513 100644 > --- a/drivers/usb/gadget/function/f_uac1.c > +++ b/drivers/usb/gadget/function/f_uac1.c > @@ -71,6 +71,17 @@ static inline struct f_uac1_opts *g_audio_to_uac1_opts(struct g_audio *audio) > * ALSA_Playback -> IT_3 -> OT_4 -> USB-IN > */ > > +static struct usb_interface_assoc_descriptor iad_desc = { > + .bLength = sizeof(iad_desc), > + .bDescriptorType = USB_DT_INTERFACE_ASSOCIATION, > + > + .bFirstInterface = 0, > + .bInterfaceCount = 3, > + .bFunctionClass = USB_CLASS_AUDIO, > + .bFunctionSubClass = 0, > + .bFunctionProtocol = UAC_VERSION_1, > +}; > + > /* B.3.1 Standard AC Interface Descriptor */ > static struct usb_interface_descriptor ac_interface_desc = { > .bLength = USB_DT_INTERFACE_SIZE, > @@ -259,6 +270,7 @@ static struct uac_iso_endpoint_descriptor as_iso_in_desc = { > }; > > static struct usb_descriptor_header *f_audio_desc[] = { > + (struct usb_descriptor_header *)&iad_desc, Why put this first? Is that a requirement? > (struct usb_descriptor_header *)&ac_interface_desc, > (struct usb_descriptor_header *)&ac_header_desc, > > @@ -293,6 +305,7 @@ static struct usb_descriptor_header *f_audio_desc[] = { > }; > > enum { > + STR_ASSOC, Again, why first? > STR_AC_IF, > STR_USB_OUT_IT, > STR_USB_OUT_IT_CH_NAMES, > @@ -310,6 +323,7 @@ enum { > > static struct usb_string strings_uac1[] = { > /* [STR_AC_IF].s = DYNAMIC, */ > + [STR_ASSOC].s = "Source/Sink", > [STR_USB_OUT_IT].s = "Playback Input terminal", > [STR_USB_OUT_IT_CH_NAMES].s = "Playback Channels", > [STR_IO_OUT_OT].s = "Playback Output terminal", > @@ -1058,6 +1072,7 @@ static void setup_descriptor(struct f_uac1_opts *opts) > as_out_header_desc.bTerminalLink = usb_out_it_desc.bTerminalID; > as_in_header_desc.bTerminalLink = usb_in_ot_desc.bTerminalID; > > + iad_desc.bInterfaceCount = 1; Why this change? > ac_header_desc->wTotalLength = cpu_to_le16(ac_header_desc->bLength); > > if (EPIN_EN(opts)) { > @@ -1068,6 +1083,7 @@ static void setup_descriptor(struct f_uac1_opts *opts) > if (FUIN_EN(opts)) > len += in_feature_unit_desc->bLength; > ac_header_desc->wTotalLength = cpu_to_le16(len); > + iad_desc.bInterfaceCount++; > } > if (EPOUT_EN(opts)) { > u16 len = le16_to_cpu(ac_header_desc->wTotalLength); > @@ -1077,9 +1093,11 @@ static void setup_descriptor(struct f_uac1_opts *opts) > if (FUOUT_EN(opts)) > len += out_feature_unit_desc->bLength; > ac_header_desc->wTotalLength = cpu_to_le16(len); > + iad_desc.bInterfaceCount++; > } > > i = 0; > + f_audio_desc[i++] = USBDHDR(&iad_desc); Again, why first? > f_audio_desc[i++] = USBDHDR(&ac_interface_desc); > f_audio_desc[i++] = USBDHDR(ac_header_desc); > > @@ -1217,6 +1235,7 @@ static int f_audio_bind(struct usb_configuration *c, struct usb_function *f) > } > } > > + iad_desc.iFunction = us[STR_ASSOC].id; > ac_interface_desc.iInterface = us[STR_AC_IF].id; > usb_out_it_desc.iTerminal = us[STR_USB_OUT_IT].id; > usb_out_it_desc.iChannelNames = us[STR_USB_OUT_IT_CH_NAMES].id; > @@ -1302,6 +1321,8 @@ static int f_audio_bind(struct usb_configuration *c, struct usb_function *f) > status = usb_interface_id(c, f); > if (status < 0) > goto err_free_fu; > + > + iad_desc.bFirstInterface = status; Shouldn't this be needed without your change? thanks, greg k-h
On Fri, 2022-06-24 at 13:39 +0200, Greg Kroah-Hartman wrote: > On Wed, Jun 22, 2022 at 04:57:57PM +0800, Chunfeng Yun wrote: > > From: xin lin <xin.lin@mediatek.com> > > > > Win10 can not enumerate composite device of UVC+UAC1+ADB without > > IAD descriptor > > in uac1.0, so add it. > > I do not know what this means at all, sorry. Can you please provide > a > better changelog text that describes what all of this is in more > detail? Ok, will add it in next version > > > > > > > Signed-off-by: xin lin <xin.lin@mediatek.com> > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > --- > > drivers/usb/gadget/function/f_uac1.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/drivers/usb/gadget/function/f_uac1.c > > b/drivers/usb/gadget/function/f_uac1.c > > index 6f0e1d803dc2..8390207bc513 100644 > > --- a/drivers/usb/gadget/function/f_uac1.c > > +++ b/drivers/usb/gadget/function/f_uac1.c > > @@ -71,6 +71,17 @@ static inline struct f_uac1_opts > > *g_audio_to_uac1_opts(struct g_audio *audio) > > * ALSA_Playback -> IT_3 -> OT_4 -> USB-IN > > */ > > > > +static struct usb_interface_assoc_descriptor iad_desc = { > > + .bLength = sizeof(iad_desc), > > + .bDescriptorType = USB_DT_INTERFACE_ASSOCIATION, > > + > > + .bFirstInterface = 0, > > + .bInterfaceCount = 3, > > + .bFunctionClass = USB_CLASS_AUDIO, > > + .bFunctionSubClass = 0, > > + .bFunctionProtocol = UAC_VERSION_1, > > +}; > > + > > /* B.3.1 Standard AC Interface Descriptor */ > > static struct usb_interface_descriptor ac_interface_desc = { > > .bLength = USB_DT_INTERFACE_SIZE, > > @@ -259,6 +270,7 @@ static struct uac_iso_endpoint_descriptor > > as_iso_in_desc = { > > }; > > > > static struct usb_descriptor_header *f_audio_desc[] = { > > + (struct usb_descriptor_header *)&iad_desc, > > Why put this first? Is that a requirement? Yes, it's a requirement, Interface Association Descriptor ECN: "An interface association descriptor must be located before the set of interface descriptors (including all alternate settings) for the interfaces it associates." > > > (struct usb_descriptor_header *)&ac_interface_desc, > > (struct usb_descriptor_header *)&ac_header_desc, > > > > @@ -293,6 +305,7 @@ static struct usb_descriptor_header > > *f_audio_desc[] = { > > }; > > > > enum { > > + STR_ASSOC, > > Again, why first? follow uac2 driver > > > STR_AC_IF, > > STR_USB_OUT_IT, > > STR_USB_OUT_IT_CH_NAMES, > > @@ -310,6 +323,7 @@ enum { > > > > static struct usb_string strings_uac1[] = { > > /* [STR_AC_IF].s = DYNAMIC, */ > > + [STR_ASSOC].s = "Source/Sink", > > [STR_USB_OUT_IT].s = "Playback Input terminal", > > [STR_USB_OUT_IT_CH_NAMES].s = "Playback Channels", > > [STR_IO_OUT_OT].s = "Playback Output terminal", > > @@ -1058,6 +1072,7 @@ static void setup_descriptor(struct > > f_uac1_opts *opts) > > as_out_header_desc.bTerminalLink = usb_out_it_desc.bTerminalID; > > as_in_header_desc.bTerminalLink = usb_in_ot_desc.bTerminalID; > > > > + iad_desc.bInterfaceCount = 1; > > Why this change? FS, HS may be different, count up them again. > > > > ac_header_desc->wTotalLength = cpu_to_le16(ac_header_desc- > > >bLength); > > > > if (EPIN_EN(opts)) { > > @@ -1068,6 +1083,7 @@ static void setup_descriptor(struct > > f_uac1_opts *opts) > > if (FUIN_EN(opts)) > > len += in_feature_unit_desc->bLength; > > ac_header_desc->wTotalLength = cpu_to_le16(len); > > + iad_desc.bInterfaceCount++; > > } > > if (EPOUT_EN(opts)) { > > u16 len = le16_to_cpu(ac_header_desc->wTotalLength); > > @@ -1077,9 +1093,11 @@ static void setup_descriptor(struct > > f_uac1_opts *opts) > > if (FUOUT_EN(opts)) > > len += out_feature_unit_desc->bLength; > > ac_header_desc->wTotalLength = cpu_to_le16(len); > > + iad_desc.bInterfaceCount++; > > } > > > > i = 0; > > + f_audio_desc[i++] = USBDHDR(&iad_desc); > > Again, why first? It is a requirement as ECN says. > > > f_audio_desc[i++] = USBDHDR(&ac_interface_desc); > > f_audio_desc[i++] = USBDHDR(ac_header_desc); > > > > @@ -1217,6 +1235,7 @@ static int f_audio_bind(struct > > usb_configuration *c, struct usb_function *f) > > } > > } > > > > + iad_desc.iFunction = us[STR_ASSOC].id; > > ac_interface_desc.iInterface = us[STR_AC_IF].id; > > usb_out_it_desc.iTerminal = us[STR_USB_OUT_IT].id; > > usb_out_it_desc.iChannelNames = us[STR_USB_OUT_IT_CH_NAMES].id; > > @@ -1302,6 +1321,8 @@ static int f_audio_bind(struct > > usb_configuration *c, struct usb_function *f) > > status = usb_interface_id(c, f); > > if (status < 0) > > goto err_free_fu; > > + > > + iad_desc.bFirstInterface = status; > > Shouldn't this be needed without your change? Need update, it's not always 0. Thanks a lot > > thanks, > > greg k-h
diff --git a/drivers/usb/gadget/function/f_uac1.c b/drivers/usb/gadget/function/f_uac1.c index 6f0e1d803dc2..8390207bc513 100644 --- a/drivers/usb/gadget/function/f_uac1.c +++ b/drivers/usb/gadget/function/f_uac1.c @@ -71,6 +71,17 @@ static inline struct f_uac1_opts *g_audio_to_uac1_opts(struct g_audio *audio) * ALSA_Playback -> IT_3 -> OT_4 -> USB-IN */ +static struct usb_interface_assoc_descriptor iad_desc = { + .bLength = sizeof(iad_desc), + .bDescriptorType = USB_DT_INTERFACE_ASSOCIATION, + + .bFirstInterface = 0, + .bInterfaceCount = 3, + .bFunctionClass = USB_CLASS_AUDIO, + .bFunctionSubClass = 0, + .bFunctionProtocol = UAC_VERSION_1, +}; + /* B.3.1 Standard AC Interface Descriptor */ static struct usb_interface_descriptor ac_interface_desc = { .bLength = USB_DT_INTERFACE_SIZE, @@ -259,6 +270,7 @@ static struct uac_iso_endpoint_descriptor as_iso_in_desc = { }; static struct usb_descriptor_header *f_audio_desc[] = { + (struct usb_descriptor_header *)&iad_desc, (struct usb_descriptor_header *)&ac_interface_desc, (struct usb_descriptor_header *)&ac_header_desc, @@ -293,6 +305,7 @@ static struct usb_descriptor_header *f_audio_desc[] = { }; enum { + STR_ASSOC, STR_AC_IF, STR_USB_OUT_IT, STR_USB_OUT_IT_CH_NAMES, @@ -310,6 +323,7 @@ enum { static struct usb_string strings_uac1[] = { /* [STR_AC_IF].s = DYNAMIC, */ + [STR_ASSOC].s = "Source/Sink", [STR_USB_OUT_IT].s = "Playback Input terminal", [STR_USB_OUT_IT_CH_NAMES].s = "Playback Channels", [STR_IO_OUT_OT].s = "Playback Output terminal", @@ -1058,6 +1072,7 @@ static void setup_descriptor(struct f_uac1_opts *opts) as_out_header_desc.bTerminalLink = usb_out_it_desc.bTerminalID; as_in_header_desc.bTerminalLink = usb_in_ot_desc.bTerminalID; + iad_desc.bInterfaceCount = 1; ac_header_desc->wTotalLength = cpu_to_le16(ac_header_desc->bLength); if (EPIN_EN(opts)) { @@ -1068,6 +1083,7 @@ static void setup_descriptor(struct f_uac1_opts *opts) if (FUIN_EN(opts)) len += in_feature_unit_desc->bLength; ac_header_desc->wTotalLength = cpu_to_le16(len); + iad_desc.bInterfaceCount++; } if (EPOUT_EN(opts)) { u16 len = le16_to_cpu(ac_header_desc->wTotalLength); @@ -1077,9 +1093,11 @@ static void setup_descriptor(struct f_uac1_opts *opts) if (FUOUT_EN(opts)) len += out_feature_unit_desc->bLength; ac_header_desc->wTotalLength = cpu_to_le16(len); + iad_desc.bInterfaceCount++; } i = 0; + f_audio_desc[i++] = USBDHDR(&iad_desc); f_audio_desc[i++] = USBDHDR(&ac_interface_desc); f_audio_desc[i++] = USBDHDR(ac_header_desc); @@ -1217,6 +1235,7 @@ static int f_audio_bind(struct usb_configuration *c, struct usb_function *f) } } + iad_desc.iFunction = us[STR_ASSOC].id; ac_interface_desc.iInterface = us[STR_AC_IF].id; usb_out_it_desc.iTerminal = us[STR_USB_OUT_IT].id; usb_out_it_desc.iChannelNames = us[STR_USB_OUT_IT_CH_NAMES].id; @@ -1302,6 +1321,8 @@ static int f_audio_bind(struct usb_configuration *c, struct usb_function *f) status = usb_interface_id(c, f); if (status < 0) goto err_free_fu; + + iad_desc.bFirstInterface = status; ac_interface_desc.bInterfaceNumber = status; uac1->ac_intf = status; uac1->ac_alt = 0;