Akash Asthana has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29104 )
Change subject: sdm845: Port I2C driver ......................................................................
Patch Set 31:
(8 comments)
I will share the final changes with Mike to upload here. Can you please answer the query on indentation.
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 inste […]
Done
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 […]
Sorry about it, I should've mentioned that changes are not yet shared here.
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, y […]
Done
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. […]
Done
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? […]
Done, We want to export handle_error and wait_till_irq_set functions as well, I have renamed them as suggested.
https://review.coreboot.org/#/c/29104/31/src/soc/qualcomm/sdm845/qcom_qup_se... PS31, Line 118:
Please fix indentation.
Similar indentation we use in our HLOS driver and it looks good to us. Not sure whether any fixed rule exists here to break a line having more than 80 characters. How do you suggest to indent here?
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 transfe […]
Done
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. […]
Done
-
Akash Asthana (Code Review)