[v4,2/3] spi: Add SPI_NO_TX/RX support

Commit Message

Alexandru Ardelean Dec. 3, 2020, 2:05 p.m. UTC

From: Dragos Bogdan <[e-mail]>

Transmit/receive only is a valid SPI mode. For example, the MOSI/TX line
might be missing from an ADC while for a DAC the MISO/RX line may be
optional. This patch adds these two new modes: SPI_NO_TX and
SPI_NO_RX. This way, the drivers will be able to identify if any of
these two lines is missing and to adjust the transfers accordingly.

Signed-off-by: Dragos Bogdan <[e-mail]>
Signed-off-by: Alexandru Ardelean <[e-mail]>
---

Changelog v3 -> v4:
* https://lore.kernel.org/linux-spi/[e-mail]/
* moved SPI_NO_TX + SPI_NO_RX to internal spi.h header
* added SPI_MODE_USER_MASK & SPI_MODE_KERNEL_MASK + BUILD_BUG_ON() check

 drivers/spi/spi.c            | 28 +++++++++++++++++++++++-----
 include/linux/spi/spi.h      |  4 ++++
 include/uapi/linux/spi/spi.h |  6 ++++++
 3 files changed, 33 insertions(+), 5 deletions(-)

Comments

Andy Shevchenko Dec. 3, 2020, 2:10 p.m. UTC | #1

On Thu, Dec 3, 2020 at 4:00 PM Alexandru Ardelean
<[e-mail]> wrote:
> From: Dragos Bogdan <[e-mail]>
>
> Transmit/receive only is a valid SPI mode. For example, the MOSI/TX line
> might be missing from an ADC while for a DAC the MISO/RX line may be
> optional. This patch adds these two new modes: SPI_NO_TX and
> SPI_NO_RX. This way, the drivers will be able to identify if any of
> these two lines is missing and to adjust the transfers accordingly.

...

> +       BUILD_BUG_ON(SPI_MODE_USER_MASK & SPI_MODE_KERNEL_MASK);

Please, use static_assert() as I have been pointed out. It may be
located outside of a function scope. You may attach it directly to the
definition of the KERNEL_MASK (I haven't tried yet with header
though).

...

> +#define SPI_NO_TX      BIT(31)         /* no transmit wire */
> +#define SPI_NO_RX      BIT(30)         /* no receive wire */
> +#define SPI_MODE_KERNEL_MASK   (SPI_NO_TX | SPI_NO_RX)

This needs a comment to explain what's going on with the flags (split).

Andy Shevchenko Dec. 3, 2020, 2:24 p.m. UTC | #2

On Thu, Dec 3, 2020 at 4:00 PM Alexandru Ardelean
<[e-mail]> wrote:

> +#define SPI_MODE_USER_MASK     \
> +       (SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST | \
> +        SPI_3WIRE | SPI_LOOP | SPI_NO_CS | SPI_READY | \
> +        SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD | \
> +        SPI_CS_WORD | SPI_TX_OCTAL | SPI_RX_OCTAL | SPI_3WIRE_HIZ)

Forgot to comment on this. Since it's an uAPI we may not fill the
holes (if any) in the future with the different semantics of values.
And this huge list of names is rather hard to read.

#define SPI_MODE_USER_MASK    (_BITUL(16) - 1)

would be sufficient.

For the record, I was thinking about providing MAX or LAST or
something like that instead of MASK and do the rest in kernel headers
/ modules, but it seems equally good/bad. Let's stick with mask as in
my initial propose and your current code.

Andy Shevchenko Dec. 3, 2020, 2:27 p.m. UTC | #3

On Thu, Dec 3, 2020 at 4:10 PM Andy Shevchenko
<[e-mail]> wrote:
> On Thu, Dec 3, 2020 at 4:00 PM Alexandru Ardelean
> <[e-mail]> wrote:

> > +#define SPI_NO_TX      BIT(31)         /* no transmit wire */
> > +#define SPI_NO_RX      BIT(30)         /* no receive wire */
> > +#define SPI_MODE_KERNEL_MASK   (SPI_NO_TX | SPI_NO_RX)
>
> This needs a comment to explain what's going on with the flags (split).

...and to be consistent with proposal in uAPI:

