Message-ID: <f2eefbeb-9a97-41db-a2e5-f069a8319900@quicinc.com>
Date: Mon, 25 Aug 2025 17:01:41 +0800
From: Haixu Cui <quic_haixcui@...cinc.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
CC: <harald.mommer@....qualcomm.com>, <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>,
        <quic_ztu@...cinc.com>
Subject: Re: [PATCH v4 3/3] SPI: Add virtio SPI driver

Hi Andy,

Thank you very much for your review and suggestions, I truly appreciate it!

I've carefully considered your comments and have replied directly under 
your comments with the corresponding changes. These updates will be 
included in next version of patch. Please let me know if there's 
anything else I should address.

Thanks again for your guidance and support.

Best Regards
Haixu Cui

On 8/20/2025 10:39 PM, Andy Shevchenko wrote:

> 
>> +#define VIRTIO_SPI_MODE_MASK \
>> +	(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)
> 
> We have SPI_MODE_X_MASK.
> 

#define VIRTIO_SPI_MODE_MASK \
         (SPI_MODE_X_MASK | SPI_CS_HIGH | SPI_LSB_FIRST)


>> +struct virtio_spi_req {
>> +	struct completion completion;
>> +	struct spi_transfer_head transfer_head	____cacheline_aligned;
>> +	const u8 *tx_buf;
>> +	u8 *rx_buf;
>> +	struct spi_transfer_result result	____cacheline_aligned;
>> +};
> 
> Dunno if `pahole` aware of ____cacheline_aligned attribute, but does it shows
> any potential improvement?
> 
> I think the fields can be reshuffled to go last and only one needs that
> attribute.
> 
> struct virtio_spi_req {
> 	struct completion completion;
> 	const u8 *tx_buf;
> 	u8 *rx_buf;
> 	struct spi_transfer_head transfer_head	____cacheline_aligned;
> 	struct spi_transfer_result result;
> };
> 
Ok, I’ll adjust the structure as recommended.



>> +static int virtio_spi_set_delays(struct spi_transfer_head *th,
>> +				 struct spi_device *spi,
>> +				 struct spi_transfer *xfer)
>> +{
>> +	int cs_setup;
>> +	int cs_word_delay_xfer;
>> +	int cs_word_delay_spi;
>> +	int delay;
>> +	int cs_hold;
>> +	int cs_inactive;
>> +	int cs_change_delay;
>> +
>> +	cs_setup = spi_delay_to_ns(&spi->cs_setup, xfer);
>> +	if (cs_setup < 0) {
> 
> Hmm... Not a problem in your code, but ns can be quite high for low speed
> links, there is a potential overflow...

You're right—ns values can be quite high on low-speed links, and there's 
a potential risk of overflow. I’ll keep a close eye on this and I've 
noted this as a follow-up action item.



> 
>> +	if (cs_word_delay_spi > cs_word_delay_xfer)
>> +		th->word_delay_ns = cpu_to_le32(cs_word_delay_spi);
>> +	else
>> +		th->word_delay_ns = cpu_to_le32(cs_word_delay_xfer);
> 
> Why not max() ?

update code with max() using:
th->word_delay_ns = cpu_to_le32(max(cs_word_delay_spi, cs_word_delay_xfer));



>> +	unsigned int outcnt = 0u;
>> +	unsigned int incnt = 0u;
> 
> Are 'u':s important in this case/

Will remove 'u' here.


>> +msg_done:
> 
>> +	kfree(spi_req);
> 
> Can be called via __free() to simplify the error handling,
> 

Yes, will update spi_req definition as follows then remove 
kfree(spi_req) here:

struct virtio_spi_req *spi_req __free(kfree);



>> +static int virtio_spi_probe(struct virtio_device *vdev)
>> +{
>> +	struct virtio_spi_priv *priv;
>> +	struct spi_controller *ctrl;
> 
>> +	int err;
> 
> Out of a sudden it's named 'err'. Please, go through the code and make
> style/naming/etc consistent.
> 
I'll update the naming to consistently use ret throughout, replacing err 
and other variants where appropriate



>> +	device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev));
> 
>> +	/* Setup ACPI node for controlled devices which will be probed through ACPI. */
>> +	ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));
> 
> This is strange. Either you need to put parent above in device_set_node() or
> drop it here. Otherwise it's inconsistent. Needs a very good explanation what's
> going on here...

I'll remove the ACPI_COMPANION_SET() line and kept only 
device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev)); to ensure consistency.

> 
>> +	dev_set_drvdata(&vdev->dev, ctrl);
>> +
>> +	err = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
>> +	if (!err && bus_num <= S16_MAX)
> 
> This is wrong. What is the bus_num value when err != 0?
> And why do we even care about this?
> 
>> +		ctrl->bus_num = bus_num;
>> +	else
>> +		ctrl->bus_num = -1;
>> +

Update as follows:
ret = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
         if (ret || bus_num > S16_MAX)
                 ctrl->bus_num = -1;
         else
                 ctrl->bus_num = bus_num;



>> +	err = virtio_spi_find_vqs(priv);
>> +	if (err) {
> 
>> +		dev_err_probe(&vdev->dev, err, "Cannot setup virtqueues\n");
>> +		return err;
> 
> 		return dev_err_probe(...);
> 
Acknowledged.




>> +	/* Register cleanup for virtqueues using devm */
>> +	err = devm_add_action_or_reset(&vdev->dev, (void (*)(void *))virtio_spi_del_vq, vdev);
>> +	if (err) {
>> +		dev_err_probe(&vdev->dev, err, "Cannot register virtqueue cleanup\n");
>> +		return err;
>> +	}
>> +
>> +	/* Use devm version to register controller */
>> +	err = devm_spi_register_controller(&vdev->dev, ctrl);
>> +	if (err) {
>> +		dev_err_probe(&vdev->dev, err, "Cannot register controller\n");
>> +		return err;
> 
> As per above.
Acknowledged

> 
>> +static int virtio_spi_freeze(struct device *dev)
>> +{
>> +	struct spi_controller *ctrl = dev_get_drvdata(dev);
>> +	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
> 
> Use dev_to_virtio()
Acknowledged




>> +static int virtio_spi_restore(struct device *dev)
>> +{
>> +	struct spi_controller *ctrl = dev_get_drvdata(dev);
>> +	struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
> 
> As per above.
Acknowledged

Powered by blists - more mailing lists