[PATCH v4 0/3] Virtio SPI Linux driver

[PATCH v4 0/3] Virtio SPI Linux driver

[Haixu Cui]

This is the 4th non-RFC version of a virtio SPI Linux driver which is
intended to be compliant with the upcoming virtio specification
version 1.4. The specification can be found in repository:
https://github.com/oasis-tcs/virtio-spec.git branch virtio-1.4.

This version builds upon the previous non-RFC V3 and RFC V8 versions,
and incorporates feedback from the community and testing improvements.

Changes between RFC V8 and non-RFC V4:
- Replaced BUILD_BUG_ON with static_assert for SPI mode constants to
  improve compile-time validation.
- Introduced VIRTIO_SPI_MODE_MASK macro to simplify SPI mode bit masking.
- Removed unnecessary __cacheline_aligned attributes from buffer
  pointers in virtio_spi_req.
- Simplified type casting in delay conversions by removing redundant
  (u32) casts.
- Replaced manual virtqueue cleanup with devm_add_action_or_reset()
  for automatic resource management.
- Migrated controller registration to devm_spi_register_controller()
  for better lifecycle handling.
- Updated power management callbacks to use struct device * and
  added virtio_spi_pm_ops
- Standardized bit macros in virtio_spi.h from BIT(x) to _BITUL(x) for
  type safety.
- Improved documentation comments in virtio_spi_config for clarity
  and consistency.

Changes between RFC V7 and RFC V8:
- Restored original Copyright.

Changes between RFC V6 and RFC V7:
- Restored original MODULE_AUTHOR information in the driver source file.
- Reinstated the original Signed-off-by tags.

Changes between RFC V5 and RFC V6:
- Dynamically allocate the virtio_spi_req structure instead of keeping
  it as a member of virtio_spi_priv.
- Remove redundant comments.

Changes between RFC V4 and RFC V5:
- Use dev_err_probe instead of dev_err in virtio_spi_probe function
  to improve error handling.
- Add comments to virtio_spi_set_delays function and revise several
  field descriptions in mode_func_supported for improved clarity.
- Update bitmask definitions from (1 << n) to BIT(n) to enhance code
  readability.

Changes between non-RFC V3 and RFC V4
- Remove the logic code for statically creating SPI devices through
  the spi_new_device function.
- Add ACPI support.
- According to Hillf Danton's comment, use init_completion instead of
  reinit_completion in virtio_spi_transfer_one function.

Changes between non-RFC V2 and non-RFC V3
- Child spi device tree nodes are supported now.
  If a child spi device tree node exists the setup of the user mode SPI
  device is done by spi_register_controller() and the driver itself does
  not call spi_new_device() any more to setup the chip selects.
  If there is no device tree child node the SPI device sets up the user
  mode SPI devices autonomously as it was before.
  
Changes between non-RFC V1 and non-RFC V2
- Remove some comments stating the obvious
- Remove error trace when devm_spi_alloc_host() failed as this is habit
- Add some blank lines to improve readabilty
- Last TODO comment removed which was used to trigger some discussion.
  Discussion did not take place, most probably the code below is correct
  as it is
- Abstained from replacing "Cannot " by "Failed to " in error messages
  as the wording "Cannot " is frequently used even when "Failed to " has
  the majority. Announced this, heard nothing about this, so added the
  "Reviewed-by" from Viresh Kumar <viresh.kumar@linaro.org> as
  everything else was done.

Changes between RFC V3 and non-RFC V1:
- Address kernel test robot comment which revealed an actual bug
- Rework some comments in the code addressing review comments
- Remove a TODO comment which has served it's purpose
- Allocate struct virtio_spi_req spi_req only once at startup
- Use callback transfer_one instead of transfer_one_message to simplify
  and shorten code. Due to this rework in the affected function(s) some
  additional changes:
  - Do init_completion() only once at startup, for re-initialization
    now reinit_completion() is used
  - Translate result codes VIRTIO_SPI_PARAM_ERR and VIRTIO_SPI_TRANS_ERR
    to appropriate Linux error codes -EINVAL and -EIO

Changes between RFC V2 and RFC V3:
- Order header inclusion alphabetically.
- Add Viresh Kumar's "signed-off" to the header files.
- Rework virtio_spi_one_transfer
  - Rework the delays according to Haixu Cui's advise. Delays are now
    handled in a new sub-function virtio_spi_set_delays.
  - Minor change: Re-formulate arguments of sg_init_one.
- Rework virtio_spi_probe
  - Replace some goto in error paths by return.
  - Add spi_unregister_controller to an error path. Abstained from
    using devm_spi_register_controller to keep order of
    de-initialization in virtio_spi_remove.
  - Add deletion of vqueue to all error paths taken after the virtqueues
    have been initialized.

Changes between RFC V1 and RFC V2:
- Update from virtio SPI draft specification V4 to V10.
- Incorporate review comments gotten from the community.
- A proposal for a performance enhancement having more than only one SPI
  message in flight had to be kept out. The more complicated code would
  have caused an unacceptable project risk now.

The driver was smoke tested on QEMU using:
- Qualcomm's target hardware with a physical SPI backend device via
  vhost-user protocol (Linux v6.12).
- OpenSynergy's proprietary virtio SPI device simulating a SPI backend
  on top of linux-next.git (v6.8).
- Regression tested on physical hardware using kernel v6.5.7.

[PATCH v4 1/3] virtio: Add ID for virtio SPI

[Haixu Cui]

Add VIRTIO_ID_SPI definition for virtio SPI.

Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/uapi/linux/virtio_ids.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index 7aa2eb766205..6c12db16faa3 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -68,6 +68,7 @@
 #define VIRTIO_ID_AUDIO_POLICY		39 /* virtio audio policy */
 #define VIRTIO_ID_BT			40 /* virtio bluetooth */
 #define VIRTIO_ID_GPIO			41 /* virtio gpio */
+#define VIRTIO_ID_SPI			45 /* virtio spi */
 
 /*
  * Virtio Transitional IDs
-- 
2.34.1

[PATCH v4 2/3] virtio-spi: Add virtio-spi.h

[Haixu Cui]

Add virtio-spi.h header for virtio SPI.

Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
---
 MAINTAINERS                     |   5 +
 include/uapi/linux/virtio_spi.h | 185 ++++++++++++++++++++++++++++++++
 2 files changed, 190 insertions(+)
 create mode 100644 include/uapi/linux/virtio_spi.h

diff --git a/MAINTAINERS b/MAINTAINERS
index daf520a13bdf..3e289677ca18 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26760,6 +26760,11 @@ S:	Maintained
 F:	include/uapi/linux/virtio_snd.h
 F:	sound/virtio/*
 
+VIRTIO SPI DRIVER
+M:	Haixu Cui <quic_haixcui@quicinc.com>
+S:	Maintained
+F:	include/uapi/linux/virtio_spi.h
+
 VIRTUAL BOX GUEST DEVICE DRIVER
 M:	Hans de Goede <hansg@kernel.org>
 M:	Arnd Bergmann <arnd@arndb.de>
diff --git a/include/uapi/linux/virtio_spi.h b/include/uapi/linux/virtio_spi.h
new file mode 100644
index 000000000000..b55877b3e525
--- /dev/null
+++ b/include/uapi/linux/virtio_spi.h
@@ -0,0 +1,185 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Copyright (C) 2023 OpenSynergy GmbH
+ * Copyright (C) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+#ifndef _LINUX_VIRTIO_VIRTIO_SPI_H
+#define _LINUX_VIRTIO_VIRTIO_SPI_H
+
+#include <linux/types.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_types.h>
+
+/* Sample data on trailing clock edge */
+#define VIRTIO_SPI_CPHA			_BITUL(0)
+/* Clock is high when IDLE */
+#define VIRTIO_SPI_CPOL			_BITUL(1)
+/* Chip Select is active high */
+#define VIRTIO_SPI_CS_HIGH			_BITUL(2)
+/* Transmit LSB first */
+#define VIRTIO_SPI_MODE_LSB_FIRST		_BITUL(3)
+/* Loopback mode */
+#define VIRTIO_SPI_MODE_LOOP			_BITUL(4)
+
+/**
+ * struct virtio_spi_config - All config fields are read-only for the
+ * Virtio SPI driver
+ * @cs_max_number: maximum number of chipselect the host SPI controller
+ *   supports.
+ * @cs_change_supported: indicates if the host SPI controller supports to toggle
+ *   chipselect after each transfer in one message:
+ *   0: unsupported, chipselect will be kept in active state throughout the
+ *      message transaction;
+ *   1: supported.
+ *   Note: Message here contains a sequence of SPI transfers.
+ * @tx_nbits_supported: indicates the supported number of bit for writing:
+ *   bit 0: DUAL (2-bit transfer), 1 for supported
+ *   bit 1: QUAD (4-bit transfer), 1 for supported
+ *   bit 2: OCTAL (8-bit transfer), 1 for supported
+ *   other bits are reserved as 0, 1-bit transfer is always supported.
+ * @rx_nbits_supported: indicates the supported number of bit for reading:
+ *   bit 0: DUAL (2-bit transfer), 1 for supported
+ *   bit 1: QUAD (4-bit transfer), 1 for supported
+ *   bit 2: OCTAL (8-bit transfer), 1 for supported
+ *   other bits are reserved as 0, 1-bit transfer is always supported.
+ * @bits_per_word_mask: mask indicating which values of bits_per_word are
+ *   supported. If not set, no limitation for bits_per_word.
+ * @mode_func_supported: indicates the following features are supported or not:
+ *   bit 0-1: CPHA feature
+ *     0b00: invalid, should support as least one CPHA setting
+ *     0b01: supports CPHA=0 only
+ *     0b10: supports CPHA=1 only
+ *     0b11: supports CPHA=0 and CPHA=1.
+ *   bit 2-3: CPOL feature
+ *     0b00: invalid, should support as least one CPOL setting
+ *     0b01: supports CPOL=0 only
+ *     0b10: supports CPOL=1 only
+ *     0b11: supports CPOL=0 and CPOL=1.
+ *   bit 4: chipselect active high feature, 0 for unsupported and 1 for
+ *     supported, chipselect active low is supported by default.
+ *   bit 5: LSB first feature, 0 for unsupported and 1 for supported,
+ *     MSB first is supported by default.
+ *   bit 6: loopback mode feature, 0 for unsupported and 1 for supported,
+ *     normal mode is supported by default.
+ * @max_freq_hz: the maximum clock rate supported in Hz unit, 0 means no
+ *   limitation for transfer speed.
+ * @max_word_delay_ns: the maximum word delay supported, in nanoseconds.
+ *   A value of 0 indicates that word delay is unsupported.
+ *   Each transfer may consist of a sequence of words.
+ * @max_cs_setup_ns: the maximum delay supported after chipselect is asserted,
+ *   in ns unit, 0 means delay is not supported to introduce after chipselect is
+ *   asserted.
+ * @max_cs_hold_ns: the maximum delay supported before chipselect is deasserted,
+ *   in ns unit, 0 means delay is not supported to introduce before chipselect
+ *   is deasserted.
+ * @max_cs_incative_ns: maximum delay supported after chipselect is deasserted,
+ *   in ns unit, 0 means delay is not supported to introduce after chipselect is
+ *   deasserted.
+ */
+struct virtio_spi_config {
+	/* # of /dev/spidev<bus_num>.CS with CS=0..chip_select_max_number -1 */
+	__u8 cs_max_number;
+	__u8 cs_change_supported;
+#define VIRTIO_SPI_RX_TX_SUPPORT_DUAL		_BITUL(0)
+#define VIRTIO_SPI_RX_TX_SUPPORT_QUAD		_BITUL(1)
+#define VIRTIO_SPI_RX_TX_SUPPORT_OCTAL		_BITUL(2)
+	__u8 tx_nbits_supported;
+	__u8 rx_nbits_supported;
+	__le32 bits_per_word_mask;
+#define VIRTIO_SPI_MF_SUPPORT_CPHA_0		_BITUL(0)
+#define VIRTIO_SPI_MF_SUPPORT_CPHA_1		_BITUL(1)
+#define VIRTIO_SPI_MF_SUPPORT_CPOL_0		_BITUL(2)
+#define VIRTIO_SPI_MF_SUPPORT_CPOL_1		_BITUL(3)
+#define VIRTIO_SPI_MF_SUPPORT_CS_HIGH		_BITUL(4)
+#define VIRTIO_SPI_MF_SUPPORT_LSB_FIRST		_BITUL(5)
+#define VIRTIO_SPI_MF_SUPPORT_LOOPBACK		_BITUL(6)
+	__le32 mode_func_supported;
+	__le32 max_freq_hz;
+	__le32 max_word_delay_ns;
+	__le32 max_cs_setup_ns;
+	__le32 max_cs_hold_ns;
+	__le32 max_cs_inactive_ns;
+};
+
+/*
+ * @chip_select_id: chipselect index the SPI transfer used.
+ *
+ * @bits_per_word: the number of bits in each SPI transfer word.
+ *
+ * @cs_change: whether to deselect device after finishing this transfer
+ *     before starting the next transfer, 0 means cs keep asserted and
+ *     1 means cs deasserted then asserted again.
+ *
+ * @tx_nbits: bus width for write transfer.
+ *     0,1: bus width is 1, also known as SINGLE
+ *     2  : bus width is 2, also known as DUAL
+ *     4  : bus width is 4, also known as QUAD
+ *     8  : bus width is 8, also known as OCTAL
+ *     other values are invalid.
+ *
+ * @rx_nbits: bus width for read transfer.
+ *     0,1: bus width is 1, also known as SINGLE
+ *     2  : bus width is 2, also known as DUAL
+ *     4  : bus width is 4, also known as QUAD
+ *     8  : bus width is 8, also known as OCTAL
+ *     other values are invalid.
+ *
+ * @reserved: for future use.
+ *
+ * @mode: SPI transfer mode.
+ *     bit 0: CPHA, determines the timing (i.e. phase) of the data
+ *         bits relative to the clock pulses.For CPHA=0, the
+ *         "out" side changes the data on the trailing edge of the
+ *         preceding clock cycle, while the "in" side captures the data
+ *         on (or shortly after) the leading edge of the clock cycle.
+ *         For CPHA=1, the "out" side changes the data on the leading
+ *         edge of the current clock cycle, while the "in" side
+ *         captures the data on (or shortly after) the trailing edge of
+ *         the clock cycle.
+ *     bit 1: CPOL, determines the polarity of the clock. CPOL=0 is a
+ *         clock which idles at 0, and each cycle consists of a pulse
+ *         of 1. CPOL=1 is a clock which idles at 1, and each cycle
+ *         consists of a pulse of 0.
+ *     bit 2: CS_HIGH, if 1, chip select active high, else active low.
+ *     bit 3: LSB_FIRST, determines per-word bits-on-wire, if 0, MSB
+ *         first, else LSB first.
+ *     bit 4: LOOP, loopback mode.
+ *
+ * @freq: the transfer speed in Hz.
+ *
+ * @word_delay_ns: delay to be inserted between consecutive words of a
+ *     transfer, in ns unit.
+ *
+ * @cs_setup_ns: delay to be introduced after CS is asserted, in ns
+ *     unit.
+ *
+ * @cs_delay_hold_ns: delay to be introduced before CS is deasserted
+ *     for each transfer, in ns unit.
+ *
+ * @cs_change_delay_inactive_ns: delay to be introduced after CS is
+ *     deasserted and before next asserted, in ns unit.
+ */
+struct spi_transfer_head {
+	__u8 chip_select_id;
+	__u8 bits_per_word;
+	__u8 cs_change;
+	__u8 tx_nbits;
+	__u8 rx_nbits;
+	__u8 reserved[3];
+	__le32 mode;
+	__le32 freq;
+	__le32 word_delay_ns;
+	__le32 cs_setup_ns;
+	__le32 cs_delay_hold_ns;
+	__le32 cs_change_delay_inactive_ns;
+};
+
+struct spi_transfer_result {
+#define VIRTIO_SPI_TRANS_OK	0
+#define VIRTIO_SPI_PARAM_ERR	1
+#define VIRTIO_SPI_TRANS_ERR	2
+	__u8 result;
+};
+
+#endif /* #ifndef _LINUX_VIRTIO_VIRTIO_SPI_H */
-- 
2.34.1

