diff mbox

[4/4] ALSA: usb-audio: UAC3 Add support for connector insertion.

Message ID 20180420170327.31569-5-jorge.sanjuan@codethink.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Jorge Sanjuan April 20, 2018, 5:03 p.m. UTC 
This adds support for the UAC3 insertion controls. The status
is reported as a boolean value in the same way it used to do
for UAC2. Hence, the presence of any connector in the response
will make the control saying the jack is connected.

The UAC2 support for this control has been moved to a dedicated
control for connectors as both UAC2 and UAC3 follow a specific
Control Request Parameter Block for this control. This parameter
block for UAC3 could not be read in the same simplistic
manner as in UAC2.

This implementation is not requesting additional information
from the HIGH CAPABILITY Connectors descriptor.

Tested with an UAC3 device with UAC2 as legacy configuration.
The connector status can be read with `amixer` and the interrupt
is also caught with `alsactl monitor`.

Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 include/linux/usb/audio-v2.h |   7 +++
 include/linux/usb/audio-v3.h |  14 ++++++
 sound/usb/mixer.c            | 111 +++++++++++++++++++++++++++++++++++++------
 3 files changed, 117 insertions(+), 15 deletions(-)

Comments

kernel test robot April 22, 2018, 8:55 p.m. UTC | #1 
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
Takashi Iwai April 23, 2018, 12:19 p.m. UTC | #2 
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
Jorge Sanjuan April 23, 2018, 4:06 p.m. UTC | #3 
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
>
diff mbox

Patch

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