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