[PATCH 0/4] ALSA: usb: UAC3 new features.

[PATCH 0/4] ALSA: usb: UAC3 new features.

[Jorge Sanjuan]

Now that the UAC3 patch [1] has made it to linux-next I have some extra
features to make a UAC3 device fully work in Linux. Including Jack 
insertion control that I have put on top of this other patch [2] for 
UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
to appear in most headset type devices.

UAC3 devices also require to have a Basic Audio Device (BADD) in a separate 
config for which both Ruslan Bilovol and myself have submited different 
approaches[3][4] but I don't know what the final merge will be. Once there 
is official support for BADD, we'll need to test it with an actual UAC3 
device to confirm it all wokrs.

All this features are tested with an actual UAC3 device that is still in
development. For this patch series, only the legacy config (#1. UAC1/UAC2)
and the UAC3 config have been tested. The BADD config is only tested using
and updated verison of [4].

[1]: https://patchwork.kernel.org/patch/10298179/
[2]: https://patchwork.kernel.org/patch/10305847/
[3]: https://patchwork.kernel.org/patch/10340851/
[4]: https://www.spinics.net/lists/alsa-devel/msg71617.html

Based on linux-next tag: next-20180420

Jorge Sanjuan (3):
  ALSA: usb-audio: UAC3. Add support for mixer unit.
  ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
  ALSA: usb-audio: UAC3 Add support for connector insertion.

Michael Drake (1):
  ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.

 include/linux/usb/audio-v2.h   |   7 ++
 include/linux/usb/audio-v3.h   |  14 ++++
 include/uapi/linux/usb/audio.h |  13 ++-
 sound/usb/mixer.c              | 175 ++++++++++++++++++++++++++++++++++++-----
 sound/usb/stream.c             |  19 ++++-
 5 files changed, 204 insertions(+), 24 deletions(-)

-- 
2.11.0

[PATCH 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.

[Jorge Sanjuan]

This adds support for the MIXER UNIT in UAC3. All the information
is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
read the rest of the logical cluster to obtain the channel config
as that wont make any difference in the current mixer behaviour.

The name of the mixer unit is not yet requested as there is not
support for the UAC3 Class Specific String requests.

Tested in an UAC3 device working as a HEADSET with a basic mixer
unit (same as the one in the BADD spec) with no controls.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 include/uapi/linux/usb/audio.h | 13 +++++++--
 sound/usb/mixer.c              | 64 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
index 3a78e7145689..f9be472cd025 100644
--- a/include/uapi/linux/usb/audio.h
+++ b/include/uapi/linux/usb/audio.h
@@ -285,9 +285,16 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
 static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
 					      int protocol)
 {
-	return (protocol == UAC_VERSION_1) ?
-		&desc->baSourceID[desc->bNrInPins + 4] :
-		&desc->baSourceID[desc->bNrInPins + 6];
+	switch (protocol) {
+	case UAC_VERSION_1:
+		return &desc->baSourceID[desc->bNrInPins + 4];
+	case UAC_VERSION_2:
+		return &desc->baSourceID[desc->bNrInPins + 6];
+	case UAC_VERSION_3:
+		return &desc->baSourceID[desc->bNrInPins + 2];
+	default:
+		return NULL;
+	}
 }
 
 static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 301ad61ed426..738ca37e6d6e 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -719,6 +719,35 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
 }
 
 /*
+ * Get logical cluster information for UAC3 devices.
+ */
+static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
+{
+	struct uac3_cluster_header_descriptor c_header;
+	int err;
+
+	err = snd_usb_ctl_msg(state->chip->dev,
+			usb_rcvctrlpipe(state->chip->dev, 0),
+			UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
+			USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+			cluster_id,
+			snd_usb_ctrl_intf(state->chip),
+			&c_header, sizeof(c_header));
+	if (err < 0)
+		goto error;
+	else if (err != sizeof(c_header)) {
+		err = -EIO;
+		goto error;
+	}
+
+	return c_header.bNrChannels;
+
+error:
+	usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
+	return err;
+}
+
+/*
  * parse the source unit recursively until it reaches to a terminal
  * or a branched unit.
  */
@@ -865,6 +894,19 @@ static int check_input_term(struct mixer_build *state, int id,
 				term->name = le16_to_cpu(d->wClockSourceStr);
 				return 0;
 			}
+			case UAC3_MIXER_UNIT: {
+				struct uac_mixer_unit_descriptor *d = p1;
+				unsigned int cluster_id = le16_to_cpu(d->baSourceID[d->bNrInPins]);
+
+				err = get_cluster_channels_v3(state, cluster_id);
+				if (err < 0)
+					return err;
+
+				term->channels = err;
+				term->type = d->bDescriptorSubtype << 16; /* virtual type */
+
+				return 0;
+			}
 			default:
 				return -ENODEV;
 			}
@@ -1801,7 +1843,7 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
 				 struct usb_audio_term *iterm)
 {
 	struct usb_mixer_elem_info *cval;
-	unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
+	unsigned int num_outs;
 	unsigned int i, len;
 	struct snd_kcontrol *kctl;
 	const struct usbmix_name_map *map;
@@ -1814,6 +1856,14 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
 	if (!cval)
 		return;
 
+	if (state->mixer->protocol == UAC_VERSION_3) {
+		num_outs = get_cluster_channels_v3(state,
+				le16_to_cpu(desc->baSourceID[desc->bNrInPins]));
+		if (num_outs < 0)
+			return;
+	} else /* UAC_VERSION_1/2 */
+		num_outs = uac_mixer_unit_bNrChannels(desc);
+
 	snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid);
 	cval->control = in_ch + 1; /* based on 1 */
 	cval->val_type = USB_MIXER_S16;
@@ -1878,8 +1928,16 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
 	int input_pins, num_ins, num_outs;
 	int pin, ich, err;
 
-	if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
-	    !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
+	if (state->mixer->protocol == UAC_VERSION_3) {
+		err = get_cluster_channels_v3(state,
+				le16_to_cpu(desc->baSourceID[desc->bNrInPins]));
+		if (err < 0)
+			return err;
+		num_outs = err;
+	} else /* UAC_VERSION_1/2 */
+		num_outs = uac_mixer_unit_bNrChannels(desc);
+
+	if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) || !num_outs) {
 		usb_audio_err(state->chip,
 			      "invalid MIXER UNIT descriptor %d\n",
 			      unitid);
-- 
2.11.0

[PATCH 2/4] ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.

[Jorge Sanjuan]

From: Michael Drake <michael.drake@codethink.co.uk>

The channel mapping is defined by bChRelationship, not bChPurpose.

Signed-off-by: Michael Drake <michael.drake@codethink.co.uk>
---
 sound/usb/stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 6a8f5843334e..956be9f7c72a 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -349,7 +349,7 @@ snd_pcm_chmap_elem *convert_chmap_v3(struct uac3_cluster_header_descriptor
 			 * TODO: this conversion is not complete, update it
 			 * after adding UAC3 values to asound.h
 			 */
-			switch (is->bChPurpose) {
+			switch (is->bChRelationship) {
 			case UAC3_CH_MONO:
 				map = SNDRV_CHMAP_MONO;
 				break;
-- 
2.11.0

[PATCH 3/4] ALSA: usb-audio: Use Class Specific EP for UAC3 devices.

[Jorge Sanjuan]

bmAtributes offset doesn't exist in the UAC3 CS_EP descriptor.
Hence, checking for pitch control as if it was UAC2 doesn't make
any sense. Use the defined UAC3 offsets instead.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 sound/usb/stream.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 956be9f7c72a..344e0ecf6d59 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -574,9 +574,11 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
 		return 0;
 	}
 
-	if (protocol == UAC_VERSION_1) {
+	switch (protocol) {
+	case UAC_VERSION_1:
 		attributes = csep->bmAttributes;
-	} else {
+		break;
+	case UAC_VERSION_2: {
 		struct uac2_iso_endpoint_descriptor *csep2 =
 			(struct uac2_iso_endpoint_descriptor *) csep;
 
@@ -585,6 +587,17 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
 		/* emulate the endpoint attributes of a v1 device */
 		if (csep2->bmControls & UAC2_CONTROL_PITCH)
 			attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
+		break;
+	}
+	case UAC_VERSION_3: {
+		struct uac3_iso_endpoint_descriptor *csep3 =
+			(struct uac3_iso_endpoint_descriptor *) csep;
+
+		/* emulate the endpoint attributes of a v1 device */
+		if (csep3->bmControls & UAC2_CONTROL_PITCH)
+			attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
+		break;
+	}
 	}
 
 	return attributes;
-- 
2.11.0

[PATCH 4/4] ALSA: usb-audio: UAC3 Add support for connector insertion.

[Jorge Sanjuan]

This adds support for the UAC3 insertion controls. The status
is reported as a boolean value in the same way it used to do
for UAC2. Hence, the presence of any connector in the response
will make the control saying the jack is connected.

The UAC2 support for this control has been moved to a dedicated
control for connectors as both UAC2 and UAC3 follow a specific
Control Request Parameter Block for this control. This parameter
block for UAC3 could not be read in the same simplistic
manner as in UAC2.

This implementation is not requesting additional information
from the HIGH CAPABILITY Connectors descriptor.

Tested with an UAC3 device with UAC2 as legacy configuration.
The connector status can be read with `amixer` and the interrupt
is also caught with `alsactl monitor`.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 include/linux/usb/audio-v2.h |   7 +++
 include/linux/usb/audio-v3.h |  14 ++++++
 sound/usb/mixer.c            | 111 +++++++++++++++++++++++++++++++++++++------
 3 files changed, 117 insertions(+), 15 deletions(-)

diff --git a/include/linux/usb/audio-v2.h b/include/linux/usb/audio-v2.h
index aaafecf073ff..a96ed2ce3254 100644
--- a/include/linux/usb/audio-v2.h
+++ b/include/linux/usb/audio-v2.h
@@ -189,6 +189,13 @@ struct uac2_iso_endpoint_descriptor {
 #define UAC2_CONTROL_DATA_OVERRUN	(3 << 2)
 #define UAC2_CONTROL_DATA_UNDERRUN	(3 << 4)
 
+/* 5.2.5.4.2 Connector Control Parameter Block */
+struct uac2_connectors_ctl_blk {
+	__u8 bNrChannels;
+	__le32 bmChannelConfig;
+	__u8 iChannelNames;
+} __attribute__((packed));
+
 /* 6.1 Interrupt Data Message */
 
 #define UAC2_INTERRUPT_DATA_MSG_VENDOR	(1 << 0)
diff --git a/include/linux/usb/audio-v3.h b/include/linux/usb/audio-v3.h
index a8959aaba0ae..6cedb6d499ba 100644
--- a/include/linux/usb/audio-v3.h
+++ b/include/linux/usb/audio-v3.h
@@ -221,6 +221,12 @@ struct uac3_iso_endpoint_descriptor {
 	__le16 wLockDelay;
 } __attribute__((packed));
 
+/* 5.2.1.6.1 INSERTION CONTROL PARAMETER BLOCK */
+struct uac3_insertion_ctl_blk {
+	__u8 bSize;
+	__u8 bmConInserted[1];
+} __attribute__ ((packed));
+
 /* 6.1 INTERRUPT DATA MESSAGE */
 struct uac3_interrupt_data_msg {
 	__u8 bInfo;
@@ -392,4 +398,12 @@ struct uac3_interrupt_data_msg {
 #define UAC3_AC_ACTIVE_INTERFACE_CONTROL	0x01
 #define UAC3_AC_POWER_DOMAIN_CONTROL		0x02
 
+/* A.23.5 TERMINAL CONTROL SELECTORS */
+#define UAC3_TE_UNDEFINED			0x00
+#define UAC3_TE_INSERTION			0x01
+#define UAC3_TE_OVERLOAD			0x02
+#define UAC3_TE_UNDERFLOW			0x03
+#define UAC3_TE_OVERFLOW			0x04
+#define UAC3_TE_LATENCY 			0x05
+
 #endif /* __LINUX_USB_AUDIO_V3_H */
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 738ca37e6d6e..4ddc5d4e1139 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1292,6 +1292,51 @@ static int mixer_ctl_master_bool_get(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
+/* get the connectors status and report it as boolean type */
+static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct usb_mixer_elem_info *cval = kcontrol->private_data;
+	struct snd_usb_audio *chip = cval->head.mixer->chip;
+	int idx = 0, validx, ret, val;
+
+	validx = cval->control << 8 | 0;
+
+	ret = snd_usb_lock_shutdown(chip) ? -EIO : 0;
+	if (ret)
+		goto error;
+
+	idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8);
+	if (cval->head.mixer->protocol == UAC_VERSION_2) {
+		struct uac2_connectors_ctl_blk uac2_conn;
+
+		ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), UAC2_CS_CUR,
+				      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+				      validx, idx, &uac2_conn, sizeof(uac2_conn));
+		val = !!uac2_conn.bNrChannels;
+	} else { /* UAC_VERSION_3 */
+		struct uac3_insertion_ctl_blk uac3_conn;
+
+		ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), UAC2_CS_CUR,
+				      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+				      validx, idx, &uac3_conn, sizeof(uac3_conn));
+		val = !!uac3_conn.bmConInserted[0];
+	}
+
+	snd_usb_unlock_shutdown(chip);
+
+	if (ret < 0) {
+error:
+		usb_audio_err(chip,
+			"cannot get connectors status: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n",
+			UAC_GET_CUR, validx, idx, cval->val_type);
+		return ret;
+	}
+
+	ucontrol->value.integer.value[0] = val;
+	return 0;
+}
+
 static struct snd_kcontrol_new usb_feature_unit_ctl = {
 	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
 	.name = "", /* will be filled later manually */
@@ -1322,6 +1367,15 @@ static struct snd_kcontrol_new usb_bool_master_control_ctl_ro = {
 	.put = NULL,
 };
 
+static struct snd_kcontrol_new usb_connector_ctl_ro = {
+	.iface = SNDRV_CTL_ELEM_IFACE_CARD,
+	.name = "", /* will be filled later manually */
+	.access = SNDRV_CTL_ELEM_ACCESS_READ,
+	.info = snd_ctl_boolean_mono_info,
+	.get = mixer_ctl_connector_get,
+	.put = NULL,
+};
+
 /*
  * This symbol is exported in order to allow the mixer quirks to
  * hook up to the standard feature unit control mechanism
@@ -1568,17 +1622,25 @@ static void build_connector_control(struct mixer_build *state,
 		return;
 	snd_usb_mixer_elem_init_std(&cval->head, state->mixer, term->id);
 	/*
-	 * The first byte from reading the UAC2_TE_CONNECTOR control returns the
-	 * number of channels connected.  This boolean ctl will simply report
-	 * if any channels are connected or not.
-	 * (Audio20_final.pdf Table 5-10: Connector Control CUR Parameter Block)
+	 * UAC2: The first byte from reading the UAC2_TE_CONNECTOR control returns the
+	 * number of channels connected.
+	 *
+	 * UAC3: The first byte specifies size of bitmap for the inserted controls. The
+	 * following byte(s) specifies which connectors are inserted.
+	 *
+	 * This boolean ctl will simply report if any channels are connected
+	 * or not.
 	 */
-	cval->control = UAC2_TE_CONNECTOR;
+	if (state->mixer->protocol == UAC_VERSION_2)
+		cval->control = UAC2_TE_CONNECTOR;
+	else /* UAC_VERSION_3 */
+		cval->control = UAC3_TE_INSERTION;
+
 	cval->val_type = USB_MIXER_BOOLEAN;
 	cval->channels = 1; /* report true if any channel is connected */
 	cval->min = 0;
 	cval->max = 1;
-	kctl = snd_ctl_new1(&usb_bool_master_control_ctl_ro, cval);
+	kctl = snd_ctl_new1(&usb_connector_ctl_ro, cval);
 	if (!kctl) {
 		usb_audio_err(state->chip, "cannot malloc kcontrol\n");
 		kfree(cval);
@@ -1904,16 +1966,29 @@ static int parse_audio_input_terminal(struct mixer_build *state, int unitid,
 				      void *raw_desc)
 {
 	struct usb_audio_term iterm;
-	struct uac2_input_terminal_descriptor *d = raw_desc;
+	unsigned int control, bmctls, term_id;
 
-	check_input_term(state, d->bTerminalID, &iterm);
 	if (state->mixer->protocol == UAC_VERSION_2) {
-		/* Check for jack detection. */
-		if (uac_v2v3_control_is_readable(d->bmControls,
-						 UAC2_TE_CONNECTOR)) {
-			build_connector_control(state, &iterm, true);
-		}
-	}
+		struct uac2_input_terminal_descriptor *d_v2 = raw_desc;
+		control = UAC2_TE_CONNECTOR;
+		term_id = d_v2->bTerminalID;
+		bmctls = d_v2->bmControls;
+	}
+	else if (state->mixer->protocol == UAC_VERSION_3) {
+		struct uac3_input_terminal_descriptor *d_v3 = raw_desc;
+		control = UAC3_TE_INSERTION;
+		term_id = d_v3->bTerminalID;
+		bmctls = d_v3->bmControls;
+	}
+	else /* UAC1. No Insertion control */
+		return 0;
+
+	check_input_term(state, term_id, &iterm);
+
+	/* Check for jack detection. */
+	if (uac_v2v3_control_is_readable(bmctls, control))
+		build_connector_control(state, &iterm, true);
+
 	return 0;
 }
 
@@ -2507,7 +2582,7 @@ static int parse_audio_unit(struct mixer_build *state, int unitid)
 	} else { /* UAC_VERSION_3 */
 		switch (p1[2]) {
 		case UAC_INPUT_TERMINAL:
-			return 0; /* NOP */
+			return parse_audio_input_terminal(state, unitid, p1);
 		case UAC3_MIXER_UNIT:
 			return parse_audio_mixer_unit(state, unitid, p1);
 		case UAC3_CLOCK_SOURCE:
@@ -2645,6 +2720,12 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer)
 			err = parse_audio_unit(&state, desc->bCSourceID);
 			if (err < 0 && err != -EINVAL)
 				return err;
+
+			if (uac_v2v3_control_is_readable(desc->bmControls,
+							 UAC3_TE_INSERTION)) {
+				build_connector_control(&state, &state.oterm,
+							false);
+			}
 		}
 	}
 
-- 
2.11.0

Re: [alsa-devel] [PATCH 3/4] ALSA: usb-audio: Use Class Specific EP for UAC3 devices.

[kbuild test robot]

Hi Jorge,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on sound/for-next]
[also build test WARNING on v4.17-rc1 next-20180420]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jorge-Sanjuan/ALSA-usb-UAC3-new-features/20180423-015726
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> sound/usb/stream.c:597:26: sparse: restricted __le32 degrades to integer

