[alsa-devel] [PATCH 06/17] firewire-lib: Add support for MIDI capture/playback

[Clemens Ladisch]

Takashi Sakamoto wrote:
> For capturing/playbacking MIDI message, this commit adds the functionality to
> multiplex/demultiplex MIDI messages into AMDTP sream in IEC 61883-6. As a
> result, the number of MIDI ports is limited by 16 ports, for my convinience.

> +++ b/sound/firewire/amdtp.h
> +/*
> + * This module supports maximum 2 MIDI channels.
> + * Then AMDTP packets include maximum 16 MIDI streams multiplexed.
> + * This is for our convinience.
> + */
> +#define AMDTP_MAX_CHANNELS_FOR_MIDI	2

This does not look like 16 ports ...

> -	unsigned int midi_ports;
> +	unsigned int midi_channels;

... and "MIDI channels" means something different.  The RP-027 document
defines things like "MIDI data stream", "MIDI Conformant Data Channel",
and "MPX-MIDI Data Channel"; please clarify which one you mean.

> IEC 61883-6 refers to AMEI/MMA RP-027 for implementation of MIDI conformant
> data. This module implement 'MIDI1.0-1x-SPEED' mode. In the specification,
> 'MIDI1.0-2x/3x-SPEED' mode is defined with 'negotiation procedure' and
> 'encapsulation details'. But I cannot find specifications about them.

The 2x/3x speed modes are not specified.

>  static void amdtp_fill_midi(struct amdtp_stream *s,
>  			    __be32 *buffer, unsigned int frames)

> +			if ((s->midi[port] != NULL) &&
> +			    test_bit(port, &s->midi_triggered)) {

The stream pointer (s->midi[]) is not needed when the stream is not
active (triggered).  So you could drop the midi_triggered field and
just update the stream pointer when triggering (like s->pcm).

> +			buffer[s->pcm_channels + c] =
> +				be32_to_cpu((b[0] << 24) | (b[1] << 16));

Endian conversions are required only for values that occupy more than
one byte.  You should make 'buffer' a byte pointer so that you can copy
b[] directly.


Regards,
Clemens