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 | 438 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 451 insertions(+)
 create mode 100644 drivers/spi/spi-virtio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index af98e411138d..50f77c0c9d3f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26794,6 +26794,7 @@ VIRTIO SPI DRIVER
 M:	Haixu Cui <quic_haixcui@quicinc.com>
 L:	virtualization@lists.linux-foundation.org
 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..ed0a071e760c
--- /dev/null
+++ b/drivers/spi/spi-virtio.c
@@ -0,0 +1,438 @@
+// 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_MODE_X_MASK | SPI_CS_HIGH | SPI_LSB_FIRST)
+
+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;
+};
+
+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;
+	}
+
+	th->word_delay_ns = cpu_to_le32(max(cs_word_delay_spi, 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 __free(kfree);
+	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 = 0;
+	unsigned int incnt = 0;
+	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:
+	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(void *data)
+{
+	struct virtio_device *vdev = data;
+
+	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 ret;
+	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));
+
+	dev_set_drvdata(&vdev->dev, ctrl);
+
+	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;
+
+	virtio_spi_read_config(vdev);
+
+	ctrl->transfer_one = virtio_spi_transfer_one;
+
+	ret = virtio_spi_find_vqs(priv);
+	if (ret)
+		return dev_err_probe(&vdev->dev, ret, "Cannot setup virtqueues\n");
+
+	/* Register cleanup for virtqueues using devm */
+	ret = devm_add_action_or_reset(&vdev->dev, virtio_spi_del_vq, vdev);
+	if (ret)
+		return dev_err_probe(&vdev->dev, ret, "Cannot register virtqueue cleanup\n");
+
+	/* Use devm version to register controller */
+	ret = devm_spi_register_controller(&vdev->dev, ctrl);
+	if (ret)
+		return dev_err_probe(&vdev->dev, ret, "Cannot register controller\n");
+
+	return 0;
+}
+
+static int virtio_spi_freeze(struct device *dev)
+{
+	struct spi_controller *ctrl = dev_get_drvdata(dev);
+	struct virtio_device *vdev = dev_to_virtio(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 = dev_to_virtio(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

On Thu, Aug 28, 2025 at 05:34:51PM +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>

A lot of headers are still missing. See below.

...


> +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;

uXX (in particular u32) is defined in types.h.

> +	/* Copy of config space max_freq_hz */
> +	u32 max_freq_hz;
> +};

...

> +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");

dev_warn() et al. are defined in dev_printk.h.

> +		return cs_setup;
> +	}
> +	th->cs_setup_ns = cpu_to_le32(cs_setup);

This requires

#include <asm/byteorder.h>

> +	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;
> +	}
> +
> +	th->word_delay_ns = cpu_to_le32(max(cs_word_delay_spi, cs_word_delay_xfer));

max() is defined in math.h.

> +	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 __free(kfree);

This is incorrect template. It's one of the exceptions when we mix code and
definitions...

> +	struct spi_transfer_head *th;
> +	struct scatterlist sg_out_head, sg_out_payload;
> +	struct scatterlist sg_in_result, sg_in_payload;

+ scatterlist.h

> +	struct scatterlist *sgs[4];
> +	unsigned int outcnt = 0;
> +	unsigned int incnt = 0;
> +	int ret;


> +	spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);

...so this should be

	struct virtio_spi_req *spi_req __free(kfree) =
		kzalloc(sizeof(*spi_req), GFP_KERNEL);

(or on one line if you are okay with a 100 limit).

And do not forget to include cleanup.h (__free() macro) and
slab.h (kzalloc() API).

> +	if (!spi_req)
> +		return -ENOMEM;

+ errno.h

But since you already have IS_ERR()/PTR_ERR() use, just

+ err.h

> +	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:
> +	if (ret)
> +		ctrl->cur_msg->status = ret;
> +
> +	return ret;
> +}

...

> +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);

See above.

> +	priv->vq = vq;
> +	return 0;
> +}

...

> +static int virtio_spi_probe(struct virtio_device *vdev)
> +{
> +	struct virtio_spi_priv *priv;
> +	struct spi_controller *ctrl;
> +	int ret;
> +	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));
> +	dev_set_drvdata(&vdev->dev, ctrl);
> +	ret = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);

+ device.h
+ property.h

> +	if (ret || bus_num > S16_MAX)

+ limits.h

> +		ctrl->bus_num = -1;
> +	else
> +		ctrl->bus_num = bus_num;

But why do we need this property at all? And where is it documented in the
device tree bindings?