vim +597 sound/usb/stream.c

   544	
   545	static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
   546						 struct usb_host_interface *alts,
   547						 int protocol, int iface_no)
   548	{
   549		/* parsed with a v1 header here. that's ok as we only look at the
   550		 * header first which is the same for both versions */
   551		struct uac_iso_endpoint_descriptor *csep;
   552		struct usb_interface_descriptor *altsd = get_iface_desc(alts);
   553		int attributes = 0;
   554	
   555		csep = snd_usb_find_desc(alts->endpoint[0].extra, alts->endpoint[0].extralen, NULL, USB_DT_CS_ENDPOINT);
   556	
   557		/* Creamware Noah has this descriptor after the 2nd endpoint */
   558		if (!csep && altsd->bNumEndpoints >= 2)
   559			csep = snd_usb_find_desc(alts->endpoint[1].extra, alts->endpoint[1].extralen, NULL, USB_DT_CS_ENDPOINT);
   560	
   561		/*
   562		 * If we can't locate the USB_DT_CS_ENDPOINT descriptor in the extra
   563		 * bytes after the first endpoint, go search the entire interface.
   564		 * Some devices have it directly *before* the standard endpoint.
   565		 */
   566		if (!csep)
   567			csep = snd_usb_find_desc(alts->extra, alts->extralen, NULL, USB_DT_CS_ENDPOINT);
   568	
   569		if (!csep || csep->bLength < 7 ||
   570		    csep->bDescriptorSubtype != UAC_EP_GENERAL) {
   571			usb_audio_warn(chip,
   572				       "%u:%d : no or invalid class specific endpoint descriptor\n",
   573				       iface_no, altsd->bAlternateSetting);
   574			return 0;
   575		}
   576	
   577		switch (protocol) {
   578		case UAC_VERSION_1:
   579			attributes = csep->bmAttributes;
   580			break;
   581		case UAC_VERSION_2: {
   582			struct uac2_iso_endpoint_descriptor *csep2 =
   583				(struct uac2_iso_endpoint_descriptor *) csep;
   584	
   585			attributes = csep->bmAttributes & UAC_EP_CS_ATTR_FILL_MAX;
   586	
   587			/* emulate the endpoint attributes of a v1 device */
   588			if (csep2->bmControls & UAC2_CONTROL_PITCH)
   589				attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
   590			break;
   591		}
   592		case UAC_VERSION_3: {
   593			struct uac3_iso_endpoint_descriptor *csep3 =
   594				(struct uac3_iso_endpoint_descriptor *) csep;
   595	
   596			/* emulate the endpoint attributes of a v1 device */
 > 597			if (csep3->bmControls & UAC2_CONTROL_PITCH)
   598				attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
   599			break;
   600		}
   601		}
   602	
   603		return attributes;
   604	}
   605	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 4/4] ALSA: usb-audio: UAC3 Add support for connector insertion.

[kbuild test robot]

Hi Jorge,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on sound/for-next]
[also build test WARNING on v4.17-rc1 next-20180420]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jorge-Sanjuan/ALSA-usb-UAC3-new-features/20180423-015726
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   sound/usb/mixer.c:899:59: sparse: cast to restricted __le16
   sound/usb/mixer.c:1923:33: sparse: cast to restricted __le16
>> sound/usb/mixer.c:1975:24: sparse: incorrect type in assignment (different base types) @@    expected unsigned int [unsigned] bmctls @@    got restrunsigned int [unsigned] bmctls @@
   sound/usb/mixer.c:1975:24:    expected unsigned int [unsigned] bmctls
   sound/usb/mixer.c:1975:24:    got restricted __le16 [usertype] bmControls
   sound/usb/mixer.c:1981:24: sparse: incorrect type in assignment (different base types) @@    expected unsigned int [unsigned] bmctls @@    got restrunsigned int [unsigned] bmctls @@
   sound/usb/mixer.c:1981:24:    expected unsigned int [unsigned] bmctls
   sound/usb/mixer.c:1981:24:    got restricted __le32 [usertype] bmControls
   sound/usb/mixer.c:2008:33: sparse: cast to restricted __le16
   sound/usb/mixer.c:2697:62: sparse: incorrect type in argument 1 (different base types) @@    expected unsigned int [unsigned] [usertype] bmControls @@    got ed int [unsigned] [usertype] bmControls @@
   sound/usb/mixer.c:2697:62:    expected unsigned int [unsigned] [usertype] bmControls
   sound/usb/mixer.c:2697:62:    got restricted __le16 [usertype] bmControls
   sound/usb/mixer.c:2724:62: sparse: incorrect type in argument 1 (different base types) @@    expected unsigned int [unsigned] [usertype] bmControls @@    got ed int [unsigned] [usertype] bmControls @@
   sound/usb/mixer.c:2724:62:    expected unsigned int [unsigned] [usertype] bmControls
   sound/usb/mixer.c:2724:62:    got restricted __le32 [usertype] bmControls
   include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
   include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
   include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
   include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
   include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
   include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
   include/linux/usb.h:1676:28: sparse: expression using sizeof(void)

vim +1975 sound/usb/mixer.c

  1964	
  1965	static int parse_audio_input_terminal(struct mixer_build *state, int unitid,
  1966					      void *raw_desc)
  1967	{
  1968		struct usb_audio_term iterm;
  1969		unsigned int control, bmctls, term_id;
  1970	
  1971		if (state->mixer->protocol == UAC_VERSION_2) {
  1972			struct uac2_input_terminal_descriptor *d_v2 = raw_desc;
  1973			control = UAC2_TE_CONNECTOR;
  1974			term_id = d_v2->bTerminalID;
> 1975			bmctls = d_v2->bmControls;
  1976		}
  1977		else if (state->mixer->protocol == UAC_VERSION_3) {
  1978			struct uac3_input_terminal_descriptor *d_v3 = raw_desc;
  1979			control = UAC3_TE_INSERTION;
  1980			term_id = d_v3->bTerminalID;
  1981			bmctls = d_v3->bmControls;
  1982		}
  1983		else /* UAC1. No Insertion control */
  1984			return 0;
  1985	
  1986		check_input_term(state, term_id, &iterm);
  1987	
  1988		/* Check for jack detection. */
  1989		if (uac_v2v3_control_is_readable(bmctls, control))
  1990			build_connector_control(state, &iterm, true);
  1991	
  1992		return 0;
  1993	}
  1994	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.

[Takashi Iwai]

On Fri, 20 Apr 2018 19:03:24 +0200,
Jorge Sanjuan wrote:
> 
> This adds support for the MIXER UNIT in UAC3. All the information
> is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
> read the rest of the logical cluster to obtain the channel config
> as that wont make any difference in the current mixer behaviour.
> 
> The name of the mixer unit is not yet requested as there is not
> support for the UAC3 Class Specific String requests.
> 
> Tested in an UAC3 device working as a HEADSET with a basic mixer
> unit (same as the one in the BADD spec) with no controls.
> 
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
> ---
>  include/uapi/linux/usb/audio.h | 13 +++++++--
>  sound/usb/mixer.c              | 64 ++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 71 insertions(+), 6 deletions(-)
> 
> diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
> index 3a78e7145689..f9be472cd025 100644
> --- a/include/uapi/linux/usb/audio.h
> +++ b/include/uapi/linux/usb/audio.h
> @@ -285,9 +285,16 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
>  static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
>  					      int protocol)
>  {
> -	return (protocol == UAC_VERSION_1) ?
> -		&desc->baSourceID[desc->bNrInPins + 4] :
> -		&desc->baSourceID[desc->bNrInPins + 6];
> +	switch (protocol) {
> +	case UAC_VERSION_1:
> +		return &desc->baSourceID[desc->bNrInPins + 4];
> +	case UAC_VERSION_2:
> +		return &desc->baSourceID[desc->bNrInPins + 6];
> +	case UAC_VERSION_3:
> +		return &desc->baSourceID[desc->bNrInPins + 2];
> +	default:
> +		return NULL;
> +	}
>  }
>  
>  static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 301ad61ed426..738ca37e6d6e 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -719,6 +719,35 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
>  }
>  
>  /*
> + * Get logical cluster information for UAC3 devices.
> + */
> +static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
> +{
> +	struct uac3_cluster_header_descriptor c_header;
> +	int err;
> +
> +	err = snd_usb_ctl_msg(state->chip->dev,
> +			usb_rcvctrlpipe(state->chip->dev, 0),
> +			UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
> +			USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
> +			cluster_id,
> +			snd_usb_ctrl_intf(state->chip),
> +			&c_header, sizeof(c_header));
> +	if (err < 0)
> +		goto error;
> +	else if (err != sizeof(c_header)) {
> +		err = -EIO;
> +		goto error;
> +	}

Try to balance to put braces in both if and else.
In this case, though, you can just do like below, too:

	if (err < 0)
		goto error;
	if (err != sizeof(c_header)) {
		err = -EIO;
		goto error;
	}


> +
> +	return c_header.bNrChannels;
> +
> +error:
> +	usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
> +	return err;
> +}
> +
> +/*
>   * parse the source unit recursively until it reaches to a terminal
>   * or a branched unit.
>   */
> @@ -865,6 +894,19 @@ static int check_input_term(struct mixer_build *state, int id,
>  				term->name = le16_to_cpu(d->wClockSourceStr);
>  				return 0;
>  			}
> +			case UAC3_MIXER_UNIT: {
> +				struct uac_mixer_unit_descriptor *d = p1;
> +				unsigned int cluster_id = le16_to_cpu(d->baSourceID[d->bNrInPins]);
> +
> +				err = get_cluster_channels_v3(state, cluster_id);
> +				if (err < 0)
> +					return err;
> +
> +				term->channels = err;
> +				term->type = d->bDescriptorSubtype << 16; /* virtual type */
> +
> +				return 0;
> +			}
>  			default:
>  				return -ENODEV;
>  			}
> @@ -1801,7 +1843,7 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
>  				 struct usb_audio_term *iterm)
>  {
>  	struct usb_mixer_elem_info *cval;
> -	unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
> +	unsigned int num_outs;
>  	unsigned int i, len;
>  	struct snd_kcontrol *kctl;
>  	const struct usbmix_name_map *map;
> @@ -1814,6 +1856,14 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
>  	if (!cval)
>  		return;
>  
> +	if (state->mixer->protocol == UAC_VERSION_3) {
> +		num_outs = get_cluster_channels_v3(state,
> +				le16_to_cpu(desc->baSourceID[desc->bNrInPins]));
> +		if (num_outs < 0)
> +			return;
> +	} else /* UAC_VERSION_1/2 */
> +		num_outs = uac_mixer_unit_bNrChannels(desc);
> +
>  	snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid);
>  	cval->control = in_ch + 1; /* based on 1 */
>  	cval->val_type = USB_MIXER_S16;
> @@ -1878,8 +1928,16 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>  	int input_pins, num_ins, num_outs;
>  	int pin, ich, err;
>  
> -	if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
> -	    !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
> +	if (state->mixer->protocol == UAC_VERSION_3) {
> +		err = get_cluster_channels_v3(state,
> +				le16_to_cpu(desc->baSourceID[desc->bNrInPins]));
> +		if (err < 0)
> +			return err;
> +		num_outs = err;
> +	} else /* UAC_VERSION_1/2 */
> +		num_outs = uac_mixer_unit_bNrChannels(desc);

These three calls are all similar.  Maybe it'd be cleaner if you
introduce another helper to get the channels for MU?

static int uac_mixer_unit_get_channels(struct mixer_build *state,
				struct uac_mixer_unit_descriptor *desc)
{
	int input_pins;
	int num_outs;

	if (desc->bLength < 11)
		return -EINVAL;
	input_pins = desc->bNrInPins;
	if (!input_pins)
		return -EINVAL;

