[alsa-devel] [PATCH] MIDI driver for Behringer BCD2000 USB device
Daniel Mack
daniel at zonque.org
Wed Feb 5 11:54:05 CET 2014
Hi Mario,
Thanks for your contribution! I'll comment inline.
On 02/01/2014 08:54 PM, Mario Kicherer wrote:
> diff --git a/sound/usb/bcd2000/bcd2000.c b/sound/usb/bcd2000/bcd2000.c
> new file mode 100644
> index 0000000..100c39f
> --- /dev/null
> +++ b/sound/usb/bcd2000/bcd2000.c
> +#ifdef CONFIG_USB_DEBUG
> +#define DEBUG 1
> +#else
> +#define DEBUG 0
> +#endif
Please use the define directly instead of introducing a new one.
> +MODULE_DEVICE_TABLE (usb, id_table);
> +MODULE_AUTHOR("Mario Kicherer, dev at kicherer.org");
> +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.
> +static int debug = 0;
> +module_param(debug, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(debug, "enable debug output (default: 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?
> +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;
> +
...
> +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;
> +
> + 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.
> +
> + 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) {
> + /*
> + * 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.
> + 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;
> +
> + /* 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.
> +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.
> + 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.
> + /* 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);
> + 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 :)
> + 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.
...
> +/*
> + * general part
> + */
> +
> +static void bcd2000_free_usb_related_resources(struct bcd2000 *bcd2k,
> + struct usb_interface *interface)
> +{
> + 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.
> + for (i = 0; i < 1; ++i) {
Why do you need a loop if it only runs once!?
> + mutex_lock(&bcd2k->mutex);
> + intf = bcd2k->intf;
> + bcd2k->intf = NULL;
> + mutex_unlock(&bcd2k->mutex);
> + if (intf) {
> + usb_set_intfdata(intf, NULL);
> + if (intf != interface)
> + 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.
> +}
> +
> +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;
> + 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.
> + 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)
> +// 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
More information about the Alsa-devel
mailing list