2020-12-03 14:04:09

by Alexandru Ardelean

[permalink] [raw]
Subject: [PATCH v4 1/3] spi: uapi: unify SPI modes into a single spi.h header

This change moves all the SPI mode bits into a separate 'spi.h' header in
uAPI. This is meant to re-use these definitions inside the kernel as well
as export them to userspace (via uAPI).

The SPI mode definitions have usually been duplicated between between
'include/linux/spi/spi.h' and 'include/uapi/linux/spi/spidev.h', so
whenever adding a new entry, this would need to be put in both headers.

They've been moved from 'include/linux/spi/spi.h', since that seems a bit
more complete; the bits have descriptions and there is the SPI_MODE_X_MASK.

This change also does a conversion of these bitfields to _BITUL() macro.

Signed-off-by: Alexandru Ardelean <[email protected]>
---

Changelog v3 -> v4:
* https://lore.kernel.org/linux-spi/[email protected]/
* uapi -> uAPI in comment
* removed extra license text in 'uapi/linux/spi/spi.h'
* using _BITUL() macro

include/linux/spi/spi.h | 23 ++---------------------
include/uapi/linux/spi/spi.h | 31 +++++++++++++++++++++++++++++++
include/uapi/linux/spi/spidev.h | 30 +-----------------------------
3 files changed, 34 insertions(+), 50 deletions(-)
create mode 100644 include/uapi/linux/spi/spi.h

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index aa09fdc8042d..a08c3f37e202 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -15,6 +15,8 @@
#include <linux/gpio/consumer.h>
#include <linux/ptp_clock_kernel.h>