	switch (state->mixer->protocol) {
	case UAC_VERSION_1:
	case UAC_VERSION_2:
	default:
		channels = uac_mixer_unit_bNrChannels(desc);
		break;
	case UAC_VERSION_3:
		channels = get_cluster_channels_v3(state, 
			le16_to_cpu(desc->baSourceID[]);
		break;
	}
	if (!channels)
		return -EINVAL;
	return channels;
}

And, as you can see in the above, you have to do the length check
before actually accessing the descriptor's field.


thanks,

Takashi

Re: [PATCH 2/4] ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.

[Takashi Iwai]

On Fri, 20 Apr 2018 19:03:25 +0200,
Jorge Sanjuan wrote:
> 
> From: Michael Drake <michael.drake@codethink.co.uk>
> 
> The channel mapping is defined by bChRelationship, not bChPurpose.
> 
> Signed-off-by: Michael Drake <michael.drake@codethink.co.uk>

The change looks OK, but your sign-off is missing.
If you submit a patch, please give always your signed-off-by tag, no
matter whether it's your original patch or not.

Also, the Fixes tag would be helpful for such a correction.


thanks,

Takashi

Re: [PATCH 4/4] ALSA: usb-audio: UAC3 Add support for connector insertion.

[Takashi Iwai]

On Fri, 20 Apr 2018 19:03:27 +0200,
Jorge Sanjuan wrote:
> 
> diff --git a/include/linux/usb/audio-v3.h b/include/linux/usb/audio-v3.h
> index a8959aaba0ae..6cedb6d499ba 100644
> --- a/include/linux/usb/audio-v3.h
> +++ b/include/linux/usb/audio-v3.h
> @@ -221,6 +221,12 @@ struct uac3_iso_endpoint_descriptor {
>  	__le16 wLockDelay;
>  } __attribute__((packed));
>  
> +/* 5.2.1.6.1 INSERTION CONTROL PARAMETER BLOCK */
> +struct uac3_insertion_ctl_blk {
> +	__u8 bSize;
> +	__u8 bmConInserted[1];

Do we need to declare this as an array?

>  static struct snd_kcontrol_new usb_feature_unit_ctl = {
>  	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
>  	.name = "", /* will be filled later manually */
> @@ -1322,6 +1367,15 @@ static struct snd_kcontrol_new usb_bool_master_control_ctl_ro = {
>  	.put = NULL,
>  };
>  
> +static struct snd_kcontrol_new usb_connector_ctl_ro = {

Put const.


> @@ -1904,16 +1966,29 @@ static int parse_audio_input_terminal(struct mixer_build *state, int unitid,
>  				      void *raw_desc)
>  {
>  	struct usb_audio_term iterm;
> -	struct uac2_input_terminal_descriptor *d = raw_desc;
> +	unsigned int control, bmctls, term_id;
>  
> -	check_input_term(state, d->bTerminalID, &iterm);
>  	if (state->mixer->protocol == UAC_VERSION_2) {
> -		/* Check for jack detection. */
> -		if (uac_v2v3_control_is_readable(d->bmControls,
> -						 UAC2_TE_CONNECTOR)) {
> -			build_connector_control(state, &iterm, true);
> -		}
> -	}
> +		struct uac2_input_terminal_descriptor *d_v2 = raw_desc;
> +		control = UAC2_TE_CONNECTOR;
> +		term_id = d_v2->bTerminalID;
> +		bmctls = d_v2->bmControls;
> +	}
> +	else if (state->mixer->protocol == UAC_VERSION_3) {

Put "else if" and the closing brace in the same line.


> +		struct uac3_input_terminal_descriptor *d_v3 = raw_desc;
> +		control = UAC3_TE_INSERTION;
> +		term_id = d_v3->bTerminalID;
> +		bmctls = d_v3->bmControls;

Doesn't it need le32_to_cpu()?

> +	}
> +	else /* UAC1. No Insertion control */

Ditto, put "else" and the closing brace in the same line.
Also, use braces for the rest block, otherwise it'll look
inconsistent.  Or rewrite with switch().

> @@ -2645,6 +2720,12 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer)
>  			err = parse_audio_unit(&state, desc->bCSourceID);
>  			if (err < 0 && err != -EINVAL)
>  				return err;
> +
> +			if (uac_v2v3_control_is_readable(desc->bmControls,

Missing le32_to_cpu()?


thanks,

Takashi

Re: [PATCH 4/4] ALSA: usb-audio: UAC3 Add support for connector insertion.

[Jorge]

Hi Takashi,

Thank you very much for the reviews. I'll put a v2 patchset with the 
suggested changes for this patch and the other ones you reviewed.

Some comments below

On 23/04/18 13:19, Takashi Iwai wrote:
> On Fri, 20 Apr 2018 19:03:27 +0200,
> Jorge Sanjuan wrote:
>>
>> diff --git a/include/linux/usb/audio-v3.h b/include/linux/usb/audio-v3.h
>> index a8959aaba0ae..6cedb6d499ba 100644
>> --- a/include/linux/usb/audio-v3.h
>> +++ b/include/linux/usb/audio-v3.h
>> @@ -221,6 +221,12 @@ struct uac3_iso_endpoint_descriptor {
>>   	__le16 wLockDelay;
>>   } __attribute__((packed));
>>   
>> +/* 5.2.1.6.1 INSERTION CONTROL PARAMETER BLOCK */
>> +struct uac3_insertion_ctl_blk {
>> +	__u8 bSize;
>> +	__u8 bmConInserted[1];
> 
> Do we need to declare this as an array?

The size of bmConInserted will be determined by bSize. We could check 
how many connectors are in the device by checking the high capability 
Connectors Descriptor. If the number of connectors was greater than 8 
this block would need to get some more bytes in the request (or we could 
just request the following bytes if bSize was greater than one).

I'll remove the array and leave it as two byte block. That should be 
enough for up to 8 connectors.

> 
>>   static struct snd_kcontrol_new usb_feature_unit_ctl = {
>>   	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
>>   	.name = "", /* will be filled later manually */
>> @@ -1322,6 +1367,15 @@ static struct snd_kcontrol_new usb_bool_master_control_ctl_ro = {
>>   	.put = NULL,
>>   };
>>   
>> +static struct snd_kcontrol_new usb_connector_ctl_ro = {
> 
> Put const.

+1	

> 
> 
>> @@ -1904,16 +1966,29 @@ static int parse_audio_input_terminal(struct mixer_build *state, int unitid,
>>   				      void *raw_desc)
>>   {
>>   	struct usb_audio_term iterm;
>> -	struct uac2_input_terminal_descriptor *d = raw_desc;
>> +	unsigned int control, bmctls, term_id;
>>   
>> -	check_input_term(state, d->bTerminalID, &iterm);
>>   	if (state->mixer->protocol == UAC_VERSION_2) {
>> -		/* Check for jack detection. */
>> -		if (uac_v2v3_control_is_readable(d->bmControls,
>> -						 UAC2_TE_CONNECTOR)) {
>> -			build_connector_control(state, &iterm, true);
>> -		}
>> -	}
>> +		struct uac2_input_terminal_descriptor *d_v2 = raw_desc;
>> +		control = UAC2_TE_CONNECTOR;
>> +		term_id = d_v2->bTerminalID;
>> +		bmctls = d_v2->bmControls;
>> +	}
>> +	else if (state->mixer->protocol == UAC_VERSION_3) {
> 
> Put "else if" and the closing brace in the same line.

Thanks. My bad.

> 
> 
>> +		struct uac3_input_terminal_descriptor *d_v3 = raw_desc;
>> +		control = UAC3_TE_INSERTION;
>> +		term_id = d_v3->bTerminalID;
>> +		bmctls = d_v3->bmControls;
> 
> Doesn't it need le32_to_cpu()?

Agreed.

> 
>> +	}
>> +	else /* UAC1. No Insertion control */
> 
> Ditto, put "else" and the closing brace in the same line.
> Also, use braces for the rest block, otherwise it'll look
> inconsistent.  Or rewrite with switch().
> 
>> @@ -2645,6 +2720,12 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer)
>>   			err = parse_audio_unit(&state, desc->bCSourceID);
>>   			if (err < 0 && err != -EINVAL)
>>   				return err;
>> +
>> +			if (uac_v2v3_control_is_readable(desc->bmControls,
> 
> Missing le32_to_cpu()?

Agreed.


Thanks,

Jorge

>  >
> thanks,
> 
> Takashi
> 

Re: [alsa-devel] [PATCH 2/4] ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.

[Ruslan Bilovol]

On Fri, Apr 20, 2018 at 8:03 PM, Jorge Sanjuan
<jorge.sanjuan@codethink.co.uk> wrote:
> From: Michael Drake <michael.drake@codethink.co.uk>
>
> The channel mapping is defined by bChRelationship, not bChPurpose.
>
> Signed-off-by: Michael Drake <michael.drake@codethink.co.uk>
> ---
>  sound/usb/stream.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/usb/stream.c b/sound/usb/stream.c
> index 6a8f5843334e..956be9f7c72a 100644
> --- a/sound/usb/stream.c
> +++ b/sound/usb/stream.c
> @@ -349,7 +349,7 @@ snd_pcm_chmap_elem *convert_chmap_v3(struct uac3_cluster_header_descriptor
>                          * TODO: this conversion is not complete, update it
>                          * after adding UAC3 values to asound.h
>                          */
> -                       switch (is->bChPurpose) {
> +                       switch (is->bChRelationship) {

Good catch!

Somehow I overlooked this, so in my case of Generic Audio it is always
mono.

Reviewed-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>

>                         case UAC3_CH_MONO:
>                                 map = SNDRV_CHMAP_MONO;
>                                 break;
> --
> 2.11.0
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH v2 0/4] ALSA: usb: UAC3 new features.

[Jorge Sanjuan]

v2 fixes:
 - If/else statements braces style fixes.
 - Add wrapping function to mixer unit code.
 - Make connectors control kctl struct const.
 - Little endian to cpu conversion in several places.
 - Sing off and add Fixes tag to fixup commit.
 - Remove flex-array for a struct that is used statically.

Now that the UAC3 patch [1] has made it to linux-next I have some extra
features to make a UAC3 device fully work in Linux. Including Jack 
insertion control that I have put on top of this other patch [2] for 
UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
to appear in most headset type devices.

UAC3 devices also require to have a Basic Audio Device (BADD) in a separate 
config for which both Ruslan Bilovol and myself have submited different 
approaches[3][4] but I don't know what the final merge will be. Once there 
is official support for BADD, we'll need to test it with an actual UAC3 
device to confirm it all wokrs.

All this features are tested with an actual UAC3 device that is still in
development. For this patch series, only the legacy config (#1. UAC1/UAC2)
and the UAC3 config have been tested. The BADD config is only tested using
and updated verison of [4].

[1]: https://patchwork.kernel.org/patch/10298179/
[2]: https://patchwork.kernel.org/patch/10305847/
[3]: https://patchwork.kernel.org/patch/10340851/
[4]: https://www.spinics.net/lists/alsa-devel/msg71617.html

Based on linux-next tag: next-20180420

Jorge Sanjuan (3):
  ALSA: usb-audio: UAC3. Add support for mixer unit.
  ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
  ALSA: usb-audio: UAC3 Add support for connector insertion.

Michael Drake (1):
  ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.

 include/linux/usb/audio-v2.h   |   7 ++
 include/linux/usb/audio-v3.h   |  14 +++
 include/uapi/linux/usb/audio.h |  13 ++-
 sound/usb/mixer.c              | 195 +++++++++++++++++++++++++++++++++++++----
 sound/usb/stream.c             |  11 ++-
 5 files changed, 217 insertions(+), 23 deletions(-)

-- 
2.11.0

[PATCH v2 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.

[Jorge Sanjuan]

This adds support for the MIXER UNIT in UAC3. All the information
is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
read the rest of the logical cluster to obtain the channel config
as that wont make any difference in the current mixer behaviour.

The name of the mixer unit is not yet requested as there is not
support for the UAC3 Class Specific String requests.

Tested in an UAC3 device working as a HEADSET with a basic mixer
unit (same as the one in the BADD spec) with no controls.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 include/uapi/linux/usb/audio.h | 13 +++++--
 sound/usb/mixer.c              | 87 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 93 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
index 3a78e7145689..f9be472cd025 100644
--- a/include/uapi/linux/usb/audio.h
+++ b/include/uapi/linux/usb/audio.h
@@ -285,9 +285,16 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
 static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
 					      int protocol)
 {
-	return (protocol == UAC_VERSION_1) ?
-		&desc->baSourceID[desc->bNrInPins + 4] :
-		&desc->baSourceID[desc->bNrInPins + 6];
+	switch (protocol) {
+	case UAC_VERSION_1:
+		return &desc->baSourceID[desc->bNrInPins + 4];
+	case UAC_VERSION_2:
+		return &desc->baSourceID[desc->bNrInPins + 6];
+	case UAC_VERSION_3:
+		return &desc->baSourceID[desc->bNrInPins + 2];
+	default:
+		return NULL;
+	}
 }
 
 static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 301ad61ed426..bf701b466a4e 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
 }
 
 /*
+ * Get logical cluster information for UAC3 devices.
+ */
+static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
+{
+	struct uac3_cluster_header_descriptor c_header;
+	int err;
+
+	err = snd_usb_ctl_msg(state->chip->dev,
+			usb_rcvctrlpipe(state->chip->dev, 0),
+			UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
+			USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+			cluster_id,
+			snd_usb_ctrl_intf(state->chip),
+			&c_header, sizeof(c_header));
+	if (err < 0)
+		goto error;
+	if (err != sizeof(c_header)) {
+		err = -EIO;
+		goto error;
+	}
+
+	return c_header.bNrChannels;
+
+error:
+	usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
+	return err;
+}
+
+/*
+ * Get number of channels for a Mixer Unit.
+ */
+static int uac_mixer_unit_get_channels(struct mixer_build *state,
+				       struct uac_mixer_unit_descriptor *desc)
+{
+	int mu_channels;
+
+	if (desc->bLength < 11)
+		return -EINVAL;
+	if (!desc->bNrInPins)
+		return -EINVAL;
+
+	switch (state->mixer->protocol) {
+	case UAC_VERSION_1:
+	case UAC_VERSION_2:
+	default:
+		mu_channels = uac_mixer_unit_bNrChannels(desc);
+		break;
+	case UAC_VERSION_3:
+		mu_channels = get_cluster_channels_v3(state,
+				le16_to_cpu(desc->baSourceID[desc->bNrInPins]));
+		break;
+	}
+
+	if (!mu_channels)
+		return -EINVAL;
+
+	return mu_channels;
+}
+
+/*
  * parse the source unit recursively until it reaches to a terminal
  * or a branched unit.
  */
@@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
 				term->name = le16_to_cpu(d->wClockSourceStr);
 				return 0;
 			}
+			case UAC3_MIXER_UNIT: {
+				struct uac_mixer_unit_descriptor *d = p1;
+
+				err = uac_mixer_unit_get_channels(state, d);
+				if (err < 0)
+					return err;
+
+				term->channels = err;
+				term->type = d->bDescriptorSubtype << 16; /* virtual type */
+
+				return 0;
+			}
 			default:
 				return -ENODEV;
 			}
@@ -1801,7 +1873,7 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
 				 struct usb_audio_term *iterm)
 {
 	struct usb_mixer_elem_info *cval;
-	unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
+	unsigned int num_outs;
 	unsigned int i, len;
 	struct snd_kcontrol *kctl;
 	const struct usbmix_name_map *map;
@@ -1814,6 +1886,10 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
 	if (!cval)
 		return;
 
+	num_outs = uac_mixer_unit_get_channels(state, desc);
+	if (num_outs < 0)
+		return;
+
 	snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid);
 	cval->control = in_ch + 1; /* based on 1 */
 	cval->val_type = USB_MIXER_S16;
@@ -1878,14 +1954,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
 	int input_pins, num_ins, num_outs;
 	int pin, ich, err;
 
-	if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
-	    !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
+	err = uac_mixer_unit_get_channels(state, desc);
+	if (err < 0) {
 		usb_audio_err(state->chip,
 			      "invalid MIXER UNIT descriptor %d\n",
 			      unitid);
-		return -EINVAL;
+		return err;
 	}
 
+	num_outs = err;
+	input_pins = desc->bNrInPins;
+
 	num_ins = 0;
 	ich = 0;
 	for (pin = 0; pin < input_pins; pin++) {
-- 
2.11.0

[PATCH v2 2/4] ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.

[Jorge Sanjuan]

From: Michael Drake <michael.drake@codethink.co.uk>

The channel mapping is defined by bChRelationship, not bChPurpose.

Fixes: 9a2fe9b801f5 ("ALSA: usb: initial USB Audio Device Class 3.0
support")
Reviewed-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Signed-off-by: Michael Drake <michael.drake@codethink.co.uk>
Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 sound/usb/stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 6a8f5843334e..956be9f7c72a 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -349,7 +349,7 @@ snd_pcm_chmap_elem *convert_chmap_v3(struct uac3_cluster_header_descriptor
 			 * TODO: this conversion is not complete, update it
 			 * after adding UAC3 values to asound.h
 			 */
-			switch (is->bChPurpose) {
+			switch (is->bChRelationship) {
 			case UAC3_CH_MONO:
 				map = SNDRV_CHMAP_MONO;
 				break;
-- 
2.11.0

[PATCH v2 3/4] ALSA: usb-audio: Use Class Specific EP for UAC3 devices.

[Jorge Sanjuan]

bmAtributes offset doesn't exist in the UAC3 CS_EP descriptor.
Hence, checking for pitch control as if it was UAC2 doesn't make
any sense. Use the defined UAC3 offsets instead.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 sound/usb/stream.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 956be9f7c72a..5ed334575fc7 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -576,7 +576,7 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
 
 	if (protocol == UAC_VERSION_1) {
 		attributes = csep->bmAttributes;
-	} else {
+	} else if (protocol == UAC_VERSION_2) {
 		struct uac2_iso_endpoint_descriptor *csep2 =
 			(struct uac2_iso_endpoint_descriptor *) csep;
 
@@ -585,6 +585,13 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
 		/* emulate the endpoint attributes of a v1 device */
 		if (csep2->bmControls & UAC2_CONTROL_PITCH)
 			attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
+	} else { /* UAC_VERSION_3 */
+		struct uac3_iso_endpoint_descriptor *csep3 =
+			(struct uac3_iso_endpoint_descriptor *) csep;
+
+		/* emulate the endpoint attributes of a v1 device */
+		if (le32_to_cpu(csep3->bmControls) & UAC2_CONTROL_PITCH)
+			attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
 	}
 
 	return attributes;
-- 
2.11.0

[PATCH v2 4/4] ALSA: usb-audio: UAC3 Add support for connector insertion.

[Jorge Sanjuan]

This adds support for the UAC3 insertion controls. The status
is reported as a boolean value in the same way it used to do
for UAC2. Hence, the presence of any connector in the response
will make the control saying the jack is connected.

The UAC2 support for this control has been moved to a dedicated
control for connectors as both UAC2 and UAC3 follow a specific
Control Request Parameter Block for this control. This parameter
block for UAC3 could not be read in the same simplistic
manner as in UAC2.

This implementation is not requesting additional information
from the HIGH CAPABILITY Connectors descriptor.

Tested with an UAC3 device with UAC2 as legacy configuration.
The connector status can be read with `amixer` and the interrupt
is also caught with `alsactl monitor`.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 include/linux/usb/audio-v2.h |   7 +++
 include/linux/usb/audio-v3.h |  14 ++++++
 sound/usb/mixer.c            | 108 +++++++++++++++++++++++++++++++++++++------
 3 files changed, 115 insertions(+), 14 deletions(-)

diff --git a/include/linux/usb/audio-v2.h b/include/linux/usb/audio-v2.h
index aaafecf073ff..a96ed2ce3254 100644
--- a/include/linux/usb/audio-v2.h
+++ b/include/linux/usb/audio-v2.h
@@ -189,6 +189,13 @@ struct uac2_iso_endpoint_descriptor {
 #define UAC2_CONTROL_DATA_OVERRUN	(3 << 2)
 #define UAC2_CONTROL_DATA_UNDERRUN	(3 << 4)
 
+/* 5.2.5.4.2 Connector Control Parameter Block */
+struct uac2_connectors_ctl_blk {
+	__u8 bNrChannels;
+	__le32 bmChannelConfig;
+	__u8 iChannelNames;
+} __attribute__((packed));
+
 /* 6.1 Interrupt Data Message */
 
 #define UAC2_INTERRUPT_DATA_MSG_VENDOR	(1 << 0)
diff --git a/include/linux/usb/audio-v3.h b/include/linux/usb/audio-v3.h
index a8959aaba0ae..eb732f6569cb 100644
--- a/include/linux/usb/audio-v3.h
+++ b/include/linux/usb/audio-v3.h
@@ -221,6 +221,12 @@ struct uac3_iso_endpoint_descriptor {
 	__le16 wLockDelay;
 } __attribute__((packed));
 
+/* 5.2.1.6.1 INSERTION CONTROL PARAMETER BLOCK */
+struct uac3_insertion_ctl_blk {
+	__u8 bSize;
+	__u8 bmConInserted;
+} __attribute__ ((packed));
+
 /* 6.1 INTERRUPT DATA MESSAGE */
 struct uac3_interrupt_data_msg {
 	__u8 bInfo;
@@ -392,4 +398,12 @@ struct uac3_interrupt_data_msg {
 #define UAC3_AC_ACTIVE_INTERFACE_CONTROL	0x01
 #define UAC3_AC_POWER_DOMAIN_CONTROL		0x02
 
+/* A.23.5 TERMINAL CONTROL SELECTORS */
+#define UAC3_TE_UNDEFINED			0x00
+#define UAC3_TE_INSERTION			0x01
+#define UAC3_TE_OVERLOAD			0x02
+#define UAC3_TE_UNDERFLOW			0x03
+#define UAC3_TE_OVERFLOW			0x04
+#define UAC3_TE_LATENCY 			0x05
+
 #endif /* __LINUX_USB_AUDIO_V3_H */
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index bf701b466a4e..9809d0a85894 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1322,6 +1322,51 @@ static int mixer_ctl_master_bool_get(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
+/* get the connectors status and report it as boolean type */
+static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct usb_mixer_elem_info *cval = kcontrol->private_data;
+	struct snd_usb_audio *chip = cval->head.mixer->chip;
+	int idx = 0, validx, ret, val;
+
+	validx = cval->control << 8 | 0;
+
+	ret = snd_usb_lock_shutdown(chip) ? -EIO : 0;
+	if (ret)
+		goto error;
+
+	idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8);
+	if (cval->head.mixer->protocol == UAC_VERSION_2) {
+		struct uac2_connectors_ctl_blk uac2_conn;
+
+		ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), UAC2_CS_CUR,
+				      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+				      validx, idx, &uac2_conn, sizeof(uac2_conn));
+		val = !!uac2_conn.bNrChannels;
+	} else { /* UAC_VERSION_3 */
+		struct uac3_insertion_ctl_blk uac3_conn;
+
+		ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), UAC2_CS_CUR,
+				      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+				      validx, idx, &uac3_conn, sizeof(uac3_conn));
+		val = !!uac3_conn.bmConInserted;
+	}
+
+	snd_usb_unlock_shutdown(chip);
+
+	if (ret < 0) {
+error:
+		usb_audio_err(chip,
+			"cannot get connectors status: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n",
+			UAC_GET_CUR, validx, idx, cval->val_type);
+		return ret;
+	}
+
+	ucontrol->value.integer.value[0] = val;
+	return 0;
+}
+
 static struct snd_kcontrol_new usb_feature_unit_ctl = {
 	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
 	.name = "", /* will be filled later manually */
@@ -1352,6 +1397,15 @@ static struct snd_kcontrol_new usb_bool_master_control_ctl_ro = {
 	.put = NULL,
 };
 
+static const struct snd_kcontrol_new usb_connector_ctl_ro = {
+	.iface = SNDRV_CTL_ELEM_IFACE_CARD,
+	.name = "", /* will be filled later manually */
+	.access = SNDRV_CTL_ELEM_ACCESS_READ,
+	.info = snd_ctl_boolean_mono_info,
+	.get = mixer_ctl_connector_get,
+	.put = NULL,
+};
+
 /*
  * This symbol is exported in order to allow the mixer quirks to
  * hook up to the standard feature unit control mechanism
@@ -1598,17 +1652,25 @@ static void build_connector_control(struct mixer_build *state,
 		return;
 	snd_usb_mixer_elem_init_std(&cval->head, state->mixer, term->id);
 	/*
-	 * The first byte from reading the UAC2_TE_CONNECTOR control returns the
-	 * number of channels connected.  This boolean ctl will simply report
-	 * if any channels are connected or not.
-	 * (Audio20_final.pdf Table 5-10: Connector Control CUR Parameter Block)
+	 * UAC2: The first byte from reading the UAC2_TE_CONNECTOR control returns the
+	 * number of channels connected.
+	 *
+	 * UAC3: The first byte specifies size of bitmap for the inserted controls. The
+	 * following byte(s) specifies which connectors are inserted.
+	 *
+	 * This boolean ctl will simply report if any channels are connected
+	 * or not.
 	 */
-	cval->control = UAC2_TE_CONNECTOR;
+	if (state->mixer->protocol == UAC_VERSION_2)
+		cval->control = UAC2_TE_CONNECTOR;
+	else /* UAC_VERSION_3 */
+		cval->control = UAC3_TE_INSERTION;
+
 	cval->val_type = USB_MIXER_BOOLEAN;
 	cval->channels = 1; /* report true if any channel is connected */
 	cval->min = 0;
 	cval->max = 1;
-	kctl = snd_ctl_new1(&usb_bool_master_control_ctl_ro, cval);
+	kctl = snd_ctl_new1(&usb_connector_ctl_ro, cval);
 	if (!kctl) {
 		usb_audio_err(state->chip, "cannot malloc kcontrol\n");
 		kfree(cval);
@@ -1930,16 +1992,28 @@ static int parse_audio_input_terminal(struct mixer_build *state, int unitid,
 				      void *raw_desc)
 {
 	struct usb_audio_term iterm;
-	struct uac2_input_terminal_descriptor *d = raw_desc;
+	unsigned int control, bmctls, term_id;
 
-	check_input_term(state, d->bTerminalID, &iterm);
 	if (state->mixer->protocol == UAC_VERSION_2) {
-		/* Check for jack detection. */
-		if (uac_v2v3_control_is_readable(d->bmControls,
-						 UAC2_TE_CONNECTOR)) {
-			build_connector_control(state, &iterm, true);
-		}
+		struct uac2_input_terminal_descriptor *d_v2 = raw_desc;
+		control = UAC2_TE_CONNECTOR;
+		term_id = d_v2->bTerminalID;
+		bmctls = le16_to_cpu(d_v2->bmControls);
+	} else if (state->mixer->protocol == UAC_VERSION_3) {
+		struct uac3_input_terminal_descriptor *d_v3 = raw_desc;
+		control = UAC3_TE_INSERTION;
+		term_id = d_v3->bTerminalID;
+		bmctls = le32_to_cpu(d_v3->bmControls);
+	} else {
+		return 0; /* UAC1. No Insertion control */
 	}
+
+	check_input_term(state, term_id, &iterm);
+
+	/* Check for jack detection. */
+	if (uac_v2v3_control_is_readable(bmctls, control))
+		build_connector_control(state, &iterm, true);
+
 	return 0;
 }
 
@@ -2528,7 +2602,7 @@ static int parse_audio_unit(struct mixer_build *state, int unitid)
 	} else { /* UAC_VERSION_3 */
 		switch (p1[2]) {
 		case UAC_INPUT_TERMINAL:
-			return 0; /* NOP */
+			return parse_audio_input_terminal(state, unitid, p1);
 		case UAC3_MIXER_UNIT:
 			return parse_audio_mixer_unit(state, unitid, p1);
 		case UAC3_CLOCK_SOURCE:
@@ -2666,6 +2740,12 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer)
 			err = parse_audio_unit(&state, desc->bCSourceID);
 			if (err < 0 && err != -EINVAL)
 				return err;
+
+			if (uac_v2v3_control_is_readable(le32_to_cpu(desc->bmControls),
+							 UAC3_TE_INSERTION)) {
+				build_connector_control(&state, &state.oterm,
+							false);
+			}
 		}
 	}
 
-- 
2.11.0

Re: [PATCH v2 2/4] ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.

[Takashi Iwai]

On Tue, 24 Apr 2018 19:24:43 +0200,
Jorge Sanjuan wrote:
> 
> From: Michael Drake <michael.drake@codethink.co.uk>
> 
> The channel mapping is defined by bChRelationship, not bChPurpose.
> 
> Fixes: 9a2fe9b801f5 ("ALSA: usb: initial USB Audio Device Class 3.0
> support")
> Reviewed-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> Signed-off-by: Michael Drake <michael.drake@codethink.co.uk>
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>

Applied this to for-linus branch now, as it's a clear fix.


thanks,

Takashi

Re: [PATCH v2 0/4] ALSA: usb: UAC3 new features.

[Takashi Iwai]

On Tue, 24 Apr 2018 19:24:41 +0200,
Jorge Sanjuan wrote:
> 
> v2 fixes:
>  - If/else statements braces style fixes.
>  - Add wrapping function to mixer unit code.
>  - Make connectors control kctl struct const.
>  - Little endian to cpu conversion in several places.
>  - Sing off and add Fixes tag to fixup commit.
>  - Remove flex-array for a struct that is used statically.
> 
> Now that the UAC3 patch [1] has made it to linux-next I have some extra
> features to make a UAC3 device fully work in Linux. Including Jack 
> insertion control that I have put on top of this other patch [2] for 
> UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
> to appear in most headset type devices.

These patches look reasonable, I'm OK to merge.  But I'll wait for
Ruslan's comments (or at best with test results).

> UAC3 devices also require to have a Basic Audio Device (BADD) in a separate 
> config for which both Ruslan Bilovol and myself have submited different 
> approaches[3][4] but I don't know what the final merge will be. Once there 
> is official support for BADD, we'll need to test it with an actual UAC3 
> device to confirm it all wokrs.

Could you guys try to get agreement which approach should we take?

I have no big preference.  Currently Ruslan's patch series look
easier, just because its addition is a bit smaller, though.


Thanks!

Takashi

> All this features are tested with an actual UAC3 device that is still in
> development. For this patch series, only the legacy config (#1. UAC1/UAC2)
> and the UAC3 config have been tested. The BADD config is only tested using
> and updated verison of [4].
> 
> [1]: https://patchwork.kernel.org/patch/10298179/
> [2]: https://patchwork.kernel.org/patch/10305847/
> [3]: https://patchwork.kernel.org/patch/10340851/
> [4]: https://www.spinics.net/lists/alsa-devel/msg71617.html
> 
> Based on linux-next tag: next-20180420
> 
> Jorge Sanjuan (3):
>   ALSA: usb-audio: UAC3. Add support for mixer unit.
>   ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
>   ALSA: usb-audio: UAC3 Add support for connector insertion.
> 
> Michael Drake (1):
>   ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.
> 
>  include/linux/usb/audio-v2.h   |   7 ++
>  include/linux/usb/audio-v3.h   |  14 +++
>  include/uapi/linux/usb/audio.h |  13 ++-
>  sound/usb/mixer.c              | 195 +++++++++++++++++++++++++++++++++++++----
>  sound/usb/stream.c             |  11 ++-
>  5 files changed, 217 insertions(+), 23 deletions(-)
> 
> -- 
> 2.11.0
> 
> 

Re: [alsa-devel] [PATCH v2 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.

[Ruslan Bilovol]

 On Tue, Apr 24, 2018 at 8:24 PM, Jorge Sanjuan
<jorge.sanjuan@codethink.co.uk> wrote:
> This adds support for the MIXER UNIT in UAC3. All the information
> is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
> read the rest of the logical cluster to obtain the channel config
> as that wont make any difference in the current mixer behaviour.
>
> The name of the mixer unit is not yet requested as there is not
> support for the UAC3 Class Specific String requests.
>
> Tested in an UAC3 device working as a HEADSET with a basic mixer
> unit (same as the one in the BADD spec) with no controls.

The patch looks OK in general, but I have few comments

>
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
> ---
>  include/uapi/linux/usb/audio.h | 13 +++++--
>  sound/usb/mixer.c              | 87 ++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 93 insertions(+), 7 deletions(-)
>
> diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
> index 3a78e7145689..f9be472cd025 100644
> --- a/include/uapi/linux/usb/audio.h
> +++ b/include/uapi/linux/usb/audio.h
> @@ -285,9 +285,16 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
>  static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
>                                               int protocol)

Name of this function is ambiguous, that's because UAC1 mixer unit
has only bmControls bitmap, whereas UAC2/3 mixer unit has two
bitmaps (bmMixerControls and bmControls), in latter case this func
returns pointer to bmMixerControls.

Maybe in the future we will need to rename it, but at least having
comment which clarifies that in case of UAC2/3 this function actually
returns pointer to bmMixerControls here will be helpful for code readers.

>  {
> -       return (protocol == UAC_VERSION_1) ?
> -               &desc->baSourceID[desc->bNrInPins + 4] :
> -               &desc->baSourceID[desc->bNrInPins + 6];
> +       switch (protocol) {
> +       case UAC_VERSION_1:
> +               return &desc->baSourceID[desc->bNrInPins + 4];
> +       case UAC_VERSION_2:
> +               return &desc->baSourceID[desc->bNrInPins + 6];
> +       case UAC_VERSION_3:
> +               return &desc->baSourceID[desc->bNrInPins + 2];
> +       default:
> +               return NULL;
> +       }
>  }
>
>  static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 301ad61ed426..bf701b466a4e 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
>  }
>
>  /*
> + * Get logical cluster information for UAC3 devices.
> + */
> +static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
> +{
> +       struct uac3_cluster_header_descriptor c_header;
> +       int err;
> +
> +       err = snd_usb_ctl_msg(state->chip->dev,
> +                       usb_rcvctrlpipe(state->chip->dev, 0),
> +                       UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
> +                       USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
> +                       cluster_id,
> +                       snd_usb_ctrl_intf(state->chip),
> +                       &c_header, sizeof(c_header));
> +       if (err < 0)
> +               goto error;
> +       if (err != sizeof(c_header)) {
> +               err = -EIO;
> +               goto error;
> +       }
> +
> +       return c_header.bNrChannels;
> +
> +error:
> +       usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
> +       return err;
> +}
> +
> +/*
> + * Get number of channels for a Mixer Unit.
> + */
> +static int uac_mixer_unit_get_channels(struct mixer_build *state,
> +                                      struct uac_mixer_unit_descriptor *desc)
> +{
> +       int mu_channels;
> +
> +       if (desc->bLength < 11)
> +               return -EINVAL;
> +       if (!desc->bNrInPins)
> +               return -EINVAL;
> +
> +       switch (state->mixer->protocol) {
> +       case UAC_VERSION_1:
> +       case UAC_VERSION_2:
> +       default:
> +               mu_channels = uac_mixer_unit_bNrChannels(desc);
> +               break;
> +       case UAC_VERSION_3:
> +               mu_channels = get_cluster_channels_v3(state,
> +                               le16_to_cpu(desc->baSourceID[desc->bNrInPins]));

It would be good to create and use some helper here, for example implement
uac3_mixer_unit_wClusterDescrID() similar to uac_mixer_unit_bNrChannels().
It will put all this conversation to a single place and will improve
code readability.

> +               break;
> +       }
> +
> +       if (!mu_channels)
> +               return -EINVAL;
> +
> +       return mu_channels;
> +}
> +
> +/*
>   * parse the source unit recursively until it reaches to a terminal
>   * or a branched unit.
>   */
> @@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
>                                 term->name = le16_to_cpu(d->wClockSourceStr);
>                                 return 0;
>                         }
> +                       case UAC3_MIXER_UNIT: {
> +                               struct uac_mixer_unit_descriptor *d = p1;
> +
> +                               err = uac_mixer_unit_get_channels(state, d);
> +                               if (err < 0)
> +                                       return err;
> +
> +                               term->channels = err;
> +                               term->type = d->bDescriptorSubtype << 16; /* virtual type */
> +
> +                               return 0;
> +                       }
>                         default:
>                                 return -ENODEV;
>                         }
> @@ -1801,7 +1873,7 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
>                                  struct usb_audio_term *iterm)
>  {
>         struct usb_mixer_elem_info *cval;
> -       unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
> +       unsigned int num_outs;
>         unsigned int i, len;
>         struct snd_kcontrol *kctl;
>         const struct usbmix_name_map *map;
> @@ -1814,6 +1886,10 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
>         if (!cval)
>                 return;
>
> +       num_outs = uac_mixer_unit_get_channels(state, desc);
> +       if (num_outs < 0)
> +               return;

This will produce an extra USB control request in UAC3 case, which we can
avoid if add num_outs to input parameters of build_mixer_unit_ctl() function.
This function is used only once inside of parse_audio_mixer_unit() which already
has channels number request and all needed checks.

Thanks,
Ruslan

> +
>         snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid);
>         cval->control = in_ch + 1; /* based on 1 */
>         cval->val_type = USB_MIXER_S16;
> @@ -1878,14 +1954,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>         int input_pins, num_ins, num_outs;
>         int pin, ich, err;
>
> -       if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
> -           !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
> +       err = uac_mixer_unit_get_channels(state, desc);
> +       if (err < 0) {
>                 usb_audio_err(state->chip,
>                               "invalid MIXER UNIT descriptor %d\n",
>                               unitid);
> -               return -EINVAL;
> +               return err;
>         }
>
> +       num_outs = err;
> +       input_pins = desc->bNrInPins;
> +
>         num_ins = 0;
>         ich = 0;
>         for (pin = 0; pin < input_pins; pin++) {
> --
> 2.11.0
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v2 3/4] ALSA: usb-audio: Use Class Specific EP for UAC3 devices.

[Ruslan Bilovol]

On Tue, Apr 24, 2018 at 8:24 PM, Jorge Sanjuan
<jorge.sanjuan@codethink.co.uk> wrote:
> bmAtributes offset doesn't exist in the UAC3 CS_EP descriptor.
> Hence, checking for pitch control as if it was UAC2 doesn't make
> any sense. Use the defined UAC3 offsets instead.
>
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>

Reviewed-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>

> ---
>  sound/usb/stream.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/sound/usb/stream.c b/sound/usb/stream.c
> index 956be9f7c72a..5ed334575fc7 100644
> --- a/sound/usb/stream.c
> +++ b/sound/usb/stream.c
> @@ -576,7 +576,7 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
>
>         if (protocol == UAC_VERSION_1) {
>                 attributes = csep->bmAttributes;
> -       } else {
> +       } else if (protocol == UAC_VERSION_2) {
>                 struct uac2_iso_endpoint_descriptor *csep2 =
>                         (struct uac2_iso_endpoint_descriptor *) csep;
>
> @@ -585,6 +585,13 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
>                 /* emulate the endpoint attributes of a v1 device */
>                 if (csep2->bmControls & UAC2_CONTROL_PITCH)
>                         attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
> +       } else { /* UAC_VERSION_3 */
> +               struct uac3_iso_endpoint_descriptor *csep3 =
> +                       (struct uac3_iso_endpoint_descriptor *) csep;
> +
> +               /* emulate the endpoint attributes of a v1 device */
> +               if (le32_to_cpu(csep3->bmControls) & UAC2_CONTROL_PITCH)
> +                       attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
>         }
>
>         return attributes;
> --
> 2.11.0
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v2 0/4] ALSA: usb: UAC3 new features.

[Ruslan Bilovol]

On Tue, Apr 24, 2018 at 9:02 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Tue, 24 Apr 2018 19:24:41 +0200,
> Jorge Sanjuan wrote:
>>
>> v2 fixes:
>>  - If/else statements braces style fixes.
>>  - Add wrapping function to mixer unit code.
>>  - Make connectors control kctl struct const.
>>  - Little endian to cpu conversion in several places.
>>  - Sing off and add Fixes tag to fixup commit.
>>  - Remove flex-array for a struct that is used statically.
>>
>> Now that the UAC3 patch [1] has made it to linux-next I have some extra
>> features to make a UAC3 device fully work in Linux. Including Jack
>> insertion control that I have put on top of this other patch [2] for
>> UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
>> to appear in most headset type devices.

Thanks for adding these improvements!

>
> These patches look reasonable, I'm OK to merge.  But I'll wait for
> Ruslan's comments (or at best with test results).

I reviewed first 3 patches and will review jack detection patch later,
and I'm going to test this patchset in a next few days.

>
>> UAC3 devices also require to have a Basic Audio Device (BADD) in a separate
>> config for which both Ruslan Bilovol and myself have submited different
>> approaches[3][4] but I don't know what the final merge will be. Once there
>> is official support for BADD, we'll need to test it with an actual UAC3
>> device to confirm it all wokrs.
>
> Could you guys try to get agreement which approach should we take?
>
> I have no big preference.  Currently Ruslan's patch series look
> easier, just because its addition is a bit smaller, though.

The BADD devices are quite simple, so direct initialization internal ALSA
structures looks easy and straightforward, comparing to generation of
missing descriptors.
I'm currently improving the patch series so it will look even more
smaller and easier, let's see how it goes

Thanks,
Ruslan

>
>
> Thanks!
>
> Takashi
>
>> All this features are tested with an actual UAC3 device that is still in
>> development. For this patch series, only the legacy config (#1. UAC1/UAC2)
>> and the UAC3 config have been tested. The BADD config is only tested using
>> and updated verison of [4].
>>
>> [1]: https://patchwork.kernel.org/patch/10298179/
>> [2]: https://patchwork.kernel.org/patch/10305847/
>> [3]: https://patchwork.kernel.org/patch/10340851/
>> [4]: https://www.spinics.net/lists/alsa-devel/msg71617.html
>>
>> Based on linux-next tag: next-20180420
>>
>> Jorge Sanjuan (3):
>>   ALSA: usb-audio: UAC3. Add support for mixer unit.
>>   ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
>>   ALSA: usb-audio: UAC3 Add support for connector insertion.
>>
>> Michael Drake (1):
>>   ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.
>>
>>  include/linux/usb/audio-v2.h   |   7 ++
>>  include/linux/usb/audio-v3.h   |  14 +++
>>  include/uapi/linux/usb/audio.h |  13 ++-
>>  sound/usb/mixer.c              | 195 +++++++++++++++++++++++++++++++++++++----
>>  sound/usb/stream.c             |  11 ++-
>>  5 files changed, 217 insertions(+), 23 deletions(-)
>>
>> --
>> 2.11.0
>>
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v2 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.

[Jorge]

On 25/04/18 23:35, Ruslan Bilovol wrote:
>   On Tue, Apr 24, 2018 at 8:24 PM, Jorge Sanjuan
> <jorge.sanjuan@codethink.co.uk> wrote:
>> This adds support for the MIXER UNIT in UAC3. All the information
>> is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
>> read the rest of the logical cluster to obtain the channel config
>> as that wont make any difference in the current mixer behaviour.
>>
>> The name of the mixer unit is not yet requested as there is not
>> support for the UAC3 Class Specific String requests.
>>
>> Tested in an UAC3 device working as a HEADSET with a basic mixer
>> unit (same as the one in the BADD spec) with no controls.
> 
> The patch looks OK in general, but I have few comments

Thanks for the review! I'll submit v3 of this patch with the suggested 
changes.

Regards,

Jorge

> 
>>
>> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
>> ---
>>   include/uapi/linux/usb/audio.h | 13 +++++--
>>   sound/usb/mixer.c              | 87 ++++++++++++++++++++++++++++++++++++++++--
>>   2 files changed, 93 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
>> index 3a78e7145689..f9be472cd025 100644
>> --- a/include/uapi/linux/usb/audio.h
>> +++ b/include/uapi/linux/usb/audio.h
>> @@ -285,9 +285,16 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
>>   static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
>>                                                int protocol)
> 
> Name of this function is ambiguous, that's because UAC1 mixer unit
> has only bmControls bitmap, whereas UAC2/3 mixer unit has two
> bitmaps (bmMixerControls and bmControls), in latter case this func
> returns pointer to bmMixerControls.
> 
> Maybe in the future we will need to rename it, but at least having
> comment which clarifies that in case of UAC2/3 this function actually
> returns pointer to bmMixerControls here will be helpful for code readers.
> 
>>   {
>> -       return (protocol == UAC_VERSION_1) ?
>> -               &desc->baSourceID[desc->bNrInPins + 4] :
>> -               &desc->baSourceID[desc->bNrInPins + 6];
>> +       switch (protocol) {
>> +       case UAC_VERSION_1:
>> +               return &desc->baSourceID[desc->bNrInPins + 4];
>> +       case UAC_VERSION_2:
>> +               return &desc->baSourceID[desc->bNrInPins + 6];
>> +       case UAC_VERSION_3:
>> +               return &desc->baSourceID[desc->bNrInPins + 2];
>> +       default:
>> +               return NULL;
>> +       }
>>   }
>>
>>   static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 301ad61ed426..bf701b466a4e 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
>>   }
>>
>>   /*
>> + * Get logical cluster information for UAC3 devices.
>> + */
>> +static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
>> +{
>> +       struct uac3_cluster_header_descriptor c_header;
>> +       int err;
>> +
>> +       err = snd_usb_ctl_msg(state->chip->dev,
>> +                       usb_rcvctrlpipe(state->chip->dev, 0),
>> +                       UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
>> +                       USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
>> +                       cluster_id,
>> +                       snd_usb_ctrl_intf(state->chip),
>> +                       &c_header, sizeof(c_header));
>> +       if (err < 0)
>> +               goto error;
>> +       if (err != sizeof(c_header)) {
>> +               err = -EIO;
>> +               goto error;
>> +       }
>> +
>> +       return c_header.bNrChannels;
>> +
>> +error:
>> +       usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
>> +       return err;
>> +}
>> +
>> +/*
>> + * Get number of channels for a Mixer Unit.
>> + */
>> +static int uac_mixer_unit_get_channels(struct mixer_build *state,
>> +                                      struct uac_mixer_unit_descriptor *desc)
>> +{
>> +       int mu_channels;
>> +
>> +       if (desc->bLength < 11)
>> +               return -EINVAL;
>> +       if (!desc->bNrInPins)
>> +               return -EINVAL;
>> +
>> +       switch (state->mixer->protocol) {
>> +       case UAC_VERSION_1:
>> +       case UAC_VERSION_2:
>> +       default:
>> +               mu_channels = uac_mixer_unit_bNrChannels(desc);
>> +               break;
>> +       case UAC_VERSION_3:
>> +               mu_channels = get_cluster_channels_v3(state,
>> +                               le16_to_cpu(desc->baSourceID[desc->bNrInPins]));
> 
> It would be good to create and use some helper here, for example implement
> uac3_mixer_unit_wClusterDescrID() similar to uac_mixer_unit_bNrChannels().
> It will put all this conversation to a single place and will improve
> code readability.
> 
>> +               break;
>> +       }
>> +
>> +       if (!mu_channels)
>> +               return -EINVAL;
>> +
>> +       return mu_channels;
>> +}
>> +
>> +/*
>>    * parse the source unit recursively until it reaches to a terminal
>>    * or a branched unit.
>>    */
>> @@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
>>                                  term->name = le16_to_cpu(d->wClockSourceStr);
>>                                  return 0;
>>                          }
>> +                       case UAC3_MIXER_UNIT: {
>> +                               struct uac_mixer_unit_descriptor *d = p1;
>> +
>> +                               err = uac_mixer_unit_get_channels(state, d);
>> +                               if (err < 0)
>> +                                       return err;
>> +
>> +                               term->channels = err;
>> +                               term->type = d->bDescriptorSubtype << 16; /* virtual type */
>> +
>> +                               return 0;
>> +                       }
>>                          default:
>>                                  return -ENODEV;
>>                          }
>> @@ -1801,7 +1873,7 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
>>                                   struct usb_audio_term *iterm)
>>   {
>>          struct usb_mixer_elem_info *cval;
>> -       unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
>> +       unsigned int num_outs;
>>          unsigned int i, len;
>>          struct snd_kcontrol *kctl;
>>          const struct usbmix_name_map *map;
>> @@ -1814,6 +1886,10 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
>>          if (!cval)
>>                  return;
>>
>> +       num_outs = uac_mixer_unit_get_channels(state, desc);
>> +       if (num_outs < 0)
>> +               return;
> 
> This will produce an extra USB control request in UAC3 case, which we can
> avoid if add num_outs to input parameters of build_mixer_unit_ctl() function.
> This function is used only once inside of parse_audio_mixer_unit() which already
> has channels number request and all needed checks.
> 
> Thanks,
> Ruslan
> 
>> +
>>          snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid);
>>          cval->control = in_ch + 1; /* based on 1 */
>>          cval->val_type = USB_MIXER_S16;
>> @@ -1878,14 +1954,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>>          int input_pins, num_ins, num_outs;
>>          int pin, ich, err;
>>
>> -       if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
>> -           !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
>> +       err = uac_mixer_unit_get_channels(state, desc);
>> +       if (err < 0) {
>>                  usb_audio_err(state->chip,
>>                                "invalid MIXER UNIT descriptor %d\n",
>>                                unitid);
>> -               return -EINVAL;
>> +               return err;
>>          }
>>
>> +       num_outs = err;
>> +       input_pins = desc->bNrInPins;
>> +
>>          num_ins = 0;
>>          ich = 0;
>>          for (pin = 0; pin < input_pins; pin++) {
>> --
>> 2.11.0
>>
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [alsa-devel] [PATCH v2 0/4] ALSA: usb: UAC3 new features.

[Jorge]

On 26/04/18 10:26, Ruslan Bilovol wrote:
> On Tue, Apr 24, 2018 at 9:02 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> On Tue, 24 Apr 2018 19:24:41 +0200,
>> Jorge Sanjuan wrote:
>>>
>>> v2 fixes:
>>>   - If/else statements braces style fixes.
>>>   - Add wrapping function to mixer unit code.
>>>   - Make connectors control kctl struct const.
>>>   - Little endian to cpu conversion in several places.
>>>   - Sing off and add Fixes tag to fixup commit.
>>>   - Remove flex-array for a struct that is used statically.
>>>
>>> Now that the UAC3 patch [1] has made it to linux-next I have some extra
>>> features to make a UAC3 device fully work in Linux. Including Jack
>>> insertion control that I have put on top of this other patch [2] for
>>> UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
>>> to appear in most headset type devices.
> 
> Thanks for adding these improvements!
> 
>>
>> These patches look reasonable, I'm OK to merge.  But I'll wait for
>> Ruslan's comments (or at best with test results).
> 
> I reviewed first 3 patches and will review jack detection patch later,
> and I'm going to test this patchset in a next few days.
> 
>>
>>> UAC3 devices also require to have a Basic Audio Device (BADD) in a separate
>>> config for which both Ruslan Bilovol and myself have submited different
>>> approaches[3][4] but I don't know what the final merge will be. Once there
>>> is official support for BADD, we'll need to test it with an actual UAC3
>>> device to confirm it all wokrs.
>>
>> Could you guys try to get agreement which approach should we take?
>>
>> I have no big preference.  Currently Ruslan's patch series look
>> easier, just because its addition is a bit smaller, though.
> 
> The BADD devices are quite simple, so direct initialization internal ALSA
> structures looks easy and straightforward, comparing to generation of
> missing descriptors.
> I'm currently improving the patch series so it will look even more
> smaller and easier, let's see how it goes

Hi Ruslan,

I agree that the BADD devices may not require that much logic using all 
the descriptors. Besides, what makes your approach more interesting to 
me is the fact that there is no need to bypass the cluster descriptor 
every single time if the UAC3 device operates in BADD mode.

I have not yet tested your patch with the UAC3 device that I have. I was 
wondering whether that BADD mixer code will work with and AS interface 
with several alt settings with different endpoints wMaxPacketSize. That 
is something I am working on/testing for this device. I'd have to take a 
closer look to the patch to provide some useful input on that.

Thanks,

Jorge

> 
> Thanks,
> Ruslan
> 
>>
>>
>> Thanks!
>>
>> Takashi
>>
>>> All this features are tested with an actual UAC3 device that is still in
>>> development. For this patch series, only the legacy config (#1. UAC1/UAC2)
>>> and the UAC3 config have been tested. The BADD config is only tested using
>>> and updated verison of [4].
>>>
>>> [1]: https://patchwork.kernel.org/patch/10298179/
>>> [2]: https://patchwork.kernel.org/patch/10305847/
>>> [3]: https://patchwork.kernel.org/patch/10340851/
>>> [4]: https://www.spinics.net/lists/alsa-devel/msg71617.html
>>>
>>> Based on linux-next tag: next-20180420
>>>
>>> Jorge Sanjuan (3):
>>>    ALSA: usb-audio: UAC3. Add support for mixer unit.
>>>    ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
>>>    ALSA: usb-audio: UAC3 Add support for connector insertion.
>>>
>>> Michael Drake (1):
>>>    ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.
>>>
>>>   include/linux/usb/audio-v2.h   |   7 ++
>>>   include/linux/usb/audio-v3.h   |  14 +++
>>>   include/uapi/linux/usb/audio.h |  13 ++-
>>>   sound/usb/mixer.c              | 195 +++++++++++++++++++++++++++++++++++++----
>>>   sound/usb/stream.c             |  11 ++-
>>>   5 files changed, 217 insertions(+), 23 deletions(-)
>>>
>>> --
>>> 2.11.0
>>>
>>>
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

[PATCH v3 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.

[Jorge Sanjuan]

This adds support for the MIXER UNIT in UAC3. All the information
is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
read the rest of the logical cluster to obtain the channel config
as that wont make any difference in the current mixer behaviour.

The name of the mixer unit is not yet requested as there is not
support for the UAC3 Class Specific String requests.

Tested in an UAC3 device working as a HEADSET with a basic mixer
unit (same as the one in the BADD spec) with no controls.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 include/uapi/linux/usb/audio.h | 19 +++++++--
 sound/usb/mixer.c              | 88 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 97 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
index 3a78e7145689..13d98e6e0db1 100644
--- a/include/uapi/linux/usb/audio.h
+++ b/include/uapi/linux/usb/audio.h
@@ -285,9 +285,22 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
 static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
 					      int protocol)
 {
-	return (protocol == UAC_VERSION_1) ?
-		&desc->baSourceID[desc->bNrInPins + 4] :
-		&desc->baSourceID[desc->bNrInPins + 6];
+	switch (protocol) {
+	case UAC_VERSION_1:
+		return &desc->baSourceID[desc->bNrInPins + 4];
+	case UAC_VERSION_2:
+		return &desc->baSourceID[desc->bNrInPins + 6];
+	case UAC_VERSION_3:
+		return &desc->baSourceID[desc->bNrInPins + 2];
+	default:
+		return NULL;
+	}
+}
+
+static inline __u16 uac3_mixer_unit_wClusterDescrID(struct uac_mixer_unit_descriptor *desc)
+{
+	return (desc->baSourceID[desc->bNrInPins + 1] << 8) |
+		desc->baSourceID[desc->bNrInPins];
 }
 
 static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 301ad61ed426..3503f4840ec3 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
 }
 
 /*
+ * Get logical cluster information for UAC3 devices.
+ */
+static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
+{
+	struct uac3_cluster_header_descriptor c_header;
+	int err;
+
+	err = snd_usb_ctl_msg(state->chip->dev,
+			usb_rcvctrlpipe(state->chip->dev, 0),
+			UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
+			USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+			cluster_id,
+			snd_usb_ctrl_intf(state->chip),
+			&c_header, sizeof(c_header));
+	if (err < 0)
+		goto error;
+	if (err != sizeof(c_header)) {
+		err = -EIO;
+		goto error;
+	}
+
+	return c_header.bNrChannels;
+
+error:
+	usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
+	return err;
+}
+
+/*
+ * Get number of channels for a Mixer Unit.
+ */
+static int uac_mixer_unit_get_channels(struct mixer_build *state,
+				       struct uac_mixer_unit_descriptor *desc)
+{
+	int mu_channels;
+
+	if (desc->bLength < 11)
+		return -EINVAL;
+	if (!desc->bNrInPins)
+		return -EINVAL;
+
+	switch (state->mixer->protocol) {
+	case UAC_VERSION_1:
+	case UAC_VERSION_2:
+	default:
+		mu_channels = uac_mixer_unit_bNrChannels(desc);
+		break;
+	case UAC_VERSION_3:
+		mu_channels = get_cluster_channels_v3(state,
+				uac3_mixer_unit_wClusterDescrID(desc));
+		break;
+	}
+
+	if (!mu_channels)
+		return -EINVAL;
+
+	return mu_channels;
+}
+
+/*
  * parse the source unit recursively until it reaches to a terminal
  * or a branched unit.
  */
@@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
 				term->name = le16_to_cpu(d->wClockSourceStr);
 				return 0;
 			}
+			case UAC3_MIXER_UNIT: {
+				struct uac_mixer_unit_descriptor *d = p1;
+
+				err = uac_mixer_unit_get_channels(state, d);
+				if (err < 0)
+					return err;
+
+				term->channels = err;
+				term->type = d->bDescriptorSubtype << 16; /* virtual type */
+
+				return 0;
+			}
 			default:
 				return -ENODEV;
 			}
@@ -1797,11 +1869,10 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid,
  */
 static void build_mixer_unit_ctl(struct mixer_build *state,
 				 struct uac_mixer_unit_descriptor *desc,
-				 int in_pin, int in_ch, int unitid,
-				 struct usb_audio_term *iterm)
+				 int in_pin, int in_ch, int num_outs,
+				 int unitid, struct usb_audio_term *iterm)
 {
 	struct usb_mixer_elem_info *cval;
-	unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
 	unsigned int i, len;
 	struct snd_kcontrol *kctl;
 	const struct usbmix_name_map *map;
@@ -1878,14 +1949,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
 	int input_pins, num_ins, num_outs;
 	int pin, ich, err;
 
-	if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
-	    !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
+	err = uac_mixer_unit_get_channels(state, desc);
+	if (err < 0) {
 		usb_audio_err(state->chip,
 			      "invalid MIXER UNIT descriptor %d\n",
 			      unitid);
-		return -EINVAL;
+		return err;
 	}
 
+	num_outs = err;
+	input_pins = desc->bNrInPins;
+
 	num_ins = 0;
 	ich = 0;
 	for (pin = 0; pin < input_pins; pin++) {
@@ -1912,7 +1986,7 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
 				}
 			}
 			if (ich_has_controls)
-				build_mixer_unit_ctl(state, desc, pin, ich,
+				build_mixer_unit_ctl(state, desc, pin, ich, num_outs,
 						     unitid, &iterm);
 		}
 	}
-- 
2.11.0

Re: [PATCH v3 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.

[Ruslan Bilovol]

On Fri, Apr 27, 2018 at 8:06 PM, Jorge Sanjuan
<jorge.sanjuan@codethink.co.uk> wrote:
> This adds support for the MIXER UNIT in UAC3. All the information
> is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
> read the rest of the logical cluster to obtain the channel config
> as that wont make any difference in the current mixer behaviour.
>
> The name of the mixer unit is not yet requested as there is not
> support for the UAC3 Class Specific String requests.
>
> Tested in an UAC3 device working as a HEADSET with a basic mixer
> unit (same as the one in the BADD spec) with no controls.

So, after deeper looking into the code and after testing this patch,
in your usecase (mixer with no controls) you'll never execute
build_mixer_unit_ctl(), correct? So did you try to just fix issues with
incorrect parsing of mixer unit descriptor?

>
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
> ---
>  include/uapi/linux/usb/audio.h | 19 +++++++--
>  sound/usb/mixer.c              | 88 ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 97 insertions(+), 10 deletions(-)
>
> diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
> index 3a78e7145689..13d98e6e0db1 100644
> --- a/include/uapi/linux/usb/audio.h
> +++ b/include/uapi/linux/usb/audio.h
> @@ -285,9 +285,22 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
>  static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
>                                               int protocol)
>  {
> -       return (protocol == UAC_VERSION_1) ?
> -               &desc->baSourceID[desc->bNrInPins + 4] :
> -               &desc->baSourceID[desc->bNrInPins + 6];
> +       switch (protocol) {
> +       case UAC_VERSION_1:
> +               return &desc->baSourceID[desc->bNrInPins + 4];
> +       case UAC_VERSION_2:
> +               return &desc->baSourceID[desc->bNrInPins + 6];
> +       case UAC_VERSION_3:
> +               return &desc->baSourceID[desc->bNrInPins + 2];
> +       default:
> +               return NULL;
> +       }
> +}
> +
> +static inline __u16 uac3_mixer_unit_wClusterDescrID(struct uac_mixer_unit_descriptor *desc)
> +{
> +       return (desc->baSourceID[desc->bNrInPins + 1] << 8) |
> +               desc->baSourceID[desc->bNrInPins];
>  }
>
>  static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 301ad61ed426..3503f4840ec3 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
>  }
>
>  /*
> + * Get logical cluster information for UAC3 devices.
> + */
> +static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
> +{
> +       struct uac3_cluster_header_descriptor c_header;
> +       int err;
> +
> +       err = snd_usb_ctl_msg(state->chip->dev,
> +                       usb_rcvctrlpipe(state->chip->dev, 0),
> +                       UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
> +                       USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
> +                       cluster_id,
> +                       snd_usb_ctrl_intf(state->chip),
> +                       &c_header, sizeof(c_header));
> +       if (err < 0)
> +               goto error;
> +       if (err != sizeof(c_header)) {
> +               err = -EIO;
> +               goto error;
> +       }
> +
> +       return c_header.bNrChannels;
> +
> +error:
> +       usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
> +       return err;
> +}
> +
> +/*
> + * Get number of channels for a Mixer Unit.
> + */
> +static int uac_mixer_unit_get_channels(struct mixer_build *state,
> +                                      struct uac_mixer_unit_descriptor *desc)
> +{
> +       int mu_channels;
> +
> +       if (desc->bLength < 11)
> +               return -EINVAL;
> +       if (!desc->bNrInPins)
> +               return -EINVAL;
> +
> +       switch (state->mixer->protocol) {
> +       case UAC_VERSION_1:
> +       case UAC_VERSION_2:
> +       default:
> +               mu_channels = uac_mixer_unit_bNrChannels(desc);
> +               break;
> +       case UAC_VERSION_3:
> +               mu_channels = get_cluster_channels_v3(state,
> +                               uac3_mixer_unit_wClusterDescrID(desc));
> +               break;
> +       }
> +
> +       if (!mu_channels)
> +               return -EINVAL;
> +
> +       return mu_channels;
> +}
> +
> +/*
>   * parse the source unit recursively until it reaches to a terminal
>   * or a branched unit.
>   */
> @@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
>                                 term->name = le16_to_cpu(d->wClockSourceStr);
>                                 return 0;
>                         }
> +                       case UAC3_MIXER_UNIT: {
> +                               struct uac_mixer_unit_descriptor *d = p1;
> +
> +                               err = uac_mixer_unit_get_channels(state, d);
> +                               if (err < 0)
> +                                       return err;
> +
> +                               term->channels = err;
> +                               term->type = d->bDescriptorSubtype << 16; /* virtual type */
> +
> +                               return 0;
> +                       }
>                         default:
>                                 return -ENODEV;
>                         }
> @@ -1797,11 +1869,10 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid,
>   */
>  static void build_mixer_unit_ctl(struct mixer_build *state,
>                                  struct uac_mixer_unit_descriptor *desc,
> -                                int in_pin, int in_ch, int unitid,
> -                                struct usb_audio_term *iterm)
> +                                int in_pin, int in_ch, int num_outs,
> +                                int unitid, struct usb_audio_term *iterm)
>  {
>         struct usb_mixer_elem_info *cval;
> -       unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
>         unsigned int i, len;
>         struct snd_kcontrol *kctl;
>         const struct usbmix_name_map *map;
> @@ -1878,14 +1949,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>         int input_pins, num_ins, num_outs;
>         int pin, ich, err;
>
> -       if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
> -           !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
> +       err = uac_mixer_unit_get_channels(state, desc);
> +       if (err < 0) {
>                 usb_audio_err(state->chip,
>                               "invalid MIXER UNIT descriptor %d\n",
>                               unitid);
> -               return -EINVAL;
> +               return err;
>         }
>
> +       num_outs = err;
> +       input_pins = desc->bNrInPins;
> +
>         num_ins = 0;
>         ich = 0;
>         for (pin = 0; pin < input_pins; pin++) {
> @@ -1912,7 +1986,7 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>                                 }
>                         }
>                         if (ich_has_controls)
> -                               build_mixer_unit_ctl(state, desc, pin, ich,
> +                               build_mixer_unit_ctl(state, desc, pin, ich, num_outs,
>                                                      unitid, &iterm);

So with current sources we will never reach this place. In your
usecase (mixer with no
controls) it obviously won't go inside.
However, I created a mixer with controls but still
build_mixer_unit_ctl() isn't executed.
This is because UAC3 input terminal parsing always returns 0 channels, this is
what still needs to be implemented (see comment "REVISIT: UAC3 IT doesn't
have channels/cfg" in check_input_term)

Beside of that, other part of this patch seems to work as expected, at least
uac_mixer_unit_get_channels() gives correct results - I checked it for
two-channels config, that is different comparing to BADD.

Thanks,
Ruslan

Re: [PATCH v3 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.

[Jorge]

On 04/05/18 01:57, Ruslan Bilovol wrote:
> On Fri, Apr 27, 2018 at 8:06 PM, Jorge Sanjuan
> <jorge.sanjuan@codethink.co.uk> wrote:
>> This adds support for the MIXER UNIT in UAC3. All the information
>> is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
>> read the rest of the logical cluster to obtain the channel config
>> as that wont make any difference in the current mixer behaviour.
>>
>> The name of the mixer unit is not yet requested as there is not
>> support for the UAC3 Class Specific String requests.
>>
>> Tested in an UAC3 device working as a HEADSET with a basic mixer
>> unit (same as the one in the BADD spec) with no controls.
> 
> So, after deeper looking into the code and after testing this patch,
> in your usecase (mixer with no controls) you'll never execute
> build_mixer_unit_ctl(), correct? So did you try to just fix issues with
> incorrect parsing of mixer unit descriptor?
> 
>>
>> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
>> ---
>>   include/uapi/linux/usb/audio.h | 19 +++++++--
>>   sound/usb/mixer.c              | 88 ++++++++++++++++++++++++++++++++++++++----
>>   2 files changed, 97 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
>> index 3a78e7145689..13d98e6e0db1 100644
>> --- a/include/uapi/linux/usb/audio.h
>> +++ b/include/uapi/linux/usb/audio.h
>> @@ -285,9 +285,22 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
>>   static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
>>                                                int protocol)
>>   {
>> -       return (protocol == UAC_VERSION_1) ?
>> -               &desc->baSourceID[desc->bNrInPins + 4] :
>> -               &desc->baSourceID[desc->bNrInPins + 6];
>> +       switch (protocol) {
>> +       case UAC_VERSION_1:
>> +               return &desc->baSourceID[desc->bNrInPins + 4];
>> +       case UAC_VERSION_2:
>> +               return &desc->baSourceID[desc->bNrInPins + 6];
>> +       case UAC_VERSION_3:
>> +               return &desc->baSourceID[desc->bNrInPins + 2];
>> +       default:
>> +               return NULL;
>> +       }
>> +}
>> +
>> +static inline __u16 uac3_mixer_unit_wClusterDescrID(struct uac_mixer_unit_descriptor *desc)
>> +{
>> +       return (desc->baSourceID[desc->bNrInPins + 1] << 8) |
>> +               desc->baSourceID[desc->bNrInPins];
>>   }
>>
>>   static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 301ad61ed426..3503f4840ec3 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
>>   }
>>
>>   /*
>> + * Get logical cluster information for UAC3 devices.
>> + */
>> +static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
>> +{
>> +       struct uac3_cluster_header_descriptor c_header;
>> +       int err;
>> +
>> +       err = snd_usb_ctl_msg(state->chip->dev,
>> +                       usb_rcvctrlpipe(state->chip->dev, 0),
>> +                       UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
>> +                       USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
>> +                       cluster_id,
>> +                       snd_usb_ctrl_intf(state->chip),
>> +                       &c_header, sizeof(c_header));
>> +       if (err < 0)
>> +               goto error;
>> +       if (err != sizeof(c_header)) {
>> +               err = -EIO;
>> +               goto error;
>> +       }
>> +
>> +       return c_header.bNrChannels;
>> +
>> +error:
>> +       usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
>> +       return err;
>> +}
>> +
>> +/*
>> + * Get number of channels for a Mixer Unit.
>> + */
>> +static int uac_mixer_unit_get_channels(struct mixer_build *state,
>> +                                      struct uac_mixer_unit_descriptor *desc)
>> +{
>> +       int mu_channels;
>> +
>> +       if (desc->bLength < 11)
>> +               return -EINVAL;
>> +       if (!desc->bNrInPins)
>> +               return -EINVAL;
>> +
>> +       switch (state->mixer->protocol) {
>> +       case UAC_VERSION_1:
>> +       case UAC_VERSION_2:
>> +       default:
>> +               mu_channels = uac_mixer_unit_bNrChannels(desc);
>> +               break;
>> +       case UAC_VERSION_3:
>> +               mu_channels = get_cluster_channels_v3(state,
>> +                               uac3_mixer_unit_wClusterDescrID(desc));
>> +               break;
>> +       }
>> +
>> +       if (!mu_channels)
>> +               return -EINVAL;
>> +
>> +       return mu_channels;
>> +}
>> +
>> +/*
>>    * parse the source unit recursively until it reaches to a terminal
>>    * or a branched unit.
>>    */
>> @@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
>>                                  term->name = le16_to_cpu(d->wClockSourceStr);
>>                                  return 0;
>>                          }
>> +                       case UAC3_MIXER_UNIT: {
>> +                               struct uac_mixer_unit_descriptor *d = p1;
>> +
>> +                               err = uac_mixer_unit_get_channels(state, d);
>> +                               if (err < 0)
>> +                                       return err;
>> +
>> +                               term->channels = err;
>> +                               term->type = d->bDescriptorSubtype << 16; /* virtual type */
>> +
>> +                               return 0;
>> +                       }
>>                          default:
>>                                  return -ENODEV;
>>                          }
>> @@ -1797,11 +1869,10 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid,
>>    */
>>   static void build_mixer_unit_ctl(struct mixer_build *state,
>>                                   struct uac_mixer_unit_descriptor *desc,
>> -                                int in_pin, int in_ch, int unitid,
>> -                                struct usb_audio_term *iterm)
>> +                                int in_pin, int in_ch, int num_outs,
>> +                                int unitid, struct usb_audio_term *iterm)
>>   {
>>          struct usb_mixer_elem_info *cval;
>> -       unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
>>          unsigned int i, len;
>>          struct snd_kcontrol *kctl;
>>          const struct usbmix_name_map *map;
>> @@ -1878,14 +1949,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>>          int input_pins, num_ins, num_outs;
>>          int pin, ich, err;
>>
>> -       if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
>> -           !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
>> +       err = uac_mixer_unit_get_channels(state, desc);
>> +       if (err < 0) {
>>                  usb_audio_err(state->chip,
>>                                "invalid MIXER UNIT descriptor %d\n",
>>                                unitid);
>> -               return -EINVAL;
>> +               return err;
>>          }
>>
>> +       num_outs = err;
>> +       input_pins = desc->bNrInPins;
>> +
>>          num_ins = 0;
>>          ich = 0;
>>          for (pin = 0; pin < input_pins; pin++) {
>> @@ -1912,7 +1986,7 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>>                                  }
>>                          }
>>                          if (ich_has_controls)
>> -                               build_mixer_unit_ctl(state, desc, pin, ich,
>> +                               build_mixer_unit_ctl(state, desc, pin, ich, num_outs,
>>                                                       unitid, &iterm);
> 
> So with current sources we will never reach this place. In your
> usecase (mixer with no
> controls) it obviously won't go inside.
> However, I created a mixer with controls but still
> build_mixer_unit_ctl() isn't executed.
> This is because UAC3 input terminal parsing always returns 0 channels, this is
> what still needs to be implemented (see comment "REVISIT: UAC3 IT doesn't
> have channels/cfg" in check_input_term)

Thanks for testing this!

I missed that one with the number of input channels. Regarding the 
"REVISIT: UAC3 IT doesn't have channels/cfg" comment, we can easily fix 
that using the helper function I added in this patch for reading the 
logical cluster's number of channels `get_cluster_channels_v3`. The 
channel config bitmap I don't see it being used at all so parsing 
iterm.channels should be enough. Any thoughts?

- Jorge

> 
> Beside of that, other part of this patch seems to work as expected, at least
> uac_mixer_unit_get_channels() gives correct results - I checked it for
> two-channels config, that is different comparing to BADD.
> 
> Thanks,
> Ruslan
> 

Re: [PATCH v3 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.

[Ruslan Bilovol]

On Tue, May 8, 2018 at 12:43 PM, Jorge <jorge.sanjuan@codethink.co.uk> wrote:
>
>
> On 04/05/18 01:57, Ruslan Bilovol wrote:
>>
>> On Fri, Apr 27, 2018 at 8:06 PM, Jorge Sanjuan
>> <jorge.sanjuan@codethink.co.uk> wrote:
>>>
>>> This adds support for the MIXER UNIT in UAC3. All the information
>>> is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
>>> read the rest of the logical cluster to obtain the channel config
>>> as that wont make any difference in the current mixer behaviour.
>>>
>>> The name of the mixer unit is not yet requested as there is not
>>> support for the UAC3 Class Specific String requests.
>>>
>>> Tested in an UAC3 device working as a HEADSET with a basic mixer
>>> unit (same as the one in the BADD spec) with no controls.
>>
>>
>> So, after deeper looking into the code and after testing this patch,
>> in your usecase (mixer with no controls) you'll never execute
>> build_mixer_unit_ctl(), correct? So did you try to just fix issues with
>> incorrect parsing of mixer unit descriptor?
>>
>>>
>>> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
>>> ---
>>>   include/uapi/linux/usb/audio.h | 19 +++++++--
>>>   sound/usb/mixer.c              | 88
>>> ++++++++++++++++++++++++++++++++++++++----
>>>   2 files changed, 97 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/usb/audio.h
>>> b/include/uapi/linux/usb/audio.h
>>> index 3a78e7145689..13d98e6e0db1 100644
>>> --- a/include/uapi/linux/usb/audio.h
>>> +++ b/include/uapi/linux/usb/audio.h
>>> @@ -285,9 +285,22 @@ static inline __u8
>>> uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
>>>   static inline __u8 *uac_mixer_unit_bmControls(struct
>>> uac_mixer_unit_descriptor *desc,
>>>                                                int protocol)
>>>   {
>>> -       return (protocol == UAC_VERSION_1) ?
>>> -               &desc->baSourceID[desc->bNrInPins + 4] :
>>> -               &desc->baSourceID[desc->bNrInPins + 6];
>>> +       switch (protocol) {
>>> +       case UAC_VERSION_1:
>>> +               return &desc->baSourceID[desc->bNrInPins + 4];
>>> +       case UAC_VERSION_2:
>>> +               return &desc->baSourceID[desc->bNrInPins + 6];
>>> +       case UAC_VERSION_3:
>>> +               return &desc->baSourceID[desc->bNrInPins + 2];
>>> +       default:
>>> +               return NULL;
>>> +       }
>>> +}
>>> +
>>> +static inline __u16 uac3_mixer_unit_wClusterDescrID(struct
>>> uac_mixer_unit_descriptor *desc)
>>> +{
>>> +       return (desc->baSourceID[desc->bNrInPins + 1] << 8) |
>>> +               desc->baSourceID[desc->bNrInPins];
>>>   }
>>>
>>>   static inline __u8 uac_mixer_unit_iMixer(struct
>>> uac_mixer_unit_descriptor *desc)
>>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>>> index 301ad61ed426..3503f4840ec3 100644
>>> --- a/sound/usb/mixer.c
>>> +++ b/sound/usb/mixer.c
>>> @@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state,
>>> struct usb_audio_term *iterm
>>>   }
>>>
>>>   /*
>>> + * Get logical cluster information for UAC3 devices.
>>> + */
>>> +static int get_cluster_channels_v3(struct mixer_build *state, unsigned
>>> int cluster_id)
>>> +{
>>> +       struct uac3_cluster_header_descriptor c_header;
>>> +       int err;
>>> +
>>> +       err = snd_usb_ctl_msg(state->chip->dev,
>>> +                       usb_rcvctrlpipe(state->chip->dev, 0),
>>> +                       UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
>>> +                       USB_RECIP_INTERFACE | USB_TYPE_CLASS |
>>> USB_DIR_IN,
>>> +                       cluster_id,
>>> +                       snd_usb_ctrl_intf(state->chip),
>>> +                       &c_header, sizeof(c_header));
>>> +       if (err < 0)
>>> +               goto error;
>>> +       if (err != sizeof(c_header)) {
>>> +               err = -EIO;
>>> +               goto error;
>>> +       }
>>> +
>>> +       return c_header.bNrChannels;
>>> +
>>> +error:
>>> +       usb_audio_err(state->chip, "cannot request logical cluster ID: %d
>>> (err: %d)\n", cluster_id, err);
>>> +       return err;
>>> +}
>>> +
>>> +/*
>>> + * Get number of channels for a Mixer Unit.
>>> + */
>>> +static int uac_mixer_unit_get_channels(struct mixer_build *state,
>>> +                                      struct uac_mixer_unit_descriptor
>>> *desc)
>>> +{
>>> +       int mu_channels;
>>> +
>>> +       if (desc->bLength < 11)
>>> +               return -EINVAL;
>>> +       if (!desc->bNrInPins)
>>> +               return -EINVAL;
>>> +
>>> +       switch (state->mixer->protocol) {
>>> +       case UAC_VERSION_1:
>>> +       case UAC_VERSION_2:
>>> +       default:
>>> +               mu_channels = uac_mixer_unit_bNrChannels(desc);
>>> +               break;
>>> +       case UAC_VERSION_3:
>>> +               mu_channels = get_cluster_channels_v3(state,
>>> +                               uac3_mixer_unit_wClusterDescrID(desc));
>>> +               break;
>>> +       }
>>> +
>>> +       if (!mu_channels)
>>> +               return -EINVAL;
>>> +
>>> +       return mu_channels;
>>> +}
>>> +
>>> +/*
>>>    * parse the source unit recursively until it reaches to a terminal
>>>    * or a branched unit.
>>>    */
>>> @@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build
>>> *state, int id,
>>>                                  term->name =
>>> le16_to_cpu(d->wClockSourceStr);
>>>                                  return 0;
>>>                          }
>>> +                       case UAC3_MIXER_UNIT: {
>>> +                               struct uac_mixer_unit_descriptor *d = p1;
>>> +
>>> +                               err = uac_mixer_unit_get_channels(state,
>>> d);
>>> +                               if (err < 0)
>>> +                                       return err;
>>> +
>>> +                               term->channels = err;
>>> +                               term->type = d->bDescriptorSubtype << 16;
>>> /* virtual type */
>>> +
>>> +                               return 0;
>>> +                       }
>>>                          default:
>>>                                  return -ENODEV;
>>>                          }
>>> @@ -1797,11 +1869,10 @@ static int parse_audio_feature_unit(struct
>>> mixer_build *state, int unitid,
>>>    */
>>>   static void build_mixer_unit_ctl(struct mixer_build *state,
>>>                                   struct uac_mixer_unit_descriptor *desc,
>>> -                                int in_pin, int in_ch, int unitid,
>>> -                                struct usb_audio_term *iterm)
>>> +                                int in_pin, int in_ch, int num_outs,
>>> +                                int unitid, struct usb_audio_term
>>> *iterm)
>>>   {
>>>          struct usb_mixer_elem_info *cval;
>>> -       unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
>>>          unsigned int i, len;
>>>          struct snd_kcontrol *kctl;
>>>          const struct usbmix_name_map *map;
>>> @@ -1878,14 +1949,17 @@ static int parse_audio_mixer_unit(struct
>>> mixer_build *state, int unitid,
>>>          int input_pins, num_ins, num_outs;
>>>          int pin, ich, err;
>>>
>>> -       if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
>>> -           !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
>>> +       err = uac_mixer_unit_get_channels(state, desc);
>>> +       if (err < 0) {
>>>                  usb_audio_err(state->chip,
>>>                                "invalid MIXER UNIT descriptor %d\n",
>>>                                unitid);
>>> -               return -EINVAL;
>>> +               return err;
>>>          }
>>>
>>> +       num_outs = err;
>>> +       input_pins = desc->bNrInPins;
>>> +
>>>          num_ins = 0;
>>>          ich = 0;
>>>          for (pin = 0; pin < input_pins; pin++) {
>>> @@ -1912,7 +1986,7 @@ static int parse_audio_mixer_unit(struct
>>> mixer_build *state, int unitid,
>>>                                  }
>>>                          }
>>>                          if (ich_has_controls)
>>> -                               build_mixer_unit_ctl(state, desc, pin,
>>> ich,
>>> +                               build_mixer_unit_ctl(state, desc, pin,
>>> ich, num_outs,
>>>                                                       unitid, &iterm);
>>
>>
>> So with current sources we will never reach this place. In your
>> usecase (mixer with no
>> controls) it obviously won't go inside.
>> However, I created a mixer with controls but still
>> build_mixer_unit_ctl() isn't executed.
>> This is because UAC3 input terminal parsing always returns 0 channels,
>> this is
>> what still needs to be implemented (see comment "REVISIT: UAC3 IT doesn't
>> have channels/cfg" in check_input_term)
>
>
> Thanks for testing this!
>
> I missed that one with the number of input channels. Regarding the "REVISIT:
> UAC3 IT doesn't have channels/cfg" comment, we can easily fix that using the
> helper function I added in this patch for reading the logical cluster's
> number of channels `get_cluster_channels_v3`. The channel config bitmap I
> don't see it being used at all so parsing iterm.channels should be enough.
> Any thoughts?
>

I looked into the sources, it seems you are right, we need only number of
channels to proceed with mixer unit created.

I can hack your patch and test it in a few days, or feel free to send v4

Thanks,
Ruslan

[PATCH v4 0/4] ALSA: usb: UAC3 new features.

[Jorge Sanjuan]

v4 Updates:
 - Removes already applied patch from v2 of this patchset.
 - Adds small patch to parse Feature Unit number of channels.
 - Rebased onto latest linux-next tag as today.

Now that the UAC3 patch [1] has made it to linux-next I have some extra
features to make a UAC3 devices fully work in Linux. Including Jack 
insertion control that I have put on top of this other patch [2] for 
UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
to appear in most headset type devices.

UAC3 devices also require to have a Basic Audio Device (BADD) in a separate 
config for which both Ruslan Bilovol and myself have submited different 
approaches[3][4]. After an ongoing discussion between Ruslan and myself
we have decided that the patch from Ruslan[3] implements a simpler and
yet more robust BADD driver.

All this features are tested with an actual UAC3 device that is still in
development. For this patch series, only the legacy config (#1. UAC1/UAC2)
and the UAC3 config have been tested. The BADD config will come in
a different patch from Ruslan.

[1]: https://patchwork.kernel.org/patch/10298179/
[2]: https://patchwork.kernel.org/patch/10305847/
[3]: https://patchwork.kernel.org/patch/10340851/
[4]: https://www.spinics.net/lists/alsa-devel/msg71617.html

Based on linux-next tag: next-20180510

Jorge Sanjuan (4):
  ALSA: usb-audio: UAC3. Add support for mixer unit.
  ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
  ALSA: usb-audio: UAC3 Add support for connector insertion.
  ALSA: usb-audio: UAC3: Parse Input Terminal number of channels.

 include/linux/usb/audio-v2.h   |   7 ++
 include/linux/usb/audio-v3.h   |  14 +++
 include/uapi/linux/usb/audio.h |  19 +++-
 sound/usb/mixer.c              | 200 ++++++++++++++++++++++++++++++++++++-----
 sound/usb/stream.c             |   9 +-
 5 files changed, 222 insertions(+), 27 deletions(-)

-- 
2.11.0

[PATCH v4 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.

[Jorge Sanjuan]

This adds support for the MIXER UNIT in UAC3. All the information
is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
read the rest of the logical cluster to obtain the channel config
as that wont make any difference in the current mixer behaviour.

The name of the mixer unit is not yet requested as there is not
support for the UAC3 Class Specific String requests.

Tested in an UAC3 device working as a HEADSET with a basic mixer
unit (same as the one in the BADD spec) with no controls.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 include/uapi/linux/usb/audio.h | 19 +++++++--
 sound/usb/mixer.c              | 88 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 97 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
index 3a78e7145689..13d98e6e0db1 100644
--- a/include/uapi/linux/usb/audio.h
+++ b/include/uapi/linux/usb/audio.h
@@ -285,9 +285,22 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
 static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
 					      int protocol)
 {
-	return (protocol == UAC_VERSION_1) ?
-		&desc->baSourceID[desc->bNrInPins + 4] :
-		&desc->baSourceID[desc->bNrInPins + 6];
+	switch (protocol) {
+	case UAC_VERSION_1:
+		return &desc->baSourceID[desc->bNrInPins + 4];
+	case UAC_VERSION_2:
+		return &desc->baSourceID[desc->bNrInPins + 6];
+	case UAC_VERSION_3:
+		return &desc->baSourceID[desc->bNrInPins + 2];
+	default:
+		return NULL;
+	}
+}
+
+static inline __u16 uac3_mixer_unit_wClusterDescrID(struct uac_mixer_unit_descriptor *desc)
+{
+	return (desc->baSourceID[desc->bNrInPins + 1] << 8) |
+		desc->baSourceID[desc->bNrInPins];
 }
 
 static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 76417943ff85..129c1397f0cb 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -719,6 +719,66 @@ static int get_term_name(struct snd_usb_audio *chip, struct usb_audio_term *iter
 }
 
 /*
+ * Get logical cluster information for UAC3 devices.
+ */
+static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
+{
+	struct uac3_cluster_header_descriptor c_header;
+	int err;
+
+	err = snd_usb_ctl_msg(state->chip->dev,
+			usb_rcvctrlpipe(state->chip->dev, 0),
+			UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
+			USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+			cluster_id,
+			snd_usb_ctrl_intf(state->chip),
+			&c_header, sizeof(c_header));
+	if (err < 0)
+		goto error;
+	if (err != sizeof(c_header)) {
+		err = -EIO;
+		goto error;
+	}
+
+	return c_header.bNrChannels;
+
+error:
+	usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
+	return err;
+}
+
+/*
+ * Get number of channels for a Mixer Unit.
+ */
+static int uac_mixer_unit_get_channels(struct mixer_build *state,
+				       struct uac_mixer_unit_descriptor *desc)
+{
+	int mu_channels;
+
+	if (desc->bLength < 11)
+		return -EINVAL;
+	if (!desc->bNrInPins)
+		return -EINVAL;
+
+	switch (state->mixer->protocol) {
+	case UAC_VERSION_1:
+	case UAC_VERSION_2:
+	default:
+		mu_channels = uac_mixer_unit_bNrChannels(desc);
+		break;
+	case UAC_VERSION_3:
+		mu_channels = get_cluster_channels_v3(state,
+				uac3_mixer_unit_wClusterDescrID(desc));
+		break;
+	}
+
+	if (!mu_channels)
+		return -EINVAL;
+
+	return mu_channels;
+}
+
+/*
  * parse the source unit recursively until it reaches to a terminal
  * or a branched unit.
  */
@@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
 				term->name = le16_to_cpu(d->wClockSourceStr);
 				return 0;
 			}
+			case UAC3_MIXER_UNIT: {
+				struct uac_mixer_unit_descriptor *d = p1;
+
+				err = uac_mixer_unit_get_channels(state, d);
+				if (err < 0)
+					return err;
+
+				term->channels = err;
+				term->type = d->bDescriptorSubtype << 16; /* virtual type */
+
+				return 0;
+			}
 			default:
 				return -ENODEV;
 			}
@@ -1798,11 +1870,10 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid,
  */
 static void build_mixer_unit_ctl(struct mixer_build *state,
 				 struct uac_mixer_unit_descriptor *desc,
-				 int in_pin, int in_ch, int unitid,
-				 struct usb_audio_term *iterm)
+				 int in_pin, int in_ch, int num_outs,
+				 int unitid, struct usb_audio_term *iterm)
 {
 	struct usb_mixer_elem_info *cval;
-	unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
 	unsigned int i, len;
 	struct snd_kcontrol *kctl;
 	const struct usbmix_name_map *map;
@@ -1879,14 +1950,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
 	int input_pins, num_ins, num_outs;
 	int pin, ich, err;
 
-	if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
-	    !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
+	err = uac_mixer_unit_get_channels(state, desc);
+	if (err < 0) {
 		usb_audio_err(state->chip,
 			      "invalid MIXER UNIT descriptor %d\n",
 			      unitid);
-		return -EINVAL;
+		return err;
 	}
 
+	num_outs = err;
+	input_pins = desc->bNrInPins;
+
 	num_ins = 0;
 	ich = 0;
 	for (pin = 0; pin < input_pins; pin++) {
@@ -1913,7 +1987,7 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
 				}
 			}
 			if (ich_has_controls)
-				build_mixer_unit_ctl(state, desc, pin, ich,
+				build_mixer_unit_ctl(state, desc, pin, ich, num_outs,
 						     unitid, &iterm);
 		}
 	}
-- 
2.11.0

[PATCH v4 2/4] ALSA: usb-audio: Use Class Specific EP for UAC3 devices.

[Jorge Sanjuan]

bmAtributes offset doesn't exist in the UAC3 CS_EP descriptor.
Hence, checking for pitch control as if it was UAC2 doesn't make
any sense. Use the defined UAC3 offsets instead.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 sound/usb/stream.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 764be07474a8..6b2924533d8d 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -576,7 +576,7 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
 
 	if (protocol == UAC_VERSION_1) {
 		attributes = csep->bmAttributes;
-	} else {
+	} else if (protocol == UAC_VERSION_2) {
 		struct uac2_iso_endpoint_descriptor *csep2 =
 			(struct uac2_iso_endpoint_descriptor *) csep;
 
@@ -585,6 +585,13 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
 		/* emulate the endpoint attributes of a v1 device */
 		if (csep2->bmControls & UAC2_CONTROL_PITCH)
 			attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
+	} else { /* UAC_VERSION_3 */
+		struct uac3_iso_endpoint_descriptor *csep3 =
+			(struct uac3_iso_endpoint_descriptor *) csep;
+
+		/* emulate the endpoint attributes of a v1 device */
+		if (le32_to_cpu(csep3->bmControls) & UAC2_CONTROL_PITCH)
+			attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
 	}
 
 	return attributes;
-- 
2.11.0

[PATCH v4 3/4] ALSA: usb-audio: UAC3 Add support for connector insertion.

[Jorge Sanjuan]

This adds support for the UAC3 insertion controls. The status
is reported as a boolean value in the same way it used to do
for UAC2. Hence, the presence of any connector in the response
will make the control saying the jack is connected.

The UAC2 support for this control has been moved to a dedicated
control for connectors as both UAC2 and UAC3 follow a specific
Control Request Parameter Block for this control. This parameter
block for UAC3 could not be read in the same simplistic
manner as in UAC2.

This implementation is not requesting additional information
from the HIGH CAPABILITY Connectors descriptor.

Tested with an UAC3 device with UAC2 as legacy configuration.
The connector status can be read with `amixer` and the interrupt
is also caught with `alsactl monitor`.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 include/linux/usb/audio-v2.h |   7 +++
 include/linux/usb/audio-v3.h |  14 ++++++
 sound/usb/mixer.c            | 108 +++++++++++++++++++++++++++++++++++++------
 3 files changed, 115 insertions(+), 14 deletions(-)

diff --git a/include/linux/usb/audio-v2.h b/include/linux/usb/audio-v2.h
index 49699255cfd3..ba4b3e3327ff 100644
--- a/include/linux/usb/audio-v2.h
+++ b/include/linux/usb/audio-v2.h
@@ -189,6 +189,13 @@ struct uac2_iso_endpoint_descriptor {
 #define UAC2_CONTROL_DATA_OVERRUN	(3 << 2)
 #define UAC2_CONTROL_DATA_UNDERRUN	(3 << 4)
 
+/* 5.2.5.4.2 Connector Control Parameter Block */
+struct uac2_connectors_ctl_blk {
+	__u8 bNrChannels;
+	__le32 bmChannelConfig;
+	__u8 iChannelNames;
+} __attribute__((packed));
+
 /* 6.1 Interrupt Data Message */
 
 #define UAC2_INTERRUPT_DATA_MSG_VENDOR	(1 << 0)
diff --git a/include/linux/usb/audio-v3.h b/include/linux/usb/audio-v3.h
index 38add1dedf2e..a710e28b5215 100644
--- a/include/linux/usb/audio-v3.h
+++ b/include/linux/usb/audio-v3.h
@@ -221,6 +221,12 @@ struct uac3_iso_endpoint_descriptor {
 	__le16 wLockDelay;
 } __attribute__((packed));
 
+/* 5.2.1.6.1 INSERTION CONTROL PARAMETER BLOCK */
+struct uac3_insertion_ctl_blk {
+	__u8 bSize;
+	__u8 bmConInserted;
+} __attribute__ ((packed));
+
 /* 6.1 INTERRUPT DATA MESSAGE */
 struct uac3_interrupt_data_msg {
 	__u8 bInfo;
@@ -392,6 +398,14 @@ struct uac3_interrupt_data_msg {
 #define UAC3_AC_ACTIVE_INTERFACE_CONTROL	0x01
 #define UAC3_AC_POWER_DOMAIN_CONTROL		0x02
 
+/* A.23.5 TERMINAL CONTROL SELECTORS */
+#define UAC3_TE_UNDEFINED			0x00
+#define UAC3_TE_INSERTION			0x01
+#define UAC3_TE_OVERLOAD			0x02
+#define UAC3_TE_UNDERFLOW			0x03
+#define UAC3_TE_OVERFLOW			0x04
+#define UAC3_TE_LATENCY 			0x05
+
 /* BADD predefined Unit/Terminal values */
 #define UAC3_BADD_IT_ID1	1  /* Input Terminal ID1: bTerminalID = 1 */
 #define UAC3_BADD_FU_ID2	2  /* Feature Unit ID2: bUnitID = 2 */
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 129c1397f0cb..431f3c319839 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1322,6 +1322,51 @@ static int mixer_ctl_master_bool_get(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
+/* get the connectors status and report it as boolean type */
+static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct usb_mixer_elem_info *cval = kcontrol->private_data;
+	struct snd_usb_audio *chip = cval->head.mixer->chip;
+	int idx = 0, validx, ret, val;
+
+	validx = cval->control << 8 | 0;
+
+	ret = snd_usb_lock_shutdown(chip) ? -EIO : 0;
+	if (ret)
+		goto error;
+
+	idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8);
+	if (cval->head.mixer->protocol == UAC_VERSION_2) {
+		struct uac2_connectors_ctl_blk uac2_conn;
+
+		ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), UAC2_CS_CUR,
+				      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+				      validx, idx, &uac2_conn, sizeof(uac2_conn));
+		val = !!uac2_conn.bNrChannels;
+	} else { /* UAC_VERSION_3 */
+		struct uac3_insertion_ctl_blk uac3_conn;
+
+		ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), UAC2_CS_CUR,
+				      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+				      validx, idx, &uac3_conn, sizeof(uac3_conn));
+		val = !!uac3_conn.bmConInserted;
+	}
+
+	snd_usb_unlock_shutdown(chip);
+
+	if (ret < 0) {
+error:
+		usb_audio_err(chip,
+			"cannot get connectors status: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n",
+			UAC_GET_CUR, validx, idx, cval->val_type);
+		return ret;
+	}
+
+	ucontrol->value.integer.value[0] = val;
+	return 0;
+}
+
 static struct snd_kcontrol_new usb_feature_unit_ctl = {
 	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
 	.name = "", /* will be filled later manually */
@@ -1352,6 +1397,15 @@ static struct snd_kcontrol_new usb_bool_master_control_ctl_ro = {
 	.put = NULL,
 };
 
+static const struct snd_kcontrol_new usb_connector_ctl_ro = {
+	.iface = SNDRV_CTL_ELEM_IFACE_CARD,
+	.name = "", /* will be filled later manually */
+	.access = SNDRV_CTL_ELEM_ACCESS_READ,
+	.info = snd_ctl_boolean_mono_info,
+	.get = mixer_ctl_connector_get,
+	.put = NULL,
+};
+
 /*
  * This symbol is exported in order to allow the mixer quirks to
  * hook up to the standard feature unit control mechanism
@@ -1598,17 +1652,25 @@ static void build_connector_control(struct mixer_build *state,
 		return;
 	snd_usb_mixer_elem_init_std(&cval->head, state->mixer, term->id);
 	/*
-	 * The first byte from reading the UAC2_TE_CONNECTOR control returns the
-	 * number of channels connected.  This boolean ctl will simply report
-	 * if any channels are connected or not.
-	 * (Audio20_final.pdf Table 5-10: Connector Control CUR Parameter Block)
+	 * UAC2: The first byte from reading the UAC2_TE_CONNECTOR control returns the
+	 * number of channels connected.
+	 *
+	 * UAC3: The first byte specifies size of bitmap for the inserted controls. The
+	 * following byte(s) specifies which connectors are inserted.
+	 *
+	 * This boolean ctl will simply report if any channels are connected
+	 * or not.
 	 */
-	cval->control = UAC2_TE_CONNECTOR;
+	if (state->mixer->protocol == UAC_VERSION_2)
+		cval->control = UAC2_TE_CONNECTOR;
+	else /* UAC_VERSION_3 */
+		cval->control = UAC3_TE_INSERTION;
+
 	cval->val_type = USB_MIXER_BOOLEAN;
 	cval->channels = 1; /* report true if any channel is connected */
 	cval->min = 0;
 	cval->max = 1;
-	kctl = snd_ctl_new1(&usb_bool_master_control_ctl_ro, cval);
+	kctl = snd_ctl_new1(&usb_connector_ctl_ro, cval);
 	if (!kctl) {
 		usb_audio_err(state->chip, "cannot malloc kcontrol\n");
 		kfree(cval);
@@ -1926,16 +1988,28 @@ static int parse_audio_input_terminal(struct mixer_build *state, int unitid,
 				      void *raw_desc)
 {
 	struct usb_audio_term iterm;
-	struct uac2_input_terminal_descriptor *d = raw_desc;
+	unsigned int control, bmctls, term_id;
 
-	check_input_term(state, d->bTerminalID, &iterm);
 	if (state->mixer->protocol == UAC_VERSION_2) {
-		/* Check for jack detection. */
-		if (uac_v2v3_control_is_readable(le16_to_cpu(d->bmControls),
-						 UAC2_TE_CONNECTOR)) {
-			build_connector_control(state, &iterm, true);
-		}
+		struct uac2_input_terminal_descriptor *d_v2 = raw_desc;
+		control = UAC2_TE_CONNECTOR;
+		term_id = d_v2->bTerminalID;
+		bmctls = le16_to_cpu(d_v2->bmControls);
+	} else if (state->mixer->protocol == UAC_VERSION_3) {
+		struct uac3_input_terminal_descriptor *d_v3 = raw_desc;
+		control = UAC3_TE_INSERTION;
+		term_id = d_v3->bTerminalID;
+		bmctls = le32_to_cpu(d_v3->bmControls);
+	} else {
+		return 0; /* UAC1. No Insertion control */
 	}
+
+	check_input_term(state, term_id, &iterm);
+
+	/* Check for jack detection. */
+	if (uac_v2v3_control_is_readable(bmctls, control))
+		build_connector_control(state, &iterm, true);
+
 	return 0;
 }
 
@@ -2526,7 +2600,7 @@ static int parse_audio_unit(struct mixer_build *state, int unitid)
 	} else { /* UAC_VERSION_3 */
 		switch (p1[2]) {
 		case UAC_INPUT_TERMINAL:
-			return 0; /* NOP */
+			return parse_audio_input_terminal(state, unitid, p1);
 		case UAC3_MIXER_UNIT:
 			return parse_audio_mixer_unit(state, unitid, p1);
 		case UAC3_CLOCK_SOURCE:
@@ -2664,6 +2738,12 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer)
 			err = parse_audio_unit(&state, desc->bCSourceID);
 			if (err < 0 && err != -EINVAL)
 				return err;
+
+			if (uac_v2v3_control_is_readable(le32_to_cpu(desc->bmControls),
+							 UAC3_TE_INSERTION)) {
+				build_connector_control(&state, &state.oterm,
+							false);
+			}
 		}
 	}
 
