diff mbox series

usb-audio: Input source control - digidesign mbox

Message ID 20210808050931.59356-1-damien@zamaudio.com
State New
Headers show
Series usb-audio: Input source control - digidesign mbox | expand

Commit Message

Damien Zammit Aug. 8, 2021, 5:09 a.m. UTC 
This adds a second mixer control to Digidesign Mbox
to select between Analog/SPDIF capture.

Users will note that selecting the SPDIF input source
automatically switches the clock mode to sync to SPDIF,
which is a feature of the hardware.

(You can change the clock source back to internal if you want
to capture from spdif but not sync to its clock although this mode
is probably not recommended).

Unfortunately, starting the stream resets both modes
to Internally clocked and Analog input mode.
---
 sound/usb/mixer_quirks.c | 197 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 175 insertions(+), 22 deletions(-)

Comments

Takashi Iwai Aug. 11, 2021, 3:26 p.m. UTC | #1 
On Sun, 08 Aug 2021 07:09:31 +0200,
Damien Zammit wrote:
> 
> This adds a second mixer control to Digidesign Mbox
> to select between Analog/SPDIF capture.
> 
> Users will note that selecting the SPDIF input source
> automatically switches the clock mode to sync to SPDIF,
> which is a feature of the hardware.
> 
> (You can change the clock source back to internal if you want
> to capture from spdif but not sync to its clock although this mode
> is probably not recommended).
> 
> Unfortunately, starting the stream resets both modes
> to Internally clocked and Analog input mode.

Please add your Signed-off-by line.  It's mandatory.

