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

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.