From: Jie Deng <jie.deng@intel.com> To: Wolfram Sang <wsa@kernel.org>, linux-i2c@vger.kernel.org, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, mst@redhat.com, jasowang@redhat.com, andriy.shevchenko@linux.intel.com, conghui.chen@intel.com, arnd@arndb.de, kblaiech@mellanox.com, jarkko.nikula@linux.intel.com, Sergey.Semin@baikalelectronics.ru, rppt@kernel.org, loic.poulain@linaro.org, tali.perry1@gmail.com, u.kleine-koenig@pengutronix.de, bjorn.andersson@linaro.org, yu1.wang@intel.com, shuo.a.liu@intel.com, viresh.kumar@linaro.org, stefanha@redhat.com, pbonzini@redhat.com Subject: Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver Date: Wed, 30 Jun 2021 14:45:55 +0800 [thread overview] Message-ID: <05cc9484-f97b-0533-64fe-ff917c6b87ee@intel.com> (raw) In-Reply-To: <YNrw4rxihFLuqLtY@ninjato> On 2021/6/29 18:07, Wolfram Sang wrote: > Hi, > > so some minor comments left: > >> + if (!msgs[i].len) >> + break; > I hope this can extended in the future to allow zero-length messages. If > this is impossible we need to set an adapter quirk instead. Yes, we can support it by removing this check and call it zero-length request. It don't think it will break anything. > >> + 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); > Is it really helpful for the user to know that msg5 failed? We don't > even say which transfer. OK. I will remove this print. >> +static u32 virtio_i2c_func(struct i2c_adapter *adap) >> +{ >> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; > You are not emulating I2C_FUNC_SMBUS_QUICK, so you need to mask it out. I will remove the check of zero-length message. > >> + snprintf(vi->adap.name, sizeof(vi->adap.name), "Virtio I2C Adapter"); > Is there something to add so you can distinguish multiple instances? > Most people want that. I find the I2C core will set a device name "i2c-%d" for this purpose, right? I think this name can be used to distinguish the adapter types while "i2c-%d" can be used to distinguish instances. Does it make sense ? >> + vi->adap.class = I2C_CLASS_DEPRECATED; >> + vi->adap.algo = &virtio_algorithm; >> + vi->adap.dev.parent = &vdev->dev; >> + vi->adap.timeout = HZ / 10; > Why so short? HZ is the kinda default value. Ah... I didn't know the I2C core had already set a default value. I will remove this line to use the default one. > >> + i2c_set_adapdata(&vi->adap, vi); >> + >> + /* Setup ACPI node for controlled devices which will be probed through ACPI */ >> + ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev)); >> + >> + ret = i2c_add_adapter(&vi->adap); >> + if (ret) { >> + virtio_i2c_del_vqs(vdev); >> + dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n"); > Won't the driver core print that for us? Yes. It seems unnecessary. Will remove it. > >> + } >> + >> + return ret; >> +} >> + >> +/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */ >> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT 0x00000001 > BIT(0)? That's better. Thank you. > > Happy hacking, > > Wolfram >
WARNING: multiple messages have this Message-ID (diff)
From: Jie Deng <jie.deng@intel.com> To: Wolfram Sang <wsa@kernel.org>, linux-i2c@vger.kernel.org, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, mst@redhat.com, jasowang@redhat.com, andriy.shevchenko@linux.intel.com, conghui.chen@intel.com, arnd@arndb.de, kblaiech@mellanox.com, jarkko.nikula@linux.intel.com, Sergey.Semin@baikalelectronics.ru, rppt@kernel.org, loic.poulain@linaro.org, tali.perry1@gmail.com, u.kleine-koenig@pengutronix.de, bjorn.andersson@linaro.org, yu1.wang@intel.com, shuo.a.liu@intel.com, viresh.kumar@linaro.org, stefanha@redhat.com, pbonzini@redhat.com Subject: Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver Date: Wed, 30 Jun 2021 14:45:55 +0800 [thread overview] Message-ID: <05cc9484-f97b-0533-64fe-ff917c6b87ee@intel.com> (raw) In-Reply-To: <YNrw4rxihFLuqLtY@ninjato> On 2021/6/29 18:07, Wolfram Sang wrote: > Hi, > > so some minor comments left: > >> + if (!msgs[i].len) >> + break; > I hope this can extended in the future to allow zero-length messages. If > this is impossible we need to set an adapter quirk instead. Yes, we can support it by removing this check and call it zero-length request. It don't think it will break anything. > >> + 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); > Is it really helpful for the user to know that msg5 failed? We don't > even say which transfer. OK. I will remove this print. >> +static u32 virtio_i2c_func(struct i2c_adapter *adap) >> +{ >> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; > You are not emulating I2C_FUNC_SMBUS_QUICK, so you need to mask it out. I will remove the check of zero-length message. > >> + snprintf(vi->adap.name, sizeof(vi->adap.name), "Virtio I2C Adapter"); > Is there something to add so you can distinguish multiple instances? > Most people want that. I find the I2C core will set a device name "i2c-%d" for this purpose, right? I think this name can be used to distinguish the adapter types while "i2c-%d" can be used to distinguish instances. Does it make sense ? >> + vi->adap.class = I2C_CLASS_DEPRECATED; >> + vi->adap.algo = &virtio_algorithm; >> + vi->adap.dev.parent = &vdev->dev; >> + vi->adap.timeout = HZ / 10; > Why so short? HZ is the kinda default value. Ah... I didn't know the I2C core had already set a default value. I will remove this line to use the default one. > >> + i2c_set_adapdata(&vi->adap, vi); >> + >> + /* Setup ACPI node for controlled devices which will be probed through ACPI */ >> + ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev)); >> + >> + ret = i2c_add_adapter(&vi->adap); >> + if (ret) { >> + virtio_i2c_del_vqs(vdev); >> + dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n"); > Won't the driver core print that for us? Yes. It seems unnecessary. Will remove it. > >> + } >> + >> + return ret; >> +} >> + >> +/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */ >> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT 0x00000001 > BIT(0)? That's better. Thank you. > > Happy hacking, > > Wolfram > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-06-30 6:46 UTC|newest] Thread overview: 114+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-23 14:19 [PATCH v10] i2c: virtio: add a virtio i2c frontend driver Jie Deng 2021-03-23 14:19 ` Jie Deng 2021-03-23 7:27 ` Viresh Kumar 2021-03-23 8:33 ` Jie Deng 2021-03-23 8:33 ` Jie Deng 2021-03-23 8:37 ` Viresh Kumar 2021-03-23 9:27 ` Arnd Bergmann 2021-03-23 9:27 ` Arnd Bergmann 2021-03-24 1:17 ` Jie Deng 2021-03-24 1:17 ` Jie Deng 2021-03-24 4:00 ` Viresh Kumar 2021-04-15 6:45 ` Viresh Kumar 2021-04-15 6:56 ` Jie Deng 2021-04-15 6:56 ` Jie Deng 2021-04-15 7:15 ` Viresh Kumar 2021-04-15 7:21 ` Wolfram Sang 2021-04-15 7:24 ` Viresh Kumar 2021-04-15 7:28 ` Wolfram Sang 2021-04-15 7:37 ` Viresh Kumar 2021-04-15 8:15 ` Jie Deng 2021-04-15 8:15 ` Jie Deng 2021-04-15 8:18 ` Wolfram Sang 2021-04-15 8:20 ` Jie Deng 2021-04-15 8:20 ` Jie Deng 2021-05-27 6:49 ` Jie Deng 2021-05-27 6:49 ` Jie Deng 2021-05-12 1:37 ` Jie Deng 2021-05-12 1:37 ` Jie Deng 2021-03-23 9:01 ` Viresh Kumar 2021-03-23 9:38 ` Viresh Kumar 2021-03-24 0:53 ` Jie Deng 2021-03-24 0:53 ` Jie Deng 2021-03-24 3:52 ` Viresh Kumar 2021-03-24 4:00 ` Jie Deng 2021-03-24 4:00 ` Jie Deng 2021-03-24 4:02 ` Viresh Kumar 2021-03-24 4:20 ` Viresh Kumar 2021-03-24 6:05 ` Jie Deng 2021-03-24 6:05 ` Jie Deng 2021-03-24 6:09 ` Viresh Kumar 2021-03-24 6:41 ` Jie Deng 2021-03-24 6:41 ` Jie Deng 2021-03-24 6:44 ` Viresh Kumar 2021-04-14 2:07 ` Jie Deng 2021-04-14 2:07 ` Jie Deng 2021-04-14 3:52 ` Viresh Kumar 2021-04-15 6:25 ` Jie Deng 2021-04-15 6:25 ` Jie Deng 2021-04-15 3:51 ` Jason Wang 2021-04-15 3:51 ` Jason Wang 2021-04-15 6:17 ` Jie Deng 2021-04-15 6:17 ` Jie Deng 2021-06-28 8:39 ` Wolfram Sang 2021-06-28 9:01 ` Arnd Bergmann 2021-06-28 9:01 ` Arnd Bergmann 2021-06-28 9:25 ` Wolfram Sang 2021-06-28 9:51 ` Arnd Bergmann 2021-06-28 9:51 ` Arnd Bergmann 2021-06-28 10:13 ` Wolfram Sang 2021-06-28 11:50 ` Arnd Bergmann 2021-06-28 11:50 ` Arnd Bergmann 2021-06-28 14:58 ` Wolfram Sang 2021-06-29 3:04 ` Jie Deng 2021-06-29 3:04 ` Jie Deng 2021-06-29 8:30 ` Wolfram Sang 2021-06-29 9:13 ` Viresh Kumar 2021-06-29 9:13 ` Viresh Kumar 2021-06-29 9:36 ` Wolfram Sang 2021-06-29 4:10 ` Viresh Kumar 2021-06-29 4:10 ` Viresh Kumar 2021-06-29 8:27 ` Wolfram Sang 2021-06-29 8:52 ` Viresh Kumar 2021-06-29 8:52 ` Viresh Kumar 2021-06-29 9:07 ` Wolfram Sang 2021-06-30 14:38 ` Wolfram Sang 2021-06-30 15:09 ` Viresh Kumar 2021-06-30 15:09 ` Viresh Kumar 2021-06-29 3:03 ` Jie Deng 2021-06-29 3:03 ` Jie Deng 2021-06-29 10:07 ` Wolfram Sang 2021-06-29 10:16 ` Viresh Kumar 2021-06-29 10:16 ` Viresh Kumar 2021-06-29 10:23 ` Wolfram Sang 2021-06-29 10:30 ` Viresh Kumar 2021-06-29 10:30 ` Viresh Kumar 2021-06-29 10:42 ` Wolfram Sang 2021-06-29 10:43 ` Wolfram Sang 2021-06-29 10:56 ` Viresh Kumar 2021-06-29 10:56 ` Viresh Kumar 2021-06-29 11:11 ` Wolfram Sang 2021-06-29 11:16 ` Viresh Kumar 2021-06-29 11:16 ` Viresh Kumar 2021-06-29 11:48 ` Wolfram Sang 2021-07-05 12:18 ` Viresh Kumar 2021-07-05 12:18 ` Viresh Kumar 2021-07-06 1:50 ` Jie Deng 2021-07-06 1:50 ` Jie Deng 2021-07-22 15:15 ` Wolfram Sang 2021-07-23 2:28 ` Viresh Kumar 2021-07-23 2:28 ` Viresh Kumar 2021-07-23 5:28 ` Viresh Kumar 2021-07-23 5:28 ` Viresh Kumar 2021-06-30 6:45 ` Jie Deng [this message] 2021-06-30 6:45 ` Jie Deng 2021-06-30 7:32 ` Wolfram Sang 2021-06-30 7:51 ` Jie Deng 2021-06-30 7:51 ` Jie Deng 2021-06-30 7:55 ` Arnd Bergmann 2021-06-30 7:55 ` Arnd Bergmann 2021-06-30 8:04 ` Wolfram Sang 2021-06-30 8:07 ` Andy Shevchenko 2021-06-30 8:07 ` Andy Shevchenko 2021-06-30 8:29 ` Arnd Bergmann 2021-06-30 8:29 ` Arnd Bergmann
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=05cc9484-f97b-0533-64fe-ff917c6b87ee@intel.com \ --to=jie.deng@intel.com \ --cc=Sergey.Semin@baikalelectronics.ru \ --cc=andriy.shevchenko@linux.intel.com \ --cc=arnd@arndb.de \ --cc=bjorn.andersson@linaro.org \ --cc=conghui.chen@intel.com \ --cc=jarkko.nikula@linux.intel.com \ --cc=jasowang@redhat.com \ --cc=kblaiech@mellanox.com \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=loic.poulain@linaro.org \ --cc=mst@redhat.com \ --cc=pbonzini@redhat.com \ --cc=rppt@kernel.org \ --cc=shuo.a.liu@intel.com \ --cc=stefanha@redhat.com \ --cc=tali.perry1@gmail.com \ --cc=u.kleine-koenig@pengutronix.de \ --cc=viresh.kumar@linaro.org \ --cc=virtualization@lists.linux-foundation.org \ --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.