Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/27172 )
Change subject: google/poppy: Add SX9310 support ......................................................................
Patch Set 2:
(10 comments)
Can you please split this change into two CLs: 1. For driver support 2. For mainboard changes
https://review.coreboot.org/#/c/27172/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/27172/1//COMMIT_MSG@7 PS1, Line 7: oogle/poppy mb/google/poppy/variants/nocturne
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/Kconfig File src/drivers/i2c/sx9310/Kconfig:
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/Kconfig@3 PS1, Line 3: default n depends on HAVE_ACPI_TABLES
Then you don't have to add #ifdef for HAVE_ACPI_TABLES in .c file
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/chip.h File src/drivers/i2c/sx9310/chip.h:
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/chip.h@4 PS1, Line 4: Inc LLC
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/registers.h File src/drivers/i2c/sx9310/registers.h:
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/registers.h@4 PS1, Line 4: Inc LLC
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/sx9310.c File src/drivers/i2c/sx9310/sx9310.c:
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/sx9310.c@4 PS1, Line 4: Inc LLC
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/sx9310.c@4 PS1, Line 4: 2016 2018
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/sx9310.c@18 PS1, Line 18: #include <console/console.h> Is this required?
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/sx9310.c@22 PS1, Line 22: #include <gpio.h> Is this required?
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/sx9310.c@43 PS1, Line 43: = NULL No need to set this to NULL. dsd is set on line 64.
https://review.coreboot.org/#/c/27172/1/src/drivers/i2c/sx9310/sx9310.c@78 PS1, Line 78: %sD%03.3X This should be 4-character long.