Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29104 )
Change subject: sdm845: Port I2C driver ......................................................................
Patch Set 31:
(8 comments)
https://review.coreboot.org/#/c/29104/31/src/soc/qualcomm/sdm845/i2c.c File src/soc/qualcomm/sdm845/i2c.c:
https://review.coreboot.org/#/c/29104/31/src/soc/qualcomm/sdm845/i2c.c@44 PS31, Line 44: struct qup_i2c_clk_fld *itr = qup_i2c_clk_map + idx; When you rewrite this to use the generic enum (see other file), I'd write this stuff like this instead:
int clk_div, t_high, t_low, t_cycle;
switch (speed) { case I2C_SPEED_STANDARD: clk_div = 7; t_high = 10; t_low = 11; t_cycle = 26; case I2C_SPEED_FAST: ... default: die("Unsupported I2C speed"); }
https://review.coreboot.org/#/c/29104/31/src/soc/qualcomm/sdm845/i2c.c@89 PS31, Line 89: (BITS_PER_WORD >> 4)); What about rewriting this to log2(BITS_PER_WORD) - 3? You said "Done" to that, but it looks like you didn't actually do it.
https://review.coreboot.org/#/c/29104/31/src/soc/qualcomm/sdm845/i2c.c@130 PS31, Line 130: segment.buf, segment.buf Note that if you combine the three size parameters into one like I'm suggesting in the other file, you can't pass both din and dout here. You'd have to do something like
void *din = NULL, *dout = NULL;
if (!(segment.flags & I2C_M_RD)) { dout = segment.buf; ... } else { din = segment.buf; ... }
https://review.coreboot.org/#/c/29104/31/src/soc/qualcomm/sdm845/include/soc... File src/soc/qualcomm/sdm845/include/soc/i2c.h:
https://review.coreboot.org/#/c/29104/31/src/soc/qualcomm/sdm845/include/soc... PS31, Line 23: }; Actually, looks like we have an existing enum for this in <device/i2c.h> (I2C_SPEED_FAST, etc.), can you use that instead?
https://review.coreboot.org/#/c/29104/31/src/soc/qualcomm/sdm845/qcom_qup_se... File src/soc/qualcomm/sdm845/qcom_qup_se.c:
https://review.coreboot.org/#/c/29104/31/src/soc/qualcomm/sdm845/qcom_qup_se... PS31, Line 101: u32 wait_till_irq_set(unsigned int bus) I think all of these functions other than qup_isr_handle() could be static?
If you do want to export them, they should probably be better namespaced (e.g. qup_wait_for_irq, qup_handle_tx, etc.).
https://review.coreboot.org/#/c/29104/31/src/soc/qualcomm/sdm845/qcom_qup_se... PS31, Line 118: Please fix indentation.
https://review.coreboot.org/#/c/29104/31/src/soc/qualcomm/sdm845/qcom_qup_se... PS31, Line 184: qup_isr_handle "ISR handle" sounds a bit weird for that this function actually does (which is handle a full transfer except for the setup part). Maybe qup_handle_transfer() or qup_finish_transfer() would be better?
https://review.coreboot.org/#/c/29104/31/src/soc/qualcomm/sdm845/qcom_qup_se... PS31, Line 185: unsigned int tx_rem_bytes, unsigned int rx_rem_bytes) You shouldn't need to pass all of size, tx_rem_bytes and rx_rem_bytes. They should all be the same (unless one of the transfer directions is NULL). You should only have a size, and then set up local variables for the other two:
unsigned int rx_rem_bytes = din ? size : 0; unsigned int tx_rem_bytes = dout ? size : 0;
-
Julius Werner (Code Review)