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]

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