At Tue, 06 Aug 2013 19:42:17 +0200, Knut Petersen wrote:
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?
Clemens commented on your code: make a single trigger function that can handle both playback and capture (one or both) streams. It makes the code more maintainable.
Hint: even for clearing the position register, you can do it in a single place like below:
if (!from_pause) { if (bits & RME96_WCR_START) writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS); if (bits & RME96_WCR_START_2) writel(0, rme96->iobase + RME96_IO_RESET_REC_POS); }
Takashi