Re: [PATCH 3/6] spi: spi-engine-ex: Add support for multi-CS devices

[Andy Shevchenko]

On Tue, Jul 14, 2026 at 02:56:16AM -0300, Jonathan Santos wrote:
> The AXI SPI Engine controller hardcoded CS index 0 when generating
> assert commands, so only the first chip select was being toggled even
> when a device declared multiple CS lines in the device tree.
> 
> Modify spi_engine_gen_cs() to accept a cs_select_mask argument and
> iterate all bits set in the effective mask (the intersection of
> xfer->cs_select_mask with spi->cs_index_mask, falling back to
> spi->cs_index_mask when the transfer mask is 0). Update
> spi_engine_compile_message() to pass each transfer's cs_select_mask
> at every CS transition, including cs_change and cs_select_mask
> change boundaries between consecutive transfers.
> 
> Likewise, modify spi_engine_setup() replacing the single-index CS
> assert with a for_each_set_bit() loop over cs_index_mask.
> 
> Set SPI_CONTROLLER_MULTI_CS flag in the probe path so the core
> multi-CS path in spi_set_cs() is activated for this controller.

...

>  static void spi_engine_gen_cs(struct spi_engine_program *p, bool dry,
> -		struct spi_device *spi, bool assert)
> +		struct spi_device *spi, bool assert, unsigned long xfer_cs_mask)
>  {
> +	unsigned long cs_index_mask = !xfer_cs_mask
> +		? spi->cs_index_mask
> +		: spi->cs_index_mask & xfer_cs_mask;

This is an interesting style. Also, why negative conditional? It's harder
to parse. First, split the assignment and the definition...

	unsigned long cs_index_mask;

>  	unsigned int mask = 0xff;
> +	unsigned int cs_bit;

...and then, for example,

	cs_index_mask = xfer_cs_mask ? spi->cs_index_mask & xfer_cs_mask :
				       spi->cs_index_mask;

>  	if (assert)
> -		mask ^= BIT(spi_get_chipselect(spi, 0));
> +		for_each_set_bit(cs_bit, &cs_index_mask, SPI_ENGINE_MAX_CS)
> +			mask ^= BIT(spi_get_chipselect(spi, cs_bit));

This requires {} now.

>  
>  	spi_engine_program_add_cmd(p, dry, SPI_ENGINE_CMD_ASSERT(0, mask));
>  }

...

> static void spi_engine_compile_message(struct spi_message *msg, bool dry,

>  		spi_engine_gen_sleep(p, dry, spi_delay_to_ns(&xfer->delay, xfer),
>  				     inst_ns, xfer->effective_speed_hz);
>  
> +		struct spi_transfer *next_xfer = list_next_entry(xfer, transfer_list);

Define the variable at the top of the scope.

>  		if (xfer->cs_change) {
>  			if (list_is_last(&xfer->transfer_list, &msg->transfers)) {
>  				keep_cs = true;
>  			} else {
>  				if (!xfer->cs_off)
> -					spi_engine_gen_cs(p, dry, spi, false);
> +					spi_engine_gen_cs(p, dry, spi, false, xfer->cs_select_mask);
>  
>  				spi_engine_gen_sleep(p, dry, spi_delay_to_ns(
>  					&xfer->cs_change_delay, xfer), inst_ns,
>  					xfer->effective_speed_hz);
>  
> -				if (!list_next_entry(xfer, transfer_list)->cs_off)
> -					spi_engine_gen_cs(p, dry, spi, true);
> +				if (!next_xfer->cs_off)
> +					spi_engine_gen_cs(p, dry, spi, true,
> +							  next_xfer->cs_select_mask);
>  			}
>  		} else if (!list_is_last(&xfer->transfer_list, &msg->transfers) &&
> -			   xfer->cs_off != list_next_entry(xfer, transfer_list)->cs_off) {
> -			spi_engine_gen_cs(p, dry, spi, xfer->cs_off);
> +			   xfer->cs_off != next_xfer->cs_off) {
> +			spi_engine_gen_cs(p, dry, spi, xfer->cs_off, xfer->cs_select_mask);
> +		} else if (!list_is_last(&xfer->transfer_list, &msg->transfers) &&
> +			   xfer->cs_select_mask != next_xfer->cs_select_mask) {
> +			spi_engine_gen_cs(p, dry, spi, true, next_xfer->cs_select_mask);
>  		}
>  	}

...

> static int spi_engine_setup(struct spi_device *device)
>  {
>  	struct spi_controller *host = device->controller;
>  	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
> +	unsigned long cs_index_mask = device->cs_index_mask;
>  	unsigned int reg;
> +	u32 cs_bit;
>  
> -	if (device->mode & SPI_CS_HIGH)
> -		spi_engine->cs_inv |= BIT(spi_get_chipselect(device, 0));
> -	else
> -		spi_engine->cs_inv &= ~BIT(spi_get_chipselect(device, 0));
> +	for_each_set_bit(cs_bit, &cs_index_mask, SPI_ENGINE_MAX_CS) {
> +		if (device->mode & SPI_CS_HIGH)
> +			spi_engine->cs_inv |= BIT(spi_get_chipselect(device,
> +								     cs_bit));

It will be easier to follow when a single line.

> +		else
> +			spi_engine->cs_inv &= ~BIT(spi_get_chipselect(device,
> +								      cs_bit));

Ditto.

> +	}

-- 
With Best Regards,
Andy Shevchenko