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

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

[-- Attachment #1: Type: text/plain, Size: 4936 bytes --]

On 2020/10/28 19:27, Michael S. Tsirkin wrote:

> 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 flexibly 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 ...
So do you tend to delete the flags from virtio_i2c_req ?
>>
>>>> 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.
I agree.
>> No need to tag the start/stop segment.
>>
>> Thanks.
>
>
> i2c_msg has flags to signal start/stop of multi-segment transactions.
>
Which flags ?

This adapter only declares following I2C functionality for this moment.

I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

So I think we only need the flag "I2C_M_RD" for transaction.

And if you think "I2C_M_RD" is redundant, I can remove the two byte 
flags from

virtio_i2c_req.

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.
> where would it return it?
The requests are being handled in order, so we can return the number of
the last segment being successfully processed.

Thanks.

>>>> 2.7.4

[-- Attachment #2: Type: text/html, Size: 8901 bytes --]