#define SPI_MODE_KERNEL_MASK   (~(BIT(30) - 1))

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index cd3c395b4e90..8668626c4a46 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1941,6 +1941,9 @@  static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 	/* Device DUAL/QUAD mode */
 	if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
 		switch (value) {
+		case 0:
+			spi->mode |= SPI_NO_TX;
+			break;
 		case 1:
 			break;
 		case 2:
@@ -1962,6 +1965,9 @@  static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 
 	if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) {
 		switch (value) {
+		case 0:
+			spi->mode |= SPI_NO_RX;
+			break;
 		case 1:
 			break;
 		case 2:
@@ -3329,12 +3335,17 @@  int spi_setup(struct spi_device *spi)
 	unsigned	bad_bits, ugly_bits;
 	int		status;
 
-	/* check mode to prevent that DUAL and QUAD set at the same time
+	/*
+	 * check mode to prevent that any two of DUAL, QUAD and NO_MOSI/MISO
+	 * are set at the same time
 	 */
-	if (((spi->mode & SPI_TX_DUAL) && (spi->mode & SPI_TX_QUAD)) ||
-		((spi->mode & SPI_RX_DUAL) && (spi->mode & SPI_RX_QUAD))) {
+	if ((hweight_long(spi->mode &
+		(SPI_TX_DUAL | SPI_TX_QUAD | SPI_NO_TX)) > 1) ||
+	    (hweight_long(spi->mode &
+		(SPI_RX_DUAL | SPI_RX_QUAD | SPI_NO_RX)) > 1)) {
 		dev_err(&spi->dev,
-		"setup: can not select dual and quad at the same time\n");
+		"setup: can not select any two of dual, quad and no-rx/tx "
+		"at the same time\n");
 		return -EINVAL;
 	}
 	/* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden
@@ -3348,7 +3359,8 @@  int spi_setup(struct spi_device *spi)
 	 * SPI_CS_WORD has a fallback software implementation,
 	 * so it is ignored here.
 	 */
-	bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD);
+	bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD |
+				 SPI_NO_TX | SPI_NO_RX);
 	/* nothing prevents from working with active-high CS in case if it
 	 * is driven by GPIO.
 	 */
@@ -3609,6 +3621,8 @@  static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 		 * 2. check tx/rx_nbits match the mode in spi_device
 		 */
 		if (xfer->tx_buf) {
+			if (spi->mode & SPI_NO_TX)
+				return -EINVAL;
 			if (xfer->tx_nbits != SPI_NBITS_SINGLE &&
 				xfer->tx_nbits != SPI_NBITS_DUAL &&
 				xfer->tx_nbits != SPI_NBITS_QUAD)
@@ -3622,6 +3636,8 @@  static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 		}
 		/* check transfer rx_nbits */
 		if (xfer->rx_buf) {
+			if (spi->mode & SPI_NO_RX)
+				return -EINVAL;
 			if (xfer->rx_nbits != SPI_NBITS_SINGLE &&
 				xfer->rx_nbits != SPI_NBITS_DUAL &&
 				xfer->rx_nbits != SPI_NBITS_QUAD)
@@ -4190,6 +4206,8 @@  static int __init spi_init(void)
 {
 	int	status;
 
+	BUILD_BUG_ON(SPI_MODE_USER_MASK & SPI_MODE_KERNEL_MASK);
+
 	buf = kmalloc(SPI_BUFSIZ, GFP_KERNEL);
 	if (!buf) {
 		status = -ENOMEM;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index a08c3f37e202..6f1905f729c4 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -6,6 +6,7 @@ 
 #ifndef __LINUX_SPI_H
 #define __LINUX_SPI_H
 
+#include <linux/bits.h>
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 #include <linux/slab.h>
@@ -166,6 +167,9 @@  struct spi_device {
 	u8			chip_select;
 	u8			bits_per_word;
 	bool			rt;
+#define SPI_NO_TX	BIT(31)		/* no transmit wire */
+#define SPI_NO_RX	BIT(30)		/* no receive wire */
+#define SPI_MODE_KERNEL_MASK	(SPI_NO_TX | SPI_NO_RX)
 	u32			mode;
 	int			irq;
 	void			*controller_state;
diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h
index 703b586f35df..e5c58748422e 100644
--- a/include/uapi/linux/spi/spi.h
+++ b/include/uapi/linux/spi/spi.h
@@ -28,4 +28,10 @@ 
 #define	SPI_RX_OCTAL		_BITUL(14)	/* receive with 8 wires */
 #define	SPI_3WIRE_HIZ		_BITUL(15)	/* high impedance turnaround */
 
+#define SPI_MODE_USER_MASK	\
+	(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST | \
+	 SPI_3WIRE | SPI_LOOP | SPI_NO_CS | SPI_READY | \
+	 SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD | \
+	 SPI_CS_WORD | SPI_TX_OCTAL | SPI_RX_OCTAL | SPI_3WIRE_HIZ)
+
 #endif /* _UAPI_SPI_H */