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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]