Re: [PATCH 1/1] ALSA: usb: initial USB Audio Device Class 3.0 support

[Takashi Iwai <tiwai@suse.de>]

On Tue, 07 Nov 2017 03:01:20 +0100,
Ruslan Bilovol wrote:
> 
> Recently released USB Audio Class 3.0 specification
> introduces many significant changes comparing to
> previous versions, like
>  - new Power Domains, support for LPM/L1
>  - new Cluster descriptor
>  - changed layout of all class-specific descriptors
>  - new High Capability descriptors
>  - New class-specific String descriptors
>  - new and removed units
>  - additional sources for interrupts
>  - removed Type II Audio Data Formats
>  - ... and many other things (check spec)
> 
> It also provides backward compatibility through
> multiple configurations, as well as requires
> mandatory support for BADD (Basic Audio Device
> Definition) on each ADC3.0 compliant device
> 
> This patch adds initial support of UAC3 specification
> that is enough for Generic I/O Profile (BAOF, BAIF)
> device support from BADD document.
> 
> Signed-off-by: Ruslan Bilovol <ruslan.bilovol@gmail.com>

The patch looks good, but the timing is fairly late for merging to
4.15.

So from my side, the primary question is whether the changes in USB
(audio) header files are OK for USB guys.

Greg, could you check these changes and give an ack if it's OK to
merge?  Or if you prefer postpone, just let me know.


Below are some nitpicking:

> --- a/include/linux/usb/audio-v2.h
> +++ b/include/linux/usb/audio-v2.h
> @@ -33,12 +33,12 @@
>   *
>   */
>  
> -static inline bool uac2_control_is_readable(u32 bmControls, u8 control)
> +static inline bool uac_v2v3_control_is_readable(u32 bmControls, u8 control)
>  {
>  	return (bmControls >> (control * 2)) & 0x1;
>  }
>  
> -static inline bool uac2_control_is_writeable(u32 bmControls, u8 control)
> +static inline bool uac_v2v3_control_is_writeable(u32 bmControls, u8 control)
>  {
>  	return (bmControls >> (control * 2)) & 0x2;
>  }

This change looks a bit strange, but later one I noticed the reason
you didn't copy these functions into usb/audio-v3.h.  Though, I guess
a compiler can optimize out even if you call the same inline code
twice for v2 and v3...

In anyway, if we do rename, let's mention about it in the comment.


> diff --git a/include/linux/usb/audio-v3.h b/include/linux/usb/audio-v3.h
> new file mode 100644
> index 0000000..cbbe51e
> --- /dev/null
> +++ b/include/linux/usb/audio-v3.h
> @@ -0,0 +1,343 @@
> +// SPDX-License-Identifier: GPL-2.0+

You can use the C-style comment for SPDX line,
  /* SPDX-License-Identifier: GPL-2.0+ */

> +static struct uac3_clock_source_descriptor *
> +	snd_usb_find_clock_source_v3(struct usb_host_interface *ctrl_iface,
> +				  int clock_id)
> +{
> +	struct uac3_clock_source_descriptor *cs = NULL;
> +
> +	while ((cs = snd_usb_find_csint_desc(ctrl_iface->extra,
> +					     ctrl_iface->extralen,
> +					     cs, UAC3_CLOCK_SOURCE))) {
> +		if (cs->bClockID == clock_id)
> +			return cs;
> +	}
> +
> +	return NULL;
> +}

We may clean up the whole find_clock_xxx stuff later, but let's keep
it dumb and straight for now...