About the code change:
> @@ -596,31 +596,53 @@ static int snd_xonar_u1_controls_create(struct usb_mixer_interface *mixer)
>  
>  /* Digidesign Mbox 1 clock source switch (internal/spdif) */
>  
> -static int snd_mbox1_switch_get(struct snd_kcontrol *kctl,
> -				struct snd_ctl_elem_value *ucontrol)
> +static int snd_mbox1_clk_switch_get(struct snd_kcontrol *kctl,
> +				    struct snd_ctl_elem_value *ucontrol)
>  {
> +	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
> +	struct snd_usb_audio *chip = list->mixer->chip;
> +	unsigned char buff[3];
> +

You need to wrap the execution with snd_usb_lock_shutdown() /
snd_usb_unlock_shutdown().

> +	/* Read clock source */
> +	snd_usb_ctl_msg(chip->dev,
> +				usb_rcvctrlpipe(chip->dev, 0), 0x81,
> +				USB_DIR_IN |
> +				USB_TYPE_CLASS |
> +				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);

Please try to align the indent level.
And, the execution error check is missing.

Also, I believe it'd be wise to make it a helper function, as this is
used in another place, too.  Ditto for other snd_usb_ctl_msg() calls,
too.

> +static int snd_mbox1_src_switch_get(struct snd_kcontrol *kctl,
> +				    struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
> +	struct snd_usb_audio *chip = list->mixer->chip;
> +	unsigned char source[1];

Missing snd_usb_lock_shutdown() / *_unlock_*() here, too.

> +	/* Read input source */
> +	snd_usb_ctl_msg(chip->dev,
> +				usb_rcvctrlpipe(chip->dev, 0), 0x81,
> +				USB_DIR_IN |
> +				USB_TYPE_CLASS |
> +				USB_RECIP_INTERFACE, 0x00, 0x500, source, 1);

Check the error.

> +	kctl->private_value = source[0] - 1;

The value returned from the hardware isn't reliable.  Check whether
it's a valid value.


> +	ucontrol->value.enumerated.item[0] = kctl->private_value;
> +	return 0;
> +}
> +
> +static int snd_mbox1_src_switch_update(struct usb_mixer_interface *mixer, int val)
> +{
> +	struct snd_usb_audio *chip = mixer->chip;
> +	int err;
> +	unsigned char buff[3];
> +	unsigned char source[1];
> +
> +	err = snd_usb_lock_shutdown(chip);
> +	if (err < 0)
> +		return err;
> +
> +	/* Read input source */
> +	err = snd_usb_ctl_msg(chip->dev,
> +				usb_rcvctrlpipe(chip->dev, 0), 0x81,
> +				USB_DIR_IN |
> +				USB_TYPE_CLASS |
> +				USB_RECIP_INTERFACE, 0x00, 0x500, source, 1);
> +	if (err < 0)
> +		goto err;
> +
> +	/* 2 possibilities:	ANALOG Source  -> 0x01
> +	 *			S/PDIF Source  -> 0x02
> +	 * Setting the input source to S/PDIF resets the clock source to S/PDIF
> +	 */
> +	source[0] = (val + 1) & 0xff;

Here better to trust private_value is a valid value, and you can drop
the superfluous "& 0xff".  It's cut down anyway.


thanks,

Takashi
diff mbox series

Patch

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index 0a3cb8fd7..ef771bb3e 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -596,31 +596,53 @@  static int snd_xonar_u1_controls_create(struct usb_mixer_interface *mixer)
 
 /* Digidesign Mbox 1 clock source switch (internal/spdif) */
 
-static int snd_mbox1_switch_get(struct snd_kcontrol *kctl,
-				struct snd_ctl_elem_value *ucontrol)
+static int snd_mbox1_clk_switch_get(struct snd_kcontrol *kctl,
+				    struct snd_ctl_elem_value *ucontrol)
 {
+	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
+	struct snd_usb_audio *chip = list->mixer->chip;
+	unsigned char buff[3];
+
+	/* Read clock source */
+	snd_usb_ctl_msg(chip->dev,
+				usb_rcvctrlpipe(chip->dev, 0), 0x81,
+				USB_DIR_IN |
+				USB_TYPE_CLASS |
+				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
+
+	if (!buff[0] && !buff[1] && !buff[2]) {
+		/* SPDIF synced */
+		kctl->private_value = 1;
+	} else {
+		/* Internally clocked */
+		kctl->private_value = 0;
+	}
+
 	ucontrol->value.enumerated.item[0] = kctl->private_value;
 	return 0;
 }
 
-static int snd_mbox1_switch_update(struct usb_mixer_interface *mixer, int val)
+static int snd_mbox1_clk_switch_update(struct usb_mixer_interface *mixer, int val)
 {
 	struct snd_usb_audio *chip = mixer->chip;
 	int err;
 	unsigned char buff[3];
+	unsigned char source[1];
 
 	err = snd_usb_lock_shutdown(chip);
 	if (err < 0)
 		return err;
 
-	/* Prepare for magic command to toggle clock source */
+	/* Read input source */
 	err = snd_usb_ctl_msg(chip->dev,
 				usb_rcvctrlpipe(chip->dev, 0), 0x81,
 				USB_DIR_IN |
 				USB_TYPE_CLASS |
-				USB_RECIP_INTERFACE, 0x00, 0x500, buff, 1);
+				USB_RECIP_INTERFACE, 0x00, 0x500, source, 1);
 	if (err < 0)
 		goto err;
+
+	/* Read clock source */
 	err = snd_usb_ctl_msg(chip->dev,
 				usb_rcvctrlpipe(chip->dev, 0), 0x81,
 				USB_DIR_IN |
@@ -644,13 +666,15 @@  static int snd_mbox1_switch_update(struct usb_mixer_interface *mixer, int val)
 		buff[0] = buff[1] = buff[2] = 0x00;
 	}
 
-	/* Send the magic command to toggle the clock source */
+	/* Toggle clock source */
 	err = snd_usb_ctl_msg(chip->dev,
 				usb_sndctrlpipe(chip->dev, 0), 0x1,
 				USB_TYPE_CLASS |
 				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
 	if (err < 0)
 		goto err;
+
+	/* Read clock source */
 	err = snd_usb_ctl_msg(chip->dev,
 				usb_rcvctrlpipe(chip->dev, 0), 0x81,
 				USB_DIR_IN |
@@ -671,8 +695,8 @@  static int snd_mbox1_switch_update(struct usb_mixer_interface *mixer, int val)
 	return err;
 }
 
-static int snd_mbox1_switch_put(struct snd_kcontrol *kctl,
-				struct snd_ctl_elem_value *ucontrol)
+static int snd_mbox1_clk_switch_put(struct snd_kcontrol *kctl,
+				    struct snd_ctl_elem_value *ucontrol)
 {
 	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
 	struct usb_mixer_interface *mixer = list->mixer;
@@ -685,12 +709,12 @@  static int snd_mbox1_switch_put(struct snd_kcontrol *kctl,
 		return 0;
 
 	kctl->private_value = new_val;
-	err = snd_mbox1_switch_update(mixer, new_val);
+	err = snd_mbox1_clk_switch_update(mixer, new_val);
 	return err < 0 ? err : 1;
 }
 
-static int snd_mbox1_switch_info(struct snd_kcontrol *kcontrol,
-				 struct snd_ctl_elem_info *uinfo)
+static int snd_mbox1_clk_switch_info(struct snd_kcontrol *kcontrol,
+				     struct snd_ctl_elem_info *uinfo)
 {
 	static const char *const texts[2] = {
 		"Internal",
@@ -700,27 +724,156 @@  static int snd_mbox1_switch_info(struct snd_kcontrol *kcontrol,
 	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);
 }
 
-static int snd_mbox1_switch_resume(struct usb_mixer_elem_list *list)
+static int snd_mbox1_clk_switch_resume(struct usb_mixer_elem_list *list)
+{
+	return snd_mbox1_clk_switch_update(list->mixer, list->kctl->private_value);
+}
+
+/* Digidesign Mbox 1 input source switch (analog/spdif) */
+
+static int snd_mbox1_src_switch_get(struct snd_kcontrol *kctl,
+				    struct snd_ctl_elem_value *ucontrol)
+{
+	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
+	struct snd_usb_audio *chip = list->mixer->chip;
+	unsigned char source[1];
+
+	/* Read input source */
+	snd_usb_ctl_msg(chip->dev,
+				usb_rcvctrlpipe(chip->dev, 0), 0x81,
+				USB_DIR_IN |
+				USB_TYPE_CLASS |
+				USB_RECIP_INTERFACE, 0x00, 0x500, source, 1);
+
+	kctl->private_value = source[0] - 1;
+	ucontrol->value.enumerated.item[0] = kctl->private_value;
+	return 0;
+}
+
+static int snd_mbox1_src_switch_update(struct usb_mixer_interface *mixer, int val)
+{
+	struct snd_usb_audio *chip = mixer->chip;
+	int err;
+	unsigned char buff[3];
+	unsigned char source[1];
+
+	err = snd_usb_lock_shutdown(chip);
+	if (err < 0)
+		return err;
+
+	/* Read input source */
+	err = snd_usb_ctl_msg(chip->dev,
+				usb_rcvctrlpipe(chip->dev, 0), 0x81,
+				USB_DIR_IN |
+				USB_TYPE_CLASS |
+				USB_RECIP_INTERFACE, 0x00, 0x500, source, 1);
+	if (err < 0)
+		goto err;
+
+	/* 2 possibilities:	ANALOG Source  -> 0x01
+	 *			S/PDIF Source  -> 0x02
+	 * Setting the input source to S/PDIF resets the clock source to S/PDIF
+	 */
+	source[0] = (val + 1) & 0xff;
+
+	/* Toggle the input source */
+	err = snd_usb_ctl_msg(chip->dev,
+				usb_sndctrlpipe(chip->dev, 0), 0x1,
+				USB_TYPE_CLASS |
+				USB_RECIP_INTERFACE, 0x00, 0x500, source, 1);
+	if (err < 0)
+		goto err;
+
+	/* Read input source */
+	err = snd_usb_ctl_msg(chip->dev,
+				usb_rcvctrlpipe(chip->dev, 0), 0x81,
+				USB_DIR_IN |
+				USB_TYPE_CLASS |
+				USB_RECIP_INTERFACE, 0x00, 0x500, source, 1);
+	if (err < 0)
+		goto err;
+
+	/* Read clock source */
+	err = snd_usb_ctl_msg(chip->dev,
+				usb_rcvctrlpipe(chip->dev, 0), 0x81,
+				USB_DIR_IN |
+				USB_TYPE_CLASS |
+				USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
+	if (err < 0)
+		goto err;
+err:
+	snd_usb_unlock_shutdown(chip);
+	return err;
+}
+
+static int snd_mbox1_src_switch_put(struct snd_kcontrol *kctl,
+				    struct snd_ctl_elem_value *ucontrol)
+{
+	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
+	struct usb_mixer_interface *mixer = list->mixer;
+	int err;
+	bool cur_val, new_val;
+
+	cur_val = kctl->private_value;
+	new_val = ucontrol->value.enumerated.item[0];
+	if (cur_val == new_val)
+		return 0;
+
+	kctl->private_value = new_val;
+	err = snd_mbox1_src_switch_update(mixer, new_val);
+	return err < 0 ? err : 1;
+}
+
+static int snd_mbox1_src_switch_info(struct snd_kcontrol *kcontrol,
+				     struct snd_ctl_elem_info *uinfo)
+{
+	static const char *const texts[2] = {
+		"Analog",
+		"S/PDIF"
+	};
+
+	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);
+}
+
+static int snd_mbox1_src_switch_resume(struct usb_mixer_elem_list *list)
 {
-	return snd_mbox1_switch_update(list->mixer, list->kctl->private_value);
+	return snd_mbox1_src_switch_update(list->mixer, list->kctl->private_value);
 }
 
-static const struct snd_kcontrol_new snd_mbox1_switch = {
+static const struct snd_kcontrol_new snd_mbox1_clk_switch = {
 	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
 	.name = "Clock Source",
 	.index = 0,
 	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
-	.info = snd_mbox1_switch_info,
-	.get = snd_mbox1_switch_get,
-	.put = snd_mbox1_switch_put,
+	.info = snd_mbox1_clk_switch_info,
+	.get = snd_mbox1_clk_switch_get,
+	.put = snd_mbox1_clk_switch_put,
+	.private_value = 0
+};
+
+static const struct snd_kcontrol_new snd_mbox1_src_switch = {
+	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+	.name = "Input Source",
+	.index = 1,
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
+	.info = snd_mbox1_src_switch_info,
+	.get = snd_mbox1_src_switch_get,
+	.put = snd_mbox1_src_switch_put,
 	.private_value = 0
 };
 
-static int snd_mbox1_create_sync_switch(struct usb_mixer_interface *mixer)
+static int snd_mbox1_controls_create(struct usb_mixer_interface *mixer)
 {
-	return add_single_ctl_with_resume(mixer, 0,
-					  snd_mbox1_switch_resume,
-					  &snd_mbox1_switch, NULL);
+	int err;
+	err = add_single_ctl_with_resume(mixer, 0,
+					 snd_mbox1_clk_switch_resume,
+					 &snd_mbox1_clk_switch, NULL);
+	if (err < 0)
+		return err;
+
+	return add_single_ctl_with_resume(mixer, 1,
+					  snd_mbox1_src_switch_resume,
+					  &snd_mbox1_src_switch, NULL);
 }
 
 /* Native Instruments device quirks */
@@ -3029,7 +3182,7 @@  int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 		break;
 
 	case USB_ID(0x0dba, 0x1000): /* Digidesign Mbox 1 */
-		err = snd_mbox1_create_sync_switch(mixer);
+		err = snd_mbox1_controls_create(mixer);
 		break;
 
 	case USB_ID(0x17cc, 0x1011): /* Traktor Audio 6 */