[v2,16/28] spi: s3c64xx: simplify s3c64xx_wait_for_pio()

s3c64xx_spi_transfer_one() makes sure that for PIO the xfer->len is
always smaller than the fifo size. Since we can't receive more that the
FIFO size, droop the loop handling, the code becomes less misleading.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 75 +++++++++------------------------------
 1 file changed, 17 insertions(+), 58 deletions(-)

On 1/25/24 20:43, Sam Protsenko wrote:
> On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> s3c64xx_spi_transfer_one() makes sure that for PIO the xfer->len is
>> always smaller than the fifo size. Since we can't receive more that the
>> FIFO size, droop the loop handling, the code becomes less misleading.
> 
> Drop (spelling)?

oh yeah, thanks.

> 
> For the patch: how exactly it was tested to make sure there is no regression?

no regression testing for the entire patch set, I have just a gs101 on
my hands.

However, we shouldn't refrain ourselves on improving things when we
think they're straight forward and they worth it. In this particular
case, for PIO, s3c64xx_spi_transfer_one() does:
	xfer->len = fifo_len - 1;
then in s3c64xx_enable_datapath() we write xfer->len and then in
s3c64xx_wait_for_pio() we code did the following:
	loops = xfer->len / FIFO_DEPTH(sdd);
loops is always zero, this is bogus and we shall remove it.

>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  drivers/spi/spi-s3c64xx.c | 75 +++++++++------------------------------
>>  1 file changed, 17 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index d2dd28ff00c6..00a0878aeb80 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -485,26 +485,6 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>>         return 0;
>>  }
>>
>> -static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
>> -                                       int timeout_ms)
>> -{
>> -       void __iomem *regs = sdd->regs;
>> -       unsigned long val = 1;
>> -       u32 status;
>> -       u32 max_fifo = FIFO_DEPTH(sdd);
>> -
>> -       if (timeout_ms)
>> -               val = msecs_to_loops(timeout_ms);
>> -
>> -       do {
>> -               status = readl(regs + S3C64XX_SPI_STATUS);
>> -       } while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < max_fifo &&
>> -                --val);
>> -
>> -       /* return the actual received data length */
>> -       return FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status);
>> -}
>> -
>>  static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
>>                                 struct spi_transfer *xfer)
>>  {
>> @@ -553,13 +533,11 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
>>                                 struct spi_transfer *xfer, bool use_irq)
>>  {
>>         void __iomem *regs = sdd->regs;
>> +       u8 *buf = xfer->rx_buf;
>> +       unsigned long time_us;
>>         unsigned long val;
>> -       u32 status;
>> -       int loops;
>> -       u32 cpy_len;
>> -       u8 *buf;
>> +       u32 status, len;
>>         int ms;
>> -       unsigned long time_us;
>>
>>         /* microsecs to xfer 'len' bytes @ 'cur_speed' */
>>         time_us = (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed;
>> @@ -582,48 +560,29 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
>>                 status = readl(regs + S3C64XX_SPI_STATUS);
>>         } while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < xfer->len &&
>>                  --val);
>> -
>>         if (!val)
>>                 return -EIO;
>>
>>         /* If it was only Tx */
>> -       if (!xfer->rx_buf) {
>> +       if (!buf) {
>>                 sdd->state &= ~TXBUSY;
>>                 return 0;
>>         }
>>
>> -       /*
>> -        * If the receive length is bigger than the controller fifo
>> -        * size, calculate the loops and read the fifo as many times.
>> -        * loops = length / max fifo size (calculated by using the
>> -        * fifo mask).
>> -        * For any size less than the fifo size the below code is
>> -        * executed atleast once.
>> -        */
>> -       loops = xfer->len / FIFO_DEPTH(sdd);
>> -       buf = xfer->rx_buf;
>> -       do {
>> -               /* wait for data to be received in the fifo */
>> -               cpy_len = s3c64xx_spi_wait_for_timeout(sdd,
>> -                                                      (loops ? ms : 0));
>> -
>> -               switch (sdd->cur_bpw) {
>> -               case 32:
>> -                       ioread32_rep(regs + S3C64XX_SPI_RX_DATA,
>> -                                    buf, cpy_len / 4);
>> -                       break;
>> -               case 16:
>> -                       ioread16_rep(regs + S3C64XX_SPI_RX_DATA,
>> -                                    buf, cpy_len / 2);
>> -                       break;
>> -               default:
>> -                       ioread8_rep(regs + S3C64XX_SPI_RX_DATA,
>> -                                   buf, cpy_len);
>> -                       break;
>> -               }
>> +       len = FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status);
>> +
>> +       switch (sdd->cur_bpw) {
>> +       case 32:
>> +               ioread32_rep(regs + S3C64XX_SPI_RX_DATA, buf, len / 4);
>> +               break;
>> +       case 16:
>> +               ioread16_rep(regs + S3C64XX_SPI_RX_DATA, buf, len / 2);
>> +               break;
>> +       default:
>> +               ioread8_rep(regs + S3C64XX_SPI_RX_DATA, buf, len);
>> +               break;
>> +       }
>>
>> -               buf = buf + cpy_len;
>> -       } while (loops--);
>>         sdd->state &= ~RXBUSY;
>>
>>         return 0;
>> --
>> 2.43.0.429.g432eaa2c6b-goog
>>

On Fri, Jan 26, 2024 at 1:56 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> On 1/25/24 20:43, Sam Protsenko wrote:
> > On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >> s3c64xx_spi_transfer_one() makes sure that for PIO the xfer->len is
> >> always smaller than the fifo size. Since we can't receive more that the
> >> FIFO size, droop the loop handling, the code becomes less misleading.
> >
> > Drop (spelling)?
>
> oh yeah, thanks.
>
> >
> > For the patch: how exactly it was tested to make sure there is no regression?
>
> no regression testing for the entire patch set, I have just a gs101 on
> my hands.
>
> However, we shouldn't refrain ourselves on improving things when we
> think they're straight forward and they worth it. In this particular

This patch clearly brings a functional change. The way I see things,
the risk of having a regression outweighs the benefits of this
refactoring. I don't think it's even methodologically right to apply
such changes without thoroughly testing it first. It might be ok for
super-easy one-line cleanups, but that's not one of those.

> case, for PIO, s3c64xx_spi_transfer_one() does:
>         xfer->len = fifo_len - 1;
> then in s3c64xx_enable_datapath() we write xfer->len and then in
> s3c64xx_wait_for_pio() we code did the following:
>         loops = xfer->len / FIFO_DEPTH(sdd);
> loops is always zero, this is bogus and we shall remove it.
>

[snip]