Re: [RFC PATCH v1 3/3] SPI: Add virtio SPI driver (V4 draft specification).

[Mark Brown]

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.

> +	// 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 on

Remove this clang-format stuff.

> +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 :(

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

> +	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
> +#endif

BUILD_BUG_ON()

Attachment: signature.asc
Description: PGP signature