Now that the UAC3 patch [1] has made it to linux-next I have some extra
features to make a UAC3 device fully work in Linux. Including Jack
insertion control that I have put on top of this other patch [2] for
UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
to appear in most headset type devices.
UAC3 devices also require to have a Basic Audio Device (BADD) in a separate
config for which both Ruslan Bilovol and myself have submited different
approaches[3][4] but I don't know what the final merge will be. Once there
is official support for BADD, we'll need to test it with an actual UAC3
device to confirm it all wokrs.
All this features are tested with an actual UAC3 device that is still in
development. For this patch series, only the legacy config (#1. UAC1/UAC2)
and the UAC3 config have been tested. The BADD config is only tested using
and updated verison of [4].
[1]: https://patchwork.kernel.org/patch/10298179/
[2]: https://patchwork.kernel.org/patch/10305847/
[3]: https://patchwork.kernel.org/patch/10340851/
[4]: https://www.spinics.net/lists/alsa-devel/msg71617.html
Based on linux-next tag: next-20180420
Jorge Sanjuan (3):
ALSA: usb-audio: UAC3. Add support for mixer unit.
ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
ALSA: usb-audio: UAC3 Add support for connector insertion.
Michael Drake (1):
ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.
include/linux/usb/audio-v2.h | 7 ++
include/linux/usb/audio-v3.h | 14 ++++
include/uapi/linux/usb/audio.h | 13 ++-
sound/usb/mixer.c | 175 ++++++++++++++++++++++++++++++++++++-----
sound/usb/stream.c | 19 ++++-
5 files changed, 204 insertions(+), 24 deletions(-)
--
2.11.0
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 <[email protected]>
---
include/linux/usb/audio-v2.h | 7 +++
include/linux/usb/audio-v3.h | 14 ++++++
sound/usb/mixer.c | 111 +++++++++++++++++++++++++++++++++++++------
3 files changed, 117 insertions(+), 15 deletions(-)
diff --git a/include/linux/usb/audio-v2.h b/include/linux/usb/audio-v2.h
index aaafecf073ff..a96ed2ce3254 100644
--- a/include/linux/usb/audio-v2.h
+++ b/include/linux/usb/audio-v2.h
@@ -189,6 +189,13 @@ struct uac2_iso_endpoint_descriptor {
#define UAC2_CONTROL_DATA_OVERRUN (3 << 2)
#define UAC2_CONTROL_DATA_UNDERRUN (3 << 4)
+/* 5.2.5.4.2 Connector Control Parameter Block */
+struct uac2_connectors_ctl_blk {
+ __u8 bNrChannels;
+ __le32 bmChannelConfig;
+ __u8 iChannelNames;
+} __attribute__((packed));
+
/* 6.1 Interrupt Data Message */
#define UAC2_INTERRUPT_DATA_MSG_VENDOR (1 << 0)
diff --git a/include/linux/usb/audio-v3.h b/include/linux/usb/audio-v3.h
index a8959aaba0ae..6cedb6d499ba 100644
--- a/include/linux/usb/audio-v3.h
+++ b/include/linux/usb/audio-v3.h
@@ -221,6 +221,12 @@ struct uac3_iso_endpoint_descriptor {
__le16 wLockDelay;
} __attribute__((packed));
+/* 5.2.1.6.1 INSERTION CONTROL PARAMETER BLOCK */
+struct uac3_insertion_ctl_blk {
+ __u8 bSize;
+ __u8 bmConInserted[1];
+} __attribute__ ((packed));
+
/* 6.1 INTERRUPT DATA MESSAGE */
struct uac3_interrupt_data_msg {
__u8 bInfo;
@@ -392,4 +398,12 @@ struct uac3_interrupt_data_msg {
#define UAC3_AC_ACTIVE_INTERFACE_CONTROL 0x01
#define UAC3_AC_POWER_DOMAIN_CONTROL 0x02
+/* A.23.5 TERMINAL CONTROL SELECTORS */
+#define UAC3_TE_UNDEFINED 0x00
+#define UAC3_TE_INSERTION 0x01
+#define UAC3_TE_OVERLOAD 0x02
+#define UAC3_TE_UNDERFLOW 0x03
+#define UAC3_TE_OVERFLOW 0x04
+#define UAC3_TE_LATENCY 0x05
+
#endif /* __LINUX_USB_AUDIO_V3_H */
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 738ca37e6d6e..4ddc5d4e1139 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1292,6 +1292,51 @@ static int mixer_ctl_master_bool_get(struct snd_kcontrol *kcontrol,
return 0;
}
+/* get the connectors status and report it as boolean type */
+static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct usb_mixer_elem_info *cval = kcontrol->private_data;
+ struct snd_usb_audio *chip = cval->head.mixer->chip;
+ int idx = 0, validx, ret, val;
+
+ validx = cval->control << 8 | 0;
+
+ ret = snd_usb_lock_shutdown(chip) ? -EIO : 0;
+ if (ret)
+ goto error;
+
+ idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8);
+ if (cval->head.mixer->protocol == UAC_VERSION_2) {
+ struct uac2_connectors_ctl_blk uac2_conn;
+
+ ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), UAC2_CS_CUR,
+ USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+ validx, idx, &uac2_conn, sizeof(uac2_conn));
+ val = !!uac2_conn.bNrChannels;
+ } else { /* UAC_VERSION_3 */
+ struct uac3_insertion_ctl_blk uac3_conn;
+
+ ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), UAC2_CS_CUR,
+ USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+ validx, idx, &uac3_conn, sizeof(uac3_conn));
+ val = !!uac3_conn.bmConInserted[0];
+ }
+
+ snd_usb_unlock_shutdown(chip);
+
+ if (ret < 0) {
+error:
+ usb_audio_err(chip,
+ "cannot get connectors status: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n",
+ UAC_GET_CUR, validx, idx, cval->val_type);
+ return ret;
+ }
+
+ ucontrol->value.integer.value[0] = val;
+ return 0;
+}
+
static struct snd_kcontrol_new usb_feature_unit_ctl = {
.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
.name = "", /* will be filled later manually */
@@ -1322,6 +1367,15 @@ static struct snd_kcontrol_new usb_bool_master_control_ctl_ro = {
.put = NULL,
};
+static struct snd_kcontrol_new usb_connector_ctl_ro = {
+ .iface = SNDRV_CTL_ELEM_IFACE_CARD,
+ .name = "", /* will be filled later manually */
+ .access = SNDRV_CTL_ELEM_ACCESS_READ,
+ .info = snd_ctl_boolean_mono_info,
+ .get = mixer_ctl_connector_get,
+ .put = NULL,
+};
+
/*
* This symbol is exported in order to allow the mixer quirks to
* hook up to the standard feature unit control mechanism
@@ -1568,17 +1622,25 @@ static void build_connector_control(struct mixer_build *state,
return;
snd_usb_mixer_elem_init_std(&cval->head, state->mixer, term->id);
/*
- * The first byte from reading the UAC2_TE_CONNECTOR control returns the
- * number of channels connected. This boolean ctl will simply report
- * if any channels are connected or not.
- * (Audio20_final.pdf Table 5-10: Connector Control CUR Parameter Block)
+ * UAC2: The first byte from reading the UAC2_TE_CONNECTOR control returns the
+ * number of channels connected.
+ *
+ * UAC3: The first byte specifies size of bitmap for the inserted controls. The
+ * following byte(s) specifies which connectors are inserted.
+ *
+ * This boolean ctl will simply report if any channels are connected
+ * or not.
*/
- cval->control = UAC2_TE_CONNECTOR;
+ if (state->mixer->protocol == UAC_VERSION_2)
+ cval->control = UAC2_TE_CONNECTOR;
+ else /* UAC_VERSION_3 */
+ cval->control = UAC3_TE_INSERTION;
+
cval->val_type = USB_MIXER_BOOLEAN;
cval->channels = 1; /* report true if any channel is connected */
cval->min = 0;
cval->max = 1;
- kctl = snd_ctl_new1(&usb_bool_master_control_ctl_ro, cval);
+ kctl = snd_ctl_new1(&usb_connector_ctl_ro, cval);
if (!kctl) {
usb_audio_err(state->chip, "cannot malloc kcontrol\n");
kfree(cval);
@@ -1904,16 +1966,29 @@ static int parse_audio_input_terminal(struct mixer_build *state, int unitid,
void *raw_desc)
{
struct usb_audio_term iterm;
- struct uac2_input_terminal_descriptor *d = raw_desc;
+ unsigned int control, bmctls, term_id;
- check_input_term(state, d->bTerminalID, &iterm);
if (state->mixer->protocol == UAC_VERSION_2) {
- /* Check for jack detection. */
- if (uac_v2v3_control_is_readable(d->bmControls,
- UAC2_TE_CONNECTOR)) {
- build_connector_control(state, &iterm, true);
- }
- }
+ struct uac2_input_terminal_descriptor *d_v2 = raw_desc;
+ control = UAC2_TE_CONNECTOR;
+ term_id = d_v2->bTerminalID;
+ bmctls = d_v2->bmControls;
+ }
+ else if (state->mixer->protocol == UAC_VERSION_3) {
+ struct uac3_input_terminal_descriptor *d_v3 = raw_desc;
+ control = UAC3_TE_INSERTION;
+ term_id = d_v3->bTerminalID;
+ bmctls = d_v3->bmControls;
+ }
+ else /* UAC1. No Insertion control */
+ return 0;
+
+ check_input_term(state, term_id, &iterm);
+
+ /* Check for jack detection. */
+ if (uac_v2v3_control_is_readable(bmctls, control))
+ build_connector_control(state, &iterm, true);
+
return 0;
}
@@ -2507,7 +2582,7 @@ static int parse_audio_unit(struct mixer_build *state, int unitid)
} else { /* UAC_VERSION_3 */
switch (p1[2]) {
case UAC_INPUT_TERMINAL:
- return 0; /* NOP */
+ return parse_audio_input_terminal(state, unitid, p1);
case UAC3_MIXER_UNIT:
return parse_audio_mixer_unit(state, unitid, p1);
case UAC3_CLOCK_SOURCE:
@@ -2645,6 +2720,12 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer)
err = parse_audio_unit(&state, desc->bCSourceID);
if (err < 0 && err != -EINVAL)
return err;
+
+ if (uac_v2v3_control_is_readable(desc->bmControls,
+ UAC3_TE_INSERTION)) {
+ build_connector_control(&state, &state.oterm,
+ false);
+ }
}
}
--
2.11.0
From: Michael Drake <[email protected]>
The channel mapping is defined by bChRelationship, not bChPurpose.
Signed-off-by: Michael Drake <[email protected]>
---
sound/usb/stream.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 6a8f5843334e..956be9f7c72a 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -349,7 +349,7 @@ snd_pcm_chmap_elem *convert_chmap_v3(struct uac3_cluster_header_descriptor
* TODO: this conversion is not complete, update it
* after adding UAC3 values to asound.h
*/
- switch (is->bChPurpose) {
+ switch (is->bChRelationship) {
case UAC3_CH_MONO:
map = SNDRV_CHMAP_MONO;
break;
--
2.11.0
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 <[email protected]>
---
sound/usb/stream.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 956be9f7c72a..344e0ecf6d59 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -574,9 +574,11 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
return 0;
}
- if (protocol == UAC_VERSION_1) {
+ switch (protocol) {
+ case UAC_VERSION_1:
attributes = csep->bmAttributes;
- } else {
+ break;
+ case UAC_VERSION_2: {
struct uac2_iso_endpoint_descriptor *csep2 =
(struct uac2_iso_endpoint_descriptor *) csep;
@@ -585,6 +587,17 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
/* emulate the endpoint attributes of a v1 device */
if (csep2->bmControls & UAC2_CONTROL_PITCH)
attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
+ break;
+ }
+ case UAC_VERSION_3: {
+ struct uac3_iso_endpoint_descriptor *csep3 =
+ (struct uac3_iso_endpoint_descriptor *) csep;
+
+ /* emulate the endpoint attributes of a v1 device */
+ if (csep3->bmControls & UAC2_CONTROL_PITCH)
+ attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
+ break;
+ }
}
return attributes;
--
2.11.0
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 <[email protected]>
---
include/uapi/linux/usb/audio.h | 13 +++++++--
sound/usb/mixer.c | 64 ++++++++++++++++++++++++++++++++++++++++--
2 files changed, 71 insertions(+), 6 deletions(-)
diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
index 3a78e7145689..f9be472cd025 100644
--- a/include/uapi/linux/usb/audio.h
+++ b/include/uapi/linux/usb/audio.h
@@ -285,9 +285,16 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
int protocol)
{
- return (protocol == UAC_VERSION_1) ?
- &desc->baSourceID[desc->bNrInPins + 4] :
- &desc->baSourceID[desc->bNrInPins + 6];
+ switch (protocol) {
+ case UAC_VERSION_1:
+ return &desc->baSourceID[desc->bNrInPins + 4];
+ case UAC_VERSION_2:
+ return &desc->baSourceID[desc->bNrInPins + 6];
+ case UAC_VERSION_3:
+ return &desc->baSourceID[desc->bNrInPins + 2];
+ default:
+ return NULL;
+ }
}
static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 301ad61ed426..738ca37e6d6e 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -719,6 +719,35 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
}
/*
+ * Get logical cluster information for UAC3 devices.
+ */
+static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
+{
+ struct uac3_cluster_header_descriptor c_header;
+ int err;
+
+ err = snd_usb_ctl_msg(state->chip->dev,
+ usb_rcvctrlpipe(state->chip->dev, 0),
+ UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
+ USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+ cluster_id,
+ snd_usb_ctrl_intf(state->chip),
+ &c_header, sizeof(c_header));
+ if (err < 0)
+ goto error;
+ else if (err != sizeof(c_header)) {
+ err = -EIO;
+ goto error;
+ }
+
+ return c_header.bNrChannels;
+
+error:
+ usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
+ return err;
+}
+
+/*
* parse the source unit recursively until it reaches to a terminal
* or a branched unit.
*/
@@ -865,6 +894,19 @@ static int check_input_term(struct mixer_build *state, int id,
term->name = le16_to_cpu(d->wClockSourceStr);
return 0;
}
+ case UAC3_MIXER_UNIT: {
+ struct uac_mixer_unit_descriptor *d = p1;
+ unsigned int cluster_id = le16_to_cpu(d->baSourceID[d->bNrInPins]);
+
+ err = get_cluster_channels_v3(state, cluster_id);
+ if (err < 0)
+ return err;
+
+ term->channels = err;
+ term->type = d->bDescriptorSubtype << 16; /* virtual type */
+
+ return 0;
+ }
default:
return -ENODEV;
}
@@ -1801,7 +1843,7 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
struct usb_audio_term *iterm)
{
struct usb_mixer_elem_info *cval;
- unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
+ unsigned int num_outs;
unsigned int i, len;
struct snd_kcontrol *kctl;
const struct usbmix_name_map *map;
@@ -1814,6 +1856,14 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
if (!cval)
return;
+ if (state->mixer->protocol == UAC_VERSION_3) {
+ num_outs = get_cluster_channels_v3(state,
+ le16_to_cpu(desc->baSourceID[desc->bNrInPins]));
+ if (num_outs < 0)
+ return;
+ } else /* UAC_VERSION_1/2 */
+ num_outs = uac_mixer_unit_bNrChannels(desc);
+
snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid);
cval->control = in_ch + 1; /* based on 1 */
cval->val_type = USB_MIXER_S16;
@@ -1878,8 +1928,16 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
int input_pins, num_ins, num_outs;
int pin, ich, err;
- if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
- !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
+ if (state->mixer->protocol == UAC_VERSION_3) {
+ err = get_cluster_channels_v3(state,
+ le16_to_cpu(desc->baSourceID[desc->bNrInPins]));
+ if (err < 0)
+ return err;
+ num_outs = err;
+ } else /* UAC_VERSION_1/2 */
+ num_outs = uac_mixer_unit_bNrChannels(desc);
+
+ if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) || !num_outs) {
usb_audio_err(state->chip,
"invalid MIXER UNIT descriptor %d\n",
unitid);
--
2.11.0
Hi Jorge,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on sound/for-next]
[also build test WARNING on v4.17-rc1 next-20180420]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jorge-Sanjuan/ALSA-usb-UAC3-new-features/20180423-015726
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> sound/usb/stream.c:597:26: sparse: restricted __le32 degrades to integer
vim +597 sound/usb/stream.c
544
545 static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
546 struct usb_host_interface *alts,
547 int protocol, int iface_no)
548 {
549 /* parsed with a v1 header here. that's ok as we only look at the
550 * header first which is the same for both versions */
551 struct uac_iso_endpoint_descriptor *csep;
552 struct usb_interface_descriptor *altsd = get_iface_desc(alts);
553 int attributes = 0;
554
555 csep = snd_usb_find_desc(alts->endpoint[0].extra, alts->endpoint[0].extralen, NULL, USB_DT_CS_ENDPOINT);
556
557 /* Creamware Noah has this descriptor after the 2nd endpoint */
558 if (!csep && altsd->bNumEndpoints >= 2)
559 csep = snd_usb_find_desc(alts->endpoint[1].extra, alts->endpoint[1].extralen, NULL, USB_DT_CS_ENDPOINT);
560
561 /*
562 * If we can't locate the USB_DT_CS_ENDPOINT descriptor in the extra
563 * bytes after the first endpoint, go search the entire interface.
564 * Some devices have it directly *before* the standard endpoint.
565 */
566 if (!csep)
567 csep = snd_usb_find_desc(alts->extra, alts->extralen, NULL, USB_DT_CS_ENDPOINT);
568
569 if (!csep || csep->bLength < 7 ||
570 csep->bDescriptorSubtype != UAC_EP_GENERAL) {
571 usb_audio_warn(chip,
572 "%u:%d : no or invalid class specific endpoint descriptor\n",
573 iface_no, altsd->bAlternateSetting);
574 return 0;
575 }
576
577 switch (protocol) {
578 case UAC_VERSION_1:
579 attributes = csep->bmAttributes;
580 break;
581 case UAC_VERSION_2: {
582 struct uac2_iso_endpoint_descriptor *csep2 =
583 (struct uac2_iso_endpoint_descriptor *) csep;
584
585 attributes = csep->bmAttributes & UAC_EP_CS_ATTR_FILL_MAX;
586
587 /* emulate the endpoint attributes of a v1 device */
588 if (csep2->bmControls & UAC2_CONTROL_PITCH)
589 attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
590 break;
591 }
592 case UAC_VERSION_3: {
593 struct uac3_iso_endpoint_descriptor *csep3 =
594 (struct uac3_iso_endpoint_descriptor *) csep;
595
596 /* emulate the endpoint attributes of a v1 device */
> 597 if (csep3->bmControls & UAC2_CONTROL_PITCH)
598 attributes |= UAC_EP_CS_ATTR_PITCH_CONTROL;
599 break;
600 }
601 }
602
603 return attributes;
604 }
605
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Jorge,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on sound/for-next]
[also build test WARNING on v4.17-rc1 next-20180420]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jorge-Sanjuan/ALSA-usb-UAC3-new-features/20180423-015726
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
sound/usb/mixer.c:899:59: sparse: cast to restricted __le16
sound/usb/mixer.c:1923:33: sparse: cast to restricted __le16
>> sound/usb/mixer.c:1975:24: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [unsigned] bmctls @@ got restrunsigned int [unsigned] bmctls @@
sound/usb/mixer.c:1975:24: expected unsigned int [unsigned] bmctls
sound/usb/mixer.c:1975:24: got restricted __le16 [usertype] bmControls
sound/usb/mixer.c:1981:24: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [unsigned] bmctls @@ got restrunsigned int [unsigned] bmctls @@
sound/usb/mixer.c:1981:24: expected unsigned int [unsigned] bmctls
sound/usb/mixer.c:1981:24: got restricted __le32 [usertype] bmControls
sound/usb/mixer.c:2008:33: sparse: cast to restricted __le16
sound/usb/mixer.c:2697:62: sparse: incorrect type in argument 1 (different base types) @@ expected unsigned int [unsigned] [usertype] bmControls @@ got ed int [unsigned] [usertype] bmControls @@
sound/usb/mixer.c:2697:62: expected unsigned int [unsigned] [usertype] bmControls
sound/usb/mixer.c:2697:62: got restricted __le16 [usertype] bmControls
sound/usb/mixer.c:2724:62: sparse: incorrect type in argument 1 (different base types) @@ expected unsigned int [unsigned] [usertype] bmControls @@ got ed int [unsigned] [usertype] bmControls @@
sound/usb/mixer.c:2724:62: expected unsigned int [unsigned] [usertype] bmControls
sound/usb/mixer.c:2724:62: got restricted __le32 [usertype] bmControls
include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
include/linux/usb.h:1676:28: sparse: expression using sizeof(void)
vim +1975 sound/usb/mixer.c
1964
1965 static int parse_audio_input_terminal(struct mixer_build *state, int unitid,
1966 void *raw_desc)
1967 {
1968 struct usb_audio_term iterm;
1969 unsigned int control, bmctls, term_id;
1970
1971 if (state->mixer->protocol == UAC_VERSION_2) {
1972 struct uac2_input_terminal_descriptor *d_v2 = raw_desc;
1973 control = UAC2_TE_CONNECTOR;
1974 term_id = d_v2->bTerminalID;
> 1975 bmctls = d_v2->bmControls;
1976 }
1977 else if (state->mixer->protocol == UAC_VERSION_3) {
1978 struct uac3_input_terminal_descriptor *d_v3 = raw_desc;
1979 control = UAC3_TE_INSERTION;
1980 term_id = d_v3->bTerminalID;
1981 bmctls = d_v3->bmControls;
1982 }
1983 else /* UAC1. No Insertion control */
1984 return 0;
1985
1986 check_input_term(state, term_id, &iterm);
1987
1988 /* Check for jack detection. */
1989 if (uac_v2v3_control_is_readable(bmctls, control))
1990 build_connector_control(state, &iterm, true);
1991
1992 return 0;
1993 }
1994
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 <[email protected]>
> ---
> 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:25 +0200,
Jorge Sanjuan wrote:
>
> From: Michael Drake <[email protected]>
>
> The channel mapping is defined by bChRelationship, not bChPurpose.
>
> Signed-off-by: Michael Drake <[email protected]>
The change looks OK, but your sign-off is missing.
If you submit a patch, please give always your signed-off-by tag, no
matter whether it's your original patch or not.
Also, the Fixes tag would be helpful for such a correction.
thanks,
Takashi
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
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
>
On Fri, Apr 20, 2018 at 8:03 PM, Jorge Sanjuan
<[email protected]> wrote:
> From: Michael Drake <[email protected]>
>
> The channel mapping is defined by bChRelationship, not bChPurpose.
>
> Signed-off-by: Michael Drake <[email protected]>
> ---
> 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 <[email protected]>
> case UAC3_CH_MONO:
> map = SNDRV_CHMAP_MONO;
> break;
> --
> 2.11.0
>
> _______________________________________________
> Alsa-devel mailing list
> [email protected]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
From: Michael Drake <[email protected]>
The channel mapping is defined by bChRelationship, not bChPurpose.
Fixes: 9a2fe9b801f5 ("ALSA: usb: initial USB Audio Device Class 3.0
support")
Reviewed-by: Ruslan Bilovol <[email protected]>
Signed-off-by: Michael Drake <[email protected]>
Signed-off-by: Jorge Sanjuan <[email protected]>
---
sound/usb/stream.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 6a8f5843334e..956be9f7c72a 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -349,7 +349,7 @@ snd_pcm_chmap_elem *convert_chmap_v3(struct uac3_cluster_header_descriptor
* TODO: this conversion is not complete, update it
* after adding UAC3 values to asound.h
*/
- switch (is->bChPurpose) {
+ switch (is->bChRelationship) {
case UAC3_CH_MONO:
map = SNDRV_CHMAP_MONO;
break;
--
2.11.0
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 <[email protected]>
---
include/uapi/linux/usb/audio.h | 13 +++++--
sound/usb/mixer.c | 87 ++++++++++++++++++++++++++++++++++++++++--
2 files changed, 93 insertions(+), 7 deletions(-)
diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
index 3a78e7145689..f9be472cd025 100644
--- a/include/uapi/linux/usb/audio.h
+++ b/include/uapi/linux/usb/audio.h
@@ -285,9 +285,16 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
int protocol)
{
- return (protocol == UAC_VERSION_1) ?
- &desc->baSourceID[desc->bNrInPins + 4] :
- &desc->baSourceID[desc->bNrInPins + 6];
+ switch (protocol) {
+ case UAC_VERSION_1:
+ return &desc->baSourceID[desc->bNrInPins + 4];
+ case UAC_VERSION_2:
+ return &desc->baSourceID[desc->bNrInPins + 6];
+ case UAC_VERSION_3:
+ return &desc->baSourceID[desc->bNrInPins + 2];
+ default:
+ return NULL;
+ }
}
static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 301ad61ed426..bf701b466a4e 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
}
/*
+ * Get logical cluster information for UAC3 devices.
+ */
+static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
+{
+ struct uac3_cluster_header_descriptor c_header;
+ int err;
+
+ err = snd_usb_ctl_msg(state->chip->dev,
+ usb_rcvctrlpipe(state->chip->dev, 0),
+ UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
+ USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+ cluster_id,
+ snd_usb_ctrl_intf(state->chip),
+ &c_header, sizeof(c_header));
+ if (err < 0)
+ goto error;
+ if (err != sizeof(c_header)) {
+ err = -EIO;
+ goto error;
+ }
+
+ return c_header.bNrChannels;
+
+error:
+ usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
+ return err;
+}
+
+/*
+ * Get number of channels for a Mixer Unit.
+ */
+static int uac_mixer_unit_get_channels(struct mixer_build *state,
+ struct uac_mixer_unit_descriptor *desc)
+{
+ int mu_channels;
+
+ if (desc->bLength < 11)
+ return -EINVAL;
+ if (!desc->bNrInPins)
+ return -EINVAL;
+
+ switch (state->mixer->protocol) {
+ case UAC_VERSION_1:
+ case UAC_VERSION_2:
+ default:
+ mu_channels = uac_mixer_unit_bNrChannels(desc);
+ break;
+ case UAC_VERSION_3:
+ mu_channels = get_cluster_channels_v3(state,
+ le16_to_cpu(desc->baSourceID[desc->bNrInPins]));
+ break;
+ }
+
+ if (!mu_channels)
+ return -EINVAL;
+
+ return mu_channels;
+}
+
+/*
* parse the source unit recursively until it reaches to a terminal
* or a branched unit.
*/
@@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
term->name = le16_to_cpu(d->wClockSourceStr);
return 0;
}
+ case UAC3_MIXER_UNIT: {
+ struct uac_mixer_unit_descriptor *d = p1;
+
+ err = uac_mixer_unit_get_channels(state, d);
+ if (err < 0)
+ return err;
+
+ term->channels = err;
+ term->type = d->bDescriptorSubtype << 16; /* virtual type */
+
+ return 0;
+ }
default:
return -ENODEV;
}
@@ -1801,7 +1873,7 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
struct usb_audio_term *iterm)
{
struct usb_mixer_elem_info *cval;
- unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
+ unsigned int num_outs;
unsigned int i, len;
struct snd_kcontrol *kctl;
const struct usbmix_name_map *map;
@@ -1814,6 +1886,10 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
if (!cval)
return;
+ num_outs = uac_mixer_unit_get_channels(state, desc);
+ if (num_outs < 0)
+ return;
+
snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid);
cval->control = in_ch + 1; /* based on 1 */
cval->val_type = USB_MIXER_S16;
@@ -1878,14 +1954,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
int input_pins, num_ins, num_outs;
int pin, ich, err;
- if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
- !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
+ err = uac_mixer_unit_get_channels(state, desc);
+ if (err < 0) {
usb_audio_err(state->chip,
"invalid MIXER UNIT descriptor %d\n",
unitid);
- return -EINVAL;
+ return err;
}
+ num_outs = err;
+ input_pins = desc->bNrInPins;
+
num_ins = 0;
ich = 0;
for (pin = 0; pin < input_pins; pin++) {
--
2.11.0
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 <[email protected]>
---
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
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 <[email protected]>
---
include/linux/usb/audio-v2.h | 7 +++
include/linux/usb/audio-v3.h | 14 ++++++
sound/usb/mixer.c | 108 +++++++++++++++++++++++++++++++++++++------
3 files changed, 115 insertions(+), 14 deletions(-)
diff --git a/include/linux/usb/audio-v2.h b/include/linux/usb/audio-v2.h
index aaafecf073ff..a96ed2ce3254 100644
--- a/include/linux/usb/audio-v2.h
+++ b/include/linux/usb/audio-v2.h
@@ -189,6 +189,13 @@ struct uac2_iso_endpoint_descriptor {
#define UAC2_CONTROL_DATA_OVERRUN (3 << 2)
#define UAC2_CONTROL_DATA_UNDERRUN (3 << 4)
+/* 5.2.5.4.2 Connector Control Parameter Block */
+struct uac2_connectors_ctl_blk {
+ __u8 bNrChannels;
+ __le32 bmChannelConfig;
+ __u8 iChannelNames;
+} __attribute__((packed));
+
/* 6.1 Interrupt Data Message */
#define UAC2_INTERRUPT_DATA_MSG_VENDOR (1 << 0)
diff --git a/include/linux/usb/audio-v3.h b/include/linux/usb/audio-v3.h
index a8959aaba0ae..eb732f6569cb 100644
--- a/include/linux/usb/audio-v3.h
+++ b/include/linux/usb/audio-v3.h
@@ -221,6 +221,12 @@ struct uac3_iso_endpoint_descriptor {
__le16 wLockDelay;
} __attribute__((packed));
+/* 5.2.1.6.1 INSERTION CONTROL PARAMETER BLOCK */
+struct uac3_insertion_ctl_blk {
+ __u8 bSize;
+ __u8 bmConInserted;
+} __attribute__ ((packed));
+
/* 6.1 INTERRUPT DATA MESSAGE */
struct uac3_interrupt_data_msg {
__u8 bInfo;
@@ -392,4 +398,12 @@ struct uac3_interrupt_data_msg {
#define UAC3_AC_ACTIVE_INTERFACE_CONTROL 0x01
#define UAC3_AC_POWER_DOMAIN_CONTROL 0x02
+/* A.23.5 TERMINAL CONTROL SELECTORS */
+#define UAC3_TE_UNDEFINED 0x00
+#define UAC3_TE_INSERTION 0x01
+#define UAC3_TE_OVERLOAD 0x02
+#define UAC3_TE_UNDERFLOW 0x03
+#define UAC3_TE_OVERFLOW 0x04
+#define UAC3_TE_LATENCY 0x05
+
#endif /* __LINUX_USB_AUDIO_V3_H */
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index bf701b466a4e..9809d0a85894 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1322,6 +1322,51 @@ static int mixer_ctl_master_bool_get(struct snd_kcontrol *kcontrol,
return 0;
}
+/* get the connectors status and report it as boolean type */
+static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct usb_mixer_elem_info *cval = kcontrol->private_data;
+ struct snd_usb_audio *chip = cval->head.mixer->chip;
+ int idx = 0, validx, ret, val;
+
+ validx = cval->control << 8 | 0;
+
+ ret = snd_usb_lock_shutdown(chip) ? -EIO : 0;
+ if (ret)
+ goto error;
+
+ idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8);
+ if (cval->head.mixer->protocol == UAC_VERSION_2) {
+ struct uac2_connectors_ctl_blk uac2_conn;
+
+ ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), UAC2_CS_CUR,
+ USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+ validx, idx, &uac2_conn, sizeof(uac2_conn));
+ val = !!uac2_conn.bNrChannels;
+ } else { /* UAC_VERSION_3 */
+ struct uac3_insertion_ctl_blk uac3_conn;
+
+ ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), UAC2_CS_CUR,
+ USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+ validx, idx, &uac3_conn, sizeof(uac3_conn));
+ val = !!uac3_conn.bmConInserted;
+ }
+
+ snd_usb_unlock_shutdown(chip);
+
+ if (ret < 0) {
+error:
+ usb_audio_err(chip,
+ "cannot get connectors status: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n",
+ UAC_GET_CUR, validx, idx, cval->val_type);
+ return ret;
+ }
+
+ ucontrol->value.integer.value[0] = val;
+ return 0;
+}
+
static struct snd_kcontrol_new usb_feature_unit_ctl = {
.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
.name = "", /* will be filled later manually */
@@ -1352,6 +1397,15 @@ static struct snd_kcontrol_new usb_bool_master_control_ctl_ro = {
.put = NULL,
};
+static const struct snd_kcontrol_new usb_connector_ctl_ro = {
+ .iface = SNDRV_CTL_ELEM_IFACE_CARD,
+ .name = "", /* will be filled later manually */
+ .access = SNDRV_CTL_ELEM_ACCESS_READ,
+ .info = snd_ctl_boolean_mono_info,
+ .get = mixer_ctl_connector_get,
+ .put = NULL,
+};
+
/*
* This symbol is exported in order to allow the mixer quirks to
* hook up to the standard feature unit control mechanism
@@ -1598,17 +1652,25 @@ static void build_connector_control(struct mixer_build *state,
return;
snd_usb_mixer_elem_init_std(&cval->head, state->mixer, term->id);
/*
- * The first byte from reading the UAC2_TE_CONNECTOR control returns the
- * number of channels connected. This boolean ctl will simply report
- * if any channels are connected or not.
- * (Audio20_final.pdf Table 5-10: Connector Control CUR Parameter Block)
+ * UAC2: The first byte from reading the UAC2_TE_CONNECTOR control returns the
+ * number of channels connected.
+ *
+ * UAC3: The first byte specifies size of bitmap for the inserted controls. The
+ * following byte(s) specifies which connectors are inserted.
+ *
+ * This boolean ctl will simply report if any channels are connected
+ * or not.
*/
- cval->control = UAC2_TE_CONNECTOR;
+ if (state->mixer->protocol == UAC_VERSION_2)
+ cval->control = UAC2_TE_CONNECTOR;
+ else /* UAC_VERSION_3 */
+ cval->control = UAC3_TE_INSERTION;
+
cval->val_type = USB_MIXER_BOOLEAN;
cval->channels = 1; /* report true if any channel is connected */
cval->min = 0;
cval->max = 1;
- kctl = snd_ctl_new1(&usb_bool_master_control_ctl_ro, cval);
+ kctl = snd_ctl_new1(&usb_connector_ctl_ro, cval);
if (!kctl) {
usb_audio_err(state->chip, "cannot malloc kcontrol\n");
kfree(cval);
@@ -1930,16 +1992,28 @@ static int parse_audio_input_terminal(struct mixer_build *state, int unitid,
void *raw_desc)
{
struct usb_audio_term iterm;
- struct uac2_input_terminal_descriptor *d = raw_desc;
+ unsigned int control, bmctls, term_id;
- check_input_term(state, d->bTerminalID, &iterm);
if (state->mixer->protocol == UAC_VERSION_2) {
- /* Check for jack detection. */
- if (uac_v2v3_control_is_readable(d->bmControls,
- UAC2_TE_CONNECTOR)) {
- build_connector_control(state, &iterm, true);
- }
+ struct uac2_input_terminal_descriptor *d_v2 = raw_desc;
+ control = UAC2_TE_CONNECTOR;
+ term_id = d_v2->bTerminalID;
+ bmctls = le16_to_cpu(d_v2->bmControls);
+ } else if (state->mixer->protocol == UAC_VERSION_3) {
+ struct uac3_input_terminal_descriptor *d_v3 = raw_desc;
+ control = UAC3_TE_INSERTION;
+ term_id = d_v3->bTerminalID;
+ bmctls = le32_to_cpu(d_v3->bmControls);
+ } else {
+ return 0; /* UAC1. No Insertion control */
}
+
+ check_input_term(state, term_id, &iterm);
+
+ /* Check for jack detection. */
+ if (uac_v2v3_control_is_readable(bmctls, control))
+ build_connector_control(state, &iterm, true);
+
return 0;
}
@@ -2528,7 +2602,7 @@ static int parse_audio_unit(struct mixer_build *state, int unitid)
} else { /* UAC_VERSION_3 */
switch (p1[2]) {
case UAC_INPUT_TERMINAL:
- return 0; /* NOP */
+ return parse_audio_input_terminal(state, unitid, p1);
case UAC3_MIXER_UNIT:
return parse_audio_mixer_unit(state, unitid, p1);
case UAC3_CLOCK_SOURCE:
@@ -2666,6 +2740,12 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer)
err = parse_audio_unit(&state, desc->bCSourceID);
if (err < 0 && err != -EINVAL)
return err;
+
+ if (uac_v2v3_control_is_readable(le32_to_cpu(desc->bmControls),
+ UAC3_TE_INSERTION)) {
+ build_connector_control(&state, &state.oterm,
+ false);
+ }
}
}
--
2.11.0
v2 fixes:
- If/else statements braces style fixes.
- Add wrapping function to mixer unit code.
- Make connectors control kctl struct const.
- Little endian to cpu conversion in several places.
- Sing off and add Fixes tag to fixup commit.
- Remove flex-array for a struct that is used statically.
Now that the UAC3 patch [1] has made it to linux-next I have some extra
features to make a UAC3 device fully work in Linux. Including Jack
insertion control that I have put on top of this other patch [2] for
UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
to appear in most headset type devices.
UAC3 devices also require to have a Basic Audio Device (BADD) in a separate
config for which both Ruslan Bilovol and myself have submited different
approaches[3][4] but I don't know what the final merge will be. Once there
is official support for BADD, we'll need to test it with an actual UAC3
device to confirm it all wokrs.
All this features are tested with an actual UAC3 device that is still in
development. For this patch series, only the legacy config (#1. UAC1/UAC2)
and the UAC3 config have been tested. The BADD config is only tested using
and updated verison of [4].
[1]: https://patchwork.kernel.org/patch/10298179/
[2]: https://patchwork.kernel.org/patch/10305847/
[3]: https://patchwork.kernel.org/patch/10340851/
[4]: https://www.spinics.net/lists/alsa-devel/msg71617.html
Based on linux-next tag: next-20180420
Jorge Sanjuan (3):
ALSA: usb-audio: UAC3. Add support for mixer unit.
ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
ALSA: usb-audio: UAC3 Add support for connector insertion.
Michael Drake (1):
ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.
include/linux/usb/audio-v2.h | 7 ++
include/linux/usb/audio-v3.h | 14 +++
include/uapi/linux/usb/audio.h | 13 ++-
sound/usb/mixer.c | 195 +++++++++++++++++++++++++++++++++++++----
sound/usb/stream.c | 11 ++-
5 files changed, 217 insertions(+), 23 deletions(-)
--
2.11.0
On Tue, 24 Apr 2018 19:24:43 +0200,
Jorge Sanjuan wrote:
>
> From: Michael Drake <[email protected]>
>
> The channel mapping is defined by bChRelationship, not bChPurpose.
>
> Fixes: 9a2fe9b801f5 ("ALSA: usb: initial USB Audio Device Class 3.0
> support")
> Reviewed-by: Ruslan Bilovol <[email protected]>
> Signed-off-by: Michael Drake <[email protected]>
> Signed-off-by: Jorge Sanjuan <[email protected]>
Applied this to for-linus branch now, as it's a clear fix.
thanks,
Takashi
On Tue, 24 Apr 2018 19:24:41 +0200,
Jorge Sanjuan wrote:
>
> v2 fixes:
> - If/else statements braces style fixes.
> - Add wrapping function to mixer unit code.
> - Make connectors control kctl struct const.
> - Little endian to cpu conversion in several places.
> - Sing off and add Fixes tag to fixup commit.
> - Remove flex-array for a struct that is used statically.
>
> Now that the UAC3 patch [1] has made it to linux-next I have some extra
> features to make a UAC3 device fully work in Linux. Including Jack
> insertion control that I have put on top of this other patch [2] for
> UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
> to appear in most headset type devices.
These patches look reasonable, I'm OK to merge. But I'll wait for
Ruslan's comments (or at best with test results).
> UAC3 devices also require to have a Basic Audio Device (BADD) in a separate
> config for which both Ruslan Bilovol and myself have submited different
> approaches[3][4] but I don't know what the final merge will be. Once there
> is official support for BADD, we'll need to test it with an actual UAC3
> device to confirm it all wokrs.
Could you guys try to get agreement which approach should we take?
I have no big preference. Currently Ruslan's patch series look
easier, just because its addition is a bit smaller, though.
Thanks!
Takashi
> All this features are tested with an actual UAC3 device that is still in
> development. For this patch series, only the legacy config (#1. UAC1/UAC2)
> and the UAC3 config have been tested. The BADD config is only tested using
> and updated verison of [4].
>
> [1]: https://patchwork.kernel.org/patch/10298179/
> [2]: https://patchwork.kernel.org/patch/10305847/
> [3]: https://patchwork.kernel.org/patch/10340851/
> [4]: https://www.spinics.net/lists/alsa-devel/msg71617.html
>
> Based on linux-next tag: next-20180420
>
> Jorge Sanjuan (3):
> ALSA: usb-audio: UAC3. Add support for mixer unit.
> ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
> ALSA: usb-audio: UAC3 Add support for connector insertion.
>
> Michael Drake (1):
> ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.
>
> include/linux/usb/audio-v2.h | 7 ++
> include/linux/usb/audio-v3.h | 14 +++
> include/uapi/linux/usb/audio.h | 13 ++-
> sound/usb/mixer.c | 195 +++++++++++++++++++++++++++++++++++++----
> sound/usb/stream.c | 11 ++-
> 5 files changed, 217 insertions(+), 23 deletions(-)
>
> --
> 2.11.0
>
>
On Tue, Apr 24, 2018 at 8:24 PM, Jorge Sanjuan
<[email protected]> 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 <[email protected]>
> ---
> 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
> [email protected]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Apr 24, 2018 at 8:24 PM, Jorge Sanjuan
<[email protected]> 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 <[email protected]>
Reviewed-by: Ruslan Bilovol <[email protected]>
> ---
> 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
> [email protected]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Apr 24, 2018 at 9:02 PM, Takashi Iwai <[email protected]> wrote:
> On Tue, 24 Apr 2018 19:24:41 +0200,
> Jorge Sanjuan wrote:
>>
>> v2 fixes:
>> - If/else statements braces style fixes.
>> - Add wrapping function to mixer unit code.
>> - Make connectors control kctl struct const.
>> - Little endian to cpu conversion in several places.
>> - Sing off and add Fixes tag to fixup commit.
>> - Remove flex-array for a struct that is used statically.
>>
>> Now that the UAC3 patch [1] has made it to linux-next I have some extra
>> features to make a UAC3 device fully work in Linux. Including Jack
>> insertion control that I have put on top of this other patch [2] for
>> UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
>> to appear in most headset type devices.
Thanks for adding these improvements!
>
> These patches look reasonable, I'm OK to merge. But I'll wait for
> Ruslan's comments (or at best with test results).
I reviewed first 3 patches and will review jack detection patch later,
and I'm going to test this patchset in a next few days.
>
>> UAC3 devices also require to have a Basic Audio Device (BADD) in a separate
>> config for which both Ruslan Bilovol and myself have submited different
>> approaches[3][4] but I don't know what the final merge will be. Once there
>> is official support for BADD, we'll need to test it with an actual UAC3
>> device to confirm it all wokrs.
>
> Could you guys try to get agreement which approach should we take?
>
> I have no big preference. Currently Ruslan's patch series look
> easier, just because its addition is a bit smaller, though.
The BADD devices are quite simple, so direct initialization internal ALSA
structures looks easy and straightforward, comparing to generation of
missing descriptors.
I'm currently improving the patch series so it will look even more
smaller and easier, let's see how it goes
Thanks,
Ruslan
>
>
> Thanks!
>
> Takashi
>
>> All this features are tested with an actual UAC3 device that is still in
>> development. For this patch series, only the legacy config (#1. UAC1/UAC2)
>> and the UAC3 config have been tested. The BADD config is only tested using
>> and updated verison of [4].
>>
>> [1]: https://patchwork.kernel.org/patch/10298179/
>> [2]: https://patchwork.kernel.org/patch/10305847/
>> [3]: https://patchwork.kernel.org/patch/10340851/
>> [4]: https://www.spinics.net/lists/alsa-devel/msg71617.html
>>
>> Based on linux-next tag: next-20180420
>>
>> Jorge Sanjuan (3):
>> ALSA: usb-audio: UAC3. Add support for mixer unit.
>> ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
>> ALSA: usb-audio: UAC3 Add support for connector insertion.
>>
>> Michael Drake (1):
>> ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.
>>
>> include/linux/usb/audio-v2.h | 7 ++
>> include/linux/usb/audio-v3.h | 14 +++
>> include/uapi/linux/usb/audio.h | 13 ++-
>> sound/usb/mixer.c | 195 +++++++++++++++++++++++++++++++++++++----
>> sound/usb/stream.c | 11 ++-
>> 5 files changed, 217 insertions(+), 23 deletions(-)
>>
>> --
>> 2.11.0
>>
>>
> _______________________________________________
> Alsa-devel mailing list
> [email protected]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 25/04/18 23:35, Ruslan Bilovol wrote:
> On Tue, Apr 24, 2018 at 8:24 PM, Jorge Sanjuan
> <[email protected]> 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 <[email protected]>
>> ---
>> 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
>> [email protected]
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
On 26/04/18 10:26, Ruslan Bilovol wrote:
> On Tue, Apr 24, 2018 at 9:02 PM, Takashi Iwai <[email protected]> wrote:
>> On Tue, 24 Apr 2018 19:24:41 +0200,
>> Jorge Sanjuan wrote:
>>>
>>> v2 fixes:
>>> - If/else statements braces style fixes.
>>> - Add wrapping function to mixer unit code.
>>> - Make connectors control kctl struct const.
>>> - Little endian to cpu conversion in several places.
>>> - Sing off and add Fixes tag to fixup commit.
>>> - Remove flex-array for a struct that is used statically.
>>>
>>> Now that the UAC3 patch [1] has made it to linux-next I have some extra
>>> features to make a UAC3 device fully work in Linux. Including Jack
>>> insertion control that I have put on top of this other patch [2] for
>>> UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
>>> to appear in most headset type devices.
>
> Thanks for adding these improvements!
>
>>
>> These patches look reasonable, I'm OK to merge. But I'll wait for
>> Ruslan's comments (or at best with test results).
>
> I reviewed first 3 patches and will review jack detection patch later,
> and I'm going to test this patchset in a next few days.
>
>>
>>> UAC3 devices also require to have a Basic Audio Device (BADD) in a separate
>>> config for which both Ruslan Bilovol and myself have submited different
>>> approaches[3][4] but I don't know what the final merge will be. Once there
>>> is official support for BADD, we'll need to test it with an actual UAC3
>>> device to confirm it all wokrs.
>>
>> Could you guys try to get agreement which approach should we take?
>>
>> I have no big preference. Currently Ruslan's patch series look
>> easier, just because its addition is a bit smaller, though.
>
> The BADD devices are quite simple, so direct initialization internal ALSA
> structures looks easy and straightforward, comparing to generation of
> missing descriptors.
> I'm currently improving the patch series so it will look even more
> smaller and easier, let's see how it goes
Hi Ruslan,
I agree that the BADD devices may not require that much logic using all
the descriptors. Besides, what makes your approach more interesting to
me is the fact that there is no need to bypass the cluster descriptor
every single time if the UAC3 device operates in BADD mode.
I have not yet tested your patch with the UAC3 device that I have. I was
wondering whether that BADD mixer code will work with and AS interface
with several alt settings with different endpoints wMaxPacketSize. That
is something I am working on/testing for this device. I'd have to take a
closer look to the patch to provide some useful input on that.
Thanks,
Jorge
>
> Thanks,
> Ruslan
>
>>
>>
>> Thanks!
>>
>> Takashi
>>
>>> All this features are tested with an actual UAC3 device that is still in
>>> development. For this patch series, only the legacy config (#1. UAC1/UAC2)
>>> and the UAC3 config have been tested. The BADD config is only tested using
>>> and updated verison of [4].
>>>
>>> [1]: https://patchwork.kernel.org/patch/10298179/
>>> [2]: https://patchwork.kernel.org/patch/10305847/
>>> [3]: https://patchwork.kernel.org/patch/10340851/
>>> [4]: https://www.spinics.net/lists/alsa-devel/msg71617.html
>>>
>>> Based on linux-next tag: next-20180420
>>>
>>> Jorge Sanjuan (3):
>>> ALSA: usb-audio: UAC3. Add support for mixer unit.
>>> ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
>>> ALSA: usb-audio: UAC3 Add support for connector insertion.
>>>
>>> Michael Drake (1):
>>> ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3.
>>>
>>> include/linux/usb/audio-v2.h | 7 ++
>>> include/linux/usb/audio-v3.h | 14 +++
>>> include/uapi/linux/usb/audio.h | 13 ++-
>>> sound/usb/mixer.c | 195 +++++++++++++++++++++++++++++++++++++----
>>> sound/usb/stream.c | 11 ++-
>>> 5 files changed, 217 insertions(+), 23 deletions(-)
>>>
>>> --
>>> 2.11.0
>>>
>>>
>> _______________________________________________
>> Alsa-devel mailing list
>> [email protected]
>> 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 <[email protected]>
---
include/uapi/linux/usb/audio.h | 19 +++++++--
sound/usb/mixer.c | 88 ++++++++++++++++++++++++++++++++++++++----
2 files changed, 97 insertions(+), 10 deletions(-)
diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
index 3a78e7145689..13d98e6e0db1 100644
--- a/include/uapi/linux/usb/audio.h
+++ b/include/uapi/linux/usb/audio.h
@@ -285,9 +285,22 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
int protocol)
{
- return (protocol == UAC_VERSION_1) ?
- &desc->baSourceID[desc->bNrInPins + 4] :
- &desc->baSourceID[desc->bNrInPins + 6];
+ switch (protocol) {
+ case UAC_VERSION_1:
+ return &desc->baSourceID[desc->bNrInPins + 4];
+ case UAC_VERSION_2:
+ return &desc->baSourceID[desc->bNrInPins + 6];
+ case UAC_VERSION_3:
+ return &desc->baSourceID[desc->bNrInPins + 2];
+ default:
+ return NULL;
+ }
+}
+
+static inline __u16 uac3_mixer_unit_wClusterDescrID(struct uac_mixer_unit_descriptor *desc)
+{
+ return (desc->baSourceID[desc->bNrInPins + 1] << 8) |
+ desc->baSourceID[desc->bNrInPins];
}
static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 301ad61ed426..3503f4840ec3 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
}
/*
+ * Get logical cluster information for UAC3 devices.
+ */
+static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
+{
+ struct uac3_cluster_header_descriptor c_header;
+ int err;
+
+ err = snd_usb_ctl_msg(state->chip->dev,
+ usb_rcvctrlpipe(state->chip->dev, 0),
+ UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
+ USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+ cluster_id,
+ snd_usb_ctrl_intf(state->chip),
+ &c_header, sizeof(c_header));
+ if (err < 0)
+ goto error;
+ if (err != sizeof(c_header)) {
+ err = -EIO;
+ goto error;
+ }
+
+ return c_header.bNrChannels;
+
+error:
+ usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
+ return err;
+}
+
+/*
+ * Get number of channels for a Mixer Unit.
+ */
+static int uac_mixer_unit_get_channels(struct mixer_build *state,
+ struct uac_mixer_unit_descriptor *desc)
+{
+ int mu_channels;
+
+ if (desc->bLength < 11)
+ return -EINVAL;
+ if (!desc->bNrInPins)
+ return -EINVAL;
+
+ switch (state->mixer->protocol) {
+ case UAC_VERSION_1:
+ case UAC_VERSION_2:
+ default:
+ mu_channels = uac_mixer_unit_bNrChannels(desc);
+ break;
+ case UAC_VERSION_3:
+ mu_channels = get_cluster_channels_v3(state,
+ uac3_mixer_unit_wClusterDescrID(desc));
+ break;
+ }
+
+ if (!mu_channels)
+ return -EINVAL;
+
+ return mu_channels;
+}
+
+/*
* parse the source unit recursively until it reaches to a terminal
* or a branched unit.
*/
@@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
term->name = le16_to_cpu(d->wClockSourceStr);
return 0;
}
+ case UAC3_MIXER_UNIT: {
+ struct uac_mixer_unit_descriptor *d = p1;
+
+ err = uac_mixer_unit_get_channels(state, d);
+ if (err < 0)
+ return err;
+
+ term->channels = err;
+ term->type = d->bDescriptorSubtype << 16; /* virtual type */
+
+ return 0;
+ }
default:
return -ENODEV;
}
@@ -1797,11 +1869,10 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid,
*/
static void build_mixer_unit_ctl(struct mixer_build *state,
struct uac_mixer_unit_descriptor *desc,
- int in_pin, int in_ch, int unitid,
- struct usb_audio_term *iterm)
+ int in_pin, int in_ch, int num_outs,
+ int unitid, struct usb_audio_term *iterm)
{
struct usb_mixer_elem_info *cval;
- unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
unsigned int i, len;
struct snd_kcontrol *kctl;
const struct usbmix_name_map *map;
@@ -1878,14 +1949,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
int input_pins, num_ins, num_outs;
int pin, ich, err;
- if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
- !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
+ err = uac_mixer_unit_get_channels(state, desc);
+ if (err < 0) {
usb_audio_err(state->chip,
"invalid MIXER UNIT descriptor %d\n",
unitid);
- return -EINVAL;
+ return err;
}
+ num_outs = err;
+ input_pins = desc->bNrInPins;
+
num_ins = 0;
ich = 0;
for (pin = 0; pin < input_pins; pin++) {
@@ -1912,7 +1986,7 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
}
}
if (ich_has_controls)
- build_mixer_unit_ctl(state, desc, pin, ich,
+ build_mixer_unit_ctl(state, desc, pin, ich, num_outs,
unitid, &iterm);
}
}
--
2.11.0
On Fri, Apr 27, 2018 at 8:06 PM, Jorge Sanjuan
<[email protected]> 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 <[email protected]>
> ---
> 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 04/05/18 01:57, Ruslan Bilovol wrote:
> On Fri, Apr 27, 2018 at 8:06 PM, Jorge Sanjuan
> <[email protected]> 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 <[email protected]>
>> ---
>> include/uapi/linux/usb/audio.h | 19 +++++++--
>> sound/usb/mixer.c | 88 ++++++++++++++++++++++++++++++++++++++----
>> 2 files changed, 97 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
>> index 3a78e7145689..13d98e6e0db1 100644
>> --- a/include/uapi/linux/usb/audio.h
>> +++ b/include/uapi/linux/usb/audio.h
>> @@ -285,9 +285,22 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
>> static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
>> int protocol)
>> {
>> - return (protocol == UAC_VERSION_1) ?
>> - &desc->baSourceID[desc->bNrInPins + 4] :
>> - &desc->baSourceID[desc->bNrInPins + 6];
>> + switch (protocol) {
>> + case UAC_VERSION_1:
>> + return &desc->baSourceID[desc->bNrInPins + 4];
>> + case UAC_VERSION_2:
>> + return &desc->baSourceID[desc->bNrInPins + 6];
>> + case UAC_VERSION_3:
>> + return &desc->baSourceID[desc->bNrInPins + 2];
>> + default:
>> + return NULL;
>> + }
>> +}
>> +
>> +static inline __u16 uac3_mixer_unit_wClusterDescrID(struct uac_mixer_unit_descriptor *desc)
>> +{
>> + return (desc->baSourceID[desc->bNrInPins + 1] << 8) |
>> + desc->baSourceID[desc->bNrInPins];
>> }
>>
>> static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 301ad61ed426..3503f4840ec3 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
>> }
>>
>> /*
>> + * Get logical cluster information for UAC3 devices.
>> + */
>> +static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
>> +{
>> + struct uac3_cluster_header_descriptor c_header;
>> + int err;
>> +
>> + err = snd_usb_ctl_msg(state->chip->dev,
>> + usb_rcvctrlpipe(state->chip->dev, 0),
>> + UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
>> + USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
>> + cluster_id,
>> + snd_usb_ctrl_intf(state->chip),
>> + &c_header, sizeof(c_header));
>> + if (err < 0)
>> + goto error;
>> + if (err != sizeof(c_header)) {
>> + err = -EIO;
>> + goto error;
>> + }
>> +
>> + return c_header.bNrChannels;
>> +
>> +error:
>> + usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
>> + return err;
>> +}
>> +
>> +/*
>> + * Get number of channels for a Mixer Unit.
>> + */
>> +static int uac_mixer_unit_get_channels(struct mixer_build *state,
>> + struct uac_mixer_unit_descriptor *desc)
>> +{
>> + int mu_channels;
>> +
>> + if (desc->bLength < 11)
>> + return -EINVAL;
>> + if (!desc->bNrInPins)
>> + return -EINVAL;
>> +
>> + switch (state->mixer->protocol) {
>> + case UAC_VERSION_1:
>> + case UAC_VERSION_2:
>> + default:
>> + mu_channels = uac_mixer_unit_bNrChannels(desc);
>> + break;
>> + case UAC_VERSION_3:
>> + mu_channels = get_cluster_channels_v3(state,
>> + uac3_mixer_unit_wClusterDescrID(desc));
>> + break;
>> + }
>> +
>> + if (!mu_channels)
>> + return -EINVAL;
>> +
>> + return mu_channels;
>> +}
>> +
>> +/*
>> * parse the source unit recursively until it reaches to a terminal
>> * or a branched unit.
>> */
>> @@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
>> term->name = le16_to_cpu(d->wClockSourceStr);
>> return 0;
>> }
>> + case UAC3_MIXER_UNIT: {
>> + struct uac_mixer_unit_descriptor *d = p1;
>> +
>> + err = uac_mixer_unit_get_channels(state, d);
>> + if (err < 0)
>> + return err;
>> +
>> + term->channels = err;
>> + term->type = d->bDescriptorSubtype << 16; /* virtual type */
>> +
>> + return 0;
>> + }
>> default:
>> return -ENODEV;
>> }
>> @@ -1797,11 +1869,10 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid,
>> */
>> static void build_mixer_unit_ctl(struct mixer_build *state,
>> struct uac_mixer_unit_descriptor *desc,
>> - int in_pin, int in_ch, int unitid,
>> - struct usb_audio_term *iterm)
>> + int in_pin, int in_ch, int num_outs,
>> + int unitid, struct usb_audio_term *iterm)
>> {
>> struct usb_mixer_elem_info *cval;
>> - unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
>> unsigned int i, len;
>> struct snd_kcontrol *kctl;
>> const struct usbmix_name_map *map;
>> @@ -1878,14 +1949,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>> int input_pins, num_ins, num_outs;
>> int pin, ich, err;
>>
>> - if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
>> - !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
>> + err = uac_mixer_unit_get_channels(state, desc);
>> + if (err < 0) {
>> usb_audio_err(state->chip,
>> "invalid MIXER UNIT descriptor %d\n",
>> unitid);
>> - return -EINVAL;
>> + return err;
>> }
>>
>> + num_outs = err;
>> + input_pins = desc->bNrInPins;
>> +
>> num_ins = 0;
>> ich = 0;
>> for (pin = 0; pin < input_pins; pin++) {
>> @@ -1912,7 +1986,7 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>> }
>> }
>> if (ich_has_controls)
>> - build_mixer_unit_ctl(state, desc, pin, ich,
>> + build_mixer_unit_ctl(state, desc, pin, ich, num_outs,
>> unitid, &iterm);
>
> So with current sources we will never reach this place. In your
> usecase (mixer with no
> controls) it obviously won't go inside.
> However, I created a mixer with controls but still
> build_mixer_unit_ctl() isn't executed.
> This is because UAC3 input terminal parsing always returns 0 channels, this is
> what still needs to be implemented (see comment "REVISIT: UAC3 IT doesn't
> have channels/cfg" in check_input_term)
Thanks for testing this!
I missed that one with the number of input channels. Regarding the
"REVISIT: UAC3 IT doesn't have channels/cfg" comment, we can easily fix
that using the helper function I added in this patch for reading the
logical cluster's number of channels `get_cluster_channels_v3`. The
channel config bitmap I don't see it being used at all so parsing
iterm.channels should be enough. Any thoughts?
- Jorge
>
> Beside of that, other part of this patch seems to work as expected, at least
> uac_mixer_unit_get_channels() gives correct results - I checked it for
> two-channels config, that is different comparing to BADD.
>
> Thanks,
> Ruslan
>
On Tue, May 8, 2018 at 12:43 PM, Jorge <[email protected]> wrote:
>
>
> On 04/05/18 01:57, Ruslan Bilovol wrote:
>>
>> On Fri, Apr 27, 2018 at 8:06 PM, Jorge Sanjuan
>> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
v4 Updates:
- Removes already applied patch from v2 of this patchset.
- Adds small patch to parse Feature Unit number of channels.
- Rebased onto latest linux-next tag as today.
Now that the UAC3 patch [1] has made it to linux-next I have some extra
features to make a UAC3 devices fully work in Linux. Including Jack
insertion control that I have put on top of this other patch [2] for
UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
to appear in most headset type devices.
UAC3 devices also require to have a Basic Audio Device (BADD) in a separate
config for which both Ruslan Bilovol and myself have submited different
approaches[3][4]. After an ongoing discussion between Ruslan and myself
we have decided that the patch from Ruslan[3] implements a simpler and
yet more robust BADD driver.
All this features are tested with an actual UAC3 device that is still in
development. For this patch series, only the legacy config (#1. UAC1/UAC2)
and the UAC3 config have been tested. The BADD config will come in
a different patch from Ruslan.
[1]: https://patchwork.kernel.org/patch/10298179/
[2]: https://patchwork.kernel.org/patch/10305847/
[3]: https://patchwork.kernel.org/patch/10340851/
[4]: https://www.spinics.net/lists/alsa-devel/msg71617.html
Based on linux-next tag: next-20180510
Jorge Sanjuan (4):
ALSA: usb-audio: UAC3. Add support for mixer unit.
ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
ALSA: usb-audio: UAC3 Add support for connector insertion.
ALSA: usb-audio: UAC3: Parse Input Terminal number of channels.
include/linux/usb/audio-v2.h | 7 ++
include/linux/usb/audio-v3.h | 14 +++
include/uapi/linux/usb/audio.h | 19 +++-
sound/usb/mixer.c | 200 ++++++++++++++++++++++++++++++++++++-----
sound/usb/stream.c | 9 +-
5 files changed, 222 insertions(+), 27 deletions(-)
--
2.11.0
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 <[email protected]>
---
include/linux/usb/audio-v2.h | 7 +++
include/linux/usb/audio-v3.h | 14 ++++++
sound/usb/mixer.c | 108 +++++++++++++++++++++++++++++++++++++------
3 files changed, 115 insertions(+), 14 deletions(-)
diff --git a/include/linux/usb/audio-v2.h b/include/linux/usb/audio-v2.h
index 49699255cfd3..ba4b3e3327ff 100644
--- a/include/linux/usb/audio-v2.h
+++ b/include/linux/usb/audio-v2.h
@@ -189,6 +189,13 @@ struct uac2_iso_endpoint_descriptor {
#define UAC2_CONTROL_DATA_OVERRUN (3 << 2)
#define UAC2_CONTROL_DATA_UNDERRUN (3 << 4)
+/* 5.2.5.4.2 Connector Control Parameter Block */
+struct uac2_connectors_ctl_blk {
+ __u8 bNrChannels;
+ __le32 bmChannelConfig;
+ __u8 iChannelNames;
+} __attribute__((packed));
+
/* 6.1 Interrupt Data Message */
#define UAC2_INTERRUPT_DATA_MSG_VENDOR (1 << 0)
diff --git a/include/linux/usb/audio-v3.h b/include/linux/usb/audio-v3.h
index 38add1dedf2e..a710e28b5215 100644
--- a/include/linux/usb/audio-v3.h
+++ b/include/linux/usb/audio-v3.h
@@ -221,6 +221,12 @@ struct uac3_iso_endpoint_descriptor {
__le16 wLockDelay;
} __attribute__((packed));
+/* 5.2.1.6.1 INSERTION CONTROL PARAMETER BLOCK */
+struct uac3_insertion_ctl_blk {
+ __u8 bSize;
+ __u8 bmConInserted;
+} __attribute__ ((packed));
+
/* 6.1 INTERRUPT DATA MESSAGE */
struct uac3_interrupt_data_msg {
__u8 bInfo;
@@ -392,6 +398,14 @@ struct uac3_interrupt_data_msg {
#define UAC3_AC_ACTIVE_INTERFACE_CONTROL 0x01
#define UAC3_AC_POWER_DOMAIN_CONTROL 0x02
+/* A.23.5 TERMINAL CONTROL SELECTORS */
+#define UAC3_TE_UNDEFINED 0x00
+#define UAC3_TE_INSERTION 0x01
+#define UAC3_TE_OVERLOAD 0x02
+#define UAC3_TE_UNDERFLOW 0x03
+#define UAC3_TE_OVERFLOW 0x04
+#define UAC3_TE_LATENCY 0x05
+
/* BADD predefined Unit/Terminal values */
#define UAC3_BADD_IT_ID1 1 /* Input Terminal ID1: bTerminalID = 1 */
#define UAC3_BADD_FU_ID2 2 /* Feature Unit ID2: bUnitID = 2 */
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 129c1397f0cb..431f3c319839 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1322,6 +1322,51 @@ static int mixer_ctl_master_bool_get(struct snd_kcontrol *kcontrol,
return 0;
}
+/* get the connectors status and report it as boolean type */
+static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct usb_mixer_elem_info *cval = kcontrol->private_data;
+ struct snd_usb_audio *chip = cval->head.mixer->chip;
+ int idx = 0, validx, ret, val;
+
+ validx = cval->control << 8 | 0;
+
+ ret = snd_usb_lock_shutdown(chip) ? -EIO : 0;
+ if (ret)
+ goto error;
+
+ idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8);
+ if (cval->head.mixer->protocol == UAC_VERSION_2) {
+ struct uac2_connectors_ctl_blk uac2_conn;
+
+ ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), UAC2_CS_CUR,
+ USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+ validx, idx, &uac2_conn, sizeof(uac2_conn));
+ val = !!uac2_conn.bNrChannels;
+ } else { /* UAC_VERSION_3 */
+ struct uac3_insertion_ctl_blk uac3_conn;
+
+ ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), UAC2_CS_CUR,
+ USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+ validx, idx, &uac3_conn, sizeof(uac3_conn));
+ val = !!uac3_conn.bmConInserted;
+ }
+
+ snd_usb_unlock_shutdown(chip);
+
+ if (ret < 0) {
+error:
+ usb_audio_err(chip,
+ "cannot get connectors status: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n",
+ UAC_GET_CUR, validx, idx, cval->val_type);
+ return ret;
+ }
+
+ ucontrol->value.integer.value[0] = val;
+ return 0;
+}
+
static struct snd_kcontrol_new usb_feature_unit_ctl = {
.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
.name = "", /* will be filled later manually */
@@ -1352,6 +1397,15 @@ static struct snd_kcontrol_new usb_bool_master_control_ctl_ro = {
.put = NULL,
};
+static const struct snd_kcontrol_new usb_connector_ctl_ro = {
+ .iface = SNDRV_CTL_ELEM_IFACE_CARD,
+ .name = "", /* will be filled later manually */
+ .access = SNDRV_CTL_ELEM_ACCESS_READ,
+ .info = snd_ctl_boolean_mono_info,
+ .get = mixer_ctl_connector_get,
+ .put = NULL,
+};
+
/*
* This symbol is exported in order to allow the mixer quirks to
* hook up to the standard feature unit control mechanism
@@ -1598,17 +1652,25 @@ static void build_connector_control(struct mixer_build *state,
return;
snd_usb_mixer_elem_init_std(&cval->head, state->mixer, term->id);
/*
- * The first byte from reading the UAC2_TE_CONNECTOR control returns the
- * number of channels connected. This boolean ctl will simply report
- * if any channels are connected or not.
- * (Audio20_final.pdf Table 5-10: Connector Control CUR Parameter Block)
+ * UAC2: The first byte from reading the UAC2_TE_CONNECTOR control returns the
+ * number of channels connected.
+ *
+ * UAC3: The first byte specifies size of bitmap for the inserted controls. The
+ * following byte(s) specifies which connectors are inserted.
+ *
+ * This boolean ctl will simply report if any channels are connected
+ * or not.
*/
- cval->control = UAC2_TE_CONNECTOR;
+ if (state->mixer->protocol == UAC_VERSION_2)
+ cval->control = UAC2_TE_CONNECTOR;
+ else /* UAC_VERSION_3 */
+ cval->control = UAC3_TE_INSERTION;
+
cval->val_type = USB_MIXER_BOOLEAN;
cval->channels = 1; /* report true if any channel is connected */
cval->min = 0;
cval->max = 1;
- kctl = snd_ctl_new1(&usb_bool_master_control_ctl_ro, cval);
+ kctl = snd_ctl_new1(&usb_connector_ctl_ro, cval);
if (!kctl) {
usb_audio_err(state->chip, "cannot malloc kcontrol\n");
kfree(cval);
@@ -1926,16 +1988,28 @@ static int parse_audio_input_terminal(struct mixer_build *state, int unitid,
void *raw_desc)
{
struct usb_audio_term iterm;
- struct uac2_input_terminal_descriptor *d = raw_desc;
+ unsigned int control, bmctls, term_id;
- check_input_term(state, d->bTerminalID, &iterm);
if (state->mixer->protocol == UAC_VERSION_2) {
- /* Check for jack detection. */
- if (uac_v2v3_control_is_readable(le16_to_cpu(d->bmControls),
- UAC2_TE_CONNECTOR)) {
- build_connector_control(state, &iterm, true);
- }
+ struct uac2_input_terminal_descriptor *d_v2 = raw_desc;
+ control = UAC2_TE_CONNECTOR;
+ term_id = d_v2->bTerminalID;
+ bmctls = le16_to_cpu(d_v2->bmControls);
+ } else if (state->mixer->protocol == UAC_VERSION_3) {
+ struct uac3_input_terminal_descriptor *d_v3 = raw_desc;
+ control = UAC3_TE_INSERTION;
+ term_id = d_v3->bTerminalID;
+ bmctls = le32_to_cpu(d_v3->bmControls);
+ } else {
+ return 0; /* UAC1. No Insertion control */
}
+
+ check_input_term(state, term_id, &iterm);
+
+ /* Check for jack detection. */
+ if (uac_v2v3_control_is_readable(bmctls, control))
+ build_connector_control(state, &iterm, true);
+
return 0;
}
@@ -2526,7 +2600,7 @@ static int parse_audio_unit(struct mixer_build *state, int unitid)
} else { /* UAC_VERSION_3 */
switch (p1[2]) {
case UAC_INPUT_TERMINAL:
- return 0; /* NOP */
+ return parse_audio_input_terminal(state, unitid, p1);
case UAC3_MIXER_UNIT:
return parse_audio_mixer_unit(state, unitid, p1);
case UAC3_CLOCK_SOURCE:
@@ -2664,6 +2738,12 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer)
err = parse_audio_unit(&state, desc->bCSourceID);
if (err < 0 && err != -EINVAL)
return err;
+
+ if (uac_v2v3_control_is_readable(le32_to_cpu(desc->bmControls),
+ UAC3_TE_INSERTION)) {
+ build_connector_control(&state, &state.oterm,
+ false);
+ }
}
}
--
2.11.0
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 <[email protected]>
---
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
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 <[email protected]>
---
sound/usb/mixer.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 431f3c319839..19b25fbc7437 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -903,9 +903,9 @@ static int check_input_term(struct mixer_build *state, int id,
* recursion calls */
term->id = id;
term->type = le16_to_cpu(d->wTerminalType);
+ term->channels = get_cluster_channels_v3(state, d->wClusterDescrID);
- /* REVISIT: UAC3 IT doesn't have channels/cfg */
- term->channels = 0;
+ /* REVISIT: UAC3 IT doesn't have channels cfg */
term->chconfig = 0;
term->name = le16_to_cpu(d->wTerminalDescrStr);
--
2.11.0
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 <[email protected]>
---
include/uapi/linux/usb/audio.h | 19 +++++++--
sound/usb/mixer.c | 88 ++++++++++++++++++++++++++++++++++++++----
2 files changed, 97 insertions(+), 10 deletions(-)
diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
index 3a78e7145689..13d98e6e0db1 100644
--- a/include/uapi/linux/usb/audio.h
+++ b/include/uapi/linux/usb/audio.h
@@ -285,9 +285,22 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
int protocol)
{
- return (protocol == UAC_VERSION_1) ?
- &desc->baSourceID[desc->bNrInPins + 4] :
- &desc->baSourceID[desc->bNrInPins + 6];
+ switch (protocol) {
+ case UAC_VERSION_1:
+ return &desc->baSourceID[desc->bNrInPins + 4];
+ case UAC_VERSION_2:
+ return &desc->baSourceID[desc->bNrInPins + 6];
+ case UAC_VERSION_3:
+ return &desc->baSourceID[desc->bNrInPins + 2];
+ default:
+ return NULL;
+ }
+}
+
+static inline __u16 uac3_mixer_unit_wClusterDescrID(struct uac_mixer_unit_descriptor *desc)
+{
+ return (desc->baSourceID[desc->bNrInPins + 1] << 8) |
+ desc->baSourceID[desc->bNrInPins];
}
static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 76417943ff85..129c1397f0cb 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -719,6 +719,66 @@ static int get_term_name(struct snd_usb_audio *chip, struct usb_audio_term *iter
}
/*
+ * Get logical cluster information for UAC3 devices.
+ */
+static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
+{
+ struct uac3_cluster_header_descriptor c_header;
+ int err;
+
+ err = snd_usb_ctl_msg(state->chip->dev,
+ usb_rcvctrlpipe(state->chip->dev, 0),
+ UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
+ USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
+ cluster_id,
+ snd_usb_ctrl_intf(state->chip),
+ &c_header, sizeof(c_header));
+ if (err < 0)
+ goto error;
+ if (err != sizeof(c_header)) {
+ err = -EIO;
+ goto error;
+ }
+
+ return c_header.bNrChannels;
+
+error:
+ usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
+ return err;
+}
+
+/*
+ * Get number of channels for a Mixer Unit.
+ */
+static int uac_mixer_unit_get_channels(struct mixer_build *state,
+ struct uac_mixer_unit_descriptor *desc)
+{
+ int mu_channels;
+
+ if (desc->bLength < 11)
+ return -EINVAL;
+ if (!desc->bNrInPins)
+ return -EINVAL;
+
+ switch (state->mixer->protocol) {
+ case UAC_VERSION_1:
+ case UAC_VERSION_2:
+ default:
+ mu_channels = uac_mixer_unit_bNrChannels(desc);
+ break;
+ case UAC_VERSION_3:
+ mu_channels = get_cluster_channels_v3(state,
+ uac3_mixer_unit_wClusterDescrID(desc));
+ break;
+ }
+
+ if (!mu_channels)
+ return -EINVAL;
+
+ return mu_channels;
+}
+
+/*
* parse the source unit recursively until it reaches to a terminal
* or a branched unit.
*/
@@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
term->name = le16_to_cpu(d->wClockSourceStr);
return 0;
}
+ case UAC3_MIXER_UNIT: {
+ struct uac_mixer_unit_descriptor *d = p1;
+
+ err = uac_mixer_unit_get_channels(state, d);
+ if (err < 0)
+ return err;
+
+ term->channels = err;
+ term->type = d->bDescriptorSubtype << 16; /* virtual type */
+
+ return 0;
+ }
default:
return -ENODEV;
}
@@ -1798,11 +1870,10 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid,
*/
static void build_mixer_unit_ctl(struct mixer_build *state,
struct uac_mixer_unit_descriptor *desc,
- int in_pin, int in_ch, int unitid,
- struct usb_audio_term *iterm)
+ int in_pin, int in_ch, int num_outs,
+ int unitid, struct usb_audio_term *iterm)
{
struct usb_mixer_elem_info *cval;
- unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
unsigned int i, len;
struct snd_kcontrol *kctl;
const struct usbmix_name_map *map;
@@ -1879,14 +1950,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
int input_pins, num_ins, num_outs;
int pin, ich, err;
- if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
- !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
+ err = uac_mixer_unit_get_channels(state, desc);
+ if (err < 0) {
usb_audio_err(state->chip,
"invalid MIXER UNIT descriptor %d\n",
unitid);
- return -EINVAL;
+ return err;
}
+ num_outs = err;
+ input_pins = desc->bNrInPins;
+
num_ins = 0;
ich = 0;
for (pin = 0; pin < input_pins; pin++) {
@@ -1913,7 +1987,7 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
}
}
if (ich_has_controls)
- build_mixer_unit_ctl(state, desc, pin, ich,
+ build_mixer_unit_ctl(state, desc, pin, ich, num_outs,
unitid, &iterm);
}
}
--
2.11.0
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 <[email protected]>
> ---
> 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);
>
On Mon, May 14, 2018 at 11:54 AM, Jorge <[email protected]> wrote:
>
>
> On 11/05/18 16:25, Jorge Sanjuan wrote:
>>
>> Obtain the number of channels for the Input Terminal from the
>> Logical Cluster Descriptor. This achieves a useful minimal parsing
>> of this unit so it can be used in other units in the topology.
>>
>> Signed-off-by: Jorge Sanjuan <[email protected]>
>> ---
>> sound/usb/mixer.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 431f3c319839..19b25fbc7437 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -903,9 +903,9 @@ static int check_input_term(struct mixer_build *state,
>> int id,
>> * recursion calls */
>> term->id = id;
>> term->type =
>> le16_to_cpu(d->wTerminalType);
>> + term->channels =
>> get_cluster_channels_v3(state, d->wClusterDescrID);
>
>
>
> Sorry about this. I just spotted that I should have used the helper function
> I added to access d->wClusterDescrID `uac3_mixer_unit_wClusterDescrID`.
>
> I got the sparse warning for the endianess and realized that. I'll resend
> this one patch.
While here, please add checking output of get_cluster_channels_v3() as
it can return negative errno.
BTW, I've just tested your Mixer patches and this is the only comment I have
so far.
Thanks,
Ruslan
>
>> - /* REVISIT: UAC3 IT doesn't have
>> channels/cfg */
>> - term->channels = 0;
>> + /* REVISIT: UAC3 IT doesn't have channels
>> cfg */
>> term->chconfig = 0;
>> term->name =
>> le16_to_cpu(d->wTerminalDescrStr);
>>
>
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 <[email protected]>
---
sound/usb/mixer.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 431f3c319839..99804cd4aed6 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -904,8 +904,12 @@ static int check_input_term(struct mixer_build *state, int id,
term->id = id;
term->type = le16_to_cpu(d->wTerminalType);
- /* REVISIT: UAC3 IT doesn't have channels/cfg */
- term->channels = 0;
+ err = get_cluster_channels_v3(state, le16_to_cpu(d->wClusterDescrID));
+ if (err < 0)
+ return err;
+ term->channels = err;
+
+ /* REVISIT: UAC3 IT doesn't have channels cfg */
term->chconfig = 0;
term->name = le16_to_cpu(d->wTerminalDescrStr);
--
2.11.0
On Fri, May 11, 2018 at 6:25 PM, Jorge Sanjuan
<[email protected]> wrote:
> This adds support for the MIXER UNIT in UAC3. All the information
> is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
> read the rest of the logical cluster to obtain the channel config
> as that wont make any difference in the current mixer behaviour.
>
> The name of the mixer unit is not yet requested as there is not
> support for the UAC3 Class Specific String requests.
>
> Tested in an UAC3 device working as a HEADSET with a basic mixer
> unit (same as the one in the BADD spec) with no controls.
I tested this patch in a similar use-case (with a simple mixer unit),
but _with_ controls and along with patch [1] from this series, which
added parsing input terminal's channels.
So everything works fine, I see all needed requests handling and
mixer unit creation on ALSA side, which I can use now.
So, as a bottom line:
Reviewed-by: Ruslan Bilovol <[email protected]>
Tested-by: Ruslan Bilovol <[email protected]>
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-May/136030.html
>
> Signed-off-by: Jorge Sanjuan <[email protected]>
> ---
> include/uapi/linux/usb/audio.h | 19 +++++++--
> sound/usb/mixer.c | 88 ++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 97 insertions(+), 10 deletions(-)
>
> diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
> index 3a78e7145689..13d98e6e0db1 100644
> --- a/include/uapi/linux/usb/audio.h
> +++ b/include/uapi/linux/usb/audio.h
> @@ -285,9 +285,22 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
> static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
> int protocol)
> {
> - return (protocol == UAC_VERSION_1) ?
> - &desc->baSourceID[desc->bNrInPins + 4] :
> - &desc->baSourceID[desc->bNrInPins + 6];
> + switch (protocol) {
> + case UAC_VERSION_1:
> + return &desc->baSourceID[desc->bNrInPins + 4];
> + case UAC_VERSION_2:
> + return &desc->baSourceID[desc->bNrInPins + 6];
> + case UAC_VERSION_3:
> + return &desc->baSourceID[desc->bNrInPins + 2];
> + default:
> + return NULL;
> + }
> +}
> +
> +static inline __u16 uac3_mixer_unit_wClusterDescrID(struct uac_mixer_unit_descriptor *desc)
> +{
> + return (desc->baSourceID[desc->bNrInPins + 1] << 8) |
> + desc->baSourceID[desc->bNrInPins];
> }
>
> static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 76417943ff85..129c1397f0cb 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -719,6 +719,66 @@ static int get_term_name(struct snd_usb_audio *chip, struct usb_audio_term *iter
> }
>
> /*
> + * Get logical cluster information for UAC3 devices.
> + */
> +static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
> +{
> + struct uac3_cluster_header_descriptor c_header;
> + int err;
> +
> + err = snd_usb_ctl_msg(state->chip->dev,
> + usb_rcvctrlpipe(state->chip->dev, 0),
> + UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
> + USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
> + cluster_id,
> + snd_usb_ctrl_intf(state->chip),
> + &c_header, sizeof(c_header));
> + if (err < 0)
> + goto error;
> + if (err != sizeof(c_header)) {
> + err = -EIO;
> + goto error;
> + }
> +
> + return c_header.bNrChannels;
> +
> +error:
> + usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
> + return err;
> +}
> +
> +/*
> + * Get number of channels for a Mixer Unit.
> + */
> +static int uac_mixer_unit_get_channels(struct mixer_build *state,
> + struct uac_mixer_unit_descriptor *desc)
> +{
> + int mu_channels;
> +
> + if (desc->bLength < 11)
> + return -EINVAL;
> + if (!desc->bNrInPins)
> + return -EINVAL;
> +
> + switch (state->mixer->protocol) {
> + case UAC_VERSION_1:
> + case UAC_VERSION_2:
> + default:
> + mu_channels = uac_mixer_unit_bNrChannels(desc);
> + break;
> + case UAC_VERSION_3:
> + mu_channels = get_cluster_channels_v3(state,
> + uac3_mixer_unit_wClusterDescrID(desc));
> + break;
> + }
> +
> + if (!mu_channels)
> + return -EINVAL;
> +
> + return mu_channels;
> +}
> +
> +/*
> * parse the source unit recursively until it reaches to a terminal
> * or a branched unit.
> */
> @@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
> term->name = le16_to_cpu(d->wClockSourceStr);
> return 0;
> }
> + case UAC3_MIXER_UNIT: {
> + struct uac_mixer_unit_descriptor *d = p1;
> +
> + err = uac_mixer_unit_get_channels(state, d);
> + if (err < 0)
> + return err;
> +
> + term->channels = err;
> + term->type = d->bDescriptorSubtype << 16; /* virtual type */
> +
> + return 0;
> + }
> default:
> return -ENODEV;
> }
> @@ -1798,11 +1870,10 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid,
> */
> static void build_mixer_unit_ctl(struct mixer_build *state,
> struct uac_mixer_unit_descriptor *desc,
> - int in_pin, int in_ch, int unitid,
> - struct usb_audio_term *iterm)
> + int in_pin, int in_ch, int num_outs,
> + int unitid, struct usb_audio_term *iterm)
> {
> struct usb_mixer_elem_info *cval;
> - unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
> unsigned int i, len;
> struct snd_kcontrol *kctl;
> const struct usbmix_name_map *map;
> @@ -1879,14 +1950,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
> int input_pins, num_ins, num_outs;
> int pin, ich, err;
>
> - if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
> - !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
> + err = uac_mixer_unit_get_channels(state, desc);
> + if (err < 0) {
> usb_audio_err(state->chip,
> "invalid MIXER UNIT descriptor %d\n",
> unitid);
> - return -EINVAL;
> + return err;
> }
>
> + num_outs = err;
> + input_pins = desc->bNrInPins;
> +
> num_ins = 0;
> ich = 0;
> for (pin = 0; pin < input_pins; pin++) {
> @@ -1913,7 +1987,7 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
> }
> }
> if (ich_has_controls)
> - build_mixer_unit_ctl(state, desc, pin, ich,
> + build_mixer_unit_ctl(state, desc, pin, ich, num_outs,
> unitid, &iterm);
> }
> }
> --
> 2.11.0
>
--
Best regards,
Ruslan Bilovol
On Fri, May 11, 2018 at 6:25 PM, Jorge Sanjuan
<[email protected]> 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 <[email protected]>
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 <[email protected]>
> ---
> 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
>
On Mon, May 14, 2018 at 2:03 PM, Jorge Sanjuan
<[email protected]> wrote:
> Obtain the number of channels for the Input Terminal from the
> Logical Cluster Descriptor. This achieves a useful minimal parsing
> of this unit so it can be used in other units in the topology.
Usually 'patch resend' means resend without any changes, and if there
are updates in the patch - it's a new version.
By the way, as I already said in comments to patch 1/4 [1], I verified
this patch successfully.
Reviewed-by: Ruslan Bilovol <[email protected]>
Tested-by: Ruslan Bilovol <[email protected]>
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-May/136044.html
>
> Signed-off-by: Jorge Sanjuan <[email protected]>
> ---
> sound/usb/mixer.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 431f3c319839..99804cd4aed6 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -904,8 +904,12 @@ static int check_input_term(struct mixer_build *state, int id,
> term->id = id;
> term->type = le16_to_cpu(d->wTerminalType);
>
> - /* REVISIT: UAC3 IT doesn't have channels/cfg */
> - term->channels = 0;
> + err = get_cluster_channels_v3(state, le16_to_cpu(d->wClusterDescrID));
> + if (err < 0)
> + return err;
> + term->channels = err;
> +
> + /* REVISIT: UAC3 IT doesn't have channels cfg */
> term->chconfig = 0;
>
> term->name = le16_to_cpu(d->wTerminalDescrStr);
> --
> 2.11.0
>
On Fri, 11 May 2018 17:25:33 +0200,
Jorge Sanjuan wrote:
>
> v4 Updates:
> - Removes already applied patch from v2 of this patchset.
> - Adds small patch to parse Feature Unit number of channels.
> - Rebased onto latest linux-next tag as today.
>
> Now that the UAC3 patch [1] has made it to linux-next I have some extra
> features to make a UAC3 devices fully work in Linux. Including Jack
> insertion control that I have put on top of this other patch [2] for
> UAC2. Also adding support for the UAC3 Mixer Unit which is most likely
> to appear in most headset type devices.
>
> UAC3 devices also require to have a Basic Audio Device (BADD) in a separate
> config for which both Ruslan Bilovol and myself have submited different
> approaches[3][4]. After an ongoing discussion between Ruslan and myself
> we have decided that the patch from Ruslan[3] implements a simpler and
> yet more robust BADD driver.
>
> All this features are tested with an actual UAC3 device that is still in
> development. For this patch series, only the legacy config (#1. UAC1/UAC2)
> and the UAC3 config have been tested. The BADD config will come in
> a different patch from Ruslan.
>
> [1]: https://patchwork.kernel.org/patch/10298179/
> [2]: https://patchwork.kernel.org/patch/10305847/
> [3]: https://patchwork.kernel.org/patch/10340851/
> [4]: https://www.spinics.net/lists/alsa-devel/msg71617.html
>
> Based on linux-next tag: next-20180510
>
> Jorge Sanjuan (4):
> ALSA: usb-audio: UAC3. Add support for mixer unit.
> ALSA: usb-audio: Use Class Specific EP for UAC3 devices.
> ALSA: usb-audio: UAC3 Add support for connector insertion.
> ALSA: usb-audio: UAC3: Parse Input Terminal number of channels.
OK, now I applied all four patches. The patch 2 was queued to
for-linus branch while others to for-next. The patch 4 was taken from
the revised v4.
Thanks!
Takashi