[PATCH v4 3/3] SPI: Add virtio SPI driver

[Haixu Cui]

This is the virtio SPI Linux kernel driver.

Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
---
 MAINTAINERS              |   1 +
 drivers/spi/Kconfig      |  11 +
 drivers/spi/Makefile     |   1 +
 drivers/spi/spi-virtio.c | 448 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 461 insertions(+)
 create mode 100644 drivers/spi/spi-virtio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3e289677ca18..ba276c71ec44 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26763,6 +26763,7 @@ F:	sound/virtio/*
 VIRTIO SPI DRIVER
 M:	Haixu Cui <quic_haixcui@quicinc.com>
 S:	Maintained
+F:	drivers/spi/spi-virtio.c
 F:	include/uapi/linux/virtio_spi.h
 
 VIRTUAL BOX GUEST DEVICE DRIVER
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 891729c9c564..7b609013fb05 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -1224,6 +1224,17 @@ config SPI_UNIPHIER
 
 	  If your SoC supports SCSSI, say Y here.
 
+config SPI_VIRTIO
+	tristate "Virtio SPI Controller"
+	depends on SPI_MASTER && VIRTIO
+	help
+	  If you say yes to this option, support will be included for the virtio
+	  SPI controller driver. The hardware can be emulated by any device model
+	  software according to the virtio protocol.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called spi-virtio.
+
 config SPI_XCOMM
 	tristate "Analog Devices AD-FMCOMMS1-EBZ SPI-I2C-bridge driver"
 	depends on I2C
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 062c85989c8c..27a7cf68d55d 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -158,6 +158,7 @@ spi-thunderx-objs			:= spi-cavium.o spi-cavium-thunderx.o
 obj-$(CONFIG_SPI_THUNDERX)		+= spi-thunderx.o
 obj-$(CONFIG_SPI_TOPCLIFF_PCH)		+= spi-topcliff-pch.o
 obj-$(CONFIG_SPI_UNIPHIER)		+= spi-uniphier.o
+obj-$(CONFIG_SPI_VIRTIO)		+= spi-virtio.o
 obj-$(CONFIG_SPI_XCOMM)		+= spi-xcomm.o
 obj-$(CONFIG_SPI_XILINX)		+= spi-xilinx.o
 obj-$(CONFIG_SPI_XLP)			+= spi-xlp.o
diff --git a/drivers/spi/spi-virtio.c b/drivers/spi/spi-virtio.c
new file mode 100644
index 000000000000..e413c9cc73de
--- /dev/null
+++ b/drivers/spi/spi-virtio.c
@@ -0,0 +1,448 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SPI bus driver for the Virtio SPI controller
+ * Copyright (C) 2023 OpenSynergy GmbH
+ * Copyright (C) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/stddef.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ring.h>
+#include <linux/virtio_spi.h>
+
+#define VIRTIO_SPI_MODE_MASK \
+	(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)
+
+struct virtio_spi_req {
+	struct completion completion;
+	struct spi_transfer_head transfer_head	____cacheline_aligned;
+	const u8 *tx_buf;
+	u8 *rx_buf;
+	struct spi_transfer_result result	____cacheline_aligned;
+};
+
+struct virtio_spi_priv {
+	/* The virtio device we're associated with */
+	struct virtio_device *vdev;
+	/* Pointer to the virtqueue */
+	struct virtqueue *vq;
+	/* Copy of config space mode_func_supported */
+	u32 mode_func_supported;
+	/* Copy of config space max_freq_hz */
+	u32 max_freq_hz;
+};
+
+static void virtio_spi_msg_done(struct virtqueue *vq)
+{
+	struct virtio_spi_req *req;
+	unsigned int len;
+
+	while ((req = virtqueue_get_buf(vq, &len)))
+		complete(&req->completion);
+}
+
+/*
+ * virtio_spi_set_delays - Set delay parameters for SPI transfer
+ *
+ * This function sets various delay parameters for SPI transfer,
+ * including delay after CS asserted, timing intervals between
+ * adjacent words within a transfer, delay before and after CS
+ * deasserted. It converts these delay parameters to nanoseconds
+ * using spi_delay_to_ns and stores the results in spi_transfer_head
+ * structure.
+ * If the conversion fails, the function logs a warning message and
+ * returns an error code.
+ *       .   .      .    .    .   .   .   .   .   .
+ * Delay + A +      + B  +    + C + D + E + F + A +
+ *       .   .      .    .    .   .   .   .   .   .
+ *    ___.   .      .    .    .   .   .___.___.   .
+ * CS#   |___.______.____.____.___.___|   .   |___._____________
+ *       .   .      .    .    .   .   .   .   .   .
+ *       .   .      .    .    .   .   .   .   .   .
+ * SCLK__.___.___NNN_____NNN__.___.___.___.___.___.___NNN_______
+ *
+ * NOTE: 1st transfer has two words, the delay between these two words are
+ * 'B' in the diagram.
+ *
+ * A => struct spi_device -> cs_setup
+ * B => max{struct spi_transfer -> word_delay, struct spi_device -> word_delay}
+ *   Note: spi_device and spi_transfer both have word_delay, Linux
+ *         choose the bigger one, refer to _spi_xfer_word_delay_update function
+ * C => struct spi_transfer -> delay
+ * D => struct spi_device -> cs_hold
+ * E => struct spi_device -> cs_inactive
+ * F => struct spi_transfer -> cs_change_delay
+ *
+ * So the corresponding relationship:
+ * A   <===> cs_setup_ns (after CS asserted)
+ * B   <===> word_delay_ns (delay between adjacent words within a transfer)
+ * C+D <===> cs_delay_hold_ns (before CS deasserted)
+ * E+F <===> cs_change_delay_inactive_ns (after CS deasserted, these two
+ * values are also recommended in the Linux driver to be added up)
+ */
+static int virtio_spi_set_delays(struct spi_transfer_head *th,
+				 struct spi_device *spi,
+				 struct spi_transfer *xfer)
+{
+	int cs_setup;
+	int cs_word_delay_xfer;
+	int cs_word_delay_spi;
+	int delay;
+	int cs_hold;
+	int cs_inactive;
+	int cs_change_delay;
+
+	cs_setup = spi_delay_to_ns(&spi->cs_setup, xfer);
+	if (cs_setup < 0) {
+		dev_warn(&spi->dev, "Cannot convert cs_setup\n");
+		return cs_setup;
+	}
+	th->cs_setup_ns = cpu_to_le32(cs_setup);
+
+	cs_word_delay_xfer = spi_delay_to_ns(&xfer->word_delay, xfer);
+	if (cs_word_delay_xfer < 0) {
+		dev_warn(&spi->dev, "Cannot convert cs_word_delay_xfer\n");
+		return cs_word_delay_xfer;
+	}
+	cs_word_delay_spi = spi_delay_to_ns(&spi->word_delay, xfer);
+	if (cs_word_delay_spi < 0) {
+		dev_warn(&spi->dev, "Cannot convert cs_word_delay_spi\n");
+		return cs_word_delay_spi;
+	}
+	if (cs_word_delay_spi > cs_word_delay_xfer)
+		th->word_delay_ns = cpu_to_le32(cs_word_delay_spi);
+	else
+		th->word_delay_ns = cpu_to_le32(cs_word_delay_xfer);
+
+	delay = spi_delay_to_ns(&xfer->delay, xfer);
+	if (delay < 0) {
+		dev_warn(&spi->dev, "Cannot convert delay\n");
+		return delay;
+	}
+	cs_hold = spi_delay_to_ns(&spi->cs_hold, xfer);
+	if (cs_hold < 0) {
+		dev_warn(&spi->dev, "Cannot convert cs_hold\n");
+		return cs_hold;
+	}
+	th->cs_delay_hold_ns = cpu_to_le32(delay + cs_hold);
+
+	cs_inactive = spi_delay_to_ns(&spi->cs_inactive, xfer);
+	if (cs_inactive < 0) {
+		dev_warn(&spi->dev, "Cannot convert cs_inactive\n");
+		return cs_inactive;
+	}
+	cs_change_delay = spi_delay_to_ns(&xfer->cs_change_delay, xfer);
+	if (cs_change_delay < 0) {
+		dev_warn(&spi->dev, "Cannot convert cs_change_delay\n");
+		return cs_change_delay;
+	}
+	th->cs_change_delay_inactive_ns =
+		cpu_to_le32(cs_inactive + cs_change_delay);
+
+	return 0;
+}
+
+static int virtio_spi_transfer_one(struct spi_controller *ctrl,
+				   struct spi_device *spi,
+				   struct spi_transfer *xfer)
+{
+	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
+	struct virtio_spi_req *spi_req;
+	struct spi_transfer_head *th;
+	struct scatterlist sg_out_head, sg_out_payload;
+	struct scatterlist sg_in_result, sg_in_payload;
+	struct scatterlist *sgs[4];
+	unsigned int outcnt = 0u;
+	unsigned int incnt = 0u;
+	int ret;
+
+	spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
+	if (!spi_req)
+		return -ENOMEM;
+
+	init_completion(&spi_req->completion);
+
+	th = &spi_req->transfer_head;
+
+	/* Fill struct spi_transfer_head */
+	th->chip_select_id = spi_get_chipselect(spi, 0);
+	th->bits_per_word = spi->bits_per_word;
+	th->cs_change = xfer->cs_change;
+	th->tx_nbits = xfer->tx_nbits;
+	th->rx_nbits = xfer->rx_nbits;
+	th->reserved[0] = 0;
+	th->reserved[1] = 0;
+	th->reserved[2] = 0;
+
+	static_assert(VIRTIO_SPI_CPHA == SPI_CPHA,
+		      "VIRTIO_SPI_CPHA must match SPI_CPHA");
+	static_assert(VIRTIO_SPI_CPOL == SPI_CPOL,
+		      "VIRTIO_SPI_CPOL must match SPI_CPOL");
+	static_assert(VIRTIO_SPI_CS_HIGH == SPI_CS_HIGH,
+		      "VIRTIO_SPI_CS_HIGH must match SPI_CS_HIGH");
+	static_assert(VIRTIO_SPI_MODE_LSB_FIRST == SPI_LSB_FIRST,
+		      "VIRTIO_SPI_MODE_LSB_FIRST must match SPI_LSB_FIRST");
+
+	th->mode = cpu_to_le32(spi->mode & VIRTIO_SPI_MODE_MASK);
+	if (spi->mode & SPI_LOOP)
+		th->mode |= cpu_to_le32(VIRTIO_SPI_MODE_LOOP);
+
+	th->freq = cpu_to_le32(xfer->speed_hz);
+
+	ret = virtio_spi_set_delays(th, spi, xfer);
+	if (ret)
+		goto msg_done;
+
+	/* Set buffers */
+	spi_req->tx_buf = xfer->tx_buf;
+	spi_req->rx_buf = xfer->rx_buf;
+
+	/* Prepare sending of virtio message */
+	init_completion(&spi_req->completion);
+
+	sg_init_one(&sg_out_head, th, sizeof(*th));
+	sgs[outcnt] = &sg_out_head;
+	outcnt++;
+
+	if (spi_req->tx_buf) {
+		sg_init_one(&sg_out_payload, spi_req->tx_buf, xfer->len);
+		sgs[outcnt] = &sg_out_payload;
+		outcnt++;
+	}
+
+	if (spi_req->rx_buf) {
+		sg_init_one(&sg_in_payload, spi_req->rx_buf, xfer->len);
+		sgs[outcnt] = &sg_in_payload;
+		incnt++;
+	}
+
+	sg_init_one(&sg_in_result, &spi_req->result,
+		    sizeof(struct spi_transfer_result));
+	sgs[outcnt + incnt] = &sg_in_result;
+	incnt++;
+
+	ret = virtqueue_add_sgs(priv->vq, sgs, outcnt, incnt, spi_req,
+				GFP_KERNEL);
+	if (ret)
+		goto msg_done;
+
+	/* Simple implementation: There can be only one transfer in flight */
+	virtqueue_kick(priv->vq);
+
+	wait_for_completion(&spi_req->completion);
+
+	/* Read result from message and translate return code */
+	switch (spi_req->result.result) {
+	case VIRTIO_SPI_TRANS_OK:
+		break;
+	case VIRTIO_SPI_PARAM_ERR:
+		ret = -EINVAL;
+		break;
+	case VIRTIO_SPI_TRANS_ERR:
+		ret = -EIO;
+		break;
+	default:
+		ret = -EIO;
+		break;
+	}
+
+msg_done:
+	kfree(spi_req);
+	if (ret)
+		ctrl->cur_msg->status = ret;
+
+	return ret;
+}
+
+static void virtio_spi_read_config(struct virtio_device *vdev)
+{
+	struct spi_controller *ctrl = dev_get_drvdata(&vdev->dev);
+	struct virtio_spi_priv *priv = vdev->priv;
+	u8 cs_max_number;
+	u8 tx_nbits_supported;
+	u8 rx_nbits_supported;
+
+	cs_max_number = virtio_cread8(vdev, offsetof(struct virtio_spi_config,
+						     cs_max_number));
+	ctrl->num_chipselect = cs_max_number;
+
+	/* Set the mode bits which are understood by this driver */
+	priv->mode_func_supported =
+		virtio_cread32(vdev, offsetof(struct virtio_spi_config,
+					      mode_func_supported));
+	ctrl->mode_bits = priv->mode_func_supported &
+			  (VIRTIO_SPI_CS_HIGH | VIRTIO_SPI_MODE_LSB_FIRST);
+	if (priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_CPHA_1)
+		ctrl->mode_bits |= VIRTIO_SPI_CPHA;
+	if (priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_CPOL_1)
+		ctrl->mode_bits |= VIRTIO_SPI_CPOL;
+	if (priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LSB_FIRST)
+		ctrl->mode_bits |= SPI_LSB_FIRST;
+	if (priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LOOPBACK)
+		ctrl->mode_bits |= SPI_LOOP;
+	tx_nbits_supported =
+		virtio_cread8(vdev, offsetof(struct virtio_spi_config,
+					     tx_nbits_supported));
+	if (tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_DUAL)
+		ctrl->mode_bits |= SPI_TX_DUAL;
+	if (tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD)
+		ctrl->mode_bits |= SPI_TX_QUAD;
+	if (tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL)
+		ctrl->mode_bits |= SPI_TX_OCTAL;
+	rx_nbits_supported =
+		virtio_cread8(vdev, offsetof(struct virtio_spi_config,
+					     rx_nbits_supported));
+	if (rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_DUAL)
+		ctrl->mode_bits |= SPI_RX_DUAL;
+	if (rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD)
+		ctrl->mode_bits |= SPI_RX_QUAD;
+	if (rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL)
+		ctrl->mode_bits |= SPI_RX_OCTAL;
+
+	ctrl->bits_per_word_mask =
+		virtio_cread32(vdev, offsetof(struct virtio_spi_config,
+					      bits_per_word_mask));
+
+	priv->max_freq_hz =
+		virtio_cread32(vdev, offsetof(struct virtio_spi_config,
+					      max_freq_hz));
+}
+
+static int virtio_spi_find_vqs(struct virtio_spi_priv *priv)
+{
+	struct virtqueue *vq;
+
+	vq = virtio_find_single_vq(priv->vdev, virtio_spi_msg_done, "spi-rq");
+	if (IS_ERR(vq))
+		return PTR_ERR(vq);
+	priv->vq = vq;
+	return 0;
+}
+
+/* Function must not be called before virtio_spi_find_vqs() has been run */
+static void virtio_spi_del_vq(struct virtio_device *vdev)
+{
+	virtio_reset_device(vdev);
+	vdev->config->del_vqs(vdev);
+}
+
+static int virtio_spi_probe(struct virtio_device *vdev)
+{
+	struct virtio_spi_priv *priv;
+	struct spi_controller *ctrl;
+	int err;
+	u32 bus_num;
+
+	ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
+	if (!ctrl)
+		return -ENOMEM;
+
+	priv = spi_controller_get_devdata(ctrl);
+	priv->vdev = vdev;
+	vdev->priv = priv;
+
+	device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev));
+
+	/* Setup ACPI node for controlled devices which will be probed through ACPI. */
+	ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));
+
+	dev_set_drvdata(&vdev->dev, ctrl);
+
+	err = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
+	if (!err && bus_num <= S16_MAX)
+		ctrl->bus_num = bus_num;
+	else
+		ctrl->bus_num = -1;
+
+	virtio_spi_read_config(vdev);
+
+	ctrl->transfer_one = virtio_spi_transfer_one;
+
+	err = virtio_spi_find_vqs(priv);
+	if (err) {
+		dev_err_probe(&vdev->dev, err, "Cannot setup virtqueues\n");
+		return err;
+	}
+
+	/* Register cleanup for virtqueues using devm */
+	err = devm_add_action_or_reset(&vdev->dev, (void (*)(void *))virtio_spi_del_vq, vdev);
+	if (err) {
+		dev_err_probe(&vdev->dev, err, "Cannot register virtqueue cleanup\n");
+		return err;
+	}
+
+	/* Use devm version to register controller */
+	err = devm_spi_register_controller(&vdev->dev, ctrl);
+	if (err) {
+		dev_err_probe(&vdev->dev, err, "Cannot register controller\n");
+		return err;
+	}
+
+	return 0;
+}
+
+static int virtio_spi_freeze(struct device *dev)
+{
+	struct spi_controller *ctrl = dev_get_drvdata(dev);
+	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
+	int ret;
+
+	ret = spi_controller_suspend(ctrl);
+	if (ret) {
+		dev_warn(dev, "cannot suspend controller (%d)\n", ret);
+		return ret;
+	}
+
+	virtio_spi_del_vq(vdev);
+	return 0;
+}
+
+static int virtio_spi_restore(struct device *dev)
+{
+	struct spi_controller *ctrl = dev_get_drvdata(dev);
+	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
+	int ret;
+
+	ret = virtio_spi_find_vqs(vdev->priv);
+	if (ret) {
+		dev_err(dev, "problem starting vqueue (%d)\n", ret);
+		return ret;
+	}
+
+	ret = spi_controller_resume(ctrl);
+	if (ret)
+		dev_err(dev, "problem resuming controller (%d)\n", ret);
+
+	return ret;
+}
+
+static struct virtio_device_id virtio_spi_id_table[] = {
+	{ VIRTIO_ID_SPI, VIRTIO_DEV_ANY_ID },
+	{}
+};
+MODULE_DEVICE_TABLE(virtio, virtio_spi_id_table);
+
+static const struct dev_pm_ops virtio_spi_pm_ops = {
+	.freeze = pm_sleep_ptr(virtio_spi_freeze),
+	.restore = pm_sleep_ptr(virtio_spi_restore),
+};
+
+static struct virtio_driver virtio_spi_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.pm = &virtio_spi_pm_ops,
+	},
+	.id_table = virtio_spi_id_table,
+	.probe = virtio_spi_probe,
+};
+module_virtio_driver(virtio_spi_driver);
+
+MODULE_AUTHOR("OpenSynergy GmbH");
+MODULE_AUTHOR("Haixu Cui <quic_haixcui@quicinc.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Virtio SPI bus driver");
-- 
2.34.1

