Re: [PATCH 3/3] spi: dw: Use the rx-sample-delay-ns value parsed by the core

[Frieder Schrempf <frieder.schrempf@kontron.de>]

On 17.09.26 17:46, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] spi: dw: unable to override controller rx-sample-delay-ns to 0
> --
> 
> commit 32c003f916e2933421d0880f812a21d5785775b7
> Author: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> spi: dw: Use the rx-sample-delay-ns value parsed by the core
> 
> This commit drops the private parsing of the rx-sample-delay-ns property
> and uses the value parsed by the core into spi_device.rx_sample_delay_ns.
> It falls back to the controller-wide default when the device does not ask
> for a delay of its own.
> 
>> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
>> index 206d3f9dd83d4..b86b607cc8179 100644
>> --- a/drivers/spi/spi-dw-core.c
>> +++ b/drivers/spi/spi-dw-core.c
>> @@ -1104,12 +1104,13 @@ static int dw_spi_setup(struct spi_device *spi)
> [ ... ]
>> -		/* Get specific / default rx-sample-delay */
>> -		if (device_property_read_u32(&spi->dev,
>> -					     "rx-sample-delay-ns",
>> -					     &rx_sample_dly_ns) != 0)
>> -			/* Use default controller value */
>> -			rx_sample_dly_ns = dws->def_rx_sample_dly_ns;
>> +		/*
>> +		 * Use the per-device value the core parsed from the peripheral
>> +		 * node, and fall back to the controller-wide default when the
>> +		 * device does not ask for a delay of its own.
>> +		 */
>> +		rx_sample_dly_ns = spi->rx_sample_delay_ns ?:
>> +				   dws->def_rx_sample_dly_ns;
> 
> [Severity: Medium]
> Does this ?: operator prevent configuring an explicit 0 ns delay?
> 
> The previous code used device_property_read_u32() in dw_spi_setup(),
> which allowed an explicit value of 0 to override the controller default:
> 
>     if (device_property_read_u32(...) != 0)
>         rx_sample_dly_ns = dws->def_rx_sample_dly_ns;
> 
> By using the ?: operator, a parsed value of 0 is treated as falsy,
> meaning an explicit rx-sample-delay-ns = <0> in the device tree will
> now fall back to the controller's default instead of overriding it to 0.
> 
> Although the commit message states there are no in-tree users relying
> on this behavior, Device Tree is a stable ABI. Does ignoring the explicit
> override of <0> break backward compatibility for out-of-tree configurations,
> potentially causing SPI RX data corruption on boards that require a 0 ns
> delay?
It is highly unlikely that anyone has an out-of-tree DT with a
controller-level rx-sample-delay-ns set and a chip-level
rx-sample-delay-ns set to zero. I don't think we should continue to
support this configuration that was probably never meant to work like
this, is undocumented and doesn't make sense.