Re: [PATCH v3 3/3] virtio-spi: Add virtio SPI driver.

[Qiang Zhang <qiang4.zhang@xxxxxxxxxxxxxxx>]

On Mon, Dec 23, 2024 at 03:34:45PM +0800, Haixu Cui wrote:
> Hi @Qiang Zhang,
> 
>     Thank you for your comments, and please refer to my response below. I
> would co-work with Harald on this driver.

Great! I'm glad to see you push this work forward. Your comments below LGTM.

Thanks,
Qiang

> 
> On 9/9/2024 8:59 PM, Qiang Zhang wrote:
> > Hi, Harald
> > 
> > On Tue, Mar 26, 2024 at 12:28:12PM +0100, Harald Mommer wrote:
> > > From: Harald Mommer <harald.mommer@xxxxxxxxxxxxxxx>
> > > 
> > > This is the virtio SPI Linux kernel driver.
> > > 
> > > Signed-off-by: Harald Mommer <Harald.Mommer@xxxxxxxxxxxxxxx>
> > [...]
> > > +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)
> > > +		return -ENOMEM;
> > > +
> > > +	priv = spi_controller_get_devdata(ctrl);
> > > +	priv->vdev = vdev;
> > > +	vdev->priv = priv;
> > > +	ctrl->dev.of_node = vdev->dev.of_node;
> > 
> > To support ACPI, ACPI node of the controller needs to be set:
> > 
> > 	/*
> > 	 * Setup ACPI node for controlled devices which will be probed through
> > 	 * ACPI.
> > 	 */
> > 	ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));
> > 
> 
> Yes, will add in next patch.
> 
> > > +	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;
> > 
> > This is not the right way to fix bus number. You can use OF alias.
> > Also, to work with ACPI, we should use common API like
> > device_property_read_u32.
> > 
> 
> I agree with you, "spi,bus-num" should not be mandatory in the device tree.
> 
> How about updating as follows:
> 
> +	err = device_property_read_u32(ctrl->dev, "spi,bus-num", &bus_num);
> +	if (!err && bus_num <= S16_MAX)
> +		ctrl->bus_num = (s16)bus_num;
> +	else
> +		ctrl->bus_num = -1;
> 
> So if "spi,bus-num" not set, bus_num is initialized as -1, then in function
> spi_register_controller, bus_num will be reassigned by OF alias(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c?h=v6.13-rc4#n3287),
> or dynamic allocated(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c?h=v6.13-rc4#n3295).
> 
> 
> > > +
> > > +	virtio_spi_read_config(vdev);
> > > +
> > > +	ctrl->transfer_one = virtio_spi_transfer_one;
> > > +
> > > +	err = virtio_spi_find_vqs(priv);
> > > +	if (err) {
> > > +		dev_err(&vdev->dev, "Cannot setup virtqueues\n");
> > > +		return err;
> > > +	}
> > > +
> > > +	board_info.max_speed_hz = priv->max_freq_hz;
> > > +	board_info.bus_num = (u16)ctrl->bus_num;
> > > +
> > > +	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;
> > > +
> > > +	err = spi_register_controller(ctrl);
> > > +	if (err) {
> > > +		dev_err(&vdev->dev, "Cannot register controller\n");
> > > +		goto err_return;
> > > +	}
> > > +
> > > +	if (vdev->dev.of_node) {
> > > +		dev_dbg(&vdev->dev, "Final setup triggered by DT child node\n");
> > > +		return 0;
> > > +	}
> > How about ACPI path?
> > > +
> > > +	/* 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;
> > > +
> > > +		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;
> > > +		}
> > > +	}
> > Just enumerate SPI devices via DT/ACPI. And a fixed SPI modalias "spi-virtio"
> > is no better than match method from DT/ACPI.
> 
> How about deleting the following code.
> 
> +	if (vdev->dev.of_node) {
> +		dev_dbg(&vdev->dev, "Final setup triggered by DT child node\n");
> +		return 0;
> +	}
> 
> +	/* 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;
> +
> +		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;
> +		}
> +	}
> 
> Then we can use the ACPI implementation in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c?h=v6.13-rc4#n3378.
> 
> Also creating SPI devices statically based on the board info seems somewhat
> unreasonable. I agree that DT/ACPI are the only reasonable way to specify
> the SPI nodes.
> 
> 
> Thanks again
> Haixu Cui
> 
> > 
> > Thanks,
> > Qiang
>