This is the 1st non-RFC version of a virtio SPI Linux driver which is
intended to be compliant with the 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 driver is the direct successor of the 3rd virtio driver RFC which
was based on the same specification text.
As in the meantime the virtio SPI specification has been accepted by
OASIS the driver is now based on an official specification (vs. a draft)
the time has come to remove the --rfc.
Changes between 1st and 2nd virtio SPI driver RFC:
- 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.
Changes between 2nd and 3rd virtio SPI driver RFC:
- 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 3rd virtio SPI driver RFC and non-RFC driver V1 (newest)
- 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
The virtio SPI driver was smoke tested on qemu using OpenSynergy's
proprietary virtio SPI device doing a SPI backend simulation on top of
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git tag
stable (commit 45ec2f5f6ed3ec3a79ba1329ad585497cdcbe663) and an adapted
version of the driver on Linux 6.5 with target hardware providing a
physical SPI backend device.
From: Harald Mommer <[email protected]>
This is the virtio SPI Linux kernel driver.
Signed-off-by: Harald Mommer <[email protected]>
---
MAINTAINERS | 6 +
drivers/spi/Kconfig | 11 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-virtio.c | 480 +++++++++++++++++++++++++++++++++++++++
4 files changed, 498 insertions(+)
create mode 100644 drivers/spi/spi-virtio.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 2ecaaec6a6bf..536fad147724 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23391,6 +23391,12 @@ S: Maintained
F: include/uapi/linux/virtio_snd.h
F: sound/virtio/*
+VIRTIO SPI DRIVER
+M: Harald Mommer <[email protected]>
+S: Maintained
+F: include/uapi/linux/virtio_spi.h
+F: drivers/spi/spi-virtio.c
+
VIRTUAL BOX GUEST DEVICE DRIVER
M: Hans de Goede <[email protected]>
M: Arnd Bergmann <[email protected]>
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ddae0fde798e..ff06e595679a 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -1125,6 +1125,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
+ This enables the Virtio SPI driver.
+
+ Virtio SPI is an SPI driver for virtual machines using Virtio.
+
+ If your Linux is a virtual machine using Virtio, say Y here.
+ If unsure, say N.
+
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 4ff8d725ba5e..ff2243e44e00 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -146,6 +146,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..7e1a75f1b45a
--- /dev/null
+++ b/drivers/spi/spi-virtio.c
@@ -0,0 +1,480 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SPI bus driver for the Virtio SPI controller
+ * Copyright (C) 2023 OpenSynergy GmbH
+ */
+
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/spi/spi.h>
+#include <linux/stddef.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ring.h>
+#include <linux/virtio_spi.h>
+
+struct virtio_spi_req {
+ struct completion completion;
+ struct spi_transfer_head transfer_head ____cacheline_aligned;
+ const uint8_t *tx_buf ____cacheline_aligned;
+ uint8_t *rx_buf ____cacheline_aligned;
+ struct spi_transfer_result result ____cacheline_aligned;
+};
+
+/* virtio_spi private data structure */
+struct virtio_spi_priv {
+ /* Virtio SPI message */
+ struct virtio_spi_req spi_req;
+ /* 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 struct spi_board_info board_info = {
+ .modalias = "spi-virtio",
+};
+
+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);
+}
+
+/*
+ * . . . . . . . . . .
+ * 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 (no matter with CS)
+ * 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((u32)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((u32)cs_word_delay_spi);
+ else
+ th->word_delay_ns = cpu_to_le32((u32)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((u32)delay + (u32)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((u32)cs_inactive + (u32)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 = &priv->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;
+
+ 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;
+
+ BUILD_BUG_ON(VIRTIO_SPI_CPHA != SPI_CPHA);
+ BUILD_BUG_ON(VIRTIO_SPI_CPOL != SPI_CPOL);
+ BUILD_BUG_ON(VIRTIO_SPI_CS_HIGH != SPI_CS_HIGH);
+ BUILD_BUG_ON(VIRTIO_SPI_MODE_LSB_FIRST != SPI_LSB_FIRST);
+
+ th->mode = cpu_to_le32(spi->mode & (SPI_LSB_FIRST | SPI_CS_HIGH |
+ SPI_CPOL | SPI_CPHA));
+ if ((spi->mode & SPI_LOOP) != 0)
+ 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 */
+ reinit_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(&priv->spi_req.completion);
+
+ /* 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 */
+ 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) != 0)
+ ctrl->mode_bits |= VIRTIO_SPI_CPHA;
+ if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_CPOL_1) != 0)
+ ctrl->mode_bits |= VIRTIO_SPI_CPOL;
+ if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LSB_FIRST) != 0)
+ ctrl->mode_bits |= SPI_LSB_FIRST;
+ if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LOOPBACK) != 0)
+ 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) != 0)
+ ctrl->mode_bits |= SPI_TX_DUAL;
+ if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD) != 0)
+ ctrl->mode_bits |= SPI_TX_QUAD;
+ if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL) != 0)
+ 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) != 0)
+ ctrl->mode_bits |= SPI_RX_DUAL;
+ if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD) != 0)
+ ctrl->mode_bits |= SPI_RX_QUAD;
+ if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL) != 0)
+ 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 (int)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_validate(struct virtio_device *vdev)
+{
+ /*
+ * SPI needs always access to the config space.
+ * Check that the driver can access the config space
+ */
+ if (!vdev->config->get) {
+ dev_err(&vdev->dev, "%s failure: config access disabled\n",
+ __func__);
+ return -EINVAL;
+ }
+
+ if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+ dev_err(&vdev->dev,
+ "device does not comply with spec version 1.x\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int virtio_spi_probe(struct virtio_device *vdev)
+{
+ struct device_node *np = vdev->dev.parent->of_node;
+ struct virtio_spi_priv *priv;
+ struct spi_controller *ctrl;
+ int err;
+ u32 bus_num;
+ u16 csi;
+
+ ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
+ if (!ctrl) {
+ dev_err(&vdev->dev, "Kernel memory exhausted in %s()\n",
+ __func__);
+ return -ENOMEM;
+ }
+
+ priv = spi_controller_get_devdata(ctrl);
+ priv->vdev = vdev;
+ vdev->priv = priv;
+ dev_set_drvdata(&vdev->dev, ctrl);
+
+ init_completion(&priv->spi_req.completion);
+
+ err = of_property_read_u32(np, "spi,bus-num", &bus_num);
+ if (!err && bus_num <= S16_MAX)
+ ctrl->bus_num = (s16)bus_num;
+
+ virtio_spi_read_config(vdev);
+
+ /* Method to do a single SPI transfer */
+ ctrl->transfer_one = virtio_spi_transfer_one;
+
+ /* Initialize virtqueues */
+ err = virtio_spi_find_vqs(priv);
+ if (err) {
+ dev_err(&vdev->dev, "Cannot setup virtqueues\n");
+ return err;
+ }
+
+ err = spi_register_controller(ctrl);
+ if (err) {
+ dev_err(&vdev->dev, "Cannot register controller\n");
+ goto err_return;
+ }
+
+ board_info.max_speed_hz = priv->max_freq_hz;
+ /* spi_new_device() currently does not use bus_num but better set it */
+ board_info.bus_num = (u16)ctrl->bus_num;
+
+ /* Add chip selects to controller */
+ for (csi = 0; csi < ctrl->num_chipselect; csi++) {
+ dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
+ board_info.chip_select = csi;
+ /* TODO: Discuss setting of board_info.mode */
+ if (!(priv->mode_func_supported & VIRTIO_SPI_CS_HIGH))
+ board_info.mode = SPI_MODE_0;
+ else
+ board_info.mode = SPI_MODE_0 | SPI_CS_HIGH;
+ if (!spi_new_device(ctrl, &board_info)) {
+ dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
+ spi_unregister_controller(ctrl);
+ err = -ENODEV;
+ goto err_return;
+ }
+ }
+
+ return 0;
+
+err_return:
+ vdev->config->del_vqs(vdev);
+ return err;
+}
+
+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 */
+ spi_unregister_controller(ctrl);
+ virtio_spi_del_vq(vdev);
+}
+
+static int virtio_spi_freeze(struct virtio_device *vdev)
+{
+ struct device *dev = &vdev->dev;
+ struct spi_controller *ctrl = dev_get_drvdata(dev);
+ int ret;
+
+ /* Stop the queue running */
+ 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 virtio_device *vdev)
+{
+ struct device *dev = &vdev->dev;
+ struct spi_controller *ctrl = dev_get_drvdata(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 },
+ { 0 },
+};
+
+static struct virtio_driver virtio_spi_driver = {
+ .driver.name = KBUILD_MODNAME,
+ .driver.owner = THIS_MODULE,
+ .id_table = virtio_spi_id_table,
+ .validate = virtio_spi_validate,
+ .probe = virtio_spi_probe,
+ .remove = virtio_spi_remove,
+ .freeze = pm_sleep_ptr(virtio_spi_freeze),
+ .restore = pm_sleep_ptr(virtio_spi_restore),
+};
+
+module_virtio_driver(virtio_spi_driver);
+MODULE_DEVICE_TABLE(virtio, virtio_spi_id_table);
+
+MODULE_AUTHOR("OpenSynergy GmbH");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Virtio SPI bus driver");
--
2.25.1
On 28-02-24, 15:27, Harald Mommer wrote:
> +static int virtio_spi_probe(struct virtio_device *vdev)
> +{
> + struct device_node *np = vdev->dev.parent->of_node;
> + struct virtio_spi_priv *priv;
> + struct spi_controller *ctrl;
> + int err;
> + u32 bus_num;
> + u16 csi;
> +
> + ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
> + if (!ctrl) {
> + dev_err(&vdev->dev, "Kernel memory exhausted in %s()\n",
> + __func__);
The print can be dropped I guess.
> + return -ENOMEM;
> + }
> +
> + priv = spi_controller_get_devdata(ctrl);
> + priv->vdev = vdev;
> + vdev->priv = priv;
> + dev_set_drvdata(&vdev->dev, ctrl);
> +
> + init_completion(&priv->spi_req.completion);
> +
> + err = of_property_read_u32(np, "spi,bus-num", &bus_num);
> + if (!err && bus_num <= S16_MAX)
> + ctrl->bus_num = (s16)bus_num;
> +
> + virtio_spi_read_config(vdev);
> +
> + /* Method to do a single SPI transfer */
The comments for obvious statements are normally not required. There
are a couple of them here.
> + ctrl->transfer_one = virtio_spi_transfer_one;
> +
> + /* Initialize virtqueues */
> + err = virtio_spi_find_vqs(priv);
> + if (err) {
> + dev_err(&vdev->dev, "Cannot setup virtqueues\n");
Maybe "Failed to" instead of "Cannot" ?
> + return err;
> + }
> +
> + err = spi_register_controller(ctrl);
> + if (err) {
> + dev_err(&vdev->dev, "Cannot register controller\n");
> + goto err_return;
> + }
> +
> + board_info.max_speed_hz = priv->max_freq_hz;
> + /* spi_new_device() currently does not use bus_num but better set it */
> + board_info.bus_num = (u16)ctrl->bus_num;
> +
> + /* Add chip selects to controller */
> + for (csi = 0; csi < ctrl->num_chipselect; csi++) {
> + dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
> + board_info.chip_select = csi;
Maybe a blank line here.
> + /* TODO: Discuss setting of board_info.mode */
> + if (!(priv->mode_func_supported & VIRTIO_SPI_CS_HIGH))
> + board_info.mode = SPI_MODE_0;
> + else
> + board_info.mode = SPI_MODE_0 | SPI_CS_HIGH;
and here to improve readability.
> + if (!spi_new_device(ctrl, &board_info)) {
> + dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
> + spi_unregister_controller(ctrl);
> + err = -ENODEV;
> + goto err_return;
> + }
> + }
> +
> + return 0;
> +
> +err_return:
> + vdev->config->del_vqs(vdev);
> + return err;
> +}
> +
> +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 */
Not sure if this comment is required or not, since we don't add
similar ones while registering.
> + spi_unregister_controller(ctrl);
> + virtio_spi_del_vq(vdev);
> +}
Other than that.
Reviewed-by: Viresh Kumar <[email protected]>
--
viresh
On 29.02.24 09:22, Viresh Kumar wrote:
> On 28-02-24, 15:27, Harald Mommer wrote:
>> +static int virtio_spi_probe(struct virtio_device *vdev)
>> +{
>> + struct device_node *np = vdev->dev.parent->of_node;
>> + struct virtio_spi_priv *priv;
>> + struct spi_controller *ctrl;
>> + int err;
>> + u32 bus_num;
>> + u16 csi;
>> +
>> + ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
>> + if (!ctrl) {
>> + dev_err(&vdev->dev, "Kernel memory exhausted in %s()\n",
>> + __func__);
> The print can be dropped I guess.
Looked around: It is habit not to do here dev_err() here so the
dev_err() is to be removed.
For curiosity I searched through the kernel whether the kernel already
leaves a trace the way down the memory allocation but somehow I landed
in the forest. Not important.
>> + return -ENOMEM;
>> + }
>> +
>> + priv = spi_controller_get_devdata(ctrl);
>> + priv->vdev = vdev;
>> + vdev->priv = priv;
>> + dev_set_drvdata(&vdev->dev, ctrl);
>> +
>> + init_completion(&priv->spi_req.completion);
>> +
>> + err = of_property_read_u32(np, "spi,bus-num", &bus_num);
>> + if (!err && bus_num <= S16_MAX)
>> + ctrl->bus_num = (s16)bus_num;
>> +
>> + virtio_spi_read_config(vdev);
>> +
>> + /* Method to do a single SPI transfer */
> The comments for obvious statements are normally not required. There
> are a couple of them here.
Removing now a few really obvious comments. This "code speaks for
itself" sitting in front of a mostly uncommented code desert is not mine
so I'm careful with this.
>
>> + ctrl->transfer_one = virtio_spi_transfer_one;
>> +
>> + /* Initialize virtqueues */
>> + err = virtio_spi_find_vqs(priv);
>> + if (err) {
>> + dev_err(&vdev->dev, "Cannot setup virtqueues\n");
> Maybe "Failed to" instead of "Cannot" ?
I grepped through the kernel for '"Cannot ' which brings all the
messages in the kernel which start with "Cannot ": 4123 matches (case
insensitive).
Did the same with `"Failed to ' which brings all the messages in the
kernel which start with "Failed to ": 34746 matches (case insensitive).
Majority uses "Failed to " but "Cannot " is also used. Both wordings
seem to be acceptable.
So this is no finding and I keep the code as it is. Otherwise we must
look again not only here but also in all other messages especially in
virtio_spi_set_delays() reworking more (for no good reason).
My wording in virtio_spi_restore() is more unusual, "problem ". Only 111
matches.
It's not wrong, it's not broken, nobody complained now, we will see.
>> + return err;
>> + }
>> +
>> + err = spi_register_controller(ctrl);
>> + if (err) {
>> + dev_err(&vdev->dev, "Cannot register controller\n");
>> + goto err_return;
>> + }
>> +
>> + board_info.max_speed_hz = priv->max_freq_hz;
>> + /* spi_new_device() currently does not use bus_num but better set it */
>> + board_info.bus_num = (u16)ctrl->bus_num;
>> +
>> + /* Add chip selects to controller */
>> + for (csi = 0; csi < ctrl->num_chipselect; csi++) {
>> + dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
>> + board_info.chip_select = csi;
> Maybe a blank line here.
>
>> + /* TODO: Discuss setting of board_info.mode */
>> + if (!(priv->mode_func_supported & VIRTIO_SPI_CS_HIGH))
>> + board_info.mode = SPI_MODE_0;
>> + else
>> + board_info.mode = SPI_MODE_0 | SPI_CS_HIGH;
> and here to improve readability.
Yes, code desert without blank lines here.
And while we are here and nobody wanted to discuss: The TODO comment is
to be removed now. In the meantime I'm convinced the code below is what
really should be done here.
>> + if (!spi_new_device(ctrl, &board_info)) {
>> + dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
>> + spi_unregister_controller(ctrl);
>> + err = -ENODEV;
>> + goto err_return;
>> + }
>> + }
>> +
>> + return 0;
>> +
>> +err_return:
>> + vdev->config->del_vqs(vdev);
>> + return err;
>> +}
>> +
>> +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 */
> Not sure if this comment is required or not, since we don't add
> similar ones while registering.
I got once from you a review comment about the de-initialization order.
Now the order is as it should be. This comment is needed to remind me
(and others) of the desired de-initialization order in case someone has
the idea to replace spi_register_controller() by
devm_spi_register_controller() in virtio_spi_probe(). By such a change
also the spi_unregister_controller() here would be removed and this
would change the de-initialization back to the undesired order.
Now there is the comment above the line being removed asking "Have you
thought about this?".
I was already reworking to devm_spi_register_controller() when I
realized late the undesired side effect and rolled back. Better we keep
this comment.
>> + spi_unregister_controller(ctrl);
>> + virtio_spi_del_vq(vdev);
>> +}
> Other than that.
>
> Reviewed-by: Viresh Kumar <[email protected]>
>
No real code changes. Some comments to be removed, some blank lines to
be added, nothing urgent even in case the driver is integrated now
locally by someone for some need. No re-test will be needed. Let's see
what comes in addition. This is for next week, by than also the
maintenance of the virtio mailing lists will have been done.