Re: [PATCH] i2c: virtio: add a virtio i2c frontend driver
From: Jason Wang
Date: Mon Sep 07 2020 - 01:40:57 EST
On 2020/9/4 下午9:21, Jie Deng wrote:
On 2020/9/4 12:06, Jason Wang wrote:
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 293e7a0..70c8e30 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -21,6 +21,17 @@ config I2C_ALI1535
This driver can also be built as a module. If so, the module
will be called i2c-ali1535.
+config I2C_VIRTIO
+ tristate "Virtio I2C Adapter"
+ depends on VIRTIO
I guess it should depend on some I2C module here.
The dependency of I2C is included in the Kconfig in its parent directory.
So there is nothing special to add here.
Ok.
+struct virtio_i2c_msg {
+ struct virtio_i2c_hdr hdr;
+ char *buf;
+ u8 status;
Any reason for separating status out of virtio_i2c_hdr?
The status is not from i2c_msg.
You meant ic2_hdr? You embed status in virtio_i2c_msg anyway.
So I put it out of virtio_i2c_hdr.
Something like status or response is pretty common in virtio request
(e.g net or scsi), if no special reason, it's better to keep it in the hdr.
+};
+
+/**
+ * struct virtio_i2c - virtio I2C data
+ * @vdev: virtio device for this controller
+ * @completion: completion of virtio I2C message
+ * @adap: I2C adapter for this controller
+ * @i2c_lock: lock for virtqueue processing
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+ struct virtio_device *vdev;
+ struct completion completion;
+ struct i2c_adapter adap;
+ struct mutex i2c_lock;
+ struct virtqueue *vq;
+};
+
+static void virtio_i2c_msg_done(struct virtqueue *vq)
+{
+ struct virtio_i2c *vi = vq->vdev->priv;
+
+ complete(&vi->completion);
+}
+
+static int virtio_i2c_add_msg(struct virtqueue *vq,
+ struct virtio_i2c_msg *vmsg,
+ struct i2c_msg *msg)
+{
+ struct scatterlist *sgs[3], hdr, bout, bin, status;
+ int outcnt = 0, incnt = 0;
+
+ if (!msg->len)
+ return -EINVAL;
+
+ vmsg->hdr.addr = msg->addr;
+ vmsg->hdr.flags = msg->flags;
+ vmsg->hdr.len = msg->len;
Missing endian conversion?
You are right. Need conversion here.
+
+ vmsg->buf = kzalloc(vmsg->hdr.len, GFP_KERNEL);
+ if (!vmsg->buf)
+ return -ENOMEM;
+
+ sg_init_one(&hdr, &vmsg->hdr, sizeof(struct virtio_i2c_hdr));
+ sgs[outcnt++] = &hdr;
+ if (vmsg->hdr.flags & I2C_M_RD) {
+ sg_init_one(&bin, vmsg->buf, msg->len);
+ sgs[outcnt + incnt++] = &bin;
+ } else {
+ memcpy(vmsg->buf, msg->buf, msg->len);
+ sg_init_one(&bout, vmsg->buf, msg->len);
+ sgs[outcnt++] = &bout;
+ }
+ sg_init_one(&status, &vmsg->status, sizeof(vmsg->status));
+ sgs[outcnt + incnt++] = &status;
+
+ return virtqueue_add_sgs(vq, sgs, outcnt, incnt, vmsg,
GFP_KERNEL);
+}
+
+static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
*msgs, int num)
+{
+ struct virtio_i2c *vi = i2c_get_adapdata(adap);
+ struct virtio_i2c_msg *vmsg_o, *vmsg_i;
+ struct virtqueue *vq = vi->vq;
+ unsigned long time_left;
+ int len, i, ret = 0;
+
+ vmsg_o = kzalloc(sizeof(*vmsg_o), GFP_KERNEL);
+ if (!vmsg_o)
+ return -ENOMEM;
It looks to me we can avoid the allocation by embedding
virtio_i2c_msg into struct virtio_i2c;
Yeah... That's better. Thanks.
+
+ mutex_lock(&vi->i2c_lock);
+ vmsg_o->buf = NULL;
+ for (i = 0; i < num; i++) {
+ ret = virtio_i2c_add_msg(vq, vmsg_o, &msgs[i]);
+ if (ret) {
+ dev_err(&adap->dev, "failed to add msg[%d] to
virtqueue.\n", i);
+ goto err_unlock_free;
+ }
+
+ virtqueue_kick(vq);
+
+ time_left = wait_for_completion_timeout(&vi->completion,
adap->timeout);
+ if (!time_left) {
+ dev_err(&adap->dev, "msg[%d]: addr=0x%x timeout.\n", i,
msgs[i].addr);
+ ret = i;
+ goto err_unlock_free;
+ }
+
+ vmsg_i = (struct virtio_i2c_msg *)virtqueue_get_buf(vq, &len);
+ if (vmsg_i) {
+ /* vmsg_i should point to the same address with vmsg_o */
+ if (vmsg_i != vmsg_o) {
+ dev_err(&adap->dev, "msg[%d]: addr=0x%x virtqueue
error.\n",
+ i, vmsg_i->hdr.addr);
+ ret = i;
+ goto err_unlock_free;
+ }
Does this imply in order completion of i2c device? (E.g what happens
if multiple virtio i2c requests are submitted)
Btw, this always use a single descriptor once a time which makes me
suspect if a virtqueue(virtio) is really needed. It looks to me we
can utilize the virtqueue by submit the request in a batch.
I'm afraid not all physical devices support batch.
Yes but I think I meant for the virtio device not the physical one. It's
impossible to forbid batching if you have a queue anyway ...
I'd like to keep the current design and consider
your suggestion as a possible optimization in the future.
Thanks.
+}
+
+static void virtio_i2c_del_vqs(struct virtio_device *vdev)
+{
+ vdev->config->reset(vdev);
Why need reset here?
Thanks
I'm following what other virtio drivers do.
They reset the devices before they clean up the queues.
You're ring.
Thanks
+ vdev->config->del_vqs(vdev);
+}
+
+static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)
+{
+ struct virtio_device *vdev = vi->vdev;
+
+ vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done,
"i2c-msg");
We've in the scope of ic2, so "msg" should be sufficient.
OK. Will change this name. Thanks.
+ return PTR_ERR_OR_ZERO(vi->vq);