Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29104 )
Change subject: sdm845: Port I2C driver ......................................................................
Patch Set 26:
(2 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);
As per a discussion with clock team they need this enum(QUP_WRAP1_S4...) for their functionality. […]
Okay, but why don't they just #include qcom_qup_se.h in clock.c and then use the enum from there? We should only need to have a single enum definition for the available QUPs and all code that cares about it can share that. I think qcom_qup_se.h would be the more natural place for that.
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@101 PS24, Line 101: BITS_PER_WORD >> 4
geni_byte_granularity is used to configure the number of protocol words in each FIFO entry. […]
Okay. I think it's fine to hardcode a single BITS_PER_WORD and the PACK_VECTORs for that (just maybe add a comment where it is defined to clarify that the two depend on each other). However, since you're writing it programmatically here anyway, can you please write it as (log2(BITS_PER_WORD) - 3)? That should come out to the same numbers for 16 and 32 but represents it more correctly (e.g. it will also yield the right value for 8).
-
Julius Werner (Code Review)