Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 23 Apr 2018 14:19:38 +0200
Message-ID: <s5hr2n6ru79.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     "Jorge Sanjuan" <jorge.sanjuan@codethink.co.uk>
Cc:     <alsa-devel@alsa-project.org>, <gregkh@linuxfoundation.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/4] ALSA: usb-audio: UAC3 Add support for connector insertion.
In-Reply-To: <20180420170327.31569-5-jorge.sanjuan@codethink.co.uk>
References: <20180420170327.31569-1-jorge.sanjuan@codethink.co.uk>
        <20180420170327.31569-5-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
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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