Re: [PATCH v2] spi: Raise limit on number of chip selects

Re: [PATCH v2] spi: Raise limit on number of chip selects

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]

On Thu, Jun 26, 2025 at 04:58:20PM +0000, Hohn, Torben wrote:
> Hello Marc,

That isn't my name...

> +#define SPI_CS_CNT_MAX 16

> If this is increased to 24 now, we need to carry another patch on top of mainline again once we add another Chipselect
> into our FPGA, or into the next iteration of our hardware. We would really prefer that a Kconfig value is used.
> We have handed a patch to pengutronix, because they can send proper emails.

> In the IIO framework there is a Konfig Value for something similar:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/iio/trigger.h#n74

This doesn't really work, we're supposed to support single kernel image
so putting per platform configuration in Kconfig ends up being at best a
usability problem.  At some point it's better to just bite the bullet
and make things dynamic.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

AW: [PATCH v2] spi: Raise limit on number of chip selects

[Hohn, Torben]

Hello Mark,


-Bruker Confidential-
> On Thu, Jun 26, 2025 at 04:58:20PM +0000, Hohn, Torben wrote:
> > Hello Marc,
>
> That isn't my name...

Sorry for that. I was also communicating with Marc Kleine-Budde and got confused.

>
> > +#define SPI_CS_CNT_MAX 16
>
> > If this is increased to 24 now, we need to carry another patch on top of mainline again once we add another Chipselect
> > into our FPGA, or into the next iteration of our hardware. We would really prefer that a Kconfig value is used.
> > We have handed a patch to pengutronix, because they can send proper emails.

> > In the IIO framework there is a Konfig Value for something similar:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/iio/trigger.h#n74
>
> This doesn't really work, we're supposed to support single kernel image
> so putting per platform configuration in Kconfig ends up being at best a
> usability problem.  At some point it's better to just bite the bullet
> and make things dynamic.

After looking a bit more throughly at the code, i dont think it is necessary to make this dynamic.
The Value at hand is actually the number of Chipselects a Device might have and not the the maximum number
of Chipselects a Controller might have.

However, the check that is failing is this one:

if (ctlr->num_chipselect > SPI_CS_CNT_MAX) {
                dev_err(&ctlr->dev, "No. of CS is more than max. no. of supported CS\n");
                return -EINVAL;
}

I do now believe, that the proper fix is to remove this check, and then reduce SPI_CS_CNT_MAX to 4 again.
Because it was increased to 16 only to satify this condition here. The code however does not depend on ctrl->num_chipselects being smaller
than SPI_CS_CNT_MAX. The only occurence of this in struct spi_controller is the last_cs stuff, which just requires array sizes of the maximum
number a device might have.

I guess the define should get a better name that makes this relationship more clear. like SPI_CS_CNT_MAX_PER_DEV or something ?
We have discovered the b4 web submit and are now able to post proper patches. Will do that once we have tested this here.

Regards,
Torben

Re: AW: [PATCH v2] spi: Raise limit on number of chip selects

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1911 bytes --]

On Mon, Jul 07, 2025 at 07:30:29AM +0000, Hohn, Torben wrote:

> I do now believe, that the proper fix is to remove this check, and then reduce SPI_CS_CNT_MAX to 4 again.
> Because it was increased to 16 only to satify this condition here. The code however does not depend on ctrl->num_chipselects being smaller
> than SPI_CS_CNT_MAX. The only occurence of this in struct spi_controller is the last_cs stuff, which just requires array sizes of the maximum
> number a device might have.

