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

[Haixu Cui]

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.