Re: [PATCH v4 2/3] virtio-spi: Add virtio-spi.h

[Andy Shevchenko]

On Wed, Aug 20, 2025 at 04:49:43PM +0800, Haixu Cui wrote:
> Add virtio-spi.h header for virtio SPI.

...

> +/**

This is kernel-doc comment...

> + * struct virtio_spi_config - All config fields are read-only for the
> + * Virtio SPI driver

> + */

...

> +/*
> + * @chip_select_id: chipselect index the SPI transfer used.
> + *

But this one (besides having tons of unneeded blank lines) is not. Why is this
inconsistency?

> + */

...

> +struct spi_transfer_result {
> +#define VIRTIO_SPI_TRANS_OK	0
> +#define VIRTIO_SPI_PARAM_ERR	1
> +#define VIRTIO_SPI_TRANS_ERR	2
> +	__u8 result;
> +};

And this data type has no doc at all...


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] SPI: Add virtio SPI driver

[Andy Shevchenko]

On Wed, Aug 20, 2025 at 04:49:44PM +0800, Haixu Cui wrote:
> This is the virtio SPI Linux kernel driver.

...

> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/stddef.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ring.h>
> +#include <linux/virtio_spi.h>

> +#define VIRTIO_SPI_MODE_MASK \
> +	(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)

