[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e61e6d6a-90e8-4ea8-beca-ab441f0eea34@quicinc.com>
Date: Fri, 4 Jul 2025 15:46:07 +0800
From: Haixu Cui <quic_haixcui@...cinc.com>
To: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>, <broonie@...nel.org>,
<virtio-dev@...ts.linux.dev>, <viresh.kumar@...aro.org>,
<linux-spi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<hdanton@...a.com>, <qiang4.zhang@...ux.intel.com>,
<alex.bennee@...aro.org>
CC: <quic_ztu@...cinc.com>
Subject: Re: [RFC PATCH v5 3/3] SPI: Add virtio SPI driver.
Hi Mukesh,
Thank you for reviewing the patch and providing your feedback. Really
appreciate your detailed suggestions.
On 6/30/2025 2:54 PM, Mukesh Kumar Savaliya wrote:
> Hi, Haixu, Thanks !
>
> On 6/20/2025 9:42 AM, Haixu Cui wrote:
>> This is the virtio SPI Linux kernel driver.
>>
>> Signed-off-by: Haixu Cui <quic_haixcui@...cinc.com>
>> ---
>> MAINTAINERS | 6 +
>> drivers/spi/Kconfig | 11 +
>> drivers/spi/Makefile | 1 +
>> drivers/spi/spi-virtio.c | 444 +++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 462 insertions(+)
>> create mode 100644 drivers/spi/spi-virtio.c
>>
>> + * So the corresponding relationship:
>> + * A <===> cs_setup_ns (after CS asserted)
> And "before clock start" ? to be added in bracket as comment.
Here I refer to the cs_setup definition in include/linux/spi/spi.h,
where the cs_setup is described only in terms of delay after CS is
asserted, without referencing the clock signal.
>
>> +
>> +static int virtio_spi_transfer_one(struct spi_controller *ctrl,
>> + struct spi_device *spi,
>> + struct spi_transfer *xfer)
>> +{
>
> [...]
>
>> + wait_for_completion(&priv->spi_req.completion);
>> +
> I see init_completion(spi_req.completion) is called during probe() but
> successive transfer doent have reinit_completion(spi_req.completion).
> wondering how is this working for back to back transfers.
In current implementation, each SPI transfer uses the same spi_req
instance, which is a member of struct virtio_spi_priv.
I'm considering removing spi_req from the virtio_spi_priv structure,
instead dynamically allocation a new spi_req for each transfer. This
way, each transfer would have its own completion object, so we could
simply call init_completion() without worrying about reinitializing a
shared one. I believe this would make the design cleaner and avoid
potential issues.
Is this approach okay with you? If so I will update the patch
accordingly in the next revision.
>> + /* Read result from message and translate return code */
>> + switch (priv->spi_req.result.result) {
>> + case VIRTIO_SPI_TRANS_OK:
>> + /* ret is 0 */
>> + break;
>> + case VIRTIO_SPI_PARAM_ERR:
>> + ret = -EINVAL;
>> + break;
>> + case VIRTIO_SPI_TRANS_ERR:
>> + ret = -EIO;
>> + break;
>> + default: /* Protocol violation */
> Comment in new line ? following same method across.
This comment seems not particularly helpful. I’ll remove it to keep
the code clean.
>> +static void virtio_spi_remove(struct virtio_device *vdev)
>> +{
>> + struct spi_controller *ctrl = dev_get_drvdata(&vdev->dev);
>> +
>> + /* Order: 1.) unregister controller, 2.) remove virtqueue */
> Is this a specific flow for virtio OR generic ? if its generic, we can
> remove the comments.
This is generic actually, I will remove it to keep code clean.
>> + spi_unregister_controller(ctrl);
>> + virtio_spi_del_vq(vdev);
>> +}
>> +
Thanks
Haixu Cui
Powered by blists - more mailing lists