OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]


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


On Thu, Dec 17, 2020 at 03:08:07PM +0800, Jie Deng wrote:
> 
> On 2020/12/16 23:52, Stefan Hajnoczi wrote:
> > 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?
> This virtio-i2c is a master device. The slave devices attached to it can be
> configured
> by the backend in the host. These slave devices can be under different host
> I2C master devices.
> The guest will see this virtio-i2c master device and its slave devices.

There could still be multiple virtio-i2c devices. I guess the guest can
use the device's bus address (e.g. virtio-pci address, virtio-mmio base
address, etc) to differentiate (e.g. guest udev rules).

No changes to the spec are needed here.

> > 
> > > +\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.
> 
> Both are OK to me. I used to use "packed" but Michael said there was no need
> to pack it.
> 
> https://lkml.org/lkml/2020/9/3/339
> 
> So you prefer it to be clear ?

Packing wasn't necessary because all fields were aligned in that patch:

  struct virtio_i2c_hdr {
    __virtio16 addr;
    __virtio16 flags;
    __virtio16 len;
  } __packed;

The struct virtio_i2c_req definition in the virtio spec patch is not
naturally aligned though. There are a few options: the fields can be
reordered to achieve that, explicit padding fields can be introduced, or
it can be explicitly marked packed.

That way the memory layout will be clear for anyone wishing to implement
the spec.

> > > +        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?
> 
> This field maps to the kernel struct "i2c_msg.addr".
> 
> struct virtio_i2c_req  *vmsg;
> struct i2c_msg *msg;
> 
> vmsg->addr  = cpu_to_le16(msg->addr);
> 
> The field in the request can be seen as host byte order
> while the link can be seen as the I2C network byte order.
> The host driver may take care thisconversion.

Please include a description in the spec of how to represent 7-bit and
10-bit I2C addresses as defined by the I2C spec in the le16 addr field.

The other option would be to refer to Linux as you did in your reply,
but nothing else in this device seems Linux-specific so let's avoid the
reference. That way the vitio-i2c spec will be more general and easy to
implement by people who are unfamiliar with Linux.

Attachment: signature.asc
Description: PGP signature



[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]