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

[Jie Deng <jie.deng@intel.com>]

On 2020/10/27 20:20, Michael S. Tsirkin wrote:
> On Tue, Oct 27, 2020 at 02:00:24PM +0800, Jie Deng wrote:
> > virtio-i2c is a virtual I2C adapter device. It provides a way
> > to ïexibly communicate with the I2C slave devices from the guest.
> >
> > This patch adds the specification for this device.
> >
> > Signed-off-by: Jie Deng<jie.deng@intel.com>
> > ---
> >   conformance.tex | 28 ++++++++++++++---
> >   content.tex     |  1 +
> >   virtio-i2c.tex  | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 119 insertions(+), 4 deletions(-)
> >   create mode 100644 virtio-i2c.tex
> >
> > +The driver queues requests to the virtqueue, and they are used by the
> > +device. The request is the representation of one segment of an I2C
> > +transaction. Each request is of form:
> > +
> > +\begin{lstlisting}
> > +struct virtio_i2c_req {
> > +        le16 addr;
> > +        le16 flags;
> > +        le16 len;
> > +        u8 buf[];
> > +        u8 status;
> > +};
> > +\end{lstlisting}
> > +
> > +The \field{addr} is the address of the I2C slave device.
> > +
> > +The first bit of \field{flags} indicates whether it is a read or write request.
> > +It means a read request if the first bit of \field{flags} is set, otherwise
> > +it is a write request. The rest bits of \field{flags} are reserved.
> > +
> > +The \field{len} is the number of data bytes in the \field{buf} being read from or
> > +written to the I2C slave address.
> > +
> > +The \field{buf} of the request contains one segment of an I2C transaction.
> > +If the first bit of \field{flags} is '1', the \field{buf} is written by the
> > +device and it contains one segment of an I2C transaction being read from the
> > +device.
> Let's give a name to the flag then? READ I guess?
> I guess this means it's an exact reverse of the write-only/read-only
> flag in the descriptor?
> I still think it's both a potential source of errors and a waste
> to have this bit in the device struct when we have a generic one.
>
> How about adding some motivation to explain why it's done
> like this?
Hmm... It seems the description here is a bit unsatisfactory. I don't 
mean to use this flag
to play the role of that flag of the descriptor. I just want to 
encapsulate all the i2c_msg fields
into the request for I2C use. The flag in the descriptor is defined from 
virtio perspective
while the flag in this request is defined from I2C perspective.

It may be necessary to reverse the cause and effect:

It seems better to say "If it is a write request (write descriptor), 
then the first bit of the flag in the request should be set to 0."
than "the first bit of the flag in the request is 0, then it is a write 
request (write descriptor)".

I will try to add more description to make it looks better.

Thanks.


> > If the first bit of \field{flags} is '0', the \field{buf} is written
> > +by the driver and it contains one segment of an I2C transaction being written
> > +to the the device.
> one the?
Right. Thanks for your careful review.


> more detail on how are multi-segment transactions formed?
> don't you need flags to start/stop?
Currently, it is designed to simply transparently transmit
the "i2c_msg messages" from the frontend OS kernel to the backend.
No need to tag the start/stop segment.

Thanks.

> > +The final \field{status} byte is written by the device: either VIRTIO_I2C_MSG_OK
> > +for success or VIRTIO_I2C_MSG_ERR for error.
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_I2C_MSG_OK     0
> > +#define VIRTIO_I2C_MSG_ERR    1
> > +\end{lstlisting}
> what if one segment in a transaction fails?

The driver shall return the number of segments successfully processed.

Thanks.

> > +\drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
> > +
> > +A driver MUST set \field{addr}, \field{flags} and \field{len} before sending
> > +the request.
> > +
> > +A driver MUST place one segment of an I2C transaction into \field{buf} if the
> > +first bit of \field{flags} is '0'.
> > +
> > +A driver MUST NOT use \field{addr}, \field{flags}, \field{len} and \field{buf}
> > +if the final \field{status} returned from the device is VIRTIO_I2C_MSG_ERR.
> > +
> > +A driver MUST queue the requests in order if multiple requests are going to
> > +be sent at a time.
> > +
> > +\devicenormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
> > +
> > +A device MUST NOT change the value of \field{addr}, \field{flags} and \field{len}.
> > +
> > +A device MUST place one segment of an I2C transaction into \field{buf} if the first
> > +bit of \field{flags} is '1'.
> > +
> > +A device MUST guarantee the requests being handled in order if multiple requests
> > +are received.
> > --
> > 2.7.4