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

[Chi-Wen Weng]

Hi Mark,

Thanks for the review.

I will address these in v2:
- add "depends on ARCH_MA35 || COMPILE_TEST" to the Kconfig entry;
- convert the file header to // comments;
- split the low-level CS register update from the SPI core .set_cs()
  callback, and make the spi-mem direct CS path handle SPI_CS_HIGH
  explicitly;
- use op->max_freq for spi-mem operations instead of spi->max_speed_hz.

I will also fix the subject lines in the next version.

Best regards,
Chi-Wen Weng

Mark Brown 於 2026/6/3 下午 05:30 寫道:
> On Wed, Jun 03, 2026 at 12:35:51PM +0800, Chi-Wen Weng wrote:
>
> > +config SPI_MA35D1_QSPI
> > +	tristate "Nuvoton MA35D1 QSPI Controller"
> > +	help
> > +	  This driver provides support for Nuvoton MA35D1
> > +	  QSPI controller in master mode.
> > +
> Other drivers for this SoC seem to have ARCH_MA35 || COMPILE_TEST?
>
> > @@ -0,0 +1,579 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Nuvoton MA35D1 QSPI controller driver
> > + *
> > + * Copyright (c) 2026 Nuvoton Technology Corp.
> > + * Author: Chi-Wen Weng <cwweng@xxxxxxxxxxx>
> > + */
> Please make the entire comment a C++ one so things look more
> intentional.
>
> > +static void nuvoton_qspi_set_cs(struct spi_device *spi, bool enable)
> > +{
> > +	struct nuvoton_qspi *qspi = spi_controller_get_devdata(spi->controller);
> > +	unsigned int cs = spi_get_chipselect(spi, 0);
> > +	u32 mask;
> > +	u32 val;
> > +
> > +	if (cs == 0)
> > +		mask = NUVOTON_QSPI_SSCTL_SS0_MASK;
> > +	else
> > +		mask = NUVOTON_QSPI_SSCTL_SS1_MASK;
> > +
> > +	val = nuvoton_qspi_read(qspi, NUVOTON_QSPI_SSCTL_OFFSET);
> > +
> > +	/* SPI core passes enable=true when CS is asserted (typically active-low) */
> > +	if (enable)
> > +		val |= mask;
> > +	else
> > +		val &= ~mask;
> > +
> > +	nuvoton_qspi_write(qspi, val, NUVOTON_QSPI_SSCTL_OFFSET);
> > +}
> Note that the core deals with SPI_CS_HIGH, the driver doesn't need to...
>
> > +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);
> > +	if (ret)
> > +		return ret;
> This uses spi->max_speed_hz but spi_mem configures p->max_freq which you
> should use (it might be lower).
>
> > +
> > +	nuvoton_qspi_set_cs(spi, true);
> ...except where you're calling in directly at which point the driver
> needs to figure this out.