[PATCH RFC 1/7] spi: Add 'rx_sampling_delay_ns' parameter for clock to RX delay

Frieder Schrempf posted 7 patches 1 month, 1 week ago
[PATCH RFC 1/7] spi: Add 'rx_sampling_delay_ns' parameter for clock to RX delay
Posted by Frieder Schrempf 1 month, 1 week ago
From: Eberhard Stoll <eberhard.stoll@kontron.de>

Some high speed SPI devices require a delay between the controller
clock and the sampling point of the client device. This 'clock to
receive data delay' value is often referenced as tCLQV in SPI device
data sheets.

Not respecting this can lead to data being sampled too early when
the client doesn't yet reflect the correct level at the data out
pin(s).

There are two ways to handle the situation:

1. Reduce the controller clock to the amount where the sampling
   point requirement of the client is still met, meaning the
   clock period being greater than two times tCLQV.

2. If the host controller supports it, compensate by delaying the
   sampling point to meet the tCLQV requirement of the client.

Add a parameter 'rx_sampling_delay_ns' in struct spi_device to enable
setting the clock to RX data delay for each SPI device, so drivers
can check and act upon this timing requirement.

Signed-off-by: Eberhard Stoll <eberhard.stoll@kontron.de>
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 include/linux/spi/spi.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index af7cfee7b8f60..4f8a0c28e1d46 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -181,6 +181,7 @@ extern void spi_transfer_cs_change_delay_exec(struct spi_message *msg,
  * @num_tx_lanes: Number of transmit lanes wired up.
  * @rx_lane_map: Map of peripheral lanes (index) to controller lanes (value).
  * @num_rx_lanes: Number of receive lanes wired up.
+ * @rx_sampling_delay_ns: spi clk to spi rx data delay
  *
  * A @spi_device is used to interchange data between an SPI target device
  * (usually a discrete chip) and CPU memory.
@@ -235,6 +236,8 @@ struct spi_device {
 	struct spi_delay	cs_setup;
 	struct spi_delay	cs_hold;
 	struct spi_delay	cs_inactive;
+	/* Transfer characteristics */
+	u32			rx_sampling_delay_ns; /* clk to rx data delay */
 
 	u8			chip_select[SPI_DEVICE_CS_CNT_MAX];
 	u8			num_chipselect;

-- 
2.53.0
Re: [PATCH RFC 1/7] spi: Add 'rx_sampling_delay_ns' parameter for clock to RX delay
Posted by Frank Li 1 month ago
From: Frank Li (AI-BOT) <frank.li@nxp.com>

> + * @rx_sampling_delay_ns: spi clk to spi rx data delay

Comment is too terse. Expand to clarify the unit and purpose:
"clock-to-RX-data delay in nanoseconds; see tCLQV in device datasheets"

> +	/* Transfer characteristics */
> +	u32			rx_sampling_delay_ns; /* clk to rx data delay */

The inline comment duplicates the field name. Remove it or expand the
block comment above to explain when/how drivers should use this field.
Also: is u32 the right type? Consider if negative values or larger
ranges are possible, or if this should be `struct spi_delay` for
consistency with cs_setup, cs_hold, cs_inactive above.

> +	struct spi_delay	cs_inactive;
> +	/* Transfer characteristics */

The comment "Transfer characteristics" is vague. Either name it more
specifically (e.g., "RX sampling configuration") or fold it into the
existing block comment at the top of the struct.

---

AI bot review and may be useless.
Re: [PATCH RFC 1/7] spi: Add 'rx_sampling_delay_ns' parameter for clock to RX delay
Posted by Mark Brown 1 month ago
On Tue, Mar 03, 2026 at 04:01:14PM -0500, Frank Li wrote:
> From: Frank Li (AI-BOT) <frank.li@nxp.com>

Please add a human step before sending these reviews.

> > + * @rx_sampling_delay_ns: spi clk to spi rx data delay

> Comment is too terse. Expand to clarify the unit and purpose:
> "clock-to-RX-data delay in nanoseconds; see tCLQV in device datasheets"

I'm not sure restating the unit in the comment as well as the name is
super useful...

> > +	/* Transfer characteristics */
> > +	u32			rx_sampling_delay_ns; /* clk to rx data delay */

> The inline comment duplicates the field name. Remove it or expand the
> block comment above to explain when/how drivers should use this field.
> Also: is u32 the right type? Consider if negative values or larger
> ranges are possible, or if this should be `struct spi_delay` for
> consistency with cs_setup, cs_hold, cs_inactive above.

...especially given the contradictory feedback here, and the comment
about a negative delay is obviously not appropriate.  I'm not convinced
anyone needs an explanation of what to do with the field either.