[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [RFC PATCH v1 3/3] SPI: Add virtio SPI driver (V4 draft specification).
On 06.11.23 14:48, Mark Brown wrote:
Reminder for me: Need still to address this, but this is not code I'm currently working on, so comes later.On Fri, Oct 27, 2023 at 06:10:16PM +0200, Harald Mommer wrote:+config SPI_VIRTIO + tristate "Virtio SPI SPI Controller" + depends on 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. +This advice is going to be inappropriate for the majortiy of guests.
Not needed any more, will be removed. Maybe I should remove this ____cacheline_aligned. It's only there because I looked too deeply into struct virtio_i2c_req, not because I think this ____cacheline_aligned is decisive here.+ // clang-format off + 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; + // clang-format onRemove this clang-format stuff.
It was a reminder for me from where I got some inspiration and to reveal that not everything was invented by me. Served it's purpose, all such comment references to foreign code is being removed.+static struct spi_board_info board_info = { + .modalias = "spi-virtio", + .max_speed_hz = 125000000, /* Arbitrary very high limit */ + .bus_num = 0, /* Patched later during initialization */ + .chip_select = 0, /* Patched later during initialization */ + .mode = SPI_MODE_0, +}; +/* Compare with file i2c_virtio.c structure virtio_i2c_msg_done */In what way is one supposed to compare with the i2c driver? What happens if the I2C driver changes? It would be better to ensure that the code can be read and understood as a standalone thing.
+ /* Fill struct spi_transfer_head */ + th->slave_id = spi->chip_select;If the spec just copied the Linux terminology it'd have few issues :(
Spec changed this in the meantime so I can do also.
+ th->bits_per_word = spi->bits_per_word; + th->cs_change = xfer->cs_change;The virtio spec for cs_change is *not* what the Linux cs_change field does, this will not do the right thing.
This is a hard one. Linux says (spi.h): "@cs_change: affects chipselect after this transfer completes ... All SPI transfers start with the relevant chipselect active. Normally it stays selected until after the last transfer in a message. Drivers can affect the chipselect signal using cs_change." V8 of the draft spec says:"cs_change indicates whether to deselect device before starting the next SPI transfer, 0 means chipselect
keep asserted and 1 means chipselect deasserted then asserted again." What I understand here (unfortunately in both texts) is - at the start of a transfer CS is made active - it is kept active during the transfer - when cs_change is 0 after the transfer CS is kept active (not changed) - when cs_change is 1 after the transfer CS is de-assertedSo if cs_change is 0 keep CS asserted after transfer, otherwise de-assert after transfer.
I fear there is some subtle thing I haven't gotten yet but I don't understand / see it.What is it?
+ th->tx_nbits = xfer->tx_nbits; + th->rx_nbits = xfer->rx_nbits; + th->reserved[0] = 0; + th->reserved[1] = 0; + th->reserved[2] = 0; + +#if (VIRTIO_SPI_CPHA != SPI_CPHA) +#error VIRTIO_SPI_CPHA != SPI_CPHA +#endifBUILD_BUG_ON()
Thanks for this one, didn't know yet.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]