| Message ID | 20240228142755.4061-1-Harald.Mommer@opensynergy.com |
|---|---|
| Headers | show |
| Series | Virtio SPI Linux driver | expand |
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. > + 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. > + 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" ? > + 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. > + 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. > + spi_unregister_controller(ctrl); > + virtio_spi_del_vq(vdev); > +} Other than that. Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
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.
This is the 1st non-RFC version of a virtio SPI Linux driver which is intended to be compliant with the the upcoming virtio specification version 1.4. The specification can be found in repository https://github.com/oasis-tcs/virtio-spec.git branch virtio-1.4. This driver is the direct successor of the 3rd virtio driver RFC which was based on the same specification text. As in the meantime the virtio SPI specification has been accepted by OASIS the driver is now based on an official specification (vs. a draft) the time has come to remove the --rfc. Changes between 1st and 2nd virtio SPI driver RFC: - Update from virtio SPI draft specification V4 to V10. - Incorporate review comments gotten from the community. A proposal for a performance enhancement having more than only one SPI message in flight had to be kept out. The more complicated code would have caused an unacceptable project risk now. Changes between 2nd and 3rd virtio SPI driver RFC: - Order header inclusion alphabetically - Add Viresh Kumar's "signed-off" to the header files - Rework virtio_spi_one_transfer() - Rework the delays according to Haixu Cui's advise. Delays are now handled in a new sub-function virtio_spi_set_delays() - Minor change: Re-formulate arguments of sg_init_one() - Rework virtio_spi_probe() - Replace some goto in error paths by return - Add spi_unregister_controller() to an error path. Abstained from using devm_spi_register_controller() to keep order of de-initialization in virtio_spi_remove(). - Add deletion of vqueue to all error paths taken after the virtqueues have been initialized Changes between 3rd virtio SPI driver RFC and non-RFC driver V1 (newest) - Address kernel test robot comment which revealed an actual bug - Rework some comments in the code addressing review comments - Remove a TODO comment which has served it's purpose - Allocate struct virtio_spi_req spi_req only once at startup - Use callback transfer_one instead of transfer_one_message to simplify and shorten code. Due to this rework in the affected function(s) some additional changes: - Do init_completion() only once at startup, for re-initialization now reinit_completion() is used - Translate result codes VIRTIO_SPI_PARAM_ERR and VIRTIO_SPI_TRANS_ERR to appropriate Linux error codes -EINVAL and -EIO The virtio SPI driver was smoke tested on qemu using OpenSynergy's proprietary virtio SPI device doing a SPI backend simulation on top of https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git tag stable (commit 45ec2f5f6ed3ec3a79ba1329ad585497cdcbe663) and an adapted version of the driver on Linux 6.5 with target hardware providing a physical SPI backend device.