Re: [PATCH v11 2/3] fpga: microchip-spi: add Microchip MPF FPGA manager

[Ivan Bornyakov]

On Mon, May 09, 2022 at 11:41:18AM +0000, Conor.Dooley@xxxxxxxxxxxxx wrote:
> Hey Ivan, one comment below.
> Thanks,
> Conor.
> 
> On 07/05/2022 08:43, Ivan Bornyakov wrote:
> > ... snip ...
> > +static int mpf_read_status(struct spi_device *spi)
> > +{
> > +       u8 status, status_command = MPF_SPI_READ_STATUS;
> > +       struct spi_transfer xfer = {
> > +               .tx_buf = &status_command,
> > +               .rx_buf = &status,
> > +               .len = 1,
> > +       };
> > +       int ret = spi_sync_transfer(spi, &xfer, 1);
> > +
> > +       if ((status & MPF_STATUS_SPI_VIOLATION) ||
> > +           (status & MPF_STATUS_SPI_ERROR))
> > +               ret = -EIO;
> > +
> > +       return ret ? : status;
> > +}
> > +
> > ... snip ...
> > +
> > +static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> > +{
> > +       int status, timeout = MPF_STATUS_POLL_TIMEOUT;
> > +
> > +       while (timeout--) {
> > +               status = mpf_read_status(spi);
> > +               if (status < 0 ||
> > +                   (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask))))
> > +                       return status;
> > +
> > +               usleep_range(1000, 2000);
> > +       }
> > +
> > +       return -EBUSY;
> > +}
> 
> Is there a reason you changed this from the snippet you sent me
> in the responses to version 8:
> static int poll_status_not_busy(struct spi_device *spi, u8 mask)
> {
> 	u8 status, status_command = MPF_SPI_READ_STATUS;
> 	int ret, timeout = MPF_STATUS_POLL_TIMEOUT;
> 	struct spi_transfer xfer = {
> 		.tx_buf = &status_command,
> 		.rx_buf = &status,
> 		.len = 1,
> 	};
> 
> 	while (timeout--) {
> 		ret = spi_sync_transfer(spi, &xfer, 1);
> 		if (ret < 0)
> 			return ret;
> 
> 		if (!(status & MPF_STATUS_BUSY) && (!mask || (status & mask)))
> 			return status;
> 
> 		usleep_range(1000, 2000);
> 	}
> 
> 	return -EBUSY;
> }
> 
> With the current version, I hit the "Failed to write bitstream
> frame" check in mpf_ops_write at random points in the transfer.
> Replacing poll_status_not_busy with the above allows it to run
> to completion.

In my eyes they are equivalent, aren't they?