Re: [PATCH 2/2] spi: s3c64xx: update flush_fifo timeout code

[Andi Shyti <andi.shyti@xxxxxxxxxx>]

On Tue, Sep 24, 2024 at 02:40:09PM GMT, Ben Dooks wrote:
> The code that checks for loops in the s3c6xx_flush_fifo() checks
> for loops being non-zero as a timeout, however the code /could/
> finish with loops being zero and the fifo being flushed...

what is the possibility of this case?

> Also, it would be useful to know what is left in the fifo for this
> error case, so update the checks to see what is left, and then also
> print the number of entries.
> 
> Signed-off-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>
> ---
>  drivers/spi/spi-s3c64xx.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 6ab416a33966..7b244e1fd58a 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -247,8 +247,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
>  		val = readl(regs + S3C64XX_SPI_STATUS);
>  	} while (TX_FIFO_LVL(val, sdd) && --loops);
>  
> -	if (loops == 0)
> -		dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n");
> +	if (TX_FIFO_LVL(val, sdd))
> +		dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO (%d left)\n", TX_FIFO_LVL(val, sdd));
>  
>  	/* Flush RxFIFO*/
>  	loops = msecs_to_loops(1);
> @@ -260,8 +260,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
>  			break;
>  	} while (--loops);
>  
> -	if (loops == 0)
> -		dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO\n");
> +	if (RX_FIFO_LVL(val, sdd))
> +		dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO (%d left)\n", RX_FIFO_LVL(val, sdd));

This change doesn't super excite me, but it's fine. Please add a
comment explaining the case when loops is '0' and the FIFO is
flushed.

With the comment given, you can have my r-b.

Thanks,
Andi

>  	val = readl(regs + S3C64XX_SPI_CH_CFG);
>  	val &= ~S3C64XX_SPI_CH_SW_RST;
> -- 
> 2.37.2.352.g3c44437643
>