[PATCH - usb: Headphone support for cm6206 1/2] usb: move snd_usb_cm106_write_int_reg to mixer_quirks.c

[PATCH - usb: Headphone support for cm6206 1/2] usb: move snd_usb_cm106_write_int_reg to mixer_quirks.c

[Adrian Pardini]

Hi all, this series of patches adds another control to cm6206 based cards to
let the user select wich ouput the "Headphone" jack mirrors. Please review.

Thanks.

Signed-off-by: Adrian Pardini <adrian.pardini@solar.org.ar>

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index e7df1e5..c4cbbc0 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -354,6 +354,22 @@ void snd_emuusb_set_samplerate(struct snd_usb_audio *chip,
 	}
 }
 
+/*
+ * C-Media CM106/CM106+ have four 16-bit internal registers that are nicely
+ * documented in the device's data sheet.
+ */
+int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value)
+{
+	u8 buf[4];
+	buf[0] = 0x20;
+	buf[1] = value & 0xff;
+	buf[2] = (value >> 8) & 0xff;
+	buf[3] = reg;
+	return snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), USB_REQ_SET_CONFIGURATION,
+			       USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_ENDPOINT,
+			       0, 0, &buf, 4, 1000);
+}
+
 int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 {
 	int err;
diff --git a/sound/usb/mixer_quirks.h b/sound/usb/mixer_quirks.h
index bdbfab0..bc5d577 100644
--- a/sound/usb/mixer_quirks.h
+++ b/sound/usb/mixer_quirks.h
@@ -9,5 +9,7 @@ void snd_emuusb_set_samplerate(struct snd_usb_audio *chip,
 void snd_usb_mixer_rc_memory_change(struct usb_mixer_interface *mixer,
 				    int unitid);
 
+int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value);
+
 #endif /* SND_USB_MIXER_QUIRKS_H */
 
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index 9a9da09..b8a5a18 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -353,22 +353,6 @@ static int snd_usb_audigy2nx_boot_quirk(struct usb_device *dev)
 	return 0;
 }
 
-/*
- * C-Media CM106/CM106+ have four 16-bit internal registers that are nicely
- * documented in the device's data sheet.
- */
-static int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value)
-{
-	u8 buf[4];
-	buf[0] = 0x20;
-	buf[1] = value & 0xff;
-	buf[2] = (value >> 8) & 0xff;
-	buf[3] = reg;
-	return snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), USB_REQ_SET_CONFIGURATION,
-			       USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_ENDPOINT,
-			       0, 0, &buf, 4, 1000);
-}
-
 static int snd_usb_cm106_boot_quirk(struct usb_device *dev)
 {
 	/*
-- 
1.5.4.3



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

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

[Adrian Pardini]

Signed-off-by: Adrian Pardini <adrian.pardini@solar.org.ar>

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index c4cbbc0..c53842c 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -370,6 +370,112 @@ int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value)
 			       0, 0, &buf, 4, 1000);
 }
 
+static int snd_cm6206_headphone_source_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol)
+{
+	ucontrol->value.integer.value[0] = kcontrol->private_value;
+	return 0;
+}
+
+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;
+
+	u16 reg2 = mixer->cm6206reg2;
+	u16 idx = kcontrol->private_value = ucontrol->value.integer.value[0];
+
+	reg2 = (u16)((((1<<7) | (idx<<5))<<8) | (reg2 & 0x1800));
+
+	snd_usb_cm106_write_int_reg(dev, 2, reg2);
+	mixer->cm6206reg2 = reg2;
+	return 0;
+}
+
+static int snd_cm6206_headphone_mute_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol)
+{
+	ucontrol->value.integer.value[0] = kcontrol->private_value & (1<<11);
+	ucontrol->value.integer.value[1] = kcontrol->private_value & (1<<12);
+	return 0;
+}
+
+
+
+static int snd_cm6206_headphone_mute_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;
+
+	u16 reg2 = mixer->cm6206reg2;
+	u16 mute;
+
+	mute = ucontrol->value.integer.value[1] ? (1<<12) : 0;
+	mute |= ucontrol->value.integer.value[0] ? (1<<11) : 0;
+	kcontrol->private_value = mute;
+
+	reg2 = (u16)(mute | (reg2 & ~0x1800));
+
+	snd_usb_cm106_write_int_reg(dev, 2, reg2);
+	mixer->cm6206reg2 = reg2;
+	return 1;
+}
+
+static int snd_cm6206_headphone_source_info(struct snd_kcontrol *kcontrol,
+					    struct snd_ctl_elem_info *uinfo)
+{
+	static const char *const names[] = {
+		"Side",
+		"Surround",
+		"Center",
+		"Front"
+	};
+
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
+	uinfo->count = 1;
+	uinfo->value.enumerated.items = 4;
+	if (uinfo->value.enumerated.item > 3)
+		uinfo->value.enumerated.item = 3;
+	strcpy(uinfo->value.enumerated.name, names[uinfo->value.enumerated.item]);
+	return 0;
+}
+
+#define snd_cm6206_headphone_mute_info  snd_ctl_boolean_stereo_info
+
+
+static struct snd_kcontrol_new cm6206_controls[] = {
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
+		.name = "Headphone Source",
+		.info = snd_cm6206_headphone_source_info,
+		.get = snd_cm6206_headphone_source_get,
+		.put = snd_cm6206_headphone_source_put,
+		.private_value = 3
+	},
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
+		.name = "Headphone",
+		.info = snd_cm6206_headphone_mute_info,
+		.get = snd_cm6206_headphone_mute_get,
+		.put = snd_cm6206_headphone_mute_put,
+		.private_value = 24
+	}
+};
+
+static int snd_cm6206_controls_create(struct usb_mixer_interface *mixer)
+{
+	int i, err;
+
+	for (i = 0; i < ARRAY_SIZE(cm6206_controls); ++i) {
+		err = snd_ctl_add(mixer->chip->card,
+				  snd_ctl_new1(&cm6206_controls[i], mixer));
+		if (err < 0)
+			return err;
+	}
+	mixer->cm6206reg2 = 0xf800; /* Headphone source=front, no mute. */
+	return 0;
+}
+
 int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 {
 	int err;
@@ -395,6 +501,12 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 			return err;
 	}
 