Ah, good - thanks for checking.  That does make sense, I didn't look
properly (it's been quite a while since I looked at this code) but we
seem to mostly be writing to last_cs[] in the controller and not reading
from it in the normal path.  The main use is to avoid duplicate and
invalid device registrations which we should do but there will be a
better way, and even if we use an array the use is localised enough that
we should readily be able to support dynamically sizing for the
controllers.

> I guess the define should get a better name that makes this relationship more clear. like SPI_CS_CNT_MAX_PER_DEV or something ?
> We have discovered the b4 web submit and are now able to post proper patches. Will do that once we have tested this here.

Yeah, the naming should be clearer.  Off the top of my head
SPI_MAX_DEV_CS or something.  We do also use it to size an array in the
controller which ought to be bigger, something like _MAX_CTRL_CS or
soemthing, and has a more pressing need to be dynamic since you can add
GPIO chip selects to most controllers fairly easily (even if the board
design issues get impractical).  That's just a first pass at naming, I'm
not attached to my option and your idea isn't the end of the world
either.  Devices are more common than controllers and should have a
lower limit if there is a limit so just detaching the two counts would
get us a long way.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: AW: [PATCH v2] spi: Raise limit on number of chip selects

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

On 07.07.2025 07:30:29, Hohn, Torben wrote:
> > > +#define SPI_CS_CNT_MAX 16
> >
> > > If this is increased to 24 now, we need to carry another patch on top of mainline again once we add another Chipselect
> > > into our FPGA, or into the next iteration of our hardware. We would really prefer that a Kconfig value is used.
> > > We have handed a patch to pengutronix, because they can send proper emails.
> 
> > > In the IIO framework there is a Konfig Value for something similar:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/iio/trigger.h#n74
> >
> > This doesn't really work, we're supposed to support single kernel image
> > so putting per platform configuration in Kconfig ends up being at best a
> > usability problem.  At some point it's better to just bite the bullet
> > and make things dynamic.
> 
> After looking a bit more throughly at the code, i dont think it is
> necessary to make this dynamic. The Value at hand is actually the
> number of Chipselects a Device might have and not the the maximum
> number of Chipselects a Controller might have.

I think it's both. The struct spi_controller uses SPI_CS_CNT_MAX:

| struct spi_controller {
[...]
| 	s8				last_cs[SPI_CS_CNT_MAX];
| 	u32				last_cs_index_mask : SPI_CS_CNT_MAX;

See discussion
https://lore.kernel.org/all/49b52941-6205-48bd-b2ae-e334018ac5cd@sirena.org.uk/
for more details.

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

AW: AW: [PATCH v2] spi: Raise limit on number of chip selects

[Hohn, Torben]

-Bruker Confidential-
> On 07.07.2025 07:30:29, Hohn, Torben wrote:
> > > > +#define SPI_CS_CNT_MAX 16
> > >
> > > > If this is increased to 24 now, we need to carry another patch on top of mainline again once we add another Chipselect
> > > > into our FPGA, or into the next iteration of our hardware. We would really prefer that a Kconfig value is used.
> > > > We have handed a patch to pengutronix, because they can send proper emails.
> >
> > > > In the IIO framework there is a Konfig Value for something similar:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/iio/trigger.h#n74
> > >
> > > This doesn't really work, we're supposed to support single kernel image
> > > so putting per platform configuration in Kconfig ends up being at best a
> > > usability problem.  At some point it's better to just bite the bullet
> > > and make things dynamic.
> >
> > After looking a bit more throughly at the code, i dont think it is
> > necessary to make this dynamic. The Value at hand is actually the
> > number of Chipselects a Device might have and not the the maximum
> > number of Chipselects a Controller might have.

> I think it's both. The struct spi_controller uses SPI_CS_CNT_MAX:
>
> | struct spi_controller {
> [...]
> |       s8                              last_cs[SPI_CS_CNT_MAX];
> |       u32                             last_cs_index_mask : SPI_CS_CNT_MAX;

This value saves that last chipselect that was active.
Before the multi-cs per device patch, this was a single u8 and no array.
The mask just states which element of last_cs is valid.
It is in struct spi_controller but since it just saves the value from one device. It still just the per device value, and unrelated to the
number of chipselects per device.
And the last_cs ccount an not be more than the maximum number of CS per device.


> See discussion
> https://lore.kernel.org/all/49b52941-6205-48bd-b2ae-e334018ac5cd@sirena.org.uk/
> for more details.


yeah... see:
https://lore.kernel.org/all/CAOiHx=mM7kpzR-MOshsgXZM+CSB0nawfWxMhpt=tuhmJyMTCzQ@mail.gmail.com/