On Fri, Nov 02, 2007 at 12:39:16PM -0400, Corey Osgood wrote:
/* Yes, this is a mess, but it's the easiest way to do it. */
- while ((inb(SMBHSTSTAT) & 1) == 1 && loops <= SMBUS_TIMEOUT)
- while ((inb(SMBHSTSTAT) & 1) == 1 && loops < SMBUS_TIMEOUT)
Rationale? Does it make a big difference?
No, not really. Just that now the loop runs SMBUS_TIMEOUT times, instead if SMBUS_TIMEOUT + 1.
OK, so it's more a cosmetical issue, no real code/functionality changes?
The offset will always be 8 bit, since there are 256 offsets (and >half of them we'll never touch anyways). The address also has to be 8 bit, since it's programmed into an 8 bit register (per vt8237r datasheet, p125).
Yep, u8 for both is fine then.
+#ifdef DIMM_SOCKETS
- ram_slots = DIMM_SOCKETS;
+#else
- ram_slots = sizeof(ctrl->channel0)/sizeof(ctrl->channel0[0]);
+#endif
Can you explain? Shouldn't DIMM_SOCKETS always match sizeof(ctrl->channel0)/sizeof(ctrl->channel0[0])? When does it happen that they do not match (and why?).
Unless someone doesn't define DIMM_SOCKETS, or uses some other name. I suppose that could be just dropped in favor of ARRAY_SIZE().
Yes, drop the DIMM_SOCKETS part and dependency IMO.
ARRAY_SIZE(ctrl->channel0);
should do (if there's only one channel).
Please explain the SPD_MEMORY_TYPE_SDRAM/SPD_MEMORY_TYPE_SDRAM_DDR2 check in the comment here.
If all you want is to know whether some sensible RAM type is returned wouldn't "> 0" and "< 0xff" be enough (as per your comment)? You don't really care about the exact type, you only want to know _if_ there's a DIMM here, correct?
Safety's sake. If the smbus happens to spurt out some odd value (I've seen 0x30 once) while this is going on, we want to be sure it's really
OK, but how do we know the odd values can never be e.g. 8 (which is valid) in some cases? In such a scenario this code wouldn't work?
valid data. Originally it only sought DDR2, but that's bad since this southbridge can be used on DDR systems as well. Looking further though, it's only DDR/DDR2, so the SDRAM bit could be dropped.
If I read this correctly you're checking whether you get one of these?
#define SPD_MEMORY_TYPE_SDRAM 4 #define SPD_MEMORY_TYPE_MULTIPLEXED_ROM 5 #define SPD_MEMORY_TYPE_SGRAM_DDR 6 #define SPD_MEMORY_TYPE_SDRAM_DDR 7 #define SPD_MEMORY_TYPE_SDRAM_DDR2 8
If we make this "> 0" and "< 0xff" ("< 10" or so should be enough, too) then this function might be usable on non-vt8237r chipsets and can go in some global SMBus file to be used by others?
Perhaps. vt8231/8235 could use something similar, they just use a big delay as of right now.
I'd rather match all legit RAM types (1-8 or so) and make it a function which can be used by every chip (not only vt*). Think EDO, DDR3, whatever.
Uwe.