Re: [PATCH v3 2/6] spi: spi-fsl-dspi: Store status directly in cur_msg->status

[Frank Li <Frank.li@nxp.com>]

On Tue, Jun 24, 2025 at 11:35:32AM +0100, James Clark wrote:
> This will allow us to return a status from the interrupt handler in a
> later commit and avoids copying it at the end of
> dspi_transfer_one_message(). For consistency make polling and DMA modes
> use the same mechanism.
>
> Refactor dspi_rxtx() and dspi_poll() to not return -EINPROGRESS because
> this isn't actually a status that was ever returned to the core layer
> but some internal state. Wherever that was used we can look at dspi->len
> instead.
>
> No functional changes intended.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-dspi.c | 68 ++++++++++++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 744dfc561db2..feb29bb92a77 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -591,11 +591,10 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>
>  static void dspi_setup_accel(struct fsl_dspi *dspi);
>
> -static int dspi_dma_xfer(struct fsl_dspi *dspi)
> +static void dspi_dma_xfer(struct fsl_dspi *dspi)
>  {
>  	struct spi_message *message = dspi->cur_msg;
>  	struct device *dev = &dspi->pdev->dev;
> -	int ret = 0;
>
>  	/*
>  	 * dspi->len gets decremented by dspi_pop_tx_pushr in
> @@ -612,14 +611,12 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
>  		message->actual_length += dspi->words_in_flight *
>  					  dspi->oper_word_size;
>
> -		ret = dspi_next_xfer_dma_submit(dspi);
> -		if (ret) {
> +		message->status = dspi_next_xfer_dma_submit(dspi);
> +		if (message->status) {
>  			dev_err(dev, "DMA transfer failed\n");
>  			break;
>  		}
>  	}
> -
> -	return ret;
>  }
>
>  static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
> @@ -986,36 +983,40 @@ static void dspi_fifo_write(struct fsl_dspi *dspi)
>  				dspi->progress, !dspi->irq);
>  }
>
> -static int dspi_rxtx(struct fsl_dspi *dspi)
> +static void dspi_rxtx(struct fsl_dspi *dspi)
>  {
>  	dspi_fifo_read(dspi);
>
>  	if (!dspi->len)
>  		/* Success! */
> -		return 0;
> +		return;
>
>  	dspi_fifo_write(dspi);
> -
> -	return -EINPROGRESS;
>  }
>
> -static int dspi_poll(struct fsl_dspi *dspi)
> +static void dspi_poll(struct fsl_dspi *dspi)
>  {
>  	int tries = 1000;
>  	u32 spi_sr;
>
> -	do {
> -		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> -		regmap_write(dspi->regmap, SPI_SR, spi_sr);
> +	while (dspi->len) {

Preivous have not checked dspi->len.

Not sure if it is logical equivalence

> +		do {
> +			regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> +			regmap_write(dspi->regmap, SPI_SR, spi_sr);
>
> -		if (spi_sr & SPI_SR_CMDTCF)
> -			break;
> -	} while (--tries);
> +			if (spi_sr & SPI_SR_CMDTCF)
> +				break;
> +		} while (--tries);
>
> -	if (!tries)
> -		return -ETIMEDOUT;
> +		if (!tries) {
> +			dspi->cur_msg->status = -ETIMEDOUT;
> +			return;
> +		}
>
> -	return dspi_rxtx(dspi);
> +		dspi_rxtx(dspi);
> +	}
> +
> +	dspi->cur_msg->status = 0;
>  }
>
>  static irqreturn_t dspi_interrupt(int irq, void *dev_id)
> @@ -1029,8 +1030,13 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>  	if (!(spi_sr & SPI_SR_CMDTCF))
>  		return IRQ_NONE;
>
> -	if (dspi_rxtx(dspi) == 0)
> +	dspi_rxtx(dspi);
> +
> +	if (!dspi->len) {
> +		if (dspi->cur_msg)
> +			WRITE_ONCE(dspi->cur_msg->status, 0);
>  		complete(&dspi->xfer_done);
> +	}
>
>  	return IRQ_HANDLED;
>  }
> @@ -1060,7 +1066,6 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  	struct spi_device *spi = message->spi;
>  	struct spi_transfer *transfer;
>  	bool cs = false;
> -	int status = 0;
>  	u32 val = 0;
>  	bool cs_change = false;
>
> @@ -1120,7 +1125,7 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  				       dspi->progress, !dspi->irq);
>
>  		if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
> -			status = dspi_dma_xfer(dspi);
> +			dspi_dma_xfer(dspi);
>  		} else {
>  			/*
>  			 * Reset completion counter to clear any extra
> @@ -1133,15 +1138,12 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>
>  			dspi_fifo_write(dspi);
>
> -			if (dspi->irq) {
> +			if (dspi->irq)
>  				wait_for_completion(&dspi->xfer_done);
> -			} else {
> -				do {
> -					status = dspi_poll(dspi);
> -				} while (status == -EINPROGRESS);
> -			}
> +			else
> +				dspi_poll(dspi);
>  		}
> -		if (status)
> +		if (READ_ONCE(message->status))

Why need READ_ONCE()? Does any hardware (DMA) set status?

where update message->status at pio mode.

Frank
>  			break;
>
>  		spi_transfer_delay_exec(transfer);
> @@ -1150,7 +1152,8 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  			dspi_deassert_cs(spi, &cs);
>  	}
>
> -	if (status || !cs_change) {
> +	dspi->cur_msg = NULL;
> +	if (message->status || !cs_change) {
>  		/* Put DSPI in stop mode */
>  		regmap_update_bits(dspi->regmap, SPI_MCR,
>  				   SPI_MCR_HALT, SPI_MCR_HALT);
> @@ -1159,10 +1162,9 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>  			;
>  	}
>
> -	message->status = status;
>  	spi_finalize_current_message(ctlr);
>
> -	return status;
> +	return message->status;
>  }
>
>  static int dspi_set_mtf(struct fsl_dspi *dspi)
>
> --
> 2.34.1
>