Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 13: Code-Review+2
(7 comments)
https://review.coreboot.org/c/coreboot/+/44476/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44476/13//COMMIT_MSG@9 PS13, Line 9: _khz Needs slight update.
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/chip... File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/chip... PS13, Line 19: I2C_FREQ_MAX = 2700, Nit, technically these should have a KEMPLD_ prefix. All the `chip.h` are included in the `build/mainboard*/*/static.c`, hence conflicts are possible and hard to control. But it's nothing that can't be fixed later if it collides eventually.
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... PS12, Line 50: stadart
? […]
Done
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... PS12, Line 52: I2C_FREQ_FAST_MODE = 400, /* 400 kHz */ : I2C_FREQ_FAST_PLUS_MODE = 1000, /* 1 MHz */
These two seem unused. Why not make them available through `chip.h`? Then […]
Done
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... PS12, Line 262: is an invalid frequency value
"is too high"?
Done
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 254: BIOS_WARNING Warning is a bit too high, from `commonlib/loglevel.h`:
Log level for when the system has noticed an issue that most likely will not preclude a successful boot.
I don't think this could make a boot fail.
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 256: config->i2c_frequency); Nit, while not mandated, it would be nice to have braces around the branches because they are (visibly) multi-lined.