Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29104 )
Change subject: sdm845: Port I2C driver ......................................................................
Patch Set 24:
(10 comments)
https://review.coreboot.org/#/c/29104/24/src/mainboard/google/cheza/mainboar... File src/mainboard/google/cheza/mainboard.c:
https://review.coreboot.org/#/c/29104/24/src/mainboard/google/cheza/mainboar... PS24, Line 42: i2c_init(12, 400 * KHz, 1); This should be an enum (QUP_WRAP1_S4 or something like that). Should probably pull the one from clock.h into qcom_qup_se.h and use it everywhere.
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c File src/soc/qualcomm/sdm845/i2c.c:
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@46 PS24, Line 46: unsigned int hz, unsigned int idx Let's not pass both Hz and an index. If you only want to use those three set operating points (I think for I2C that's fine since frequencies are very standardized there), just combine these into a single enum parameter (and name the enum values I2C_100KHZ, I2C_400KHZ, I2C_1MHZ or something like that).
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@101 PS24, Line 101: BITS_PER_WORD >> 4 How exactly does this work? When it says byte_granularity I would expect 1 means 1 byte, 2 means 2 bytes, etc... but 8 >> 4 is actually 0. Is that really what you wanted? And would this still work if BITS_PER_WORD was 16 (so this would become 1), 32 (this would become 2) or 64 (this would be 4)?
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@118 PS24, Line 118: setbits_le32(®s->geni_s_irq_enable, S_CMD_DONE_EN); Why not combine this with the other write to that register above?
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@122 PS24, Line 122: static u32 wait_till_irq_set(unsigned int bus) This is the exact same function as in the SPI code, right? Let's put it in qup.c so both drivers can share it.
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@187 PS24, Line 187: static void i2c_handle_error(unsigned int bus) Actually... I think everything up to an including this function is 100% identical with SPI? Let's please merge all of that into single copies in qup.c.
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@228 PS24, Line 228: stopwatch_init_msecs_expire(&sw, 1000); ...and I think we can also factor out this loop (including the error checking) into a separate function that can be shared with the SPI code.
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@245 PS24, Line 245: (m_irq & M_CMD_DONE_EN) nit: Can remove this part of the check. !A || (A && B) is logically equivalent to just !A || B
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@247 PS24, Line 247: printk(BIOS_INFO, "%s:Error: Transfer failed\n", __func__); nit: Please write as "ERROR: %s failed\n". We like having ERROR: at the start of the line to make it easily noticeable. Also, printk level should probably be BIOS_ERR.
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@261 PS24, Line 261: /* Set stretch = 0 for the last transfer */ Please also explain what that bit does and why it needs to be set only for the last transfer.