Re: [PATCH v3] virtio-spi: add the device specification

[Harald Mommer <harald.mommer@opensynergy.com>]

Hello,

I'm currently integrating your V3 update in our proprietary virtio SPI 
device and in our virtio SPI Linux driver.

The virtio SPI Linux driver is currently not yet in a shape to be shown 
and the paperwork with the company lawyer is not yet done.

Some short first remarks, I'll have to look again into the updated V3 
specification next week.

On 01.09.23 05:29, Haixu Cui wrote:
> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_head {
> +        u8 slave_id;
> +        u8 bits_per_word;
> +        u8 cs_change;
> +        u8 tx_nbits;
> +        u8 rx_nbits;
The "u8 reserved[3];" which was previously here must remain for 
alignment reasons. The "le32 mode" needs to be cleanly and visibly 
aligned to a 4 byte boundary.
> +        le32 mode;
> +        le32 freq;

Any hint what to do with the next 3 values in Linux? Currently on the 
device side I've a check that all values are zero. If this is not the case:

if ([any of those values is != zero]) {
ÂÂ ÂÂÂÂ pr_warn_once("word_delay_ns, cs_setup_ns or cs_delay_hold_ns 
not supported\n");
ÂÂ ÂÂÂÂ /* Pray that the SPI device can still cope with the timing */
}

No idea how to cope with those values using /dev/spidevXXXX.0 as backend 
device. Looked into some data sheets: If being not on Linux but on some 
small embedded controllers I would probably also have difficulties in 
some cases.

I guess printing a warning and hoping that the SPI device still can cope 
with the timing is the only thing which can be done in some 
environments. However having those values in the message is probably a 
good idea, this is not the point.

> +        le32 word_delay_ns;
> +        le32 cs_setup_ns;
> +        le32 cs_delay_hold_ns;
> +        le32 cs_change_delay_inactive_ns;
> +};
> +\end{lstlisting}
> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_result {
> +        u8 result;
> +};
> +\end{lstlisting}
> +
> +\begin{lstlisting}
> +struct virtio_spi_transfer_req {
> +        struct virtio_spi_transfer_head head;
> +        u8 tx_buf[];
> +        u8 rx_buf[];
> +        struct virtio_spi_transfer_result result;
Exactly the order which is needed. Having the result as last element 
there are no alignment problems with word sizes > 8 bits. My former 
proposal to move the result field to a different place was simply wrong. 
Perfect now.
> +};
> +\end{lstlisting}