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

Sat Jul 21 00:05:50 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 2:

(90 comments)

https://review.coreboot.org/#/c/27483/2/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/2/src/soc/qualcomm/sdm845/include/soc/qcom-geni-se.h@75
PS2, Line 75: 	(((0xFFFFFFFF) - (1UL << (l)) + 1) & (0xFFFFFFFF >> (BITS_PER_LONG - 1 - (h))))
line over 80 characters

https://review.coreboot.org/#/c/27483/2/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/2/src/soc/qualcomm/sdm845/include/soc/spi-geni-qcom.h@26
PS2, Line 26: }irq_status_t;
space required after that close brace '}'

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/include/soc/spi-geni-qcom.h@29
PS2, Line 29: 	u8* phys_addr;
"foo* bar" should be "foo *bar"

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/include/soc/spi-geni-qcom.h@48
PS2, Line 48:     u8 *rx_buf;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/include/soc/spi-geni-qcom.h@49
PS2, Line 49:     u8 *tx_buf;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c
File src/soc/qualcomm/sdm845/spi_qup.c:

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@28
PS2, Line 28: //helper funtions
'funtions' may be misspelled - perhaps 'functions'?

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@110
PS2, Line 110: #define min(x,y) ({ \
space required after that ',' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@117
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@118
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@125
PS2, Line 125:  #define REG_OUT(base, offset, value)                                           \
line over 80 characters

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@148
PS2, Line 148:     u8 * reg = base + offset;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@148
PS2, Line 148:     u8 * reg = base + offset;
"foo * bar" should be "foo *bar"

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@149
PS2, Line 149:     return *(volatile uint32_t *)(reg);
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@161
PS2, Line 161:     (*(volatile uint32_t *)(base + offset) = (value));
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@163
PS2, Line 163: }
void function return statements are not generally useful

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@418
PS2, Line 418: 							// & SPI_CS_CLK_DELAY_MSK;
line over 80 characters

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@436
PS2, Line 436: 	idx=1;
spaces required around that '=' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@437
PS2, Line 437: 	div=2;
spaces required around that '=' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@439
PS2, Line 439: 	mode =(slave_cfg->clk_polarity|slave_cfg->clk_phase);
spaces required around that '=' (ctx:WxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@448
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@451
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@534
PS2, Line 534: 	if (!mas->tx_rem_bytes) {
braces {} are not necessary for single statement blocks

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@544
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@594
PS2, Line 594: 	if(!(m_irq & IRQ_TRIGGER))
space required before the open parenthesis '('

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@614
PS2, Line 614: 			geni_write_reg(0, mas->phys_addr, SE_GENI_TX_WATERMARK_REG);
line over 80 characters

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@617
PS2, Line 617: 		if (mas->rx_rem_bytes)
suspect code indent for conditional statements (16, 16)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@629
PS2, Line 629: 	int max_delay=MAX_DELAY_STEP;
spaces required around that '=' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@631
PS2, Line 631: 	do{
space required before the open brace '{'

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@632
PS2, Line 632: 		if(geni_spi_irq(mas))
space required before the open parenthesis '('

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@636
PS2, Line 636: 	}while(max_delay > 0);
space required after that close brace '}'

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@636
PS2, Line 636: 	}while(max_delay > 0);
space required before the open parenthesis '('

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@638
PS2, Line 638: 	if(max_delay == 0)
space required before the open parenthesis '('

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@646
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@650
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@659
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@662
PS2, Line 662:  	u32 m_param = 0;
code indent should use tabs where possible

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@662
PS2, Line 662:  	u32 m_param = 0;
please, no space before tabs

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@662
PS2, Line 662:  	u32 m_param = 0;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@664
PS2, Line 664:  	u32 trans_len = 0;
code indent should use tabs where possible

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@664
PS2, Line 664:  	u32 trans_len = 0;
please, no space before tabs

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@664
PS2, Line 664:  	u32 trans_len = 0;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@665
PS2, Line 665: 	int bytes_per_word=0;
spaces required around that '=' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@667
PS2, Line 667:     spi_bus->num_xfers = 1;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@668
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@669
PS2, Line 669:     spi_bus->tx_buf = (u8 *)dout;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@670
PS2, Line 670:     spi_bus->rx_buf = (u8 *)din;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@671
PS2, Line 671:     spi_bus->tx_rem_bytes = 0;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@683
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@683
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@684
PS2, Line 684:  		trans_len =
code indent should use tabs where possible

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@684
PS2, Line 684:  		trans_len =
please, no space before tabs

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@684
PS2, Line 684:  		trans_len =
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@685
PS2, Line 685:  			((spi_bus->size << 3) / spi_bus->cur_word_len) & TRANS_LEN_MSK;
line over 80 characters

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@685
PS2, Line 685:  			((spi_bus->size << 3) / spi_bus->cur_word_len) & TRANS_LEN_MSK;
code indent should use tabs where possible

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@685
PS2, Line 685:  			((spi_bus->size << 3) / spi_bus->cur_word_len) & TRANS_LEN_MSK;
please, no space before tabs

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@685
PS2, Line 685:  			((spi_bus->size << 3) / spi_bus->cur_word_len) & TRANS_LEN_MSK;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@686
PS2, Line 686:  	} else {
code indent should use tabs where possible

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@686
PS2, Line 686:  	} else {
please, no space before tabs

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@686
PS2, Line 686:  	} else {
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@687
PS2, Line 687:  		bytes_per_word = (spi_bus->cur_word_len / BITS_PER_BYTE) + 1;
code indent should use tabs where possible

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@687
PS2, Line 687:  		bytes_per_word = (spi_bus->cur_word_len / BITS_PER_BYTE) + 1;
please, no space before tabs

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@687
PS2, Line 687:  		bytes_per_word = (spi_bus->cur_word_len / BITS_PER_BYTE) + 1;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@689
PS2, Line 689:  		trans_len = (spi_bus->size / bytes_per_word) & TRANS_LEN_MSK;
code indent should use tabs where possible

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@689
PS2, Line 689:  		trans_len = (spi_bus->size / bytes_per_word) & TRANS_LEN_MSK;
please, no space before tabs

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@689
PS2, Line 689:  		trans_len = (spi_bus->size / bytes_per_word) & TRANS_LEN_MSK;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@690
PS2, Line 690:  	}
code indent should use tabs where possible

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@690
PS2, Line 690:  	}
please, no space before tabs

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@690
PS2, Line 690:  	}
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@694
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@698
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@701
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@704
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@706
PS2, Line 706: 	if(spi_irq_poll(spi_bus)){
space required before the open brace '{'

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@706
PS2, Line 706: 	if(spi_irq_poll(spi_bus)){
space required before the open parenthesis '('

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@711
PS2, Line 711: 	else
else should follow close brace '}'

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@723
PS2, Line 723:     spi_bus->phys_addr = (u8 *)0x880000;
please, no spaces at the start of a line

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@724
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@724
PS2, Line 724:     u8* spi_bus_base   = spi_bus->phys_addr;
"foo* bar" should be "foo *bar"

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@731
PS2, Line 731: 	geni_se_init(spi_bus_base, 0,(spi_bus->rx_fifo_depth-2));
space required after that ',' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@733
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@734
PS2, 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/2/src/soc/qualcomm/sdm845/spi_qup.c@737
PS2, Line 737: 	REG_OUT(0,0x3500000,0x85);
space required after that ',' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@737
PS2, Line 737: 	REG_OUT(0,0x3500000,0x85);
space required after that ',' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@738
PS2, Line 738: 	REG_OUT(0,0x3501000,0x285);
space required after that ',' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@738
PS2, Line 738: 	REG_OUT(0,0x3501000,0x285);
space required after that ',' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@739
PS2, Line 739: 	REG_OUT(0,0x3502000,0x284);
space required after that ',' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@739
PS2, Line 739: 	REG_OUT(0,0x3502000,0x284);
space required after that ',' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@740
PS2, Line 740: 	REG_OUT(0,0x3503000,0x285);
space required after that ',' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@740
PS2, Line 740: 	REG_OUT(0,0x3503000,0x285);
space required after that ',' (ctx:VxV)

https://review.coreboot.org/#/c/27483/2/src/soc/qualcomm/sdm845/spi_qup.c@758
PS2, 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: 2
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:05:50 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180720/ae282894/attachment.html>