Re: [PATCH v5 1/2] spi: support inter-word delay requirement for devices

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Jonas,

On Tue, Jan 29, 2019 at 9:55 PM Jonas Bonn <jonas@xxxxxxxxxxx> wrote:
> Some devices are slow and cannot keep up with the SPI bus and therefore
> require a short delay between words of the SPI transfer.
>
> The example of this that I'm looking at is a SAMA5D2 with a minimum SPI
> clock of 400kHz talking to an AVR-based SPI slave.  The AVR cannot put
> bytes on the bus fast enough to keep up with the SoC's SPI controller
> even at the lowest bus speed.
>
> This patch introduces the ability to specify a required inter-word
> delay for SPI devices.  It is up to the controller driver to configure
> itself accordingly in order to introduce the requested delay.
>
> Note that, for spi_transfer, there is already a field word_delay that
> provides similar functionality.  This field, however, is specified in
> clock cycles (and worse, SPI controller cycles, not SCK cycles); that
> makes this value dependent on the master clock instead of the device
> clock for which the delay is intended to provide some relief.  This
> patch leaves this old word_delay in place and provides a time-based
> word_delay_us alongside it; the new field fits in the struct padding
> so struct size is constant.  There is only one in-kernel user of the
> word_delay field and presumably that driver could be reworked to use
> the time-based value instead.

Thanks for your patch!

> The time-based delay is limited to 8 bits as these delays are intended
> to be short.  The SAMA5D2 that I've tested this on limits delays to a
> maximum of ~100us, which is already many word-transfer periods even at
> the minimum transfer speed supported by the controller.

Still, the similar delay_usecs uses a u16.

> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h

> @@ -803,6 +808,7 @@ struct spi_transfer {
>  #define        SPI_NBITS_DUAL          0x02 /* 2bits transfer */
>  #define        SPI_NBITS_QUAD          0x04 /* 4bits transfer */
>         u8              bits_per_word;
> +       u8              word_delay_us;

us for µs

>         u16             delay_usecs;

usecs for µs

Can we please try to be consistent?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds