[Alsa-devel] [RFC 4/8] snd-aoa: add i2sbus

[Takashi Iwai]

At Tue, 06 Jun 2006 13:17:27 +0200,
Johannes Berg wrote:
> 
> On Fri, 2006-06-02 at 16:23 +0200, Takashi Iwai wrote:
> > > +	if (I2S_CLOCK_SPEED_18MHz % rate == 0) {
> > > +		if ((I2S_CLOCK_SPEED_18MHz / rate) % mclk == 0) {
> > 
> > Equivalent with "I2S_CLOCK_SPEED_18MHZ % (rate * mclk) == 0" ?
> 
> Yeah, I guess, never really thought about that, just wrote it down the
> way I thought to do it :) That said, I think it's more readable if
> written that way, do you want me to change it regardless?

I found a single if is more readable (and good for compiler).

> > > +	/* well, we really should support scatter/gather DMA */
> > > +	/* FIXME FIXME FIXME: If this fails, we BUG() when the alsa layer
> > > +	 * later tries to allocate memory. Apparently we should be setting
> > > +	 * some device pointer for that ...
> > > +	 */
> > > +	snd_pcm_lib_preallocate_pages_for_all(
> > > +		dev->pcm, SNDRV_DMA_TYPE_DEV,
> > > +		snd_dma_pci_data(macio_get_pci_dev(i2sdev->macio)),
> > > +		64 * 1024, 64 * 1024);
> > 
> > Is the comment true?  Yes, you have to set the device pointer via
> > snd_pcm_lib_preallocate*().  But it must be OK even if preallocate
> > fails.
> 
> Hah, I don't know actually, I didn't know you set the pointer using this
> function, when I wrote the comment I just had forgotten the preallocate
> call!
> Does that mean that _preallocate_pages_for_all() has the side effect of
> setting the pointer? If so, imho that's pretty bad.

No, the only requirement is that you have to call snd_pcm_lib_malloc()
with proper type and assigned device pointer if you use
snd_pcm_lib_malloc() function.  (If not called, you've got an error
when compiled with debug option.)


Takashi