Re: [PATCH spi-next 11/11] spi: spi-fsl-lpspi: fsl_lpspi_set_bitrate(): don't treat devtype_data->prescale_max == 0 as a special case

[James Clark <james.clark@linaro.org>]

On 16/03/2026 11:19 am, Marc Kleine-Budde wrote:
> Hello James,
> 
> I've no strong feelings about this patch, I can drop it in the v2.
> 
> On 16.03.2026 09:57:15, James Clark wrote:
>> On 16/03/2026 8:39 am, Marc Kleine-Budde wrote:
>>> The i.MX93 is affected by erratum ERR051608, the maximum prescaler value is
>>> 1 not 7 as in the original datasheet.
>>>
>>> On unaffected SoC the maximum prescaler is 7. Commit 783bf5d09f86 ("spi:
>>> spi-fsl-lpspi: limit PRESCALE bit in TCR register") added struct
>>> fsl_lpspi_devtype_data to hold the system dependent prescale_max value.
>>>
>>> Commit 9bbfb1ec959c ("spi: spi-fsl-lpspi: Treat prescale_max == 0 as no
>>> erratum") introduced the special value 0 to signal the default and
>>> corresponding run-time check with a conditional operator.
>>>
>>> To simplify the code, set the prescale_max of imx7ulp_lpspi_devtype_data
>>
>> The whole point of the original change was to not repeat the normal value
>> for every device in the config just because one device has an errata. This
>> new change is exactly the opposite of simplification IMO.
> 
> Interesting point of view.
> 
> My reasoning is: When I look at the fsl_lpspi_set_bitrate() function,
> there's the special case handling with the ternary for the "0".
> 
> I asked myself directly special case, why? Why not directly use the
> value as it's provided by the devtype_data?
> 

Those structs are configuration for different devices though. Having a 7 
in there implies that it's something that is configurable to different 
values. When I was adding a new device that made be crawl through a load 
of datasheets to see why arbitrary values were needed only to find out 
that it's only for one errata where it's 1 and actually every other 
device should be 7.

I tried to change it to a bool "has the errata" or "doesn't have it", as 
that's the extent of the valid configurations that should be exposed. 
But Frank said he preferred a number with 0 as the default. That seemed 
like a slight improvement to me so that's how it ended up like that.

>>> and s32g_lpspi_devtype_data to 7 and in fsl_lpspi_set_bitrate() directly
>>> use the fsl_lpspi->devtype_data->prescale_max.
>>>
>>> On ARM64 this leads to a reduction of the code of 52 bytes:
>>>
>>> | add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-52 (-52)
>>> | Function                                     old     new   delta
>>> | fsl_lpspi_setup_transfer.isra                956     936     -20
>>> | $x                                          7144    7112     -32
> 
>> On clang 15 this change makes it bigger by 220 bytes rather than
>> smaller.
> 
> Quite interesting, with llvm-21 the size increases, too:
> 
> | add/remove: 0/0 grow/shrink: 3/1 up/down: 432/-32 (400)
> | Function                                     old     new   delta
> | fsl_lpspi_setup_transfer                    1072    1216    +144
> | .Ltmp1                                      7116    7260    +144
> | $x                                          7196    7340    +144
> | $d                                          5659    5627     -32
> | Total: Before=28126, After=28526, chg +1.42%
> 
>> I
>> checked because I was suspicious that removing a single ternary could save
>> 52 bytes, but it looks like the outcome after all the optimisations is just
>> unpredictable so there isn't much point in making changes like this.
>>
>> You also didn't say _why_ we need to save 50 bytes, or what the performance
>> impact is.
> 
> I just checked the impact on the code size with gcc and was positively
> surprised. :) I don't need to save 50 bytes and there's probably no
> measurable performance impact.
> 
> regards,
> Marc
>