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

[Chi-Wen Weng <cwweng.linux@gmail.com>]

Mark Brown 於 2026/9/23 下午 08:25 寫道:
> 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.

Hi Mark,

Thanks for the detailed review.

Regarding SPIEN, there are two hardware requirements/behaviors worth
clarifying for the MA35D1 SPI controller:

1. SPIEN must be cleared, and SPIENSTS must become 0, before modifying
    CTL, CLKDIV, SSCTL, or FIFOCTL.

2. Clearing SPIEN does not change or tristate the SPI CLK or MOSI output
    levels. Their output levels are retained while SPIEN is cleared, so
    disabling the controller does not introduce a signal glitch on those
    lines.

Therefore, disabling and re-enabling the controller is required when
the transfer configuration needs to be changed. However, I agree that
the current driver does this unconditionally for every transfer, which
is more than necessary.

I will rework this in v2 to avoid unnecessary SPIEN toggling and only
disable the controller when required for reconfiguration, while still
honoring the register programming requirements above.

I will also address the other comments by relying on the SPI core for
the transfer defaults and validation it already provides, caching the
input clock rate instead of calling clk_get_rate() for every transfer,
removing the unnecessary SSCTL locking, and dropping the explicit
of_node assignment.

Thanks,
Chi-Wen