We have SPI_MODE_X_MASK.

...

> +struct virtio_spi_req {
> +	struct completion completion;
> +	struct spi_transfer_head transfer_head	____cacheline_aligned;
> +	const u8 *tx_buf;
> +	u8 *rx_buf;
> +	struct spi_transfer_result result	____cacheline_aligned;
> +};

Dunno if `pahole` aware of ____cacheline_aligned attribute, but does it shows
any potential improvement?

I think the fields can be reshuffled to go last and only one needs that
attribute.

struct virtio_spi_req {
	struct completion completion;
	const u8 *tx_buf;
	u8 *rx_buf;
	struct spi_transfer_head transfer_head	____cacheline_aligned;
	struct spi_transfer_result result;
};

...

> +static int virtio_spi_set_delays(struct spi_transfer_head *th,
> +				 struct spi_device *spi,
> +				 struct spi_transfer *xfer)
> +{
> +	int cs_setup;
> +	int cs_word_delay_xfer;
> +	int cs_word_delay_spi;
> +	int delay;
> +	int cs_hold;
> +	int cs_inactive;
> +	int cs_change_delay;
> +
> +	cs_setup = spi_delay_to_ns(&spi->cs_setup, xfer);
> +	if (cs_setup < 0) {

Hmm... Not a problem in your code, but ns can be quite high for low speed
links, there is a potential overflow...

> +		dev_warn(&spi->dev, "Cannot convert cs_setup\n");
> +		return cs_setup;
> +	}
> +	th->cs_setup_ns = cpu_to_le32(cs_setup);
> +
> +	cs_word_delay_xfer = spi_delay_to_ns(&xfer->word_delay, xfer);
> +	if (cs_word_delay_xfer < 0) {
> +		dev_warn(&spi->dev, "Cannot convert cs_word_delay_xfer\n");
> +		return cs_word_delay_xfer;
> +	}
> +	cs_word_delay_spi = spi_delay_to_ns(&spi->word_delay, xfer);
> +	if (cs_word_delay_spi < 0) {
> +		dev_warn(&spi->dev, "Cannot convert cs_word_delay_spi\n");
> +		return cs_word_delay_spi;
> +	}

> +	if (cs_word_delay_spi > cs_word_delay_xfer)
> +		th->word_delay_ns = cpu_to_le32(cs_word_delay_spi);
> +	else
> +		th->word_delay_ns = cpu_to_le32(cs_word_delay_xfer);

Why not max() ?

> +	delay = spi_delay_to_ns(&xfer->delay, xfer);
> +	if (delay < 0) {
> +		dev_warn(&spi->dev, "Cannot convert delay\n");
> +		return delay;
> +	}
> +	cs_hold = spi_delay_to_ns(&spi->cs_hold, xfer);
> +	if (cs_hold < 0) {
> +		dev_warn(&spi->dev, "Cannot convert cs_hold\n");
> +		return cs_hold;
> +	}
> +	th->cs_delay_hold_ns = cpu_to_le32(delay + cs_hold);
> +
> +	cs_inactive = spi_delay_to_ns(&spi->cs_inactive, xfer);
> +	if (cs_inactive < 0) {
> +		dev_warn(&spi->dev, "Cannot convert cs_inactive\n");
> +		return cs_inactive;
> +	}
> +	cs_change_delay = spi_delay_to_ns(&xfer->cs_change_delay, xfer);
> +	if (cs_change_delay < 0) {
> +		dev_warn(&spi->dev, "Cannot convert cs_change_delay\n");
> +		return cs_change_delay;
> +	}
> +	th->cs_change_delay_inactive_ns =
> +		cpu_to_le32(cs_inactive + cs_change_delay);
> +
> +	return 0;
> +}

...

> +static int virtio_spi_transfer_one(struct spi_controller *ctrl,
> +				   struct spi_device *spi,
> +				   struct spi_transfer *xfer)
> +{
> +	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
> +	struct virtio_spi_req *spi_req;
> +	struct spi_transfer_head *th;
> +	struct scatterlist sg_out_head, sg_out_payload;
> +	struct scatterlist sg_in_result, sg_in_payload;
> +	struct scatterlist *sgs[4];

> +	unsigned int outcnt = 0u;
> +	unsigned int incnt = 0u;

Are 'u':s important in this case/

> +	int ret;
> +
> +	spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
> +	if (!spi_req)
> +		return -ENOMEM;
> +
> +	init_completion(&spi_req->completion);
> +
> +	th = &spi_req->transfer_head;
> +
> +	/* Fill struct spi_transfer_head */
> +	th->chip_select_id = spi_get_chipselect(spi, 0);
> +	th->bits_per_word = spi->bits_per_word;
> +	th->cs_change = xfer->cs_change;
> +	th->tx_nbits = xfer->tx_nbits;
> +	th->rx_nbits = xfer->rx_nbits;
> +	th->reserved[0] = 0;
> +	th->reserved[1] = 0;
> +	th->reserved[2] = 0;
> +
> +	static_assert(VIRTIO_SPI_CPHA == SPI_CPHA,
> +		      "VIRTIO_SPI_CPHA must match SPI_CPHA");
> +	static_assert(VIRTIO_SPI_CPOL == SPI_CPOL,
> +		      "VIRTIO_SPI_CPOL must match SPI_CPOL");
> +	static_assert(VIRTIO_SPI_CS_HIGH == SPI_CS_HIGH,
> +		      "VIRTIO_SPI_CS_HIGH must match SPI_CS_HIGH");
> +	static_assert(VIRTIO_SPI_MODE_LSB_FIRST == SPI_LSB_FIRST,
> +		      "VIRTIO_SPI_MODE_LSB_FIRST must match SPI_LSB_FIRST");
> +
> +	th->mode = cpu_to_le32(spi->mode & VIRTIO_SPI_MODE_MASK);
> +	if (spi->mode & SPI_LOOP)
> +		th->mode |= cpu_to_le32(VIRTIO_SPI_MODE_LOOP);
> +
> +	th->freq = cpu_to_le32(xfer->speed_hz);
> +
> +	ret = virtio_spi_set_delays(th, spi, xfer);
> +	if (ret)
> +		goto msg_done;
> +
> +	/* Set buffers */
> +	spi_req->tx_buf = xfer->tx_buf;
> +	spi_req->rx_buf = xfer->rx_buf;
> +
> +	/* Prepare sending of virtio message */
> +	init_completion(&spi_req->completion);
> +
> +	sg_init_one(&sg_out_head, th, sizeof(*th));
> +	sgs[outcnt] = &sg_out_head;
> +	outcnt++;
> +
> +	if (spi_req->tx_buf) {
> +		sg_init_one(&sg_out_payload, spi_req->tx_buf, xfer->len);
> +		sgs[outcnt] = &sg_out_payload;
> +		outcnt++;
> +	}
> +
> +	if (spi_req->rx_buf) {
> +		sg_init_one(&sg_in_payload, spi_req->rx_buf, xfer->len);
> +		sgs[outcnt] = &sg_in_payload;
> +		incnt++;
> +	}
> +
> +	sg_init_one(&sg_in_result, &spi_req->result,
> +		    sizeof(struct spi_transfer_result));
> +	sgs[outcnt + incnt] = &sg_in_result;
> +	incnt++;
> +
> +	ret = virtqueue_add_sgs(priv->vq, sgs, outcnt, incnt, spi_req,
> +				GFP_KERNEL);
> +	if (ret)
> +		goto msg_done;
> +
> +	/* Simple implementation: There can be only one transfer in flight */
> +	virtqueue_kick(priv->vq);
> +
> +	wait_for_completion(&spi_req->completion);
> +
> +	/* Read result from message and translate return code */
> +	switch (spi_req->result.result) {
> +	case VIRTIO_SPI_TRANS_OK:
> +		break;
> +	case VIRTIO_SPI_PARAM_ERR:
> +		ret = -EINVAL;
> +		break;
> +	case VIRTIO_SPI_TRANS_ERR:
> +		ret = -EIO;
> +		break;
> +	default:
> +		ret = -EIO;
> +		break;
> +	}
> +
> +msg_done:

> +	kfree(spi_req);

Can be called via __free() to simplify the error handling,

> +	if (ret)
> +		ctrl->cur_msg->status = ret;
> +
> +	return ret;
> +}

...

> +static int virtio_spi_probe(struct virtio_device *vdev)
> +{
> +	struct virtio_spi_priv *priv;
> +	struct spi_controller *ctrl;

> +	int err;

Out of a sudden it's named 'err'. Please, go through the code and make
style/naming/etc consistent.

> +	u32 bus_num;
> +
> +	ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	priv = spi_controller_get_devdata(ctrl);
> +	priv->vdev = vdev;
> +	vdev->priv = priv;

> +	device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev));