> +	virtio_spi_read_config(vdev);
> +
> +	ctrl->transfer_one = virtio_spi_transfer_one;
> +
> +	ret = virtio_spi_find_vqs(priv);
> +	if (ret)
> +		return dev_err_probe(&vdev->dev, ret, "Cannot setup virtqueues\n");
> +
> +	/* Register cleanup for virtqueues using devm */
> +	ret = devm_add_action_or_reset(&vdev->dev, virtio_spi_del_vq, vdev);
> +	if (ret)
> +		return dev_err_probe(&vdev->dev, ret, "Cannot register virtqueue cleanup\n");
> +
> +	/* Use devm version to register controller */
> +	ret = devm_spi_register_controller(&vdev->dev, ctrl);
> +	if (ret)
> +		return dev_err_probe(&vdev->dev, ret, "Cannot register controller\n");
> +
> +	return 0;
> +}

...

> +static struct virtio_device_id virtio_spi_id_table[] = {

The type is or should be defined in mod_devicetable.h.

> +	{ VIRTIO_ID_SPI, VIRTIO_DEV_ANY_ID },
> +	{}
> +};

...

> +static const struct dev_pm_ops virtio_spi_pm_ops = {

The type is defined in pm.h.

> +	.freeze = pm_sleep_ptr(virtio_spi_freeze),
> +	.restore = pm_sleep_ptr(virtio_spi_restore),
> +};


-- 
With Best Regards,
Andy Shevchenko

Hi Andy,

On 9/1/2025 8:07 PM, Andy Shevchenko wrote:
> On Thu, Aug 28, 2025 at 05:34:51PM +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>
> 
> A lot of headers are still missing. See below.
> 
> ...
> 

This driver compiles successfully, and I believe all required 
definitions are resolved through indirect inclusion. For example, since 
I included virtio.h, there is no need to explicitly include device.h, 
scatterlist.h or types.h.

I avoided redundant #includes to keep the code clean and minimal.

If there are any essential headers I’ve overlooked, please feel free to 
highlight them—I’ll gladly include them in the next revision.



> 
>> +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 __free(kfree);
> 
> This is incorrect template. It's one of the exceptions when we mix code and
> definitions...
>> +	spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
> 
> ...so this should be
> 
> 	struct virtio_spi_req *spi_req __free(kfree) =
> 		kzalloc(sizeof(*spi_req), GFP_KERNEL);
> 
> (or on one line if you are okay with a 100 limit).
> 

I plan to update the code as follows:

struct virtio_spi_req *spi_req __free(kfree) = NULL;
spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
if(!spi_req)
     return -ENOMEM;

