On Wed, 2017-11-29 at 10:55 +0000, Jorge Sanjuan wrote:
The Basic Audio Device 3 (BADD) spec requires the host to have "inferred" descriptors when a BADD profile ID is specified. This descriptor generator creates a buffer with known descriptors for each profile that can be then read to create the alsa audio device(s). The UAC3 compliant devices should have one configuration that is BADD compliant.
The USB request from host are bypassed and these buffers are read instead.
This patch series covers (for now) the following topologies:
- HEADPHONE. - HEADSET.
For more information refer to the spec at:
http://www.usb.org/developers/docs/devclass_docs/
Signed-off-by: Jorge Sanjuan jorge.sanjuan@codethink.co.uk
[...]
--- /dev/null +++ b/sound/usb/badd.c @@ -0,0 +1,495 @@ +/*
- * USB: Audio Class 3: support for BASIC AUDIO DEVICE v.3
- * Author: Jorge Sanjuan jorge.sanjuan@codethink.co.uk
- * Copyright (C) 2017 Codethink Ltd.
- * This program is free software; you can redistribute it and/or modify
Drop the GPL boilerplate and add an SPDX licence identifier instead (see commit b24413180f56).
[...]
+struct uac3_feature_unit_descriptor inf_feat_unit_id2 = {
- .bLength = 0x0F, /* 0x13 if stereo */
- .bDescriptorType = USB_DT_CS_INTERFACE,
- .bDescriptorSubtype = UAC3_FEATURE_UNIT,
- .bUnitID = 0x02,
- /* bSourceID -> Profile dependent */
- .bmaControls[0] = 0x03, /* Mute */
- .bmaControls[1] = 0x0C, /* Chn1 Vol */
- /* If stereo */
- //.bmaControls[2] = 0x0C, /* Chn2 Vol */
Kernel style is to use only /* */ comments, not // comments.
[...]
+/**
- badd_gen_cluster_desc - This is to bypass the GET_HIGH_CAPABILITY
- UAC3 requests for getting cluster descriptors and, instead, generate
- the cluster on the host side as per BADD specification.
- */
A kernel-doc comment mst have a one-line summary and then a description of each parameter. This function probably doesn't need that kind of documentation, so you could drop the extra * from the comment opener.
+void * badd_gen_cluster_desc(unsigned int m_s)
No space after the *. Use checkpatch.pl to check for trivial style issues like this.
+{
- struct uac3_cluster_header_descriptor cluster;
- struct uac3_cluster_information_segment_descriptor segment;
- struct uac3_cluster_end_segment_descriptor end;
- void *buffer;
- int pos = 0;
- int length;
- end.wLength = 0x03;
I think this needs to be a little-endian, in which case you need to use cpu_to_le16(0x03).
Similarly for all the other 16/32-bit fields in descriptors.
- end.bSegmentType = UAC3_END_SEGMENT;
- if (m_s == 0x01) { /* Mono */
Rather than commenting that this constant means Mono, you should name it with an enum or macro. (Or if the variable is actually a number of channels, then you should rename it.)
length = 0x10;
Where does this number come from? Shouldn't it be written as sizeof(cluster) + sizeof(segment) + sizeof(end)?
[...]
+/**
- badd_gen_csint_desc - generates a buffer with the inferred
- descriptors that are predefined in the BADD specfication.
A kernel-doc summary has to be a single line. This is not just a recommendation, it's a hard constraint of the script.
There are a lot more magic numbers in here that ought to be replaced by either named constants or an expression using sizeof().
[...]
--- a/sound/usb/stream.c +++ b/sound/usb/stream.c @@ -37,6 +37,7 @@ #include "format.h" #include "clock.h" #include "stream.h" +#include "badd.h" /* * free a substream @@ -485,7 +486,11 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip, struct usb_interface_descriptor *altsd = get_iface_desc(alts); int attributes = 0;
- csep = snd_usb_find_desc(alts->endpoint[0].extra, alts->endpoint[0].extralen, NULL, USB_DT_CS_ENDPOINT);
- if (protocol == UAC_VERSION_3 &&
chip->badd_profile > UAC3_FUNCTION_SUBCLASS_FULL_ADC_3_0)
This looks excessively indented.
csep = badd_get_ep_dec();- else
csep = snd_usb_find_desc(alts->endpoint[0].extra, alts->endpoint[0].extralen, NULL, USB_DT_CS_ENDPOINT);/* Creamware Noah has this descriptor after the 2nd endpoint */ if (!csep && altsd->bNumEndpoints >= 2)
[...]
@@ -715,11 +723,33 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no) struct uac3_as_header_descriptor *as; struct uac3_cluster_header_descriptor *cluster; struct uac3_hc_descriptor_header hc_header;
struct usb_host_interface *badd_ctrl_intf;void *badd_extra;int badd_extralen;u16 cluster_id, wLength;
as = snd_usb_find_csint_desc(alts->extra,alts->extralen,NULL, UAC_AS_GENERAL);
badd_ctrl_intf = kzalloc(sizeof(*badd_ctrl_intf), GFP_KERNEL);
Missing an error check here.
memcpy(badd_ctrl_intf, chip->ctrl_intf, sizeof(*badd_ctrl_intf));if (chip->badd_profile > UAC3_FUNCTION_SUBCLASS_FULL_ADC_3_0) {as = badd_get_as_iface(stream, ep_packsize);err = badd_gen_csint_desc(&badd_extra, chip->badd_profile, as->wClusterDescrID);if (err <= 0 || !badd_extra) {dev_err(&dev->dev,"%u:%d : Cannot set BADD profile 0x%x. err=%d. badd_extra %p\n",iface_no, altno, chip->badd_profile, err, badd_extra);return err;}badd_extralen = err;badd_ctrl_intf->extra = badd_extra;badd_ctrl_intf->extralen = badd_extralen;
I don't think the badd_extra or badd_extralen variables are really needed here - they just seem to complicate this.
} elseas = snd_usb_find_csint_desc(alts->extra,alts->extralen,NULL, UAC_AS_GENERAL);if (!as) { dev_err(&dev->dev, @@ -743,53 +773,64 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no) continue; }
/** Get number of channels and channel map through* High Capability Cluster Descriptor** First step: get High Capability header and* read size of Cluster Descriptor*/err = snd_usb_ctl_msg(chip->dev,usb_rcvctrlpipe(chip->dev, 0),UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,cluster_id,snd_usb_ctrl_intf(chip),&hc_header, sizeof(hc_header));if (err < 0)return err;else if (err != sizeof(hc_header)) {dev_err(&dev->dev,"%u:%d : can't get High Capability descriptor\n",iface_no, altno);return -EIO;}/** Second step: allocate needed amount of memory* and request Cluster Descriptor*/wLength = le16_to_cpu(hc_header.wLength);cluster = kzalloc(wLength, GFP_KERNEL);if (!cluster)return -ENOMEM;err = snd_usb_ctl_msg(chip->dev,usb_rcvctrlpipe(chip->dev, 0),UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,cluster_id,snd_usb_ctrl_intf(chip),cluster, wLength);if (err < 0) {kfree(cluster);return err;} else if (err != wLength) {dev_err(&dev->dev,"%u:%d : can't get Cluster Descriptor\n",iface_no, altno);kfree(cluster);return -EIO;
if (chip->badd_profile > UAC3_FUNCTION_SUBCLASS_FULL_ADC_3_0) {cluster = badd_gen_cluster_desc(cluster_id);if (!cluster) {dev_err(&dev->dev,"%u:%d : can't get Cluster Descriptor\n",iface_no, altno);return -ENOMEM;}} else {/** Get number of channels and channel map through* High Capability Cluster Descriptor** First step: get High Capability header and* read size of Cluster Descriptor*/err = snd_usb_ctl_msg(chip->dev,usb_rcvctrlpipe(chip->dev, 0),UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,cluster_id,snd_usb_ctrl_intf(chip),&hc_header, sizeof(hc_header));if (err < 0)return err;else if (err != sizeof(hc_header)) {dev_err(&dev->dev,"%u:%d : can't get High Capability descriptor\n",iface_no, altno);return -EIO;}/** Second step: allocate needed amount of memory* and request Cluster Descriptor*/wLength = le16_to_cpu(hc_header.wLength);cluster = kzalloc(wLength, GFP_KERNEL);if (!cluster)return -ENOMEM;err = snd_usb_ctl_msg(chip->dev,usb_rcvctrlpipe(chip->dev, 0),UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,cluster_id,snd_usb_ctrl_intf(chip),cluster, wLength);if (err < 0) {kfree(cluster);return err;} else if (err != wLength) {dev_err(&dev->dev,"%u:%d : can't get Cluster Descriptor\n",iface_no, altno);kfree(cluster);return -EIO;}
This is a large block of code getting indented many levels deep. I can't help thinking that it also ought to be turned into a separate function.
All the error cases in this block will now leak badd_ctrl_inf, and it would be easier to avoid that if it's turned into a separate function.
} num_channels = cluster->bNrChannels; @@ -802,7 +843,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no) /* lookup the terminal associated to this interface * to extract the clock */ input_term = snd_usb_find_input_terminal_descriptor(
chip->ctrl_intf,
badd_ctrl_intf,as->bTerminalLink); if (input_term) { @@ -810,7 +851,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no) break; }
output_term = snd_usb_find_output_terminal_descriptor(chip->ctrl_intf,
output_term = snd_usb_find_output_terminal_descriptor(badd_ctrl_intf,as->bTerminalLink); if (output_term) { clock = output_term->bCSourceID;
[...]
It looks like badd_ctrl_inf *always* gets leaked.
Ben.