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. -- http://www.hermann-uwe.de | http://www.holsham-traders.de http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
signature.asc
Description: Digital signature
-- linuxbios mailing list linuxbios@linuxbios.org http://www.linuxbios.org/mailman/listinfo/linuxbios