> +	/* Setup ACPI node for controlled devices which will be probed through ACPI. */
> +	ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));

This is strange. Either you need to put parent above in device_set_node() or
drop it here. Otherwise it's inconsistent. Needs a very good explanation what's
going on here...

> +	dev_set_drvdata(&vdev->dev, ctrl);
> +
> +	err = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
> +	if (!err && bus_num <= S16_MAX)

This is wrong. What is the bus_num value when err != 0?
And why do we even care about this?

> +		ctrl->bus_num = bus_num;
> +	else
> +		ctrl->bus_num = -1;
> +
> +	virtio_spi_read_config(vdev);
> +
> +	ctrl->transfer_one = virtio_spi_transfer_one;
> +
> +	err = virtio_spi_find_vqs(priv);
> +	if (err) {

> +		dev_err_probe(&vdev->dev, err, "Cannot setup virtqueues\n");
> +		return err;

		return dev_err_probe(...);

> +	}

> +	/* Register cleanup for virtqueues using devm */
> +	err = devm_add_action_or_reset(&vdev->dev, (void (*)(void *))virtio_spi_del_vq, vdev);
> +	if (err) {
> +		dev_err_probe(&vdev->dev, err, "Cannot register virtqueue cleanup\n");
> +		return err;
> +	}
> +
> +	/* Use devm version to register controller */
> +	err = devm_spi_register_controller(&vdev->dev, ctrl);
> +	if (err) {
> +		dev_err_probe(&vdev->dev, err, "Cannot register controller\n");
> +		return err;

As per above.

> +	}
> +
> +	return 0;
> +}

...

> +static int virtio_spi_freeze(struct device *dev)
> +{
> +	struct spi_controller *ctrl = dev_get_drvdata(dev);
> +	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);

Use dev_to_virtio()

> +	int ret;
> +
> +	ret = spi_controller_suspend(ctrl);
> +	if (ret) {
> +		dev_warn(dev, "cannot suspend controller (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	virtio_spi_del_vq(vdev);
> +	return 0;
> +}
> +
> +static int virtio_spi_restore(struct device *dev)
> +{
> +	struct spi_controller *ctrl = dev_get_drvdata(dev);
> +	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);

As per above.

> +	int ret;
> +
> +	ret = virtio_spi_find_vqs(vdev->priv);
> +	if (ret) {
> +		dev_err(dev, "problem starting vqueue (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = spi_controller_resume(ctrl);
> +	if (ret)
> +		dev_err(dev, "problem resuming controller (%d)\n", ret);
> +
> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] SPI: Add virtio SPI driver

[kernel test robot]

Hi Haixu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-spi/for-next]
[also build test WARNING on linus/master v6.17-rc2 next-20250820]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Haixu-Cui/virtio-Add-ID-for-virtio-SPI/20250820-165441
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
patch link:    https://lore.kernel.org/r/20250820084944.84505-4-quic_haixcui%40quicinc.com
patch subject: [PATCH v4 3/3] SPI: Add virtio SPI driver
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20250821/202508211051.cuOjaH00-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 93d24b6b7b148c47a2fa228a4ef31524fa1d9f3f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250821/202508211051.cuOjaH00-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508211051.cuOjaH00-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/spi/spi-virtio.c:373:45: warning: cast from 'void (*)(struct virtio_device *)' to 'void (*)(void *)' converts to incompatible function type [-Wcast-function-type-strict]
     373 |         err = devm_add_action_or_reset(&vdev->dev, (void (*)(void *))virtio_spi_del_vq, vdev);
         |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/device/devres.h:166:34: note: expanded from macro 'devm_add_action_or_reset'
     166 |         __devm_add_action_or_reset(dev, action, data, #action)
         |                                         ^~~~~~
   1 warning generated.


vim +373 drivers/spi/spi-virtio.c

   333	
   334	static int virtio_spi_probe(struct virtio_device *vdev)
   335	{
   336		struct virtio_spi_priv *priv;
   337		struct spi_controller *ctrl;
   338		int err;
   339		u32 bus_num;
   340	
   341		ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
   342		if (!ctrl)
   343			return -ENOMEM;
   344	
   345		priv = spi_controller_get_devdata(ctrl);
   346		priv->vdev = vdev;
   347		vdev->priv = priv;
   348	
   349		device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev));
   350	
   351		/* Setup ACPI node for controlled devices which will be probed through ACPI. */
   352		ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));
   353	
   354		dev_set_drvdata(&vdev->dev, ctrl);
   355	
   356		err = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
   357		if (!err && bus_num <= S16_MAX)
   358			ctrl->bus_num = bus_num;
   359		else
   360			ctrl->bus_num = -1;
   361	
   362		virtio_spi_read_config(vdev);
   363	
   364		ctrl->transfer_one = virtio_spi_transfer_one;
   365	
   366		err = virtio_spi_find_vqs(priv);
   367		if (err) {
   368			dev_err_probe(&vdev->dev, err, "Cannot setup virtqueues\n");
   369			return err;
   370		}
   371	
   372		/* Register cleanup for virtqueues using devm */
 > 373		err = devm_add_action_or_reset(&vdev->dev, (void (*)(void *))virtio_spi_del_vq, vdev);
   374		if (err) {
   375			dev_err_probe(&vdev->dev, err, "Cannot register virtqueue cleanup\n");
   376			return err;
   377		}
   378	
   379		/* Use devm version to register controller */
   380		err = devm_spi_register_controller(&vdev->dev, ctrl);
   381		if (err) {
   382			dev_err_probe(&vdev->dev, err, "Cannot register controller\n");
   383			return err;
   384		}
   385	
   386		return 0;
   387	}
   388	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 2/3] virtio-spi: Add virtio-spi.h

[Michael S. Tsirkin]

On Wed, Aug 20, 2025 at 04:49:43PM +0800, Haixu Cui wrote:
> Add virtio-spi.h header for virtio SPI.
> 
> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
> ---
>  MAINTAINERS                     |   5 +
>  include/uapi/linux/virtio_spi.h | 185 ++++++++++++++++++++++++++++++++
>  2 files changed, 190 insertions(+)
>  create mode 100644 include/uapi/linux/virtio_spi.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index daf520a13bdf..3e289677ca18 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26760,6 +26760,11 @@ S:	Maintained
>  F:	include/uapi/linux/virtio_snd.h
>  F:	sound/virtio/*
>  
> +VIRTIO SPI DRIVER
> +M:	Haixu Cui <quic_haixcui@quicinc.com>
> +S:	Maintained
> +F:	include/uapi/linux/virtio_spi.h
> +

I would add a mailing list:

virtualization@lists.linux-foundation.org


>  VIRTUAL BOX GUEST DEVICE DRIVER
>  M:	Hans de Goede <hansg@kernel.org>
>  M:	Arnd Bergmann <arnd@arndb.de>
> diff --git a/include/uapi/linux/virtio_spi.h b/include/uapi/linux/virtio_spi.h
> new file mode 100644
> index 000000000000..b55877b3e525
> --- /dev/null
> +++ b/include/uapi/linux/virtio_spi.h
> @@ -0,0 +1,185 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Copyright (C) 2023 OpenSynergy GmbH
> + * Copyright (C) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#ifndef _LINUX_VIRTIO_VIRTIO_SPI_H
> +#define _LINUX_VIRTIO_VIRTIO_SPI_H
> +
> +#include <linux/types.h>
> +#include <linux/virtio_config.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_types.h>
> +
> +/* Sample data on trailing clock edge */
> +#define VIRTIO_SPI_CPHA			_BITUL(0)
> +/* Clock is high when IDLE */
> +#define VIRTIO_SPI_CPOL			_BITUL(1)
> +/* Chip Select is active high */
> +#define VIRTIO_SPI_CS_HIGH			_BITUL(2)
> +/* Transmit LSB first */
> +#define VIRTIO_SPI_MODE_LSB_FIRST		_BITUL(3)
> +/* Loopback mode */
> +#define VIRTIO_SPI_MODE_LOOP			_BITUL(4)
> +
> +/**
> + * struct virtio_spi_config - All config fields are read-only for the
> + * Virtio SPI driver
> + * @cs_max_number: maximum number of chipselect the host SPI controller
> + *   supports.
> + * @cs_change_supported: indicates if the host SPI controller supports to toggle
> + *   chipselect after each transfer in one message:
> + *   0: unsupported, chipselect will be kept in active state throughout the
> + *      message transaction;
> + *   1: supported.
> + *   Note: Message here contains a sequence of SPI transfers.
> + * @tx_nbits_supported: indicates the supported number of bit for writing:
> + *   bit 0: DUAL (2-bit transfer), 1 for supported
> + *   bit 1: QUAD (4-bit transfer), 1 for supported
> + *   bit 2: OCTAL (8-bit transfer), 1 for supported
> + *   other bits are reserved as 0, 1-bit transfer is always supported.
> + * @rx_nbits_supported: indicates the supported number of bit for reading:
> + *   bit 0: DUAL (2-bit transfer), 1 for supported
> + *   bit 1: QUAD (4-bit transfer), 1 for supported
> + *   bit 2: OCTAL (8-bit transfer), 1 for supported
> + *   other bits are reserved as 0, 1-bit transfer is always supported.
> + * @bits_per_word_mask: mask indicating which values of bits_per_word are
> + *   supported. If not set, no limitation for bits_per_word.
> + * @mode_func_supported: indicates the following features are supported or not:
> + *   bit 0-1: CPHA feature
> + *     0b00: invalid, should support as least one CPHA setting
> + *     0b01: supports CPHA=0 only
> + *     0b10: supports CPHA=1 only
> + *     0b11: supports CPHA=0 and CPHA=1.
> + *   bit 2-3: CPOL feature
> + *     0b00: invalid, should support as least one CPOL setting
> + *     0b01: supports CPOL=0 only
> + *     0b10: supports CPOL=1 only
> + *     0b11: supports CPOL=0 and CPOL=1.
> + *   bit 4: chipselect active high feature, 0 for unsupported and 1 for
> + *     supported, chipselect active low is supported by default.
> + *   bit 5: LSB first feature, 0 for unsupported and 1 for supported,
> + *     MSB first is supported by default.
> + *   bit 6: loopback mode feature, 0 for unsupported and 1 for supported,
> + *     normal mode is supported by default.
> + * @max_freq_hz: the maximum clock rate supported in Hz unit, 0 means no
> + *   limitation for transfer speed.
> + * @max_word_delay_ns: the maximum word delay supported, in nanoseconds.
> + *   A value of 0 indicates that word delay is unsupported.
> + *   Each transfer may consist of a sequence of words.
> + * @max_cs_setup_ns: the maximum delay supported after chipselect is asserted,
> + *   in ns unit, 0 means delay is not supported to introduce after chipselect is
> + *   asserted.
> + * @max_cs_hold_ns: the maximum delay supported before chipselect is deasserted,
> + *   in ns unit, 0 means delay is not supported to introduce before chipselect
> + *   is deasserted.
> + * @max_cs_incative_ns: maximum delay supported after chipselect is deasserted,
> + *   in ns unit, 0 means delay is not supported to introduce after chipselect is
> + *   deasserted.
> + */
> +struct virtio_spi_config {
> +	/* # of /dev/spidev<bus_num>.CS with CS=0..chip_select_max_number -1 */
> +	__u8 cs_max_number;
> +	__u8 cs_change_supported;
> +#define VIRTIO_SPI_RX_TX_SUPPORT_DUAL		_BITUL(0)
> +#define VIRTIO_SPI_RX_TX_SUPPORT_QUAD		_BITUL(1)
> +#define VIRTIO_SPI_RX_TX_SUPPORT_OCTAL		_BITUL(2)
> +	__u8 tx_nbits_supported;
> +	__u8 rx_nbits_supported;
> +	__le32 bits_per_word_mask;
> +#define VIRTIO_SPI_MF_SUPPORT_CPHA_0		_BITUL(0)
> +#define VIRTIO_SPI_MF_SUPPORT_CPHA_1		_BITUL(1)
> +#define VIRTIO_SPI_MF_SUPPORT_CPOL_0		_BITUL(2)
> +#define VIRTIO_SPI_MF_SUPPORT_CPOL_1		_BITUL(3)
> +#define VIRTIO_SPI_MF_SUPPORT_CS_HIGH		_BITUL(4)
> +#define VIRTIO_SPI_MF_SUPPORT_LSB_FIRST		_BITUL(5)
> +#define VIRTIO_SPI_MF_SUPPORT_LOOPBACK		_BITUL(6)
> +	__le32 mode_func_supported;
> +	__le32 max_freq_hz;
> +	__le32 max_word_delay_ns;
> +	__le32 max_cs_setup_ns;
> +	__le32 max_cs_hold_ns;
> +	__le32 max_cs_inactive_ns;
> +};
> +
> +/*
> + * @chip_select_id: chipselect index the SPI transfer used.
> + *
> + * @bits_per_word: the number of bits in each SPI transfer word.
> + *
> + * @cs_change: whether to deselect device after finishing this transfer
> + *     before starting the next transfer, 0 means cs keep asserted and
> + *     1 means cs deasserted then asserted again.
> + *
> + * @tx_nbits: bus width for write transfer.
> + *     0,1: bus width is 1, also known as SINGLE
> + *     2  : bus width is 2, also known as DUAL
> + *     4  : bus width is 4, also known as QUAD
> + *     8  : bus width is 8, also known as OCTAL
> + *     other values are invalid.
> + *
> + * @rx_nbits: bus width for read transfer.
> + *     0,1: bus width is 1, also known as SINGLE
> + *     2  : bus width is 2, also known as DUAL
> + *     4  : bus width is 4, also known as QUAD
> + *     8  : bus width is 8, also known as OCTAL
> + *     other values are invalid.
> + *
> + * @reserved: for future use.
> + *
> + * @mode: SPI transfer mode.
> + *     bit 0: CPHA, determines the timing (i.e. phase) of the data
> + *         bits relative to the clock pulses.For CPHA=0, the
> + *         "out" side changes the data on the trailing edge of the
> + *         preceding clock cycle, while the "in" side captures the data
> + *         on (or shortly after) the leading edge of the clock cycle.
> + *         For CPHA=1, the "out" side changes the data on the leading
> + *         edge of the current clock cycle, while the "in" side
> + *         captures the data on (or shortly after) the trailing edge of
> + *         the clock cycle.
> + *     bit 1: CPOL, determines the polarity of the clock. CPOL=0 is a
> + *         clock which idles at 0, and each cycle consists of a pulse
> + *         of 1. CPOL=1 is a clock which idles at 1, and each cycle
> + *         consists of a pulse of 0.
> + *     bit 2: CS_HIGH, if 1, chip select active high, else active low.
> + *     bit 3: LSB_FIRST, determines per-word bits-on-wire, if 0, MSB
> + *         first, else LSB first.
> + *     bit 4: LOOP, loopback mode.
> + *
> + * @freq: the transfer speed in Hz.
> + *
> + * @word_delay_ns: delay to be inserted between consecutive words of a
> + *     transfer, in ns unit.
> + *
> + * @cs_setup_ns: delay to be introduced after CS is asserted, in ns
> + *     unit.
> + *
> + * @cs_delay_hold_ns: delay to be introduced before CS is deasserted
> + *     for each transfer, in ns unit.
> + *
> + * @cs_change_delay_inactive_ns: delay to be introduced after CS is
> + *     deasserted and before next asserted, in ns unit.
> + */
> +struct spi_transfer_head {
> +	__u8 chip_select_id;
> +	__u8 bits_per_word;
> +	__u8 cs_change;
> +	__u8 tx_nbits;
> +	__u8 rx_nbits;
> +	__u8 reserved[3];
> +	__le32 mode;
> +	__le32 freq;
> +	__le32 word_delay_ns;
> +	__le32 cs_setup_ns;
> +	__le32 cs_delay_hold_ns;
> +	__le32 cs_change_delay_inactive_ns;
> +};
> +
> +struct spi_transfer_result {
> +#define VIRTIO_SPI_TRANS_OK	0
> +#define VIRTIO_SPI_PARAM_ERR	1
> +#define VIRTIO_SPI_TRANS_ERR	2
> +	__u8 result;
> +};
> +
> +#endif /* #ifndef _LINUX_VIRTIO_VIRTIO_SPI_H */
> -- 
> 2.34.1
> 

