View Change

8 comments:

• File src/soc/qualcomm/sdm845/i2c.c:

  • Patch Set #31, 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");
     }

  • Patch Set #31, 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.

  • Patch Set #31, 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;
       ...
     }

• File src/soc/qualcomm/sdm845/include/soc/i2c.h:

  • Patch Set #31, 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?

• File src/soc/qualcomm/sdm845/qcom_qup_se.c:

  • Patch Set #31, 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.).

  • Patch Set #31, Line 118:

    Please fix indentation.

  • Patch Set #31, 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?

  • Patch Set #31, 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;

To view, visit change 29104. To unsubscribe, or for help writing mail filters, visit settings.