This follows the pattern used in 
virtio_net(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/virtio_net.c?h=v6.17-rc4#n3746)

I'd like to check if this style is acceptable here, thanks.



> 
>> +	if (ret || bus_num > S16_MAX)
> 
> + limits.h
> 
>> +		ctrl->bus_num = -1;
>> +	else
>> +		ctrl->bus_num = bus_num;
> 
> But why do we need this property at all? And where is it documented in the
> device tree bindings?

I’ve reviewed other SPI drivers in the kernel, and it seems that bus_num 
is not a required property in most cases. I’ll remove the related code 
in the next revision.

Thanks again.

BR
Haixu Cui

On Wed, Sep 03, 2025 at 05:04:46PM +0800, Haixu Cui wrote:
> On 9/1/2025 8:07 PM, Andy Shevchenko wrote:
> > On Thu, Aug 28, 2025 at 05:34:51PM +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>
> > 
> > A lot of headers are still missing. See below.
> 
> This driver compiles successfully, and I believe all required definitions
> are resolved through indirect inclusion. For example, since I included
> virtio.h, there is no need to explicitly include device.h, scatterlist.h or
> types.h.
> 
> I avoided redundant #includes to keep the code clean and minimal.
> 
> If there are any essential headers I’ve overlooked, please feel free to
> highlight them—I’ll gladly include them in the next revision.

The rationale is described on https://include-what-you-use.org/.

...

> I plan to update the code as follows:
> 
> struct virtio_spi_req *spi_req __free(kfree) = NULL;
> spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
> if(!spi_req)
>     return -ENOMEM;
> 
> This follows the pattern used in
> virtio_net(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/virtio_net.c?h=v6.17-rc4#n3746)
> 
> I'd like to check if this style is acceptable here, thanks.

The style is fine. The potential issue (not now and probably never) is that the
scope of the variable in this case is different which might lead to unexpected
side-effects. That said, You can go with it.

-- 
With Best Regards,
Andy Shevchenko

On 9/3/2025 6:27 PM, Andy Shevchenko wrote:
> On Wed, Sep 03, 2025 at 05:04:46PM +0800, Haixu Cui wrote:
>> On 9/1/2025 8:07 PM, Andy Shevchenko wrote:
>>> On Thu, Aug 28, 2025 at 05:34:51PM +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>
>>>
>>> A lot of headers are still missing. See below.
>>
>> This driver compiles successfully, and I believe all required definitions
>> are resolved through indirect inclusion. For example, since I included
>> virtio.h, there is no need to explicitly include device.h, scatterlist.h or
>> types.h.
>>
>> I avoided redundant #includes to keep the code clean and minimal.
>>
>> If there are any essential headers I’ve overlooked, please feel free to
>> highlight them—I’ll gladly include them in the next revision.
> 
> The rationale is described on https://include-what-you-use.org/.
> 

Hi Andy,

Thanks for your feedback and for pointing me to the iwyu guidelines.

I've experimented with the iwyu tool, and while for spi-virtio.c I 
noticed that it recommends header that is not directly to the code - 
such as vdso/cache.h - and occasionally suggests re-include header like 
linux/spi/spi.h that is already present.

iwyu is a power tool expecially in application-level development for C++ 
projects where header dependencies are more straightforward. However it 
seems iwyu may not yet be fully suited for analyzing Linux kernel due to 
its complexity and conditional inclusions.

Additionally, I’ve verified that the driver compiles successfully with 
both gcc and clang, which indicates that all required definitions are 
either directly or indirectly resolved.

I appreciate your guidance and will continue to refine the patch with 
clarity and maintainability in mind.

Best Regards
haixu Cui

On Wed, Sep 10, 2025 at 11:51:03AM +0800, Haixu Cui wrote:
> On 9/3/2025 6:27 PM, Andy Shevchenko wrote:
> > On Wed, Sep 03, 2025 at 05:04:46PM +0800, Haixu Cui wrote:
> > > On 9/1/2025 8:07 PM, Andy Shevchenko wrote:
> > > > On Thu, Aug 28, 2025 at 05:34:51PM +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>
> > > > 
> > > > A lot of headers are still missing. See below.
> > > 
> > > This driver compiles successfully, and I believe all required definitions
> > > are resolved through indirect inclusion. For example, since I included
> > > virtio.h, there is no need to explicitly include device.h, scatterlist.h or
> > > types.h.
> > > 
> > > I avoided redundant #includes to keep the code clean and minimal.
> > > 
> > > If there are any essential headers I’ve overlooked, please feel free to
> > > highlight them—I’ll gladly include them in the next revision.
> > 
> > The rationale is described on https://include-what-you-use.org/.
> 
> Thanks for your feedback and for pointing me to the iwyu guidelines.
> 
> I've experimented with the iwyu tool, and while for spi-virtio.c I noticed
> that it recommends header that is not directly to the code - such as
> vdso/cache.h - and occasionally suggests re-include header like
> linux/spi/spi.h that is already present.

> iwyu is a power tool expecially in application-level development for C++
> projects where header dependencies are more straightforward. However it
> seems iwyu may not yet be fully suited for analyzing Linux kernel due to its
> complexity and conditional inclusions.

I was not talking about the tool, yes, this tool is not ready for Linux kernel.
Jonathan Cameron played with it to make it useful for the Linux kernel project,
but it seems the work is stalled.

> Additionally, I’ve verified that the driver compiles successfully with both
> gcc and clang, which indicates that all required definitions are either
> directly or indirectly resolved.

The IWYU principle is against the includes that indirectly resolve types and
APIs. The problem that some headers are included in others and that relation
is guaranteed, but we do not have an official list of the guarantees, so this
is a tribal knowledge.

Basically the developer should know a bit more about the Linux kernel header
organisation to fulfill IWYU. And this is currently problematic.

> I appreciate your guidance and will continue to refine the patch with
> clarity and maintainability in mind.

-- 
With Best Regards,
Andy Shevchenko

Hello,

On 9/1/25 14:07, Andy Shevchenko wrote:
> On Thu, Aug 28, 2025 at 05:34:51PM +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>
> 
> A lot of headers are still missing. See below.

The driver should compile with the set of included headers.

> 
> ...
> 
> 
>> +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;
> 
> uXX (in particular u32) is defined in types.h.
> 
>> +	/* Copy of config space max_freq_hz */
>> +	u32 max_freq_hz;
>> +};

u32 is for sure defined in types.h. But looking into another device (gpio-virtio.c) I see that there is also u8 and u32 used and I don't see that there types.h is included directly in the C file. It must be included indirectly as the driver compiles. Same happens here.

I'm not aware of a requirement in the style guide for the Linux kernel that all used headers have to be included directly in the C file but this does not mean that such a requirement does not exist. Can anyone point my nose on it?

Regards
Harald Mommer

On Tue, Sep 02, 2025 at 05:54:39PM +0200, Harald Mommer wrote:

> I'm not aware of a requirement in the style guide for the Linux kernel
> that all used headers have to be included directly in the C file but
> this does not mean that such a requirement does not exist. Can anyone
> point my nose on it?

It's a general best practice for maintainability, I'm not aware of
anywhere where it's specifically written down but it's something people
commonly try to do.  That said so long as things compile I'm not going
to block anything on this sort of extremely pedantic review, especially
at this late stage in review.  I would have a lot more time for patches
for such issues, this is rather stop energy.