On Thu, Oct 14, 2010 at 3:02 PM, Nils <[email protected]> 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 -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
