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

[Marc Kleine-Budde]

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?

> > 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

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Attachment: signature.asc
Description: PGP signature