[alsa-devel] [PATCH - usb: Headphone support for cm6206 2/2] usb: adds headphone source and mute controls to cm6206

[Takashi Iwai]

At Fri, 30 Jul 2010 11:51:32 -0300,
Adrian Pardini wrote:
> 
> On 30/07/2010, Takashi Iwai <tiwai at suse.de> wrote:
> > At Tue, 20 Jul 2010 19:44:29 -0300,
> > Adrian Pardini wrote:
> >>
> >> +static int snd_cm6206_headphone_source_get(struct snd_kcontrol *kcontrol,
> >> struct snd_ctl_elem_value *ucontrol)
> >
> > Try to avoid too long lines.  Once when you feed your patch to
> > scripts/checkpatch.pl, you'll see it :)
> >
> 
> Ah, I forgot that step. Yes, it promptly complained.
> 
> >> +static int snd_cm6206_headphone_source_put(struct snd_kcontrol *kcontrol,
> >> struct snd_ctl_elem_value *ucontrol)
> >> +{
> >> +	struct usb_mixer_interface *mixer = snd_kcontrol_chip(kcontrol);
> >> +	struct usb_device *dev = mixer->chip->dev;
> >> +
> >
> > Remove a blank line here.
> >
> 
> done.
> 
> 
> >> +	u16 reg2 = mixer->cm6206reg2;
> >
> > I think the corresponding change in mixer.h is missing?
> >
> 
> Yes, it's missing. Is ok to cache the register value there? I'm doing
> it that way because it doesn't interfere with other functions of the
> board and also haven't managed yet to read it with an usb transfer.

It's a feasible solution, so far, I think.
We can make a generic quirk-spec data in future, though... 


> >> +	u16 idx = kcontrol->private_value = ucontrol->value.integer.value[0];
> >> +
> >> +	reg2 = (u16)((((1<<7) | (idx<<5))<<8) | (reg2 & 0x1800));
> >
> > We are no lispers.  Please reduce parentheses.  Also avoid unnecessary
> > cast.
> 
> ok, is something like this more acceptable?
> 
> reg2 = ((1<<7 | idx<<5)<<8) | (reg2 & 0x1800);

The parentheses around bit-shifts are still recommended.
So,
	reg2 = (((1<<7) | (idx<<5)) << 8) | (reg2 & 0x1800);
or
	reg2 &= 0x1800;
	reg2 |= ((1 << 7) | (idx << 5)) << 8;
or
	reg2 = (1 << 15) | (idx << 13) | (reg2 & 0x1800);

or so.  Less depth of parentheses, easier to read.


thanks,

Takashi