Skip to contentSkip to conversation

spi: ma35d1: Add Nuvoton MA35D1 SPI controller support

PATCH Superseded Chi-Wen Weng 4 participants 10 messages Sep 23 – Sep 24

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover spi: ma35d1: Add Nuvoton MA35D1 SPI controller support

From: Chi-Wen Weng <[email protected]>

This series adds Devicetree binding and driver support for the SPI controller found in the Nuvoton MA35D1 SoC.

The driver implements SPI host mode using polling-based PIO transfers. It supports SPI modes 0 through 3, LSB-first operation, and word sizes from 8 to 32 bits.

The controller provides two native chip-select outputs. GPIO chip selects are also supported through the SPI core GPIO descriptor infrastructure.

The driver programs the clock divider according to the hardware requirement that only odd CLKDIV register values are valid, and rounds the divisor so that the generated SPI clock does not exceed the requested transfer frequency.

DMA, target mode, half-duplex, 3-wire operation, FIFO threshold interrupt transfers, and the I2S function sharing the same hardware block are not included in this initial version.

Patch 1 adds the Devicetree binding.
Patch 2 adds the MA35D1 SPI controller driver.

Chi-Wen Weng (2):
dt-bindings: spi: nuvoton,ma35d1-spi: Add Nuvoton MA35D1 SPI
controller
spi: ma35d1: Add Nuvoton MA35D1 SPI controller support

 .../bindings/spi/nuvoton,ma35d1-spi.yaml      |  70 ++
 drivers/spi/Kconfig                           |  11 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-ma35d1.c                      | 701 ++++++++++++++++++
 4 files changed, 783 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/nuvoton,ma35d1-spi.yaml
 create mode 100644 drivers/spi/spi-ma35d1.c
Signature · 1 line
2.25.1
#1 lore raw

PATCH 1/2 dt-bindings: spi: nuvoton,ma35d1-spi: Add Nuvoton MA35D1 SPI controller

From: Chi-Wen Weng <[email protected]>

Add a Devicetree binding for the SPI controller found in the Nuvoton MA35D1 SoC.

The controller provides two native chip-select outputs and supports additional GPIO chip selects through the generic SPI controller binding.

Describe the controller register region, clock, reset, and optional interrupt resources.

---
 .../bindings/spi/nuvoton,ma35d1-spi.yaml      | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/nuvoton,ma35d1-spi.yaml
Documentation/devicetree/bindings/spi/nuvoton,ma35d1-spi.yaml new file +70 -0
new file mode 100644
index 000000000000..4e1afbdb195d
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/nuvoton,ma35d1-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton MA35D1 SPI Controller
+
+description: |
+  The Nuvoton MA35D1 SPI controller supports host and target operation and
+  provides two native chip-select outputs. GPIO chip selects are supported
+  through the generic SPI controller binding.
+
+maintainers:
+  - Chi-Wen Weng <[email protected]>
+
+allOf:
+  - $ref: /schemas/spi/spi-controller.yaml#
+
+properties:
+  compatible:
+    const: nuvoton,ma35d1-spi
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  num-cs:
+    minimum: 1
+    maximum: 2
+    default: 2
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - resets
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        spi@40600000 {
+            compatible = "nuvoton,ma35d1-spi";
+            reg = <0x0 0x40600000 0x0 0x10000>;
+            interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&clk SPI0_GATE>;
+            resets = <&sys MA35D1_RESET_SPI0>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+        };
+    };
+...
Signature · 1 line
2.25.1
maintainer
#2 lore raw

Re: [PATCH 1/2] dt-bindings: spi: nuvoton,ma35d1-spi: Add Nuvoton MA35D1 SPI controller

Quoted 4 lines · Chi-Wen Weng

From: Chi-Wen Weng <[email protected]>

Add a Devicetree binding for the SPI controller found in the Nuvoton MA35D1 SoC.

Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.

#3 lore raw

Re: [PATCH 1/2] dt-bindings: spi: nuvoton,ma35d1-spi: Add Nuvoton MA35D1 SPI controller

Mark Brown 於 2026/9/23 下午 07:59 寫道:

Quoted 11 lines · Chi-Wen Weng
Quoted 4 lines

From: Chi-Wen Weng <[email protected]>

Add a Devicetree binding for the SPI controller found in the Nuvoton MA35D1 SoC.

Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.

Hi Mark,

Thanks for the review.

I will update the binding patch subject in the next revision to follow the SPI subsystem convention, for example:

spi: dt-bindings: nuvoton,ma35d1-spi: Add Nuvoton MA35D1 SPI controller

I won't resend the series for this change alone.

Thanks,
Chi-Wen

#4 lore raw

Re: [PATCH 1/2] dt-bindings: spi: nuvoton,ma35d1-spi: Add Nuvoton MA35D1 SPI controller

