Werner Zeh 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 5:
(13 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.
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.
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)
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 83: if (!tree_dev || !tree_dev->enabled) { I would change the logic here to && otherwise you can end up dereferencing a NULL pointer for "tree_dev->enabled".
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 90: !config || !config->early_init Same here
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. It will then make it consistent with the same usage in i2c_enable_acpi_mode().
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.
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 182: (u32) Same here.
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.
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?
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"
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 already. As you do not need the value of base_address I would not spend a variable on it. Just use the following instead: if (!dw_i2c_base_address(bus)) return;
And get rid of the variable base_address.
https://review.coreboot.org/c/coreboot/+/36062/5/src/soc/intel/fsp_baytrail/... PS5, Line 288: , No need for a trailing comma hee.