Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 9 May 2022 20:16:21 +0300
From:   Ivan Bornyakov <i.bornyakov@metrotek.ru>
To:     Conor.Dooley@microchip.com
Cc:     mdf@kernel.org, hao.wu@intel.com, yilun.xu@intel.com,
        trix@redhat.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-fpga@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        system@metrotek.ru
Subject: Re: [PATCH v11 2/3] fpga: microchip-spi: add Microchip MPF FPGA
 manager
Message-ID: <20220509171621.zk4owxwlngxjodgz@x260>
References: <20220507074304.11144-1-i.bornyakov@metrotek.ru>
 <20220507074304.11144-3-i.bornyakov@metrotek.ru>
 <bd5cb37b-ee56-f6d5-2d98-c08566b60728@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <bd5cb37b-ee56-f6d5-2d98-c08566b60728@microchip.com>
Precedence: bulk

On Mon, May 09, 2022 at 11:41:18AM +0000, Conor.Dooley@microchip.com 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?