From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: AIpwx4/bnfx4baPwTC35ka0qXi08qJk2VnKuI1EDEvTMtAH+AmEVVQc5P+EmeGGh/isFMLnWikem ARC-Seal: i=1; a=rsa-sha256; t=1524481383; cv=none; d=google.com; s=arc-20160816; b=dBmlpbEad1bC5m2Z0GtnLsX7I4YAWyQJzf3FUO2I787PsYa+0kj9zKo5jqBow2iBOq DxgywkY9Xsu/Ug8om32vkRPTw0FB7wK8HYm/JeNsDAcB6x2mka1eSsM2ngSmBtuDHcTG 2Ir2ZZsIxMb7bK2MQYANcXFjkfyIdZcHq8A5RkKfSAXbOtWnQ8J8yP2jXMpECXjST6iE +TlyAIClHQtEsXA0yJakurhumsnh/w6rVmP+zZVSLPQyztAvziUXHFvT1why6aPW5Khu vkcRyeYc85vXZSBwGLnyiP/ePEHtYCs5emKtiM3t3H64rQg1xnNV7WyLTWKlLFeonh38 129A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=mime-version:user-agent:references:in-reply-to:subject:cc:to:from :message-id:date:arc-authentication-results; bh=VuUBdsYoL0gTikwcECgBTP+bI9R7haaaG7KXiRkemPE=; b=eRCiOb7ITq+Rjpe4md0CsYwQKhvAQK1Y0b5YS3kWXrXpXgMR0mATJCwNTVPpikIBTO P1TGeM/DWYm3G6RVA5UkqF0bbvVOWvki9ADuy436X/5SegBYTkCAct+tFmvJ1aXudShE uWTrPiViZSIdT/EPFV+HblN1lsL2XhrYQ361gcUNsUbrp0CdblB6J08pmn5OVyW/mdB1 ZeibfOrkg+m36SfueO7PzH5J6YdTo8KXj/7Iyq/EEtaS3ocRgKj/6lk7Xn8TN7ROk/hG +L9NN/Pf0iWX2hJM8+bXYXPtMGDqUxOenffcnq8tHdpxrXNKvKh8Lsnho22YBVFncula osVQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of tiwai@suse.de designates 195.135.220.15 as permitted sender) smtp.mailfrom=tiwai@suse.de Authentication-Results: mx.google.com; spf=pass (google.com: domain of tiwai@suse.de designates 195.135.220.15 as permitted sender) smtp.mailfrom=tiwai@suse.de Date: Mon, 23 Apr 2018 13:03:00 +0200 Message-ID: From: Takashi Iwai To: "Jorge Sanjuan" Cc: , , Subject: Re: [PATCH 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit. In-Reply-To: <20180420170327.31569-2-jorge.sanjuan@codethink.co.uk> References: <20180420170327.31569-1-jorge.sanjuan@codethink.co.uk> <20180420170327.31569-2-jorge.sanjuan@codethink.co.uk> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1598285487518505397?= X-GMAIL-MSGID: =?utf-8?q?1598534591245628710?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 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 > --- > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit. Date: Mon, 23 Apr 2018 13:03:00 +0200 Message-ID: References: <20180420170327.31569-1-jorge.sanjuan@codethink.co.uk> <20180420170327.31569-2-jorge.sanjuan@codethink.co.uk> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 5E5612675DF for ; Mon, 23 Apr 2018 13:03:00 +0200 (CEST) In-Reply-To: <20180420170327.31569-2-jorge.sanjuan@codethink.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Jorge Sanjuan Cc: gregkh@linuxfoundation.org, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org List-Id: alsa-devel@alsa-project.org 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 > --- > 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