Re: [PATCH 08/39] firewire-lib: Add support for duplex streams synchronization in blocking mode

[Clemens Ladisch <clemens@ladisch.de>]

Takashi Sakamoto wrote:
> Generally, the devices can synchronize to handle 'presentation timestamp'
> in CIP packets. This commit adds functionality to pick up this timestamp from
> in-packets transmitted by the device, then use it for out packets.
>
> In current implementation, this module generated the timestamp by itself. This
> is 'SYT Match' mode. Then this drive acts as synchronization master. This
> commit allows this module to act as synchronization slave.
>
> This commit restricts this mechanism is only available in blocking mode because
> handling the timestamp in non-blocking mode is more complicated than in
> blocking mode.

> +++ b/sound/firewire/amdtp.c
> @@ -72,6 +73,10 @@ int amdtp_stream_init(struct amdtp_stream *s, struct fw_unit *unit,
> +	s->sync_slave = ERR_PTR(-1);

This pointer does not report any actual error code either.

> @@ -626,7 +631,7 @@ static void handle_out_packet(struct amdtp_stream *s, unsigned int syt)
>  {
> -	struct snd_pcm_substream *pcm;
> +	struct snd_pcm_substream *pcm = NULL;

Why this change in this patch?

> @@ -768,6 +773,15 @@ static void in_stream_callback(struct fw_iso_context *context, u32 cycle,
> +		/* Process sync slave stream */
> +		if ((s->flags & CIP_BLOCKING) &&
> +		    (s->flags & CIP_SYNC_TO_DEVICE) &&
> +		    s->sync_slave->callbacked) {

It might be easier to check just s->sync_slave instead of multiple flags.


> +	/* when sync to device, flush the packets for slave stream */
> +	if ((s->flags & CIP_BLOCKING) &&
> +	    (s->flags & CIP_SYNC_TO_DEVICE) && s->sync_slave->callbacked)

Same here.

> +/* this is executed one time */
> +static void amdtp_stream_callback(struct fw_iso_context *context, u32 cycle,

This name is rather generic; this is not "the" callback, but the
"initial" or "first" callback.

> ...
> +	return;
> +}

This is not necessary here.

> +static inline void amdtp_stream_set_sync(enum cip_flags sync_mode,
> +					 struct amdtp_stream *master,
> +					 struct amdtp_stream *slave)
> +{
> +	/* clear sync flag */
> +	master->flags &= ~CIP_SYNC_TO_DEVICE;
> +	slave->flags &= ~CIP_SYNC_TO_DEVICE;

This needs to be done only if sync_mode != CIP_SYNC_TO_DEVICE.


Regards,
Clemens