[alsa-devel] [PATCH 3/3] M-Audio FTU: Add effect controls

[Felix Homann]

Hi,

2012/4/23 Takashi Iwai <tiwai at suse.de>:
> At Mon, 23 Apr 2012 11:16:08 +0200,
> Felix Homann wrote:
>>
>>
>> Signed-off-by: Felix Homann <linuxaudio at showlabor.de>
>
> Again, a bit more explanations please.
>
>>
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 596744f..be46657 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -775,6 +775,24 @@ static void volume_control_quirks(struct usb_mixer_elem_info *cval,
>>                                 struct snd_kcontrol *kctl)
>>  {
>>       switch (cval->mixer->chip->usb_id) {
>> +     case USB_ID(0x0763, 0x2081): /* M-Audio Fast Track Ultra 8R */
>> +     case USB_ID(0x0763, 0x2080): /* M-Audio Fast Track Ultra */
>> +             if ((strcmp(kctl->id.name, "Effect Duration") == 0)) {
>> +                     snd_printk(KERN_INFO
>> +                             "set quirk for FTU Effect Duration\n");
>
> Put a prefix in the info print to be identified properly.
> snd_printk() might be just printk() when CONFIG_SND_VERBOSE_PRINTK
> isn't set.

Hmm, again I just followed the examples of other volume quirks. What
kind of prefix would you like?

>
>
>> +                     cval->min = 0x0000;
>> +                     cval->max = 0x7f00;
>> +                     cval->res = 0x0100;
>> +                     break;
>> +             }
>> +             if ((strcmp(kctl->id.name, "Effect Volume") == 0) |
>
> Use the logical OR '||'.

Oh yes, stupid me...

> Also the parenthesis is superfluous.

I know. I put them there because checkpatch.pl complained. What's the
right way to cope with checkpatch.pl complains then?


>>
>>  /* M-Audio FastTrack Ultra quirks */
>>
>> +/* FTU Effect switch */
>> +struct snd_maudio_ftu_effect_switch_private_value {
>
> Well... The length of a name is a matter of taste, but it looks
> unnecessarily too long to me, as it appears in the casts.

It's not to my taste either. But it's in line with the existing ftu
quirks. I'm already preparing a patch for the M-Audio C-400 mixer.
What do you think of

* renaming the FTU specifiy functions from snd_maudio_ftu_* to snd_ftu_*
* renaming functions common to different M-Audio devices to snd_maudio_*
* specifically renaming snd_maudio_ftu_effect_switch_private_value to
snd_maudio_effect_switch_priv_val (well, it's just a bit shorter...)



>> +     mixer = (struct usb_mixer_interface *) pval->mixer;
>> +     if (mixer == NULL) {
>> +             snd_printd(KERN_ERR "mixer == NULL");
>
> Use snd_BUG_ON() in such a case.

What's that?


>> +static int snd_maudio_ftu_create_effect_switch(struct usb_mixer_interface *mixer)
>> +{
>> +     struct snd_kcontrol_new template = {
>> +             .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
>> +             .name = "Effect Program Switch",
>> +             .index = 0,
>> +             .access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
>> +             .info = snd_maudio_ftu_effect_switch_info,
>> +             .get = snd_maudio_ftu_effect_switch_get,
>> +             .put = snd_maudio_ftu_effect_switch_put
>> +     };
>
> Use static.

OK.


>> +static int snd_maudio_ftu_create_effect_volume_ctl(struct usb_mixer_interface *mixer)
>> +{
>> +     unsigned int id, control, cmask;
>> +     int val_type;
>> +
>> +     char name[] = "Effect Volume";
>
> Use static.

OK.

>
>> +
>> +     id = 6;
>> +     val_type = USB_MIXER_U8;
>> +     control = 2;
>> +     cmask = 0;
>
> Use const.

OK.

>> +static int snd_maudio_ftu_create_effect_duration_ctl(struct usb_mixer_interface *mixer)
>> +{
>> +     unsigned int id, control, cmask;
>> +     int val_type;
>> +
>> +     char name[] = "Effect Duration";
>> +
>> +     id = 6;
>> +     val_type = USB_MIXER_S16;
>> +     control = 3;
>> +     cmask = 0;
>
> Use const.

OK.


>> +                             "Input A Capture Volume", NULL);
>> +     snd_create_std_mono_ctl(mixer, 10, 2, 0x2, USB_MIXER_S16,
>> +                             "Input B Capture Volume", NULL);
>
> These should have been applied in the first patch.

OK. I wasn't aware of the extremely reasonable policy of not breaking
the build with a patch...

Sorry,

Felix