Re: [PATCH v4 2/3] virtio-spi: Add virtio-spi.h

[Michael S. Tsirkin]

On Wed, Aug 20, 2025 at 04:49:43PM +0800, Haixu Cui wrote:
> Add virtio-spi.h header for virtio SPI.
> 
> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
> ---
>  MAINTAINERS                     |   5 +
>  include/uapi/linux/virtio_spi.h | 185 ++++++++++++++++++++++++++++++++
>  2 files changed, 190 insertions(+)
>  create mode 100644 include/uapi/linux/virtio_spi.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index daf520a13bdf..3e289677ca18 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26760,6 +26760,11 @@ S:	Maintained
>  F:	include/uapi/linux/virtio_snd.h
>  F:	sound/virtio/*
>  
> +VIRTIO SPI DRIVER
> +M:	Haixu Cui <quic_haixcui@quicinc.com>
> +S:	Maintained
> +F:	include/uapi/linux/virtio_spi.h
> +
>  VIRTUAL BOX GUEST DEVICE DRIVER
>  M:	Hans de Goede <hansg@kernel.org>
>  M:	Arnd Bergmann <arnd@arndb.de>
> diff --git a/include/uapi/linux/virtio_spi.h b/include/uapi/linux/virtio_spi.h
> new file mode 100644
> index 000000000000..b55877b3e525
> --- /dev/null
> +++ b/include/uapi/linux/virtio_spi.h
> @@ -0,0 +1,185 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Copyright (C) 2023 OpenSynergy GmbH
> + * Copyright (C) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#ifndef _LINUX_VIRTIO_VIRTIO_SPI_H
> +#define _LINUX_VIRTIO_VIRTIO_SPI_H
> +
> +#include <linux/types.h>
> +#include <linux/virtio_config.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_types.h>
> +
> +/* Sample data on trailing clock edge */
> +#define VIRTIO_SPI_CPHA			_BITUL(0)
> +/* Clock is high when IDLE */
> +#define VIRTIO_SPI_CPOL			_BITUL(1)
> +/* Chip Select is active high */
> +#define VIRTIO_SPI_CS_HIGH			_BITUL(2)
> +/* Transmit LSB first */
> +#define VIRTIO_SPI_MODE_LSB_FIRST		_BITUL(3)
> +/* Loopback mode */
> +#define VIRTIO_SPI_MODE_LOOP			_BITUL(4)

It is generally preferable to have an enum with just bit
numbers.


E.g.

enum {
VIRTIO_SPI_F_CPHA = 0,
}


Userspace can add _BITUL wrappers itself if it
wants.


