On Thu, Oct 14, 2010 at 3:02 PM, Nils njacobs8@hetnet.nl wrote:
Hello Myles, Thanks for the review.
Op donderdag 14 oktober 2010 18:53:43 schreef u:
Your patch is rather long. It would be easier to review if you split out the white space & comment fixes, then the usage of msr names instead of magic values. It looks like this would make your patch much smaller. Another thing that would help would be if you used svn cp so that it's obvious how you changed the lx code to adapt it for the gx2. When a patch looks like it has hundreds of lines of new code, it's understandable that it could take longer to review.
Sorry! I will try to do better next time.
No need to apologize. I was just trying to make it less frustrating. The white space, comment fixes, and some of the other simple changes could be reviewed by almost anyone.
+#define DIMM0 0xA0 +#define DIMM1 0xA2
- if (device != DIMM0)
- return 0xFF; /* No DIMM1, don't even try. */
Why do you define DIMM1 to be 0xA2 (a reasonable value) if there is no DIMM1? Could we leave it undefined or define it to be something obviously bogus?
I am not the designer of that code i copied it from lippert/roadrunner-lx . When i leave DIMM1 undefined i get following errors/warnings:
Maybe we should define it to be 0xFF. In the future, I would think that these values will move into Kconfig, and I think it will make that conversion easier if we already know which values are bogus.
Thanks, Myles