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

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

On Thu, Oct 22, 2020 at 03:45:16PM +0800, Jie Deng wrote:
> On 2020/10/21 20:08, Michael S. Tsirkin wrote:
> 
> > On Tue, Sep 22, 2020 at 10:49:02AM +0800, Jie Deng wrote:
> > 
> > +
> > +The first bit of \field{flags} indicates whether it is a read or write request.
> > +It mesns a read request if the first bit of \field{flags} is set, otherwise
> > 
> > means
> 
> I will fix this typo. Thank you.
> 
> > > +it is a write request. The rest bits of \field{flags} are reserved.
> > Do we really need this flg? write request is just a big write followed by 1
> > byte read, read request is write read 1 byte read.
> The "addr", "flags", "len" and "buf" are all from Linux kernel structure
> "struct i2c_msg".
> This design may help the backend driver to extract "struct i2c_msg" from
> "struct virtio_i2c_req" easily.
> 
> The "flags" has many bits been defined in I2C. It is no mere an
> identification for
> read/write. Although, currently, we only used the first bit, we may use
> other bits
> for feature extension in the future. So I think it's necessary to keep it.
> 
> Thanks.

It's ok to reserve 2 bytes for now.




> 
> > 
> > 
> > > +The \field{len} of the request indicates the length of the I2C message.
> > > +
> > > +The \field{buf} of the request contains the I2C message. If the first bit of
> > > +\field{flags} is '1', the \field{buf} is written by the device and it contains
> > > +the message read from the device. If the first bit of \field{flags} is '0', the
> > > +\field{buf} is written by the driver and it contains the message written to the
> > > +the device.
> > > +
> > > +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}
> > > +
> > > +\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 message.
> > > +
> > > +A driver MUST place the I2C message into \field{buf} if the first bit of
> > > +\field{flags} is '0'.
> > > +
> > > +A driver MUST NOT use the message if the final \field{status}
> > 
> > What does "use the message" mean? What message?
> I mean  the "addr", "flags" , "len " and "buf " in "virtio_i2c_req" here.
> I will describe it more precisely in v2.
> 
> Thanks.


Right. For example it is very unclear what does the
buffer map to.  a single i2c_msg? then do we not need STOP
etc etc ... ?


> > > return from the
> > > +device is VIRTIO_I2C_MSG_ERR.
> > returned?
> 
> I will fix this typo. Thank you.
> 
> 
> > > +
> > > +\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 the message into \field{buf} if the first bit of \field{flags}
> > > +is '1'.
> > 
> > again, what the message?
> > 
> Here is the I2C message. I will describe it more precisely in v2.
> 
> Thanks.