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

[Takashi Iwai]

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