> +
> +/**
> + * struct virtio_spi_config - All config fields are read-only for the
> + * Virtio SPI driver
> + * @cs_max_number: maximum number of chipselect the host SPI controller
> + *   supports.
> + * @cs_change_supported: indicates if the host SPI controller supports to toggle
> + *   chipselect after each transfer in one message:
> + *   0: unsupported, chipselect will be kept in active state throughout the
> + *      message transaction;
> + *   1: supported.
> + *   Note: Message here contains a sequence of SPI transfers.
> + * @tx_nbits_supported: indicates the supported number of bit for writing:
> + *   bit 0: DUAL (2-bit transfer), 1 for supported
> + *   bit 1: QUAD (4-bit transfer), 1 for supported
> + *   bit 2: OCTAL (8-bit transfer), 1 for supported
> + *   other bits are reserved as 0, 1-bit transfer is always supported.
> + * @rx_nbits_supported: indicates the supported number of bit for reading:
> + *   bit 0: DUAL (2-bit transfer), 1 for supported
> + *   bit 1: QUAD (4-bit transfer), 1 for supported
> + *   bit 2: OCTAL (8-bit transfer), 1 for supported
> + *   other bits are reserved as 0, 1-bit transfer is always supported.
> + * @bits_per_word_mask: mask indicating which values of bits_per_word are
> + *   supported. If not set, no limitation for bits_per_word.
> + * @mode_func_supported: indicates the following features are supported or not:
> + *   bit 0-1: CPHA feature
> + *     0b00: invalid, should support as least one CPHA setting
> + *     0b01: supports CPHA=0 only
> + *     0b10: supports CPHA=1 only
> + *     0b11: supports CPHA=0 and CPHA=1.
> + *   bit 2-3: CPOL feature
> + *     0b00: invalid, should support as least one CPOL setting
> + *     0b01: supports CPOL=0 only
> + *     0b10: supports CPOL=1 only
> + *     0b11: supports CPOL=0 and CPOL=1.
> + *   bit 4: chipselect active high feature, 0 for unsupported and 1 for
> + *     supported, chipselect active low is supported by default.
> + *   bit 5: LSB first feature, 0 for unsupported and 1 for supported,
> + *     MSB first is supported by default.
> + *   bit 6: loopback mode feature, 0 for unsupported and 1 for supported,
> + *     normal mode is supported by default.
> + * @max_freq_hz: the maximum clock rate supported in Hz unit, 0 means no
> + *   limitation for transfer speed.
> + * @max_word_delay_ns: the maximum word delay supported, in nanoseconds.
> + *   A value of 0 indicates that word delay is unsupported.
> + *   Each transfer may consist of a sequence of words.
> + * @max_cs_setup_ns: the maximum delay supported after chipselect is asserted,
> + *   in ns unit, 0 means delay is not supported to introduce after chipselect is
> + *   asserted.
> + * @max_cs_hold_ns: the maximum delay supported before chipselect is deasserted,
> + *   in ns unit, 0 means delay is not supported to introduce before chipselect
> + *   is deasserted.
> + * @max_cs_incative_ns: maximum delay supported after chipselect is deasserted,
> + *   in ns unit, 0 means delay is not supported to introduce after chipselect is
> + *   deasserted.
> + */
> +struct virtio_spi_config {
> +	/* # of /dev/spidev<bus_num>.CS with CS=0..chip_select_max_number -1 */
> +	__u8 cs_max_number;
> +	__u8 cs_change_supported;
> +#define VIRTIO_SPI_RX_TX_SUPPORT_DUAL		_BITUL(0)
> +#define VIRTIO_SPI_RX_TX_SUPPORT_QUAD		_BITUL(1)
> +#define VIRTIO_SPI_RX_TX_SUPPORT_OCTAL		_BITUL(2)
> +	__u8 tx_nbits_supported;
> +	__u8 rx_nbits_supported;
> +	__le32 bits_per_word_mask;
> +#define VIRTIO_SPI_MF_SUPPORT_CPHA_0		_BITUL(0)
> +#define VIRTIO_SPI_MF_SUPPORT_CPHA_1		_BITUL(1)
> +#define VIRTIO_SPI_MF_SUPPORT_CPOL_0		_BITUL(2)
> +#define VIRTIO_SPI_MF_SUPPORT_CPOL_1		_BITUL(3)
> +#define VIRTIO_SPI_MF_SUPPORT_CS_HIGH		_BITUL(4)
> +#define VIRTIO_SPI_MF_SUPPORT_LSB_FIRST		_BITUL(5)
> +#define VIRTIO_SPI_MF_SUPPORT_LOOPBACK		_BITUL(6)
> +	__le32 mode_func_supported;
> +	__le32 max_freq_hz;
> +	__le32 max_word_delay_ns;
> +	__le32 max_cs_setup_ns;
> +	__le32 max_cs_hold_ns;
> +	__le32 max_cs_inactive_ns;
> +};
> +
> +/*
> + * @chip_select_id: chipselect index the SPI transfer used.
> + *
> + * @bits_per_word: the number of bits in each SPI transfer word.
> + *
> + * @cs_change: whether to deselect device after finishing this transfer
> + *     before starting the next transfer, 0 means cs keep asserted and
> + *     1 means cs deasserted then asserted again.
> + *
> + * @tx_nbits: bus width for write transfer.
> + *     0,1: bus width is 1, also known as SINGLE
> + *     2  : bus width is 2, also known as DUAL
> + *     4  : bus width is 4, also known as QUAD
> + *     8  : bus width is 8, also known as OCTAL
> + *     other values are invalid.
> + *
> + * @rx_nbits: bus width for read transfer.
> + *     0,1: bus width is 1, also known as SINGLE
> + *     2  : bus width is 2, also known as DUAL
> + *     4  : bus width is 4, also known as QUAD
> + *     8  : bus width is 8, also known as OCTAL
> + *     other values are invalid.
> + *
> + * @reserved: for future use.
> + *
> + * @mode: SPI transfer mode.
> + *     bit 0: CPHA, determines the timing (i.e. phase) of the data
> + *         bits relative to the clock pulses.For CPHA=0, the
> + *         "out" side changes the data on the trailing edge of the
> + *         preceding clock cycle, while the "in" side captures the data
> + *         on (or shortly after) the leading edge of the clock cycle.
> + *         For CPHA=1, the "out" side changes the data on the leading
> + *         edge of the current clock cycle, while the "in" side
> + *         captures the data on (or shortly after) the trailing edge of
> + *         the clock cycle.
> + *     bit 1: CPOL, determines the polarity of the clock. CPOL=0 is a
> + *         clock which idles at 0, and each cycle consists of a pulse
> + *         of 1. CPOL=1 is a clock which idles at 1, and each cycle
> + *         consists of a pulse of 0.
> + *     bit 2: CS_HIGH, if 1, chip select active high, else active low.
> + *     bit 3: LSB_FIRST, determines per-word bits-on-wire, if 0, MSB
> + *         first, else LSB first.
> + *     bit 4: LOOP, loopback mode.
> + *
> + * @freq: the transfer speed in Hz.
> + *
> + * @word_delay_ns: delay to be inserted between consecutive words of a
> + *     transfer, in ns unit.
> + *
> + * @cs_setup_ns: delay to be introduced after CS is asserted, in ns
> + *     unit.
> + *
> + * @cs_delay_hold_ns: delay to be introduced before CS is deasserted
> + *     for each transfer, in ns unit.
> + *
> + * @cs_change_delay_inactive_ns: delay to be introduced after CS is
> + *     deasserted and before next asserted, in ns unit.
> + */
> +struct spi_transfer_head {
> +	__u8 chip_select_id;
> +	__u8 bits_per_word;
> +	__u8 cs_change;
> +	__u8 tx_nbits;
> +	__u8 rx_nbits;
> +	__u8 reserved[3];
> +	__le32 mode;
> +	__le32 freq;
> +	__le32 word_delay_ns;
> +	__le32 cs_setup_ns;
> +	__le32 cs_delay_hold_ns;
> +	__le32 cs_change_delay_inactive_ns;
> +};
> +
> +struct spi_transfer_result {
> +#define VIRTIO_SPI_TRANS_OK	0
> +#define VIRTIO_SPI_PARAM_ERR	1
> +#define VIRTIO_SPI_TRANS_ERR	2
> +	__u8 result;
> +};
> +
> +#endif /* #ifndef _LINUX_VIRTIO_VIRTIO_SPI_H */
> -- 
> 2.34.1
> 

Re: [PATCH v4 2/3] virtio-spi: Add virtio-spi.h

[Haixu Cui]

On 8/20/2025 10:22 PM, Andy Shevchenko wrote:
> On Wed, Aug 20, 2025 at 04:49:43PM +0800, Haixu Cui wrote:
>> Add virtio-spi.h header for virtio SPI.
> 
> ...
> 
>> +/**
> 
> This is kernel-doc comment...
> 
>> + * struct virtio_spi_config - All config fields are read-only for the
>> + * Virtio SPI driver
> 
>> + */
> 
> ...
> 
>> +/*
>> + * @chip_select_id: chipselect index the SPI transfer used.
>> + *
> 
> But this one (besides having tons of unneeded blank lines) is not. Why is this
> inconsistency?
> 
>> + */
> 

Thank you for pointing out the inconsistency. I will update the comment 
to follow the kernel-doc format and remove the unnecessary blank lines.
This change will be reflected in the next patch version.

> ...
> 
>> +struct spi_transfer_result {
>> +#define VIRTIO_SPI_TRANS_OK	0
>> +#define VIRTIO_SPI_PARAM_ERR	1
>> +#define VIRTIO_SPI_TRANS_ERR	2
>> +	__u8 result;
>> +};
> 
> And this data type has no doc at all...
> 
Okay, will add definition for spi_tranafer_result.

Re: [PATCH v4 3/3] SPI: Add virtio SPI driver

[Haixu Cui]

Hi Andy,

Thank you very much for your review and suggestions, I truly appreciate it!

I've carefully considered your comments and have replied directly under 
your comments with the corresponding changes. These updates will be 
included in next version of patch. Please let me know if there's 
anything else I should address.

Thanks again for your guidance and support.

Best Regards
Haixu Cui

On 8/20/2025 10:39 PM, Andy Shevchenko wrote:

> 
>> +#define VIRTIO_SPI_MODE_MASK \
>> +	(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)
> 
> We have SPI_MODE_X_MASK.
> 

#define VIRTIO_SPI_MODE_MASK \
         (SPI_MODE_X_MASK | SPI_CS_HIGH | SPI_LSB_FIRST)


>> +struct virtio_spi_req {
>> +	struct completion completion;
>> +	struct spi_transfer_head transfer_head	____cacheline_aligned;
>> +	const u8 *tx_buf;
>> +	u8 *rx_buf;
>> +	struct spi_transfer_result result	____cacheline_aligned;
>> +};
> 
> Dunno if `pahole` aware of ____cacheline_aligned attribute, but does it shows
> any potential improvement?
> 
> I think the fields can be reshuffled to go last and only one needs that
> attribute.
> 
> struct virtio_spi_req {
> 	struct completion completion;
> 	const u8 *tx_buf;
> 	u8 *rx_buf;
> 	struct spi_transfer_head transfer_head	____cacheline_aligned;
> 	struct spi_transfer_result result;
> };
> 
Ok, I’ll adjust the structure as recommended.



>> +static int virtio_spi_set_delays(struct spi_transfer_head *th,
>> +				 struct spi_device *spi,
>> +				 struct spi_transfer *xfer)
>> +{
>> +	int cs_setup;
>> +	int cs_word_delay_xfer;
>> +	int cs_word_delay_spi;
>> +	int delay;
>> +	int cs_hold;
>> +	int cs_inactive;
>> +	int cs_change_delay;
>> +
>> +	cs_setup = spi_delay_to_ns(&spi->cs_setup, xfer);
>> +	if (cs_setup < 0) {
> 
> Hmm... Not a problem in your code, but ns can be quite high for low speed
> links, there is a potential overflow...

You're right—ns values can be quite high on low-speed links, and there's 
a potential risk of overflow. I’ll keep a close eye on this and I've 
noted this as a follow-up action item.



> 
>> +	if (cs_word_delay_spi > cs_word_delay_xfer)
>> +		th->word_delay_ns = cpu_to_le32(cs_word_delay_spi);
>> +	else
>> +		th->word_delay_ns = cpu_to_le32(cs_word_delay_xfer);
> 
> Why not max() ?

update code with max() using:
th->word_delay_ns = cpu_to_le32(max(cs_word_delay_spi, cs_word_delay_xfer));



>> +	unsigned int outcnt = 0u;
>> +	unsigned int incnt = 0u;
> 
> Are 'u':s important in this case/

Will remove 'u' here.


>> +msg_done:
> 
>> +	kfree(spi_req);
> 
> Can be called via __free() to simplify the error handling,
> 

Yes, will update spi_req definition as follows then remove 
kfree(spi_req) here:

struct virtio_spi_req *spi_req __free(kfree);



>> +static int virtio_spi_probe(struct virtio_device *vdev)
>> +{
>> +	struct virtio_spi_priv *priv;
>> +	struct spi_controller *ctrl;
> 
>> +	int err;
> 
> Out of a sudden it's named 'err'. Please, go through the code and make
> style/naming/etc consistent.
> 
I'll update the naming to consistently use ret throughout, replacing err 
and other variants where appropriate



>> +	device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev));
> 
>> +	/* Setup ACPI node for controlled devices which will be probed through ACPI. */
>> +	ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));
> 
> This is strange. Either you need to put parent above in device_set_node() or
> drop it here. Otherwise it's inconsistent. Needs a very good explanation what's
> going on here...

I'll remove the ACPI_COMPANION_SET() line and kept only 
device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev)); to ensure consistency.

> 
>> +	dev_set_drvdata(&vdev->dev, ctrl);
>> +
>> +	err = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
>> +	if (!err && bus_num <= S16_MAX)
> 
> This is wrong. What is the bus_num value when err != 0?
> And why do we even care about this?
> 
>> +		ctrl->bus_num = bus_num;
>> +	else
>> +		ctrl->bus_num = -1;
>> +

Update as follows:
ret = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
         if (ret || bus_num > S16_MAX)
                 ctrl->bus_num = -1;
         else
                 ctrl->bus_num = bus_num;



