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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2963 bytes --]

On Wed, Sep 23, 2026 at 12:19:26PM +0800, Chi-Wen Weng wrote:
> From: Chi-Wen Weng <cwweng@nuvoton.com>
> 
> Add support for the SPI controller found in the Nuvoton MA35D1 SoC.

This looks pretty good, almost all of the comments below are just
stylistic things due to duplicating work that the core already does but
there's one query about possibly excessively turning the controller on
and off.

> +struct nuvoton_spi {
> +	void __iomem *regs;
> +	struct clk *clk;
> +	struct device *dev;
> +
> +	/* Protects read-modify-write accesses to the SSCTL register. */
> +	spinlock_t ssctl_lock;

It's not clear what this is protecting, all the users look to be called
from the SPI core in a single threaded way.

> +static int nuvoton_spi_set_speed(struct nuvoton_spi *nspi,
> +				 struct spi_transfer *xfer, u32 speed_hz)
> +{
> +	unsigned long clk_rate;
> +	unsigned int divisor;
> +	u32 clkdiv;
> +	int ret;
> +
> +	clk_rate = clk_get_rate(nspi->clk);
> +	if (!clk_rate) {
> +		dev_err(nspi->dev, "failed to get clock rate\n");
> +		return -EINVAL;
> +	}

You probably don't need to do this on every transfer, the driver doesn't
change the rate and it's not the cheapest call.

> +static int nuvoton_spi_configure_transfer(struct nuvoton_spi *nspi,
> +					  struct spi_device *spi,
> +					  struct spi_transfer *xfer,
> +					  u8 bpw)
> +{
> +	u32 speed_hz = xfer->speed_hz ?: spi->max_speed_hz;

The core will ensure the transfer has a speed set.

> +static int nuvoton_spi_txrx(struct nuvoton_spi *nspi,
> +			    struct spi_transfer *xfer, u8 bpw)
> +{
> +	unsigned int bytes_per_word;
> +	unsigned int offset;
> +	u32 val;
> +	int ret;
> +
> +	bytes_per_word = spi_bpw_to_bytes(bpw);
> +	if (!bytes_per_word)
> +		return -EINVAL;
> +
> +	if (xfer->len % bytes_per_word)
> +		return -EINVAL;

The core ensures these too.

> +static int nuvoton_spi_transfer_one(struct spi_controller *ctlr,
> +				    struct spi_device *spi,
> +				    struct spi_transfer *xfer)
> +{
> +	struct nuvoton_spi *nspi = spi_controller_get_devdata(ctlr);
> +	u8 bpw = xfer->bits_per_word ?: spi->bits_per_word;
> +	int disable_ret;
> +	int ret;
> +
> +	if (!xfer->len)
> +		return 0;
> +
> +	if (!bpw)
> +		bpw = NUVOTON_SPI_DEFAULT_BPW;
> +
> +	if (bpw < 8 || bpw > 32)
> +		return -EINVAL;

Again the core is checking this stuff.

> +	ret = nuvoton_spi_enable(nspi);
> +	if (ret) {
> +		dev_err(nspi->dev, "failed to enable controller\n");
> +		goto out_disable;
> +	}

Do you need to enable and disable on every transfer, or could this be
done once per message?  If you need to disable to reconfigure it's a bit
more complicated, but otherwise it's overhead to bounce the controller
on and off.  Potentially it might glitch the data lines.

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

> +	ctlr->dev.of_node = dev->of_node;

The core does this for you.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]