usb: move snd_usb_cm106_write_int_reg to mixer_quirks.c

PATCH In review Adrian Pardini 2 participants 7 messages Jul 20, 2010 – Jul 30, 2010 Sound

Share & tools

Adrian Pardini

author

2010-07-20 22:42 UTC #0 lore raw

PATCH 1/2 usb: move snd_usb_cm106_write_int_reg to mixer_quirks.c

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 <[email protected]>

sound/usb/mixer_quirks.c +16 -0

index e7df1e5..c4cbbc0 100644
@@ -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;

sound/usb/mixer_quirks.h +2 -0

index bdbfab0..bc5d577 100644
@@ -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 */

sound/usb/quirks.c +0 ~~-16~~

index 9a9da09..b8a5a18 100644
@@ -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)
 {
 	/*

Signature · 8 lines

1.5.4.3



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

Adrian Pardini

author

2010-07-20 22:44 UTC #1 lore raw

PATCH 2/2 usb: adds headphone source and mute controls to cm6206

Signed-off-by Adrian Pardini <[email protected]>

sound/usb/mixer_quirks.c +112 -0

index c4cbbc0..c53842c 100644
@@ -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;
 }

Signature · 9 lines

1.5.4.3




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

Takashi Iwai

2010-07-30 12:31 UTC #2 lore raw

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

At Tue, 20 Jul 2010 19:44:29 -0300,

Quoted 2 lines · Adrian Pardini

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

sound/usb/mixer_quirks.c int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value) · line 383

Quoted 5 lines

sound/usb/mixer_quirks.c +5 -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;
+

Remove a blank line here.

Review

Quoted 1 line

(quoted diff) +1 -0
+	u16 reg2 = mixer->cm6206reg2;

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

sound/usb/mixer_quirks.c int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value) · line 387

Quoted 3 lines

sound/usb/mixer_quirks.c +3 -0

+	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

Adrian Pardini

author

2010-07-30 14:51 UTC #3 lore raw

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

Quoted 9 lines · Takashi Iwai

At Tue, 20 Jul 2010 19:44:29 -0300,
Quoted 3 lines · Adrian Pardini
+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.

Quoted 9 lines

sound/usb/mixer_quirks.c int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value) · line 383
Quoted 6 lines
+static int snd_cm6206_headphone_source_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol)
sound/usb/mixer_quirks.c +4 -0
+{
+	struct usb_mixer_interface *mixer = snd_kcontrol_chip(kcontrol);
+	struct usb_device *dev = mixer->chip->dev;
+
Remove a blank line here.

done.

Quoted 4 lines

Review
Quoted 1 line
(quoted diff) +1 -0
+	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.

Quoted 6 lines

sound/usb/mixer_quirks.c int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value) · line 387
Quoted 3 lines
sound/usb/mixer_quirks.c +3 -0
+	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.

Signature · 3 lines

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

Takashi Iwai

2010-07-30 15:08 UTC #4 lore raw

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

At Fri, 30 Jul 2010 11:51:32 -0300,

Quoted 27 lines · Adrian Pardini

Quoted 9 lines · Takashi Iwai
At Tue, 20 Jul 2010 19:44:29 -0300,
Quoted 3 lines · Adrian Pardini
+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.
Quoted 9 lines
sound/usb/mixer_quirks.c int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value) · line 383
Quoted 6 lines
+static int snd_cm6206_headphone_source_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol)
sound/usb/mixer_quirks.c +4 -0
+{
+	struct usb_mixer_interface *mixer = snd_kcontrol_chip(kcontrol);
+	struct usb_device *dev = mixer->chip->dev;
+
Remove a blank line here.
done.

Quoted 8 lines

Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+	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...

Quoted 10 lines

Quoted 6 lines
sound/usb/mixer_quirks.c int snd_usb_cm106_write_int_reg(struct usb_device *dev, int reg, u16 value) · line 387
Quoted 3 lines
sound/usb/mixer_quirks.c +3 -0
+	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

Takashi Iwai

2010-07-30 12:24 UTC #5 lore raw

In reply to Adrian Pardini

At Tue, 20 Jul 2010 19:42:28 -0300,

Quoted 3 lines · 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.

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

thanks,

Takashi

Quoted 85 lines

Thanks.

Signed-off-by Adrian Pardini <[email protected]>

sound/usb/mixer_quirks.c +16 -0

index e7df1e5..c4cbbc0 100644
@@ -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;

sound/usb/mixer_quirks.h +2 -0

index bdbfab0..bc5d577 100644
@@ -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 */

sound/usb/quirks.c +0 ~~-16~~

index 9a9da09..b8a5a18 100644
@@ -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)
 {
 	/*

Signature · 12 lines

1.5.4.3



-- 
Adrian.
http://elesquinazotango.com.ar
http://www.noalcodigodescioli.blogspot.com/
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Adrian Pardini

author

2010-07-30 14:39 UTC #6 lore raw

Quoted 10 lines · Takashi Iwai

At Tue, 20 Jul 2010 19:42:28 -0300,
Quoted 5 lines · 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.
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.

Signature · 3 lines

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