[alsa-devel] [PATCH 26/44] fireworks: Add PCM interface

[Clemens Ladisch]

Takashi Sakamoto wrote:
> This commit adds a functionality to capture/playback PCM samples.
>
> +++ b/sound/firewire/fireworks/fireworks_pcm.c
> +static unsigned int freq_table[] = {

This table is never changed; it can be made const.

> +static int
> +hw_rule_xxxxx(struct snd_pcm_hw_params *params,
> +	     struct snd_pcm_hw_rule *rule,
> +	     struct snd_efw *efw, unsigned int *channels)

The channels parameters can be made const, too.

> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +		substream->runtime->hw.formats = SNDRV_PCM_FMTBIT_S32;
> +	} else {
> +		substream->runtime->hw.formats = AMDTP_OUT_PCM_FORMAT_BITS;

The should have been a similar symbol for AMDTP capture streams.

> +	/*
> +	 * AMDTP functionality in firewire-lib require periods to be aligned to
> +	 * 16 bit, or 24bit inner 32bit.
> +	 */
> +	err = snd_pcm_hw_constraint_step(substream->runtime, 0,
> +					 SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 32);
> +	if (err < 0)
> +		goto end;
> +	err = snd_pcm_hw_constraint_step(substream->runtime, 0,
> +					 SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 32);
> +	if (err < 0)
> +		goto end;

The comment is not correct.

Period and buffer sizes are always aligned to the frame size, because
they are measured in frames.

The DICE driver aligns the period and buffer sizes to a multiple of
32 *frames* because the dual-wire sample transfer functions did not
handle buffer boundaries in the middle of a packet, and period
interrupts are more accurate if the period boundaries are at the exact
end a packet.  (The alignment could be reduced to 16 or 8 frames at
lower sample rates, but I was too lazy.)

This alignment makes sense only when the driver uses blocking mode, so
the snd-firewire-speakers driver does not use any alignment.

Aligning to 32 *bytes* instead of *frames* does not make sense, because
there is no such constraint in the hardware or the driver.

This driver uses blocking mode, so aligning to packets makes sense.
Replace _BYTES with _SIZE, and change the comment to something like this
(I should have written a comment like this in dice.c in the first place):

/*
 * Align the period size to SYT_INTERVAL to align the period interrupts
 * with the packet boundaries.  Align the buffer size to SYT_INTERVAL to
 * avoid having a buffer boundary inside a packet.
 */

> +static int pcm_open(struct snd_pcm_substream *substream)
> +{
> +	...
> +	/*
> +	 * When source of clock is not internal or any PCM streams are running,
> +	 * available sampling rate is limited at current sampling rate.
> +	 */
> +	if ((clock_source != SND_EFW_CLOCK_SOURCE_INTERNAL) ||
> +	    amdtp_stream_pcm_running(&efw->tx_stream) ||
> +	    amdtp_stream_pcm_running(&efw->rx_stream)) {

Opening the playback and capture streams of a single PCM device is
protected with the same mutex, but this does not help against races with
the MIDI callbacks.

This substream management code must be protected with a mutex.
(Also in hw_params and hw_free.)


Regards,
Clemens