+#include <uapi/linux/spi/spi.h>
+
struct dma_chan;
struct property_entry;
struct spi_controller;
@@ -165,27 +167,6 @@ struct spi_device {
u8 bits_per_word;
bool rt;
u32 mode;
-#define SPI_CPHA 0x01 /* clock phase */
-#define SPI_CPOL 0x02 /* clock polarity */
-#define SPI_MODE_0 (0|0) /* (original MicroWire) */
-#define SPI_MODE_1 (0|SPI_CPHA)
-#define SPI_MODE_2 (SPI_CPOL|0)
-#define SPI_MODE_3 (SPI_CPOL|SPI_CPHA)
-#define SPI_MODE_X_MASK (SPI_CPOL|SPI_CPHA)
-#define SPI_CS_HIGH 0x04 /* chipselect active high? */
-#define SPI_LSB_FIRST 0x08 /* per-word bits-on-wire */
-#define SPI_3WIRE 0x10 /* SI/SO signals shared */
-#define SPI_LOOP 0x20 /* loopback mode */
-#define SPI_NO_CS 0x40 /* 1 dev/bus, no chipselect */
-#define SPI_READY 0x80 /* slave pulls low to pause */
-#define SPI_TX_DUAL 0x100 /* transmit with 2 wires */
-#define SPI_TX_QUAD 0x200 /* transmit with 4 wires */
-#define SPI_RX_DUAL 0x400 /* receive with 2 wires */
-#define SPI_RX_QUAD 0x800 /* receive with 4 wires */
-#define SPI_CS_WORD 0x1000 /* toggle cs after each word */
-#define SPI_TX_OCTAL 0x2000 /* transmit with 8 wires */
-#define SPI_RX_OCTAL 0x4000 /* receive with 8 wires */
-#define SPI_3WIRE_HIZ 0x8000 /* high impedance turnaround */
int irq;
void *controller_state;
void *controller_data;
diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h
new file mode 100644
index 000000000000..703b586f35df
--- /dev/null
+++ b/include/uapi/linux/spi/spi.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+#ifndef _UAPI_SPI_H
+#define _UAPI_SPI_H
+
+#include <linux/const.h>
+
+#define SPI_CPHA _BITUL(0) /* clock phase */
+#define SPI_CPOL _BITUL(1) /* clock polarity */
+
+#define SPI_MODE_0 (0|0) /* (original MicroWire) */
+#define SPI_MODE_1 (0|SPI_CPHA)
+#define SPI_MODE_2 (SPI_CPOL|0)
+#define SPI_MODE_3 (SPI_CPOL|SPI_CPHA)
+#define SPI_MODE_X_MASK (SPI_CPOL|SPI_CPHA)
+
+#define SPI_CS_HIGH _BITUL(2) /* chipselect active high? */
+#define SPI_LSB_FIRST _BITUL(3) /* per-word bits-on-wire */
+#define SPI_3WIRE _BITUL(4) /* SI/SO signals shared */
+#define SPI_LOOP _BITUL(5) /* loopback mode */
+#define SPI_NO_CS _BITUL(6) /* 1 dev/bus, no chipselect */
+#define SPI_READY _BITUL(7) /* slave pulls low to pause */
+#define SPI_TX_DUAL _BITUL(8) /* transmit with 2 wires */
+#define SPI_TX_QUAD _BITUL(9) /* transmit with 4 wires */
+#define SPI_RX_DUAL _BITUL(10) /* receive with 2 wires */
+#define SPI_RX_QUAD _BITUL(11) /* receive with 4 wires */
+#define SPI_CS_WORD _BITUL(12) /* toggle cs after each word */
+#define SPI_TX_OCTAL _BITUL(13) /* transmit with 8 wires */
+#define SPI_RX_OCTAL _BITUL(14) /* receive with 8 wires */
+#define SPI_3WIRE_HIZ _BITUL(15) /* high impedance turnaround */
+
+#endif /* _UAPI_SPI_H */
diff --git a/include/uapi/linux/spi/spidev.h b/include/uapi/linux/spi/spidev.h
index d56427c0b3e0..0c3da08f2aff 100644
--- a/include/uapi/linux/spi/spidev.h
+++ b/include/uapi/linux/spi/spidev.h
@@ -25,35 +25,7 @@

#include <linux/types.h>
#include <linux/ioctl.h>
-
-/* User space versions of kernel symbols for SPI clocking modes,
- * matching <linux/spi/spi.h>
- */
-
-#define SPI_CPHA 0x01
-#define SPI_CPOL 0x02
-
-#define SPI_MODE_0 (0|0)
-#define SPI_MODE_1 (0|SPI_CPHA)
-#define SPI_MODE_2 (SPI_CPOL|0)
-#define SPI_MODE_3 (SPI_CPOL|SPI_CPHA)
-
-#define SPI_CS_HIGH 0x04
-#define SPI_LSB_FIRST 0x08
-#define SPI_3WIRE 0x10
-#define SPI_LOOP 0x20
-#define SPI_NO_CS 0x40
-#define SPI_READY 0x80
-#define SPI_TX_DUAL 0x100
-#define SPI_TX_QUAD 0x200
-#define SPI_RX_DUAL 0x400
-#define SPI_RX_QUAD 0x800
-#define SPI_CS_WORD 0x1000
-#define SPI_TX_OCTAL 0x2000
-#define SPI_RX_OCTAL 0x4000
-#define SPI_3WIRE_HIZ 0x8000
-
-/*---------------------------------------------------------------------------*/
+#include <linux/spi/spi.h>

/* IOCTL commands */

--
2.17.1


2020-12-03 14:04:17

by Alexandru Ardelean

[permalink] [raw]
Subject: [PATCH v4 3/3] spi: dt-bindings: document zero value for spi-{rx,tx}-bus-width properties

Following a change to the SPI framework, providing a value of zero for
'spi-rx-bus-width' and 'spi-tx-bus-width' is now possible and will
essentially mean that no RX or TX is allowed.

Signed-off-by: Alexandru Ardelean <[email protected]>
---

Changelog v3 -> v4:
* https://lore.kernel.org/linux-spi/[email protected]/
* fix typos

Documentation/devicetree/bindings/spi/spi-controller.yaml | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 1b56d5e40f1f..cf663e4a4afe 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -125,8 +125,9 @@ patternProperties:
spi-rx-bus-width:
description:
Bus width to the SPI bus used for read transfers.
+ If 0 is provided, then no RX will be possible on this device.
$ref: /schemas/types.yaml#/definitions/uint32
- enum: [1, 2, 4, 8]
+ enum: [0, 1, 2, 4, 8]
default: 1

spi-rx-delay-us:
@@ -136,8 +137,9 @@ patternProperties:
spi-tx-bus-width:
description:
Bus width to the SPI bus used for write transfers.
+ If 0 is provided, then no TX will be possible on this device.
$ref: /schemas/types.yaml#/definitions/uint32
- enum: [1, 2, 4, 8]
+ enum: [0, 1, 2, 4, 8]
default: 1

spi-tx-delay-us:
--
2.17.1

2020-12-03 14:05:02

by Alexandru Ardelean

[permalink] [raw]
Subject: [PATCH v4 2/3] spi: Add SPI_NO_TX/RX support

From: Dragos Bogdan <[email protected]>

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 <[email protected]>
Signed-off-by: Alexandru Ardelean <[email protected]>
---

Changelog v3 -> v4:
* https://lore.kernel.org/linux-spi/[email protected]/
* 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(-)

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

2020-12-03 14:14:13

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v4 2/3] spi: Add SPI_NO_TX/RX support

On Thu, Dec 3, 2020 at 4:00 PM Alexandru Ardelean
<[email protected]> wrote:
> From: Dragos Bogdan <[email protected]>
>
> 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).

