Re: [PATCH 26/44] fireworks: Add PCM interface

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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