Re: [PATCH RFC 5/7] spi: Add RX sampling point adjustment

Frank Li
2026-03-03T21:02:57Z

From: Frank Li (AI-BOT) <frank.li@nxp.com>

> +unsigned int spi_calc_rx_sampling_point(struct spi_device *spi, unsigned int *freq,
> + u16 max_delay_cycles,
> + u16 delay_cycles_per_clock_cycle)
> +{
> + unsigned long long temp;
> + u16 delay_cycles;
> +
> + /* if sampling delay is zero, we assume the default sampling point can be used */
> + if (!spi->rx_sampling_delay_ns)
> + return 0;
> +
> + temp = *freq * delay_cycles_per_clock_cycle * spi->rx_sampling_delay_ns;
> + do_div(temp, 1000000000UL);
> + delay_cycles = temp;

Potential overflow: multiplying three u32/u16 values before assigning to
u64. Cast operands to u64 before multiplication to avoid intermediate
overflow.

> + if (delay_cycles > max_delay_cycles) {
> + /*
> + * Reduce the clock to the point where the sampling delay requirement
> + * can be met.
> + */
> + delay_cycles = max_delay_cycles;
> + temp = (1000000000UL * delay_cycles);
> + do_div(temp, spi->rx_sampling_delay_ns * delay_cycles_per_clock_cycle);
> + *freq = temp;

Same overflow risk in denominator multiplication. Cast to u64 first.

> + dev_dbg(&spi->controller->dev, "calculated RX sampling point delay: %u cycle(s) at %lu KHz", delay_cycles, *freq / 1000);

Line exceeds 80 chars. Break into two lines.

> +unsigned int spi_set_rx_sampling_point(struct spi_device *spi, unsigned int freq)
> +{
> + if (spi->controller->set_rx_sampling_point)
> + return spi->controller->set_rx_sampling_point(spi, spi->max_speed_hz);
> +
> + return freq;
> +}

Bug: function receives 'freq' parameter but ignores it and passes
'spi->max_speed_hz' to the handler instead. Should pass 'freq' or remove
the parameter.

> + spi->max_speed_hz = spi_set_rx_sampling_point(spi, spi->max_speed_hz);

This call happens after spi_set_cs_timing() check but before the mutex is
unlocked. Verify this is the intended placement and that no locking issues
exist with controller callbacks.

AI bot review and may be useless.