--
With Best Regards,
Andy Shevchenko

2020-12-03 14:16:55

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v4 1/3] spi: uapi: unify SPI modes into a single spi.h header

On Thu, Dec 3, 2020 at 4:00 PM Alexandru Ardelean
<[email protected]> wrote:
>
> This change moves all the SPI mode bits into a separate 'spi.h' header in
> uAPI. This is meant to re-use these definitions inside the kernel as well
> as export them to userspace (via uAPI).
>
> The SPI mode definitions have usually been duplicated between between
> 'include/linux/spi/spi.h' and 'include/uapi/linux/spi/spidev.h', so
> whenever adding a new entry, this would need to be put in both headers.
>
> They've been moved from 'include/linux/spi/spi.h', since that seems a bit
> more complete; the bits have descriptions and there is the SPI_MODE_X_MASK.
>
> This change also does a conversion of these bitfields to _BITUL() macro.

Looks nice to me now, FWIW,
Reviewed-by: Andy Shevchenko <[email protected]>

> Signed-off-by: Alexandru Ardelean <[email protected]>
> ---
>
> Changelog v3 -> v4:
> * https://lore.kernel.org/linux-spi/[email protected]/
> * uapi -> uAPI in comment
> * removed extra license text in 'uapi/linux/spi/spi.h'
> * using _BITUL() macro
>
> include/linux/spi/spi.h | 23 ++---------------------
> include/uapi/linux/spi/spi.h | 31 +++++++++++++++++++++++++++++++
> include/uapi/linux/spi/spidev.h | 30 +-----------------------------
> 3 files changed, 34 insertions(+), 50 deletions(-)
> create mode 100644 include/uapi/linux/spi/spi.h
>
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index aa09fdc8042d..a08c3f37e202 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -15,6 +15,8 @@
> #include <linux/gpio/consumer.h>
> #include <linux/ptp_clock_kernel.h>
>
> +#include <uapi/linux/spi/spi.h>
> +
> struct dma_chan;
> struct property_entry;
> struct spi_controller;
> @@ -165,27 +167,6 @@ struct spi_device {
> u8 bits_per_word;
> bool rt;
> u32 mode;
> -#define SPI_CPHA 0x01 /* clock phase */
> -#define SPI_CPOL 0x02 /* clock polarity */
> -#define SPI_MODE_0 (0|0) /* (original MicroWire) */
> -#define SPI_MODE_1 (0|SPI_CPHA)
> -#define SPI_MODE_2 (SPI_CPOL|0)
> -#define SPI_MODE_3 (SPI_CPOL|SPI_CPHA)
> -#define SPI_MODE_X_MASK (SPI_CPOL|SPI_CPHA)
> -#define SPI_CS_HIGH 0x04 /* chipselect active high? */
> -#define SPI_LSB_FIRST 0x08 /* per-word bits-on-wire */
> -#define SPI_3WIRE 0x10 /* SI/SO signals shared */
> -#define SPI_LOOP 0x20 /* loopback mode */
> -#define SPI_NO_CS 0x40 /* 1 dev/bus, no chipselect */
> -#define SPI_READY 0x80 /* slave pulls low to pause */
> -#define SPI_TX_DUAL 0x100 /* transmit with 2 wires */
> -#define SPI_TX_QUAD 0x200 /* transmit with 4 wires */
> -#define SPI_RX_DUAL 0x400 /* receive with 2 wires */
> -#define SPI_RX_QUAD 0x800 /* receive with 4 wires */
> -#define SPI_CS_WORD 0x1000 /* toggle cs after each word */
> -#define SPI_TX_OCTAL 0x2000 /* transmit with 8 wires */
> -#define SPI_RX_OCTAL 0x4000 /* receive with 8 wires */
> -#define SPI_3WIRE_HIZ 0x8000 /* high impedance turnaround */
> int irq;
> void *controller_state;
> void *controller_data;
> diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h
> new file mode 100644
> index 000000000000..703b586f35df
> --- /dev/null
> +++ b/include/uapi/linux/spi/spi.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +#ifndef _UAPI_SPI_H
> +#define _UAPI_SPI_H
> +
> +#include <linux/const.h>
> +
> +#define SPI_CPHA _BITUL(0) /* clock phase */
> +#define SPI_CPOL _BITUL(1) /* clock polarity */
> +
> +#define SPI_MODE_0 (0|0) /* (original MicroWire) */
> +#define SPI_MODE_1 (0|SPI_CPHA)
> +#define SPI_MODE_2 (SPI_CPOL|0)
> +#define SPI_MODE_3 (SPI_CPOL|SPI_CPHA)
> +#define SPI_MODE_X_MASK (SPI_CPOL|SPI_CPHA)
> +
> +#define SPI_CS_HIGH _BITUL(2) /* chipselect active high? */
> +#define SPI_LSB_FIRST _BITUL(3) /* per-word bits-on-wire */
> +#define SPI_3WIRE _BITUL(4) /* SI/SO signals shared */
> +#define SPI_LOOP _BITUL(5) /* loopback mode */
> +#define SPI_NO_CS _BITUL(6) /* 1 dev/bus, no chipselect */
> +#define SPI_READY _BITUL(7) /* slave pulls low to pause */
> +#define SPI_TX_DUAL _BITUL(8) /* transmit with 2 wires */
> +#define SPI_TX_QUAD _BITUL(9) /* transmit with 4 wires */
> +#define SPI_RX_DUAL _BITUL(10) /* receive with 2 wires */
> +#define SPI_RX_QUAD _BITUL(11) /* receive with 4 wires */
> +#define SPI_CS_WORD _BITUL(12) /* toggle cs after each word */
> +#define SPI_TX_OCTAL _BITUL(13) /* transmit with 8 wires */
> +#define SPI_RX_OCTAL _BITUL(14) /* receive with 8 wires */
> +#define SPI_3WIRE_HIZ _BITUL(15) /* high impedance turnaround */
> +
> +#endif /* _UAPI_SPI_H */
> diff --git a/include/uapi/linux/spi/spidev.h b/include/uapi/linux/spi/spidev.h
> index d56427c0b3e0..0c3da08f2aff 100644
> --- a/include/uapi/linux/spi/spidev.h
> +++ b/include/uapi/linux/spi/spidev.h
> @@ -25,35 +25,7 @@
>
> #include <linux/types.h>
> #include <linux/ioctl.h>
> -
> -/* User space versions of kernel symbols for SPI clocking modes,
> - * matching <linux/spi/spi.h>
> - */
> -
> -#define SPI_CPHA 0x01
> -#define SPI_CPOL 0x02
> -
> -#define SPI_MODE_0 (0|0)
> -#define SPI_MODE_1 (0|SPI_CPHA)
> -#define SPI_MODE_2 (SPI_CPOL|0)
> -#define SPI_MODE_3 (SPI_CPOL|SPI_CPHA)
> -
> -#define SPI_CS_HIGH 0x04
> -#define SPI_LSB_FIRST 0x08
> -#define SPI_3WIRE 0x10
> -#define SPI_LOOP 0x20
> -#define SPI_NO_CS 0x40
> -#define SPI_READY 0x80
> -#define SPI_TX_DUAL 0x100
> -#define SPI_TX_QUAD 0x200
> -#define SPI_RX_DUAL 0x400
> -#define SPI_RX_QUAD 0x800
> -#define SPI_CS_WORD 0x1000
> -#define SPI_TX_OCTAL 0x2000
> -#define SPI_RX_OCTAL 0x4000
> -#define SPI_3WIRE_HIZ 0x8000
> -
> -/*---------------------------------------------------------------------------*/
> +#include <linux/spi/spi.h>
>
> /* IOCTL commands */
>
> --
> 2.17.1
>


--
With Best Regards,
Andy Shevchenko

2020-12-03 14:28:16

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v4 2/3] spi: Add SPI_NO_TX/RX support