>> +	err = virtio_spi_find_vqs(priv);
>> +	if (err) {
> 
>> +		dev_err_probe(&vdev->dev, err, "Cannot setup virtqueues\n");
>> +		return err;
> 
> 		return dev_err_probe(...);
> 
Acknowledged.




>> +	/* Register cleanup for virtqueues using devm */
>> +	err = devm_add_action_or_reset(&vdev->dev, (void (*)(void *))virtio_spi_del_vq, vdev);
>> +	if (err) {
>> +		dev_err_probe(&vdev->dev, err, "Cannot register virtqueue cleanup\n");
>> +		return err;
>> +	}
>> +
>> +	/* Use devm version to register controller */
>> +	err = devm_spi_register_controller(&vdev->dev, ctrl);
>> +	if (err) {
>> +		dev_err_probe(&vdev->dev, err, "Cannot register controller\n");
>> +		return err;
> 
> As per above.
Acknowledged

> 
>> +static int virtio_spi_freeze(struct device *dev)
>> +{
>> +	struct spi_controller *ctrl = dev_get_drvdata(dev);
>> +	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
> 
> Use dev_to_virtio()
Acknowledged




>> +static int virtio_spi_restore(struct device *dev)
>> +{
>> +	struct spi_controller *ctrl = dev_get_drvdata(dev);
>> +	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
> 
> As per above.
Acknowledged

Re: [PATCH v4 2/3] virtio-spi: Add virtio-spi.h

[Haixu Cui]

On 8/21/2025 4:42 PM, Michael S. Tsirkin wrote:
> On Wed, Aug 20, 2025 at 04:49:43PM +0800, Haixu Cui wrote:
>> Add virtio-spi.h header for virtio SPI.
>>
>> Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
>> ---
>>   MAINTAINERS                     |   5 +
>>   include/uapi/linux/virtio_spi.h | 185 ++++++++++++++++++++++++++++++++
>>   2 files changed, 190 insertions(+)
>>   create mode 100644 include/uapi/linux/virtio_spi.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index daf520a13bdf..3e289677ca18 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -26760,6 +26760,11 @@ S:	Maintained
>>   F:	include/uapi/linux/virtio_snd.h
>>   F:	sound/virtio/*
>>   
>> +VIRTIO SPI DRIVER
>> +M:	Haixu Cui <quic_haixcui@quicinc.com>
>> +S:	Maintained
>> +F:	include/uapi/linux/virtio_spi.h
>> +
> 
> I would add a mailing list:
> 
> virtualization@lists.linux-foundation.org
> 
> 

Hi Michael,

Thank you for the suggestion to add a mailing list to the MAINTAINERS entry.

I noticed that other VIRTIO drivers, such as VIRTIO BALLOON, are 
currently using virtualization@lists.linux.dev rather than 
virtualization@lists.linux-foundation.org.

Just to confirm—should I use virtualization@lists.linux.dev for 
consistency, or is virtualization@lists.linux-foundation.org the updated 
preferred list?

Appreciate your guidance!

Best regards,
Haixu Cui

Re: [PATCH v4 2/3] virtio-spi: Add virtio-spi.h

[Haixu Cui]

On 8/21/2025 4:45 PM, Michael S. Tsirkin wrote:


>> +
>> +/* Sample data on trailing clock edge */
>> +#define VIRTIO_SPI_CPHA			_BITUL(0)
>> +/* Clock is high when IDLE */
>> +#define VIRTIO_SPI_CPOL			_BITUL(1)
>> +/* Chip Select is active high */
>> +#define VIRTIO_SPI_CS_HIGH			_BITUL(2)
>> +/* Transmit LSB first */
>> +#define VIRTIO_SPI_MODE_LSB_FIRST		_BITUL(3)
>> +/* Loopback mode */
>> +#define VIRTIO_SPI_MODE_LOOP			_BITUL(4)
> 
> It is generally preferable to have an enum with just bit
> numbers.
> 
> 
> E.g.
> 
> enum {
> VIRTIO_SPI_F_CPHA = 0,
> }
> 
> 
> Userspace can add _BITUL wrappers itself if it
> wants.
> 
> 

Hi Michael,

Thank you for the suggestion regarding the bit definitions.

Would it be acceptable to keep the current macro definitions with 
_BITUL() because these macros are also used within the virtio SPI driver 
itself?

Looking forward to your guidance.

Best regards,
Haixu Cui

Re: [PATCH v4 2/3] virtio-spi: Add virtio-spi.h

[Michael S. Tsirkin]

On Mon, Aug 25, 2025 at 05:12:44PM +0800, Haixu Cui wrote:
> 
> On 8/21/2025 4:42 PM, Michael S. Tsirkin wrote:
> > On Wed, Aug 20, 2025 at 04:49:43PM +0800, Haixu Cui wrote:
> > > Add virtio-spi.h header for virtio SPI.
> > > 
> > > Signed-off-by: Haixu Cui <quic_haixcui@quicinc.com>
> > > ---
> > >   MAINTAINERS                     |   5 +
> > >   include/uapi/linux/virtio_spi.h | 185 ++++++++++++++++++++++++++++++++
> > >   2 files changed, 190 insertions(+)
> > >   create mode 100644 include/uapi/linux/virtio_spi.h
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index daf520a13bdf..3e289677ca18 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -26760,6 +26760,11 @@ S:	Maintained
> > >   F:	include/uapi/linux/virtio_snd.h
> > >   F:	sound/virtio/*
> > > +VIRTIO SPI DRIVER
> > > +M:	Haixu Cui <quic_haixcui@quicinc.com>
> > > +S:	Maintained
> > > +F:	include/uapi/linux/virtio_spi.h
> > > +
> > 
> > I would add a mailing list:
> > 
> > virtualization@lists.linux-foundation.org
> > 
> > 
> 
> Hi Michael,
> 
> Thank you for the suggestion to add a mailing list to the MAINTAINERS entry.
> 
> I noticed that other VIRTIO drivers, such as VIRTIO BALLOON, are currently
> using virtualization@lists.linux.dev rather than
> virtualization@lists.linux-foundation.org.
> 
> Just to confirm—should I use virtualization@lists.linux.dev for consistency,
> or is virtualization@lists.linux-foundation.org the updated preferred list?
> 
> Appreciate your guidance!
> 
> Best regards,
> Haixu Cui


virtualization@lists.linux.dev

Same as others.

Re: [PATCH v4 2/3] virtio-spi: Add virtio-spi.h

[Michael S. Tsirkin]

On Mon, Aug 25, 2025 at 05:19:03PM +0800, Haixu Cui wrote:
> 
> 
> On 8/21/2025 4:45 PM, Michael S. Tsirkin wrote:
> 
> 
> > > +
> > > +/* Sample data on trailing clock edge */
> > > +#define VIRTIO_SPI_CPHA			_BITUL(0)
> > > +/* Clock is high when IDLE */
> > > +#define VIRTIO_SPI_CPOL			_BITUL(1)
> > > +/* Chip Select is active high */
> > > +#define VIRTIO_SPI_CS_HIGH			_BITUL(2)
> > > +/* Transmit LSB first */
> > > +#define VIRTIO_SPI_MODE_LSB_FIRST		_BITUL(3)
> > > +/* Loopback mode */
> > > +#define VIRTIO_SPI_MODE_LOOP			_BITUL(4)
> > 
> > It is generally preferable to have an enum with just bit
> > numbers.
> > 
> > 
> > E.g.
> > 
> > enum {
> > VIRTIO_SPI_F_CPHA = 0,
> > }
> > 
> > 
> > Userspace can add _BITUL wrappers itself if it
> > wants.
> > 
> > 
> 
> Hi Michael,
> 
> Thank you for the suggestion regarding the bit definitions.
> 
> Would it be acceptable to keep the current macro definitions with _BITUL()
> because these macros are also used within the virtio SPI driver itself?
> 
> Looking forward to your guidance.
> 
> Best regards,
> Haixu Cui
> 


move them to the .c file if you want them.

Re: [PATCH v4 2/3] virtio-spi: Add virtio-spi.h

[Haixu Cui]

On 8/28/2025 6:34 PM, Michael S. Tsirkin wrote:
> On Mon, Aug 25, 2025 at 05:19:03PM +0800, Haixu Cui wrote:
>>
>>
>> On 8/21/2025 4:45 PM, Michael S. Tsirkin wrote:
>>
>>
>>>> +
>>>> +/* Sample data on trailing clock edge */
>>>> +#define VIRTIO_SPI_CPHA			_BITUL(0)
>>>> +/* Clock is high when IDLE */
>>>> +#define VIRTIO_SPI_CPOL			_BITUL(1)
>>>> +/* Chip Select is active high */
>>>> +#define VIRTIO_SPI_CS_HIGH			_BITUL(2)
>>>> +/* Transmit LSB first */
>>>> +#define VIRTIO_SPI_MODE_LSB_FIRST		_BITUL(3)
>>>> +/* Loopback mode */
>>>> +#define VIRTIO_SPI_MODE_LOOP			_BITUL(4)
>>>
>>> It is generally preferable to have an enum with just bit
>>> numbers.
>>>
>>>
>>> E.g.
>>>
>>> enum {
>>> VIRTIO_SPI_F_CPHA = 0,
>>> }
>>>
>>>
>>> Userspace can add _BITUL wrappers itself if it
>>> wants.
>>>
>>>
>>
>> Hi Michael,
>>
>> Thank you for the suggestion regarding the bit definitions.
>>
>> Would it be acceptable to keep the current macro definitions with _BITUL()
>> because these macros are also used within the virtio SPI driver itself?
>>
>> Looking forward to your guidance.
>>
>> Best regards,
>> Haixu Cui
>>
> 
> 
> move them to the .c file if you want them.
> 

Hi Michael,
I've observed that other virtio drivers - such as virtio-i2c and 
virtio-net - commonly define feature bits directly in their headers and 
use them in their drivers.

To maintain alignment with those drivers and ensure consistency in 
usage, I’d prefer to retain the current macro-based approach for 
defining feature bits.

Do you think this would be reasonable in this case?

thanks

Re: [PATCH v4 2/3] virtio-spi: Add virtio-spi.h

[Haixu Cui]

On 8/28/2025 6:34 PM, Michael S. Tsirkin wrote:
> On Mon, Aug 25, 2025 at 05:19:03PM +0800, Haixu Cui wrote:
>>
>>
>> On 8/21/2025 4:45 PM, Michael S. Tsirkin wrote:
>>
>>
>>>> +
>>>> +/* Sample data on trailing clock edge */
>>>> +#define VIRTIO_SPI_CPHA			_BITUL(0)
>>>> +/* Clock is high when IDLE */
>>>> +#define VIRTIO_SPI_CPOL			_BITUL(1)
>>>> +/* Chip Select is active high */
>>>> +#define VIRTIO_SPI_CS_HIGH			_BITUL(2)
>>>> +/* Transmit LSB first */
>>>> +#define VIRTIO_SPI_MODE_LSB_FIRST		_BITUL(3)
>>>> +/* Loopback mode */
>>>> +#define VIRTIO_SPI_MODE_LOOP			_BITUL(4)
>>>
>>> It is generally preferable to have an enum with just bit
>>> numbers.
>>>
>>>
>>> E.g.
>>>
>>> enum {
>>> VIRTIO_SPI_F_CPHA = 0,
>>> }
>>>
>>>
>>> Userspace can add _BITUL wrappers itself if it
>>> wants.
>>>
>>>
>>
>> Hi Michael,
>>
>> Thank you for the suggestion regarding the bit definitions.
>>
>> Would it be acceptable to keep the current macro definitions with _BITUL()
>> because these macros are also used within the virtio SPI driver itself?
>>
>> Looking forward to your guidance.
>>
>> Best regards,
>> Haixu Cui
>>
> 
> 
> move them to the .c file if you want them.
> 

Hi Michael,
I've observed that other virtio drivers - such as virtio-i2c and 
virtio-net - commonly define feature bits directly in their headers and 
use them in their drivers.

To maintain alignment with those drivers and ensure consistency in 
usage, I’d prefer to retain the current macro-based approach for 
defining feature bits.

Do you think this would be reasonable in this case?

Thanks & BR