[alsa-devel] [PATCH v2] MIDI driver for Behringer BCD2000 USB device
Clemens Ladisch
clemens at ladisch.de
Thu Feb 6 21:51:12 CET 2014
Mario Kicherer wrote:
> 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?
Yes; you aren't using any snd_hwdep* functions.
>> 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.
> Should I remove it? I find it useful, e.g., to recognize it in a "lsmod".
"snd-bcd2000" would not be recognizable?
>>> -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.
>> 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.
>>> +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);
Yes.
The code that modifies this field also needs ACCESS_ONCE.
>>> +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
More information about the Alsa-devel
mailing list