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

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-fea... 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

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

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

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

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

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

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

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;

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