(Nov 26 2013 00:27), Clemens Ladisch wrote:
>> - if (s->dual_wire)
>> + if (s->direction == AMDTP_TRANSMIT_STREAM) {
>> + if (s->dual_wire)
>> + s->transfer_samples = amdtp_read_s16_dualwire;
>> + else
>> + s->transfer_samples = amdtp_read_s16;
>> + } else if (s->dual_wire)
>> s->transfer_samples = amdtp_write_s16_dualwire;
>> else
>> s->transfer_samples = amdtp_write_s16;
>
> It's inconsistent to have braces around the if branch but not around the
> else branch.
OK. I'm sorry but I forget to run checkpatch.pl for this series.
>> +static void amdtp_read_s16(struct amdtp_stream *s,
>> + struct snd_pcm_substream *pcm,
>> + __be32 *buffer, unsigned int frames)
>> +{
>
>> + *dst = be32_to_cpu(buffer[c]) << 8;
>
> >> 8
> (also in _dualwire)
I also took mistakes of this bit-shift in this patch and patch 11.
I should have tested it with S16LE.
> It might be a good idea to completely drop S16 support.
Please give me a bit time to consider about this.
>> +static void amdtp_read_s32_dualwire(struct amdtp_stream *s,
>> + struct snd_pcm_substream *pcm,
>> + __be32 *buffer, unsigned int frames)
>> +{
>
>> + for (i = 0; i < frames; ++i) {
>> + for (c = 0; c < channels; ++c) {
>> + *dst =
>> + be32_to_cpu(buffer[c]) << 8;
>> + dst++;
>> + }
>> + buffer += 1;
>> + for (c = 0; c < channels; ++c) {
>> + *dst =
>> + be32_to_cpu(buffer[c]) << 8;
>> + dst++;
>> + }
>> + buffer += s->data_block_quadlets - 1;
>
> This is not correct.
>
> If we assume SYT_INTERVAL = 4, two channels at 192 kHz, and MIDI, the
> samples L1 R1 L2 R2 L3 R3 ... L8 R8 would be arranged in the data
> block's quadlets like four channels at 96 kHz, like this:
>
> L1 L4 R1 R4 L2 L5 R2 R5 L3 L6 R3 R6 L4 L7 R4 R8 MIDI
This is my fault to create patches. These codes will be correctly
modified in later patch 11. But I should have notice in this patch...
>> +static void handle_in_packet(struct amdtp_stream *s,
>> + unsigned int payload_quadlets,
>> + __be32 *buffer)
>
>> + if (((cip_header[0] & CIP_EOH_MASK) == CIP_EOH) ||
>> + ((cip_header[1] & CIP_EOH_MASK) != CIP_EOH) ||
>> + ((cip_header[1] & CIP_FMT_MASK) != CIP_FMT_AM)) {
>> + dev_info(&s->unit->device,
>> + "Invalid CIP header for AMDTP: %08X:%08X\n",
>> + cip_header[0], cip_header[1]);
>
> If some device sends 'wrong' values for all packets, the log will
> overflow with these messages. Please use printk_ratelimit().
OK.
>> + if ((payload_quadlets < 3) ||
>> + ((s->flags & CIP_BLOCKING) &&
>> + ((cip_header[1] & CIP_SYT_MASK) == CIP_SYT_NO_INFO)))
>
> In the general case, we do not know if the stream is blocking or non-
> blocking, so the driver should always be able to handle (=ignore) no-
> data packets.
OK. And we should consider about AMDTP special non-empty packet for
no-data, I should have written like this:
#define AMDTP_FDF_NO_DATA 0xff
/* ignore CIP empty packet or AMDTP NO-DATA packet */
if ((payload_quadlets < 3) ||
((cip_header[1] & CIP_FDF_MASK) >> CIP_FDF_SFC_SHIFT) ==
AMDTP_FDF_NO_DATA)))
>> + data_block_counter = (cip_header[1] & AMDTP_DBC_MASK);
>
> These parentheses are not needed.
OK.
>> + s->data_block_counter = (s->data_block_counter + data_blocks) & 0xff;
>> + if (s->data_block_counter != data_block_counter) {
>> + dev_err(&s->unit->device,
>> + "Detect uncontinuity of CIP packets\n");
>> + s->data_block_counter = data_block_counter;
>> + return;
>> + }
>
> The correct thing to do would be to insert the missing samples, or to
> stop the stream.
OK. But I remember there are some devices which transmits packets with
0x0000 in its SYT field (Digidesign 003 Rack, I don't touch its
development.). So I want to remove these codes if possible.
Thank you.
Takashi Sakamoto