Re: [PATCH 2/2] spi: Add Nuvoton MA35D1 QSPI controller support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Passing a pointer to `op->cmd.opcode` (a `u16`) to a buffer-oriented transfer function causes command corruption on Big-Endian systems.
- [High] Failure to drain the RX FIFO during TX-only transfers causes the SPI controller to stall or drop data due to overflow.
- [High] The driver configures the bidirectional data pins as outputs during SPI memory dummy cycles, risking bus contention.
- [Medium] Memory operations (`spi_mem_exec_op`) will fail with `-EINVAL` if the client device configures a 16-bit or 32-bit default word size.
- [Low] The Kconfig symbol `SPI_MA35D1_QSPI` is missing architecture dependencies.
--

commit 6406cd6c82b9bd8e78df612f2de82f09e50e1f89
Author: Chi-Wen Weng <cwweng.linux@gmail.com>
Subject: spi: Add Nuvoton MA35D1 QSPI controller support

This commit adds support for the Nuvoton MA35D1 QSPI controller in master mode.

> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 957c3e065b839..7e67eebba2427 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -695,6 +695,12 @@ config SPI_LJCA
>  	  This driver can also be built as a module. If so, the module
>  	  will be called spi-ljca.
>  
> +config SPI_MA35D1_QSPI
> +	tristate "Nuvoton MA35D1 QSPI Controller"

[Severity: Low]
Should this config symbol include architecture dependencies, such as
depends on ARCH_MA35 || COMPILE_TEST? 

Adding dependencies would help prevent cluttering the configuration menu for
users building kernels for unrelated hardware architectures.

> +	help
> +	  This driver provides support for Nuvoton MA35D1
> +	  QSPI controller in master mode.

[ ... ]

> diff --git a/drivers/spi/spi-ma35d1-qspi.c b/drivers/spi/spi-ma35d1-qspi.c
> new file mode 100644
> index 0000000000000..119c06464cb01
> --- /dev/null
> +++ b/drivers/spi/spi-ma35d1-qspi.c

[ ... ]

> +static int nuvoton_qspi_txrx(struct nuvoton_qspi *qspi, const void *txbuf,
> +			     void *rxbuf, unsigned int len)
> +{

[ ... ]

> +	for (i = 0; i < words; i++) {
> +		ret = nuvoton_qspi_wait_tx_not_full(qspi);
> +		if (ret) {
> +			dev_err(qspi->dev, "TX FIFO full timeout\n");
> +			return ret;
> +		}
> +
> +		nuvoton_qspi_write(qspi, nuvoton_qspi_tx_word(txbuf, i,
> +							      qspi->bits_per_word),
> +				   NUVOTON_QSPI_TX_OFFSET);
> +
> +		if (rxbuf) {
> +			ret = nuvoton_qspi_wait_rx_not_empty(qspi);
> +			if (ret) {
> +				dev_err(qspi->dev, "RX FIFO empty timeout\n");
> +				return ret;
> +			}
> +
> +			val = nuvoton_qspi_read(qspi, NUVOTON_QSPI_RX_OFFSET);
> +			nuvoton_qspi_rx_word(rxbuf, i, val, qspi->bits_per_word);
> +		}
> +	}

[Severity: High]
Does skipping the RX FIFO read when rxbuf is NULL cause the RX FIFO to overflow
during TX-only transfers?

In standard SPI operations, every transmitted word also shifts a word into
the RX FIFO. Because this loop never empties the RX FIFO in transmit-only cases
(like a flash Page Program), could the FIFO rapidly fill up and cause the SPI
controller state machine to stall or drop data? 

A dummy read might be needed for every word written, even when rxbuf is not
provided.

[ ... ]

> +static int nuvoton_qspi_mem_exec_op(struct spi_mem *mem,
> +				    const struct spi_mem_op *op)
> +{
> +	struct spi_device *spi = mem->spi;
> +	struct nuvoton_qspi *qspi = spi_controller_get_devdata(spi->controller);
> +	u8 addr[4];
> +	int ret;
> +	int i;
> +
> +	ret = nuvoton_qspi_setup_transfer(spi, NULL);

[Severity: Medium]
Will this inherit the default word size from the SPI device?

If a client device configures a 16-bit or 32-bit default word size,
bytes_per_word will become 2 or 4. When the driver attempts to transmit the
1-byte command phase below (len = 1), nuvoton_qspi_txrx() will execute its
modulus check and return -EINVAL. 

Could this explicitly enforce an 8-bit word size for memory operations instead
of relying on the device's default?

> +	if (ret)
> +		return ret;
> +
> +	nuvoton_qspi_set_cs(spi, true);
> +
> +	nuvoton_qspi_set_bus_width(qspi, op->cmd.buswidth, SPI_MEM_DATA_OUT);
> +	ret = nuvoton_qspi_txrx(qspi, &op->cmd.opcode, NULL, 1);

[Severity: High]
Could passing the address of op->cmd.opcode cause command corruption on
big-endian systems?

The op->cmd.opcode field is defined as a u16 in struct spi_mem_op. When passed
directly to nuvoton_qspi_txrx() as a byte buffer (const void *txbuf), the
function will read the first byte at the lowest memory address. 

On big-endian platforms, this reads the most significant byte (0x00) instead
of the actual command byte, which would break SPI flash communication.

> +	if (ret)
> +		goto out_deassert_cs;

[ ... ]

> +	if (op->dummy.nbytes) {
> +		nuvoton_qspi_set_bus_width(qspi, op->dummy.buswidth,
> +					   SPI_MEM_DATA_OUT);

[Severity: High]
Is it safe to hardcode SPI_MEM_DATA_OUT for the dummy phase?

For Dual or Quad I/O modes (buswidth > 1), setting the data direction to output
configures the SPI data pins to actively drive the bus. However, dummy cycles
in SPI flash protocols provide a high-impedance turnaround time so the flash
chip can prepare to drive the bus for the data input phase. 

Actively driving the lines from the host controller during this phase might
lead to direct bus contention.

> +		ret = nuvoton_qspi_txrx(qspi, NULL, NULL, op->dummy.nbytes);
> +		if (ret)
> +			goto out_deassert_cs;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603043551.1062112-1-cwweng.linux@gmail.com?part=2