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:   Fri, 4 Feb 2022 14:09:21 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Krishna Yarlagadda <kyarlagadda@nvidia.com>
Cc:     thierry.reding@gmail.com, jonathanh@nvidia.com,
        linux-spi@vger.kernel.org, linux-tegra@vger.kernel.org,
        skomatineni@nvidia.com, ldewangan@nvidia.com, robh+dt@kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        p.zabel@pengutronix.de
Subject: Re: [PATCH 6/6] spi: tegra210-quad: combined sequence mode
Message-ID: <Yf0zkdS2nqHOZjMG@sirena.org.uk>
References: <1643970576-31503-1-git-send-email-kyarlagadda@nvidia.com>
 <1643970576-31503-7-git-send-email-kyarlagadda@nvidia.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="a24AQkG5lH/zcdwt"
Content-Disposition: inline
In-Reply-To: <1643970576-31503-7-git-send-email-kyarlagadda@nvidia.com>
Precedence: bulk


--a24AQkG5lH/zcdwt
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Feb 04, 2022 at 03:59:36PM +0530, Krishna Yarlagadda wrote:

> +	/* Process individual transfer list */
> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		if (transfer_phase == CMD_TRANSFER) {

> +		} else if (transfer_phase == ADDR_TRANSFER) {

> +		} else {

Looks like you're writing a switch statement here...

> +			/* X1 SDR mode */
> +			cmd_config = tegra_qspi_cmd_config(false, 0,
> +							   xfer->len);
> +			cmd_value = *((const u8 *)(xfer->tx_buf));
> +
> +			len = xfer->len;

> +			/* X1 SDR mode */
> +			addr_config = tegra_qspi_addr_config(false, 0,
> +							     xfer->len);
> +			address_value = *((const u32 *)(xfer->tx_buf));

> +			/* Program Command, Address value in register */
> +			tegra_qspi_writel(tqspi, cmd_value, QSPI_CMB_SEQ_CMD);
> +			tegra_qspi_writel(tqspi, address_value,
> +					  QSPI_CMB_SEQ_ADDR);
> +			/* Program Command and Address config in register */
> +			tegra_qspi_writel(tqspi, cmd_config,
> +					  QSPI_CMB_SEQ_CMD_CFG);
> +			tegra_qspi_writel(tqspi, addr_config,
> +					  QSPI_CMB_SEQ_ADDR_CFG);

It looks like the command and address have to be specific lengths?  If
that's the case then

> +	if (cdata->is_cmb_xfer && transfer_count == 3)
> +		ret = tegra_qspi_combined_seq_xfer(tqspi, msg);
> +	else
> +		ret = tegra_qspi_non_combined_seq_xfer(tqspi, msg);

This check needs to be more specific.  But like I said in reply to the
binding patch I don't see why we can't just pattern match on the data
without requiring a property here, we'd need to check that the message
is suitable no matter what.

--a24AQkG5lH/zcdwt
Content-Type: application/pgp-signature; name="signature.asc"

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmH9M5AACgkQJNaLcl1U
h9CHWAf9Fv7WpqdKpI5qGpLRrspS+onHqhjxBDK3qP8XYNE8GGIoewvk/y93TrmP
PT7a/zw1UGFtvHomlXqcnvm7KxGfbsOr+JfBbWQ8hrddfbof84GSaWNyRiJl80Y7
gmMNPp1WfNen9c5z0wwyH0LS2c+6W4PdOwgipGoGQd4gh1vVzM1zRCja47CM1BD6
r8WiE3UnnBch6gpC20Ggrb4qjb2piUie7wAdBcvf8Ol9CCU8olylsE9mkE8yTPXR
5LJ02ayJG9HxTRxlhTG8zEGlhXTq/MI/is7a14BFQt2E3r92WqQBt26dp3jKcmlS
k9jlTRaQCxNF0WpexPzjM5jpjPZefA==
=0qb3
-----END PGP SIGNATURE-----

--a24AQkG5lH/zcdwt--