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.
Attachment:
signature.asc
Description: PGP signature
