Re: [alsa-devel] [PATCH - [hdspm] 2/3] Enable RME RayDAT and AIO
At Tue, 18 Jan 2011 15:12:18 +0100, Adrian Knoth wrote:
@@ -29,12 +42,13 @@ #include <linux/moduleparam.h> #include <linux/slab.h> #include <linux/pci.h> -#include <linux/math64.h>
No math64 stuff is necessary?
+#include <linux/sysfs.h>
It's not implemented yet, so don't include it.
-static DEFINE_PCI_DEVICE_TABLE(snd_hdspm_ids) = { +static struct pci_device_id snd_hdspm_ids[] __devinitdata = {
Why open-coded again?
@@ -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.
static void hdspm_set_dds_value(struct hdspm *hdspm, int rate)
...
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...
The rest is too long to review. Would be nice if you can split the whole patch in logical orders...
thanks,
Takashi
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
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
participants (2)
-
Adrian Knoth -
Takashi Iwai