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);

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;

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

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

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

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++) {

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;

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

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

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

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); } }

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); + } } }

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);
  
  

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);

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