[alsa-devel] [PATCH 4/4 v4] ALSA: usb-audio: Scarlett mixer interface for 6i6, 18i6, 18i8 and 18i20

[Takashi Iwai]

At Mon,  3 Nov 2014 16:58:16 -0600,
Chris J Arges wrote:
> 
> This code contains the Scarlett mixer interface code that was originally
> written by Tobias Hoffman and Robin Gareus. Because the device doesn't
> properly implement UAC2 this code adds a mixer quirk for the device.
> 
> Changes from the original code include removing the metering code along with
> dead code and comments. Compiler warnings were fixed. The code to initialize
> the sampling rate was causing a crash this was fixed as discussed on the
> mailing list. Error, and info messages were convered to dev_err and dev_info
> interfaces. The custom scarlett_mixer_elem_info struct was replaced with the
> more generic usb_mixer_elem_info to be able to recycle more code from mixer.c.
> 
> This patch also makes additional modifications based on upstream comments.
> Individual control creation functions are removed and a generic
> function is no used. Macros for function calls are removed to improve
> readability. Hardcoded control initialization is removed.
> 
> Signed-off-by: Chris J Arges <chris.j.arges at canonical.com>

The new patch looks almost good.  A remaining concern is:

> +static const struct scarlett_mixer_elem_enum_info opt_save = {
> +	.start = 0,
> +	.len = 2,
> +	.names = (const char * const[]){
> +		"---", "Save"

This isn't quite intuitive.  And I think this is an abuse of ctl
enum (see below).

Also...

> +static int scarlett_ctl_enum_get(struct snd_kcontrol *kctl,
> +				 struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct usb_mixer_elem_info *elem = kctl->private_data;
> +	struct scarlett_mixer_elem_enum_info *opt = elem->private_data;
> +	int err, val;
> +
> +	err = snd_usb_get_cur_mix_value(elem, 0, 0, &val);
> +	if (err < 0)
> +		return err;
> +
> +	if ((opt->start == -1) && (val > opt->len)) /* >= 0x20 */
> +		val = 0;
> +	else
> +		val = clamp(val - opt->start, 0, opt->len-1);

Is the if condition above really correct?  It's not obvious to me what
this really checks.

Now back to "save to hw" ctl:

> +static int scarlett_ctl_save_get(struct snd_kcontrol *kctl,
> +				 struct snd_ctl_elem_value *ucontrol)
> +{
> +	ucontrol->value.enumerated.item[0] = 0;
> +	return 0;
> +}

So, here is the problem.  You want to use this ctl to trigger
something.  This is no correct behavior for an enum ctl.
If the put callback succeeds, the value should be stored.
(Of course, then it won't work as you expected.)

What actually this control does?  Why it can't be done always
(transparently)?

> +static int scarlett_ctl_save_put(struct snd_kcontrol *kctl,
> +				 struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct usb_mixer_elem_info *elem = kctl->private_data;
> +	struct snd_usb_audio *chip = elem->mixer->chip;
> +	char buf[] = { 0x00, 0xa5 };
> +	int err;
> +
> +	if (ucontrol->value.enumerated.item[0] > 0) {
> +		err = snd_usb_ctl_msg(chip->dev,
> +			usb_sndctrlpipe(chip->dev, 0), UAC2_CS_MEM,
> +			USB_RECIP_INTERFACE | USB_TYPE_CLASS |
> +			USB_DIR_OUT, 0x005a, snd_usb_ctrl_intf(chip) |
> +			(0x3c << 8), buf, 2);
> +		if (err < 0)
> +			return err;
> +
> +		usb_audio_info(elem->mixer->chip,
> +			 "scarlett: saved settings to hardware.\n");

Using *_info() here is rather annoying, it may spew too many random
messages.

> +static int add_new_ctl(struct usb_mixer_interface *mixer,
> +		       const struct snd_kcontrol_new *ncontrol,
> +		       int index, int offset, int num,
> +		       int val_type, int channels, const char *name,
> +		       const struct scarlett_mixer_elem_enum_info *opt,
> +		       struct usb_mixer_elem_info **elem_ret
> +)
> +{
> +	struct snd_kcontrol *kctl;
> +	struct usb_mixer_elem_info *elem;
> +	int err;
> +
> +	elem = kzalloc(sizeof(*elem), GFP_KERNEL);
> +	if (!elem)
> +		return -ENOMEM;
> +
> +	elem->mixer = mixer;
> +	elem->control = offset;
> +	elem->idx_off = num;
> +	elem->id = index;
> +	elem->val_type = val_type;
> +
> +	elem->channels = channels;
> +
> +	/* add scarlett_mixer_elem_enum_info struct */
> +	elem->private_data = (void *)opt;
> +
> +	kctl = snd_ctl_new1(ncontrol, elem);
> +	if (!kctl) {
> +		usb_audio_err(mixer->chip, "cannot malloc kcontrol\n");

This is superfluous.  kmalloc() already gives warning.

> +static int add_output_ctls(struct usb_mixer_interface *mixer,
> +			   int index, const char *name,
> +			   const struct scarlett_device_info *info)
> +{
> +	int err;
> +	char mx[48];

Use SNDRV_CTL_ELEM_ID_NAME_MAXLEN (which is 44).

> +static int scarlett_controls_create_generic(struct usb_mixer_interface *mixer,
> +	const struct scarlett_device_info *info)
> +{
> +	int i = 0;
> +	int err = 0;
> +	char mx[32];

Ditto.
Also, use snprintf() consistently.

> +/*
> + * Create and initialize a mixer for the Focusrite(R) Scarlett
> + */
> +int snd_scarlett_controls_create(struct usb_mixer_interface *mixer)
> +{
> +	int err, i, o;
> +	char mx[32];

Ditto.


thanks,

Takashi