Re: [PATCH] rme96 synchronization support

[Takashi Iwai <tiwai@suse.de>]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel