usb-audio: Add mixer control for Digidesign Mbox 1 clock source

PATCH In review Damien Zammit 2 participants 6 messages Oct 31, 2014 Sound

Share & tools

Damien Zammit

author

2014-10-31 01:57 UTC #0 lore raw

PATCH 1/1 usb-audio: Add mixer control for Digidesign Mbox 1 clock source

This patch provides a mixer control for selecting the clock source of the Digidesign Mbox 1 to either internal clock or S/PDIF external. Trial and error and bus snooping were the only way to get this information, but it works on the hardware.

Quoted 1 line

From d19804aa8b1d3355b647a3209dce4cb5c181ee2b Mon Sep 17 00:00:00 2001

From: Damien Zammit <[email protected]>
Date: Fri, 31 Oct 2014 12:44:25 +1100
Subject: [PATCH] snd-usb-audio: Add mixer control for Digidesign Mbox 1 clock
source

Signed-off-by Damien Zammit <[email protected]>

---
 sound/usb/mixer.h        |   1 +
 sound/usb/mixer_maps.c   |   9 +++
 sound/usb/mixer_quirks.c | 186 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 196 insertions(+)

sound/usb/mixer.h +1 -0

index 73b1f64..5ab6935 100644
@@ -23,6 +23,7 @@ struct usb_mixer_interface {
 
 	u8 audigy2nx_leds[3];
 	u8 xonar_u1_status;
+	u8 mbox1_status;
 };
 
 #define MAX_CHANNELS	16	/* max logical channels */

sound/usb/mixer_maps.c +9 -0

index d1d72ff..9ee3fc2 100644
@@ -179,6 +179,11 @@ static struct usbmix_name_map audigy2nx_map[] = {
 	{ 0 } /* terminator */
 };
 
