Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47392 )
Change subject: WIP: soc/mediatek/mt8192: modify i2c and dma reset flow ......................................................................
Patch Set 3:
(11 comments)
https://review.coreboot.org/c/coreboot/+/47392/3/src/soc/mediatek/common/i2c... File src/soc/mediatek/common/i2c.c:
PS3: Please remove execute permission.
https://review.coreboot.org/c/coreboot/+/47392/3/src/soc/mediatek/common/i2c... PS3, Line 13: inline I think we don't necessarily need this to be inline.
https://review.coreboot.org/c/coreboot/+/47392/3/src/soc/mediatek/common/i2c... PS3, Line 15: struct const struct
https://review.coreboot.org/c/coreboot/+/47392/3/src/soc/mediatek/common/i2c... PS3, Line 16: struct const struct
https://review.coreboot.org/c/coreboot/+/47392/3/src/soc/mediatek/common/i2c... PS3, Line 17: Check bus < ARRAY_SIZE(mtk_i2c_bus_controller)
https://review.coreboot.org/c/coreboot/+/47392/3/src/soc/mediatek/common/i2c... PS3, Line 26: ( No need for parentheses.
https://review.coreboot.org/c/coreboot/+/47392/3/src/soc/mediatek/common/i2c... PS3, Line 27: ( No need for parentheses.
https://review.coreboot.org/c/coreboot/+/47392/3/src/soc/mediatek/common/i2c... PS3, Line 35: udelay(50); Why do we need 50us delay here, but only 10us for I2C_APDMA_ASYNC?
https://review.coreboot.org/c/coreboot/+/47392/3/src/soc/mediatek/common/inc... File src/soc/mediatek/common/include/soc/i2c_common.h:
PS3: Please remove execute permission.
https://review.coreboot.org/c/coreboot/+/47392/3/src/soc/mediatek/common/inc... PS3, Line 67: r R
https://review.coreboot.org/c/coreboot/+/47392/3/src/soc/mediatek/common/inc... PS3, Line 74: i2c Take the chance to fix this to "I2C"?