-- 
2.11.0

[PATCH v4 4/4] ALSA: usb-audio: UAC3: Parse Input Terminal number of channels.

[Jorge Sanjuan]

Obtain the number of channels for the Input Terminal from the
Logical Cluster Descriptor. This achieves a useful minimal parsing
of this unit so it can be used in other units in the topology.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 sound/usb/mixer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 431f3c319839..19b25fbc7437 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -903,9 +903,9 @@ static int check_input_term(struct mixer_build *state, int id,
 				 * recursion calls */
 				term->id = id;
 				term->type = le16_to_cpu(d->wTerminalType);
+				term->channels = get_cluster_channels_v3(state, d->wClusterDescrID);
 
-				/* REVISIT: UAC3 IT doesn't have channels/cfg */
-				term->channels = 0;
+				/* REVISIT: UAC3 IT doesn't have channels cfg */
 				term->chconfig = 0;
 
 				term->name = le16_to_cpu(d->wTerminalDescrStr);
-- 
2.11.0

Re: [PATCH v4 4/4] ALSA: usb-audio: UAC3: Parse Input Terminal number of channels.

[Jorge]

On 11/05/18 16:25, Jorge Sanjuan wrote:
> Obtain the number of channels for the Input Terminal from the
> Logical Cluster Descriptor. This achieves a useful minimal parsing
> of this unit so it can be used in other units in the topology.
> 
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
> ---
>   sound/usb/mixer.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 431f3c319839..19b25fbc7437 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -903,9 +903,9 @@ static int check_input_term(struct mixer_build *state, int id,
>   				 * recursion calls */
>   				term->id = id;
>   				term->type = le16_to_cpu(d->wTerminalType);
> +				term->channels = get_cluster_channels_v3(state, d->wClusterDescrID);


Sorry about this. I just spotted that I should have used the helper 
function I added to access d->wClusterDescrID 
`uac3_mixer_unit_wClusterDescrID`.

I got the sparse warning for the endianess and realized that. I'll 
resend this one patch.

>   
> -				/* REVISIT: UAC3 IT doesn't have channels/cfg */
> -				term->channels = 0;
> +				/* REVISIT: UAC3 IT doesn't have channels cfg */
>   				term->chconfig = 0;
>   
>   				term->name = le16_to_cpu(d->wTerminalDescrStr);
> 

Re: [PATCH v4 4/4] ALSA: usb-audio: UAC3: Parse Input Terminal number of channels.

[Ruslan Bilovol]

On Mon, May 14, 2018 at 11:54 AM, Jorge <jorge.sanjuan@codethink.co.uk> wrote:
>
>
> On 11/05/18 16:25, Jorge Sanjuan wrote:
>>
>> Obtain the number of channels for the Input Terminal from the
>> Logical Cluster Descriptor. This achieves a useful minimal parsing
>> of this unit so it can be used in other units in the topology.
>>
>> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
>> ---
>>   sound/usb/mixer.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 431f3c319839..19b25fbc7437 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -903,9 +903,9 @@ static int check_input_term(struct mixer_build *state,
>> int id,
>>                                  * recursion calls */
>>                                 term->id = id;
>>                                 term->type =
>> le16_to_cpu(d->wTerminalType);
>> +                               term->channels =
>> get_cluster_channels_v3(state, d->wClusterDescrID);
>
>
>
> Sorry about this. I just spotted that I should have used the helper function
> I added to access d->wClusterDescrID `uac3_mixer_unit_wClusterDescrID`.
>
> I got the sparse warning for the endianess and realized that. I'll resend
> this one patch.

While here, please add checking output of get_cluster_channels_v3() as
it can return negative errno.

BTW, I've just tested your Mixer patches and this is the only comment I have
so far.

Thanks,
Ruslan

>
>>   -                             /* REVISIT: UAC3 IT doesn't have
>> channels/cfg */
>> -                               term->channels = 0;
>> +                               /* REVISIT: UAC3 IT doesn't have channels
>> cfg */
>>                                 term->chconfig = 0;
>>                                 term->name =
>> le16_to_cpu(d->wTerminalDescrStr);
>>
>

[RESEND PATCH v4 4/4] ALSA: usb-audio: UAC3: Parse Input Terminal number of channels.

[Jorge Sanjuan]

Obtain the number of channels for the Input Terminal from the
Logical Cluster Descriptor. This achieves a useful minimal parsing
of this unit so it can be used in other units in the topology.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 sound/usb/mixer.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 431f3c319839..99804cd4aed6 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -904,8 +904,12 @@ static int check_input_term(struct mixer_build *state, int id,
 				term->id = id;
 				term->type = le16_to_cpu(d->wTerminalType);
 
-				/* REVISIT: UAC3 IT doesn't have channels/cfg */
-				term->channels = 0;
+				err = get_cluster_channels_v3(state, le16_to_cpu(d->wClusterDescrID));
+				if (err < 0)
+					return err;
+				term->channels = err;
+
+				/* REVISIT: UAC3 IT doesn't have channels cfg */
 				term->chconfig = 0;
 
 				term->name = le16_to_cpu(d->wTerminalDescrStr);
-- 
2.11.0

Re: [PATCH v4 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.

[Ruslan Bilovol]

On Fri, May 11, 2018 at 6:25 PM, Jorge Sanjuan
<jorge.sanjuan@codethink.co.uk> wrote:
> This adds support for the MIXER UNIT in UAC3. All the information
> is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
> read the rest of the logical cluster to obtain the channel config
> as that wont make any difference in the current mixer behaviour.
>
> The name of the mixer unit is not yet requested as there is not
> support for the UAC3 Class Specific String requests.
>
> Tested in an UAC3 device working as a HEADSET with a basic mixer
> unit (same as the one in the BADD spec) with no controls.

I tested this patch in a similar use-case (with a simple mixer unit),
but _with_ controls and along with patch [1] from this series, which
added parsing input terminal's channels.
So everything works fine, I see all needed requests handling and
mixer unit creation on ALSA side, which I can use now.

So, as a bottom line:
Reviewed-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Tested-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-May/136030.html

>
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
> ---
>  include/uapi/linux/usb/audio.h | 19 +++++++--
>  sound/usb/mixer.c              | 88 ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 97 insertions(+), 10 deletions(-)
>
> diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
> index 3a78e7145689..13d98e6e0db1 100644
> --- a/include/uapi/linux/usb/audio.h
> +++ b/include/uapi/linux/usb/audio.h
> @@ -285,9 +285,22 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
>  static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
>                                               int protocol)
>  {
> -       return (protocol == UAC_VERSION_1) ?
> -               &desc->baSourceID[desc->bNrInPins + 4] :
> -               &desc->baSourceID[desc->bNrInPins + 6];
> +       switch (protocol) {
> +       case UAC_VERSION_1:
> +               return &desc->baSourceID[desc->bNrInPins + 4];
> +       case UAC_VERSION_2:
> +               return &desc->baSourceID[desc->bNrInPins + 6];
> +       case UAC_VERSION_3:
> +               return &desc->baSourceID[desc->bNrInPins + 2];
> +       default:
> +               return NULL;
> +       }
> +}
> +
> +static inline __u16 uac3_mixer_unit_wClusterDescrID(struct uac_mixer_unit_descriptor *desc)
> +{
> +       return (desc->baSourceID[desc->bNrInPins + 1] << 8) |
> +               desc->baSourceID[desc->bNrInPins];
>  }
>
>  static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 76417943ff85..129c1397f0cb 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -719,6 +719,66 @@ static int get_term_name(struct snd_usb_audio *chip, struct usb_audio_term *iter
>  }
>
>  /*
> + * Get logical cluster information for UAC3 devices.
> + */
> +static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
> +{
> +       struct uac3_cluster_header_descriptor c_header;
> +       int err;
> +
> +       err = snd_usb_ctl_msg(state->chip->dev,
> +                       usb_rcvctrlpipe(state->chip->dev, 0),
> +                       UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
> +                       USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
> +                       cluster_id,
> +                       snd_usb_ctrl_intf(state->chip),
> +                       &c_header, sizeof(c_header));
> +       if (err < 0)
> +               goto error;
> +       if (err != sizeof(c_header)) {
> +               err = -EIO;
> +               goto error;
> +       }
> +
> +       return c_header.bNrChannels;
> +
> +error:
> +       usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
> +       return err;
> +}
> +
> +/*
> + * Get number of channels for a Mixer Unit.
> + */
> +static int uac_mixer_unit_get_channels(struct mixer_build *state,
> +                                      struct uac_mixer_unit_descriptor *desc)
> +{
> +       int mu_channels;
> +
> +       if (desc->bLength < 11)
> +               return -EINVAL;
> +       if (!desc->bNrInPins)
> +               return -EINVAL;
> +
> +       switch (state->mixer->protocol) {
> +       case UAC_VERSION_1:
> +       case UAC_VERSION_2:
> +       default:
> +               mu_channels = uac_mixer_unit_bNrChannels(desc);
> +               break;
> +       case UAC_VERSION_3:
> +               mu_channels = get_cluster_channels_v3(state,
> +                               uac3_mixer_unit_wClusterDescrID(desc));
> +               break;
> +       }
> +
> +       if (!mu_channels)
> +               return -EINVAL;
> +
> +       return mu_channels;
> +}
> +
> +/*
>   * parse the source unit recursively until it reaches to a terminal
>   * or a branched unit.
>   */
> @@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
>                                 term->name = le16_to_cpu(d->wClockSourceStr);
>                                 return 0;
>                         }
> +                       case UAC3_MIXER_UNIT: {
> +                               struct uac_mixer_unit_descriptor *d = p1;
> +
> +                               err = uac_mixer_unit_get_channels(state, d);
> +                               if (err < 0)
> +                                       return err;
> +
> +                               term->channels = err;
> +                               term->type = d->bDescriptorSubtype << 16; /* virtual type */
> +
> +                               return 0;
> +                       }
>                         default:
>                                 return -ENODEV;
>                         }
> @@ -1798,11 +1870,10 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid,
>   */
>  static void build_mixer_unit_ctl(struct mixer_build *state,
>                                  struct uac_mixer_unit_descriptor *desc,
> -                                int in_pin, int in_ch, int unitid,
> -                                struct usb_audio_term *iterm)
> +                                int in_pin, int in_ch, int num_outs,
> +                                int unitid, struct usb_audio_term *iterm)
>  {
>         struct usb_mixer_elem_info *cval;
> -       unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
>         unsigned int i, len;
>         struct snd_kcontrol *kctl;
>         const struct usbmix_name_map *map;
> @@ -1879,14 +1950,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>         int input_pins, num_ins, num_outs;
>         int pin, ich, err;
>
> -       if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
> -           !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
> +       err = uac_mixer_unit_get_channels(state, desc);
> +       if (err < 0) {
>                 usb_audio_err(state->chip,
>                               "invalid MIXER UNIT descriptor %d\n",
>                               unitid);
> -               return -EINVAL;
> +               return err;
>         }
>
> +       num_outs = err;
> +       input_pins = desc->bNrInPins;
> +
>         num_ins = 0;
>         ich = 0;
>         for (pin = 0; pin < input_pins; pin++) {
> @@ -1913,7 +1987,7 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>                                 }
>                         }
>                         if (ich_has_controls)
> -                               build_mixer_unit_ctl(state, desc, pin, ich,
> +                               build_mixer_unit_ctl(state, desc, pin, ich, num_outs,
>                                                      unitid, &iterm);
>                 }
>         }
> --
> 2.11.0
>



