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

[Haixu Cui]

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