build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/27483 )
Change subject: sdm845: Add SPI QUP driver ......................................................................
Patch Set 3:
(51 comments)
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/include/soc/... File src/soc/qualcomm/sdm845/include/soc/qcom-geni-se.h:
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/include/soc/... PS3, Line 75: (((0xFFFFFFFF) - (1UL << (l)) + 1) & (0xFFFFFFFF >> (BITS_PER_LONG - 1 - (h)))) line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/include/soc/... File src/soc/qualcomm/sdm845/include/soc/spi-geni-qcom.h:
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/include/soc/... PS3, Line 26: }irq_status_t; space required after that close brace '}'
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/include/soc/... PS3, Line 29: u8* phys_addr; "foo* bar" should be "foo *bar"
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/include/soc/... PS3, Line 48: u8 *rx_buf; please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/include/soc/... PS3, Line 49: u8 *tx_buf; please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c File src/soc/qualcomm/sdm845/spi_qup.c:
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@28 PS3, Line 28: //helper funtions 'funtions' may be misspelled - perhaps 'functions'?
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@11... PS3, Line 110: #define min(x, y) ( { \ space prohibited after that open parenthesis '('
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@11... PS3, Line 117: #define IRQ_TRIGGER (M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN | M_TX_FIFO_WATERMARK_EN \ line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@11... PS3, Line 118: | M_CMD_DONE_EN | M_CMD_CANCEL_EN | M_CMD_ABORT_EN) please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@12... PS3, Line 125: #define REG_OUT(base, offset, value) \ line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@14... PS3, Line 148: u8 * reg = base + offset; please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@14... PS3, Line 148: u8 * reg = base + offset; "foo * bar" should be "foo *bar"
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@14... PS3, Line 149: return *(volatile uint32_t *)(reg); please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@16... PS3, Line 161: (*(volatile uint32_t *)(base + offset) = (value)); please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@16... PS3, Line 163: } void function return statements are not generally useful
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@41... PS3, Line 418: // & SPI_CS_CLK_DELAY_MSK; line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@43... PS3, Line 436: idx=1; spaces required around that '=' (ctx:VxV)
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@43... PS3, Line 437: div=2; spaces required around that '=' (ctx:VxV)
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@43... PS3, Line 439: mode =(slave_cfg->clk_polarity|slave_cfg->clk_phase); spaces required around that '=' (ctx:WxV)
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@44... PS3, Line 448: geni_write_reg(demux_output_inv, spi_bus->phys_addr, SE_SPI_DEMUX_OUTPUT_INV); line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@45... PS3, Line 451: geni_write_reg(spi_delay_params, spi_bus->phys_addr, SE_SPI_DELAY_COUNTERS); line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@53... PS3, Line 534: if (!mas->tx_rem_bytes) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@54... PS3, Line 544: u32 rx_fifo_status = geni_read_reg(mas->phys_addr, SE_GENI_RX_FIFO_STATUS); line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@61... PS3, Line 614: geni_write_reg(0, mas->phys_addr, SE_GENI_TX_WATERMARK_REG); line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@61... PS3, Line 617: if (mas->rx_rem_bytes) suspect code indent for conditional statements (16, 16)
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@62... PS3, Line 629: int max_delay=MAX_DELAY_STEP; spaces required around that '=' (ctx:VxV)
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@63... PS3, Line 636: }while(max_delay > 0); space required after that close brace '}'
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@63... PS3, Line 636: }while(max_delay > 0); space required before the open parenthesis '('
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@64... PS3, Line 646: geni_write_reg(M_GENI_CMD_CANCEL, mas->phys_addr, SE_GENI_M_CMD_CTRL_REG); line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@65... PS3, Line 650: geni_write_reg(M_GENI_CMD_ABORT, mas->phys_addr, SE_GENI_M_CMD_CTRL_REG); line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@65... PS3, Line 659: struct spi_geni_master *spi_bus = to_spi_geni_master(slave); please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@66... PS3, Line 665: int bytes_per_word=0; spaces required around that '=' (ctx:VxV)
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@66... PS3, Line 667: spi_bus->num_xfers = 1; please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@66... PS3, Line 668: spi_bus->size = bytes_out > bytes_in ? bytes_out : bytes_in; please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@66... PS3, Line 669: spi_bus->tx_buf = (u8 *)dout; please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@67... PS3, Line 670: spi_bus->rx_buf = (u8 *)din; please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@67... PS3, Line 671: spi_bus->tx_rem_bytes = 0; please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@68... PS3, Line 683: if (!(spi_bus->cur_word_len % MIN_WORD_LEN)) { please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@68... PS3, Line 683: if (!(spi_bus->cur_word_len % MIN_WORD_LEN)) { suspect code indent for conditional statements (4, 16)
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@68... PS3, Line 685: ((spi_bus->size << 3) / spi_bus->cur_word_len) & TRANS_LEN_MSK; line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@69... PS3, Line 694: geni_write_reg(trans_len, spi_bus->phys_addr, SE_SPI_TX_TRANS_LEN); line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@69... PS3, Line 698: geni_write_reg(trans_len, spi_bus->phys_addr, SE_SPI_RX_TRANS_LEN); line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@70... PS3, Line 701: geni_setup_m_cmd(spi_bus->phys_addr, m_cmd, m_param); please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@70... PS3, Line 704: geni_write_reg(spi_bus->tx_wm, spi_bus->phys_addr, SE_GENI_TX_WATERMARK_REG); line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@71... PS3, Line 711: else else should follow close brace '}'
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@72... PS3, Line 723: spi_bus->phys_addr = (u8 *)0x880000; please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@72... PS3, Line 724: u8* spi_bus_base = spi_bus->phys_addr; please, no spaces at the start of a line
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@72... PS3, Line 724: u8* spi_bus_base = spi_bus->phys_addr; "foo* bar" should be "foo *bar"
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@73... PS3, Line 733: /*TODO :slave_cfg Should be set by client before xfer, initializing default values*/ line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@73... PS3, Line 734: spi_bus->slave_cfg = (struct spi_cfg) {SPI_CLOCK_PHASE_FIRST, SPI_POLARITY_LOW, SPI_POLARITY_LOW, SPI_4_WIRE_MODE, 0x8}; line over 80 characters
https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@75... PS3, Line 758: } void function return statements are not generally useful