From: Jie Deng <jie.deng@intel.com> To: Viresh Kumar <viresh.kumar@linaro.org> Cc: linux-i2c@vger.kernel.org, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, wsa@kernel.org, wsa+renesas@sang-engineering.com, mst@redhat.com, arnd@arndb.de, jasowang@redhat.com, andriy.shevchenko@linux.intel.com, yu1.wang@intel.com, shuo.a.liu@intel.com, conghui.chen@intel.com, stefanha@redhat.com Subject: Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver Date: Thu, 1 Jul 2021 14:10:48 +0800 [thread overview] Message-ID: <cb35472d-f79e-f3f8-405f-35c699d897a1@intel.com> (raw) In-Reply-To: <20210701040436.p7kega6rzeqz5tlm@vireshk-i7> On 2021/7/1 12:04, Viresh Kumar wrote: > On 01-07-21, 11:24, Jie Deng wrote: >> Changes v10 -> v11 >> - Remove vi->adap.class = I2C_CLASS_DEPRECATED. >> - Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused". >> - Remove "struct mutex lock" in "struct virtio_i2c". >> - Support zero-length request. >> - Remove unnecessary logs. >> - Remove vi->adap.timeout = HZ / 10, just use the default value. >> - Use BIT(0) to define VIRTIO_I2C_FLAGS_FAIL_NEXT. >> - Add the virtio_device index to adapter's naming mechanism. > Thanks Jie. > > I hope you are going to send a fix for specification as well (for the > zero-length request) ? Yes. I will send that fix once this patch get merged. > >> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c >> +static int virtio_i2c_send_reqs(struct virtqueue *vq, >> + struct virtio_i2c_req *reqs, >> + struct i2c_msg *msgs, int nr) >> +{ >> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr; >> + int i, outcnt, incnt, err = 0; >> + >> + for (i = 0; i < nr; i++) { >> + /* >> + * Only 7-bit mode supported for this moment. For the address format, >> + * Please check the Virtio I2C Specification. >> + */ >> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1); >> + >> + if (i != nr - 1) >> + reqs[i].out_hdr.flags = cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT); >> + >> + outcnt = incnt = 0; >> + sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr)); >> + sgs[outcnt++] = &out_hdr; >> + >> + reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1); >> + if (!reqs[i].buf) >> + break; >> + >> + sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len); > The len can be zero here for zero-length transfers. > >> + >> + if (msgs[i].flags & I2C_M_RD) >> + sgs[outcnt + incnt++] = &msg_buf; >> + else >> + sgs[outcnt++] = &msg_buf; >> + >> + sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr)); >> + sgs[outcnt + incnt++] = &in_hdr; > Why are we still sending the msg_buf if the length is 0? Sending the > buffer makes sense if you have some data to send, but otherwise it is > just an extra sg element, which isn't required to be sent. I think a fixed number of sgs will make things easier to develop backend. If you prefer to parse the number of descriptors instead of using the msg length to distinguish the zero-length request from other requests, I'm OK to set a limit. if (!msgs[i].len) { sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len); if (msgs[i].flags & I2C_M_RD) sgs[outcnt + incnt++] = &msg_buf; else sgs[outcnt++] = &msg_buf; } > >> +#ifdef CONFIG_PM_SLEEP >> +static int virtio_i2c_freeze(struct virtio_device *vdev) >> +{ >> + virtio_i2c_del_vqs(vdev); >> + return 0; >> +} >> + >> +static int virtio_i2c_restore(struct virtio_device *vdev) >> +{ >> + return virtio_i2c_setup_vqs(vdev->priv); >> +} >> +#endif >> + >> +static struct virtio_driver virtio_i2c_driver = { >> + .id_table = id_table, >> + .probe = virtio_i2c_probe, >> + .remove = virtio_i2c_remove, >> + .driver = { >> + .name = "i2c_virtio", >> + }, >> +#ifdef CONFIG_PM_SLEEP > You could avoid this pair of ifdef by creating dummy versions of below > routines for !CONFIG_PM_SLEEP case. Up to you. Thank you. I'd like to keep the same.
WARNING: multiple messages have this Message-ID (diff)
From: Jie Deng <jie.deng@intel.com> To: Viresh Kumar <viresh.kumar@linaro.org> Cc: yu1.wang@intel.com, arnd@arndb.de, mst@redhat.com, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, wsa@kernel.org, wsa+renesas@sang-engineering.com, linux-i2c@vger.kernel.org, stefanha@redhat.com, shuo.a.liu@intel.com, andriy.shevchenko@linux.intel.com, conghui.chen@intel.com Subject: Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver Date: Thu, 1 Jul 2021 14:10:48 +0800 [thread overview] Message-ID: <cb35472d-f79e-f3f8-405f-35c699d897a1@intel.com> (raw) In-Reply-To: <20210701040436.p7kega6rzeqz5tlm@vireshk-i7> On 2021/7/1 12:04, Viresh Kumar wrote: > On 01-07-21, 11:24, Jie Deng wrote: >> Changes v10 -> v11 >> - Remove vi->adap.class = I2C_CLASS_DEPRECATED. >> - Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused". >> - Remove "struct mutex lock" in "struct virtio_i2c". >> - Support zero-length request. >> - Remove unnecessary logs. >> - Remove vi->adap.timeout = HZ / 10, just use the default value. >> - Use BIT(0) to define VIRTIO_I2C_FLAGS_FAIL_NEXT. >> - Add the virtio_device index to adapter's naming mechanism. > Thanks Jie. > > I hope you are going to send a fix for specification as well (for the > zero-length request) ? Yes. I will send that fix once this patch get merged. > >> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c >> +static int virtio_i2c_send_reqs(struct virtqueue *vq, >> + struct virtio_i2c_req *reqs, >> + struct i2c_msg *msgs, int nr) >> +{ >> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr; >> + int i, outcnt, incnt, err = 0; >> + >> + for (i = 0; i < nr; i++) { >> + /* >> + * Only 7-bit mode supported for this moment. For the address format, >> + * Please check the Virtio I2C Specification. >> + */ >> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1); >> + >> + if (i != nr - 1) >> + reqs[i].out_hdr.flags = cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT); >> + >> + outcnt = incnt = 0; >> + sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr)); >> + sgs[outcnt++] = &out_hdr; >> + >> + reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1); >> + if (!reqs[i].buf) >> + break; >> + >> + sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len); > The len can be zero here for zero-length transfers. > >> + >> + if (msgs[i].flags & I2C_M_RD) >> + sgs[outcnt + incnt++] = &msg_buf; >> + else >> + sgs[outcnt++] = &msg_buf; >> + >> + sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr)); >> + sgs[outcnt + incnt++] = &in_hdr; > Why are we still sending the msg_buf if the length is 0? Sending the > buffer makes sense if you have some data to send, but otherwise it is > just an extra sg element, which isn't required to be sent. I think a fixed number of sgs will make things easier to develop backend. If you prefer to parse the number of descriptors instead of using the msg length to distinguish the zero-length request from other requests, I'm OK to set a limit. if (!msgs[i].len) { sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len); if (msgs[i].flags & I2C_M_RD) sgs[outcnt + incnt++] = &msg_buf; else sgs[outcnt++] = &msg_buf; } > >> +#ifdef CONFIG_PM_SLEEP >> +static int virtio_i2c_freeze(struct virtio_device *vdev) >> +{ >> + virtio_i2c_del_vqs(vdev); >> + return 0; >> +} >> + >> +static int virtio_i2c_restore(struct virtio_device *vdev) >> +{ >> + return virtio_i2c_setup_vqs(vdev->priv); >> +} >> +#endif >> + >> +static struct virtio_driver virtio_i2c_driver = { >> + .id_table = id_table, >> + .probe = virtio_i2c_probe, >> + .remove = virtio_i2c_remove, >> + .driver = { >> + .name = "i2c_virtio", >> + }, >> +#ifdef CONFIG_PM_SLEEP > You could avoid this pair of ifdef by creating dummy versions of below > routines for !CONFIG_PM_SLEEP case. Up to you. Thank you. I'd like to keep the same. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-07-01 6:11 UTC|newest] Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-01 3:24 [PATCH v11] i2c: virtio: add a virtio i2c frontend driver Jie Deng 2021-07-01 3:24 ` Jie Deng 2021-07-01 4:04 ` Viresh Kumar 2021-07-01 4:04 ` Viresh Kumar 2021-07-01 6:10 ` Jie Deng [this message] 2021-07-01 6:10 ` Jie Deng 2021-07-01 6:18 ` Viresh Kumar 2021-07-01 6:18 ` Viresh Kumar 2021-07-02 3:36 ` Jie Deng 2021-07-02 3:36 ` Jie Deng 2021-07-02 4:56 ` Viresh Kumar 2021-07-02 4:56 ` Viresh Kumar 2021-07-01 19:24 ` Wolfram Sang 2021-07-02 4:55 ` Viresh Kumar 2021-07-02 4:55 ` Viresh Kumar 2021-07-02 6:22 ` Wolfram Sang 2021-07-02 6:52 ` Jie Deng 2021-07-02 6:52 ` Jie Deng 2021-07-02 6:56 ` Viresh Kumar 2021-07-02 6:56 ` Viresh Kumar 2021-07-02 7:11 ` Wolfram Sang 2021-07-02 7:15 ` Jie Deng 2021-07-02 7:15 ` Jie Deng 2021-07-02 7:21 ` Viresh Kumar 2021-07-02 7:21 ` Viresh Kumar 2021-07-02 7:36 ` Wolfram Sang 2021-07-01 8:50 ` kernel test robot 2021-07-01 8:50 ` kernel test robot 2021-07-01 8:50 ` kernel test robot 2021-07-01 10:00 ` kernel test robot 2021-07-01 10:00 ` kernel test robot 2021-07-01 10:00 ` kernel test robot 2021-07-02 3:12 ` Jie Deng 2021-07-02 3:12 ` Jie Deng 2021-07-02 3:12 ` Jie Deng 2021-07-02 6:38 ` [kbuild-all] " Rong Chen 2021-07-02 6:38 ` Rong Chen 2021-07-01 10:45 ` Andy Shevchenko 2021-07-01 10:45 ` Andy Shevchenko 2021-07-02 1:05 ` Jie Deng 2021-07-02 1:05 ` Jie Deng
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=cb35472d-f79e-f3f8-405f-35c699d897a1@intel.com \ --to=jie.deng@intel.com \ --cc=andriy.shevchenko@linux.intel.com \ --cc=arnd@arndb.de \ --cc=conghui.chen@intel.com \ --cc=jasowang@redhat.com \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mst@redhat.com \ --cc=shuo.a.liu@intel.com \ --cc=stefanha@redhat.com \ --cc=viresh.kumar@linaro.org \ --cc=virtualization@lists.linux-foundation.org \ --cc=wsa+renesas@sang-engineering.com \ --cc=wsa@kernel.org \ --cc=yu1.wang@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.