Hi Clemens,
(Apr 4 2014 17:48), Clemens Ladisch wrote:
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.
OK.
+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.
OK.
- 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.
Do you suggest to add AMDTP_IN_PCM_FORMAT_BITS?
- /*
* 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.
...
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.
*/
Now I realize to misunderstand these codes.
Currently firewire-lib exports a table for SYT_INTERVAL so we can make better rules for this, can't we?
+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.)
Hm. I have no ideas for such races, except for substream counter. Can I request you more explaination? What race state can appears between PCM/MIDI functionalities?
For hw_params/hw_free, please see my reply to [24/44].
Thank you
Takashi Sakamoto o-takashi@sakamocchi.jp