Re: [PATCH v2] MIDI driver for Behringer BCD2000 USB device

[Mario Kicherer <dev@kicherer.org>]

On 06.02.2014 10:21, Clemens Ladisch wrote:
>> +config SND_USB_BCD2000
>> +	tristate "Behringer BCD2000 MIDI driver"
>> +	select SND_HWDEP
>
> Why 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?

> 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".

>> -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?

>> +#include "../usbaudio.h"
>> +#include "../midi.h"
>
> Are these headers needed?

After cleaning up the code with Daniel, no, it's not needed anymore. 
Removed.


>> +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() ?

>> +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.

>> +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.

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);


>> +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