Re: [PATCH] rme96 synchronization support

[Knut Petersen <Knut_Petersen@t-online.de>]

On 05.08.2013 15:18, Clemens Ladisch wrote:
> Knut Petersen wrote:
>>   			      SNDRV_PCM_INFO_MMAP_VALID |
>> +                              SNDRV_PCM_INFO_SYNC_START |
> Please use tabs like in other similar parts of the file.

ack

>>   static void
>> +snd_rme96_playback_start(struct rme96 *rme96,
>> +			 int from_pause)
>> +{
>> +	if ((rme96->playback_substream && rme96->capture_substream) &&
>> +	    (rme96->playback_substream->group == rme96->capture_substream->group)) {
>> +		snd_rme96_playcap_start(rme96,from_pause);
>> +	} else {
>> +		if (!from_pause) {
>> +			writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
>> +		}
>> +		rme96->wcreg |= RME96_WCR_START;
>> +		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>> +	}
>> +}
> This is hard to maintain because there is duplicated code.
>
> The proper way to write a synchronized start/stop trigger is to collect
> the needed register bits first, and then apply them at once to the
> register; this works for both single and grouped usage:

Yes, there is duplicated code ... I´ll remove that in a V2 of the patch.
But I do not think that it is less readable/maintainable if bit masks are
prepared at the place of use ...

>> -	
>> +
> Are these whitespace changes deliberate?

No, they are artifacts of inserting/removing debug code at a place
where originally a line with nothing but a tabulator is present.

Your comments were all about coding style. Have you had a look
at the content of the code?

cu,
  Knut