Attention is currently required from: Thomas Heijligen, Jean THOMAS.

View Change

21 comments:

• Patchset:

  • Patch Set #4:

    Looks like a lot, but actually just some minor issues :) Looks good to me else. Thanks for contributing!

• File dirtyjtag_spi.c:

  • Patch Set #4, Line 34: DJTAGUNK

    It seems this is unused?

    What does UNK mean? Unknown? If so, I would write that out.

  • Patch Set #4, Line 39: 0xC0CA

    Please use lower case

  • Patch Set #4, Line 47: dirytjtag_command_identifier

    typo: dir*ty*jtag

  • Patch Set #4, Line 57: dirytjtag_signal_identifier

    typo: dir*ty*jtag

  • Patch Set #4, Line 66: char* data

    `char *data`

  • Patch Set #4, Line 87: char* data

    `char *data`

  • Patch Set #4, Line 125: static int dirtyjtag_djtag1_spi_send_command(struct dirtyjtag_spi_data *context, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr)

    This is also pretty long. Same as with line 174.

  • Patch Set #4, Line 128: writecnt+readcnt

    spaces

  • Patch Set #4, Line 129: size_t num_xfer = (len+max_xfer_size-1)/max_xfer_size; // ceil(len/max_xfer_size)

    spaces

  • Patch Set #4, Line 131: char *tx_buf = malloc(max_xfer_size*num_xfer);

    spaces

  • Patch Set #4, Line 136: char *rx_buf = malloc(max_xfer_size*num_xfer);

    spaces

  • Patch Set #4, Line 146: len%max_xfer_size

    add spaces before and after %

  • Patch Set #4, Line 147: len%max_xfer_size

    same here

  • Patch Set #4, Line 149: txn_size = 30;

    How about setting this value during declaration in line 145? So it would be the "default" and if applicable it gets overridden. This way we can remove the else-block then.

  • Patch Set #4, Line 152: tx_buf+30*i

    same here

  • Patch Set #4, Line 155: rx_buf+i*30

    same here

  • Patch Set #4, Line 174: static int dirtyjtag_spi_spi_send_command(const struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr)

    That line is pretty long. Please move each argument into a separate line or group them somehow. For example `writecnt` and `readcnt` could go into one line. Same with `writearr` and `readarr`.

  • Patch Set #4, Line 248: tmp, strerror(errno));

    nit: I would use one line here. It's not that long.

  • Patch Set #4, Line 289: 0xFF

    lower case

  • Patch Set #4, Line 290: 0xFF

    make this lower case

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