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

[Johannes Berg]

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?

> > +	list_for_each_entry(cii, &sdev->codec_list, list) {
> > +		if (cii->codec->open) {
> > +			err = cii->codec->open(cii, pi->substream);
> > +			if (err) {
> > +				result = err;
> > +				goto out_unlock;
> 
> What happens if the first code is opened but fail the secondary?
> No need to close the first?

Yes, that needs to be done, good catch.

> > +	/* 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.

> > +static irqreturn_t i2sbus_bus_intr(int irq, void *devid, struct pt_regs *regs)
> > +{
> > +	struct i2sbus_dev *dev = devid;
> > +	u32 intreg;
> > +
> > +	spin_lock(&dev->low_lock);
> > +	intreg = in_le32(&dev->intfregs->intr_ctl);
> > +
> > +	printk(KERN_INFO "i2sbus: interrupt, intr reg is 0x%x!\n", intreg);
> 
> Should this be really always printed?

no, gone.

johannes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 793 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20060606/6a335b7c/attachment.pgp>