Re: [PATCH v3 2/6] spi: support simultaneous assertion of multiple CS

[David Lechner]

On 8/17/26 6:32 PM, Jonathan Santos wrote:
> Some SPI controllers allow multiple CS lines to be toggled at the same
> time. The existing code always used CS index 0 when tracking the last
> active CS in spi_set_cs(), and unconditionally set cs_index_mask to
> BIT(0) when parsing DT, both forcing the single CS usage.
> 
> Modify spi_set_cs() to iterate last_cs[] using each logical CS index
> instead of always reading index 0. Modify of_spi_parse_dt() to build
> cs_index_mask from all parsed CS entries rather than hardcoding BIT(0),
> so the controller correctly identifies which CS lines belong to a device
> when asserting them simultaneously.
> 
> Board info, ACPI, and ancillary device paths are not updated here.
> Board info would require an API change to accept an array of CS values
> and is left for a follow-up when we have a use case for this. Ancillary
> devices are by design single-CS, so multi-CS is not a current use case for
> them. ACPI represents the CS as a 64-bit integer with no established
> convention for encoding multiple CS indices yet, so any extension there
> would require a separate specification effort.
> 
> Acked-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@xxxxxxxxxx>
> ---
> Changes in v3:
> * None.
> 
> Changes in v2:
> * Include Summary describind why the other SPI paths were not addressed
>   here.
> ---
>  drivers/spi/spi.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index d9e6b4b87c89..55fb96fea243 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1090,7 +1090,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
>  	spi->controller->last_cs_index_mask = spi->cs_index_mask;
>  	for (idx = 0; idx < SPI_DEVICE_CS_CNT_MAX; idx++) {
>  		if (enable && idx < spi->num_chipselect)
> -			spi->controller->last_cs[idx] = spi_get_chipselect(spi, 0);
> +			spi->controller->last_cs[idx] = spi_get_chipselect(spi, idx);
>  		else
>  			spi->controller->last_cs[idx] = SPI_INVALID_CS;
>  	}
> @@ -2594,10 +2594,11 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>  		spi_set_chipselect(spi, idx, cs[idx]);
>  
>  	/*
> -	 * By default spi->chip_select[0] will hold the physical CS number,
> -	 * so set bit 0 in spi->cs_index_mask.
> +	 * Set cs_index_mask to indicate which logical CS indices are active.
> +	 * Each bit corresponds to a logical CS index in the spi->chip_select array.
>  	 */
> -	spi->cs_index_mask = BIT(0);
> +	for (idx = 0; idx < rc; idx++)
> +		spi->cs_index_mask |= BIT(idx);
>  
>  	/* Device speed */
>  	if (!of_property_read_u32(nc, "spi-max-frequency", &value))

I have the same concern that sashiko calls out here.

Existing users of multi-cs (not including spi-mem) follow the pattern
that a SPI device gets registered with the CS at index 0 and they later
create an auxiliary using the additional CS. This would cause the main
device to now assert both CS. Not what we want to happen.

If I understood (and remember) the previous explanations of this series
correctly, we have a different case for this one.

We want a main device that acts as a single composite device that asserts
all 4 CS at the same time. Then we also need 4 auxiliary devices that
only assert one CS at a time for configuring the individual chips.

So it seems to me like we need a new DT property or some way to be able to
tell the difference to decide whether we just use the first CS here or all
of them.

Perhaps another possibility would be to leave this code the way it is and
do it this way instead:
- The SPI device passed to the IIO driver is just the first chip (one CS)
- The IIO driver then registers auxiliary drivers for the other 3 chips
  (also 1 CS each)
- These 4 devices will be used individual to handle configuration.
- The IIO driver registers a separate composite device that has the
  multiple lanes and and multiple CS. Whether this using the same
  auxiliary mechanism with additional parameters or something new
  probably doesn't matter too much.
- In this way of doing things, it would not make sense to have the
  spi-rx-bus-width property in the devcietree since as far as the
  devicetree is concerned, these are more-or-less 4 separate devices
  (from the SPI point of view).  Instead, this new composite device
  registration function would be the one setting the number of lanes
  based on the number of chip selects.

---
sashiko output for reference:

--- Patch [2]: [PATCH v3 2/6] spi: support simultaneous assertion of multiple CS ---
commit 17290644299c39d6ca397b14180cd885c5b04919
Author: Jonathan Santos <Jonathan.Santos@xxxxxxxxxx>
Subject: spi: support simultaneous assertion of multiple CS

This commit modifies spi_set_cs() and of_spi_parse_dt() to allow
simultaneous toggling of multiple chip select lines. It builds
cs_index_mask from all parsed CS entries instead of hardcoding
bit 0.

> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index d9e6b4b87c899..55fb96fea2432 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c

[ ... ]

> @@ -2594,10 +2594,11 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>  		spi_set_chipselect(spi, idx, cs[idx]);
>  
>  	/*
> -	 * By default spi->chip_select[0] will hold the physical CS number,
> -	 * so set bit 0 in spi->cs_index_mask.
> +	 * Set cs_index_mask to indicate which logical CS indices are active.
> +	 * Each bit corresponds to a logical CS index in the spi->chip_select array.
>  	 */
> -	spi->cs_index_mask = BIT(0);
> +	for (idx = 0; idx < rc; idx++)
> +		spi->cs_index_mask |= BIT(idx);

Will this loop break backward compatibility for devices that use multiple
reg entries for independent logical chip selects?

For example, if a device has multiple reg entries and uses
spi_new_ancillary_device(), the primary device's cs_index_mask will now have
multiple bits set. During a transfer on the primary device, all of these
parsed chip selects would be asserted simultaneously.

Could this result in multiple chips driving the shared MISO line at the same
time, causing bus contention and data corruption?

>  
>  	/* Device speed */
>  	if (!of_property_read_u32(nc, "spi-max-frequency", &value))