Re: [alsa-devel] [PATCH 4/4] ALSA: usb: add UAC3 BADD profiles support

[Ruslan Bilovol]

On Thu, Apr 19, 2018 at 1:19 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> On Sat, 14 Apr 2018 00:24:26 +0200,
> Ruslan Bilovol wrote:
>>
>> +static void build_feature_ctl_badd(struct usb_mixer_interface *mixer,
>> +                           unsigned int ctl_mask, int control, int unitid,
>> +                           const struct usbmix_name_map *badd_map)
>> +{
> ....
>> +     kctl = snd_ctl_new1(&usb_feature_unit_ctl, cval);
>> +
>> +     if (!kctl) {
>> +             usb_audio_err(mixer->chip, "cannot malloc kcontrol\n");
>
> No need for error message after malloc failure.  The kernel is already
> chatty about it.

Okay. BTW, I'm trying to avoid separate badd variant of
build_feature_ctl(), so this most probably will go away.

There are many existing places in usb-snd which can be cleaned
up though.

>
>
>> +static int snd_usb_mixer_controls_badd(struct usb_mixer_interface *mixer,
>> +                                    int ctrlif)
>> +{
>> +     struct usb_device *dev = mixer->chip->dev;
>> +     struct usb_interface_assoc_descriptor *assoc;
>> +     int badd_profile = mixer->chip->badd_profile;
>> +     const struct usbmix_ctl_map *map;
>> +     int p_chmask = 0, c_chmask = 0, st_chmask = 0;
>> +     int i;
>> +
>> +     assoc = usb_ifnum_to_if(dev, ctrlif)->intf_assoc;
>> +
>> +     /* Detect BADD capture/playback channels from AS EP descriptors */
>> +     for (i = 0; i < assoc->bInterfaceCount; i++) {
>> +             int intf = assoc->bFirstInterface + i;
>> +
>> +             if (intf != ctrlif) {
>
> In this case, it's better to skip like
>
>                 if (intf == ctrlif)
>                         continue;
>
> so that we can save an indentation for the whole long block.

Good point, will do it

>
>> +     switch (badd_profile) {
>> +     default:
>> +             return -EINVAL;
>> +     case UAC3_FUNCTION_SUBCLASS_GENERIC_IO:
>> +             /*
>> +              * BAIF, BAOF or combination of both
>> +              * IN: Mono or Stereo cfg, Mono alt possible
>> +              * OUT: Mono or Stereo cfg, Mono alt possible
>> +              */
>> +             /* c_chmask := DYNAMIC */
>> +             /* p_chmask := DYNAMIC */
>> +             if (!c_chmask && !p_chmask) {
>> +                     usb_audio_err(mixer->chip,
>> +                             "BADD GENERIC_IO profile: no channels?\n");
>> +                     return -EINVAL;
>> +             }
>> +             break;
>
> Maybe we can simplify the whole switch/case with a table lookup.

Yes, it should be possible, I'll implent it that way

Thanks,
Ruslan

> For example,
>
>         for (f = uac3_func_tables; f->name; f++) {
>                 if (badd_profile == f->subclass)
>                         break;
>         }
>         if (!f->name)
>                 return -EINVAL;
>         if (!uac3_func_has_valid_channels(mixer, f, c_chmask, p_chmask))
>                 return -EINVAL;
>         st_chmask = f->st_chmask;
>
> and in uac3_func_has_valid_channels(),
>
> static bool uac3_func_has_valid_channels()
> {
>         if ((f->c_chmask < 0 && !c_chmask) ||
>             (f->c_chmask >= 0 && f->c_chmask != c_chmask)) {
>                 usb_audio_warn(mixer->chip, "BAAD %s c_chmask mismatch",
>                                 f->name);
>                 return false;
>         }
>         if ((f->p_chmask < 0 && !p_chmask) ||
>             (f->p_chmask >= 0 && f->p_chmask != p_chmask)) {
>                 usb_audio_warn(mixer->chip, "BAAD %s p_chmask mismatch",
>                                 f->name);
>                 return false;
>         }
>         return true;
> }
>
>
> thanks,
>
> Takashi