Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/19372 )
Change subject: soc/intel/common/block: Add Intel common smbus code ......................................................................
Patch Set 5:
(6 comments)
https://review.coreboot.org/#/c/19372/5/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/smbus.h:
PS5, Line 21: do_smbus_write_byte
These are very poor names for globally visible functions.
Done.Revised name in PS#6
PS5, Line 23: enable_smbus
This is overly generic. While there are other cases like this in the tree.
Done.Changed to smbus_common_init
https://review.coreboot.org/#/c/19372/5/src/soc/intel/common/block/smbus/smb... File src/soc/intel/common/block/smbus/smbus.c:
PS5, Line 81: SMBUS_IO_BASE
This is now being applied to all users of this module?
Yes. SMBUS IO base address is common to skylake/apollolake/glk and upcoming Intel SOCs.
Line 103: 0xa123, /* SunRisePoint H */
Add ids to pci_ids.h ?
ok. Do you suggest moving it to soc/pci_ids.h or define pci_ids.h under common?
https://review.coreboot.org/#/c/19372/5/src/soc/intel/common/block/smbus/smb... File src/soc/intel/common/block/smbus/smbuslib.c:
PS5, Line 24: 0xefa0
This is duplicated in more than one place?
Ok.move it to intelblocks/smbus.h?
Line 180: #if !ENV_RAMSTAGE
Why are these functions being guarded?
There was definition conflict due to declaration under device/early_smbus.h and device/smbus.h for smbus_write_byte/smbus_read_byte functions,had to limit scope.
-
Aamir Bohra (Code Review)