Re: [virtio-comment] [PATCH v5] virtio-i2c: add the device specification

[Stefan Hajnoczi <stefanha@redhat.com>]

On Wed, Nov 25, 2020 at 01:55:18PM +0800, Jie Deng wrote:
> diff --git a/virtio-i2c.tex b/virtio-i2c.tex
> new file mode 100644
> index 0000000..fdb0050
> --- /dev/null
> +++ b/virtio-i2c.tex
> @@ -0,0 +1,139 @@
> +\section{I2C Adapter Device}\label{sec:Device Types / I2C Adapter Device}
> +
> +virtio-i2c is a virtual I2C adapter device. It provides a way to flexibly
> +organize and use the host I2C slave devices from the guest. By attaching
> +the host ACPI I2C slave nodes to the virtual I2C adapter device, the guest can
> +communicate with them without changing or adding extra drivers for these
> +slave I2C devices. 

Is there a way to identify I2C busses if more than one virtio-i2c device
is present? For example, imagine a host with 2 I2C busses. How does the
guest know which virtio-i2c device connects to which host bus?

> +\begin{lstlisting}
> +struct virtio_i2c_req {
> +        le16 addr;
> +        le32 flags;

What is the memory layout?

1. 0x0 addr, 0x2 flags

or

2. 0x0 addr, 0x4 flags

This is unclear to me. I don't see a general statement in the spec about
struct field alignment/padding and no details in this new spec change.

> +        le16 written;
> +        le16 read;
> +        u8 write_buf[];
> +        u8 read_buf[];
> +        u8 status;
> +};
> +\end{lstlisting}
> +
> +The \field{addr} of the request is the address of the I2C slave device.

https://en.wikipedia.org/wiki/I2c#Addressing_structure suggests there
are at least 7-bit and 10-bit addressing schemes in I2C. How does this
map to the little-endian 16-bit addr field?

> +The \field{flags} of the request is currently reserved as zero for future
> +feature extensibility.
> +
> +The \field{written} of the request is the number of data bytes in the \field{write_buf}
> +being written to the I2C slave address.

This field seems redundant since the device can determine the size of
write_buf implicitly from the total out buffer size. virtio-blk takes
this approach.

> +The \field{read} of the request is the number of data bytes in the \field{read_buf}
> +being read from the I2C slave address.

Same here. virtio-blk doesn't use an explicit read size field because
the device can determine it.

Attachment: signature.asc
Description: PGP signature