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

[Takashi Iwai]

At Tue, 18 Jan 2011 16:24:08 +0100,
Adrian Knoth wrote:
> 
> > > -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.

I guess this was changed in the upstream from the version Florian's
code based on.

> 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)

Yes, it's always bad to fork in such a way.  If forking, it should track
also the upstream changes (which can be better done via git).


> > >  	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...?

Yep.

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

Fair enough.

> > 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.

Too bad.

> 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.

Thanks.

Could you repost the fixed patches?  Don't forget to put me to Cc
(or send me and put alsa-devel ML to Cc), so that it reaches to me
directly.  I don't look through ML so regularly, and alsa-devel ML
blocks the posts from non-subscribers or ones with too large contents
for approvals.


Takashi