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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] spi: ma35d1: arbitrary num-cs limit rejects valid GPIO chip selects
- [Medium] spi: ma35d1: missing PM suspend/resume hooks breaks hardware after deep sleep
--

commit 5e29b920c6ee893a0356b0c18698305417951afd
Author: Chi-Wen Weng <cwweng@nuvoton.com>

spi: ma35d1: Add Nuvoton MA35D1 SPI controller support

This commit adds support for the SPI controller found in the Nuvoton
MA35D1 SoC, handling SPI host mode using PIO transfers and configuring
clock dividers, chip selects, and basic transfer settings.

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

[ ... ]

> +static void nuvoton_spi_set_cs_level(struct nuvoton_spi *nspi,
> +				     unsigned int cs, bool assert)
> +{
> +	u32 mask;
> +
> +	switch (cs) {
> +	case 0:
> +		mask = NUVOTON_SPI_SSCTL_SS0_MASK;
> +		break;
> +	case 1:
> +		mask = NUVOTON_SPI_SSCTL_SS1_MASK;
> +		break;
> +	default:
> +		dev_warn(nspi->dev, "invalid chip select %u\n", cs);
> +		return;
> +	}
> +
> +	nuvoton_spi_update_ssctl_bits(nspi, mask, assert ? mask : 0);
> +}

[ ... ]

> +static int nuvoton_spi_hw_init(struct nuvoton_spi *nspi)
> +{

[ ... ]

> +	nuvoton_spi_update_bits(nspi, NUVOTON_SPI_PDMACTL_OFFSET,
> +				NUVOTON_SPI_PDMACTL_TXPDMAEN_MASK |
> +				NUVOTON_SPI_PDMACTL_RXPDMAEN_MASK, 0);
> +
> +	fifo_mask = NUVOTON_SPI_FIFOCTL_SLVBERX_MASK |
> +		    NUVOTON_SPI_FIFOCTL_TXUFIEN_MASK |
> +		    NUVOTON_SPI_FIFOCTL_TXUFPOL_MASK |
> +		    NUVOTON_SPI_FIFOCTL_RXOVIEN_MASK |
> +		    NUVOTON_SPI_FIFOCTL_RXTOIEN_MASK |
> +		    NUVOTON_SPI_FIFOCTL_TXTHIEN_MASK |
> +		    NUVOTON_SPI_FIFOCTL_RXTHIEN_MASK;
> +
> +	nuvoton_spi_update_bits(nspi, NUVOTON_SPI_FIFOCTL_OFFSET, fifo_mask, 0);

[Severity: Medium]
Will these critical non-transfer-specific register initializations (like 
PDMA and FIFO control configurations) be lost if the hardware loses state
during a deep sleep?

[ ... ]

> +static void nuvoton_spi_set_cs(struct spi_device *spi, bool level)
> +{
> +	struct nuvoton_spi *nspi = spi_controller_get_devdata(spi->controller);
> +
> +	/*
> +	 * The SPI core passes the physical CS level to ->set_cs(). This
> +	 * initial driver only supports active-low native chip selects.
> +	 */
> +	nuvoton_spi_set_cs_level(nspi, spi_get_chipselect(spi, 0), !level);
> +}

[Severity: Medium]
Will this unconditionally pass the CS index to nuvoton_spi_set_cs_level()
without checking if it is a GPIO chip select? 

The SPI core calls this function to ensure native CS lines are inactive
even when using GPIO chip selects. This could result in 
nuvoton_spi_set_cs_level() spamming the kernel log with "invalid chip 
select" warnings on every transfer targeting a GPIO CS with an index >= 2.

[ ... ]

> +static int nuvoton_spi_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = device_property_read_u32(dev, "num-cs", &num_cs);
> +	if (ret && ret != -EINVAL)
> +		return dev_err_probe(dev, ret, "failed to read num-cs\n");
> +
> +	if (!num_cs || num_cs > NUVOTON_SPI_MAX_NATIVE_CS)
> +		return dev_err_probe(dev, -EINVAL, "invalid num-cs %u\n",
> +				     num_cs);

[Severity: Medium]
Does this check improperly reject valid device tree configurations? A user 
might legitimately specify more than two total chip selects by utilizing 
GPIOs alongside the native ones.

[ ... ]

> +static struct platform_driver nuvoton_spi_driver = {
> +	.driver = {
> +		.name = "ma35d1-spi",
> +		.of_match_table = nuvoton_spi_of_match,
> +	},
> +	.probe = nuvoton_spi_probe,
> +};
> +module_platform_driver(nuvoton_spi_driver);

[Severity: Medium]
Does this platform driver lack a .pm assignment, causing it to completely 
miss system suspend and resume events? 

Without calling spi_controller_suspend() and spi_controller_resume(), the
SPI core might not be notified to pause its queues, which could lead to 
hangs, timeouts, or data corruption on subsequent transfers when resuming 
from sleep.

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