Re: [WIP PATCH] Scarlett mixer interface for 6i6, 18i6, 18i8 and 18i20.

[Tobias Hoffmann <smilingthax@googlemail.com>]

On 09/10/14 10:31, Daniel Mack wrote:
>
>>>> +#if 0 /* rg debug XXX */
>>>> +	snd_printk(KERN_ERR "req ctl value: req = %#x, rtype = %#x, wValue = %#x, wIndex = %#x, size = %d\n",
>>>> +		   bRequest, (USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN), wValue, idx, size);
>>>> +#endif
>>> That can be moved to snd_printdd()
>> These are left overs from Robins code, even I didn't need them. It's
>> probably safe to just remove all the
>>     #if 0 /* rg debug XXX */  ... #endif
>> from the patch.
> Sure it's safe, as dead code doesn't do anything :) The question is
> whether the line produces any useful information for people having
> trouble with their device. If so, we can just convert it to snd_printdd().

Well, yes. I was looking for a short word for "no one will miss it", 
when I wrote "safe"...


> 	>  The "Save to HW" enum uses a special kcontrol .get/.put combo:
>> .get always returns 0, and .put issues a special command to the hardware.
>>
>> I not 100% sure what the "best" return value of the .put function would
>> be in that case.
>> AFAIUI, .put functions have to return whether the value has changed...
>> and here:
>>    - something happend (supporting return 1;)
>>    - but .get still returns 0 (supporting return 0;)
> Ah, I see. Yes, I'd consider that a misuse of the ALSA control API, as
> you're not dealing with an actual control here. I think that should
> become a hwdep interface then.

I think I've seen the same idea ("abuse") in another driver(?).


>
>> But I found this...:
>>> +static struct snd_kcontrol_new usb_scarlett_ctl_save = {
>>> +	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
>>> +	.name = "",
>>> +	.info = scarlett_ctl_enum_info,
>>> +	.get =  scarlett_ctl_save_get,
>>> +	.get =  scarlett_ctl_save_put,
>>> +};
>> BUG! .get is set twice instead of .get and .put...
> Right, so the code in .put was never actually used, right? Did the
> compiler not warn about that?

No, it does not warn...
I do have some memories of testing that code (i.e. the printk was 
called), and of experimenting with .get / .put for that particular 
control, esp. concerning  alsactl store/restore.

I will do a full compile / test cycle, when David has incorporated all 
the comments into a new patch.

> ARRAY_SIZE(s18i8_text) == 35, so  .len = (ARRAY_SIZE()-8) ?
> I not sure that's helpful...
> The better way to think about it is:
>     .opt_matrix.len = .mix_start + 1,
>     .opt_master.len = .opt_matrix.len + .matrix_out    // == ARRAY_SIZE()
> Hmm, yes. I was just thinking about something that makes the values look
> a bit less magical.

Maybe a comment at the first such definition helps.

   Tobias