Uwe Poeche has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36062 )
Change subject: soc/intel/fsp_baytrail: use designware I2C driver ......................................................................
Patch Set 6:
(11 comments)
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... File src/soc/intel/fsp_baytrail/i2c.c:
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 22: #include <reg_script.h>
You do not use regscripts here so remove this include.
Done
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 24: #include "chip.h"
Move local include to last line with a leading empty line.
Done
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 16: #include <cbmem.h> : #include <device/device.h> : #include <device/pci_def.h> : #include <device/pci.h> : #include <device/pci_ids.h> : #include <drivers/i2c/designware/dw_i2c.h> : #include <reg_script.h> : #include <soc/pci_devs.h> : #include "chip.h" : #include <console/console.h> : #include <soc/i2c.h> : #include <soc/iosf.h> : #include <soc/iomap.h> : #include <soc/nvs.h>
Provide a proper order for the includes (alphabetical sorted with local includes being the last ones […]
Done
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 141: res
You could have named this bar instead of res as you are about dealing with the BAR0 resource. […]
Done
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 178: (u32)
So far you use the uintxx_t types here. Please be consistent and stick with one format.
Done
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 182: (u32)
Same here.
Done
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 226: base = pci_read_config32(dev, PCI_BASE_ADDRESS_0) & 0xfffffff0;
You could use the already used ALIGN_DOWN macro here to stay consistent.
I find using real masks more readable and changed consistent in this file to this method.
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 247: config->
Would it be more reliable to check config pointer being not NULL before usage?
Done
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 252: its
either "is" or "it is" or "it's"
Done
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 262: base_address = dw_i2c_base_address(bus); : if (!base_address) : return;
Here you check if a base address was already assigned to see if the device has been initialized alre […]
Done
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 288: ,
No need for a trailing comma hee.
Done