-- 
Best regards,
Ruslan Bilovol

Re: [PATCH v4 2/4] ALSA: usb-audio: Use Class Specific EP for UAC3 devices.

[Ruslan Bilovol]

On Fri, May 11, 2018 at 6:25 PM, Jorge Sanjuan
<jorge.sanjuan@codethink.co.uk> wrote:
> bmAtributes offset doesn't exist in the UAC3 CS_EP descriptor.
> Hence, checking for pitch control as if it was UAC2 doesn't make
> any sense. Use the defined UAC3 offsets instead.

This one I already reviewed in v2 and there is no changes in v4,
so still:
Reviewed-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>

By the way, this patch is an independent change and can go
into v4.17-rcXX, if it's not too late for it.

Thanks,
Ruslan

>
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
> ---
>  sound/usb/stream.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/sound/usb/stream.c b/sound/usb/stream.c
> index 764be07474a8..6b2924533d8d 100644
> --- a/sound/usb/stream.c
> +++ b/sound/usb/stream.c
> @@ -576,7 +576,7 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
>
>         if (protocol == UAC_VERSION_1) {
>                 attributes = csep->bmAttributes;
> -       } else {
> +       } else if (protocol == UAC_VERSION_2) {
>                 struct uac2_iso_endpoint_descriptor *csep2 =
>                         (struct uac2_iso_endpoint_descriptor *) csep;
>
> @@ -585,6 +585,13 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
>                 /* emulate the endpoint attributes of a v1 device */
>                 if (csep2->bmControls & UAC2_CONTROL_PITCH)
>                         attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
> +       } else { /* UAC_VERSION_3 */
> +               struct uac3_iso_endpoint_descriptor *csep3 =
> +                       (struct uac3_iso_endpoint_descriptor *) csep;
> +
> +               /* emulate the endpoint attributes of a v1 device */
> +               if (le32_to_cpu(csep3->bmControls) & UAC2_CONTROL_PITCH)
> +                       attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
>         }
>
>         return attributes;
> --
> 2.11.0
>

