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 <[email protected]>
--
viresh
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
