Received-SPF: pass (google.com: domain of linux-kernel+bounces-208540-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 10 Jun 2024 17:19:16 +0100
From: Mark Brown <broonie@kernel.org>
To: Witold Sadowski <wsadowski@marvell.com>
Cc: linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org,
	devicetree@vger.kernel.org, robh@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	pthombar@cadence.com
Subject: Re: [PATCH v8 4/4] spi: cadence: Add MRVL overlay xfer operation
 support
Message-ID: <ZmcnhGH2fcmrXn1G@finisterre.sirena.org.uk>
References: <20240607151831.3858304-1-wsadowski@marvell.com>
 <20240607151831.3858304-5-wsadowski@marvell.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="QscN4rSe5xkktnMl"
Content-Disposition: inline
In-Reply-To: <20240607151831.3858304-5-wsadowski@marvell.com>


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

On Fri, Jun 07, 2024 at 08:18:31AM -0700, Witold Sadowski wrote:

> +static int cdns_xspi_prepare_generic(int cs, const void *dout, int len, int glue, u32 *cmd_regs)
> +{
> +	u8 *data = (u8 *)dout;
> +	int i;
> +	int data_counter = 0;
> +
> +	memset(cmd_regs, 0x00, 6*4);

The magic numbers here aren't great...

> +static unsigned char reverse_bits(unsigned char num)
> +{
> +	unsigned int count = sizeof(num) * 8 - 1;
> +	unsigned int reverse_num = num;
> +
> +	num >>= 1;
> +	while (num) {
> +		reverse_num <<= 1;
> +		reverse_num |= num & 1;
> +		num >>= 1;
> +		count--;
> +	}
> +	reverse_num <<= count;
> +	return reverse_num;
> +}

I can't help but think there ought to be a helper for this though I
can't think what it is off the top of my head.  If there isn't it
probably makes sense to add this as one.

> +	/* Enable xfer state machine */
> +	if (!cdns_xspi->xfer_in_progress) {
> +		u32 xfer_control = readl(cdns_xspi->xferbase + MRVL_XFER_FUNC_CTRL);
> +
> +		cdns_xspi->current_xfer_qword = 0;
> +		cdns_xspi->xfer_in_progress = true;
> +		xfer_control |= (MRVL_XFER_RECEIVE_ENABLE |
> +				 MRVL_XFER_CLK_CAPTURE_POL |
> +				 MRVL_XFER_FUNC_START |
> +				 MRVL_XFER_SOFT_RESET |
> +				 FIELD_PREP(MRVL_XFER_CS_N_HOLD, (1 << cs)));
> +		xfer_control &= ~(MRVL_XFER_FUNC_ENABLE | MRVL_XFER_CLK_DRIVE_POL);
> +		writel(xfer_control, cdns_xspi->xferbase + MRVL_XFER_FUNC_CTRL);
> +	}

Could this just be a prepare_transfer_hardware() and we could just use
transfer_one()?

> +	list_for_each_entry(t, &m->transfers, transfer_list) {
> +		u8 *txd = (u8 *) t->tx_buf;
> +		u8 *rxd = (u8 *) t->rx_buf;
> +		u8 data[10];
> +		u32 cmd_regs[6];
> +
> +		if (!txd)
> +			txd = data;
> +
> +		cdns_xspi->in_buffer = txd + 1;
> +		cdns_xspi->out_buffer = txd + 1;

Oh?

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

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmZnJ4QACgkQJNaLcl1U
h9Dp1wf/WWCSFxYA9ICwOA6a517Qv5oZM6qYTRD9DXmo/lVgjct+IPOYv8EzpMJn
6R13HN446bDgxlYUEKBj3khExObjLdA+JbrEa50OV6ylKDWUcYfhGw7l4FNKQdmp
dlztImDchBdOisfrcVyLk/yzZZ5YHlFzmYldqrcVDQYQySDtuRt3uGm7wwWDdcu4
BaOSqwoBskBGobu3S/TFdOa1wNusmc8s6K3Ti5j6y0q0X1lgRfmanltHuD6XBWn5
NGrkO7xN/v2DyQKmCkKcDpSWUllFYEqHDhDZjr8EkrUQlkIvhtI5Tf1/E1r5qyOY
kkyDt6TcBguiNI+NuByW8RL9kB2QjA==
=LJaH
-----END PGP SIGNATURE-----

--QscN4rSe5xkktnMl--