Re: [PATCH v5 1/7] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration

David Lechner
2024-06-26T14:57:35Z

On 6/25/24 4:53 PM, Marcelo Schmitt wrote:
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> 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 */

These two flags above are still not used anywhere and are redundant with
the SPI_MOSI_IDLE_LOW/HIGH flags below so I don't think we should be adding
these.

>
> /* 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 */

The patch series that added SPI_MOSI_IDLE_LOW [1] also added it to spidev. Do
we need to do the same for SPI_MOSI_IDLE_HIGH?

Also, what is the plan for adding these flags to other SPI controllers. For
example, the IMX controller in [1] sounds like it should also support
SPI_MOSI_IDLE_HIGH. And your comments on an earlier version of this series
made it sound like Raspberry Pi is always SPI_MOSI_IDLE_LOW, so should
have that flag.

[1]: https://lore.kernel.org/linux-spi/20230530141641.1155691-1-boerge.struempfel@gmail.com/

>
> /*
> * 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 */