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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c0647197-5c87-4154-8a50-92e7dd2297ec@opensynergy.com>
Date: Tue, 26 Mar 2024 12:19:06 +0100
From: Harald Mommer <harald.mommer@...nsynergy.com>
To: Haixu Cui <quic_haixcui@...cinc.com>,
 Viresh Kumar <viresh.kumar@...aro.org>
Cc: Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org,
 linux-kernel@...r.kernel.org, quic_ztu@...cinc.com,
 Matti Moell <Matti.Moell@...nsynergy.com>,
 Mikhail Golubev <Mikhail.Golubev@...nsynergy.com>
Subject: Re: [PATCH v2 3/3] virtio-spi: Add virtio SPI driver.

Hello,

On 26.03.24 10:26, Haixu Cui wrote:
>
>> 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;

got it when looking into Viresh's code changes.

>>
>> 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".

The following lines

     board_info.max_speed_hz = priv->max_freq_hz;
     board_info.bus_num = (u16)ctrl->bus_num;

     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;

are moved before the sequence

     err = spi_register_controller(ctrl);
     if (err) {
         dev_err(&vdev->dev, "Cannot register controller\n");
         goto err_return;
     }

     if (vdev->dev.of_node) { // <=== This block is new
         dev_dbg(&vdev->dev, "Final setup triggered by DT child node\n");
         return 0;
     }

>
>         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)) // 
> <=== This if else is moved up. It is csi invariant anyway.
>                 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;
>             }
>         }
>
>

This means when there is no device tree entry everything behaves as 
before (for loop executed) and if there is a device tree entry the chip 
select will be setup already by spi_register_controller() and the for 
loop need not and will not be executed not annoying anyone with 
"chipselect 0 already in use".

I was undecided whether to keep Viresh Kumar's "reviewed by" in the 
commit message because I'm unsure whether this is to be considered as a 
substantial change. Changes are small but program logic is changed (vs. 
change of comment, removal of trace, change of trace wording) so I think 
it may be considered as a substantial change and therefore I'll remove 
the "reviewed-by" now.