In reply to Chi-Wen Weng
Quoted 4 lines · Chi-Wen Weng

From: Chi-Wen Weng <[email protected]>

Add a Devicetree binding for the SPI controller found in the Nuvoton MA35D1 SoC.

I could swear I saw it last days... I did check and indeed. There is qspi which is almost the same binding. Why this cannot be in the same file? What are the differences justifying duplicating files?

Best regards,
Krzysztof

#5 lore raw

Re: [PATCH 1/2] dt-bindings: spi: nuvoton,ma35d1-spi: Add Nuvoton MA35D1 SPI controller

Krzysztof Kozlowski 於 2026/9/23 下午 08:35 寫道:

Quoted 6 lines · Chi-Wen Weng
Quoted 4 lines

From: Chi-Wen Weng <[email protected]>

Add a Devicetree binding for the SPI controller found in the Nuvoton MA35D1 SoC.

Quoted 8 lines

I could swear I saw it last days... I did check and indeed. There is qspi which is almost the same binding. Why this cannot be in the same file? What are the differences justifying duplicating files?

Best regards,
Krzysztof

Hi Krzysztof,

Thanks for pointing this out.

The MA35D1 SPI and QSPI controllers are separate hardware IPs. QSPI additionally supports dual/quad/DTR transfers and a target-mode timeout function, while the SPI controller does not.

However, these differences do not require different Devicetree properties. The register resource, clock, reset, interrupt and native chip-select descriptions are otherwise the same, so I agree that they should share one binding.

There is currently one driver-related difference: the existing QSPI driver does not yet support GPIO chip selects, while the new SPI driver does. I will therefore keep the QSPI cs-gpios restriction in the shared schema for now using a compatible-specific conditional.

We also plan to add GPIO chip-select support to the QSPI driver in a follow-up patch. Once that support is added, the QSPI-specific cs-gpios restriction can be removed and both compatibles can use the same schema constraints.

I will merge the SPI and QSPI bindings in v2.

Thanks,
Chi-Wen

#6 lore raw

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

In reply to Chi-Wen Weng

From: Chi-Wen Weng <[email protected]>

Add support for the SPI controller found in the Nuvoton MA35D1 SoC.

The driver supports SPI host mode using PIO transfers, with SPI modes 0 through 3, LSB-first operation, and word sizes from 8 to 32 bits.

Support the two native chip-select outputs as well as GPIO chip selects through the SPI core GPIO descriptor support. Protect read-modify-write accesses to the slave-select control register with a dedicated spinlock.

Configure the clock divider according to the hardware requirement that only odd CLKDIV values are valid, ensuring that the generated SPI clock does not exceed the requested transfer frequency.

---
 drivers/spi/Kconfig      |  11 +
 drivers/spi/Makefile     |   1 +
 drivers/spi/spi-ma35d1.c | 701 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 713 insertions(+)
 create mode 100644 drivers/spi/spi-ma35d1.c
drivers/spi/Kconfig +11 -0
index a0f37e2df716..c446ddaa4897 100644
@@ -694,6 +694,17 @@ config SPI_LJCA
 	  This driver can also be built as a module. If so, the module
 	  will be called spi-ljca.
 
+config SPI_MA35D1
+	tristate "Nuvoton MA35D1 SPI controller"
+	depends on ARCH_MA35 || COMPILE_TEST
+	help
+	  This enables support for the SPI controller found in
+	  Nuvoton MA35D1 SoCs.
+
+	  The controller supports standard SPI peripheral devices with
+	  configurable clock polarity and phase, chip select, and data
+	  word sizes from 8 to 32 bits.
+
 config SPI_MA35D1_QSPI
 	tristate "Nuvoton MA35D1 QSPI controller"
 	depends on ARCH_MA35 || COMPILE_TEST
drivers/spi/Makefile +1 -0
index a18814fbde93..8fcdee4b6eec 100644
@@ -86,6 +86,7 @@ obj-$(CONFIG_SPI_LOONGSON_CORE)		+= spi-loongson-core.o
 obj-$(CONFIG_SPI_LOONGSON_PCI)		+= spi-loongson-pci.o
 obj-$(CONFIG_SPI_LOONGSON_PLATFORM)	+= spi-loongson-plat.o
 obj-$(CONFIG_SPI_LP8841_RTC)		+= spi-lp8841-rtc.o
+obj-$(CONFIG_SPI_MA35D1)		+= spi-ma35d1.o
 obj-$(CONFIG_SPI_MA35D1_QSPI)		+= spi-ma35d1-qspi.o
 obj-$(CONFIG_SPI_MESON_SPICC)		+= spi-meson-spicc.o
 obj-$(CONFIG_SPI_MESON_SPIFC)		+= spi-meson-spifc.o
drivers/spi/spi-ma35d1.c new file +701 -0

701 lines not on this page · raw message