+static struct usbmix_name_map mbox1_map[] = {
+	{ 1, "Control" },
+	{ 0 } /* terminator */
+};
+
 static struct usbmix_selector_map c400_selectors[] = {
 	{
 		.id = 0x80,

@@ -416,6 +421,10 @@ static struct usbmix_ctl_map usbmix_ctl_maps[] = {
 		.map = aureon_51_2_map,
 	},
 	{
+		.id = USB_ID(0x0dba, 0x1000),
+		.map = mbox1_map,
+	},
+	{
 		.id = USB_ID(0x13e5, 0x0001),
 		.map = scratch_live_map,
 		.ignore_ctl_error = 1,

sound/usb/mixer_quirks.c +186 -0

198 lines not on this page · raw message

Signature · 1 line

1.9.1

Takashi Sakamoto

2014-10-31 03:29 UTC #1 lore raw

Hi Damien,

Quoted 4 lines · Damien Zammit

This patch provides a mixer control for selecting the clock source of the Digidesign Mbox 1 to either internal clock or S/PDIF external. Trial and error and bus snooping were the only way to get this information, but it works on the hardware.

This patch includes lines over 80 characters. Furthermore, these lines include inappropriate white-space for indentation. How about indenting with tab only and add more line-breaks between each function parameter?

$ ./scripts/checkpatch.pl /tmp/0001-mbox1-spdif.patch
WARNING: line over 80 characters
#134: FILE: sound/usb/mixer_quirks.c:641:
+                                     usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,

WARNING: line over 80 characters
#135: FILE: sound/usb/mixer_quirks.c:642:
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,

WARNING: line over 80 characters
#140: FILE: sound/usb/mixer_quirks.c:647:
+ usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,

WARNING: line over 80 characters
#141: FILE: sound/usb/mixer_quirks.c:648:
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT,

WARNING: line over 80 characters
#159: FILE: sound/usb/mixer_quirks.c:666:
+ usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,

WARNING: line over 80 characters
#160: FILE: sound/usb/mixer_quirks.c:667:
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT,

WARNING: line over 80 characters
#165: FILE: sound/usb/mixer_quirks.c:672:
+ usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81,

WARNING: line over 80 characters
#166: FILE: sound/usb/mixer_quirks.c:673:
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT,

total: 0 errors, 8 warnings, 226 lines checked

/tmp/0001-mbox1-spdif.patch has style problems, please review.

If any of these errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.

Regards

Takashi Sakamoto
[email protected]

Damien Zammit

author

2014-10-31 04:02 UTC #2 lore raw

Hi, sorry for the style problems. See attached for better version.

Damien

Quoted 61 lines · Takashi Sakamoto

Hi Damien,
Quoted 4 lines · Damien Zammit
This patch provides a mixer control for selecting the clock source of the Digidesign Mbox 1 to either internal clock or S/PDIF external. Trial and error and bus snooping were the only way to get this information, but it works on the hardware.
This patch includes lines over 80 characters. Furthermore, these lines include inappropriate white-space for indentation. How about indenting with tab only and add more line-breaks between each function parameter?
$ ./scripts/checkpatch.pl /tmp/0001-mbox1-spdif.patch
WARNING: line over 80 characters
#134: FILE: sound/usb/mixer_quirks.c:641:
+                                     usb_rcvctrlpipe(mixer->chip->dev,
0), 0x81,
WARNING: line over 80 characters
#135: FILE: sound/usb/mixer_quirks.c:642:
+ USB_DIR_IN | USB_TYPE_CLASS |
USB_RECIP_INTERFACE,
WARNING: line over 80 characters
#140: FILE: sound/usb/mixer_quirks.c:647:
+ usb_rcvctrlpipe(mixer->chip->dev,
0), 0x81,
WARNING: line over 80 characters
#141: FILE: sound/usb/mixer_quirks.c:648:
+ USB_DIR_IN | USB_TYPE_CLASS |
USB_RECIP_ENDPOINT,
WARNING: line over 80 characters
#159: FILE: sound/usb/mixer_quirks.c:666:
+ usb_rcvctrlpipe(mixer->chip->dev,
0), 0x81,
WARNING: line over 80 characters
#160: FILE: sound/usb/mixer_quirks.c:667:
+ USB_DIR_IN | USB_TYPE_CLASS |
USB_RECIP_ENDPOINT,
WARNING: line over 80 characters
#165: FILE: sound/usb/mixer_quirks.c:672:
+ usb_rcvctrlpipe(mixer->chip->dev,
0), 0x81,
WARNING: line over 80 characters
#166: FILE: sound/usb/mixer_quirks.c:673:
+ USB_DIR_IN | USB_TYPE_CLASS |
USB_RECIP_ENDPOINT,
total: 0 errors, 8 warnings, 226 lines checked
/tmp/0001-mbox1-spdif.patch has style problems, please review.
If any of these errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.

Quoted 5 lines

Regards
Takashi Sakamoto
[email protected]
From 57c714d22b9749be4e3fa3ce5e185832863b60c2 Mon Sep 17 00:00:00 2001

From: Damien Zammit <[email protected]>
Date: Fri, 31 Oct 2014 12:44:25 +1100
Subject: [PATCH] snd-usb-audio: Add mixer control for Digidesign Mbox 1 clock
source

Signed-off-by Damien Zammit <[email protected]>

---
 sound/usb/mixer.h        |   1 +
 sound/usb/mixer_maps.c   |   9 +++
 sound/usb/mixer_quirks.c | 190 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 200 insertions(+)

sound/usb/mixer.h +1 -0

index 73b1f64..5ab6935 100644
@@ -23,6 +23,7 @@ struct usb_mixer_interface {
 
 	u8 audigy2nx_leds[3];
 	u8 xonar_u1_status;
+	u8 mbox1_status;
 };
 
 #define MAX_CHANNELS	16	/* max logical channels */

sound/usb/mixer_maps.c +9 -0

index d1d72ff..9ee3fc2 100644
@@ -179,6 +179,11 @@ static struct usbmix_name_map audigy2nx_map[] = {
 	{ 0 } /* terminator */
 };
 
+static struct usbmix_name_map mbox1_map[] = {
+	{ 1, "Control" },
+	{ 0 } /* terminator */
+};
+
 static struct usbmix_selector_map c400_selectors[] = {
 	{
 		.id = 0x80,

@@ -416,6 +421,10 @@ static struct usbmix_ctl_map usbmix_ctl_maps[] = {
 		.map = aureon_51_2_map,
 	},
 	{
+		.id = USB_ID(0x0dba, 0x1000),
+		.map = mbox1_map,
+	},
+	{
 		.id = USB_ID(0x13e5, 0x0001),
 		.map = scratch_live_map,
 		.ignore_ctl_error = 1,

sound/usb/mixer_quirks.c +190 -0

202 lines not on this page · raw message

Signature · 1 line

1.9.1

Takashi Sakamoto

2014-10-31 04:29 UTC #3 lore raw

Hi Damien,

Quoted 1 line · Damien Zammit

Hi, sorry for the style problems. See attached for better version.

4 points.

1. please use white-spaces or tabs for indentation to align parameters to function-start blacket:

Review

Quoted 8 lines

(quoted diff) +6 -0

+static int snd_mbox1_switch_put(struct snd_kcontrol *kctl,
+                                       struct snd_ctl_elem_value *ucontrol)
 
+static int snd_mbox1_switch_info(struct snd_kcontrol *kcontrol,
+                                       struct snd_ctl_elem_info *uinfo)
 
+static int snd_mbox1_switch_put(struct snd_kcontrol *kctl,
+                                       struct snd_ctl_elem_value *ucontrol)

2.please use 'const char *const' for immutable arrays for immutable strings:

Review

Quoted 4 lines

(quoted diff) +4 -0

+       static const char *texts[2] = {"Internal",
+                                      "S/PDIF"
+       };
+

3. I think in your case, snd_ctl_enum_info() is available in struct snd_kcontrol_new.info() callback. Please read this thread:
[alsa-devel] [PATCH 00/43] Spread usage of snd_ctl_elem_info()
http://mailman.alsa-project.org/pipermail/alsa-devel/2014-October/082573.html

4. I think 'err' local variable in snd_mbox1_create_sync_switch() can be removed because it's assign and evaluated at once:

Review

Quoted 5 lines

(quoted diff) +5 -0

+       err = snd_ctl_add(mixer->chip->card, kctl);
+       if (err < 0)
+               return err;
+
+       return 0;

See attached patch.

Regards

Takashi Sakamoto
[email protected]

Quoted 1 line

From 6e889e1fc7d7101cfef14484849a412b002798e9 Mon Sep 17 00:00:00 2001

From: Takashi Sakamoto <[email protected]>
Date: Fri, 31 Oct 2014 13:17:19 +0900
Subject: [PATCH] Comments for mbox1-spdif-2.patch

See:
[alsa-devel] [PATCH] usb-audio: Add mixer control for Digidesign Mbox 1 clock source
http://mailman.alsa-project.org/pipermail/alsa-devel/2014-October/083264.html

---
 sound/usb/mixer_quirks.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

sound/usb/mixer_quirks.c +8 ~~-20~~

index 6a21dec..87f121f 100644
@@ -574,7 +574,7 @@ struct snd_mbox1_switch_priv_val {
 };
 
 static int snd_mbox1_switch_get(struct snd_kcontrol *kctl,
-					struct snd_ctl_elem_value *ucontrol)
+				struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_mbox1_switch_priv_val *pval;
 	unsigned char value;

@@ -597,7 +597,7 @@ static int snd_mbox1_switch_get(struct snd_kcontrol *kctl,
 }
 
 static int snd_mbox1_switch_put(struct snd_kcontrol *kctl,
-					struct snd_ctl_elem_value *ucontrol)
+				struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_usb_audio *chip;
 	struct snd_mbox1_switch_priv_val *pval;

@@ -692,21 +692,14 @@ static int snd_mbox1_switch_put(struct snd_kcontrol *kctl,
 }
 
 static int snd_mbox1_switch_info(struct snd_kcontrol *kcontrol,
-					struct snd_ctl_elem_info *uinfo)
+				 struct snd_ctl_elem_info *uinfo)
 {
-	static const char *texts[2] = {"Internal",
-				       "S/PDIF"
+	static const char *const texts[2] = {
+		"Internal",
+		"S/PDIF"
 	};
 
-	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
-	uinfo->count = 1;
-	uinfo->value.enumerated.items = 2;
-	if (uinfo->value.enumerated.item > 1)
-		uinfo->value.enumerated.item = 1;
-	strcpy(uinfo->value.enumerated.name,
-		texts[uinfo->value.enumerated.item]);
-
-	return 0;
+	return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);;
 }
 
 static int snd_mbox1_create_sync_switch(struct usb_mixer_interface *mixer)

@@ -722,7 +715,6 @@ static int snd_mbox1_create_sync_switch(struct usb_mixer_interface *mixer)
 		.put = snd_mbox1_switch_put
 	};
 
-	int err;
 	struct snd_kcontrol *kctl;
 	struct snd_mbox1_switch_priv_val *pval;

@@ -741,11 +733,7 @@ static int snd_mbox1_create_sync_switch(struct usb_mixer_interface *mixer)
 		return -ENOMEM;
 	}
 
-	err = snd_ctl_add(mixer->chip->card, kctl);
-	if (err < 0)
-		return err;
-
-	return 0;
+	return snd_ctl_add(mixer->chip->card, kctl);
 }
 
 /* Native Instruments device quirks */

Signature · 1 line

1.9.1

Damien Zammit

author

2014-10-31 04:45 UTC #4 lore raw

Hi Takashi,

Quoted 1 line · Takashi Sakamoto

See attached patch.

I have applied your changes, patch 3 attached.

I am also working on another patch that will provide duplex support with a locked frequency at 48kHz. I already have it working on the hardware but there is some code refactoring required before I can submit it again for review.

Damien

Quoted 1 line

From 39642d6806d8b19612b5e1e95b8538927af1b273 Mon Sep 17 00:00:00 2001

From: Damien Zammit <[email protected]>
Date: Fri, 31 Oct 2014 12:44:25 +1100
Subject: [PATCH] snd-usb-audio: Add mixer control for Digidesign Mbox 1 clock
source

Signed-off-by Damien Zammit <[email protected]>

---
 sound/usb/mixer.h        |   1 +
 sound/usb/mixer_maps.c   |   9 +++
 sound/usb/mixer_quirks.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 187 insertions(+)

sound/usb/mixer.h +1 -0

index 73b1f64..5ab6935 100644
@@ -23,6 +23,7 @@ struct usb_mixer_interface {
 
 	u8 audigy2nx_leds[3];
 	u8 xonar_u1_status;
+	u8 mbox1_status;
 };
 
 #define MAX_CHANNELS	16	/* max logical channels */

sound/usb/mixer_maps.c +9 -0

index d1d72ff..9ee3fc2 100644
@@ -179,6 +179,11 @@ static struct usbmix_name_map audigy2nx_map[] = {
 	{ 0 } /* terminator */
 };
 
+static struct usbmix_name_map mbox1_map[] = {
+	{ 1, "Control" },
+	{ 0 } /* terminator */
+};
+
 static struct usbmix_selector_map c400_selectors[] = {
 	{
 		.id = 0x80,

@@ -416,6 +421,10 @@ static struct usbmix_ctl_map usbmix_ctl_maps[] = {
 		.map = aureon_51_2_map,
 	},
 	{
+		.id = USB_ID(0x0dba, 0x1000),
+		.map = mbox1_map,
+	},
+	{
 		.id = USB_ID(0x13e5, 0x0001),
 		.map = scratch_live_map,
 		.ignore_ctl_error = 1,

sound/usb/mixer_quirks.c +177 -0

189 lines not on this page · raw message

Signature · 1 line

1.9.1

Takashi Sakamoto

2014-10-31 05:13 UTC #5 lore raw

In reply to Takashi Sakamoto

Quoted 6 lines · Takashi Sakamoto

2.please use 'const char *const' for immutable arrays for immutable strings:
Quoted 4 lines
(quoted diff) +4 -0
+       static const char *texts[2] = {"Internal",
+                                      "S/PDIF"
+       };
+

Oops. I mistook to use immutable as 'may not be changed'. It should be 'mutable'...

Regards

Takashi Sakamoto
[email protected]