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?
> + 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...
> + for (ii = 0; ii < sgt->nents; ii++)
> + sg_total_len += sg_dma_len(sgt->sgl + ii);
Why are we calling the iterator ii here?
> + 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.
> +/* 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.
> +#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?
> + if (ctrl->xfer_mode == QSPI_FIFO) {
> + } else if (ctrl->xfer_mode == QSPI_DMA) {
> }
This should be a switch statement.
Attachment:
signature.asc
Description: PGP signature
