MIDI driver for Behringer BCD2000 USB device
PATCH 1/1 MIDI driver for Behringer BCD2000 USB device
This patch adds initial support for the Behringer BCD2000 USB DJ controller. At the moment, only the MIDI part of the device is working, i.e. knobs, buttons and LEDs.
I also plan to add support for the audio part, but I assume that this will require more effort than the rather simple MIDI interface. Progress can be tracked at https://github.com/anyc/snd-usb-bcd2000.
--- sound/usb/Kconfig | 14 + sound/usb/Makefile | 2 +- sound/usb/bcd2000/Makefile | 3 + sound/usb/bcd2000/bcd2000.c | 629 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 647 insertions(+), 1 deletion(-) create mode 100644 sound/usb/bcd2000/Makefile create mode 100644 sound/usb/bcd2000/bcd2000.c
sound/usb/Kconfig +14 -0
@@ -146,5 +146,19 @@ config SND_USB_HIFACE To compile this driver as a module, choose M here: the module will be called snd-usb-hiface. +config SND_USB_BCD2000 + tristate "Behringer BCD2000 MIDI driver" + select SND_HWDEP + select SND_RAWMIDI + help + Say Y here to include MIDI support for the Behringer BCD2000 DJ + controller. + + Audio support is still work-in-progress at + https://github.com/anyc/snd-usb-bcd2000 + + To compile this driver as a module, choose M here: the module + will be called snd-usb-bcd2000. + endif # SND_USB
sound/usb/Makefile +1 -1
@@ -23,4 +23,4 @@ obj-$(CONFIG_SND_USB_UA101) += snd-usbmidi-lib.o obj-$(CONFIG_SND_USB_USX2Y) += snd-usbmidi-lib.o obj-$(CONFIG_SND_USB_US122L) += snd-usbmidi-lib.o -obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ +obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/
sound/usb/bcd2000/Makefile new file +3 -0
@@ -0,0 +1,3 @@ +snd-usb-bcd2000-y := bcd2000.o + +obj-$(CONFIG_SND_USB_BCD2000) += snd-usb-bcd2000.o
sound/usb/bcd2000/bcd2000.c new file +629 -0
629 lines not on this page · raw message
Signature · 1 line
1.8.3.2
Hi Mario,
Thanks for your contribution! I'll comment inline.
sound/usb/bcd2000/bcd2000.c line 25
Quoted 11 lines · Mario Kicherer
sound/usb/bcd2000/bcd2000.c new file +5
-0+#ifdef CONFIG_USB_DEBUG +#define DEBUG 1 +#else +#define DEBUG 0 +#endif
Please use the define directly instead of introducing a new one.
sound/usb/bcd2000/bcd2000.c line 54
Quoted 4 lines
sound/usb/bcd2000/bcd2000.c new file +4
-0+MODULE_DEVICE_TABLE (usb, id_table); +MODULE_AUTHOR ("Mario Kicherer, [email protected]" ); +MODULE_DESCRIPTION ("Behringer BCD2000 driver" ); +MODULE_LICENSE ("GPL" );
You can move those lines to the very end of the file, which is slightly more common.
Quoted 4 lines
sound/usb/bcd2000/bcd2000.c new file +4
-0+static int debug =0 ; +module_param (debug, int,S_IRUGO |S_IWUSR ); +MODULE_PARM_DESC (debug,"enable debug output (default: 0)" ); +
sound/usb/bcd2000/bcd2000.c line 69
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+static unsigned char set_led_prefix[] = {0x03 ,0x00 }; + +static unsigned char bcd2000_init_sequence[] = { +0x07 ,0x00 ,0x00 ,0x00 ,0x78 ,0x48 ,0x1c ,0x81 , +0xc4 ,0x00 ,0x00 ,0x00 ,0x5e ,0x53 ,0x4a ,0xf7 , +0x18 ,0xfa ,0x11 ,0xff ,0x6c ,0xf3 ,0x90 ,0xff , +0x00 ,0x00 ,0x00 ,0x00 ,0x01 ,0x00 ,0x00 ,0x00 , +0x18 ,0xfa ,0x11 ,0xff ,0x14 ,0x00 ,0x00 ,0x00 , +0x00 ,0x00 ,0x00 ,0x00 ,0xf2 ,0x34 ,0x4a ,0xf7 , +0x18 ,0xfa ,0x11 ,0xff };
Your patch has a huge number of style issues, which scripts/checkpatch.pl will point you to. I got:
total: 743 errors, 96 warnings, 656 lines checked
Can you fix them up an resend please?
Quoted 33 lines
sound/usb/bcd2000/bcd2000.c new file +33
-0+struct bcd2000 { +struct usb_device *dev; +struct snd_card *card; +struct usb_interface *intf; +int card_index; +struct snd_pcm *pcm; +struct list_head midi_list; +spinlock_t lock; +struct mutex mutex; + +int midi_out_active; +struct snd_rawmidi *rmidi; +struct snd_rawmidi_substream *midi_receive_substream; +struct snd_rawmidi_substream *midi_out_substream; + +unsigned char midi_in_buf[BUFSIZE ]; +unsigned char midi_out_buf[BUFSIZE ]; + +unsigned char midi_cmd_buf[3 ]; +unsigned char midi_cmd_offset; + +struct urb * midi_out_urb; +struct urb * midi_in_urb; +}; + +static int index[SNDRV_CARDS ] =SNDRV_DEFAULT_IDX ; +static char *id[SNDRV_CARDS ] =SNDRV_DEFAULT_STR ; +static bool enable[SNDRV_CARDS ] =SNDRV_DEFAULT_ENABLE_PNP ; + +static DEFINE_MUTEX (devices_mutex); +static unsigned int devices_used; +static struct usb_driver bcd2000_driver; +
...
Quoted 16 lines
sound/usb/bcd2000/bcd2000.c new file +16
-0+static void bcd2000_midi_input_trigger (struct snd_rawmidi_substream *substream,int up) +{ +struct bcd2000 *bcd2k = substream->rmidi->private_data; + +if (!bcd2k) +return ; + + bcd2k->midi_receive_substream = up ? substream :NULL ; +} + +static void bcd2000_midi_handle_input (struct bcd2000 *bcd2k, +const unsigned char *buf,int len) +{ +unsigned char length, cmd, buf_offset, tocopy; +int ret; +
sound/usb/bcd2000/bcd2000.c line 144
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+if (!bcd2k->midi_receive_substream) +return ; + +if (DEBUG || debug) { + printk(KERN_DEBUG PREFIX "received (%d bytes): ", len); +for (ret=0 ; ret < len; ret++) { +printk ("%x " , buf[ret]); + } +printk ("\n" ); + }
We have kernel functions in lib/hexdump.c for this.
Quoted 34 lines
sound/usb/bcd2000/bcd2000.c new file +34
-0+ +if (len <2 ) +return ; + +/* +* The BCD2000 sends MIDI commands with 2 or 3 bytes length. In case of +* 2 bytes, the command is the same as before. +* +* Packet structure: mm nn oo (pp) +* mm: payload length +* nn: MIDI command or note +* oo: note or velocity +* pp: velocity +*/ + + length = buf[0 ]; + +/* ignore packets without payload, should not happen */ +if (length ==0 ) +return ; + +/* +* The MIDI packets the BCD2000 sends can be arbitrarily truncated or +* concatenated. Therefore, this loop accumulates the bytes from the +* input buffer until a full valid MIDI packet is in the MIDI command +* buffer "midi_cmd_buf". +*/ + + buf_offset =1 ; +while (buf_offset <= length) { + cmd = buf[buf_offset]; + +if (bcd2k->midi_cmd_offset ==0 && cmd !=0x90 && cmd !=0xb0 ) { +/*
sound/usb/bcd2000/bcd2000.c line 188
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+* this is a 2-byte midi packet -> reuse command byte from +* last midi packet +*/ + bcd2k->midi_cmd_offset =1 ; + } + +/* determine the number of bytes we want to copy this time */ + tocopy =min (3 - bcd2k->midi_cmd_offset, length - (buf_offset -1 )); + +if (tocopy >0 ) {
tocopy is unsigned char, so this check is always true, and the copy routine can overflow in case bcd2k->midi_cmd_offset < 3.
Quoted 8 lines
sound/usb/bcd2000/bcd2000.c new file +8
-0+memcpy (&bcd2k->midi_cmd_buf[bcd2k->midi_cmd_offset], + &buf[buf_offset], tocopy); + }else + printk(KERN_ERR PREFIX "error parsing\n"); + + bcd2k->midi_cmd_offset += tocopy; + buf_offset += tocopy; +
sound/usb/bcd2000/bcd2000.c line 206
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+/* is our MIDI packet complete? */ +if (bcd2k->midi_cmd_offset ==3 ) { +if (DEBUG || debug) { + printk(KERN_DEBUG PREFIX "send midi packet (%d bytes): ", + bcd2k->midi_cmd_offset); +for (ret=0 ;ret< bcd2k->midi_cmd_offset;ret++) { +printk ("%x " , bcd2k->midi_cmd_buf[ret]); + } +printk ("\n" ); + }
See above.
sound/usb/bcd2000/bcd2000.c line 238
Quoted 7 lines
sound/usb/bcd2000/bcd2000.c new file +7
-0+static void bcd2000_midi_send (struct bcd2000 *bcd2k, +struct snd_rawmidi_substream *substream) +{ +int len, ret; + +/* copy the "set LED" command bytes */ +memcpy (bcd2k->midi_out_buf, set_led_prefix,sizeof (set_led_prefix));
Add a BUILD_BUG_ON() here to make sure sizeof(set_led_prefix) is never bigger than you static buffer.
sound/usb/bcd2000/bcd2000.c line 256
Quoted 7 lines
sound/usb/bcd2000/bcd2000.c new file +7
-0+if (DEBUG || debug) { + printk(KERN_DEBUG PREFIX "sending: "); + for (ret=0;ret< 3 + len;ret++) { +printk ("%x " , bcd2k->midi_out_buf[ret]); + } +printk ("\n" ); + }
See above.
Quoted 6 lines
sound/usb/bcd2000/bcd2000.c new file +6
-0+/* send packet to the BCD2000 */ + ret =usb_submit_urb (bcd2k->midi_out_urb,GFP_ATOMIC ); +if (ret <0 ) { + printk(KERN_ERR PREFIX +"bcd2000_midi_send(%p): usb_submit_urb() failed," + "ret=%d, len=%d: ", substream, ret, len);
sound/usb/bcd2000/bcd2000.c line 273
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+switch (ret) { +case -ENOMEM :printk ("ENOMEM\n" );break ; +case -ENODEV :printk ("ENODEV\n" );break ; +case -ENXIO :printk ("ENXIO\n" );break ; +case -EINVAL :printk ("EINVAL\n" );break ; +case -EAGAIN :printk ("EAGAIN\n" );break ; +case -EFBIG :printk ("EFBIG\n" );break ; +case -EPIPE :printk ("EPIPE\n" );break ; +case -EMSGSIZE :printk ("EMSGSIZE\n" );break ; +default :printk ("\n" );
That you'll fix when following the checkpatch.pl complaints :)
sound/usb/bcd2000/bcd2000.c line 286
Quoted 3 lines
sound/usb/bcd2000/bcd2000.c new file +3
-0+ bcd2k->midi_out_active =0 ; +if (urb->status !=0 ) { + printk(KERN_ERR PREFIX "urb status: %d\n", urb->status);
Try to avoid printk() and use snd_printk() or dev_err() instead.
...
Quoted 6 lines
sound/usb/bcd2000/bcd2000.c new file +6
-0+/* +* general part +*/ + +static void bcd2000_free_usb_related_resources (struct bcd2000 *bcd2k, +struct usb_interface *interface)
sound/usb/bcd2000/bcd2000.c line 443
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+{ +unsigned int i; +struct usb_interface *intf; + +// mutex_lock(&ua->mutex); +// free_stream_urbs(&ua->capture); +// free_stream_urbs(&ua->playback); +// mutex_unlock(&ua->mutex); +// free_stream_buffers(ua, &ua->capture); +// free_stream_buffers(ua, &ua->playback);
Please, no dead code. Remove those lines entirely if you don't need them.
sound/usb/bcd2000/bcd2000.c line 445
Quoted 1 line
sound/usb/bcd2000/bcd2000.c new file +1
-0+for (i =0 ; i <1 ; ++i) {
Why do you need a loop if it only runs once!?
Quoted 7 lines
sound/usb/bcd2000/bcd2000.c new file +7
-0+mutex_lock (&bcd2k->mutex); + intf = bcd2k->intf; + bcd2k->intf =NULL ; +mutex_unlock (&bcd2k->mutex); +if (intf) { +usb_set_intfdata (intf,NULL ); +if (intf != interface)
sound/usb/bcd2000/bcd2000.c line 462
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+usb_driver_release_interface (&bcd2000_driver, + intf); + } + } +} + +static void bcd2000_card_free (struct snd_card *card) +{ +// struct bcd2000 *bcd2k = card->private_data; +// mutex_destroy(&ua->mutex);
Remove dead code.
Quoted 31 lines
sound/usb/bcd2000/bcd2000.c new file +31
-0+} + +static int bcd2000_probe (struct usb_interface *interface,const struct usb_device_id *usb_id) +{ +struct snd_card *card; +struct bcd2000 * bcd2k; +unsigned int card_index; +char usb_path[32 ]; +int err; + + +mutex_lock (&devices_mutex); + +for (card_index =0 ; card_index <SNDRV_CARDS ; ++card_index) +if (enable[card_index] && !(devices_used & (1 << card_index))) +break ; +if (card_index >=SNDRV_CARDS ) { +mutex_unlock (&devices_mutex); +return -ENOENT ; + } + err =snd_card_create (index[card_index], id[card_index],THIS_MODULE , +sizeof (*bcd2k), &card); +if (err <0 ) { +mutex_unlock (&devices_mutex); +return err; + } + card->private_free = bcd2000_card_free; + bcd2k = card->private_data; + bcd2k->dev =interface_to_usbdev (interface); + bcd2k->card = card; + bcd2k->card_index = card_index;
sound/usb/bcd2000/bcd2000.c line 503
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+INIT_LIST_HEAD (&bcd2k->midi_list); +spin_lock_init (&bcd2k->lock); +mutex_init (&bcd2k->mutex); + +// INIT_LIST_HEAD(&bcd2k->ready_playback_urbs); +// tasklet_init(&bcd2k->playback_tasklet, +// playback_tasklet, (unsigned long)bcd2k); +// init_waitqueue_head(&bcd2k->alsa_capture_wait); +// init_waitqueue_head(&bcd2k->rate_feedback_wait); +// init_waitqueue_head(&bcd2k->alsa_playback_wait);
Dito.
Quoted 30 lines
sound/usb/bcd2000/bcd2000.c new file +30
-0+ bcd2k->intf = interface; + +snd_card_set_dev (card, &interface->dev); + +strcpy (card->driver,DEVICE_NAME ); +strcpy (card->shortname,DEVICE_SHORTNAME ); +usb_make_path (bcd2k->dev, usb_path,sizeof (usb_path)); +snprintf (bcd2k->card->longname,sizeof (bcd2k->card->longname), +DEVICE_NAME ", at %s, %s speed" , + usb_path, + bcd2k->dev->speed ==USB_SPEED_HIGH ?"high" :"full" ); + + printk(KERN_INFO PREFIX "%s", bcd2k->card->longname); ++ // err = alloc_stream_buffers(bcd2k, &bcd2k->capture); +// if (err < 0) +// goto probe_error; +// err = alloc_stream_buffers(bcd2k, &bcd2k->playback); +// if (err < 0) +// goto probe_error; +// +// err = alloc_stream_urbs(bcd2k, &bcd2k->capture, capture_urb_complete); +// if (err < 0) +// goto probe_error; +// err = alloc_stream_urbs(bcd2k, &bcd2k->playback, playback_urb_complete); +// if (err < 0) +// goto probe_error; +// +// err = snd_pcm_new(card, "bcd2000", 0, 1, 1, &bcd2k->pcm); +// if (err < 0)
sound/usb/bcd2000/bcd2000.c line 544
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+// goto probe_error; +// bcd2k->pcm->private_data = bcd2k; +// strcpy(bcd2k->pcm->name, "bcd2000"); +// snd_pcm_set_ops(bcd2k->pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_pcm_ops); +// snd_pcm_set_ops(bcd2k->pcm, SNDRV_PCM_STREAM_CAPTURE, &capture_pcm_ops); + +// err = snd_usbmidi_create(card, bcd2k->intf, +// &bcd2k->midi_list, &midi_quirk); +// if (err < 0) +// goto probe_error;
Dito.
I'll do another review once those issues are addressed.
Thanks,
Daniel
Hi Daniel,
thank you very much for your comments! I hope I fixed most of the points but I also have a few questions/comments:
Quoted 7 lines · Daniel Mack
sound/usb/bcd2000/bcd2000.c line 25
Quoted 5 lines
sound/usb/bcd2000/bcd2000.c new file +5
-0+#ifdef CONFIG_USB_DEBUG +#define DEBUG 1 +#else +#define DEBUG 0 +#endif Please use the define directly instead of introducing a new one.
I replaced it with:
#ifndef CONFIG_USB_DEBUG
#define CONFIG_USB_DEBUG 0
#endif
As it can be undefined. I hope that's okay.
Quoted 2 lines
Your patch has a huge number of style issues, which
scripts/checkpatch.plwill point you to. I got:
Interesting, I copied many issues from other modules. :) If fixed most of them except these:
WARNING: quoted string split across lines
#310: FILE: sound/usb/bcd2000/bcd2000.c:254:
+ "bcd2000_midi_send(%p): usb_submit_urb() failed"
+ "ret=%d, len=%d\n", substream, ret, len);
WARNING: quoted string split across lines
#392: FILE: sound/usb/bcd2000/bcd2000.c:336:
+ "bcd2000_init_device: usb_submit_urb() failed,"
+ "ret=%d: ", ret);
I don't know how to avoid them, as all strings in one line would be longer than 80 characters.
Quoted 7 lines
sound/usb/bcd2000/bcd2000.c line 188
Quoted 4 lines
sound/usb/bcd2000/bcd2000.c new file +4
-0+/* determine the number of bytes we want to copy this time */ + tocopy =min (3 - bcd2k->midi_cmd_offset, length - (buf_offset -1 )); + +if (tocopy >0 ) {tocopy is unsigned char, so this check is always true, and the copy routine can overflow in case
bcd2k->midi_cmd_offset< 3.
I don't understand your point here. It could be zero. But I changed it to a condition that checks if both offsets are within the length of the two involved buffers.
Quoted 1 line
Please, no dead code. Remove those lines entirely if you don't need them.
Ah okay, I planned to reactive them with the audio part. But I removed them for now.
As far as I understood, I should repost the new patch as [PATCH v2]. I will check this evening if the device still works and send it to the list afterwards.
Thank you!
Mario
Quoted 15 lines · Mario Kicherer
Hi Daniel,
thank you very much for your comments! I hope I fixed most of the points but I also have a few questions/comments:
Quoted 7 lines · Daniel Mack
sound/usb/bcd2000/bcd2000.c line 25
Quoted 5 lines
sound/usb/bcd2000/bcd2000.c new file +5
-0+#ifdef CONFIG_USB_DEBUG +#define DEBUG 1 +#else +#define DEBUG 0 +#endif Please use the define directly instead of introducing a new one.
Quoted 5 lines
I replaced it with:
#ifndef
CONFIG_USB_DEBUG
#defineCONFIG_USB_DEBUG0
#endif
No, don't fiddle with CONFIG_* symbols. What I mean is: use the CONFIG_ macro in your code instead if 'DEBUG'. And also, maybe use CONFIG_SND_DEBUG instead.
Quoted 1 line
As it can be undefined. I hope that's okay.
Ah, so I'd recommend you move code that depends on this define into a separate function, and stub it out with a no-op if it's not defined. Something like this:
#ifdef CONFIG_SND_DEBUG
static void foo_dump()
{
...
}#else
static inline void foo_dump() {}
#endif
...
{
...
foo_dump();
...
}It makes to code more readable if there are less #ifdef.
Quoted 4 lines
Quoted 2 lines
Your patch has a huge number of style issues, which
scripts/checkpatch.plwill point you to. I got:Interesting, I copied many issues from other modules. :)
You're welcome to send patches to fix them :)
A general rule is: "Do as we say, not as we do".
Quoted 7 lines
If fixed most of them except these:
WARNING: quoted string split across lines
#310: FILE:sound/usb/bcd2000/bcd2000.c:254:(quoted diff) +2
-0+ "bcd2000_midi_send(%p): usb_submit_urb() failed" + "ret=%d, len=%d\n", substream, ret, len);
Quoted 8 lines
WARNING: quoted string split across lines
#392: FILE:sound/usb/bcd2000/bcd2000.c:336:
+ "bcd2000_init_device:usb_submit_urb()
failed,"
+ "ret=%d: ", ret);I don't know how to avoid them, as all strings in one line would be longer than 80 characters.
Yep, I'd say that's okay. I'd prefer non-split string over the 80-char rule. However, consider using __func__ rather than the manual function name copy; that might solve your problem as well.
Quoted 9 lines
Quoted 7 lines
sound/usb/bcd2000/bcd2000.c line 188
Quoted 4 lines
sound/usb/bcd2000/bcd2000.c new file +4
-0+/* determine the number of bytes we want to copy this time */ + tocopy =min (3 - bcd2k->midi_cmd_offset, length - (buf_offset -1 )); + +if (tocopy >0 ) {tocopy is unsigned char, so this check is always true, and the copy routine can overflow in case
bcd2k->midi_cmd_offset< 3.I don't understand your point here. It could be zero.
Yes, I'm sorry. I was looking at the '< 0' condition that can occur if 'bcd2k->midi_cmd_offset < 3'.
Quoted 3 lines
But I changed it
to a condition that checks if both offsets are within the length of the two involved buffers.
Great. Please also double-check whether a maliciously behaving device could cause any harm to your buffer logic. It's easy to hack a simple microcontroller to match the driver and then through random stuff at your routines. Your code must be able to cope with everything it is fed to.
Quoted 6 lines
Quoted 1 line
Please, no dead code. Remove those lines entirely if you don't need them.
Ah okay, I planned to reactive them with the audio part. But I removed them for now.
As far as I understood, I should repost the new patch as [PATCH v2].
Correct.
Quoted 3 lines
I
will check this evening if the device still works and send it to the list afterwards.
Thanks,
Daniel