Received-SPF: pass (google.com: domain of linux-kernel+bounces-67068-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 15 Feb 2024 14:14:29 +0000
From: Mark Brown <broonie@kernel.org>
To: Md Sadre Alam <quic_mdalam@quicinc.com>
Cc: andersson@kernel.org, konrad.dybcio@linaro.org, robh@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	miquel.raynal@bootlin.com, richard@nod.at, vigneshr@ti.com,
	manivannan.sadhasivam@linaro.org, linux-arm-msm@vger.kernel.org,
	linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org,
	quic_srichara@quicinc.com, quic_varada@quicinc.com
Subject: Re: [PATCH 3/5] spi: spi-qpic: Add qpic spi nand driver support
Message-ID: <21dde665-54b4-48e4-b963-1008ac890df3@sirena.org.uk>
References: <20240215134856.1313239-1-quic_mdalam@quicinc.com>
 <20240215134856.1313239-4-quic_mdalam@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="XhokIFG6tLIbxTBQ"
Content-Disposition: inline
In-Reply-To: <20240215134856.1313239-4-quic_mdalam@quicinc.com>


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

On Thu, Feb 15, 2024 at 07:18:54PM +0530, Md Sadre Alam wrote:

> +config SPI_QPIC_SNAND
> +	tristate "QPIC SNAND controller"
> +	default y

Why is this driver so special it should be enabled by default?

> +	depends on ARCH_QCOM

Please add an || COMPILE_TEST so this gets some build coverage.

> +	help
> +	  QPIC_SNAND (QPIC SPI NAND) driver for Qualcomm QPIC controller.
> +	  QPIC controller supports both parallel nand and serial nand.
> +	  This config will enable serial nand driver for QPIC controller.
> +
>  config SPI_QUP
>  	tristate "Qualcomm SPI controller with QUP interface"
>  	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 4ff8d725ba5e..1ac3bac35007 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -153,6 +153,7 @@ obj-$(CONFIG_SPI_XTENSA_XTFPGA)		+= spi-xtensa-xtfpga.o
>  obj-$(CONFIG_SPI_ZYNQ_QSPI)		+= spi-zynq-qspi.o
>  obj-$(CONFIG_SPI_ZYNQMP_GQSPI)		+= spi-zynqmp-gqspi.o
>  obj-$(CONFIG_SPI_AMD)			+= spi-amd.o
> +obj-$(CONFIG_SPI_QPIC_SNAND)            += spi-qpic-snand.o

Please keep this sorted.

> --- /dev/null
> +++ b/drivers/spi/spi-qpic-snand.c
> @@ -0,0 +1,1025 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.

Please make the entire comment a C++ one so things look more
intentional.

> +#define snandc_set_read_loc_first(snandc, reg, cw_offset, read_size, is_last_read_loc)	\
> +snandc_set_reg(snandc, reg,			\
> +	      ((cw_offset) << READ_LOCATION_OFFSET) |		\
> +	      ((read_size) << READ_LOCATION_SIZE) |			\
> +	      ((is_last_read_loc) << READ_LOCATION_LAST))
> +
> +#define snandc_set_read_loc_last(snandc, reg, cw_offset, read_size, is_last_read_loc)	\
> +snandc_set_reg(snandc, reg,			\
> +	      ((cw_offset) << READ_LOCATION_OFFSET) |		\
> +	      ((read_size) << READ_LOCATION_SIZE) |			\
> +	      ((is_last_read_loc) << READ_LOCATION_LAST))

For type safety and legibility please write these as functions, mark
them as static inline if needed.

> +void snandc_set_reg(struct qcom_nand_controller *snandc, int offset, u32 val)
> +{
> +	struct nandc_regs *regs = snandc->regs;
> +	__le32 *reg;
> +
> +	reg = offset_to_nandc_reg(regs, offset);
> +
> +	if (reg)
> +		*reg = cpu_to_le32(val);
> +}

This silently ignores writes to invalid registers, that doesn't seem
great.

> +	return snandc->ecc_stats.failed ? -EBADMSG : snandc->ecc_stats.bitflips;

For legibility please just write normal conditional statements.

> +static int qpic_snand_program_execute(struct qcom_nand_controller *snandc,
> +				      const struct spi_mem_op *op)
> +{

> +       int num_cw = 4;

> +	data_buf = (u8 *)snandc->wbuf;

Why the cast?  If it's needed that smells like it's masking a bug, it
looks like it's casting from a u8 * to a u8 *.

> +	for (i = 0; i < num_cw; i++) {
> +		int data_size;

All these functions appear to hard code "num_cw" to 4.  What is "num_cw"
and why are we doing this per function?

> +static int qpic_snand_program_execute(struct qcom_nand_controller *snandc,
> +                                     const struct spi_mem_op *op)

> +	if (op->cmd.opcode == SPINAND_READID) {
> +		snandc->buf_count = 4;
> +		read_reg_dma(snandc, NAND_READ_ID, 1, NAND_BAM_NEXT_SGL);
> +
> +		ret = submit_descs(snandc);
> +		if (ret)
> +			dev_err(snandc->dev, "failure in submitting descriptor for readid\n");
> +
> +		nandc_read_buffer_sync(snandc, true);
> +		memcpy(op->data.buf.in, snandc->reg_read_buf, snandc->buf_count);

These memcpy()s don't seem great, why aren't we just reading directly
into the output buffer?

> +	if (op->cmd.opcode == SPINAND_GET_FEATURE) {

This function looks like it should be a switch statement.

> +static bool qpic_snand_is_page_op(const struct spi_mem_op *op)
> +{

> +	if (op->data.dir == SPI_MEM_DATA_IN) {
> +		if (op->addr.buswidth == 4 && op->data.buswidth == 4)
> +			return true;
> +
> +		if (op->addr.nbytes == 2 && op->addr.buswidth == 1)
> +			return true;
> +
> +	} else if (op->data.dir == SPI_MEM_DATA_OUT) {
> +		if (op->data.buswidth == 4)
> +			return true;
> +		if (op->addr.nbytes == 2 && op->addr.buswidth == 1)
> +			return true;
> +	}

Again looks like a switch statement.

> +	ctlr = devm_spi_alloc_master(dev, sizeof(*snandc));
> +	if (!ctlr)
> +		return -ENOMEM;

Please use _alloc_controller.

> +static int qcom_snand_remove(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> +
> +	spi_unregister_controller(ctlr);
> +
> +	return 0;
> +}

We don't disable any of the clocks in the remove path.

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

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmXOHEQACgkQJNaLcl1U
h9DPNQf+J50wqlt9PTavj5k8BZ9Z6zwjzegCMvrnHopmC1YAf4xbXkQCAQxV7emn
twIkNpsgq47QVVagkvGaKItUPEEBYLfAD0xSueD9f45evFnwqXWn0vKkaVIN4rwu
1G/1V7E6yS71iuctmkMd1QDTey4Q2zG869/GYDKkBC01JFJ5yWzMi8kpCetcJYs1
D1a6PVUMWv6wWaCGIcXPjPXguhKmy2S3Z6WSqZ/pr8hxDfNHgy58ie9lvh71axNc
AM+nlRRggHB6he9/9EtGMxe1nNAmFa5i6rIPVHlPYgbmKiy9H325PnGwXsoJ0GRb
3BRpV35HMNNSHJyfXyBP/J3nWz7MqQ==
=n07f
-----END PGP SIGNATURE-----

--XhokIFG6tLIbxTBQ--