[alsa-devel] [WIP PATCH] Scarlett mixer interface for 6i6, 18i6, 18i8 and 18i20.

[Tobias Hoffmann]

On 08/10/14 23:21, Robin Gareus wrote:
> Thanks David for picking this up.
>
> On 10/08/2014 09:04 PM, Daniel Mack wrote:
>>> +
>>> +/* #define WITH_METER */
>>> +/* #define WITH_LOGSCALEMETER */
>> These should either be converted to module parameters, or removed
>> alltogether. Why are they configurable, anyway?
> Mainly because I lost interest in Focusrite devices before finishing the
> work. They were handy during initial development and the idea was to
> just enable them by default at some point.
>
> Last I checked they both worked fine for the 18i6, but it seems support
> other Scarlett devices is missing and/or untested, so I guess they're
> commented out for that reason.

AFAIR in the latest version something crashes when opening such 
"channels" with (I think) alsamixer.
I didn't even try to debug that, because no Mixer-GUI could actually 
display "realtime" metering data by periodically polling channel values, 
IIRC. The even bigger problem is, that the Scarlett HW returns all 
metering data at once (for a given metering point), e.g. all 18 inputs 
together, in contrast to the mixer-channels, where each value can be set 
separately.
Naively mapped, one would get three snd_kcontrols (metering points: 
input, mix, pcm) with snd_ctl_elem_info::count = 18, 6, 18 (e.g.). 
Alternatively, the driver would have to read all values, cache them 
internally and expose them as separate snd_kcontrols. In either case, a 
mixer GUI probably still has to have special knowledge about the card to 
display the values in a helpful way.

Other drivers seem to use the hwdep-API for things like metering.
I do not understand the hwdep-API enough to implement it as such.
And as trrichad wrote a GUI [1], that can be used to display the hwdep 
data correctly.

The one reason why the metering code is still in the driver (and 
disabled), is because it basically documents how to read the metering 
values...

>
>> +#define CTL_SWITCH(cmd, off, no, count, name) \
>> +	do { \
>> +		err = add_new_ctl(mixer,&usb_scarlett_ctl_switch, cmd, off, no, 2,
> count, name, NULL,&elem); \
>> +		if (err<  0) \
>> +			return err; \
>> +	} while (0)
> curious, why "do { } while (0)" and not just  "{ }" ?

I didn't even have { }, it was just
     err = ...;
     if (err<0) return err;

I would guess checkpatch suggested to add "do {} while(0)" for style 
reasons?


>
>> Hmm, I don't really like the style of magically returning from macros
> Yes, same here. I'm initially responsible for one such macro and wanted
> to get rid of it. I'm sorry if that lead Tobias to adding more in that
> style.

No, it's not your fault. I tried other ways, but the code only became 
less readable:
  - each operation can return an error, which has to be checked. Adding 
the error checks everywhere (i.e. inlining the macros) just clutters the 
code.
  - CTL_ENUM, INIT and CTL_MIXER are called in a loop, based on data in 
the static sXiY_info device info structs. And you don't want arrays with 
 >100 entries *for each supported Scarlett type*.
  - also add_output_ctls() calculates some of the values from the 
function arguments.
  - In many places the same macro (e.g. CTL_SWITCH) isn't actually 
repeated that often, but different CTL_* are used.

Still, some macros can probably be inlined, and esp. the device-specific 
scarlett_sXiY_controls init functions now turn out to be similar enough 
that a static struct entries per device and a generic init  function 
based on these entris seem to make sense.

   Tobias

[1] https://github.com/trrichard/ScarlettMixer