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

[Ruslan Bilovol]

On Wed, Nov 8, 2017 at 4:38 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> 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@xxxxxxxxx>
>
> The patch looks good, but the timing is fairly late for merging to
> 4.15.

That's OK

>
> 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...

The main idea of this rename was to make code more clear.
Since with UAC3 clock handling (and many other things) still is the
same as in UAC2 but discriptor layouts and some constatns numbers
changed. So it will possible to not introduce extra comparisons
to run the same function.
On the other hand we still can use uac2 named function, but
programming becomes pure hell in this case. This is because
some part of constatns are common for UAC1 and UAC2, some
are common UAC2 and UAC3, and some are common for
all three of them. To make the code more understandable,
I tried to change common function names to see in the code
what is common.

By the way, I'll try to do it another way in this particular case

>
> 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
>    */

Correct. I used it only because other places in clock.c has
same coding style. Will fix it.

>
>> +             /* 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?

We have the same info print in UAC2 version of function, let's keep
it consistent (or change in both places)

>
>
>>  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.

Agree.

>
>
>> @@ -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.

Agree, will fix it

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

Yes, somehow I missed this before submitting the patch. Will fix it

>
>> @@ -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.

Okay, I'll do that.

Thanks,
Ruslan Bilovol