+	if (mixer->chip->usb_id == USB_ID(0x0d8c, 0x0102)) {
+		err = snd_cm6206_controls_create(mixer);
+		if (err < 0)
+			return err;
+	}
+
 	return 0;
 }
 
-- 
1.5.4.3




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

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

[Takashi Iwai]

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 :) 

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

> +	u16 reg2 = mixer->cm6206reg2;

I think the corresponding change in mixer.h is missing?

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


thanks,

Takashi

Re: [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@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/

Re: [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@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

Re: [PATCH - usb: Headphone support for cm6206 1/2] usb: move snd_usb_cm106_write_int_reg to mixer_quirks.c

[Takashi Iwai]

At Tue, 20 Jul 2010 19:42:28 -0300,
Adrian Pardini wrote:
> 
> Hi all, this series of patches adds another control to cm6206 based cards to
> let the user select wich ouput the "Headphone" jack mirrors. Please review.

You can just remove "static" instead of moving the whole function,
right?


thanks,

Takashi

> 
> Thanks.
> 
> Signed-off-by: Adrian Pardini <adrian.pardini@solar.org.ar>
> 
> diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
> index e7df1e5..c4cbbc0 100644
> --- a/sound/usb/mixer_quirks.c
> +++ b/sound/usb/mixer_quirks.c
> @@ -354,6 +354,22 @@ void snd_emuusb_set_samplerate(struct snd_usb_audio *chip,
>  	}
>  }
>  
> +/*
> + * C-Media CM106/CM106+ have four 16-bit internal registers that are nicely
> + * documented in the device's data sheet.
> + */
> +int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value)
> +{
> +	u8 buf[4];
> +	buf[0] = 0x20;
> +	buf[1] = value & 0xff;
> +	buf[2] = (value >> 8) & 0xff;
> +	buf[3] = reg;
> +	return snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), USB_REQ_SET_CONFIGURATION,
> +			       USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_ENDPOINT,
> +			       0, 0, &buf, 4, 1000);
> +}
> +
>  int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
>  {
>  	int err;
> diff --git a/sound/usb/mixer_quirks.h b/sound/usb/mixer_quirks.h
> index bdbfab0..bc5d577 100644
> --- a/sound/usb/mixer_quirks.h
> +++ b/sound/usb/mixer_quirks.h
> @@ -9,5 +9,7 @@ void snd_emuusb_set_samplerate(struct snd_usb_audio *chip,
>  void snd_usb_mixer_rc_memory_change(struct usb_mixer_interface *mixer,
>  				    int unitid);
>  
> +int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value);
> +
>  #endif /* SND_USB_MIXER_QUIRKS_H */
>  
> diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
> index 9a9da09..b8a5a18 100644
> --- a/sound/usb/quirks.c
> +++ b/sound/usb/quirks.c
> @@ -353,22 +353,6 @@ static int snd_usb_audigy2nx_boot_quirk(struct usb_device *dev)
>  	return 0;
>  }
>  
> -/*
> - * C-Media CM106/CM106+ have four 16-bit internal registers that are nicely
> - * documented in the device's data sheet.
> - */
> -static int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value)
> -{
> -	u8 buf[4];
> -	buf[0] = 0x20;
> -	buf[1] = value & 0xff;
> -	buf[2] = (value >> 8) & 0xff;
> -	buf[3] = reg;
> -	return snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), USB_REQ_SET_CONFIGURATION,
> -			       USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_ENDPOINT,
> -			       0, 0, &buf, 4, 1000);
> -}
> -
>  static int snd_usb_cm106_boot_quirk(struct usb_device *dev)
>  {
>  	/*
> -- 
> 1.5.4.3
> 
> 
> 
> -- 
> Adrian.
> http://elesquinazotango.com.ar
> http://www.noalcodigodescioli.blogspot.com/
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH - usb: Headphone support for cm6206 1/2] usb: move snd_usb_cm106_write_int_reg to mixer_quirks.c

[Adrian Pardini]

On 30/07/2010, Takashi Iwai <tiwai@suse.de> wrote:
> At Tue, 20 Jul 2010 19:42:28 -0300,
> Adrian Pardini wrote:
>>
>> Hi all, this series of patches adds another control to cm6206 based cards
>> to
>> let the user select wich ouput the "Headphone" jack mirrors. Please
>> review.
>
> You can just remove "static" instead of moving the whole function,
> right?

Aww nuts you are right, sorry about that. Now i'm working with the
mapping of volume controls values, will resend next week when / if
everything works fine.

Thanks.


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