Re: [PATCH v3 03/13] spi: parse two-element spi-max-frequency property

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Backwards compatibility breakage for unmodified drivers on updated device trees.
- [Low] Inconsistent fallback for malformed device trees with more than two elements in `spi-max-frequency`.
- [Low] The `base_speed_hz` field is missing from `struct spi_board_info` and is not propagated in `spi_new_device()`.
--

commit 60222d0f7864f9f6bf9e85bd6b2a082b000222bb
Author: Santhosh Kumar K <s-k6@ti.com>

spi: parse two-element spi-max-frequency property

Some SPI controllers support high-speed operating modes that require
controller-side configuration before the device can be driven at its
rated maximum. Extend struct spi_device with a base_speed_hz field and update
of_spi_parse_dt() to parse a two-element spi-max-frequency property.

> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 104279858f567..cdd864a5c136b 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c

[ ... ]

> @@ -2598,9 +2598,20 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>  	 */
>  	spi->cs_index_mask = BIT(0);
>  
> -	/* Device speed */
> -	if (!of_property_read_u32(nc, "spi-max-frequency", &value))
> +	/*
> +	 * Device speed: a single value sets max_speed_hz; two values set
> +	 * base_speed_hz (conservative) and max_speed_hz (maximum after
> +	 * controller-side configuration).
> +	 */
> +	nfreq = of_property_count_u32_elems(nc, "spi-max-frequency");
> +	if (nfreq == 2) {
> +		of_property_read_u32_index(nc, "spi-max-frequency", 0,
> +					   &spi->base_speed_hz);
> +		of_property_read_u32_index(nc, "spi-max-frequency", 1,
> +					   &spi->max_speed_hz);

[Severity: High]
Will this break backward compatibility for unmodified drivers on updated device
trees?

If a two-element property is provided, spi->max_speed_hz receives the higher
speed from the second element. Unmodified drivers often leave xfer->speed_hz
as 0 for default transfers, which the SPI core __spi_validate() defaults to
spi->max_speed_hz.

Would this cause the default transfer to be attempted at the high speed before
the device is actually configured via PHY tuning, breaking communication?

> +	} else if (!of_property_read_u32(nc, "spi-max-frequency", &value)) {

[Severity: Low]
Does this create an inconsistent fallback for malformed device trees?

If spi-max-frequency has 3 or more elements, the check for nfreq == 2 fails
and the code falls through to of_property_read_u32(). This reads the first
element (index 0) into the value variable, which then gets assigned to
spi->max_speed_hz.

This means if nfreq == 2, max_speed_hz gets index 1, but if nfreq > 2, it gets
index 0 instead.

>  		spi->max_speed_hz = value;
> +	}
>  
>  	/* Device CS delays */
>  	of_spi_parse_dt_cs_delay(nc, &spi->cs_setup, "spi-cs-setup-delay-ns");

> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 79513f5941cca..14a0b4c9b2281 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h

[ ... ]

> @@ -191,6 +192,7 @@ struct spi_device {
>  	struct device		dev;
>  	struct spi_controller	*controller;
>  	u32			max_speed_hz;
> +	u32			base_speed_hz;

[Severity: Low]
Should base_speed_hz also be added to struct spi_board_info and propagated in
spi_new_device()?

Currently, board files and other programmatic (non-Device Tree) instantiation
methods cannot configure or utilize this new PHY tuning baseline feature
because the template structure was not updated to include this field.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527175527.2247679-1-s-k6@ti.com?part=3