Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers

[david laight <david.laight@runbox.com>]

On Wed, 26 Nov 2025 08:54:40 +0100
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Make both handlers to be shorter and easier to understand.
> While at it, unify their style.

The read code looks dubious... (nothing new though)

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/spi/spi-microchip-core-spi.c | 32 +++++++++-------------------
>  1 file changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/spi/spi-microchip-core-spi.c b/drivers/spi/spi-microchip-core-spi.c
> index 08ccdc5f0cc9..f8184b711272 100644
> --- a/drivers/spi/spi-microchip-core-spi.c
> +++ b/drivers/spi/spi-microchip-core-spi.c
> @@ -91,21 +91,14 @@ static inline void mchp_corespi_disable(struct mchp_corespi *spi)
>  static inline void mchp_corespi_read_fifo(struct mchp_corespi *spi, u32 fifo_max)
>  {
>  	for (int i = 0; i < fifo_max; i++) {
> -		u32 data;
> -
>  		while (readb(spi->regs + MCHP_CORESPI_REG_STAT) &
>  		       MCHP_CORESPI_STATUS_RXFIFO_EMPTY)
>  			;

This is a hard spin until data is available.
I think 'spi' is a bit like 'i2c' (etc) so is quite slow.
Even if the code thinks there are 'fifo_max' bytes in the fifo it seems
wrong to spin indefinitely.
If the code is trying to read a response that is still arriving from the
physical hardware is is positively wrong.
If some hardware has a glitch that FIFO_EMPTY is temporarily incorrectly set
after a read - then maybe you need some recovery code.
Otherwise I suspect FIFO_EMPTY should generate a short read.

The write code (below) does an 'early terminate'' on fifo full.
Presumably it is woken by an interrupt to continue the write?

I actually doubt that transferring messages that are larger than the
device fifo is ever going to be completely reliable.
You'd need to guarantee the interrupt latency to update the fifo be short
enough to guarantee the fifo won't underflow/overflow.
(Unless the spi hardware 'clock stops' the physical interface when the fifo
if full/empty - which is effectively what happens when software 'bit-bangs'
these serial interfaces.)

>  
> -		data = readb(spi->regs + MCHP_CORESPI_REG_RXDATA);
> +		if (spi->rx_buf)
> +			*spi->rx_buf++ = readb(spi->regs + MCHP_CORESPI_REG_RXDATA);
>  
>  		spi->rx_len--;
> -		if (!spi->rx_buf)
> -			continue;
> -
> -		*spi->rx_buf = data;
> -
> -		spi->rx_buf++;
>  	}
>  }
>  
> @@ -127,23 +120,18 @@ static void mchp_corespi_disable_ints(struct mchp_corespi *spi)
>  
>  static inline void mchp_corespi_write_fifo(struct mchp_corespi *spi, u32 fifo_max)
>  {
> -	int i = 0;
> -
> -	while ((i < fifo_max) &&
> -	       !(readb(spi->regs + MCHP_CORESPI_REG_STAT) &
> -		 MCHP_CORESPI_STATUS_TXFIFO_FULL)) {
> -		u32 word;
> -
> -		word = spi->tx_buf ? *spi->tx_buf : 0xaa;
> -		writeb(word, spi->regs + MCHP_CORESPI_REG_TXDATA);
> +	for (int i = 0; i < fifo_max; i++) {

unsigned int or u32 ??

> +		if (readb(spi->regs + MCHP_CORESPI_REG_STAT) &
> +		    MCHP_CORESPI_STATUS_TXFIFO_FULL)
> +			break;
>  
>  		if (spi->tx_buf)
> -			spi->tx_buf++;
> +			writeb(*spi->tx_buf++, spi->regs + MCHP_CORESPI_REG_TXDATA);
> +		elsespi->regs + MCHP_CORESPI_REG_TXDATA);
> +			writeb(0xaa, spi->regs + MCHP_CORESPI_REG_TXDATA);

I'm not sure I don't prefer the version with one writeb() call.
How about:
		writeb(spi->tx_buf ? *spi->tx_buf++ : 0xaa,
			spi->regs + MCHP_CORESPI_REG_TXDATA);

	David
			
>  
> -		i++;
> +		spi->tx_len--;
>  	}
> -
> -	spi->tx_len -= i;
>  }
>  
>  static void mchp_corespi_set_cs(struct spi_device *spi, bool disable)