On Tue, Dec 27, 2022 at 01:04:49PM +0300, Ivan Bornyakov wrote:
> Original busy loop with retries count in mpf_poll_status() is not too
> reliable, as it takes different times on different systems. Replace it
> with read_poll_timeout() macro.
>
> While at it, fix polling stop condition to met function's original
> intention declared in the comment. The issue with original polling stop
> condition is that it stops if any of mask bits is set, while intention
> was to stop if all mask bits is set. This was not noticible because only
> MPF_STATUS_READY is passed as mask argument and it is BIT(1).
>
> Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager")
> Signed-off-by: Ivan Bornyakov <i.bornyakov@xxxxxxxxxxx>
> ---
> drivers/fpga/microchip-spi.c | 24 ++++++++++--------------
> 1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> index e72fedd93a27..8d1d9476d0cc 100644
> --- a/drivers/fpga/microchip-spi.c
> +++ b/drivers/fpga/microchip-spi.c
> @@ -6,6 +6,7 @@
> #include <asm/unaligned.h>
> #include <linux/delay.h>
> #include <linux/fpga/fpga-mgr.h>
> +#include <linux/iopoll.h>
> #include <linux/module.h>
> #include <linux/of_device.h>
> #include <linux/spi/spi.h>
> @@ -33,7 +34,7 @@
>
> #define MPF_BITS_PER_COMPONENT_SIZE 22
>
> -#define MPF_STATUS_POLL_RETRIES 10000
> +#define MPF_STATUS_POLL_TIMEOUT (2 * USEC_PER_SEC)
> #define MPF_STATUS_BUSY BIT(0)
> #define MPF_STATUS_READY BIT(1)
> #define MPF_STATUS_SPI_VIOLATION BIT(2)
> @@ -197,21 +198,16 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr,
> /* Poll HW status until busy bit is cleared and mask bits are set. */
> static int mpf_poll_status(struct mpf_priv *priv, u8 mask)
> {
> - int status, retries = MPF_STATUS_POLL_RETRIES;
> + int ret, status;
>
> - while (retries--) {
> - status = mpf_read_status(priv);
> - if (status < 0)
> - return status;
> -
> - if (status & MPF_STATUS_BUSY)
> - continue;
> -
> - if (!mask || (status & mask))
> - return status;
> - }
Would you mind moving the small comment before the function down to here
and mentioning handling the failure case?
The current implementation is a bit easier to understand than the new
version that's wrapped in read_poll_timeout().
Otherwise, change seems like a good idea.
Acked-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> + ret = read_poll_timeout(mpf_read_status, status,
> + (status < 0) ||
> + ((status & (MPF_STATUS_BUSY | mask)) == mask),
> + if (ret < 0)
> + return ret;
>
> - return -EBUSY;
> + return status;
> }
>
> static int mpf_spi_write(struct mpf_priv *priv, const void *buf, size_t buf_size)
> --
> 2.38.2
>
>
Attachment:
signature.asc
Description: PGP signature
