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

[Takashi Iwai]

At Mon, 23 Apr 2012 12:19:39 +0200,
Felix Homann wrote:
> 
> 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?

Just like "usb-audio:" or something like that.

> >> +                     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?

It complained because of the bit OR.  With the logical OR, it
shouldn't complain.

> >>
> >>  /* 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.

Then follow your instinct :)

> 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?

It's what you need :)  Use like
	if (snd_BUG_ON(!mixer))
		return -EINVAL;


thanks,

Takashi