Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2021/3/1 19:54, Viresh Kumar wrote:
On 01-03-21, 14:41, Jie Deng wrote:
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @write_buf: contains one I2C segment being written to the device
+ * @read_buf: contains one I2C segment being read from the device
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+	struct virtio_i2c_out_hdr out_hdr;
+	u8 *write_buf;
+	u8 *read_buf;
+	struct virtio_i2c_in_hdr in_hdr;
+};
I am not able to appreciate the use of write/read bufs here as we
aren't trying to read/write data in the same transaction. Why do we
have two bufs here as well as in specs ?

What about this on top of your patch ?

---
  drivers/i2c/busses/i2c-virtio.c | 31 +++++++++++--------------------
  include/uapi/linux/virtio_i2c.h |  3 +--
  2 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
index 8c8bc9545418..e71ab1d2c83f 100644
--- a/drivers/i2c/busses/i2c-virtio.c
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -67,14 +67,13 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
  		if (!buf)
  			break;
+ reqs[i].buf = buf;
+		sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
+
  		if (msgs[i].flags & I2C_M_RD) {
-			reqs[i].read_buf = buf;
-			sg_init_one(&msg_buf, reqs[i].read_buf, msgs[i].len);
  			sgs[outcnt + incnt++] = &msg_buf;
  		} else {
-			reqs[i].write_buf = buf;
-			memcpy(reqs[i].write_buf, msgs[i].buf, msgs[i].len);
-			sg_init_one(&msg_buf, reqs[i].write_buf, msgs[i].len);
+			memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);
  			sgs[outcnt++] = &msg_buf;
  		}
@@ -84,13 +83,8 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
  		err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
  		if (err < 0) {
  			pr_err("failed to add msg[%d] to virtqueue.\n", i);
-			if (msgs[i].flags & I2C_M_RD) {
-				kfree(reqs[i].read_buf);
-				reqs[i].read_buf = NULL;
-			} else {
-				kfree(reqs[i].write_buf);
-				reqs[i].write_buf = NULL;
-			}
+			kfree(reqs[i].buf);
+			reqs[i].buf = NULL;
  			break;
  		}
  	}
@@ -118,14 +112,11 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
  			break;
  		}
- if (msgs[i].flags & I2C_M_RD) {
-			memcpy(msgs[i].buf, req->read_buf, msgs[i].len);
-			kfree(req->read_buf);
-			req->read_buf = NULL;
-		} else {
-			kfree(req->write_buf);
-			req->write_buf = NULL;
-		}
+		if (msgs[i].flags & I2C_M_RD)
+			memcpy(msgs[i].buf, req->buf, msgs[i].len);
+
+		kfree(req->buf);
+		req->buf = NULL;
  	}
return i;
diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
index 92febf0c527e..61f0086ac75b 100644
--- a/include/uapi/linux/virtio_i2c.h
+++ b/include/uapi/linux/virtio_i2c.h
@@ -48,8 +48,7 @@ struct virtio_i2c_in_hdr {
   */
  struct virtio_i2c_req {
  	struct virtio_i2c_out_hdr out_hdr;
-	u8 *write_buf;
-	u8 *read_buf;
+	u8 *buf;
  	struct virtio_i2c_in_hdr in_hdr;
  };

That's my original proposal. I used to mirror this interface with "struct i2c_msg".

But the design philosophy of virtio TC is that VIRTIO devices are not specific to Linux so the specs design should avoid the limitations of the current Linux driver behavior.

We had some discussion about this. You may check these links to learn the story.
https://lists.oasis-open.org/archives/virtio-comment/202010/msg00016.html
https://lists.oasis-open.org/archives/virtio-comment/202010/msg00033.html
https://lists.oasis-open.org/archives/virtio-comment/202011/msg00025.html



[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux