Re: [PATCH 2/2] spi: spi-qcom-qspi: Add DMA mode support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



All reviewers,

Thank you very much for your time and review...

While I am addressing other comments, below are some responses...


On 4/4/2023 11:47 PM, Mark Brown wrote:
On Tue, Apr 04, 2023 at 11:33:20PM +0530, Vijaya Krishna Nivarthi wrote:

A few quick review comments, mostly coding style though.

+struct qspi_cmd_desc {
+	uint32_t data_address;
+	uint32_t next_descriptor;
+	uint32_t direction:1;
+	uint32_t multi_io_mode:3;
+	uint32_t reserved1:4;
+	uint32_t fragment:1;
+	uint32_t reserved2:7;
+	uint32_t length:16;
+	//------------------------//
What does this mean?

That separates the part of cmd_desc that is visible to the HW and the part that is required by the SW after xfer is complete.
I can add a comment in v2?



+	uint8_t *bounce_src;
+	uint8_t *bounce_dst;
+	uint32_t bounce_length;
+};
+
+#define QSPI_MAX_NUM_DESC 5
  struct qspi_xfer {
Missing blank line after the define...


Will address in v2


+	for (ii = 0; ii < sgt->nents; ii++)
+		sg_total_len += sg_dma_len(sgt->sgl + ii);
Why are we calling the iterator ii here?


Calling it ii helps in finding iterator more easily in code.

should I stick to i in v2?


+	if (ctrl->xfer.dir == QSPI_READ)
+		byte_ptr = (uint8_t *)xfer->rx_buf;
+	else
+		byte_ptr = (uint8_t *)xfer->tx_buf;
If we need to cast to or from void * there's some sort of problem.


the tx_buf is a const void*

in v2 I will cast for tx_buf only?


+/* Switch to DMA if transfer length exceeds this */
+#define QSPI_MAX_BYTES_FIFO 64
+#define NO_TX_DATA_DELAY_FOR_DMA 1
+#define DMA_CONDITIONAL (xfer->len > QSPI_MAX_BYTES_FIFO)
+
DMA_CONDITIONAL absolutely should not be a define, this just makes
things harder to read.  Just have everything call can_dma() when needed.


Will address in v2


+#if NO_TX_DATA_DELAY_FOR_DMA
+		mstr_cfg &= ~(TX_DATA_OE_DELAY_MSK | TX_DATA_DELAY_MSK);
+#endif
Why would we add extra delays if we don't need them, might someone set
this and if so when?


I believe its used when some slave devices need a delay between clock and data.

Its configured as 1 for PIO_MODE(FIFO) right now.

For DMA_MODE we are not using same, both seem to work for DMA.


+	if (ctrl->xfer_mode == QSPI_FIFO) {
+	} else if (ctrl->xfer_mode == QSPI_DMA) {
  	}
This should be a switch statement.


Will address in v2




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux