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