Re: [RESEND PATCH v4 4/4] ALSA: usb-audio: UAC3: Parse Input Terminal number of channels.

[Ruslan Bilovol]

On Mon, May 14, 2018 at 2:03 PM, Jorge Sanjuan
<jorge.sanjuan@codethink.co.uk> wrote:
> Obtain the number of channels for the Input Terminal from the
> Logical Cluster Descriptor. This achieves a useful minimal parsing
> of this unit so it can be used in other units in the topology.

Usually 'patch resend' means resend without any changes, and if there
are updates in the patch - it's a new version.

By the way, as I already said in comments to patch 1/4 [1], I verified
this patch successfully.

Reviewed-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Tested-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-May/136044.html

>
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
> ---
>  sound/usb/mixer.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 431f3c319839..99804cd4aed6 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -904,8 +904,12 @@ static int check_input_term(struct mixer_build *state, int id,
>                                 term->id = id;
>                                 term->type = le16_to_cpu(d->wTerminalType);
>
> -                               /* REVISIT: UAC3 IT doesn't have channels/cfg */
> -                               term->channels = 0;
> +                               err = get_cluster_channels_v3(state, le16_to_cpu(d->wClusterDescrID));
> +                               if (err < 0)
> +                                       return err;
> +                               term->channels = err;
> +
> +                               /* REVISIT: UAC3 IT doesn't have channels cfg */
>                                 term->chconfig = 0;
>
>                                 term->name = le16_to_cpu(d->wTerminalDescrStr);
> --
> 2.11.0
>

Re: [PATCH v4 0/4] ALSA: usb: UAC3 new features.

[Takashi Iwai]

On Fri, 11 May 2018 17:25:33 +0200,
Jorge Sanjuan wrote:
> 
> v4 Updates:
>  - Removes already applied patch from v2 of this patchset.
>  - Adds small patch to parse Feature Unit number of channels.
>  - Rebased onto latest linux-next tag as today.
> 
> Now that the UAC3 patch [1] has made it to linux-next I have some extra
> features to make a UAC3 devices fully work in Linux. Including Jack 
> insertion control that I have put on top of this other patch [2] for 
> UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
> to appear in most headset type devices.
> 
> UAC3 devices also require to have a Basic Audio Device (BADD) in a separate 
> config for which both Ruslan Bilovol and myself have submited different 
> approaches[3][4]. After an ongoing discussion between Ruslan and myself
> we have decided that the patch from Ruslan[3] implements a simpler and
> yet more robust BADD driver.
> 
> All this features are tested with an actual UAC3 device that is still in
> development. For this patch series, only the legacy config (#1. UAC1/UAC2)
> and the UAC3 config have been tested. The BADD config will come in
> a different patch from Ruslan.
> 
> [1]: https://patchwork.kernel.org/patch/10298179/
> [2]: https://patchwork.kernel.org/patch/10305847/
> [3]: https://patchwork.kernel.org/patch/10340851/
> [4]: https://www.spinics.net/lists/alsa-devel/msg71617.html
> 
> Based on linux-next tag: next-20180510
> 
> Jorge Sanjuan (4):
>   ALSA: usb-audio: UAC3. Add support for mixer unit.
>   ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
>   ALSA: usb-audio: UAC3 Add support for connector insertion.
>   ALSA: usb-audio: UAC3: Parse Input Terminal number of channels.

OK, now I applied all four patches.  The patch 2 was queued to
for-linus branch while others to for-next.  The patch 4 was taken from
the revised v4.


Thanks!

Takashi