[coreboot-gerrit] Change in coreboot[master]: sdm845: Add SPI QUP driver

Sat Jul 21 00:17:51 CEST 2018

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/qcom-geni-se.h
File src/soc/qualcomm/sdm845/include/soc/qcom-geni-se.h:

https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/include/soc/qcom-geni-se.h@75
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/spi-geni-qcom.h
File src/soc/qualcomm/sdm845/include/soc/spi-geni-qcom.h:

https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/include/soc/spi-geni-qcom.h@26
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/spi-geni-qcom.h@29
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/spi-geni-qcom.h@48
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/spi-geni-qcom.h@49
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@110
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@117
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@118
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@125
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@148
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@148
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@149
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@161
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@163
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@418
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@436
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@437
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@439
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@448
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@451
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@534
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@544
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@614
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@617
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@629
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@636
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@636
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@646
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@650
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@659
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@665
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@667
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@668
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@669
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@670
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@671
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@683
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@683
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@685
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@694
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@698
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@701
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@704
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@711
PS3, Line 711: 	else
else should follow close brace '}'

https://review.coreboot.org/#/c/27483/3/src/soc/qualcomm/sdm845/spi_qup.c@723
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@724
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@724
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@733
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@734
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@758
PS3, Line 758: }
void function return statements are not generally useful

-- 
To view, visit https://review.coreboot.org/27483
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I35061727d5ccc550eaeb06caef4524bc4cf25b54
Gerrit-Change-Number: 27483
Gerrit-PatchSet: 3
Gerrit-Owner: T.Michael Turney <tturne at codeaurora.org>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: T Michael Turney <mturney at codeaurora.org>
Gerrit-Reviewer: Vin Kamath <vink at codeaurora.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Julius Werner <jwerner at chromium.org>
Gerrit-Comment-Date: Fri, 20 Jul 2018 22:17:51 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180720/5d89e3a4/attachment.html>