On Fri, Nov 02, 2007 at 05:44:03PM +0100, Stefan Reinauer wrote: > > > -u8 smbus_read_byte(u32 dimm, u32 offset) > > > +/** > > > + * Read a byte from the smbus > > > + * > > > + * @param dimm The address location of the dimm on the smbus > > > + * @param offset The offset the data is located at > > > + */ > > > > > +u8 smbus_read_byte(u8 dimm, u8 offset) > > > > I'm still not entirely sure they're always only 8 bit. Do you have a > > pointer to a datasheet or spec or standard where it's explicitly > > defined as 8 bit? Yes, it _is_ 8 bits in most cases, but can we be sure > > that it'll be 8 bit in _all_ of them? On all chipsets and controllers? > > smbus address space is 8 bits. Let's reflect this in the code. We can
ACK.
> think about this when we find the first machine where it is not.
I don't know if there are any, I was just speculating. If SMBus address
space is specified as 8 bits u8 is the way to go.
>
> > > +inline u8 spd_read_byte(u32 address, u8 offset)
> > > +{
> > > + return smbus_read_byte(address, offset);
> > > +}
> >
> > Hm, this is usually done in auto.c per-mainboard, I think. Either
> > you make spd_read_byte() a wrapper for smbus_read_byte(), or you
> > use the "fake spd" method to return hard-coded settings if there's
> > no real SPD data to be read.
>
> Yes, this is a mainboard specific abstraction. It might have to
> circumvent smbus switches on the bus.
OK, should be dropped here then.
> > Not sure this function makes sense in vt8237r_early_smbus.c, because of
> > the above and also because it's not SMBus-related per se. I'd say drop it.
> >
> > Also, address is 32bit here but 8bit in smbus_read_byte()?
>
> That's fine. The i2c has 8 (actually 7) bits address space. spd might
> not necessarily be behind the i2c bus. Or it might have a number of
> switches. So you might have an address of 0xaabbccdd saying that aa, bb,
> cc, dd are the settings of 3 i2c switches, while dd is the actual
> address of the spd on the bus _after_ doing the other settings. This is
> done on the agámi aruma in a similar way.
Thanks for the clarifications! For this patch that's not relevant though
as spd_read_byte() should be dropped anyway then as it's board specific.
But maybe we should have this info somewhere in the wiki?
http://linuxbios.org/Developer_Manual ?
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
