On Mon, Dec 26, 2022 at 08:24:06PM +0200, Ilpo Järvinen wrote:
> On Mon, 26 Dec 2022, 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.
> >
> > Fixes: 5f8d4a900830 ("fpga: microchip-spi: add Microchip MPF FPGA manager")
> > Signed-off-by: Ivan Bornyakov <i.bornyakov@xxxxxxxxxxx>
> > ---
> > drivers/fpga/microchip-spi.c | 25 +++++++++++--------------
> > 1 file changed, 11 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> > index e72fedd93a27..f3ddfab87499 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,17 @@ 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;
> > - }
> > + ret = read_poll_timeout(mpf_read_status, status,
> > + (status < 0) ||
> > + (!(status & MPF_STATUS_BUSY) &&
> > + (!mask || (status & mask))),
>
> I'm just noting that this code does not match function's comment
> (neither pre nor post diff code). The comment claims "mask bits are set"
> but the code tests "any mask bit set".
>
> However, currently it causes no issue because only MPF_STATUS_READY is
> passed and it's BIT(1).
>
> I think the condition matching to the comment would be this (IMHO, it is
> also slightly simpler because !mask doesn't need to be special cased):
>
> ((status & (MPF_STATUS_BUSY | mask)) == mask)
>
> (But not necessarily to be changed in this patch.)
>
> --
> i.
>
Agree. Thank you for pointing out!
> > + 0, MPF_STATUS_POLL_TIMEOUT, false, priv);
> > + 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)
> >
>
