wang qii has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47800 )
Change subject: soc/mediatek/mt8192: add i2c driver support ......................................................................
Patch Set 6:
(7 comments)
https://review.coreboot.org/c/coreboot/+/47800/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47800/3//COMMIT_MSG@9 PS3, Line 9: e
Not done?
Done
https://review.coreboot.org/c/coreboot/+/47800/3//COMMIT_MSG@10 PS3, Line 10:
Please note the datasheet name und revision. […]
Done
https://review.coreboot.org/c/coreboot/+/47800/3//COMMIT_MSG@12 PS3, Line 12:
Not done?
Done
https://review.coreboot.org/c/coreboot/+/47800/3/src/soc/mediatek/mt8192/i2c... File src/soc/mediatek/mt8192/i2c.c:
https://review.coreboot.org/c/coreboot/+/47800/3/src/soc/mediatek/mt8192/i2c... PS3, Line 145: static void mtk_i2c_speed_init(uint8_t bus)
Why can’t this be common code?
Different soc has some different register, such as: ltiming, so we need a place to initialize them separately.
https://review.coreboot.org/c/coreboot/+/47800/3/src/soc/mediatek/mt8192/i2c... PS3, Line 156: (400 * KHz * sample_div * 2) * clock_div);
Should fit into 96 characters.
The previous codes follow the 80 characters rule. Maybe we need a new patch to fix these code style problems.
https://review.coreboot.org/c/coreboot/+/47800/3/src/soc/mediatek/mt8192/i2c... PS3, Line 161: Timing
timing
Ack
https://review.coreboot.org/c/coreboot/+/47800/3/src/soc/mediatek/mt8192/i2c... PS3, Line 169: clock_div - 1);
Fits into one line.
The previous codes follow the 80 characters rule. Maybe we need a new patch to fix these code style problems.