[alsa-devel] [PATCH - [hdspm] 2/3] Enable RME RayDAT and AIO

[Adrian Knoth]

On Tue, Jan 18, 2011 at 03:55:56PM +0100, Takashi Iwai wrote:

> > -#include <linux/math64.h>
> No math64 stuff is necessary?

Probably not. At least it works and compiles without. Perhaps
cross-included from somewhere else? I see div_u64 down there, so I guess
linux/math64.h should be included. Fixed.

> > +#include <linux/sysfs.h>
> It's not implemented yet, so don't include it.

ACK.

> > -static DEFINE_PCI_DEVICE_TABLE(snd_hdspm_ids) = {
> > +static struct pci_device_id snd_hdspm_ids[] __devinitdata = {
> Why open-coded again?

No particular reason. Now you mention it, I'll change it back.

How did it end up there?: I took the code from Florian's tarball. Looks
like there was some divergent development in the meantime (between his
fork and mainline kernel)

> > @@ -796,6 +1009,8 @@ static void hdspm_silence_playback(struct hdspm *hdspm)
> >  	int n = hdspm->period_bytes;
> >  	void *buf = hdspm->playback_buffer;
> >  
> > +	snd_printk(KERN_INFO "hdspm_silence_playback\n");
> Don't put such a verbose debug message in frequently called callbacks.

ACK. Removed.

> >  	n = div_u64(n, rate);
> >  	/* n should be less than 2^32 for being written to FREQ register */
> > -	snd_BUG_ON(n >> 32);
> > +	snd_BUG_ON((n >> 32) == 0);
> This check is consistent with the comment above...

Inconsistent I guess...?

I suggest to remove the line. We know n, we control rate, this should
never be an issue.


> The rest is too long to review. Would be nice if you can split the
> whole patch in logical orders...

I'm afraid I can't, because there's no logical order. There's just this
single hdspm.c (and .h) by Florian and my style fixes. No VCS.

So whatever is wrong needs to be spotted by some experienced ALSA dev. I
guess you'd be faster fixing it instead of mailing me all the things
that are wrong, but I'm happy to change things you point out.


Cheers

-- 
mail: adi at thur.de  	http://adi.thur.de	PGP/GPG: key via keyserver