On 6/18/24 6:10 PM, Marcelo Schmitt wrote:
> +
> +MOSI idle state configuration
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Common SPI protocol implementations don't specify any state or behavior for the
> +MOSI line when the controller is not clocking out data. However, there do exist
> +peripherals that require specific MOSI line state when data is not being clocked
> +out. For example, if the peripheral expects the MOSI line to be high when the
> +controller is not clocking out data (SPI_MOSI_IDLE_HIGH), then a transfer in SPI
> +mode 0 would look like the following:
> +
> +::
> +
> + nCSx ___ ___
> + \_________________________________________________________________/
> + • •
> + • •
> + SCLK ___ ___ ___ ___ ___ ___ ___ ___
> + _______/ \___/ \___/ \___/ \___/ \___/ \___/ \___/ \_____
> + • : ; : ; : ; : ; : ; : ; : ; : ; •
> + • : ; : ; : ; : ; : ; : ; : ; : ; •
> + MOSI _____ _______ _______ _______________ ___
> + 0x56 \_0_____/ 1 \_0_____/ 1 \_0_____/ 1 1 \_0_____/
> + • ; ; ; ; ; ; ; ; •
> + • ; ; ; ; ; ; ; ; •
> + MISO XXX__________ _______________________ _______ XXX
> + 0xBA XXX__/ 1 \_____0_/ 1 1 1 \_____0__/ 1 \____0__XXX
> +
> +Legend::
> +
> + • marks the start/end of transmission;
> + : marks when data is clocked into the peripheral;
> + ; marks when data is clocked into the controller;
> + X marks when line states are not specified.
> +
> +In this extension to the usual SPI protocol, the MOSI line state is specified to
> +be kept high when CS is active but the controller is not clocking out data to
I think it would be less ambiguous to say "asserted" instead of "active".
> +the peripheral and also when CS is inactive.
As I mentioned in a previous review, I think the key detail here is that the
MOSI line has to be in the required state during the CS line assertion
(falling edge). I didn't really get that from the current wording. The current
wording makes it sound like MOSI needs to be high indefinitely longer.
> +
> +Peripherals that require this extension must request it by setting the
> +SPI_MOSI_IDLE_HIGH bit into the mode attribute of their struct spi_device and
Could use inline code formatting for C code bits, e.g. ``struct spi_device``
``SPI_MOSI_IDLE_HIGH``, etc.
> +call spi_setup(). Controllers that support this extension should indicate it by> +setting SPI_MOSI_IDLE_HIGH in the mode_bits attribute of their struct
> +spi_controller. The configuration to idle MOSI low is analogous but uses the
> +SPI_MOSI_IDLE_LOW mode bit.
> +
> +
> THANKS TO
> ---------
> Contributors to Linux-SPI discussions include (in alphabetical order,
...
> index e8e1e798924f..8e50a8559225 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -599,6 +599,12 @@ struct spi_controller {
> * assert/de-assert more than one chip select at once.
> */
> #define SPI_CONTROLLER_MULTI_CS BIT(7)
> + /*
> + * The spi-controller is capable of keeping the MOSI line low or high
> + * when not clocking out data.
> + */
> +#define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8) /* Can idle MOSI low */
> +#define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9) /* Can idle MOSI high */
I don't see where these are used anywhere else in the series. They
seem redundant with SPI_MOSI_IDLE_LOW and SPI_MOSI_IDLE_HIGH.
>
> /* Flag indicating if the allocation of this struct is devres-managed */
> bool devm_allocated;
> diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h
> index ca56e477d161..ee4ac812b8f8 100644
> --- a/include/uapi/linux/spi/spi.h
> +++ b/include/uapi/linux/spi/spi.h
> @@ -28,7 +28,8 @@
> #define SPI_RX_OCTAL _BITUL(14) /* receive with 8 wires */
> #define SPI_3WIRE_HIZ _BITUL(15) /* high impedance turnaround */
> #define SPI_RX_CPHA_FLIP _BITUL(16) /* flip CPHA on Rx only xfer */
> -#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave mosi line low when idle */
> +#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave MOSI line low when idle */
> +#define SPI_MOSI_IDLE_HIGH _BITUL(18) /* leave MOSI line high when idle */
>
> /*
> * All the bits defined above should be covered by SPI_MODE_USER_MASK.
> @@ -38,6 +39,6 @@
> * These bits must not overlap. A static assert check should make sure of that.
> * If adding extra bits, make sure to increase the bit index below as well.
> */
> -#define SPI_MODE_USER_MASK (_BITUL(18) - 1)
> +#define SPI_MODE_USER_MASK (_BITUL(19) - 1)
>
> #endif /* _UAPI_SPI_H */
