| Message ID | 20240429112843.67628-8-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: use 'time_left' instead of 'timeout' with wait_for_*() functions | expand |
On Mon, 29 Apr 2024 13:28:40 +0200 Wolfram Sang <wsa+renesas@sang-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@sang-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@arm.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@%iHz for %i(%i)ms", > dev_name(&spi->dev), tfr->len, tfr->speed_hz,
> > - if (!timeout) > > + time_left = wait_for_completion_timeout(&sspi->dma_rx_done, > > + time_left); > > Nit: indentation is off here. Regardless: Oh, right. Thanks, will fix!
Dne ponedeljek, 29. april 2024 ob 13:28:40 GMT +2 je Wolfram Sang napisal(a): > 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@sang-engineering.com> Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com> Best regards, Jernej > --- > 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); > + 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@%iHz for %i(%i)ms", > dev_name(&spi->dev), tfr->len, tfr->speed_hz, >
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); + 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@%iHz for %i(%i)ms", dev_name(&spi->dev), tfr->len, tfr->speed_hz,
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@sang-engineering.com> --- drivers/spi/spi-sun6i.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)