On Thu, Dec 3, 2020 at 4:00 PM Alexandru Ardelean
<[email protected]> 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.

--
With Best Regards,
Andy Shevchenko

2020-12-03 14:31:15

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v4 2/3] spi: Add SPI_NO_TX/RX support

On Thu, Dec 3, 2020 at 4:10 PM Andy Shevchenko
<[email protected]> wrote:
> On Thu, Dec 3, 2020 at 4:00 PM Alexandru Ardelean
> <[email protected]> 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))

--
With Best Regards,
Andy Shevchenko

2020-12-09 20:42:07

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v4 3/3] spi: dt-bindings: document zero value for spi-{rx,tx}-bus-width properties

On Thu, 03 Dec 2020 16:05:31 +0200, Alexandru Ardelean wrote:
> Following a change to the SPI framework, providing a value of zero for
> 'spi-rx-bus-width' and 'spi-tx-bus-width' is now possible and will
> essentially mean that no RX or TX is allowed.
>
> Signed-off-by: Alexandru Ardelean <[email protected]>
> ---
>
> Changelog v3 -> v4:
> * https://lore.kernel.org/linux-spi/[email protected]/
> * fix typos
>
> Documentation/devicetree/bindings/spi/spi-controller.yaml | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>

Reviewed-by: Rob Herring <[email protected]>