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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Oct 28, 2020 at 03:54:39PM +0800, Jie Deng wrote:
> 
> 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.


I think it's better to have the device take it from the virtio descriptor though.
Duplicating info just causes bugs ...

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

From spec perspective we don't really care. We also don't
assume driver and device are using linux.

> No need to tag the start/stop segment.
> 
> Thanks.



i2c_msg has flags to signal start/stop of multi-segment transactions.


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

where would it return it?

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