LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jorge <jorge.sanjuan@codethink.co.uk>
To: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Cc: Takashi Iwai <tiwai@suse.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [alsa-devel] [PATCH v2 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.
Date: Thu, 26 Apr 2018 17:56:37 +0100	[thread overview]
Message-ID: <3dbeff72-d3c8-7a6d-4b46-61ca9d175eda@codethink.co.uk> (raw)
In-Reply-To: <CAB=otbQPX4QW8DKNGTS1dCRg2FUp46KCYL+ADnG=VkWPjg3YuQ@mail.gmail.com>



On 25/04/18 23:35, Ruslan Bilovol wrote:
>   On Tue, Apr 24, 2018 at 8:24 PM, Jorge Sanjuan
> <jorge.sanjuan@codethink.co.uk> wrote:
>> This adds support for the MIXER UNIT in UAC3. All the information
>> is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
>> read the rest of the logical cluster to obtain the channel config
>> as that wont make any difference in the current mixer behaviour.
>>
>> The name of the mixer unit is not yet requested as there is not
>> support for the UAC3 Class Specific String requests.
>>
>> Tested in an UAC3 device working as a HEADSET with a basic mixer
>> unit (same as the one in the BADD spec) with no controls.
> 
> The patch looks OK in general, but I have few comments

Thanks for the review! I'll submit v3 of this patch with the suggested 
changes.

Regards,

Jorge

> 
>>
>> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
>> ---
>>   include/uapi/linux/usb/audio.h | 13 +++++--
>>   sound/usb/mixer.c              | 87 ++++++++++++++++++++++++++++++++++++++++--
>>   2 files changed, 93 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
>> index 3a78e7145689..f9be472cd025 100644
>> --- a/include/uapi/linux/usb/audio.h
>> +++ b/include/uapi/linux/usb/audio.h
>> @@ -285,9 +285,16 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
>>   static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
>>                                                int protocol)
> 
> Name of this function is ambiguous, that's because UAC1 mixer unit
> has only bmControls bitmap, whereas UAC2/3 mixer unit has two
> bitmaps (bmMixerControls and bmControls), in latter case this func
> returns pointer to bmMixerControls.
> 
> Maybe in the future we will need to rename it, but at least having
> comment which clarifies that in case of UAC2/3 this function actually
> returns pointer to bmMixerControls here will be helpful for code readers.
> 
>>   {
>> -       return (protocol == UAC_VERSION_1) ?
>> -               &desc->baSourceID[desc->bNrInPins + 4] :
>> -               &desc->baSourceID[desc->bNrInPins + 6];
>> +       switch (protocol) {
>> +       case UAC_VERSION_1:
>> +               return &desc->baSourceID[desc->bNrInPins + 4];
>> +       case UAC_VERSION_2:
>> +               return &desc->baSourceID[desc->bNrInPins + 6];
>> +       case UAC_VERSION_3:
>> +               return &desc->baSourceID[desc->bNrInPins + 2];
>> +       default:
>> +               return NULL;
>> +       }
>>   }
>>
>>   static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 301ad61ed426..bf701b466a4e 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
>>   }
>>
>>   /*
>> + * Get logical cluster information for UAC3 devices.
>> + */
>> +static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
>> +{
>> +       struct uac3_cluster_header_descriptor c_header;
>> +       int err;
>> +
>> +       err = snd_usb_ctl_msg(state->chip->dev,
>> +                       usb_rcvctrlpipe(state->chip->dev, 0),
>> +                       UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
>> +                       USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
>> +                       cluster_id,
>> +                       snd_usb_ctrl_intf(state->chip),
>> +                       &c_header, sizeof(c_header));
>> +       if (err < 0)
>> +               goto error;
>> +       if (err != sizeof(c_header)) {
>> +               err = -EIO;
>> +               goto error;
>> +       }
>> +
>> +       return c_header.bNrChannels;
>> +
>> +error:
>> +       usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
>> +       return err;
>> +}
>> +
>> +/*
>> + * Get number of channels for a Mixer Unit.
>> + */
>> +static int uac_mixer_unit_get_channels(struct mixer_build *state,
>> +                                      struct uac_mixer_unit_descriptor *desc)
>> +{
>> +       int mu_channels;
>> +
>> +       if (desc->bLength < 11)
>> +               return -EINVAL;
>> +       if (!desc->bNrInPins)
>> +               return -EINVAL;
>> +
>> +       switch (state->mixer->protocol) {
>> +       case UAC_VERSION_1:
>> +       case UAC_VERSION_2:
>> +       default:
>> +               mu_channels = uac_mixer_unit_bNrChannels(desc);
>> +               break;
>> +       case UAC_VERSION_3:
>> +               mu_channels = get_cluster_channels_v3(state,
>> +                               le16_to_cpu(desc->baSourceID[desc->bNrInPins]));
> 
> It would be good to create and use some helper here, for example implement
> uac3_mixer_unit_wClusterDescrID() similar to uac_mixer_unit_bNrChannels().
> It will put all this conversation to a single place and will improve
> code readability.
> 
>> +               break;
>> +       }
>> +
>> +       if (!mu_channels)
>> +               return -EINVAL;
>> +
>> +       return mu_channels;
>> +}
>> +
>> +/*
>>    * parse the source unit recursively until it reaches to a terminal
>>    * or a branched unit.
>>    */
>> @@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
>>                                  term->name = le16_to_cpu(d->wClockSourceStr);
>>                                  return 0;
>>                          }
>> +                       case UAC3_MIXER_UNIT: {
>> +                               struct uac_mixer_unit_descriptor *d = p1;
>> +
>> +                               err = uac_mixer_unit_get_channels(state, d);
>> +                               if (err < 0)
>> +                                       return err;
>> +
>> +                               term->channels = err;
>> +                               term->type = d->bDescriptorSubtype << 16; /* virtual type */
>> +
>> +                               return 0;
>> +                       }
>>                          default:
>>                                  return -ENODEV;
>>                          }
>> @@ -1801,7 +1873,7 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
>>                                   struct usb_audio_term *iterm)
>>   {
>>          struct usb_mixer_elem_info *cval;
>> -       unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
>> +       unsigned int num_outs;
>>          unsigned int i, len;
>>          struct snd_kcontrol *kctl;
>>          const struct usbmix_name_map *map;
>> @@ -1814,6 +1886,10 @@ static void build_mixer_unit_ctl(struct mixer_build *state,
>>          if (!cval)
>>                  return;
>>
>> +       num_outs = uac_mixer_unit_get_channels(state, desc);
>> +       if (num_outs < 0)
>> +               return;
> 
> This will produce an extra USB control request in UAC3 case, which we can
> avoid if add num_outs to input parameters of build_mixer_unit_ctl() function.
> This function is used only once inside of parse_audio_mixer_unit() which already
> has channels number request and all needed checks.
> 
> Thanks,
> Ruslan
> 
>> +
>>          snd_usb_mixer_elem_init_std(&cval->head, state->mixer, unitid);
>>          cval->control = in_ch + 1; /* based on 1 */
>>          cval->val_type = USB_MIXER_S16;
>> @@ -1878,14 +1954,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>>          int input_pins, num_ins, num_outs;
>>          int pin, ich, err;
>>
>> -       if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
>> -           !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
>> +       err = uac_mixer_unit_get_channels(state, desc);
>> +       if (err < 0) {
>>                  usb_audio_err(state->chip,
>>                                "invalid MIXER UNIT descriptor %d\n",
>>                                unitid);
>> -               return -EINVAL;
>> +               return err;
>>          }
>>
>> +       num_outs = err;
>> +       input_pins = desc->bNrInPins;
>> +
>>          num_ins = 0;
>>          ich = 0;
>>          for (pin = 0; pin < input_pins; pin++) {
>> --
>> 2.11.0
>>
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

  reply	other threads:[~2018-04-26 16:56 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-20 17:03 [PATCH 0/4] ALSA: usb: UAC3 new features Jorge Sanjuan
2018-04-20 17:03 ` [PATCH 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit Jorge Sanjuan
2018-04-23 11:03   ` Takashi Iwai
2018-04-20 17:03 ` [PATCH 2/4] ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3 Jorge Sanjuan
2018-04-23 12:11   ` Takashi Iwai
2018-04-24  8:03   ` [alsa-devel] " Ruslan Bilovol
2018-04-20 17:03 ` [PATCH 3/4] ALSA: usb-audio: Use Class Specific EP for UAC3 devices Jorge Sanjuan
2018-04-22 20:30   ` [alsa-devel] " kbuild test robot
2018-04-20 17:03 ` [PATCH 4/4] ALSA: usb-audio: UAC3 Add support for connector insertion Jorge Sanjuan
2018-04-22 20:55   ` kbuild test robot
2018-04-23 12:19   ` Takashi Iwai
2018-04-23 16:06     ` Jorge
2018-04-24 17:24 ` [PATCH v2 0/4] ALSA: usb: UAC3 new features Jorge Sanjuan
2018-04-24 17:24   ` [PATCH v2 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit Jorge Sanjuan
2018-04-25 22:35     ` [alsa-devel] " Ruslan Bilovol
2018-04-26 16:56       ` Jorge [this message]
2018-04-27 17:06     ` [PATCH v3 " Jorge Sanjuan
2018-05-04  0:57       ` Ruslan Bilovol
2018-05-08  9:43         ` Jorge
2018-05-09 22:11           ` Ruslan Bilovol
2018-04-24 17:24   ` [PATCH v2 2/4] ALSA: usb-audio: ADC3: Fix channel mapping conversion for ADC3 Jorge Sanjuan
2018-04-24 17:55     ` Takashi Iwai
2018-04-24 17:24   ` [PATCH v2 3/4] ALSA: usb-audio: Use Class Specific EP for UAC3 devices Jorge Sanjuan
2018-04-25 22:53     ` [alsa-devel] " Ruslan Bilovol
2018-04-24 17:24   ` [PATCH v2 4/4] ALSA: usb-audio: UAC3 Add support for connector insertion Jorge Sanjuan
2018-04-24 18:02   ` [PATCH v2 0/4] ALSA: usb: UAC3 new features Takashi Iwai
2018-04-26  9:26     ` [alsa-devel] " Ruslan Bilovol
2018-04-26 17:13       ` Jorge
2018-05-11 15:25 ` [PATCH v4 " Jorge Sanjuan
2018-05-11 15:25   ` [PATCH v4 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit Jorge Sanjuan
2018-05-14 20:54     ` Ruslan Bilovol
2018-05-11 15:25   ` [PATCH v4 2/4] ALSA: usb-audio: Use Class Specific EP for UAC3 devices Jorge Sanjuan
2018-05-14 21:00     ` Ruslan Bilovol
2018-05-11 15:25   ` [PATCH v4 3/4] ALSA: usb-audio: UAC3 Add support for connector insertion Jorge Sanjuan
2018-05-11 15:25   ` [PATCH v4 4/4] ALSA: usb-audio: UAC3: Parse Input Terminal number of channels Jorge Sanjuan
2018-05-14  8:54     ` Jorge
2018-05-14  9:36       ` Ruslan Bilovol
2018-05-14 11:03     ` [RESEND PATCH " Jorge Sanjuan
2018-05-14 21:05       ` Ruslan Bilovol
2018-05-15  5:38   ` [PATCH v4 0/4] ALSA: usb: UAC3 new features Takashi Iwai

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3dbeff72-d3c8-7a6d-4b46-61ca9d175eda@codethink.co.uk \
    --to=jorge.sanjuan@codethink.co.uk \
    --cc=alsa-devel@alsa-project.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ruslan.bilovol@gmail.com \
    --cc=tiwai@suse.com \
    --subject='Re: [alsa-devel] [PATCH v2 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).