MIDI driver for Behringer BCD2000 USB device
PATCH v2 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.
Changes since v1:
- fixed the various code style issues, thanks to Daniel Mack and checkpatch.pl.
--- sound/usb/Kconfig | 14 ++ sound/usb/Makefile | 2 +- sound/usb/bcd2000/Makefile | 3 + sound/usb/bcd2000/bcd2000.c | 553 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 571 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 +553 -0
553 lines not on this page · raw message
Signature · 1 line
1.8.3.2
Quoted 11 lines · Mario Kicherer
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.
Changes since v1:
- fixed the various code style issues, thanks to Daniel Mack and checkpatch.pl.
Hmm, we're getting there :) But I still see:
total: 571 errors, 6 warnings, 580 lines checked
Mostly due to:
ERROR: DOS line endings
Did you send the patch using git send-email?
Also, please have a quick look at Documentation/CodingStyle.
Quoted 20 lines
sound/usb/bcd2000/bcd2000.c new file +17
-0@@ -0,0 +1,553 @@ +/* +* Behringer BCD2000 driver +* +* Copyright (C) 2014 Mario Kicherer ([email protected]) +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/kernel.h>
sound/usb/bcd2000/bcd2000.c line 27
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+#include <linux/errno.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/usb.h> +#include <linux/usb/audio.h> +#include <sound/core.h> +#include <sound/initval.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h>
As long as you don't need them, skip the pcm includes.
Quoted 6 lines
sound/usb/bcd2000/bcd2000.c new file +6
-0+#include <sound/rawmidi.h> +#include "../usbaudio.h" +#include "../midi.h" + +#define DEVICE_NAME "Behringer BCD2000" +#define DEVICE_SHORTNAME "bcd2000"
sound/usb/bcd2000/bcd2000.c line 43
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+ +#define PREFIX DEVICE_SHORTNAME ": " +#define BUFSIZE 64 + +static struct usb_device_id id_table[] = { + {USB_DEVICE (0x1397 ,0x00bd ) }, + { }, +}; + +static int debug;
Hmm, what about the idea of moving the hex_dump function into an own helper? See below ...
sound/usb/bcd2000/bcd2000.c line 56
Quoted 13 lines
sound/usb/bcd2000/bcd2000.c new file +13
-0+module_param (debug, int,S_IRUGO |S_IWUSR ); +MODULE_PARM_DESC (debug,"enable debug output (default: 0)" ); + +static unsigned char device_cmd_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 };
'};' should be in a new line.
sound/usb/bcd2000/bcd2000.c line 63
Quoted 6 lines
sound/usb/bcd2000/bcd2000.c new file +6
-0+struct bcd2000 { +struct usb_device *dev; +struct snd_card *card; +struct usb_interface *intf; +int card_index; +struct snd_pcm *pcm;
Unused?
sound/usb/bcd2000/bcd2000.c line 65
Quoted 2 lines
sound/usb/bcd2000/bcd2000.c new file +2
-0+struct list_head midi_list; +spinlock_t lock;
Unused?
sound/usb/bcd2000/bcd2000.c line 66
Quoted 1 line
sound/usb/bcd2000/bcd2000.c new file +1
-0+struct mutex mutex;
That one's also kinda unused (see below)
...
sound/usb/bcd2000/bcd2000.c line 94
Quoted 8 lines
sound/usb/bcd2000/bcd2000.c new file +8
-0+static DEFINE_MUTEX (devices_mutex); +static unsigned int devices_used; +static struct usb_driver bcd2000_driver; + + + + +
Kill excessive newlines here.
sound/usb/bcd2000/bcd2000.c line 128
Quoted 12 lines
sound/usb/bcd2000/bcd2000.c new file +12
-0+static void bcd2000_midi_handle_input (struct bcd2000 *bcd2k, +const unsigned char *buf,int len) +{ +unsigned char length, cmd, buf_offset, tocopy; + +if (!bcd2k->midi_receive_substream) +return ; + +if (debug) { +print_hex_dump (KERN_DEBUG ,"received from device: " , +DUMP_PREFIX_NONE ,16 ,1 , buf, len, false); + }
You could add something like this:
#ifdef CONFIG_SND_DEBUG
static void bcd2000_midi_hex_dump(const char *prefix,
const unsigned char *buf, int len)
{
print_hex_dump(KERN_DEBUG, prefix,
DUMP_PREFIX_NONE, 16, 1, buf, len, false);
}
#else
static inline void bcd2000_midi_hex_dump(const char *prefix,
const unsigned char *buf, int len) {}#endif
And then just call the function from various places. That would safe you the module parameter, the condition check, and an indentation level.
Quoted 42 lines
sound/usb/bcd2000/bcd2000.c new file +42
-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 */ +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 ) { +/* +* 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,
sound/usb/bcd2000/bcd2000.c line 180
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+ length - (buf_offset - 1)); + +/* safety check */ +if (bcd2k->midi_cmd_offset + tocopy <BUFSIZE && + buf_offset + tocopy < len) { +memcpy (&bcd2k->midi_cmd_buf[bcd2k->midi_cmd_offset], + &buf[buf_offset], tocopy); + }else { + snd_printk(KERN_ERR PREFIX "access violation in %s\n", + __func__);
You want to drop the entire packet here, right?
Quoted 4 lines
sound/usb/bcd2000/bcd2000.c new file +4
-0+ } + + bcd2k->midi_cmd_offset += tocopy; + buf_offset += tocopy;
sound/usb/bcd2000/bcd2000.c line 194
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) { +print_hex_dump (KERN_DEBUG , +"sending to userspace: " , +DUMP_PREFIX_NONE ,16 ,1 , + bcd2k->midi_cmd_buf, + bcd2k->midi_cmd_offset, false); + }
{} braces are not needed when a block only holds one instruction. See Documentation/CodingStyle.
sound/usb/bcd2000/bcd2000.c line 212
Quoted 3 lines
sound/usb/bcd2000/bcd2000.c new file +3
-0+static int bcd2000_midi_output_close (struct snd_rawmidi_substream *substream) +{ +struct bcd2000 *bcd2k = substream->rmidi->private_data;
Newline here, and elsewhere in your code, after variable declarations.
Quoted 29 lines
sound/usb/bcd2000/bcd2000.c new file +29
-0+if (bcd2k->midi_out_active) { +usb_free_urb (bcd2k->midi_out_urb); + bcd2k->midi_out_active =0 ; + } +return 0 ; +} + +static void bcd2000_midi_send (struct bcd2000 *bcd2k, +struct snd_rawmidi_substream *substream) +{ +int len, ret; + +BUILD_BUG_ON (sizeof (device_cmd_prefix) >=BUFSIZE ); +/* copy the "set LED" command bytes */ +memcpy (bcd2k->midi_out_buf, device_cmd_prefix, +sizeof (device_cmd_prefix)); + +/* +* get MIDI packet and leave space for command prefix +* and payload length +*/ + len =snd_rawmidi_transmit (substream, + bcd2k->midi_out_buf +3 ,BUFSIZE -3 ); + +if (len <=0 ) +return ; + +/* set payload length */ + bcd2k->midi_out_buf[2 ] = len;
sound/usb/bcd2000/bcd2000.c line 251
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+ bcd2k->midi_out_urb->transfer_buffer_length =BUFSIZE ; + +if (debug) { +print_hex_dump (KERN_DEBUG ,"sending to device: " , +DUMP_PREFIX_NONE ,16 ,1 , + bcd2k->midi_out_buf, len+3 , false); + } + +/* send packet to the BCD2000 */ + ret =usb_submit_urb (bcd2k->midi_out_urb,GFP_ATOMIC );
This function is not called from atomic context, so GFP_KERNEL is ok.
sound/usb/bcd2000/bcd2000.c line 258
Quoted 7 lines
sound/usb/bcd2000/bcd2000.c new file +7
-0+if (ret <0 ) { +dev_err (&bcd2k->dev->dev,PREFIX +"%s (%p): usb_submit_urb() failed" +"ret=%d, len=%d\n" , __func__, substream, ret, len); + }else { + bcd2k->midi_out_active =1 ; + }
See above.
Quoted 18 lines
sound/usb/bcd2000/bcd2000.c new file +18
-0+} + +static void bcd2000_midi_output_done (struct urb *urb) +{ +struct bcd2000 *bcd2k = urb->context; + + bcd2k->midi_out_active =0 ; +if (urb->status !=0 ) { +dev_err (&urb->dev->dev,PREFIX "urb status: %d\n" , urb->status); +return ; + } + +if (!bcd2k->midi_out_substream) +return ; + +bcd2000_midi_send (bcd2k, bcd2k->midi_out_substream); +} +
...
sound/usb/bcd2000/bcd2000.c line 400
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c new file +10
-0+static void bcd2000_close_midi (struct bcd2000 *bcd2k) +{ +usb_free_urb (bcd2k->midi_out_urb); +usb_free_urb (bcd2k->midi_in_urb); +} + + + + +
Too many newlines :)
sound/usb/bcd2000/bcd2000.c line 413
Quoted 13 lines
sound/usb/bcd2000/bcd2000.c new file +13
-0+/* +* general part +*/ + +static void bcd2000_free_usb_related_resources (struct bcd2000 *bcd2k, +struct usb_interface *interface) +{ +struct usb_interface *intf; + +mutex_lock (&bcd2k->mutex); + intf = bcd2k->intf; + bcd2k->intf =NULL ; +mutex_unlock (&bcd2k->mutex);
What are you trying to lock here, and against what? Without another call site, this mutex is futile.
Thanks,
Daniel
At Wed, 05 Feb 2014 23:32:23 +0100,
Quoted 14 lines · Daniel Mack
Quoted 11 lines · Mario Kicherer
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.
Changes since v1:
- fixed the various code style issues, thanks to Daniel Mack and checkpatch.pl.
Quoted 7 lines
Hmm, we're getting there :) But I still see:
total: 571 errors, 6 warnings, 580 lines checked
Mostly due to:
ERROR: DOS line endings
This must depend how you extract the mail content.
I can save the mail as a Unix plain text, and checkpatch complains only two:
================================================================
WARNING: quoted string split across lines
#400: FILE: sound/usb/bcd2000/bcd2000.c:255:
+ "%s (%p): usb_submit_urb() failed"
+ "ret=%d, len=%d\n", __func__, substream, ret, len);
WARNING: quoted string split across lines
#482: FILE: sound/usb/bcd2000/bcd2000.c:337:
+ "%s: usb_submit_urb() failed,"
+ "ret=%d: ", __func__, ret);
total: 0 errors, 2 warnings, 580 lines checked
================================================================
Actually, these two should be fixed. Better to keep the string in a single line than breaking 80 chars limit, in general. This would make it easier to work with grep.
Takashi
Mario Kicherer
AppliedThanks, applied most fixes. Further points:
Quoted 5 lines · Daniel Mack
Mostly due to:
ERROR: DOS line endings
Did you send the patch using git send-email?
Yes, I only get the two warnings as Takashi. I also moved the strings in one line as he suggested.
Quoted 9 lines
sound/usb/bcd2000/bcd2000.c line 63
Quoted 6 lines
sound/usb/bcd2000/bcd2000.c new file +6
-0+struct bcd2000 { +struct usb_device *dev; +struct snd_card *card; +struct usb_interface *intf; +int card_index; +struct snd_pcm *pcm;Unused?
Quoted 8 lines
sound/usb/bcd2000/bcd2000.c line 65
Quoted 2 lines
sound/usb/bcd2000/bcd2000.c new file +2
-0+struct list_head midi_list; +spinlock_t lock;Unused?
sound/usb/bcd2000/bcd2000.c line 66
Quoted 1 line
sound/usb/bcd2000/bcd2000.c new file +1
-0+struct mutex mutex;That one's also kinda unused (see below)
You're right. The original module had multiple interfaces. Removed.
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c line 94
Quoted 8 lines
sound/usb/bcd2000/bcd2000.c new file +8
-0+static DEFINE_MUTEX (devices_mutex); +static unsigned int devices_used; +static struct usb_driver bcd2000_driver; + + + + +Kill excessive newlines here.
Okay, removed. I find little gaps useful to separate logical parts.
Quoted 12 lines
You could add something like this:
#ifdef
CONFIG_SND_DEBUG
static void bcd2000_midi_hex_dump(const char *prefix,
const unsigned char *buf, int len){ print_hex_dump(KERN_DEBUG, prefix, DUMP_PREFIX_NONE, 16, 1, buf, len, false); } #else static inline void bcd2000_midi_hex_dump(const char *prefix, const unsigned char *buf, int len) {}
Quoted 4 lines
#endif
And then just call the function from various places. That would safe you the module parameter, the condition check, and an indentation level.
Hm, I removed the CONFIG_ part as the module parameter would allow me and possible users to debug things spontaneously. But I can use our approach if you think it would be better nonetheless?
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c line 180
Quoted 8 lines
sound/usb/bcd2000/bcd2000.c new file +8
-0+/* safety check */ +if (bcd2k->midi_cmd_offset + tocopy <BUFSIZE && + buf_offset + tocopy < len) { +memcpy (&bcd2k->midi_cmd_buf[bcd2k->midi_cmd_offset], + &buf[buf_offset], tocopy); + }else { + snd_printk(KERN_ERR PREFIX "access violation in %s\n", + __func__);You want to drop the entire packet here, right?
Hm, I'm not sure. A "return" after the snd_printk would do, right?
Thank you very much!
Mario
Quoted 8 lines · Mario Kicherer
Quoted 3 lines · Daniel Mack
ERROR: DOS line endings
Did you send the patch using git send-email?
Yes, I only get the two warnings as Takashi. I also moved the strings in one line as he suggested.
As Takashi pointed out, that might be my bad. I'm puzzled though, as I didn't change my procedure.
Quoted 14 lines
Quoted 14 lines
You could add something like this:
#ifdef
CONFIG_SND_DEBUG
static void bcd2000_midi_hex_dump(const char *prefix,
const unsigned char *buf, int len){ print_hex_dump(KERN_DEBUG, prefix, DUMP_PREFIX_NONE, 16, 1, buf, len, false); } #else static inline void bcd2000_midi_hex_dump(const char *prefix, const unsigned char *buf, int len) {}#endif
Quoted 6 lines
Quoted 2 lines
And then just call the function from various places. That would safe you the module parameter, the condition check, and an indentation level.
Hm, I removed the
CONFIG_part as the module parameter would allow me and possible users to debug things spontaneously. But I can use our approach if you think it would be better nonetheless?
I think so. Less debug conditionals in the code usually result in better readability. But it's a nit really.
Quoted 11 lines
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c line 180
Quoted 8 lines
sound/usb/bcd2000/bcd2000.c new file +8
-0+/* safety check */ +if (bcd2k->midi_cmd_offset + tocopy <BUFSIZE && + buf_offset + tocopy < len) { +memcpy (&bcd2k->midi_cmd_buf[bcd2k->midi_cmd_offset], + &buf[buf_offset], tocopy); + }else { + snd_printk(KERN_ERR PREFIX "access violation in %s\n", + __func__);You want to drop the entire packet here, right?
Quoted 1 line
Hm, I'm not sure. A "return" after the
snd_printkwould do, right?
Yes, that should do.
Daniel
sound/usb/Kconfig config SND_USB_HIFACE · line 151
Quoted 7 lines · Mario Kicherer
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.
sound/usb/Kconfig +3
-0+config SND_USB_BCD2000 + tristate "Behringer BCD2000 MIDI driver" + select SND_HWDEP
Why HWDEP?
sound/usb/Kconfig config SND_USB_HIFACE · line 161
Quoted 2 lines
sound/usb/Kconfig +2
-0+ To compile this driver as a module, choose M here: the module + will be called snd-usb-bcd2000.
There is no need to have "usb" in the module name if that is the only bus this device works with.
sound/usb/Makefile obj-$(CONFIG_SND_USB_UA101) += snd-usbmidi-lib.o · line 26
Quoted 2 lines
sound/usb/Makefile +1
-1-obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ +obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/
If there is only a single source file, just put it into misc/. If you plan to have multiple source files, then this file should have "_midi" in its name.
sound/usb/bcd2000/bcd2000.c line 30
Quoted 2 lines
sound/usb/bcd2000/bcd2000.c new file +2
-0+#include "../usbaudio.h" +#include "../midi.h"
Are these headers needed?
sound/usb/bcd2000/bcd2000.c line 112
Quoted 7 lines
sound/usb/bcd2000/bcd2000.c new file +7
-0+static void bcd2000_midi_input_trigger (struct snd_rawmidi_substream *substream, +int up) +{ +struct bcd2000 *bcd2k = substream->rmidi->private_data; + +if (!bcd2k) +return ;
This cannot happen. And if it happens anyway, silently returning is not necessarily a better option than crashing.
sound/usb/bcd2000/bcd2000.c line 155
Quoted 8 lines
sound/usb/bcd2000/bcd2000.c new file +8
-0+static void bcd2000_midi_handle_input (struct bcd2000 *bcd2k, +const unsigned char *buf,int len) +/* +* 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". +*/
ALSA does not require full MIDI commands, and handles running status.
sound/usb/bcd2000/bcd2000.c line 214
Quoted 5 lines
sound/usb/bcd2000/bcd2000.c new file +5
-0+static int bcd2000_midi_output_close (struct snd_rawmidi_substream *substream) +{ +struct bcd2000 *bcd2k = substream->rmidi->private_data; +if (bcd2k->midi_out_active) { +usb_free_urb (bcd2k->midi_out_urb);
Why is this the right place to free the URB?
sound/usb/bcd2000/bcd2000.c line 274
Quoted 7 lines
sound/usb/bcd2000/bcd2000.c new file +6
-0+static void bcd2000_midi_output_done (struct urb *urb) +{ +if (!bcd2k->midi_out_substream) +return ; + +bcd2000_midi_send (bcd2k, bcd2k->midi_out_substream);
The midi_out_substream field gets modified asynchronously by the trigger callback; this might happen between these two statements.
Assuming that setting a pointer is atomic, use ACCESS_ONCE.
sound/usb/bcd2000/bcd2000.c line 376
Quoted 4 lines
sound/usb/bcd2000/bcd2000.c new file +4
-0+static void bcd2000_init_midi (struct bcd2000 *bcd2k) +{ + bcd2k->midi_in_urb =usb_alloc_urb (0 ,GFP_KERNEL ); + bcd2k->midi_out_urb =usb_alloc_urb (0 ,GFP_KERNEL );
Error checking.
sound/usb/bcd2000/bcd2000.c line 471
Quoted 7 lines
sound/usb/bcd2000/bcd2000.c new file +7
-0+static int bcd2000_probe (struct usb_interface *interface, +const struct usb_device_id *usb_id) +{ +snprintf (bcd2k->card->longname,sizeof (bcd2k->card->longname), +DEVICE_NAME ", at %s, %s speed" , + usb_path, + bcd2k->dev->speed ==USB_SPEED_HIGH ?"high" :"full" );
The BCD2000 does not support high speed.
sound/usb/bcd2000/bcd2000.c line 514
Quoted 4 lines
sound/usb/bcd2000/bcd2000.c new file +4
-0+static void bcd2000_disconnect (struct usb_interface *interface) +{ +list_for_each (midi, &bcd2k->midi_list) +snd_usbmidi_disconnect (midi);
midi_list is not actually used.
Regards,
Clemens
Quoted 5 lines · Clemens Ladisch
sound/usb/Kconfig config SND_USB_HIFACE · line 151
Quoted 3 lines
sound/usb/Kconfig +3
-0+config SND_USB_BCD2000 + tristate "Behringer BCD2000 MIDI driver" + select SND_HWDEPWhy HWDEP?
I admit, I copied this from the other modules (caiaq I believe). After grepping the documentation, I'm not sure if I need this. Should I remove it?
Quoted 2 lines
There is no need to have "usb" in the module name if that is the only bus this device works with.
Okay, after looking at the other modules I thought that it is common to use the "snd-usb" prefix. Should I remove it? I find it useful, e.g., to recognize it in a "lsmod".
Quoted 6 lines
sound/usb/Makefile obj-$(CONFIG_SND_USB_UA101) += snd-usbmidi-lib.o · line 26
Quoted 2 lines
sound/usb/Makefile +1
-1-obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ +obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/If there is only a single source file, just put it into misc/. If you plan to have multiple source files, then this file should have "
_midi" in its name.
Okay, I thought that the code is short enough to put everything in one file but I think with the audio part it would be best to split the file. I can move it to misc/ or do you think an own subdirectory is okay to avoid moving it in later versions?
Quoted 4 lines
sound/usb/bcd2000/bcd2000.c line 30
Quoted 2 lines
sound/usb/bcd2000/bcd2000.c new file +2
-0+#include "../usbaudio.h" +#include "../midi.h" Are these headers needed?
After cleaning up the code with Daniel, no, it's not needed anymore. Removed.
Quoted 10 lines
sound/usb/bcd2000/bcd2000.c line 155
Quoted 8 lines
sound/usb/bcd2000/bcd2000.c new file +8
-0+static void bcd2000_midi_handle_input (struct bcd2000 *bcd2k, +const unsigned char *buf,int len) +/* +* 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". +*/ ALSA does not require full MIDI commands, and handles running status.
Oh, okay. So, should I just pass everything through to snd_rawmidi_receive() ?
Quoted 7 lines
sound/usb/bcd2000/bcd2000.c line 214
Quoted 5 lines
sound/usb/bcd2000/bcd2000.c new file +5
-0+static int bcd2000_midi_output_close (struct snd_rawmidi_substream *substream) +{ +struct bcd2000 *bcd2k = substream->rmidi->private_data; +if (bcd2k->midi_out_active) { +usb_free_urb (bcd2k->midi_out_urb);Why is this the right place to free the URB?
Good point, doesn't really make sense as it only allocated once. I'll remove it.
Quoted 11 lines
sound/usb/bcd2000/bcd2000.c line 274
Quoted 7 lines
sound/usb/bcd2000/bcd2000.c new file +6
-0+static void bcd2000_midi_output_done (struct urb *urb) +{ +if (!bcd2k->midi_out_substream) +return ; + +bcd2000_midi_send (bcd2k, bcd2k->midi_out_substream);The
midi_out_substreamfield gets modified asynchronously by the trigger callback; this might happen between these two statements.
Quoted 1 line
Assuming that setting a pointer is atomic, use
ACCESS_ONCE.
You mean like this:
struct snd_rawmidi_substream * midi_out_substream; midi_out_substream = ACCESS_ONCE(bcd2k->midi_out_substream); if (!midi_out_substream)
return;
bcd2000_midi_send(bcd2k, midi_out_substream);
Quoted 6 lines
sound/usb/bcd2000/bcd2000.c line 376
Quoted 4 lines
sound/usb/bcd2000/bcd2000.c new file +4
-0+static void bcd2000_init_midi (struct bcd2000 *bcd2k) +{ + bcd2k->midi_in_urb =usb_alloc_urb (0 ,GFP_KERNEL ); + bcd2k->midi_out_urb =usb_alloc_urb (0 ,GFP_KERNEL );Error checking.
Okay, I guess I should close everything I opened in this module before or would a "return" suffice?
Thank you for your comments!
Mario
Quoted 10 lines · Mario Kicherer
Quoted 5 lines · Clemens Ladisch
sound/usb/Kconfig config SND_USB_HIFACE · line 151
Quoted 3 lines
sound/usb/Kconfig +3
-0+config SND_USB_BCD2000 + tristate "Behringer BCD2000 MIDI driver" + select SND_HWDEPWhy HWDEP?
I admit, I copied this from the other modules (caiaq I believe). After grepping the documentation, I'm not sure if I need this. Should I remove it?
Yes; you aren't using any snd_hwdep* functions.
Quoted 5 lines
Quoted 2 lines
There is no need to have "usb" in the module name if that is the only bus this device works with.
Okay, after looking at the other modules I thought that it is common to use the "snd-usb" prefix.
The USB audio class driver uses "snd-usb-audio" because "snd-audio" would be completely useless as a name. Other drivers then blindly copied this.
Quoted 1 line
Should I remove it? I find it useful, e.g., to recognize it in a "lsmod".
"snd-bcd2000" would not be recognizable?
Quoted 11 lines
Quoted 6 lines
sound/usb/Makefile obj-$(CONFIG_SND_USB_UA101) += snd-usbmidi-lib.o · line 26
Quoted 2 lines
sound/usb/Makefile +1
-1-obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ +obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/If there is only a single source file, just put it into misc/. If you plan to have multiple source files, then this file should have "
_midi" in its name.Okay, I thought that the code is short enough to put everything in one file but I think with the audio part it would be best to split the file. I can move it to misc/ or do you think an own subdirectory is okay to avoid moving it in later versions?
It doesn't make much sense to put it into a directory that you know you will move it out of later.
Quoted 4 lines
Quoted 1 line
ALSA does not require full MIDI commands, and handles running status.
Oh, okay. So, should I just pass everything through to
snd_rawmidi_receive()?
Yes.
Quoted 13 lines
Quoted 11 lines
sound/usb/bcd2000/bcd2000.c line 274
Quoted 7 lines
sound/usb/bcd2000/bcd2000.c new file +6
-0+static void bcd2000_midi_output_done (struct urb *urb) +{ +if (!bcd2k->midi_out_substream) +return ; + +bcd2000_midi_send (bcd2k, bcd2k->midi_out_substream);The
midi_out_substreamfield gets modified asynchronously by the trigger callback; this might happen between these two statements.Quoted 1 line
Assuming that setting a pointer is atomic, use
ACCESS_ONCE.
Quoted 7 lines
You mean like this:
struct
snd_rawmidi_substream*midi_out_substream;midi_out_substream=ACCESS_ONCE(bcd2k->midi_out_substream); if (!midi_out_substream)return;
bcd2000_midi_send(bcd2k, midi_out_substream);
Yes.
The code that modifies this field also needs ACCESS_ONCE.
Quoted 9 lines
Quoted 6 lines
sound/usb/bcd2000/bcd2000.c line 376
Quoted 4 lines
sound/usb/bcd2000/bcd2000.c new file +4
-0+static void bcd2000_init_midi (struct bcd2000 *bcd2k) +{ + bcd2k->midi_in_urb =usb_alloc_urb (0 ,GFP_KERNEL ); + bcd2k->midi_out_urb =usb_alloc_urb (0 ,GFP_KERNEL );Error checking.
Okay, I guess I should close everything I opened in this module before or would a "return" suffice?
Typically, in case of an error, a function is responsible to revert everything allocated in this function, i.e.:
static int bcd2000_init_midi(struct bcd2000 *bcd2k)
{
bcd2k->midi_in_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!bcd2k->midi_in_urb)
return -ENOMEM;
bcd2k->midi_out_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!bcd2k->midi_out_urb) {
usb_free_urb(bcd2k->midi_in_urb);
return -ENOMEM;
}
...Regards,
Clemens