Re: [PATCH 3/5] spi: spi-qpic: Add qpic spi nand driver support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 2/15/2024 7:44 PM, Mark Brown wrote:
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?
  Sorry by mistake I kept this enabled by default, will change in next
  patch.

+	depends on ARCH_QCOM

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

+	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.
Ok

--- /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.
Ok

+#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.
Ok

+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.
Ok

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

For legibility please just write normal conditional statements.
Ok

+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 *.
Ok Will fix in next patch.

+	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?
QPIC controller internally works on code word size not on page and each
code word size is 512-bytes so if page size is 2K then num_cw = 4, if page
size is 4K then num_cw = 8.
Will not hard code this value to 4 or 8 , will fix this in next patch.

+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?
  This reg_read_buf is being used in common API so that it will be used by both
  serial nand as well raw nand, so I can't directly use the output buffer since
  internally CW mechanism I have to maintain in common API.

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

This function looks like it should be a switch statement.
Ok

+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.
Ok

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

Please use _alloc_controller.
Ok

+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.
OK will fix in next patch.

Thanks for reviewing, will address all your comments in the next patch.

Regards,
Alam.




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux