[alsa-devel] [PATCH 4/4] Add handling AMDTP receive stream

[Takashi Sakamoto]

Clemens,

Thanks for your review. Thanks for your review. I arrange these issues 
to 6 items below.

amdtp[PATCH4/4]:
1. It would be easier to add some local variables for CIP headers.
OK. Actually be32_to_cpu() was used six times. I add "u32 cip_header[2]" 
for them and reduce the time.

2. "CIP header error" should be handled approproately, ignored or abort.
OK. Here I want to select ignoring.

3. The comment about Fireworks' data block counter
I'm sorry but it includes wrong description in the patch. It was old one 
working with inappropriate code. The wrong code reported me the wrong 
result  and I added the comment with it...

This comment is true:
"Echo Audio's Fireworks reports wrong number of data block counter. It 
always reports it with increment by 8 blocks even if actual data blocks 
different from 8."

4. The comment related to ignoring SYT field should be indicate that 
ALSA has no spec equivalent to "presentation timestamp".
OK. I rewrite the comment to mean it.

5. "data_block_counter" is unused.
I forget to add some description to commit log that I plan to use it for 
future patches to add handling MIDI. For my convinience, I want to keep 
it here.

6. typo of "calcurate"
OK. I rewrite.


Regards

Takashi Sakamoto
o-takashi at sakamocchi.jp

(Apr 28 2013 21:52), Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> To handle AMDTP receive stream, this patch adds some codes with condition of its
>> direction and new functions. Once amdtp_stream_init() is executed with its
>> direction, AMDTP receive and transmit stream can be handles by the same way.
>>
>> Unfortunatelly Echo Audio's Fireworks(TM) is not fully compliant to IEC 61883-6
>> against their guide. This patch include some work arounds for Fireworks.
>
>> +++ b/sound/firewire/amdtp.c
>
>> +static void handle_in_packet_data(struct amdtp_stream *s,
>> +						unsigned int data_quadlets)
>
>> +	/* checking CIP headers for AMDTP with restriction of this module */
>> +	if (((be32_to_cpu(buffer[0]) & CIP_EOH_MASK) == CIP_EOH) ||
>> +	    ((be32_to_cpu(buffer[1]) & CIP_EOH_MASK) != CIP_EOH) ||
>> +	    ((be32_to_cpu(buffer[1]) & CIP_FMT_MASK) != CIP_FMT_AM)) {
>
> It would be easier and a little bit more efficient to convert the two
> header quadlets at the beginning and to store them in some local
> variables.
>
>> +		dev_err(&s->unit->device, "CIP headers error: %08X:%08X\n",
>> +			be32_to_cpu(buffer[0]), be32_to_cpu(buffer[1]));
>> +		return;
>
> This error should either be ignored (resubmit the packet) or cause the
> stream to abort (see "queueing error" below).
>
> But I'm not sure which.
>
>> +		 * Echo Audio's Fireworks reports wrong number of data block
>> +		 * counter. Mostly it reports it with increment of 8 blocks
>> +		 * but sometimes it increments with NO-DATA packet.
>
> Does this imply that the DBC field could not be used to detect dropped
> packets?
>
>> +		 * Handling syt field is related to time stamp,
>> +		 * but the cost is bigger than the effect.
>
> Actually, the SYT specifies the presentation timestamp, but when doing
> capturing in ALSA, there is no "presentation" in the sense of the spec,
> so not handling the SYT field is the correct thing to do regardless of
> the cost.  :-)
>
>> +		 * NOTE: this module doesn't check dbc ...
>
>> +		data_block_counter  = (be32_to_cpu(buffer[0]) & AMDTP_DBC_MASK);
>
> Unused.
>
>> +		/* for next packet */
>> +		s->data_block_quadlets = data_block_quadlets;
>> +		s->data_block_counter  = data_block_counter;
>
> Unused.
>
>> +	/* calcurate packet index */
>
>> +	/* calcurate period and buffer borders */
>
> 	   calculate
>
>
> Regards,
> Clemens
>