> +static int __uac3_clock_find_source(struct snd_usb_audio *chip, int entity_id,
> +				   unsigned long *visited, bool validate)
> +{
(snip)
> +		/* the entity ID we are looking for is a selector.
> +		 * find out what it currently selects */

The multi-line comment should be properly formatted like
  /*
   * XXX
   */

> +		/* The current clock source is invalid, try others. */
> +		for (i = 1; i <= selector->bNrInPins; i++) {
> +			int err;
> +
> +			if (i == cur)
> +				continue;
> +
> +			ret = __uac3_clock_find_source(chip, selector->baCSourceID[i - 1],
> +				visited, true);
> +			if (ret < 0)
> +				continue;
> +
> +			err = uac_clock_selector_set_val(chip, entity_id, i);
> +			if (err < 0)
> +				continue;
> +
> +			usb_audio_info(chip,
> +				 "found and selected valid clock source %d\n",
> +				 ret);

Do we need to print this at each time?


>  static int parse_audio_format_i(struct snd_usb_audio *chip,
> -				struct audioformat *fp, unsigned int format,
> -				struct uac_format_type_i_continuous_descriptor *fmt)
> +				struct audioformat *fp, u64 format,
> +				void *_fmt)
>  {
>  	snd_pcm_format_t pcm_format;
> +	unsigned int fmt_type;
>  	int ret;
>  
> -	if (fmt->bFormatType == UAC_FORMAT_TYPE_III) {
> +	switch (fp->protocol) {
> +	default:
> +	case UAC_VERSION_1:
> +	case UAC_VERSION_2: {
> +		struct uac_format_type_i_continuous_descriptor *fmt = _fmt;
> +
> +		fmt_type = fmt->bFormatType;
> +		break;
> +	}
> +	case UAC_VERSION_3: {
> +		/* fp->fmt_type is already set in this case */
> +		fmt_type = fp->fmt_type;
> +		break;
> +	}
> +	}

The double-braces don't look beautiful.  And in this case it's just
for the temporary fmt variable, so it can be moved to the top level
and drop the internal whole braces.


> @@ -405,15 +435,20 @@ static int parse_audio_format_i(struct snd_usb_audio *chip,
>  	 */
>  	switch (fp->protocol) {
>  	default:
> -	case UAC_VERSION_1:
> +	case UAC_VERSION_1: {
> +		struct uac_format_type_i_continuous_descriptor *fmt = _fmt;
> +
>  		fp->channels = fmt->bNrChannels;
>  		ret = parse_audio_format_rates_v1(chip, fp, (unsigned char *) fmt, 7);
>  		break;
> +	}
>  	case UAC_VERSION_2:
> +	case UAC_VERSION_3: {
>  		/* fp->channels is already set in this case */
> -		ret = parse_audio_format_rates_v2(chip, fp);
> +		ret = parse_audio_format_rates_v2v3(chip, fp);
>  		break;
>  	}
> +	}

DItto.

> @@ -520,3 +555,23 @@ int snd_usb_parse_audio_format(struct snd_usb_audio *chip,
>  	return 0;
>  }
>  
> +int snd_usb_parse_audio_format_v3(struct snd_usb_audio *chip,
> +			       struct audioformat *fp,
> +			       struct uac3_as_header_descriptor *as,
> +			       int stream)
> +{
> +	u64 format = le64_to_cpu(as->bmFormats);
> +	int err;
> +
> +	/* Type I format bits are D0..D6 */
> +	if (format & 0x7f)
> +		fp->fmt_type = UAC_FORMAT_TYPE_I;
> +	else
> +		fp->fmt_type = UAC_FORMAT_TYPE_III;
> +
> +	err = parse_audio_format_i(chip, fp, format, as);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;

You can simply return from parse_audio_format_i() without err check.

> @@ -460,9 +461,10 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval,
>  
>  	validx += cval->idx_off;
>  
> +

Superfluous blank line.

> @@ -718,16 +721,104 @@ static int check_input_term(struct mixer_build *state, int id,
> +				} else { /* UAC_VERSION_2 */
> +					struct uac2_input_terminal_descriptor *d = p1;
> +
> +					/* call recursively to verify that the
> +					 * referenced clock entity is valid */

Fix the comment style.

> @@ -624,38 +708,158 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no)
>  				iface_no, altno, as->bTerminalLink);
>  			continue;
>  		}
> -		}
>  
> -		/* get format type */
> -		fmt = snd_usb_find_csint_desc(alts->extra, alts->extralen, NULL, UAC_FORMAT_TYPE);
> -		if (!fmt) {
> +		case UAC_VERSION_3: {
> +			struct uac3_input_terminal_descriptor *input_term;
> +			struct uac3_output_terminal_descriptor *output_term;
> +			struct uac3_as_header_descriptor *as;
> +			struct uac3_cluster_header_descriptor *cluster;
> +			struct uac3_hc_descriptor_header hc_header;
> +			u16 cluster_id, wLength;
(snip)

Now snd_usb_parse_audio_interface() becomes too lengthy and hard to
read through.  It'd be great if you can split / cleanup this in
another patch.


thanks,

Takashi