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

[Adrian Pardini]

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.

>> +	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);



Thanks a lot for your time.


-- 
Adrian.
http://elesquinazotango.com.ar
http://www.noalcodigodescioli.blogspot.com/