Re: [RFC PATCH v4 3/3] SPI: Add virtio SPI driver

[Haixu Cui <quic_haixcui@quicinc.com>]

Hi Mukesh,

Thank you for your detailed review and valuable feedback.
I have carefully considered your comments and would like to
address them as follows:


> Could you please help add exact function description which can help 
> understand below details with some background/purpose ?
> 

Different delay parameters impact various phases of SPI transfers,
governing the assertion/deassertion of the CS and the timing intervals
between adjacent words in a single transfer or between consecutive
transfers.

Some delays are inherent device attributes, such as word_delay and
cs_setup in the spi_device structure, while others are specified
per-transfer via fields in the spi_transfer structure, such as
cs_change_delay and delay.

Although virtio SPI cannot directly configure transfer-stage delays by
manipulating hardware registers or precisely control CS signal behavior,
it can indirectly achieve fine-grained transfer control by passing
parameters like cs_delay_hold_ns and cs_setup_ns in the
spi_transfer_head structure to the host.

Function virtio_spi_set_delays is designed to implement this feature,
and will add description like:

/*
  * virtio_spi_set_delays - Set delay parameters for SPI transfer
  *
  * This function sets various delay parameters for SPI transfer,
  * including delay after CS asserted, timing intervals between
  * adjacent words within a transfer, delay before abdafter CS
  * deasserted. It converts these delay parameters to nanoseconds using
  * spi_delay_to_ns and stores the results in spi_transfer_head
  * structure.
  * If the conversion fails, the function logs a warning message and
  * returns an error code.
  */

Could you please review this comment and let me know if it's clear
and easy to understand? Your feedback will be greatly appreciated.



>> + * E => struct spi_device -> cs_inactive
>> + * F => struct spi_transfer -> cs_change_delay

> Alignment of single line arrow  -> .

Alignment is now consistent with "=>", could you please confirm if it 
also needs to be aligned with "->"?



>> + *
>> + * So the corresponding relationship:
>> + * A <===> cs_setup_ns (after CS asserted)
>> + * B <===> word_delay_ns (no matter with CS)
> what does it mean when you say "no matter with CS" ? Any other 
> simplified statement ?
> Delay from sending actual data /Clock generation ?

"no matter with CS" means this delay is not related to CS actually.
B is the timing interval between 2 words, during which the CS remains 
asserted.

I agree this description is unclear, just update as:

B <===> word_delay_ns (delay between adjacent words within a transfer)

>> + * C+D <===> cs_delay_hold_ns (before CS deasserted)
>> + * E+F <===> cs_change_delay_inactive_ns (after CS deasserted, these two
>> + * values are also recommended in the Linux driver to be added up)
> Alignment of all double dashed line arrows <===> to left side symbols .

OK, will update to be aligned with "<===>".


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

Yes, will update as:

dev_err_probe(&vdev->dev, err, "Cannot setup virtqueues\n");


Thanks & BR
Haixu Cui