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

From: Chris J Arges <chris.j.arges@canonical.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: Tobias Hoffmann <th55@gmx.de>,
	robin@gareus.org, clemens@ladisch.de,
	alsa-devel@alsa-project.org, david.henningsson@canonical.com
Subject: Re: [PATCH 4/4 v4] ALSA: usb-audio: Scarlett mixer interface for 6i6, 18i6, 18i8 and 18i20
Date: Tue, 4 Nov 2014 13:56:15 -0600	[thread overview]
Message-ID: <20141104195615.GB9391@canonical.com> (raw)
In-Reply-To: <s5hzjc7uktt.wl-tiwai@suse.de>

On Tue, Nov 04, 2014 at 03:00:46PM +0100, Takashi Iwai wrote:
> At Tue, 04 Nov 2014 14:16:13 +0100,
> Tobias Hoffmann wrote:
> > 
> > > +#define TXT_OFF	"Off"
> > > +#define TXT_PCM(num) "PCM " #num
> > > +#define TXT_ANALOG(num) "Analog " #num
> > > +#define TXT_SPDIF(num) "SPDIF " #num
> > > +#define TXT_ADAT(num) "ADAT " #num
> > > +#define TXT_MIX(letter) "MIX " #letter
> > 
> > My reasoning for using:
> >    static const char txtOff[] = "Off",
> >            txtPcm1[] = "PCM 1", txtPcm2[] = "PCM 2",
> >            txtPcm3[] = "PCM 3", txtPcm4[] = "PCM 4",  [...]
> > instead of repeating the strings for each device (or using macros that 
> > expand to the same strings) was that the final .o / .ko would contain 
> > the string data only once.
> 
> The compiler should be clever enough.
> 
> > But maybe (as Clemens suggested) the strings *for .opt_master and 
> > .opt_matrix* should just be created dynamically in 
> > scarlett_ctl_enum_info, instead of using snd_ctl_enum_info.
> > I want to point out that scarlett_device_info already contains the 
> > necessary information to do this:
> > 
> > 	.matrix_out = 6,
> > ...
> > 	.pcm_start = 0,  // pcm(1) .. pcm(analog_start-pcm_start)=pcm(12)
> > 	.analog_start = 12, // analog(1) .. analog(4)
> > 	.spdif_start = 16, // spdif(1) .. spdif(2)
> > 	.adat_start = 18,  // adat(1) .. adat(0)  [i.e. no adat]
> > 	.mix_start = 18, // mix('A') .. mix('F')=mix('A' + matrix_out)
> > 
> > 
> > 
> > and Off(-1). Only elem->private_data has to be of type 
> > scarlett_mixer_elem_enum_info for the usual enums (impedance, pad, ...), 
> > but of type scarlett_device_info for the master and mixer enums...
> 
> Yeah, that would be also cleaner.
> 
> 
> > Some more comments inline:
> > 
> > On 04/11/14 11:18, Takashi Iwai wrote:
> > > 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).
> > It's a hack to expose the "Save to hardware" functionality without 
> > requiring a special mixer application. Robin's original patch already 
> > contained this (ab)use.
> 
> Heh, the history can't be an excuse to continue the abuse :)
> 
> > > 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.
> > 
> > (opt->start == -1) is used in enums that control the routing, to 
> > represent the "Off"-value.
> > But the device uses number_of_master_enum_values+1 (even for the matrix 
> > enum) as "Off".
> > 
> > That would make "Off" the last enum value in Mixer applications, which 
> > is undesired (and, for the matrix enums, this still results in a gap 
> > between last_matrix_enum_index and Off=last_mixer_enum_index).
> > 
> > scarlett_ctl_enum_put did contain the inverse logic (before cleanup):
> > 
> > > #if 0 // TODO?
> > >         if (val == -1) {
> > >                 val = elem->enum->len + 1; /* only true for master, 
> > > not for mixer [also master must be used] */
> > >                 // ... or? > 0x20,  18i8: 0x22
> > >         } else
> > > #endif
> > >         val = val + elem->opt->start;
> > 
> > It is commented out, because the device also recognized val = -1 + -1; 
> > as "Off", without the need to somehow expose the device-specific 
> > enum->len of .opt_master to all the .opt_matrix controls.
> > It's a bit dirty, but it works...
> 
> Oh, no this is too tricky.  If you want to keep this coding, you have
> to put this whole comment in the code.  Otherwise no reader
> understands the code correctly.
> 
> Please, don't play with too much tricks.  This is no hot path. The
> code size and speed don't matter so much.  The readability has to be
> cared at most, instead.
> 
> 
> > > 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)?
> > 
> > What it does is it store the current settings in non-volatile memory for 
> > standalone operation (i.e. USB not connected). It's the device 
> > configuration after power-on.
> > 
> > Loading this driver then reinitalizes the device to basically zero 
> > values (userspace "acontrol restore" will finally set the device to what 
> > the user expects).
> > The zeroing is unfortunately necessary, because the device does not 
> > support reading back certain mixer values after power-on (only garbage 
> > is returned). After initialization, only cval->cache_val is ever 
> > returned by snd_usb_get_cur_mix_value.
> > 
> > 
> > >> +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.
> > OTOH, saving the settings to hardware should be a rare operation (the 
> > non-volatile memory might also be  designed to withstand only a limited 
> > amount of read-write cycles).
> 
> If so, implementing it as a mixer ctl is a wrong design.  I'd suggest
> rather to drop whole this stuff at first.  If really inevitably
> needed, you can implement it in a different way.
> 
> 
> thanks,
> 
> Takashi

Takashi,
I'll drop it for the initial patchset. Clemens mentioned implementing it as a
mixer control that allows no access but a TLV_COMMAND. Perhaps that could be a
start and then writing support in any userspace applications to expose that 
'button' control correctly.

--chris