Messages in this thread | | | Subject | Re: [PATCH] i2c: virtio: add a virtio i2c frontend driver | From | Jie Deng <> | Date | Fri, 4 Sep 2020 21:21:07 +0800 |
| |
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.
> >> >> +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. So I put it out of virtio_i2c_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. 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.
> >> + 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);
| |