Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 4 Apr 2023 19:17:08 +0100
From:   Mark Brown <broonie@kernel.org>
To:     Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
Cc:     agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        vkoul@kernel.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-spi@vger.kernel.org, quic_msavaliy@quicinc.com,
        dianders@chromium.org, mka@chromium.org, swboyd@chromium.org,
        quic_vtanuku@quicinc.com
Subject: Re: [PATCH 2/2] spi: spi-qcom-qspi: Add DMA mode support
Message-ID: <d784dab7-a1a6-4db7-aa13-e39e9904f342@sirena.org.uk>
References: <1680631400-28865-1-git-send-email-quic_vnivarth@quicinc.com>
 <1680631400-28865-3-git-send-email-quic_vnivarth@quicinc.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="kDrRpbySrwTW+T6a"
Content-Disposition: inline
In-Reply-To: <1680631400-28865-3-git-send-email-quic_vnivarth@quicinc.com>
Precedence: bulk


--kDrRpbySrwTW+T6a
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

--kDrRpbySrwTW+T6a
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmQsaaMACgkQJNaLcl1U
h9BZfQf9E/FbfM9Y8h2lD6huqt78OFAf4jE1tjFt0kIcwXUTKo8u2VXaJ8sshPn5
rIlF+ebk/XkQ1XkEyxP/oaCj8W5Jnut1nIjMAkjKrwjN/vrHN8O/HO5C9A0SXq3p
uxefYwl9dgHK15YL+GpqD+syqm1KglJKE57uJGcV/FMD3UraYXrxBU0LH+nHCkR8
fK7tGA0V6YyMazCSXCP9gmd5FL/kdriaX+arH/WErKhxFeLsd4aiqwNSXsV0S9My
xbWwJd55w0umfou96GNxqblH5CCyQSFjBdhu34EKXTr3nNnX3a5WT25tE+xWfcAf
EUqHBrMNpvUHv9xGonWDnEicW50ylA==
=qQMD
-----END PGP SIGNATURE-----

--kDrRpbySrwTW+T6a--