Re: [PATCH v2 3/3] virtio-spi: Add virtio SPI driver.

From: Haixu Cui
Date: Tue Mar 26 2024 - 05:38:35 EST


Hi all,

On 3/20/2024 11:12 AM, Viresh Kumar wrote:
Hi Harald,

On 19-03-24, 16:05, Harald Mommer wrote:
On 18.03.24 10:39, Haixu Cui wrote:
On 3/4/2024 11:43 PM, 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)
+        return -ENOMEM;
+
+    priv = spi_controller_get_devdata(ctrl);
+    priv->vdev = vdev;
+    vdev->priv = priv;
+    dev_set_drvdata(&vdev->dev, ctrl);

    ctrl->dev.of_node is not set, so the child node cannot be parsed. I
would say, it's necessary to support the virtio spi device node in the
format:


What you most probably want to have here is

  ctrl->dev.of_node = np;

Looking at how i2c-virtio does it, it should be tied to the device itself
instead of its parent:

Yes, it is
ctrl->dev.of_node = vdev->dev.of_node;


ctrl->dev.of_node = vdev->dev.of_node;

    virtio-spi@4b013000 {
        compatible = "virtio,mmio";
        reg = <0x4b013000 0x1000>;
        interrupts = <0 497 4>;

        spi {
            compatible = "virtio,device2d";
            #address-cells = <1>;
            #size-cells = <0>;

            spidev {
                compatible = "xxx";
                reg = <0>;
                spi-max-frequency = <100000>;
            };
        };
    };

Right, some work was done in the past to standardize these compatibles:

$ git log -p --stat --reverse 0d8c9e7d4b40..694a1116b405

Here I would like the inner layer "spidev", to match then probe the spidev driver, the "reg" is the chip select index. "spi-max-frequency" is not necessary, while It doesn't matter.

You can also customize the inner layer to match your own driver.

In my test, I set the cs max number as 1, and in device tree node inner layer, reg as 0. So certainly spidev driver probed and spidev0.0 is added successfully.

But then the driver proceed to the following code, chip select index 0 device is created again, the driver fail with log: "chipselect 0 already in use".

for (csi = 0; csi < ctrl->num_chipselect; csi++) {
dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
board_info.chip_select = csi;

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


I hope I made myself clear. Thank you, Harald and Kumar. Best regards.