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

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

Hi Michael and Paolo,

Have you reached an agreement on the design of "virtio_i2c_req" ?

Personally, I think the design in v4 can already handle the multi-segment transactions between frontend and backend.

Fore example, the frontend may kick the sequence "write read read ..." to the backend at a time.

The segments can be aggregated into "i2c_msg list" and sent to the hardware at a time by the backend.

    The host native drivers will finally send these segments using the
    methods they support.

Does this make sense ?

Thanks.

On 2020/11/13 10:39, Jie Deng wrote:

>       
>       On 2020/11/12 16:54, Michael S. Tsirkin wrote:
>       
> > On Thu, Nov 12, 2020 at 09:51:17AM +0800,
> >         Jie Deng wrote:
> >         
> > > On 2020/11/10 18:59, Paolo Bonzini
> > >           wrote:
> > >           
> > > > On 10/11/20 11:33, Michael S. Tsirkin
> > > >             wrote:
> > > >             
> > > > > > Sorry I didn't reply last week,
> > > > > >                 but I really would prefer if virtio-i2c
> > > > > >                 
> > > > > >                 supported write-read transactions.
> > > > > >                 
> > > > > >               
> > > > >               And I think an easier way is to allow multi-segment
> > > > >               transactions
> > > > >               
> > > > >               probably using the stop flag.
> > > > >               
> > > > >             
> > > >             You cannot always be sure that the host would support them
> > > >             (with the
> > > >             
> > > >             exception of write+read transactions, which are pretty much
> > > >             universal),
> > > >             
> > > >             so you would also need to describe what the host features
> > > >             are.
> > > >             
> > > >             
> > > >             Paolo
> > > >             
> > > >             
> > > >           
> > >           Then I will send v5 to support write+read transactions using
> > >           Paolo's
> > >           
> > >           proposal.
> > >           
> > >           I think we may have a simple version merged first and then
> > >           improve it
> > >           
> > >           according
> > >           
> > >           to the actual needs.
> > >           
> > >           
> > >           Thank you.
> > >           
> > >         
> >         I frankly don't get it, you put read/write flags there even
> >         though they
> >         
> >         are not needed, saying it's intended to mirror Linux, then don't
> >         want to
> >         
> >         put STOP there even though this is exactly what Linux has ...
> >         
> >       
>       
>       Personally, I intended to mirror the Linux "i2c_msg" to have
>       following interface.
>       
>       
>       struct virtio_i2c_req {
>       
>       ÂÂÂÂÂÂÂ le16 addr;
>       
>       ÂÂÂÂÂÂÂ le16 flags;
>       
>       ÂÂÂÂÂÂÂ le16 len;
>       
>       ÂÂÂÂÂÂÂ u8 buf[];
>       
>       ÂÂÂÂÂÂÂ u8 status;
>       
>       
>       };
>       
>       
>       I intended to implement the standard rules for I2C transactions
>       
>       (no I2C_FUNC_PROTOCOL_MANGLING) for the first step. So in the
>       
>       v1~v3 only the "I2C_M_RD" was used and other bits of the flags
>       were reserved.
>       
>       (I2C_M_STOP requires I2C_FUNC_PROTOCOL_MANGLING).
>       
>       
>       For me, I intend to mirror the "i2c_msg" from the Linux
>       completely.
>       
>       
>       I learned about Michael also intends to keep above interface but
>       want to
>       
>       remove some unnecessary bit (e.g. I2C_M_RD) from the flags
>       (Correct me if not right).
>       
>       
>       Paolo prefers following interface without flags.
>       
>       
>       struct virtio_i2c_req {
>       
>       ÂÂÂÂle16 addr;
>       
>       ÂÂÂÂle16 written;
>       
>       ÂÂÂÂle16 read;
>       
>       ÂÂÂÂu8 bufwrite[];
>       
>       ÂÂÂÂu8 status;
>       
>       ÂÂÂÂu8 bufread[];
>       
>       };
>       
>       
>       So Michael and Paolo, I hope we can agree on the interface first.
>       
>       
>       Any suggestions ?
>       
>       
>       Thanks.
>       
>       
>