[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240429131600.4e910d8a@donnerap.manchester.arm.com>
Date: Mon, 29 Apr 2024 13:16:00 +0100
From: Andre Przywara <andre.przywara@....com>
To: Wolfram Sang <wsa+renesas@...g-engineering.com>
Cc: linux-spi@...r.kernel.org, Mark Brown <broonie@...nel.org>, Chen-Yu Tsai
<wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland
<samuel@...lland.org>, linux-arm-kernel@...ts.infradead.org,
linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/8] spi: sun6i: use 'time_left' variable with
wait_for_completion_timeout()
On Mon, 29 Apr 2024 13:28:40 +0200
Wolfram Sang <wsa+renesas@...g-engineering.com> wrote:
Hi,
> There is a confusing pattern in the kernel to use a variable named 'timeout' to
> store the result of wait_for_completion_timeout() causing patterns like:
>
> timeout = wait_for_completion_timeout(...)
> if (!timeout) return -ETIMEDOUT;
>
> with all kinds of permutations. Use 'time_left' as a variable to make the code
> self explaining.
>
> Fix to the proper variable type 'unsigned long' while here.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@...g-engineering.com>
> ---
> drivers/spi/spi-sun6i.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> index cd018ea1abf1..7bbe7ea0d66a 100644
> --- a/drivers/spi/spi-sun6i.c
> +++ b/drivers/spi/spi-sun6i.c
> @@ -277,7 +277,8 @@ static int sun6i_spi_transfer_one(struct spi_controller *host,
> struct spi_transfer *tfr)
> {
> struct sun6i_spi *sspi = spi_controller_get_devdata(host);
> - unsigned int div, div_cdr1, div_cdr2, timeout;
> + unsigned int div, div_cdr1, div_cdr2;
> + unsigned long time_left;
> unsigned int start, end, tx_time;
> unsigned int trig_level;
> unsigned int tx_len = 0, rx_len = 0, nbits = 0;
> @@ -488,26 +489,26 @@ static int sun6i_spi_transfer_one(struct spi_controller *host,
>
> tx_time = spi_controller_xfer_timeout(host, tfr);
> start = jiffies;
> - timeout = wait_for_completion_timeout(&sspi->done,
> - msecs_to_jiffies(tx_time));
> + time_left = wait_for_completion_timeout(&sspi->done,
> + msecs_to_jiffies(tx_time));
>
> if (!use_dma) {
> sun6i_spi_drain_fifo(sspi);
> } else {
> - if (timeout && rx_len) {
> + if (time_left && rx_len) {
> /*
> * Even though RX on the peripheral side has finished
> * RX DMA might still be in flight
> */
> - timeout = wait_for_completion_timeout(&sspi->dma_rx_done,
> - timeout);
> - if (!timeout)
> + time_left = wait_for_completion_timeout(&sspi->dma_rx_done,
> + time_left);
Nit: indentation is off here. Regardless:
Reviewed-by: Andre Przywara <andre.przywara@....com>
Cheers,
Andre
> + if (!time_left)
> dev_warn(&host->dev, "RX DMA timeout\n");
> }
> }
>
> end = jiffies;
> - if (!timeout) {
> + if (!time_left) {
> dev_warn(&host->dev,
> "%s: timeout transferring %u bytes@...z for %i(%i)ms",
> dev_name(&spi->dev), tfr->len, tfr->speed_hz,
Powered by blists - more mailing lists