Signature · 1 line
2.25.1
#7 lore raw
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…

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 <[email protected]>

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.

Quoted 5 lines
drivers/spi/spi-ma35d1.c new file +0 -0
new file mode 100644
index 0000000000000..14aebf5019203

[ ... ]

Quoted 19 lines
drivers/spi/spi-ma35d1.c new file +19 -0
+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);
+}

[ ... ]

Quoted 2 lines
drivers/spi/spi-ma35d1.c new file +2 -0
+static int nuvoton_spi_hw_init(struct nuvoton_spi *nspi)
+{

[ ... ]

drivers/spi/spi-ma35d1.c line 583

Quoted 13 lines
drivers/spi/spi-ma35d1.c new file +13 -0
+	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?

[ ... ]

drivers/spi/spi-ma35d1.c line 462

Quoted 10 lines
drivers/spi/spi-ma35d1.c new file +10 -0
+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.

[ ... ]

Quoted 2 lines
drivers/spi/spi-ma35d1.c new file +2 -0
+static int nuvoton_spi_probe(struct platform_device *pdev)
+{

[ ... ]

drivers/spi/spi-ma35d1.c line 648

Quoted 7 lines
drivers/spi/spi-ma35d1.c new file +7 -0
+	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.

[ ... ]

drivers/spi/spi-ma35d1.c line 697

Quoted 8 lines
drivers/spi/spi-ma35d1.c new file +8 -0
+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.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
maintainer
#8 lore raw
In reply to Chi-Wen Weng
Quoted 3 lines · Chi-Wen Weng

From: Chi-Wen Weng <[email protected]>

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.

drivers/spi/spi-ma35d1.c line 105

Quoted 7 lines
drivers/spi/spi-ma35d1.c new file +7 -0
+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.

drivers/spi/spi-ma35d1.c line 258

Quoted 13 lines
drivers/spi/spi-ma35d1.c new file +13 -0
+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.

drivers/spi/spi-ma35d1.c line 280

Quoted 6 lines
drivers/spi/spi-ma35d1.c new file +6 -0
+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.

Quoted 4 lines
drivers/spi/spi-ma35d1.c new file +4 -0
+static int nuvoton_spi_txrx(struct nuvoton_spi *nspi,
+			    struct spi_transfer *xfer, u8 bpw)
+{
+	unsigned int bytes_per_word;

drivers/spi/spi-ma35d1.c line 376

Quoted 10 lines
drivers/spi/spi-ma35d1.c new file +10 -0
+	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.

Quoted 7 lines
drivers/spi/spi-ma35d1.c new file +7 -0
+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;

drivers/spi/spi-ma35d1.c line 480

Quoted 10 lines
drivers/spi/spi-ma35d1.c new file +10 -0
+	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.

drivers/spi/spi-ma35d1.c line 490

Quoted 5 lines
drivers/spi/spi-ma35d1.c new file +5 -0
+	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.

drivers/spi/spi-ma35d1.c

Quoted 4 lines
drivers/spi/spi-ma35d1.c new file +3 -0
+static int nuvoton_spi_probe(struct platform_device *pdev)
+{
 
+	ctlr->dev.of_node = dev->of_node;

The core does this for you.

#9 lore raw

Mark Brown 於 2026/9/23 下午 08:25 寫道:

Quoted 95 lines · Chi-Wen Weng
Quoted 3 lines

From: Chi-Wen Weng <[email protected]>

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.

drivers/spi/spi-ma35d1.c line 105

Quoted 7 lines
drivers/spi/spi-ma35d1.c new file +7 -0
+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.

drivers/spi/spi-ma35d1.c line 258

Quoted 13 lines
drivers/spi/spi-ma35d1.c new file +13 -0
+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.

drivers/spi/spi-ma35d1.c line 280

Quoted 6 lines
drivers/spi/spi-ma35d1.c new file +6 -0
+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.

Quoted 4 lines
drivers/spi/spi-ma35d1.c new file +4 -0
+static int nuvoton_spi_txrx(struct nuvoton_spi *nspi,
+			    struct spi_transfer *xfer, u8 bpw)
+{
+	unsigned int bytes_per_word;

drivers/spi/spi-ma35d1.c line 376

Quoted 10 lines
drivers/spi/spi-ma35d1.c new file +10 -0
+	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.

Quoted 7 lines
drivers/spi/spi-ma35d1.c new file +7 -0
+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;

drivers/spi/spi-ma35d1.c line 480

Quoted 10 lines
drivers/spi/spi-ma35d1.c new file +10 -0
+	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.

drivers/spi/spi-ma35d1.c line 490

Quoted 5 lines
drivers/spi/spi-ma35d1.c new file +5 -0
+	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.

Quoted 6 lines

drivers/spi/spi-ma35d1.c

Quoted 4 lines
drivers/spi/spi-ma35d1.c new file +3 -0
+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