Re: [RFC PATCH v5 3/3] SPI: Add virtio SPI driver.

From: Haixu Cui
Date: Fri Jul 04 2025 - 03:46:43 EST


Hi Mukesh,

Thank you for reviewing the patch and providing your feedback. Really
appreciate your detailed suggestions.

On 6/30/2025 2:54 PM, Mukesh Kumar Savaliya wrote:
Hi, Haixu, Thanks !

On 6/20/2025 9:42 AM, Haixu Cui wrote:
This is the virtio SPI Linux kernel driver.

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


+ * So the corresponding relationship:
+ * A   <===> cs_setup_ns (after CS asserted)
And "before clock start" ? to be added in bracket as comment.

Here I refer to the cs_setup definition in include/linux/spi/spi.h,
where the cs_setup is described only in terms of delay after CS is
asserted, without referencing the clock signal.


+
+static int virtio_spi_transfer_one(struct spi_controller *ctrl,
+                   struct spi_device *spi,
+                   struct spi_transfer *xfer)
+{

[...]

+    wait_for_completion(&priv->spi_req.completion);
+
I see init_completion(spi_req.completion) is called during probe() but successive transfer doent have reinit_completion(spi_req.completion). wondering how is this working for back to back transfers.

In current implementation, each SPI transfer uses the same spi_req
instance, which is a member of struct virtio_spi_priv.

I'm considering removing spi_req from the virtio_spi_priv structure,
instead dynamically allocation a new spi_req for each transfer. This
way, each transfer would have its own completion object, so we could
simply call init_completion() without worrying about reinitializing a
shared one. I believe this would make the design cleaner and avoid
potential issues.

Is this approach okay with you? If so I will update the patch
accordingly in the next revision.

+    /* Read result from message and translate return code */
+    switch (priv->spi_req.result.result) {
+    case VIRTIO_SPI_TRANS_OK:
+        /* ret is 0 */
+        break;
+    case VIRTIO_SPI_PARAM_ERR:
+        ret = -EINVAL;
+        break;
+    case VIRTIO_SPI_TRANS_ERR:
+        ret = -EIO;
+        break;
+    default: /* Protocol violation */
Comment in new line ? following same method across.

This comment seems not particularly helpful. I’ll remove it to keep
the code clean.

+static void virtio_spi_remove(struct virtio_device *vdev)
+{
+    struct spi_controller *ctrl = dev_get_drvdata(&vdev->dev);
+
+    /* Order: 1.) unregister controller, 2.) remove virtqueue */
Is this a specific flow for virtio OR generic ? if its generic, we can remove the comments.

This is generic actually, I will remove it to keep code clean.
+    spi_unregister_controller(ctrl);
+    virtio_spi_del_vq(vdev);
+}
+

Thanks
Haixu Cui