At Fri, 30 Jul 2010 11:51:32 -0300,
Adrian Pardini wrote:
>
> On 30/07/2010, Takashi Iwai <tiwai@xxxxxxx> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
