Re: [PATCH v2] spi: Add FSI-attached SPI controller driver

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Tue, Feb 25, 2020 at 7:08 AM Joel Stanley <joel@xxxxxxxxx> wrote:
> On Mon, 3 Feb 2020 at 22:30, Eddie James <eajames@xxxxxxxxxxxxx> wrote:

...

> > +static int fsi_spi_data_in(u64 in, u8 *rx, int len)
> > +{
> > +       int i;
> > +       int num_bytes = min(len, 8);
> > +
> > +       for (i = 0; i < num_bytes; ++i)
> > +               rx[i] = (u8)(in >> (8 * ((num_bytes - 1) - i)));
> > +
> > +       return num_bytes;
> > +}
> > +
> > +static int fsi_spi_data_out(u64 *out, const u8 *tx, int len)
> > +{
> > +       int i;
> > +       int num_bytes = min(len, 8);
> > +
> > +       *out = 0ULL;
> > +
> > +       for (i = 0; i < num_bytes; ++i)
> > +               *out |= (u64)tx[i] << (8 * (8 - (i + 1)));
>
> Did this work with non-8 byte transfers? I think the second 8 should
> be num_bytes.
>
> The loop requires careful reading to check. I wonder if we could do
> this instead, which eliminates a lot duplicated loads and stores and
> is easier to read:
>
>        uint8_t *outp = (uint8_t *)out;
>
>        for (i = 0; i < num_bytes; ++i) {
>                outp[num_bytes - (i + 1)] = tx[i];
>        }

Have you had a chance to read my review of this? What do you think
about put_unaligned*()/get_unaligned*() instead of above?

> > +       return num_bytes;
> > +}

...

> > +static int fsi_spi_transfer_init(struct fsi_spi *ctx)
> > +{
> > +       int rc;
> > +       bool reset = false;
> > +       unsigned long end;
> > +       u64 seq_state;
> > +       u64 clock_cfg = 0ULL;
> > +       u64 status = 0ULL;
> > +       u64 wanted_clock_cfg = SPI_FSI_CLOCK_CFG_ECC_DISABLE |
> > +               SPI_FSI_CLOCK_CFG_SCK_NO_DEL |
> > +               FIELD_PREP(SPI_FSI_CLOCK_CFG_SCK_DIV, 4);
> > +
> > +       end = jiffies + msecs_to_jiffies(SPI_FSI_INIT_TIMEOUT_MS);
> > +       do {
> > +               if (time_after(jiffies, end))
> > +                       return -ETIMEDOUT;
>
> How tightly does this loop spin?
>
> Should there be a delay inside of it?
>
> > +
> > +               rc = fsi_spi_read_reg(ctx, SPI_FSI_STATUS, &status);
> > +               if (rc)
> > +                       return rc;
> > +
> > +               if (status & (SPI_FSI_STATUS_ANY_ERROR |
> > +                             SPI_FSI_STATUS_TDR_FULL |
> > +                             SPI_FSI_STATUS_RDR_FULL)) {
> > +                       if (reset)
> > +                               return -EIO;
> > +
> > +                       rc = fsi_spi_reset(ctx);
> > +                       if (rc)
> > +                               return rc;
> > +
> > +                       reset = true;
> > +                       continue;
> > +               }
> > +
> > +               seq_state = status & SPI_FSI_STATUS_SEQ_STATE;
> > +       } while (seq_state && (seq_state != SPI_FSI_STATUS_SEQ_STATE_IDLE));
>
> ../drivers/spi/spi-fsi.c: In function ‘fsi_spi_transfer_one_message’:
> ../drivers/spi/spi-fsi.c:363:11: warning: ‘seq_state’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
>   363 |  } while (seq_state && (seq_state != SPI_FSI_STATUS_SEQ_STATE_IDLE));
>       |           ^~~~~~~~~

It's bogus warning, though, I think, easy to fix by reshuffling loop body.

> > +       rc = fsi_spi_read_reg(ctx, SPI_FSI_CLOCK_CFG, &clock_cfg);
> > +       if (rc)
> > +               return rc;
> > +
> > +       if ((clock_cfg & (SPI_FSI_CLOCK_CFG_MM_ENABLE |
> > +                         SPI_FSI_CLOCK_CFG_ECC_DISABLE |
> > +                         SPI_FSI_CLOCK_CFG_MODE |
> > +                         SPI_FSI_CLOCK_CFG_SCK_RECV_DEL |
> > +                         SPI_FSI_CLOCK_CFG_SCK_DIV)) != wanted_clock_cfg)
> > +               rc = fsi_spi_write_reg(ctx, SPI_FSI_CLOCK_CFG,
> > +                                      wanted_clock_cfg);
> > +
> > +       return rc;
> > +}

-- 
With Best Regards,
Andy Shevchenko