From: Harald Mommer <harald.mommer@opensynergy.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: virtio-dev@lists.oasis-open.org,
Haixu Cui <quic_haixcui@quicinc.com>,
Mark Brown <broonie@kernel.org>,
linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org,
quic_ztu@quicinc.com, Matti Moell <Matti.Moell@opensynergy.com>,
Mikhail Golubev <Mikhail.Golubev@opensynergy.com>
Subject: [virtio-dev] Re: [PATCH v1 3/3] virtio-spi: Add virtio SPI driver.
Date: Thu, 29 Feb 2024 15:00:21 +0100 [thread overview]
Message-ID: <992c3573-f87c-447c-b8d7-d3bb29b0c5f0@opensynergy.com> (raw)
In-Reply-To: <20240229082224.2kh2scyjpielwx7v@vireshk-i7>
On 29.02.24 09:22, Viresh Kumar wrote:
> On 28-02-24, 15:27, Harald Mommer wrote:
>> +static int virtio_spi_probe(struct virtio_device *vdev)
>> +{
>> + struct device_node *np = vdev->dev.parent->of_node;
>> + struct virtio_spi_priv *priv;
>> + struct spi_controller *ctrl;
>> + int err;
>> + u32 bus_num;
>> + u16 csi;
>> +
>> + ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
>> + if (!ctrl) {
>> + dev_err(&vdev->dev, "Kernel memory exhausted in %s()\n",
>> + __func__);
> The print can be dropped I guess.
Looked around: It is habit not to do here dev_err() here so the
dev_err() is to be removed.
For curiosity I searched through the kernel whether the kernel already
leaves a trace the way down the memory allocation but somehow I landed
in the forest. Not important.
>> + return -ENOMEM;
>> + }
>> +
>> + priv = spi_controller_get_devdata(ctrl);
>> + priv->vdev = vdev;
>> + vdev->priv = priv;
>> + dev_set_drvdata(&vdev->dev, ctrl);
>> +
>> + init_completion(&priv->spi_req.completion);
>> +
>> + err = of_property_read_u32(np, "spi,bus-num", &bus_num);
>> + if (!err && bus_num <= S16_MAX)
>> + ctrl->bus_num = (s16)bus_num;
>> +
>> + virtio_spi_read_config(vdev);
>> +
>> + /* Method to do a single SPI transfer */
> The comments for obvious statements are normally not required. There
> are a couple of them here.
Removing now a few really obvious comments. This "code speaks for
itself" sitting in front of a mostly uncommented code desert is not mine
so I'm careful with this.
>
>> + ctrl->transfer_one = virtio_spi_transfer_one;
>> +
>> + /* Initialize virtqueues */
>> + err = virtio_spi_find_vqs(priv);
>> + if (err) {
>> + dev_err(&vdev->dev, "Cannot setup virtqueues\n");
> Maybe "Failed to" instead of "Cannot" ?
I grepped through the kernel for '"Cannot ' which brings all the
messages in the kernel which start with "Cannot ": 4123 matches (case
insensitive).
Did the same with `"Failed to ' which brings all the messages in the
kernel which start with "Failed to ": 34746 matches (case insensitive).
Majority uses "Failed to " but "Cannot " is also used. Both wordings
seem to be acceptable.
So this is no finding and I keep the code as it is. Otherwise we must
look again not only here but also in all other messages especially in
virtio_spi_set_delays() reworking more (for no good reason).
My wording in virtio_spi_restore() is more unusual, "problem ". Only 111
matches.
It's not wrong, it's not broken, nobody complained now, we will see.
>> + return err;
>> + }
>> +
>> + err = spi_register_controller(ctrl);
>> + if (err) {
>> + dev_err(&vdev->dev, "Cannot register controller\n");
>> + goto err_return;
>> + }
>> +
>> + board_info.max_speed_hz = priv->max_freq_hz;
>> + /* spi_new_device() currently does not use bus_num but better set it */
>> + board_info.bus_num = (u16)ctrl->bus_num;
>> +
>> + /* Add chip selects to controller */
>> + for (csi = 0; csi < ctrl->num_chipselect; csi++) {
>> + dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
>> + board_info.chip_select = csi;
> Maybe a blank line here.
>
>> + /* TODO: Discuss setting of board_info.mode */
>> + if (!(priv->mode_func_supported & VIRTIO_SPI_CS_HIGH))
>> + board_info.mode = SPI_MODE_0;
>> + else
>> + board_info.mode = SPI_MODE_0 | SPI_CS_HIGH;
> and here to improve readability.
Yes, code desert without blank lines here.
And while we are here and nobody wanted to discuss: The TODO comment is
to be removed now. In the meantime I'm convinced the code below is what
really should be done here.
>> + if (!spi_new_device(ctrl, &board_info)) {
>> + dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
>> + spi_unregister_controller(ctrl);
>> + err = -ENODEV;
>> + goto err_return;
>> + }
>> + }
>> +
>> + return 0;
>> +
>> +err_return:
>> + vdev->config->del_vqs(vdev);
>> + return err;
>> +}
>> +
>> +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 */
> Not sure if this comment is required or not, since we don't add
> similar ones while registering.
I got once from you a review comment about the de-initialization order.
Now the order is as it should be. This comment is needed to remind me
(and others) of the desired de-initialization order in case someone has
the idea to replace spi_register_controller() by
devm_spi_register_controller() in virtio_spi_probe(). By such a change
also the spi_unregister_controller() here would be removed and this
would change the de-initialization back to the undesired order.
Now there is the comment above the line being removed asking "Have you
thought about this?".
I was already reworking to devm_spi_register_controller() when I
realized late the undesired side effect and rolled back. Better we keep
this comment.
>> + spi_unregister_controller(ctrl);
>> + virtio_spi_del_vq(vdev);
>> +}
> Other than that.
>
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
>
No real code changes. Some comments to be removed, some blank lines to
be added, nothing urgent even in case the driver is integrated now
locally by someone for some need. No re-test will be needed. Let's see
what comes in addition. This is for next week, by than also the
maintenance of the virtio mailing lists will have been done.
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
prev parent reply other threads:[~2024-02-29 14:00 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-28 14:27 [virtio-dev] [PATCH v1 0/3] Virtio SPI Linux driver Harald Mommer
2024-02-28 14:27 ` [virtio-dev] [PATCH v1 1/3] virtio: Add ID for virtio SPI Harald Mommer
2024-02-28 14:27 ` [virtio-dev] [PATCH v1 2/3] virtio-spi: Add virtio-spi.h Harald Mommer
2024-02-28 14:27 ` [virtio-dev] [PATCH v1 3/3] virtio-spi: Add virtio SPI driver Harald Mommer
2024-02-29 8:22 ` [virtio-dev] " Viresh Kumar
2024-02-29 14:00 ` Harald Mommer [this message]
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=992c3573-f87c-447c-b8d7-d3bb29b0c5f0@opensynergy.com \
--to=harald.mommer@opensynergy.com \
--cc=Matti.Moell@opensynergy.com \
--cc=Mikhail.Golubev@opensynergy.com \
--cc=broonie@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=quic_haixcui@quicinc.com \
--cc=quic_ztu@quicinc.com \
--cc=viresh.kumar@linaro.org \
--cc=virtio-dev@lists.oasis-open.org \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).