[PATCH v4 0/6] Add support for AD4000 series of ADCs

[PATCH v4 0/6] Add support for AD4000 series of ADCs

[Marcelo Schmitt]

This patch series extends the SPI bitbang, gpio, and spi-engine controllers to
support configurable MOSI line idle state.
It then introduces the ad4000 driver which uses the MOSI idle configuration to
properly support AD4000 series of ADCs.

Change log v3 -> v4:

[SPI]
- spi: Added documentation for the MOSI idle configuration.
- spi: spi_setup() now fails on improper MOSI idle state configuration.
- spi: spi_setup() now fails if controller doesn't support requested MOSI config.
- spi: spi-engine: Only set MOSI idle mode bits if spi-engine version supports it.
[Device tree]
- dt: Made grouped compatible strings for devices that are similar to each other.
- dt: Updated dt-bindings to constrain properties that depend on reg access to
  "3-wire" mode only.
- dt: adi,gain-milli is now a 16-bit device tree property.
[IIO/ADC]
- ad4000: Used devm_regulator_get_enable_read_voltage() for ref regulator.
- ad4000: Tweaked gpiod_set_value comment explaining what happens when CNV GPIO is
  defined and when it is not.
- ad4000: Device configuration register write will now only happen if device is
  connected in a mode that allows register access.
- ad4000: scale attribute now only writeable if device connection allows user to
  change the scale.
- ad4000: scale_available attribute now is only visible if scale is writeable.
- ad4000: many minor changes.


Link to v3: https://lore.kernel.org/linux-iio/cover.1717539384.git.marcelo.schmitt@analog.com/
Link to v2: https://lore.kernel.org/linux-iio/cover.1712585500.git.marcelo.schmitt@analog.com/
Link to v1: https://lore.kernel.org/linux-iio/cover.1711131830.git.marcelo.schmitt@analog.com/

Thanks,
Marcelo

Marcelo Schmitt (6):
  spi: Enable controllers to extend the SPI protocol with MOSI idle
    configuration
  spi: bitbang: Implement support for MOSI idle state configuration
  spi: spi-gpio: Add support for MOSI idle state configuration
  spi: spi-axi-spi-engine: Add support for MOSI idle configuration
  dt-bindings: iio: adc: Add AD4000
  iio: adc: Add support for AD4000

 .../bindings/iio/adc/adi,ad4000.yaml          | 231 ++++++
 Documentation/spi/spi-summary.rst             |  83 ++
 MAINTAINERS                                   |   8 +
 drivers/iio/adc/Kconfig                       |  12 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ad4000.c                      | 715 ++++++++++++++++++
 drivers/spi/spi-axi-spi-engine.c              |   8 +
 drivers/spi/spi-bitbang.c                     |  24 +
 drivers/spi/spi-gpio.c                        |  12 +-
 drivers/spi/spi.c                             |   9 +-
 include/linux/spi/spi.h                       |   6 +
 include/linux/spi/spi_bitbang.h               |   1 +
 include/uapi/linux/spi/spi.h                  |   5 +-
 13 files changed, 1111 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
 create mode 100644 drivers/iio/adc/ad4000.c

-- 
2.43.0

[PATCH v4 1/6] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration

[Marcelo Schmitt]

The behavior of an SPI controller data output line (SDO or MOSI or COPI
(Controller Output Peripheral Input) for disambiguation) is usually not
specified when the controller is not clocking out data on SCLK edges.
However, there do exist SPI peripherals that require specific MOSI line
state when data is not being clocked out of the controller.

A SPI controller may set the MOSI line on SCLK edges then bring it low when
no data is going out or leave the line the state of the last transfer bit.
More elaborated controllers are capable to set the MOSI idle state
according to different configurable levels and thus are more suitable for
interfacing with restrictive peripherals.

Add SPI mode bits to allow peripherals to request explicit MOSI idle state
when needed.

When supporting a particular MOSI idle configuration, the data output line
state is expected to remain at the configured level when the controller is
not clocking out data. When a device that needs a specific MOSI idle state
is identified, its driver should request the MOSI idle configuration by
setting the proper SPI mode bit.

Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
Hi, so, this is an improved version of MOSI idle configuration support based on
comments to the previous set.
I'm actually not sure I did everything requested for the SPI subsystem.
First replies to v3 brought the idea of having a feature detection mechanism.
I didn't really get how to do that. If feature detection is required, can
somebody please provide some pointers on how to implement that?

 Documentation/spi/spi-summary.rst | 83 +++++++++++++++++++++++++++++++
 drivers/spi/spi.c                 |  9 +++-
 include/linux/spi/spi.h           |  6 +++
 include/uapi/linux/spi/spi.h      |  5 +-
 4 files changed, 100 insertions(+), 3 deletions(-)

diff --git a/Documentation/spi/spi-summary.rst b/Documentation/spi/spi-summary.rst
index 7f8accfae6f9..49346708b522 100644
--- a/Documentation/spi/spi-summary.rst
+++ b/Documentation/spi/spi-summary.rst
@@ -614,6 +614,89 @@ queue, and then start some asynchronous transfer engine (unless it's
 already running).
 
 
+Extensions to the SPI protocol
+------------------------------
+The fact that SPI doesn't have a formal specification or standard permits chip
+manufacturers to implement the SPI protocol in slightly different ways. In most
+cases, SPI protocol implementations from different vendors are compatible among
+each other. For example, in SPI mode 0 (CPOL=0, CPHA=0) the bus lines may behave
+like the following:
+
+::
+
+  nCSx ___                                                                   ___
+          \_________________________________________________________________/
+          •                                                                 •
+          •                                                                 •
+  SCLK         ___     ___     ___     ___     ___     ___     ___     ___
+       _______/   \___/   \___/   \___/   \___/   \___/   \___/   \___/   \_____
+          •   :   ;   :   ;   :   ;   :   ;   :   ;   :   ;   :   ;   :   ; •
+          •   :   ;   :   ;   :   ;   :   ;   :   ;   :   ;   :   ;   :   ; •
+  MOSI XXX__________         _______                 _______         ________XXX
+  0xA5 XXX__/ 1     \_0_____/ 1     \_0_______0_____/ 1     \_0_____/ 1    \_XXX
+          •       ;       ;       ;       ;       ;       ;       ;       ; •
+          •       ;       ;       ;       ;       ;       ;       ;       ; •
+  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 some few cases, chips extend the SPI protocol by specifying line behaviors
+that other SPI protocols don't (e.g. data line state for when CS is inactive).
+Those distinct SPI protocols, modes, and configurations are supported by
+different SPI mode flags.
+
+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
+the peripheral and also when CS is inactive.
+
+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
+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,
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 289feccca376..8e567d5b1945 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3921,6 +3921,12 @@ int spi_setup(struct spi_device *spi)
 		(SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
 		 SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL)))
 		return -EINVAL;
+	/* Check against conflicting MOSI idle configuration */
+	if ((spi->mode & SPI_MOSI_IDLE_LOW) && (spi->mode & SPI_MOSI_IDLE_HIGH)) {
+		dev_err(&spi->dev,
+			"setup: MOSI configured to simultaneously idle low and high.\n");
+		return -EINVAL;
+	}
 	/*
 	 * Help drivers fail *cleanly* when they need options
 	 * that aren't supported with their current controller.
@@ -3928,7 +3934,8 @@ int spi_setup(struct spi_device *spi)
 	 * so it is ignored here.
 	 */
 	bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD |
-				 SPI_NO_TX | SPI_NO_RX);
+				 SPI_NO_TX | SPI_NO_RX | SPI_MOSI_IDLE_LOW |
+				 SPI_MOSI_IDLE_HIGH);
 	ugly_bits = bad_bits &
 		    (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
 		     SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL);
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 */
 
 	/* 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 */
-- 
2.43.0

[PATCH v4 2/6] spi: bitbang: Implement support for MOSI idle state configuration

[Marcelo Schmitt]

Some SPI peripherals may require strict MOSI line state when the controller
is not clocking out data.
Implement support for MOSI idle state configuration (low or high) by
setting the data output line level on controller setup and after transfers.
Bitbang operations now call controller specific set_mosi_idle() call back
to set MOSI to its idle state.
The MOSI line is kept at its idle state if no tx buffer is provided.

Acked-by: Nuno Sa <nuno.sa@analog.com>
Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
 drivers/spi/spi-bitbang.c       | 24 ++++++++++++++++++++++++
 include/linux/spi/spi_bitbang.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index ca5cc67555c5..8cc522bf444c 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -63,21 +63,28 @@ static unsigned bitbang_txrx_8(
 	unsigned flags
 )
 {
+	struct spi_bitbang	*bitbang;
 	unsigned		bits = t->bits_per_word;
 	unsigned		count = t->len;
 	const u8		*tx = t->tx_buf;
 	u8			*rx = t->rx_buf;
 
+	bitbang = spi_controller_get_devdata(spi->controller);
 	while (likely(count > 0)) {
 		u8		word = 0;
 
 		if (tx)
 			word = *tx++;
+		else
+			word = spi->mode & SPI_MOSI_IDLE_HIGH ? 0xFF : 0;
 		word = txrx_word(spi, ns, word, bits, flags);
 		if (rx)
 			*rx++ = word;
 		count -= 1;
 	}
+	if (bitbang->set_mosi_idle)
+		bitbang->set_mosi_idle(spi);
+
 	return t->len - count;
 }
 
@@ -92,21 +99,28 @@ static unsigned bitbang_txrx_16(
 	unsigned flags
 )
 {
+	struct spi_bitbang	*bitbang;
 	unsigned		bits = t->bits_per_word;
 	unsigned		count = t->len;
 	const u16		*tx = t->tx_buf;
 	u16			*rx = t->rx_buf;
 
+	bitbang = spi_controller_get_devdata(spi->controller);
 	while (likely(count > 1)) {
 		u16		word = 0;
 
 		if (tx)
 			word = *tx++;
+		else
+			word = spi->mode & SPI_MOSI_IDLE_HIGH ? 0xFFFF : 0;
 		word = txrx_word(spi, ns, word, bits, flags);
 		if (rx)
 			*rx++ = word;
 		count -= 2;
 	}
+	if (bitbang->set_mosi_idle)
+		bitbang->set_mosi_idle(spi);
+
 	return t->len - count;
 }
 
@@ -121,21 +135,28 @@ static unsigned bitbang_txrx_32(
 	unsigned flags
 )
 {
+	struct spi_bitbang	*bitbang;
 	unsigned		bits = t->bits_per_word;
 	unsigned		count = t->len;
 	const u32		*tx = t->tx_buf;
 	u32			*rx = t->rx_buf;
 
+	bitbang = spi_controller_get_devdata(spi->controller);
 	while (likely(count > 3)) {
 		u32		word = 0;
 
 		if (tx)
 			word = *tx++;
+		else
+			word = spi->mode & SPI_MOSI_IDLE_HIGH ? 0xFFFFFFFF : 0;
 		word = txrx_word(spi, ns, word, bits, flags);
 		if (rx)
 			*rx++ = word;
 		count -= 4;
 	}
+	if (bitbang->set_mosi_idle)
+		bitbang->set_mosi_idle(spi);
+
 	return t->len - count;
 }
 
@@ -211,6 +232,9 @@ int spi_bitbang_setup(struct spi_device *spi)
 			goto err_free;
 	}
 
+	if (bitbang->set_mosi_idle)
+		bitbang->set_mosi_idle(spi);
+
 	dev_dbg(&spi->dev, "%s, %u nsec/bit\n", __func__, 2 * cs->nsecs);
 
 	return 0;
diff --git a/include/linux/spi/spi_bitbang.h b/include/linux/spi/spi_bitbang.h
index b930eca2ef7b..1a54b593c691 100644
--- a/include/linux/spi/spi_bitbang.h
+++ b/include/linux/spi/spi_bitbang.h
@@ -22,6 +22,7 @@ struct spi_bitbang {
 #define	BITBANG_CS_ACTIVE	1	/* normally nCS, active low */
 #define	BITBANG_CS_INACTIVE	0
 
+	void	(*set_mosi_idle)(struct spi_device *spi);
 	/* txrx_bufs() may handle dma mapping for transfers that don't
 	 * already have one (transfer.{tx,rx}_dma is zero), or use PIO
 	 */
-- 
2.43.0

[PATCH v4 3/6] spi: spi-gpio: Add support for MOSI idle state configuration

[Marcelo Schmitt]

Implement MOSI idle low and MOSI idle high to better support peripherals
that request specific MOSI behavior.

Acked-by: Nuno Sa <nuno.sa@analog.com>
Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
 drivers/spi/spi-gpio.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c
index 909cce109bba..d3b8c99f0cb4 100644
--- a/drivers/spi/spi-gpio.c
+++ b/drivers/spi/spi-gpio.c
@@ -236,6 +236,14 @@ static void spi_gpio_chipselect(struct spi_device *spi, int is_active)
 	}
 }
 
+static void spi_gpio_set_mosi_idle(struct spi_device *spi)
+{
+	struct spi_gpio *spi_gpio = spi_to_spi_gpio(spi);
+
+	gpiod_set_value_cansleep(spi_gpio->mosi,
+				 !!(spi->mode & SPI_MOSI_IDLE_HIGH));
+}
+
 static int spi_gpio_setup(struct spi_device *spi)
 {
 	struct gpio_desc	*cs;
@@ -411,7 +419,8 @@ static int spi_gpio_probe(struct platform_device *pdev)
 
 	host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
 	host->mode_bits = SPI_3WIRE | SPI_3WIRE_HIZ | SPI_CPHA | SPI_CPOL |
-			    SPI_CS_HIGH | SPI_LSB_FIRST;
+			  SPI_CS_HIGH | SPI_LSB_FIRST | SPI_MOSI_IDLE_LOW |
+			  SPI_MOSI_IDLE_HIGH;
 	if (!spi_gpio->mosi) {
 		/* HW configuration without MOSI pin
 		 *
@@ -436,6 +445,7 @@ static int spi_gpio_probe(struct platform_device *pdev)
 	host->flags |= SPI_CONTROLLER_GPIO_SS;
 	bb->chipselect = spi_gpio_chipselect;
 	bb->set_line_direction = spi_gpio_set_direction;
+	bb->set_mosi_idle = spi_gpio_set_mosi_idle;
 
 	if (host->flags & SPI_CONTROLLER_NO_TX) {
 		bb->txrx_word[SPI_MODE_0] = spi_gpio_spec_txrx_word_mode0;
-- 
2.43.0

[PATCH v4 4/6] spi: spi-axi-spi-engine: Add support for MOSI idle configuration

[Marcelo Schmitt]

Implement MOSI idle low and MOSI idle high to better support peripherals
that request specific MOSI behavior.

Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
 drivers/spi/spi-axi-spi-engine.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index 0aa31d745734..787e22ae80c0 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -41,6 +41,7 @@
 #define SPI_ENGINE_CONFIG_CPHA			BIT(0)
 #define SPI_ENGINE_CONFIG_CPOL			BIT(1)
 #define SPI_ENGINE_CONFIG_3WIRE			BIT(2)
+#define SPI_ENGINE_CONFIG_SDO_IDLE		BIT(3)
 
 #define SPI_ENGINE_INST_TRANSFER		0x0
 #define SPI_ENGINE_INST_ASSERT			0x1
@@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct spi_device *spi)
 		config |= SPI_ENGINE_CONFIG_CPHA;
 	if (spi->mode & SPI_3WIRE)
 		config |= SPI_ENGINE_CONFIG_3WIRE;
+	if (spi->mode & SPI_MOSI_IDLE_HIGH)
+		config |= SPI_ENGINE_CONFIG_SDO_IDLE;
+	if (spi->mode & SPI_MOSI_IDLE_LOW)
+		config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;
 
 	return config;
 }
@@ -646,6 +651,9 @@ static int spi_engine_probe(struct platform_device *pdev)
 
 	host->dev.of_node = pdev->dev.of_node;
 	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
+	if (ADI_AXI_PCORE_VER_MAJOR(version) >= 1 &&
+	    ADI_AXI_PCORE_VER_MINOR(version) >= 3)
+		host->mode_bits |=  SPI_MOSI_IDLE_LOW | SPI_MOSI_IDLE_HIGH;
 	host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
 	host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2;
 	host->transfer_one_message = spi_engine_transfer_one_message;
-- 
2.43.0

[PATCH v4 5/6] dt-bindings: iio: adc: Add AD4000

[Marcelo Schmitt]

Add device tree documentation for AD4000 series of ADC devices.

Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf

Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
 .../bindings/iio/adc/adi,ad4000.yaml          | 231 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 238 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
new file mode 100644
index 000000000000..ba50f9e0784b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
@@ -0,0 +1,231 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4000 and similar Analog to Digital Converters
+
+maintainers:
+  - Marcelo Schmitt <marcelo.schmitt@analog.com>
+
+description: |
+  Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
+  Specifications can be found at:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - const: adi,ad4000
+      - items:
+          - enum:
+              - adi,ad4004
+              - adi,ad4008
+          - const: adi,ad4000
+      - const: adi,ad4001
+      - items:
+          - enum:
+              - adi,ad4005
+          - const: adi,ad4001
+      - const: adi,ad4002
+      - items:
+          - enum:
+              - adi,ad4006
+              - adi,ad4010
+          - const: adi,ad4002
+      - const: adi,ad4003
+      - items:
+          - enum:
+              - adi,ad4007
+              - adi,ad4011
+          - const: adi,ad4003
+      - const: adi,ad4020
+      - items:
+          - enum:
+              - adi,ad4021
+              - adi,ad4022
+          - const: adi,ad4020
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
+
+  adi,spi-mode:
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [ single, chain ]
+    description: |
+      This property indicates the SPI wiring configuration.
+
+      When this property is omitted, it is assumed that the device is using what
+      the datasheet calls "4-wire mode". This is the conventional SPI mode used
+      when there are multiple devices on the same bus. In this mode, the CNV
+      line is used to initiate the conversion and the SDI line is connected to
+      CS on the SPI controller.
+
+      When this property is present, it indicates that the device is using one
+      of the following alternative wiring configurations:
+
+      * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
+        definition of 3-wire mode is NOT at all related to the standard
+        spi-3wire property!) This mode is often used when the ADC is the only
+        device on the bus. In this mode, SDI is connected to MOSI or to VIO, and
+        the CNV line can be connected to the CS line of the SPI controller or to
+        a GPIO, in which case the CS line of the controller is unused.
+      * chain: The datasheet calls this "chain mode". This mode is used to save
+        on wiring when multiple ADCs are used. In this mode, the SDI line of
+        one chip is tied to the SDO of the next chip in the chain and the SDI of
+        the last chip in the chain is tied to GND. Only the first chip in the
+        chain is connected to the SPI bus. The CNV line of all chips are tied
+        together. The CS line of the SPI controller can be used as the CNV line
+        only if it is active high.
+
+  '#daisy-chained-devices': true
+
+  vdd-supply:
+    description: A 1.8V supply that powers the chip (VDD).
+
+  vio-supply:
+    description:
+      A 1.8V to 5.5V supply for the digital inputs and outputs (VIO).
+
+  ref-supply:
+    description:
+      A 2.5 to 5V supply for the external reference voltage (REF).
+
+  cnv-gpios:
+    description:
+      The Convert Input (CNV). This input has multiple functions. It initiates
+      the conversions and selects the SPI mode of the device (chain or CS). In
+      'single' mode, this property is omitted if the CNV pin is connected to the
+      CS line of the SPI controller.
+    maxItems: 1
+
+  adi,high-z-input:
+    type: boolean
+    description:
+      High-Z mode allows the amplifier and RC filter in front of the ADC to be
+      chosen based on the signal bandwidth of interest, rather than the settling
+      requirements of the switched capacitor SAR ADC inputs.
+
+  adi,gain-milli:
+    description: |
+      The hardware gain applied to the ADC input (in milli units).
+      The gain provided by the ADC input scaler is defined by the hardware
+      connections between chip pins OUT+, R1K-, R1K1-, R1K+, R1K1+, and OUT-.
+      If not present, default to 1000 (no actual gain applied).
+    $ref: /schemas/types.yaml#/definitions/uint16
+    enum: [454, 909, 1000, 1900]
+    default: 1000
+
+  interrupts:
+    description:
+      The SDO pin can also function as a busy indicator. This node should be
+      connected to an interrupt that is triggered when the SDO line goes low
+      while the SDI line is high and the CNV line is low ('single' mode) or the
+      SDI line is low and the CNV line is high ('multi' mode); or when the SDO
+      line goes high while the SDI and CNV lines are high (chain mode),
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - vio-supply
+  - ref-supply
+
+allOf:
+  # in '4-wire' mode, cnv-gpios is required, for other modes it is optional
+  - if:
+      not:
+        required:
+          - adi,spi-mode
+    then:
+      required:
+        - cnv-gpios
+  # The configuration register can only be accessed in '3-wire' mode
+  - if:
+      not:
+        properties:
+          adi,spi-mode:
+            contains:
+              enum:
+                - single
+    then:
+      properties:
+        adi,high-z-input: false
+  # chain mode has lower SCLK max rate
+  - if:
+      required:
+        - adi,spi-mode
+      properties:
+        adi,spi-mode:
+          const: chain
+    then:
+      properties:
+        spi-max-frequency:
+          maximum: 50000000 # for VIO > 2.7 V, 40000000 for VIO > 1.7 V
+      required:
+        - '#daisy-chained-devices'
+    else:
+      properties:
+        '#daisy-chained-devices': false
+  # Gain property only applies to ADAQ devices
+  - if:
+      properties:
+        compatible:
+          not:
+            contains:
+              enum:
+                - adi,adaq4001
+                - adi,adaq4003
+    then:
+      properties:
+        adi,gain-milli: false
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        /* Example for a AD devices */
+        adc@0 {
+            compatible = "adi,ad4020";
+            reg = <0>;
+            spi-max-frequency = <71000000>;
+            vdd-supply = <&supply_1_8V>;
+            vio-supply = <&supply_1_8V>;
+            ref-supply = <&supply_5V>;
+            cnv-gpios = <&gpio0 88 GPIO_ACTIVE_HIGH>;
+        };
+    };
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        /* Example for a ADAQ devices */
+        adc@0 {
+            compatible = "adi,adaq4003";
+            reg = <0>;
+            spi-max-frequency = <80000000>;
+            vdd-supply = <&supply_1_8V>;
+            vio-supply = <&supply_1_8V>;
+            ref-supply = <&supply_5V>;
+            adi,high-z-input;
+            adi,gain-milli = /bits/ 16 <454>;
+            adi,spi-mode = "single";
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index bff979a507ba..1f052b9cd912 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1200,6 +1200,13 @@ W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
 F:	drivers/iio/dac/ad3552r.c
 
+ANALOG DEVICES INC AD4000 DRIVER
+M:	Marcelo Schmitt <marcelo.schmitt@analog.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
+
 ANALOG DEVICES INC AD4130 DRIVER
 M:	Cosmin Tanislav <cosmin.tanislav@analog.com>
 L:	linux-iio@vger.kernel.org
-- 
2.43.0

[PATCH v4 6/6] iio: adc: Add support for AD4000

[Marcelo Schmitt]

Add support for AD4000 series of low noise, low power, high speed,
successive aproximation register (SAR) ADCs.

Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
 MAINTAINERS              |   1 +
 drivers/iio/adc/Kconfig  |  12 +
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad4000.c | 715 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 729 insertions(+)
 create mode 100644 drivers/iio/adc/ad4000.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 1f052b9cd912..c732cf13f511 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1206,6 +1206,7 @@ L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
+F:	drivers/iio/adc/ad4000.c
 
 ANALOG DEVICES INC AD4130 DRIVER
 M:	Cosmin Tanislav <cosmin.tanislav@analog.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 5030319249c5..dcc49d9711a4 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -21,6 +21,18 @@ config AD_SIGMA_DELTA
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 
+config AD4000
+	tristate "Analog Devices AD4000 ADC Driver"
+	depends on SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for Analog Devices AD4000 high speed
+	  SPI analog to digital converters (ADC).
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad4000.
+
 config AD4130
 	tristate "Analog Device AD4130 ADC Driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 37ac689a0209..c32bd0ef6128 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -6,6 +6,7 @@
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
+obj-$(CONFIG_AD4000) += ad4000.o
 obj-$(CONFIG_AD4130) += ad4130.o
 obj-$(CONFIG_AD7091R) += ad7091r-base.o
 obj-$(CONFIG_AD7091R5) += ad7091r5.o
diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
new file mode 100644
index 000000000000..310f81a2a1d9
--- /dev/null
+++ b/drivers/iio/adc/ad4000.c
@@ -0,0 +1,715 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AD4000 SPI ADC driver
+ *
+ * Copyright 2024 Analog Devices Inc.
+ */
+#include <asm/unaligned.h>
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+#include <linux/units.h>
+#include <linux/util_macros.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+#define AD4000_READ_COMMAND	0x54
+#define AD4000_WRITE_COMMAND	0x14
+
+#define AD4000_CONFIG_REG_DEFAULT	0xE1
+
+/* AD4000 Configuration Register programmable bits */
+#define AD4000_CFG_STATUS		BIT(4) /* Status bits output */
+#define AD4000_CFG_SPAN_COMP		BIT(3) /* Input span compression  */
+#define AD4000_CFG_HIGHZ		BIT(2) /* High impedance mode  */
+#define AD4000_CFG_TURBO		BIT(1) /* Turbo mode */
+
+#define AD4000_TQUIET1_NS		190
+#define AD4000_TQUIET2_NS		60
+#define AD4000_TCONV_NS			320
+
+#define AD4000_18BIT_MSK	GENMASK(31, 14)
+#define AD4000_20BIT_MSK	GENMASK(31, 12)
+
+#define AD4000_DIFF_CHANNEL(_sign, _real_bits, _3wire)				\
+{										\
+	.type = IIO_VOLTAGE,							\
+	.indexed = 1,								\
+	.differential = 1,							\
+	.channel = 0,								\
+	.channel2 = 1,								\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
+			      BIT(IIO_CHAN_INFO_SCALE),				\
+	.info_mask_separate_available = _3wire ? BIT(IIO_CHAN_INFO_SCALE) : 0,	\
+	.scan_type = {								\
+		.sign = _sign,							\
+		.realbits = _real_bits,						\
+		.storagebits = _real_bits > 16 ? 32 : 16,			\
+		.shift = _real_bits > 16 ? 32 - _real_bits : 0,			\
+		.endianness = IIO_BE,						\
+	},									\
+}
+
+#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _3wire)			\
+{										\
+	.type = IIO_VOLTAGE,							\
+	.indexed = 1,								\
+	.channel = 0,								\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
+			      BIT(IIO_CHAN_INFO_SCALE) |			\
+			      BIT(IIO_CHAN_INFO_OFFSET),			\
+	.info_mask_separate_available = _3wire ? BIT(IIO_CHAN_INFO_SCALE) : 0,	\
+	.scan_type = {								\
+		.sign = _sign,							\
+		.realbits = _real_bits,						\
+		.storagebits = _real_bits > 16 ? 32 : 16,			\
+		.shift = _real_bits > 16 ? 32 - _real_bits : 0,			\
+		.endianness = IIO_BE,						\
+	},									\
+}
+
+enum ad4000_spi_mode {
+	/* datasheet calls this "4-wire mode" (controller CS goes to ADC SDI!) */
+	AD4000_SPI_MODE_DEFAULT,
+	/* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
+	AD4000_SPI_MODE_SINGLE,
+};
+
+/* maps adi,spi-mode property value to enum */
+static const char * const ad4000_spi_modes[] = {
+	[AD4000_SPI_MODE_DEFAULT] = "",
+	[AD4000_SPI_MODE_SINGLE] = "single",
+};
+
+struct ad4000_chip_info {
+	const char *dev_name;
+	struct iio_chan_spec chan_spec;
+	struct iio_chan_spec three_w_chan_spec;
+};
+
+static const struct ad4000_chip_info ad4000_chip_info = {
+	.dev_name = "ad4000",
+	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
+	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 1),
+};
+
+static const struct ad4000_chip_info ad4001_chip_info = {
+	.dev_name = "ad4001",
+	.chan_spec = AD4000_DIFF_CHANNEL('s', 16, 0),
+	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 16, 1),
+};
+
+static const struct ad4000_chip_info ad4002_chip_info = {
+	.dev_name = "ad4002",
+	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 0),
+	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 1),
+};
+
+static const struct ad4000_chip_info ad4003_chip_info = {
+	.dev_name = "ad4003",
+	.chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
+	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
+};
+
+static const struct ad4000_chip_info ad4004_chip_info = {
+	.dev_name = "ad4004",
+	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
+	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 1),
+};
+
+static const struct ad4000_chip_info ad4005_chip_info = {
+	.dev_name = "ad4005",
+	.chan_spec = AD4000_DIFF_CHANNEL('s', 16, 0),
+	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 16, 1),
+};
+
+static const struct ad4000_chip_info ad4006_chip_info = {
+	.dev_name = "ad4006",
+	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 0),
+	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 1),
+};
+
+static const struct ad4000_chip_info ad4007_chip_info = {
+	.dev_name = "ad4007",
+	.chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
+	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
+};
+
+static const struct ad4000_chip_info ad4008_chip_info = {
+	.dev_name = "ad4008",
+	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
+	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 1),
+};
+
+static const struct ad4000_chip_info ad4010_chip_info = {
+	.dev_name = "ad4010",
+	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 0),
+	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 1),
+};
+
+static const struct ad4000_chip_info ad4011_chip_info = {
+	.dev_name = "ad4011",
+	.chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
+	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
+};
+
+static const struct ad4000_chip_info ad4020_chip_info = {
+	.dev_name = "ad4020",
+	.chan_spec = AD4000_DIFF_CHANNEL('s', 20, 0),
+	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 20, 1),
+};
+
+static const struct ad4000_chip_info ad4021_chip_info = {
+	.dev_name = "ad4021",
+	.chan_spec = AD4000_DIFF_CHANNEL('s', 20, 0),
+	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 20, 1),
+};
+
+static const struct ad4000_chip_info ad4022_chip_info = {
+	.dev_name = "ad4022",
+	.chan_spec = AD4000_DIFF_CHANNEL('s', 20, 0),
+	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 20, 1),
+};
+
+static const struct ad4000_chip_info adaq4001_chip_info = {
+	.dev_name = "adaq4001",
+	.chan_spec = AD4000_DIFF_CHANNEL('s', 16, 0),
+	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 16, 1),
+};
+
+static const struct ad4000_chip_info adaq4003_chip_info = {
+	.dev_name = "adaq4003",
+	.chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
+	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
+};
+
+struct ad4000_state {
+	struct spi_device *spi;
+	struct gpio_desc *cnv_gpio;
+	struct spi_transfer xfers[2];
+	struct spi_message msg;
+	int vref_mv;
+	enum ad4000_spi_mode spi_mode;
+	bool span_comp;
+	bool turbo_mode;
+	u16 gain_milli;
+	int scale_tbl[2][2];
+
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	struct {
+		union {
+			__be16 sample_buf16;
+			__be32 sample_buf32;
+		} data;
+		s64 timestamp __aligned(8);
+	} scan __aligned(IIO_DMA_MINALIGN);
+	__be16 tx_buf;
+	__be16 rx_buf;
+};
+
+static void ad4000_fill_scale_tbl(struct ad4000_state *st,
+				  struct iio_chan_spec const *chan)
+{
+	int val, tmp0, tmp1;
+	int scale_bits;
+	u64 tmp2;
+
+	/*
+	 * ADCs that output two's complement code have one less bit to express
+	 * voltage magnitude.
+	 */
+	if (chan->scan_type.sign == 's')
+		scale_bits = chan->scan_type.realbits - 1;
+	else
+		scale_bits = chan->scan_type.realbits;
+
+	/*
+	 * The gain is stored as a fraction of 1000 and, as we need to
+	 * divide vref_mv by the gain, we invert the gain/1000 fraction.
+	 * Also multiply by an extra MILLI to preserve precision.
+	 * Thus, we have MILLI * MILLI equals MICRO as fraction numerator.
+	 */
+	val = mult_frac(st->vref_mv, MICRO, st->gain_milli);
+	/* Would multiply by NANO here but we multiplied by extra MILLI */
+	tmp2 = shift_right((u64)val * MICRO, scale_bits);
+	tmp0 = div_s64_rem(tmp2, NANO, &tmp1);
+	/* Store scale for when span compression is disabled */
+	st->scale_tbl[0][0] = tmp0; /* Integer part */
+	st->scale_tbl[0][1] = abs(tmp1); /* Fractional part */
+	/* Store scale for when span compression is enabled */
+	st->scale_tbl[1][0] = tmp0;
+	/* The integer part is always zero so don't bother to divide it. */
+	if (chan->differential)
+		st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
+	else
+		st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
+}
+
+static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
+{
+	st->tx_buf = cpu_to_be16(AD4000_WRITE_COMMAND << BITS_PER_BYTE | val);
+	return spi_write(st->spi, &st->tx_buf, sizeof(st->tx_buf));
+}
+
+static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
+{
+	struct spi_transfer t[] = {
+		{
+			.tx_buf = &st->tx_buf,
+			.rx_buf = &st->rx_buf,
+			.len = 2,
+		},
+	};
+	int ret;
+
+	st->tx_buf = cpu_to_be16(AD4000_READ_COMMAND << BITS_PER_BYTE);
+	ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
+	if (ret < 0)
+		return ret;
+
+	*val = be16_to_cpu(st->rx_buf);
+
+	return ret;
+}
+
+static void ad4000_unoptimize_msg(void *msg)
+{
+	spi_unoptimize_message(msg);
+}
+
+/*
+ * This executes a data sample transfer for when the device connections are
+ * in "3-wire" mode, selected by setting the adi,spi-mode device tree property
+ * to "single". In this connection mode, the ADC SDI pin is connected to MOSI or
+ * to VIO and ADC CNV pin is connected either to a SPI controller CS or to a GPIO.
+ * AD4000 series of devices initiate conversions on the rising edge of CNV pin.
+ *
+ * If the CNV pin is connected to an SPI controller CS line (which is by default
+ * active low), the ADC readings would have a latency (delay) of one read.
+ * Moreover, since we also do ADC sampling for filling the buffer on triggered
+ * buffer mode, the timestamps of buffer readings would be disarranged.
+ * To prevent the read latency and reduce the time discrepancy between the
+ * sample read request and the time of actual sampling by the ADC, do a
+ * preparatory transfer to pulse the CS/CNV line.
+ */
+static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st,
+					     const struct iio_chan_spec *chan)
+{
+	unsigned int cnv_pulse_time = st->turbo_mode ? AD4000_TQUIET1_NS
+						     : AD4000_TCONV_NS;
+	struct spi_transfer *xfers = st->xfers;
+	int ret;
+
+	xfers[0].cs_change = 1;
+	xfers[0].cs_change_delay.value = cnv_pulse_time;
+	xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
+
+	xfers[1].rx_buf = &st->scan.data;
+	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
+	xfers[1].delay.value = AD4000_TQUIET2_NS;
+	xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS;
+
+	spi_message_init_with_transfers(&st->msg, st->xfers, 2);
+
+	ret = spi_optimize_message(st->spi, &st->msg);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(&st->spi->dev, ad4000_unoptimize_msg,
+					&st->msg);
+}
+
+/*
+ * This executes a data sample transfer for when the device connections are
+ * in "4-wire" mode, selected when the adi,spi-mode device tree
+ * property is absent or empty. In this connection mode, the controller CS pin
+ * is connected to ADC SDI pin and a GPIO is connected to ADC CNV pin.
+ * The GPIO connected to ADC CNV pin is set outside of the SPI transfer.
+ */
+static int ad4000_prepare_4wire_mode_message(struct ad4000_state *st,
+					     const struct iio_chan_spec *chan)
+{
+	unsigned int cnv_to_sdi_time = st->turbo_mode ? AD4000_TQUIET1_NS
+						      : AD4000_TCONV_NS;
+	struct spi_transfer *xfers = st->xfers;
+	int ret;
+
+	/*
+	 * Dummy transfer to cause enough delay between CNV going high and SDI
+	 * going low.
+	 */
+	xfers[0].cs_off = 1;
+	xfers[0].delay.value = cnv_to_sdi_time;
+	xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
+
+	xfers[1].rx_buf = &st->scan.data;
+	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
+
+	spi_message_init_with_transfers(&st->msg, st->xfers, 2);
+
+	ret = spi_optimize_message(st->spi, &st->msg);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(&st->spi->dev, ad4000_unoptimize_msg,
+					&st->msg);
+}
+
+static int ad4000_convert_and_acquire(struct ad4000_state *st)
+{
+	int ret;
+
+	/*
+	 * In 4-wire mode, the CNV line is held high for the entire conversion
+	 * and acquisition process. In other modes, the CNV GPIO is optional
+	 * and, if provided, replaces controller CS. If CNV GPIO is not defined
+	 * gpiod_set_value_cansleep() has no effect.
+	 */
+	gpiod_set_value_cansleep(st->cnv_gpio, 1);
+	ret = spi_sync(st->spi, &st->msg);
+	gpiod_set_value_cansleep(st->cnv_gpio, 0);
+
+	return ret;
+}
+
+static int ad4000_single_conversion(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan, int *val)
+{
+	struct ad4000_state *st = iio_priv(indio_dev);
+	u32 sample;
+	int ret;
+
+	ret = ad4000_convert_and_acquire(st);
+	if (ret < 0)
+		return ret;
+
+	if (chan->scan_type.storagebits > 16)
+		sample = be32_to_cpu(st->scan.data.sample_buf32);
+	else
+		sample = be16_to_cpu(st->scan.data.sample_buf16);
+
+	switch (chan->scan_type.realbits) {
+	case 16:
+		break;
+	case 18:
+		sample = FIELD_GET(AD4000_18BIT_MSK, sample);
+		break;
+	case 20:
+		sample = FIELD_GET(AD4000_20BIT_MSK, sample);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (chan->scan_type.sign == 's')
+		*val = sign_extend32(sample, chan->scan_type.realbits - 1);
+
+	return IIO_VAL_INT;
+}
+
+static int ad4000_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long info)
+{
+	struct ad4000_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+			return ad4000_single_conversion(indio_dev, chan, val);
+		unreachable();
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->scale_tbl[st->span_comp][0];
+		*val2 = st->scale_tbl[st->span_comp][1];
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = 0;
+		if (st->span_comp)
+			*val = mult_frac(st->vref_mv, 1, 10);
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4000_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long info)
+{
+	struct ad4000_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (int *)st->scale_tbl;
+		*length = 2 * 2;
+		*type = IIO_VAL_INT_PLUS_NANO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+}
+
+static int ad4000_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val, int val2,
+			    long mask)
+{
+	struct ad4000_state *st = iio_priv(indio_dev);
+	unsigned int reg_val;
+	bool span_comp_en;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+			ret = ad4000_read_reg(st, &reg_val);
+			if (ret < 0)
+				return ret;
+
+			span_comp_en = val2 == st->scale_tbl[1][1];
+			reg_val &= ~AD4000_CFG_SPAN_COMP;
+			reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en);
+
+			ret = ad4000_write_reg(st, reg_val);
+			if (ret < 0)
+				return ret;
+
+			st->span_comp = span_comp_en;
+			return 0;
+		}
+		unreachable();
+	default:
+		return -EINVAL;
+	}
+}
+
+static irqreturn_t ad4000_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ad4000_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad4000_convert_and_acquire(st);
+	if (ret < 0)
+		goto err_out;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
+					   iio_get_time_ns(indio_dev));
+
+err_out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info ad4000_3wire_info = {
+	.read_raw = &ad4000_read_raw,
+	.read_avail = &ad4000_read_avail,
+	.write_raw = &ad4000_write_raw,
+	.write_raw_get_fmt = &ad4000_write_raw_get_fmt,
+
+};
+
+static const struct iio_info ad4000_info = {
+	.read_raw = &ad4000_read_raw,
+};
+
+static int ad4000_config(struct ad4000_state *st)
+{
+	unsigned int reg_val = AD4000_CONFIG_REG_DEFAULT;
+
+	if (device_property_present(&st->spi->dev, "adi,high-z-input"))
+		reg_val |= FIELD_PREP(AD4000_CFG_HIGHZ, 1);
+
+	return ad4000_write_reg(st, reg_val);
+}
+
+static int ad4000_probe(struct spi_device *spi)
+{
+	const struct ad4000_chip_info *chip;
+	struct iio_dev *indio_dev;
+	struct ad4000_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	chip = spi_get_device_match_data(spi);
+	if (!chip)
+		return -EINVAL;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+
+	ret = devm_regulator_get_enable(&spi->dev, "vdd");
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "Failed to enable VDD supply\n");
+
+	ret = devm_regulator_get_enable(&spi->dev, "vio");
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "Failed to enable VIO supply\n");
+
+	st->vref_mv = devm_regulator_get_enable_read_voltage(&spi->dev, "ref");
+	if (ret < 0)
+		return dev_err_probe(&spi->dev, st->vref_mv,
+				     "Failed to get ref regulator reference\n");
+	st->vref_mv = st->vref_mv / 1000;
+
+	st->cnv_gpio = devm_gpiod_get_optional(&spi->dev, "cnv", GPIOD_OUT_HIGH);
+	if (IS_ERR(st->cnv_gpio))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio),
+				     "Failed to get CNV GPIO");
+
+	ret = device_property_match_property_string(&spi->dev, "adi,spi-mode",
+						    ad4000_spi_modes,
+						    ARRAY_SIZE(ad4000_spi_modes));
+	/* Default to 4-wire mode if adi,spi-mode property is not present */
+	if (ret == -EINVAL)
+		st->spi_mode = AD4000_SPI_MODE_DEFAULT;
+	else if (ret < 0)
+		return dev_err_probe(&spi->dev, ret,
+				     "getting adi,spi-mode property failed\n");
+	else
+		st->spi_mode = ret;
+
+	switch (st->spi_mode) {
+	case AD4000_SPI_MODE_DEFAULT:
+		indio_dev->info = &ad4000_info;
+		indio_dev->channels = &chip->chan_spec;
+		ret = ad4000_prepare_4wire_mode_message(st, indio_dev->channels);
+		if (ret)
+			return ret;
+
+		break;
+	case AD4000_SPI_MODE_SINGLE:
+		indio_dev->info = &ad4000_3wire_info;
+		indio_dev->channels = &chip->three_w_chan_spec;
+
+		/*
+		 * In "3-wire mode", the ADC SDI line must be kept high when
+		 * data is not being clocked out of the controller.
+		 * Request the SPI controller to make MOSI idle high.
+		 */
+		spi->mode = SPI_MODE_0 | SPI_MOSI_IDLE_HIGH;
+		ret = spi_setup(spi);
+		if (ret < 0)
+			return ret;
+
+		ret = ad4000_prepare_3wire_mode_message(st, indio_dev->channels);
+		if (ret)
+			return ret;
+
+		ret = ad4000_config(st);
+		if (ret < 0)
+			dev_warn(&st->spi->dev, "Failed to config device\n");
+
+		break;
+	}
+
+	indio_dev->name = chip->dev_name;
+	indio_dev->num_channels = 1;
+
+	/* Hardware gain only applies to ADAQ devices */
+	st->gain_milli = 1000;
+	if (device_property_present(&spi->dev, "adi,gain-milli")) {
+		ret = device_property_read_u16(&spi->dev, "adi,gain-milli",
+					       &st->gain_milli);
+		if (ret)
+			return dev_err_probe(&spi->dev, ret,
+					     "Failed to read gain property\n");
+	}
+
+	ad4000_fill_scale_tbl(st, indio_dev->channels);
+
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+					      &iio_pollfunc_store_time,
+					      &ad4000_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id ad4000_id[] = {
+	{ "ad4000", (kernel_ulong_t)&ad4000_chip_info },
+	{ "ad4001", (kernel_ulong_t)&ad4001_chip_info },
+	{ "ad4002", (kernel_ulong_t)&ad4002_chip_info },
+	{ "ad4003", (kernel_ulong_t)&ad4003_chip_info },
+	{ "ad4004", (kernel_ulong_t)&ad4004_chip_info },
+	{ "ad4005", (kernel_ulong_t)&ad4005_chip_info },
+	{ "ad4006", (kernel_ulong_t)&ad4006_chip_info },
+	{ "ad4007", (kernel_ulong_t)&ad4007_chip_info },
+	{ "ad4008", (kernel_ulong_t)&ad4008_chip_info },
+	{ "ad4010", (kernel_ulong_t)&ad4010_chip_info },
+	{ "ad4011", (kernel_ulong_t)&ad4011_chip_info },
+	{ "ad4020", (kernel_ulong_t)&ad4020_chip_info },
+	{ "ad4021", (kernel_ulong_t)&ad4021_chip_info },
+	{ "ad4022", (kernel_ulong_t)&ad4022_chip_info },
+	{ "adaq4001", (kernel_ulong_t)&adaq4001_chip_info },
+	{ "adaq4003", (kernel_ulong_t)&adaq4003_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad4000_id);
+
+static const struct of_device_id ad4000_of_match[] = {
+	{ .compatible = "adi,ad4000", .data = &ad4000_chip_info },
+	{ .compatible = "adi,ad4001", .data = &ad4001_chip_info },
+	{ .compatible = "adi,ad4002", .data = &ad4002_chip_info },
+	{ .compatible = "adi,ad4003", .data = &ad4003_chip_info },
+	{ .compatible = "adi,ad4004", .data = &ad4004_chip_info },
+	{ .compatible = "adi,ad4005", .data = &ad4005_chip_info },
+	{ .compatible = "adi,ad4006", .data = &ad4006_chip_info },
+	{ .compatible = "adi,ad4007", .data = &ad4007_chip_info },
+	{ .compatible = "adi,ad4008", .data = &ad4008_chip_info },
+	{ .compatible = "adi,ad4010", .data = &ad4010_chip_info },
+	{ .compatible = "adi,ad4011", .data = &ad4011_chip_info },
+	{ .compatible = "adi,ad4020", .data = &ad4020_chip_info },
+	{ .compatible = "adi,ad4021", .data = &ad4021_chip_info },
+	{ .compatible = "adi,ad4022", .data = &ad4022_chip_info },
+	{ .compatible = "adi,adaq4001", .data = &adaq4001_chip_info },
+	{ .compatible = "adi,adaq4003", .data = &adaq4003_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad4000_of_match);
+
+static struct spi_driver ad4000_driver = {
+	.driver = {
+		.name   = "ad4000",
+		.of_match_table = ad4000_of_match,
+	},
+	.probe          = ad4000_probe,
+	.id_table       = ad4000_id,
+};
+module_spi_driver(ad4000_driver);
+
+MODULE_AUTHOR("Marcelo Schmitt <marcelo.schmitt@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD4000 ADC driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 346 bytes --]

On Tue, Jun 18, 2024 at 08:10:44PM -0300, Marcelo Schmitt wrote:

> First replies to v3 brought the idea of having a feature detection mechanism.
> I didn't really get how to do that. If feature detection is required, can
> somebody please provide some pointers on how to implement that?

Look at the checks in spi_setup() for bad_bits.  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marcelo Schmitt]

On 06/19, Mark Brown wrote:
> On Tue, Jun 18, 2024 at 08:10:44PM -0300, Marcelo Schmitt wrote:
> 
> > First replies to v3 brought the idea of having a feature detection mechanism.
> > I didn't really get how to do that. If feature detection is required, can
> > somebody please provide some pointers on how to implement that?
> 
> Look at the checks in spi_setup() for bad_bits.  

Yes, I added MOSI idle configuration bits to bad_bits so spi_setup() fails
if the feature is requested but not supported:

	bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD |
				 SPI_NO_TX | SPI_NO_RX | SPI_MOSI_IDLE_LOW |
				 SPI_MOSI_IDLE_HIGH);

am I still missing anything?

Thanks,
Marcelo

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

On Wed, Jun 19, 2024 at 09:42:23AM -0300, Marcelo Schmitt wrote:
> On 06/19, Mark Brown wrote:
> > On Tue, Jun 18, 2024 at 08:10:44PM -0300, Marcelo Schmitt wrote:

> > > First replies to v3 brought the idea of having a feature detection mechanism.
> > > I didn't really get how to do that. If feature detection is required, can
> > > somebody please provide some pointers on how to implement that?

> > Look at the checks in spi_setup() for bad_bits.  

> Yes, I added MOSI idle configuration bits to bad_bits so spi_setup() fails
> if the feature is requested but not supported:

> 	bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD |
> 				 SPI_NO_TX | SPI_NO_RX | SPI_MOSI_IDLE_LOW |
> 				 SPI_MOSI_IDLE_HIGH);

> am I still missing anything?

That should be fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 5/6] dt-bindings: iio: adc: Add AD4000

[David Lechner]

On 6/18/24 6:12 PM, Marcelo Schmitt wrote:
> Add device tree documentation for AD4000 series of ADC devices.
> 
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf

Datasheets URLs are listed in the patch, so probably don't need them
in the commit message too.

> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad4000.yaml          | 231 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 238 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> new file mode 100644
> index 000000000000..ba50f9e0784b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> @@ -0,0 +1,231 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD4000 and similar Analog to Digital Converters
> +
> +maintainers:
> +  - Marcelo Schmitt <marcelo.schmitt@analog.com>
> +
> +description: |
> +  Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
> +  Specifications can be found at:
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> +
> +$ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: adi,ad4000
> +      - items:
> +          - enum:
> +              - adi,ad4004
> +              - adi,ad4008
> +          - const: adi,ad4000
> +      - const: adi,ad4001
> +      - items:
> +          - enum:
> +              - adi,ad4005
> +          - const: adi,ad4001
> +      - const: adi,ad4002
> +      - items:
> +          - enum:
> +              - adi,ad4006
> +              - adi,ad4010
> +          - const: adi,ad4002
> +      - const: adi,ad4003
> +      - items:
> +          - enum:
> +              - adi,ad4007
> +              - adi,ad4011
> +          - const: adi,ad4003
> +      - const: adi,ad4020
> +      - items:
> +          - enum:
> +              - adi,ad4021
> +              - adi,ad4022
> +          - const: adi,ad4020
> +

There are data sheets listed for adaq400x but no compatibles.

> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
> +
> +  adi,spi-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ single, chain ]
> +    description: |
> +      This property indicates the SPI wiring configuration.
> +
> +      When this property is omitted, it is assumed that the device is using what
> +      the datasheet calls "4-wire mode". This is the conventional SPI mode used
> +      when there are multiple devices on the same bus. In this mode, the CNV
> +      line is used to initiate the conversion and the SDI line is connected to
> +      CS on the SPI controller.
> +
> +      When this property is present, it indicates that the device is using one
> +      of the following alternative wiring configurations:
> +
> +      * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
> +        definition of 3-wire mode is NOT at all related to the standard
> +        spi-3wire property!) This mode is often used when the ADC is the only
> +        device on the bus. In this mode, SDI is connected to MOSI or to VIO, and
> +        the CNV line can be connected to the CS line of the SPI controller or to
> +        a GPIO, in which case the CS line of the controller is unused.
> +      * chain: The datasheet calls this "chain mode". This mode is used to save
> +        on wiring when multiple ADCs are used. In this mode, the SDI line of
> +        one chip is tied to the SDO of the next chip in the chain and the SDI of
> +        the last chip in the chain is tied to GND. Only the first chip in the
> +        chain is connected to the SPI bus. The CNV line of all chips are tied
> +        together. The CS line of the SPI controller can be used as the CNV line
> +        only if it is active high.
> +
> +  '#daisy-chained-devices': true
> +
> +  vdd-supply:
> +    description: A 1.8V supply that powers the chip (VDD).
> +
> +  vio-supply:
> +    description:
> +      A 1.8V to 5.5V supply for the digital inputs and outputs (VIO).
> +
> +  ref-supply:
> +    description:
> +      A 2.5 to 5V supply for the external reference voltage (REF).
> +
> +  cnv-gpios:
> +    description:
> +      The Convert Input (CNV). This input has multiple functions. It initiates
> +      the conversions and selects the SPI mode of the device (chain or CS). In
> +      'single' mode, this property is omitted if the CNV pin is connected to the
> +      CS line of the SPI controller.
> +    maxItems: 1
> +
> +  adi,high-z-input:
> +    type: boolean
> +    description:
> +      High-Z mode allows the amplifier and RC filter in front of the ADC to be
> +      chosen based on the signal bandwidth of interest, rather than the settling
> +      requirements of the switched capacitor SAR ADC inputs.
> +
> +  adi,gain-milli:
> +    description: |
> +      The hardware gain applied to the ADC input (in milli units).
> +      The gain provided by the ADC input scaler is defined by the hardware
> +      connections between chip pins OUT+, R1K-, R1K1-, R1K+, R1K1+, and OUT-.
> +      If not present, default to 1000 (no actual gain applied).
> +    $ref: /schemas/types.yaml#/definitions/uint16

Any particular reason why this needs to be uint16 instead of the more
common uint32?

> +    enum: [454, 909, 1000, 1900]
> +    default: 1000
> +
> +  interrupts:
> +    description:
> +      The SDO pin can also function as a busy indicator. This node should be
> +      connected to an interrupt that is triggered when the SDO line goes low
> +      while the SDI line is high and the CNV line is low ('single' mode) or the
> +      SDI line is low and the CNV line is high ('multi' mode); or when the SDO
> +      line goes high while the SDI and CNV lines are high (chain mode),
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +  - vio-supply
> +  - ref-supply
> +
> +allOf:
> +  # in '4-wire' mode, cnv-gpios is required, for other modes it is optional
> +  - if:
> +      not:
> +        required:
> +          - adi,spi-mode
> +    then:
> +      required:
> +        - cnv-gpios
> +  # The configuration register can only be accessed in '3-wire' mode
> +  - if:
> +      not:
> +        properties:
> +          adi,spi-mode:
> +            contains:
> +              enum:
> +                - single

adi,spi-mode is not an array and we are only checking for one value,
so this could be simplified to:

  properties:
    adi,spi-mode:
      const: single

> +    then:
> +      properties:
> +        adi,high-z-input: false
> +  # chain mode has lower SCLK max rate
> +  - if:
> +      required:
> +        - adi,spi-mode
> +      properties:
> +        adi,spi-mode:
> +          const: chain
> +    then:
> +      properties:
> +        spi-max-frequency:
> +          maximum: 50000000 # for VIO > 2.7 V, 40000000 for VIO > 1.7 V
> +      required:
> +        - '#daisy-chained-devices'
> +    else:
> +      properties:
> +        '#daisy-chained-devices': false
> +  # Gain property only applies to ADAQ devices
> +  - if:
> +      properties:
> +        compatible:
> +          not:
> +            contains:
> +              enum:
> +                - adi,adaq4001
> +                - adi,adaq4003
> +    then:
> +      properties:
> +        adi,gain-milli: false
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        /* Example for a AD devices */

Comments are a bit redundant since it says "examples:" above and
the type of the chip in the compatible string.

> +        adc@0 {
> +            compatible = "adi,ad4020";
> +            reg = <0>;
> +            spi-max-frequency = <71000000>;
> +            vdd-supply = <&supply_1_8V>;
> +            vio-supply = <&supply_1_8V>;
> +            ref-supply = <&supply_5V>;
> +            cnv-gpios = <&gpio0 88 GPIO_ACTIVE_HIGH>;
> +        };
> +    };
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        /* Example for a ADAQ devices */
> +        adc@0 {
> +            compatible = "adi,adaq4003";
> +            reg = <0>;
> +            spi-max-frequency = <80000000>;
> +            vdd-supply = <&supply_1_8V>;
> +            vio-supply = <&supply_1_8V>;
> +            ref-supply = <&supply_5V>;
> +            adi,high-z-input;
> +            adi,gain-milli = /bits/ 16 <454>;
> +            adi,spi-mode = "single";
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bff979a507ba..1f052b9cd912 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1200,6 +1200,13 @@ W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>  F:	drivers/iio/dac/ad3552r.c
>  
> +ANALOG DEVICES INC AD4000 DRIVER
> +M:	Marcelo Schmitt <marcelo.schmitt@analog.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Supported
> +W:	https://ez.analog.com/linux-software-drivers
> +F:	Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> +
>  ANALOG DEVICES INC AD4130 DRIVER
>  M:	Cosmin Tanislav <cosmin.tanislav@analog.com>
>  L:	linux-iio@vger.kernel.org

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

[David Lechner]

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

Re: [PATCH v4 4/6] spi: spi-axi-spi-engine: Add support for MOSI idle configuration

[David Lechner]

On 6/18/24 6:11 PM, Marcelo Schmitt wrote:
> Implement MOSI idle low and MOSI idle high to better support peripherals
> that request specific MOSI behavior.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
>  drivers/spi/spi-axi-spi-engine.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
> index 0aa31d745734..787e22ae80c0 100644
> --- a/drivers/spi/spi-axi-spi-engine.c
> +++ b/drivers/spi/spi-axi-spi-engine.c
> @@ -41,6 +41,7 @@
>  #define SPI_ENGINE_CONFIG_CPHA			BIT(0)
>  #define SPI_ENGINE_CONFIG_CPOL			BIT(1)
>  #define SPI_ENGINE_CONFIG_3WIRE			BIT(2)
> +#define SPI_ENGINE_CONFIG_SDO_IDLE		BIT(3)

Calling this SPI_ENGINE_CONFIG_SDO_IDLE_HIGH would make it more
clear what happens when the bit is enabled.

>  
>  #define SPI_ENGINE_INST_TRANSFER		0x0
>  #define SPI_ENGINE_INST_ASSERT			0x1
> @@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct spi_device *spi)
>  		config |= SPI_ENGINE_CONFIG_CPHA;
>  	if (spi->mode & SPI_3WIRE)
>  		config |= SPI_ENGINE_CONFIG_3WIRE;
> +	if (spi->mode & SPI_MOSI_IDLE_HIGH)
> +		config |= SPI_ENGINE_CONFIG_SDO_IDLE;
> +	if (spi->mode & SPI_MOSI_IDLE_LOW)
> +		config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;
>  
>  	return config;
>  }
> @@ -646,6 +651,9 @@ static int spi_engine_probe(struct platform_device *pdev)
>  
>  	host->dev.of_node = pdev->dev.of_node;
>  	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
> +	if (ADI_AXI_PCORE_VER_MAJOR(version) >= 1 &&

Currently, the major version is required to be 1, so this check is not
strictly needed.

> +	    ADI_AXI_PCORE_VER_MINOR(version) >= 3)
> +		host->mode_bits |=  SPI_MOSI_IDLE_LOW | SPI_MOSI_IDLE_HIGH;



>  	host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
>  	host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2;
>  	host->transfer_one_message = spi_engine_transfer_one_message;

Re: [PATCH v4 6/6] iio: adc: Add support for AD4000

[David Lechner]

On 6/18/24 6:12 PM, Marcelo Schmitt wrote:
> Add support for AD4000 series of low noise, low power, high speed,
> successive aproximation register (SAR) ADCs.

spelling: approximation

Probably also worth mentioning ADAQ here too.

> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---

...

> diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
> new file mode 100644
> index 000000000000..310f81a2a1d9
> --- /dev/null
> +++ b/drivers/iio/adc/ad4000.c
> @@ -0,0 +1,715 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD4000 SPI ADC driver
> + *
> + * Copyright 2024 Analog Devices Inc.

Would be handy to have links to the datasheets here too.

> + */
> +#include <asm/unaligned.h>

Doesn't look like this header is needed.

> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>

Or this one.

> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>

Or this one.

> +#include <linux/units.h>
> +#include <linux/util_macros.h>

I would put a blank line here or keep iio/*
in alphabetical order with the rest.

> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

Not used?

> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#define AD4000_READ_COMMAND	0x54
> +#define AD4000_WRITE_COMMAND	0x14
> +
> +#define AD4000_CONFIG_REG_DEFAULT	0xE1
> +
> +/* AD4000 Configuration Register programmable bits */
> +#define AD4000_CFG_STATUS		BIT(4) /* Status bits output */
> +#define AD4000_CFG_SPAN_COMP		BIT(3) /* Input span compression  */
> +#define AD4000_CFG_HIGHZ		BIT(2) /* High impedance mode  */
> +#define AD4000_CFG_TURBO		BIT(1) /* Turbo mode */> +
> +#define AD4000_TQUIET1_NS		190
> +#define AD4000_TQUIET2_NS		60
> +#define AD4000_TCONV_NS			320
> +
> +#define AD4000_18BIT_MSK	GENMASK(31, 14)
> +#define AD4000_20BIT_MSK	GENMASK(31, 12)
> +
> +#define AD4000_DIFF_CHANNEL(_sign, _real_bits, _3wire)				\

_3wire isn't very clear what the intention is. _can_write_reg is what we
really mean, right?

It is still technically possible to have a 3-wire mode where we can't write
registers because the SPI controller doesn't support MOSI idle high or MOSI
isn't wired up at all. So even if the driver doesn't currently support this,
saying that _3wire implies that we can write registers still seems misleading.


> +{										\
> +	.type = IIO_VOLTAGE,							\
> +	.indexed = 1,								\
> +	.differential = 1,							\
> +	.channel = 0,								\
> +	.channel2 = 1,								\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
> +			      BIT(IIO_CHAN_INFO_SCALE),				\> +	.info_mask_separate_available = _3wire ? BIT(IIO_CHAN_INFO_SCALE) : 0,	\
> +	.scan_type = {								\
> +		.sign = _sign,							\
> +		.realbits = _real_bits,						\
> +		.storagebits = _real_bits > 16 ? 32 : 16,			\
> +		.shift = _real_bits > 16 ? 32 - _real_bits : 0,			\
> +		.endianness = IIO_BE,						\
> +	},									\
> +}
> +
> +#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _3wire)			\
> +{										\
> +	.type = IIO_VOLTAGE,							\
> +	.indexed = 1,								\
> +	.channel = 0,								\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
> +			      BIT(IIO_CHAN_INFO_SCALE) |			\
> +			      BIT(IIO_CHAN_INFO_OFFSET),			\> +	.info_mask_separate_available = _3wire ? BIT(IIO_CHAN_INFO_SCALE) : 0,	\
> +	.scan_type = {								\
> +		.sign = _sign,							\
> +		.realbits = _real_bits,						\
> +		.storagebits = _real_bits > 16 ? 32 : 16,			\
> +		.shift = _real_bits > 16 ? 32 - _real_bits : 0,			\
> +		.endianness = IIO_BE,						\
> +	},									\
> +}
> +
> +enum ad4000_spi_mode {
> +	/* datasheet calls this "4-wire mode" (controller CS goes to ADC SDI!) */
> +	AD4000_SPI_MODE_DEFAULT,
> +	/* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
> +	AD4000_SPI_MODE_SINGLE,> +};
> +
> +/* maps adi,spi-mode property value to enum */
> +static const char * const ad4000_spi_modes[] = {
> +	[AD4000_SPI_MODE_DEFAULT] = "",
> +	[AD4000_SPI_MODE_SINGLE] = "single",
> +};
> +
> +struct ad4000_chip_info {
> +	const char *dev_name;
> +	struct iio_chan_spec chan_spec;
> +	struct iio_chan_spec three_w_chan_spec;
> +};

I understand the reason for doing this, but it still seems a bit weird
to me to have two different sets of specs for the same chip. I guess
we'll see what Jonathan has to say about this.

> +
> +static const struct ad4000_chip_info ad4000_chip_info = {
> +	.dev_name = "ad4000",
> +	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
> +	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4001_chip_info = {
> +	.dev_name = "ad4001",
> +	.chan_spec = AD4000_DIFF_CHANNEL('s', 16, 0),
> +	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 16, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4002_chip_info = {
> +	.dev_name = "ad4002",
> +	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 0),
> +	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4003_chip_info = {
> +	.dev_name = "ad4003",
> +	.chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
> +	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4004_chip_info = {
> +	.dev_name = "ad4004",
> +	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
> +	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4005_chip_info = {
> +	.dev_name = "ad4005",
> +	.chan_spec = AD4000_DIFF_CHANNEL('s', 16, 0),
> +	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 16, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4006_chip_info = {
> +	.dev_name = "ad4006",
> +	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 0),
> +	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4007_chip_info = {
> +	.dev_name = "ad4007",
> +	.chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
> +	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4008_chip_info = {
> +	.dev_name = "ad4008",
> +	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
> +	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4010_chip_info = {
> +	.dev_name = "ad4010",
> +	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 0),
> +	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4011_chip_info = {
> +	.dev_name = "ad4011",
> +	.chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
> +	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4020_chip_info = {
> +	.dev_name = "ad4020",
> +	.chan_spec = AD4000_DIFF_CHANNEL('s', 20, 0),
> +	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 20, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4021_chip_info = {
> +	.dev_name = "ad4021",
> +	.chan_spec = AD4000_DIFF_CHANNEL('s', 20, 0),
> +	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 20, 1),
> +};
> +
> +static const struct ad4000_chip_info ad4022_chip_info = {
> +	.dev_name = "ad4022",
> +	.chan_spec = AD4000_DIFF_CHANNEL('s', 20, 0),
> +	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 20, 1),
> +};
> +
> +static const struct ad4000_chip_info adaq4001_chip_info = {
> +	.dev_name = "adaq4001",
> +	.chan_spec = AD4000_DIFF_CHANNEL('s', 16, 0),
> +	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 16, 1),
> +};
> +
> +static const struct ad4000_chip_info adaq4003_chip_info = {
> +	.dev_name = "adaq4003",
> +	.chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
> +	.three_w_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
> +};
> +
> +struct ad4000_state {
> +	struct spi_device *spi;
> +	struct gpio_desc *cnv_gpio;
> +	struct spi_transfer xfers[2];
> +	struct spi_message msg;
> +	int vref_mv;
> +	enum ad4000_spi_mode spi_mode;
> +	bool span_comp;
> +	bool turbo_mode;
> +	u16 gain_milli;
> +	int scale_tbl[2][2];
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 */
> +	struct {
> +		union {
> +			__be16 sample_buf16;
> +			__be32 sample_buf32;
> +		} data;
> +		s64 timestamp __aligned(8);
> +	} scan __aligned(IIO_DMA_MINALIGN);
> +	__be16 tx_buf;
> +	__be16 rx_buf;
> +};
> +
> +static void ad4000_fill_scale_tbl(struct ad4000_state *st,
> +				  struct iio_chan_spec const *chan)
> +{
> +	int val, tmp0, tmp1;
> +	int scale_bits;
> +	u64 tmp2;
> +
> +	/*
> +	 * ADCs that output two's complement code have one less bit to express
> +	 * voltage magnitude.
> +	 */
> +	if (chan->scan_type.sign == 's')
> +		scale_bits = chan->scan_type.realbits - 1;
> +	else
> +		scale_bits = chan->scan_type.realbits;
> +
> +	/*
> +	 * The gain is stored as a fraction of 1000 and, as we need to
> +	 * divide vref_mv by the gain, we invert the gain/1000 fraction.
> +	 * Also multiply by an extra MILLI to preserve precision.
> +	 * Thus, we have MILLI * MILLI equals MICRO as fraction numerator.
> +	 */
> +	val = mult_frac(st->vref_mv, MICRO, st->gain_milli);
> +	/* Would multiply by NANO here but we multiplied by extra MILLI */
> +	tmp2 = shift_right((u64)val * MICRO, scale_bits);
> +	tmp0 = div_s64_rem(tmp2, NANO, &tmp1);
> +	/* Store scale for when span compression is disabled */
> +	st->scale_tbl[0][0] = tmp0; /* Integer part */
> +	st->scale_tbl[0][1] = abs(tmp1); /* Fractional part */
> +	/* Store scale for when span compression is enabled */
> +	st->scale_tbl[1][0] = tmp0;
> +	/* The integer part is always zero so don't bother to divide it. */
> +	if (chan->differential)
> +		st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
> +	else
> +		st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
> +}
> +
> +static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
> +{
> +	st->tx_buf = cpu_to_be16(AD4000_WRITE_COMMAND << BITS_PER_BYTE | val);

Seems like a complicated way of writing:

	st->tx_buf[0] = AD4000_WRITE_COMMAND;
	st->tx_buf[1] = val;

(tx_buf would of course have to be changed to `u8 tx_buf[2]`)

> +	return spi_write(st->spi, &st->tx_buf, sizeof(st->tx_buf));
> +}
> +
> +static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
> +{
> +	struct spi_transfer t[] = {
> +		{
> +			.tx_buf = &st->tx_buf,
> +			.rx_buf = &st->rx_buf,
> +			.len = 2,
> +		},
> +	};

Don't need array for one item.

> +	int ret;
> +
> +	st->tx_buf = cpu_to_be16(AD4000_READ_COMMAND << BITS_PER_BYTE);
> +	ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = be16_to_cpu(st->rx_buf);

This doesn't mask out the "don't care" bits. This could just be:

	*val = st->rx_buf[1];

(rx_buf would of course have to be changed to `u8 rx_buf[2]`)

...

or could just replace all of this this will spi_w8r8() and have
a one-line function.

> +
> +	return ret;
> +}
> +
> +static void ad4000_unoptimize_msg(void *msg)
> +{
> +	spi_unoptimize_message(msg);
> +}
> +
> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "3-wire" mode, selected by setting the adi,spi-mode device tree property
> + * to "single". In this connection mode, the ADC SDI pin is connected to MOSI or
> + * to VIO and ADC CNV pin is connected either to a SPI controller CS or to a GPIO.
> + * AD4000 series of devices initiate conversions on the rising edge of CNV pin.
> + *
> + * If the CNV pin is connected to an SPI controller CS line (which is by default
> + * active low), the ADC readings would have a latency (delay) of one read.
> + * Moreover, since we also do ADC sampling for filling the buffer on triggered
> + * buffer mode, the timestamps of buffer readings would be disarranged.
> + * To prevent the read latency and reduce the time discrepancy between the
> + * sample read request and the time of actual sampling by the ADC, do a
> + * preparatory transfer to pulse the CS/CNV line.
> + */
> +static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st,
> +					     const struct iio_chan_spec *chan)
> +{
> +	unsigned int cnv_pulse_time = st->turbo_mode ? AD4000_TQUIET1_NS
> +						     : AD4000_TCONV_NS;
> +	struct spi_transfer *xfers = st->xfers;
> +	int ret;
> +
> +	xfers[0].cs_change = 1;> +	xfers[0].cs_change_delay.value = cnv_pulse_time;
> +	xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +	xfers[1].rx_buf = &st->scan.data;
> +	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);

Do we really need to read 32-bits for 18-bit data? It seems like we could
get away with reading 3 bytes.

> +	xfers[1].delay.value = AD4000_TQUIET2_NS;
> +	xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +	spi_message_init_with_transfers(&st->msg, st->xfers, 2);
> +
> +	ret = spi_optimize_message(st->spi, &st->msg);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(&st->spi->dev, ad4000_unoptimize_msg,
> +					&st->msg);
> +}
> +
> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "4-wire" mode, selected when the adi,spi-mode device tree
> + * property is absent or empty. In this connection mode, the controller CS pin
> + * is connected to ADC SDI pin and a GPIO is connected to ADC CNV pin.
> + * The GPIO connected to ADC CNV pin is set outside of the SPI transfer.
> + */
> +static int ad4000_prepare_4wire_mode_message(struct ad4000_state *st,
> +					     const struct iio_chan_spec *chan)
> +{
> +	unsigned int cnv_to_sdi_time = st->turbo_mode ? AD4000_TQUIET1_NS
> +						      : AD4000_TCONV_NS;
> +	struct spi_transfer *xfers = st->xfers;
> +	int ret;
> +
> +	/*
> +	 * Dummy transfer to cause enough delay between CNV going high and SDI
> +	 * going low.
> +	 */
> +	xfers[0].cs_off = 1;
> +	xfers[0].delay.value = cnv_to_sdi_time;
> +	xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +	xfers[1].rx_buf = &st->scan.data;
> +	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> +
> +	spi_message_init_with_transfers(&st->msg, st->xfers, 2);
> +
> +	ret = spi_optimize_message(st->spi, &st->msg);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(&st->spi->dev, ad4000_unoptimize_msg,
> +					&st->msg);
> +}
> +
> +static int ad4000_convert_and_acquire(struct ad4000_state *st)
> +{
> +	int ret;
> +
> +	/*
> +	 * In 4-wire mode, the CNV line is held high for the entire conversion
> +	 * and acquisition process. In other modes, the CNV GPIO is optional
> +	 * and, if provided, replaces controller CS. If CNV GPIO is not defined
> +	 * gpiod_set_value_cansleep() has no effect.
> +	 */
> +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> +	ret = spi_sync(st->spi, &st->msg);
> +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +
> +	return ret;
> +}
> +
> +static int ad4000_single_conversion(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan, int *val)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +	u32 sample;
> +	int ret;
> +
> +	ret = ad4000_convert_and_acquire(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (chan->scan_type.storagebits > 16)
> +		sample = be32_to_cpu(st->scan.data.sample_buf32);
> +	else
> +		sample = be16_to_cpu(st->scan.data.sample_buf16);
> +
> +	switch (chan->scan_type.realbits) {
> +	case 16:
> +		break;
> +	case 18:
> +		sample = FIELD_GET(AD4000_18BIT_MSK, sample);
> +		break;
> +	case 20:
> +		sample = FIELD_GET(AD4000_20BIT_MSK, sample);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Could we use scan_type.shift here instead of the switch statement?

> +
> +	if (chan->scan_type.sign == 's')
> +		*val = sign_extend32(sample, chan->scan_type.realbits - 1);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int ad4000_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long info)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> +			return ad4000_single_conversion(indio_dev, chan, val);
> +		unreachable();
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->scale_tbl[st->span_comp][0];
> +		*val2 = st->scale_tbl[st->span_comp][1];
> +		return IIO_VAL_INT_PLUS_NANO;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = 0;
> +		if (st->span_comp)
> +			*val = mult_frac(st->vref_mv, 1, 10);
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4000_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long info)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = (int *)st->scale_tbl;
> +		*length = 2 * 2;
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				    struct iio_chan_spec const *chan, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +}
> +
> +static int ad4000_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val, int val2,
> +			    long mask)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +	unsigned int reg_val;
> +	bool span_comp_en;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +			ret = ad4000_read_reg(st, &reg_val);
> +			if (ret < 0)
> +				return ret;
> +
> +			span_comp_en = val2 == st->scale_tbl[1][1];
> +			reg_val &= ~AD4000_CFG_SPAN_COMP;
> +			reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en);
> +
> +			ret = ad4000_write_reg(st, reg_val);
> +			if (ret < 0)
> +				return ret;
> +
> +			st->span_comp = span_comp_en;
> +			return 0;
> +		}
> +		unreachable();
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static irqreturn_t ad4000_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4000_convert_and_acquire(st);
> +	if (ret < 0)
> +		goto err_out;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
> +					   iio_get_time_ns(indio_dev));

We specified iio_pollfunc_store_time, so we should use pf->timestamp here.

> +
> +err_out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info ad4000_3wire_info = {
> +	.read_raw = &ad4000_read_raw,
> +	.read_avail = &ad4000_read_avail,
> +	.write_raw = &ad4000_write_raw,
> +	.write_raw_get_fmt = &ad4000_write_raw_get_fmt,
> +
> +};
> +
> +static const struct iio_info ad4000_info = {
> +	.read_raw = &ad4000_read_raw,
> +};
> +
> +static int ad4000_config(struct ad4000_state *st)
> +{> +	unsigned int reg_val = AD4000_CONFIG_REG_DEFAULT;
> +
> +	if (device_property_present(&st->spi->dev, "adi,high-z-input"))
> +		reg_val |= FIELD_PREP(AD4000_CFG_HIGHZ, 1);> +
> +	return ad4000_write_reg(st, reg_val);
> +}
> +
> +static int ad4000_probe(struct spi_device *spi)
> +{
> +	const struct ad4000_chip_info *chip;
> +	struct iio_dev *indio_dev;
> +	struct ad4000_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = spi_get_device_match_data(spi);
> +	if (!chip)
> +		return -EINVAL;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	ret = devm_regulator_get_enable(&spi->dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "Failed to enable VDD supply\n");
> +
> +	ret = devm_regulator_get_enable(&spi->dev, "vio");
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "Failed to enable VIO supply\n");
> +
> +	st->vref_mv = devm_regulator_get_enable_read_voltage(&spi->dev, "ref");

	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "ref");

> +	if (ret < 0)
> +		return dev_err_probe(&spi->dev, st->vref_mv,

		return dev_err_probe(&spi->dev, ret,

> +				     "Failed to get ref regulator reference\n");
> +	st->vref_mv = st->vref_mv / 1000;

	st->vref_mv = ret / 1000;

> +
> +	st->cnv_gpio = devm_gpiod_get_optional(&spi->dev, "cnv", GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->cnv_gpio))
> +		return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio),
> +				     "Failed to get CNV GPIO");
> +
> +	ret = device_property_match_property_string(&spi->dev, "adi,spi-mode",
> +						    ad4000_spi_modes,
> +						    ARRAY_SIZE(ad4000_spi_modes));
> +	/* Default to 4-wire mode if adi,spi-mode property is not present */
> +	if (ret == -EINVAL)
> +		st->spi_mode = AD4000_SPI_MODE_DEFAULT;
> +	else if (ret < 0)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "getting adi,spi-mode property failed\n");
> +	else
> +		st->spi_mode = ret;

Could save a few lines and avoid checkpatch warning like this:

	if (ret < 0 && ret != -EINVAL)
		return dev_err_probe(&spi->dev, ret,
				     "getting adi,spi-mode property failed\n");

	st->spi_mode = ret == -EINVAL ? AD4000_SPI_MODE_DEFAULT : ret;

> +
> +	switch (st->spi_mode) {
> +	case AD4000_SPI_MODE_DEFAULT:
> +		indio_dev->info = &ad4000_info;
> +		indio_dev->channels = &chip->chan_spec;
> +		ret = ad4000_prepare_4wire_mode_message(st, indio_dev->channels);
> +		if (ret)
> +			return ret;
> +
> +		break;
> +	case AD4000_SPI_MODE_SINGLE:
> +		indio_dev->info = &ad4000_3wire_info;
> +		indio_dev->channels = &chip->three_w_chan_spec;
> +
> +		/*
> +		 * In "3-wire mode", the ADC SDI line must be kept high when
> +		 * data is not being clocked out of the controller.
> +		 * Request the SPI controller to make MOSI idle high.
> +		 */
> +		spi->mode = SPI_MODE_0 | SPI_MOSI_IDLE_HIGH;

Maybe better to make this 

	spi->mode |= SPI_MOSI_IDLE_HIGH;

and let the DT determine the other flags?

> +		ret = spi_setup(spi);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = ad4000_prepare_3wire_mode_message(st, indio_dev->channels);
> +		if (ret)
> +			return ret;
> +
> +		ret = ad4000_config(st);
> +		if (ret < 0)
> +			dev_warn(&st->spi->dev, "Failed to config device\n");
> +
> +		break;

Need a default:? Especially since chain mode is not supported.

> +	}
> +
> +	indio_dev->name = chip->dev_name;
> +	indio_dev->num_channels = 1;
> +
> +	/* Hardware gain only applies to ADAQ devices */
> +	st->gain_milli = 1000;
> +	if (device_property_present(&spi->dev, "adi,gain-milli")) {
> +		ret = device_property_read_u16(&spi->dev, "adi,gain-milli",
> +					       &st->gain_milli);
> +		if (ret)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Failed to read gain property\n");
> +	}
> +
> +	ad4000_fill_scale_tbl(st, indio_dev->channels);
> +
> +	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> +					      &iio_pollfunc_store_time,
> +					      &ad4000_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +

Re: [PATCH v4 5/6] dt-bindings: iio: adc: Add AD4000

[Marcelo Schmitt]

On 06/19, David Lechner wrote:
> On 6/18/24 6:12 PM, Marcelo Schmitt wrote:
> > Add device tree documentation for AD4000 series of ADC devices.
> > 
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> 
> Datasheets URLs are listed in the patch, so probably don't need them
> in the commit message too.

okay, removed them for v5.

> 
...
> 
> There are data sheets listed for adaq400x but no compatibles.

Ah, that's correct. I missed
      - const: adi,adaq4001
      - const: adi,adaq4003

thanks
> 
...
> > +
> > +  adi,gain-milli:
> > +    description: |
> > +      The hardware gain applied to the ADC input (in milli units).
> > +      The gain provided by the ADC input scaler is defined by the hardware
> > +      connections between chip pins OUT+, R1K-, R1K1-, R1K+, R1K1+, and OUT-.
> > +      If not present, default to 1000 (no actual gain applied).
> > +    $ref: /schemas/types.yaml#/definitions/uint16
> 
> Any particular reason why this needs to be uint16 instead of the more
> common uint32?

The values fit into 16 bits and Nuno asked to make it a 16-bit property.

> 
> > +    enum: [454, 909, 1000, 1900]
> > +    default: 1000
> > +
...
> > +  # The configuration register can only be accessed in '3-wire' mode
> > +  - if:
> > +      not:
> > +        properties:
> > +          adi,spi-mode:
> > +            contains:
> > +              enum:
> > +                - single
> 
> adi,spi-mode is not an array and we are only checking for one value,
> so this could be simplified to:
> 
>   properties:
>     adi,spi-mode:
>       const: single
> 
ok, changed to do the check that way.

...
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        /* Example for a AD devices */
> 
> Comments are a bit redundant since it says "examples:" above and
> the type of the chip in the compatible string.
> 
I like the comments. I think it makes clear we have AD and ADAQ devices but ok,
removing them for v5.

> > +        adc@0 {
> > +            compatible = "adi,ad4020";
> > +            reg = <0>;
> > +            spi-max-frequency = <71000000>;
> > +            vdd-supply = <&supply_1_8V>;
> > +            vio-supply = <&supply_1_8V>;
> > +            ref-supply = <&supply_5V>;
> > +            cnv-gpios = <&gpio0 88 GPIO_ACTIVE_HIGH>;
> > +        };
> > +    };
> > +  - |
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        /* Example for a ADAQ devices */
> > +        adc@0 {
> > +            compatible = "adi,adaq4003";
> > +            reg = <0>;
> > +            spi-max-frequency = <80000000>;
> > +            vdd-supply = <&supply_1_8V>;
> > +            vio-supply = <&supply_1_8V>;
> > +            ref-supply = <&supply_5V>;
> > +            adi,high-z-input;
> > +            adi,gain-milli = /bits/ 16 <454>;
> > +            adi,spi-mode = "single";
> > +        };
> > +    };

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

[David Lechner]

On 6/18/24 6:10 PM, Marcelo Schmitt wrote:

...

> @@ -3928,7 +3934,8 @@ int spi_setup(struct spi_device *spi)
>  	 * so it is ignored here.
>  	 */
>  	bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD |
> -				 SPI_NO_TX | SPI_NO_RX);
> +				 SPI_NO_TX | SPI_NO_RX | SPI_MOSI_IDLE_LOW |
> +				 SPI_MOSI_IDLE_HIGH);

This looks wrong to me. Adding flags here causes them to be ignored
rather than to be checked.

I also did a runtime check with a random driver and a SPI controller
that does not have the flag.

	spi->mode |= SPI_MOSI_IDLE_LOW;
	ret = spi_setup(spi);
	if (ret)
		return ret;

It incorrectly passes when used with this change but correctly fails
without this change.

>  	ugly_bits = bad_bits &
>  		    (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
>  		     SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL);

Re: [PATCH v4 4/6] spi: spi-axi-spi-engine: Add support for MOSI idle configuration

[Marcelo Schmitt]

On 06/19, David Lechner wrote:
> On 6/18/24 6:11 PM, Marcelo Schmitt wrote:
> > Implement MOSI idle low and MOSI idle high to better support peripherals
> > that request specific MOSI behavior.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > ---
> >  drivers/spi/spi-axi-spi-engine.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
> > index 0aa31d745734..787e22ae80c0 100644
> > --- a/drivers/spi/spi-axi-spi-engine.c
> > +++ b/drivers/spi/spi-axi-spi-engine.c
> > @@ -41,6 +41,7 @@
> >  #define SPI_ENGINE_CONFIG_CPHA			BIT(0)
> >  #define SPI_ENGINE_CONFIG_CPOL			BIT(1)
> >  #define SPI_ENGINE_CONFIG_3WIRE			BIT(2)
> > +#define SPI_ENGINE_CONFIG_SDO_IDLE		BIT(3)
> 
> Calling this SPI_ENGINE_CONFIG_SDO_IDLE_HIGH would make it more
> clear what happens when the bit is enabled.

Yeah, agreed. Changing to SPI_ENGINE_CONFIG_SDO_IDLE_HIGH.

> 
> >  
> >  #define SPI_ENGINE_INST_TRANSFER		0x0
> >  #define SPI_ENGINE_INST_ASSERT			0x1
> > @@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct spi_device *spi)
> >  		config |= SPI_ENGINE_CONFIG_CPHA;
> >  	if (spi->mode & SPI_3WIRE)
> >  		config |= SPI_ENGINE_CONFIG_3WIRE;
> > +	if (spi->mode & SPI_MOSI_IDLE_HIGH)
> > +		config |= SPI_ENGINE_CONFIG_SDO_IDLE;
> > +	if (spi->mode & SPI_MOSI_IDLE_LOW)
> > +		config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;
> >  
> >  	return config;
> >  }
> > @@ -646,6 +651,9 @@ static int spi_engine_probe(struct platform_device *pdev)
> >  
> >  	host->dev.of_node = pdev->dev.of_node;
> >  	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
> > +	if (ADI_AXI_PCORE_VER_MAJOR(version) >= 1 &&
> 
> Currently, the major version is required to be 1, so this check is not
> strictly needed.
> 
This is expecting the MOSI idle feature to be available on all versions from 1.3 on.
Will SPI-Engine always be major version 1?

> > +	    ADI_AXI_PCORE_VER_MINOR(version) >= 3)
> > +		host->mode_bits |=  SPI_MOSI_IDLE_LOW | SPI_MOSI_IDLE_HIGH;
> 
> 
> 
> >  	host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> >  	host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2;
> >  	host->transfer_one_message = spi_engine_transfer_one_message;
> 

Re: [PATCH v4 4/6] spi: spi-axi-spi-engine: Add support for MOSI idle configuration

[David Lechner]

On 6/19/24 12:27 PM, Marcelo Schmitt wrote:
> On 06/19, David Lechner wrote:
>> On 6/18/24 6:11 PM, Marcelo Schmitt wrote:



>>> @@ -646,6 +651,9 @@ static int spi_engine_probe(struct platform_device *pdev)
>>>  
>>>  	host->dev.of_node = pdev->dev.of_node;
>>>  	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
>>> +	if (ADI_AXI_PCORE_VER_MAJOR(version) >= 1 &&
>>
>> Currently, the major version is required to be 1, so this check is not
>> strictly needed.
>>
> This is expecting the MOSI idle feature to be available on all versions from 1.3 on.
> Will SPI-Engine always be major version 1?

<yoda voice>Difficult to see, the future is.</yoda voice>

It's fine if you want to leave it the way it is.

> 
>>> +	    ADI_AXI_PCORE_VER_MINOR(version) >= 3)
>>> +		host->mode_bits |=  SPI_MOSI_IDLE_LOW | SPI_MOSI_IDLE_HIGH;
>>

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

[Marcelo Schmitt]

On 06/19, David Lechner wrote:
> 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".

I'm not sure. IMHO, it looks less ambiguous to say a CS is active.
I think the most common for CS lines is to have a CS that is active low (i.e.
the line is at a low voltage level when the controller is selecting the device).
To me, "assert" sounds closer to the idea o setting something (like a bit to 1),
which is the opposite of active low CS.
Though, no strong opinion about it.
I go with what the maintainers prefer.

> 
> > +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.

It may be that we only need MOSI high just before bringing CS low. Though,
I don't see that info in the datasheets. How much time would MOSI be required
to be high prior to bringing CS low? The timing diagrams for register access and
ADC sampling in "3-wire" mode all start and end with MOSI at logical 1 (high).
I think reg access work if MOSI is brought low after CS gets low, but sample
read definitely don't work.

From the info available in datasheets, it looks like MOSI is indeed expected 
to be high indefinitely amount of time. Except when the controller is clocking
out data to the peripheral.

Even if find out the amount of time MOSI would be required high prior to CS low,
then we would need some sort of MOSI high/low state set with a delay prior to
active CS. That might be enough to support the AD4000 series of devices but,
would it be worth the added complexity?

> 
> > +
> > +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.
ok, updated those for v5.

> 
> > +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.
> 
Good point.
They are currently not being used.
Comparing with what we have for SPI_CONTROLLER_MULTI_CS, I'm thinking it may be
handy to have dt properties to indicate controller MOSI idle capabilities.
Does that sound reasonable?

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

Re: [PATCH v4 5/6] dt-bindings: iio: adc: Add AD4000

[David Lechner]

On 6/18/24 6:12 PM, Marcelo Schmitt wrote:
> Add device tree documentation for AD4000 series of ADC devices.
> 
...

> +  adi,spi-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ single, chain ]
> +    description: |
> +      This property indicates the SPI wiring configuration.
> +
> +      When this property is omitted, it is assumed that the device is using what
> +      the datasheet calls "4-wire mode". This is the conventional SPI mode used
> +      when there are multiple devices on the same bus. In this mode, the CNV
> +      line is used to initiate the conversion and the SDI line is connected to
> +      CS on the SPI controller.
> +
> +      When this property is present, it indicates that the device is using one
> +      of the following alternative wiring configurations:
> +
> +      * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
> +        definition of 3-wire mode is NOT at all related to the standard
> +        spi-3wire property!) This mode is often used when the ADC is the only
> +        device on the bus. In this mode, SDI is connected to MOSI or to VIO, and
> +        the CNV line can be connected to the CS line of the SPI controller or to
> +        a GPIO, in which case the CS line of the controller is unused.
> +      * chain: The datasheet calls this "chain mode". This mode is used to save
> +        on wiring when multiple ADCs are used. In this mode, the SDI line of
> +        one chip is tied to the SDO of the next chip in the chain and the SDI of
> +        the last chip in the chain is tied to GND. Only the first chip in the
> +        chain is connected to the SPI bus. The CNV line of all chips are tied
> +        together. The CS line of the SPI controller can be used as the CNV line
> +        only if it is active high.
> +

After reviewing the driver and going back and looking at the diagrams in [1] again,
I think we are missing a wiring mode here. What the driver is calling "single/3-wire"
mode is actually using 4 wires and is the wiring mode that I suggested should be
the default since that is the only wiring configuration where we can read/write
registers.

[1]: https://lore.kernel.org/linux-iio/87058695-a1a6-4e68-87c5-accdb8451bf4@baylibre.com/

So to recap, this is what I suggest we should do:

default unnamed mode:

  * Wiring:
      ADC    HOST
      ---    ----
      CNV    CS (or GPIO)
      SDI    SDO
      SDO    SDI
      SCLK   SCLK
  * Requires SPI controller with SPI_MOSI_IDLE_HIGH/LOW capability
  * Can read/write registers
  * Can do "3-wire mode"-style reads (turbo and not turbo)
    * Requires SPI_MOSI_IDLE_HIGH
  * Can do "4-wire mode"-style reads (turbo and not turbo)
    * Requires SPI_MOSI_IDLE_HIGH, SPI_CS_HIGH (or no CS and cnv-gpios)
  * Can do "daisy-chain mode"-style reads
    * Requires SPI_MOSI_IDLE_LOW, SPI_CS_HIGH (or no CS and cnv-gpios)
  * #daisy-chained-devices is optional

"single" mode:

  * Wiring: same as default except ADC SDI is hard-wired to logic high.
  * Cannot read/write registers.
  * Doesn't require special SPI controller (no SPI_MOSI_IDLE_HIGH/LOW)
  * Can do "3-wire mode"-style reads (turbo and not turbo)
  * #daisy-chained-devices is forbidden
  * Use case: save one wire, works with any SPI controller

"multi" mode:

  * Wiring:
      ADC    HOST
      ---    ----
      CNV    GPIO
      SDI    CSn
      SDO    SDI
      SCLK   SCLK

  * Cannot read/write registers.
  * Doesn't require special SPI controller (no SPI_MOSI_IDLE_HIGH/LOW)
  * Can do "4-wire mode"-style reads (not turbo)
  * #daisy-chained-devices is forbidden
  * Use case: multiple ADCs can share one CNV trigger

"chain" mode:

  * Wiring: same as default except ADC SDI is hard-wired to logic low.
  * Cannot read/write registers.
  * Doesn't require special SPI controller (no SPI_MOSI_IDLE_HIGH/LOW)
  * Can do "daisy-chain mode"-style reads (requires CS high or cnv-gpios)
  * #daisy-chained-devices is required
  * Use case: save one wire, works with any SPI controller

---

To put it more simply in the bindings though, really this property
is just describing how the SDI pin is wired. (CNV pin wiring can be
inferred from this property and presence or absence of cnv-gpios
property.) So maybe better would be:

  adi,sdi-pin:
    $ref: /schemas/types.yaml#/definitions/string
    enum: [ high, low, cs ]
    description:
       Describes how the ADC SDI pin is wired. When this property is
       omitted, ADC SDI is connected to host SDO. "high" indicates
       that the ADC SDI pin is hard-wired to logic high (VIO).
       "low" indicates that it is hard-wired low (GND). "cs" indicates
       that the ADC SDI pin is connected to the host CS line.

And put a note about the specialized SPI controller requirements in
the main description.

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

[David Lechner]

On 6/19/24 1:58 PM, Marcelo Schmitt wrote:
> On 06/19, David Lechner wrote:
>> On 6/18/24 6:10 PM, Marcelo Schmitt wrote:
>>
>>

...

>>
>>> +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.
> 
> It may be that we only need MOSI high just before bringing CS low. Though,
> I don't see that info in the datasheets. How much time would MOSI be required
> to be high prior to bringing CS low? The timing diagrams for register access and
> ADC sampling in "3-wire" mode all start and end with MOSI at logical 1 (high).
> I think reg access work if MOSI is brought low after CS gets low, but sample
> read definitely don't work.
> 
> From the info available in datasheets, it looks like MOSI is indeed expected 
> to be high indefinitely amount of time. Except when the controller is clocking
> out data to the peripheral.
> 
> Even if find out the amount of time MOSI would be required high prior to CS low,
> then we would need some sort of MOSI high/low state set with a delay prior to
> active CS. That might be enough to support the AD4000 series of devices but,
> would it be worth the added complexity?
> 

It needs to happen at the same time as setting CPOL for the SCLK line for the
device that is about to have the CS asserted. So I don't think we are breaking
new ground here. Typically, in most datasheets I've seen they tend to say
something like 2 ns before the CS change. So in most cases, I don't think
anyone bothers adding a delay. But if a longer delay was really needed for
a specific peripheral, we could add a SPI xfer with no read/write that has
cs_off=1 and a delay to get the correct state of both MOSI and SCLK a longer
time before the CS change.

>>
>>> 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.
>>
> Good point.
> They are currently not being used.
> Comparing with what we have for SPI_CONTROLLER_MULTI_CS, I'm thinking it may be
> handy to have dt properties to indicate controller MOSI idle capabilities.
> Does that sound reasonable?

I don't see any properties in spi-controller.yaml related to
SPI_CONTROLLER_MULTI_CS. So I don't see how a property for
the idle capabilities would be useful there.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]

On Wed, Jun 19, 2024 at 03:58:00PM -0300, Marcelo Schmitt wrote:
> On 06/19, David Lechner wrote:
> > On 6/18/24 6:10 PM, Marcelo Schmitt wrote:

> > > +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".

> I'm not sure. IMHO, it looks less ambiguous to say a CS is active.
> I think the most common for CS lines is to have a CS that is active low (i.e.
> the line is at a low voltage level when the controller is selecting the device).
> To me, "assert" sounds closer to the idea o setting something (like a bit to 1),
> which is the opposite of active low CS.
> Though, no strong opinion about it.
> I go with what the maintainers prefer.

I think they're synonyms but asserted is the more common term for chip
selects.


> > > +#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.

> Good point.
> They are currently not being used.
> Comparing with what we have for SPI_CONTROLLER_MULTI_CS, I'm thinking it may be
> handy to have dt properties to indicate controller MOSI idle capabilities.
> Does that sound reasonable?

We shouldn't need DT properties, we should just know if the controller
supports this based on knowing what controller is, and I'd not expect it
to depend on board wiring.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marcelo Schmitt]

On 06/19, David Lechner wrote:
> On 6/18/24 6:10 PM, Marcelo Schmitt wrote:
> 
> ...
> 
> > @@ -3928,7 +3934,8 @@ int spi_setup(struct spi_device *spi)
> >  	 * so it is ignored here.
> >  	 */
> >  	bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD |
> > -				 SPI_NO_TX | SPI_NO_RX);
> > +				 SPI_NO_TX | SPI_NO_RX | SPI_MOSI_IDLE_LOW |
> > +				 SPI_MOSI_IDLE_HIGH);
> 
> This looks wrong to me. Adding flags here causes them to be ignored
> rather than to be checked.
> 
> I also did a runtime check with a random driver and a SPI controller
> that does not have the flag.
> 
> 	spi->mode |= SPI_MOSI_IDLE_LOW;
> 	ret = spi_setup(spi);
> 	if (ret)
> 		return ret;
> 
> It incorrectly passes when used with this change but correctly fails
> without this change.

That's right, adding flags to bad_bits makes spi_setup() ignore those mode bits
instead of failing when they don't match.
Changed bad_bits assignment back to what it was in v3 (i.e. no change to bad_bits).
	bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD |
				 SPI_NO_TX | SPI_NO_RX);

> 
> >  	ugly_bits = bad_bits &
> >  		    (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
> >  		     SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL);
> 

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

[Marcelo Schmitt]

On 06/19, David Lechner wrote:
> On 6/19/24 1:58 PM, Marcelo Schmitt wrote:
> > On 06/19, David Lechner wrote:
> >> On 6/18/24 6:10 PM, Marcelo Schmitt wrote:
> >>
> >>
> 
> ...
> 
> >>
> >>> +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.
> > 
> > It may be that we only need MOSI high just before bringing CS low. Though,
> > I don't see that info in the datasheets. How much time would MOSI be required
> > to be high prior to bringing CS low? The timing diagrams for register access and
> > ADC sampling in "3-wire" mode all start and end with MOSI at logical 1 (high).
> > I think reg access work if MOSI is brought low after CS gets low, but sample
> > read definitely don't work.
> > 
> > From the info available in datasheets, it looks like MOSI is indeed expected 
> > to be high indefinitely amount of time. Except when the controller is clocking
> > out data to the peripheral.
> > 
> > Even if find out the amount of time MOSI would be required high prior to CS low,
> > then we would need some sort of MOSI high/low state set with a delay prior to
> > active CS. That might be enough to support the AD4000 series of devices but,
> > would it be worth the added complexity?
> > 
> 
> It needs to happen at the same time as setting CPOL for the SCLK line for the
> device that is about to have the CS asserted. So I don't think we are breaking
> new ground here. Typically, in most datasheets I've seen they tend to say
> something like 2 ns before the CS change. So in most cases, I don't think
which datasheets? Are any of those for devices supported by the ad4000 driver?

> anyone bothers adding a delay. But if a longer delay was really needed for
> a specific peripheral, we could add a SPI xfer with no read/write that has
> cs_off=1 and a delay to get the correct state of both MOSI and SCLK a longer
> time before the CS change.

I don't know if that would actually work. I have not tested doing something like that.
This also implies the controller will be able to start the next transfer right
after the first preparatory transfer ends and it will meet that inter-transfer
timing requirement (which I still didn't find documented anywhere).
I'm not convinced that would be the best way to support those devices.

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

[Marcelo Schmitt]

On 06/19, Mark Brown wrote:
> On Wed, Jun 19, 2024 at 03:58:00PM -0300, Marcelo Schmitt wrote:
> > On 06/19, David Lechner wrote:
> > > On 6/18/24 6:10 PM, Marcelo Schmitt wrote:
> 
> > > > +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".

ack, replaced "active" by "asserted" when describing CS state for v5.

> 
> > I'm not sure. IMHO, it looks less ambiguous to say a CS is active.
> > I think the most common for CS lines is to have a CS that is active low (i.e.
> > the line is at a low voltage level when the controller is selecting the device).
> > To me, "assert" sounds closer to the idea o setting something (like a bit to 1),
> > which is the opposite of active low CS.
> > Though, no strong opinion about it.
> > I go with what the maintainers prefer.
> 
> I think they're synonyms but asserted is the more common term for chip
> selects.
> 
> 
> > > > +#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.
> 
> > Good point.
> > They are currently not being used.
> > Comparing with what we have for SPI_CONTROLLER_MULTI_CS, I'm thinking it may be
> > handy to have dt properties to indicate controller MOSI idle capabilities.
> > Does that sound reasonable?
> 
> We shouldn't need DT properties, we should just know if the controller
> supports this based on knowing what controller is, and I'd not expect it
> to depend on board wiring.

Okay, though, I fail to see the need for 
#define SPI_CONTROLLER_MOSI_IDLE_LOW    BIT(8)  /* Can idle MOSI low */
#define SPI_CONTROLLER_MOSI_IDLE_HIGH   BIT(9)  /* Can idle MOSI high */

It looks like SPI_CONTROLLER bits are used to tweak controller operation in
various ways.
Right now, I'm not aware of any additional tweak needed to support the MOSI idle
feature. I have tested that on Raspberry Pi with bitbang/gpio controller and on
CoraZ7 with spi-engine and it did work fine in those setups.
Anyway, I'm prone to implement any additional changes to make this set better.

Thanks,
Marcelo

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

[David Lechner]

On 6/20/24 10:12 AM, Marcelo Schmitt wrote:
> On 06/19, David Lechner wrote:
>> On 6/19/24 1:58 PM, Marcelo Schmitt wrote:
>>> On 06/19, David Lechner wrote:
>>>> On 6/18/24 6:10 PM, Marcelo Schmitt wrote:
>>>>
>>>>
>>
>> ...
>>
>>>>
>>>>> +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.
>>>
>>> It may be that we only need MOSI high just before bringing CS low. Though,
>>> I don't see that info in the datasheets. How much time would MOSI be required
>>> to be high prior to bringing CS low? The timing diagrams for register access and
>>> ADC sampling in "3-wire" mode all start and end with MOSI at logical 1 (high).
>>> I think reg access work if MOSI is brought low after CS gets low, but sample
>>> read definitely don't work.
>>>
>>> From the info available in datasheets, it looks like MOSI is indeed expected 
>>> to be high indefinitely amount of time. Except when the controller is clocking
>>> out data to the peripheral.
>>>
>>> Even if find out the amount of time MOSI would be required high prior to CS low,
>>> then we would need some sort of MOSI high/low state set with a delay prior to
>>> active CS. That might be enough to support the AD4000 series of devices but,
>>> would it be worth the added complexity?
>>>
>>
>> It needs to happen at the same time as setting CPOL for the SCLK line for the
>> device that is about to have the CS asserted. So I don't think we are breaking
>> new ground here. Typically, in most datasheets I've seen they tend to say
>> something like 2 ns before the CS change. So in most cases, I don't think
> which datasheets? Are any of those for devices supported by the ad4000 driver?

In the AD4000 datasheet, Figure 59, it shows the time needed for SDI setup
before CS assertion, labeled as t_SSDICNV. Table 2 gives this value to be
2 ns.

So unless a SPI controller has a functional clock of > 500 MHz or somehow
sets the SDI state and the CS assertion in the same register write this
minimum delay will always be met. I highly suspect noting like this has
happened before so no one ever needed to worry about the timing and it
just works (for the similar case of CPOL).

> 
>> anyone bothers adding a delay. But if a longer delay was really needed for
>> a specific peripheral, we could add a SPI xfer with no read/write that has
>> cs_off=1 and a delay to get the correct state of both MOSI and SCLK a longer
>> time before the CS change.
> 
> I don't know if that would actually work. I have not tested doing something like that.
> This also implies the controller will be able to start the next transfer right
> after the first preparatory transfer ends and it will meet that inter-transfer
> timing requirement (which I still didn't find documented anywhere).
> I'm not convinced that would be the best way to support those devices.

I did something like this in the ad7944 driver where we needed an up to
500ns delay before asserting CS. On SPI controllers without a hardware
sleep or FIFO, the delay will of course be much longer. But the delay
is just a minimum delay, so longer doesn't hurt. It just affects the
max sample rate that can be reliably achieved.

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

[Marcelo Schmitt]

On 06/20, David Lechner wrote:
> On 6/20/24 10:12 AM, Marcelo Schmitt wrote:
> > On 06/19, David Lechner wrote:
> >> On 6/19/24 1:58 PM, Marcelo Schmitt wrote:
> >>> On 06/19, David Lechner wrote:
> >>>> On 6/18/24 6:10 PM, Marcelo Schmitt wrote:
> >>>>
> >>>>
> >>
> >> ...
> >>
> >>>>
> >>>>> +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.
> >>>
> >>> It may be that we only need MOSI high just before bringing CS low. Though,
> >>> I don't see that info in the datasheets. How much time would MOSI be required
> >>> to be high prior to bringing CS low? The timing diagrams for register access and
> >>> ADC sampling in "3-wire" mode all start and end with MOSI at logical 1 (high).
> >>> I think reg access work if MOSI is brought low after CS gets low, but sample
> >>> read definitely don't work.
> >>>
> >>> From the info available in datasheets, it looks like MOSI is indeed expected 
> >>> to be high indefinitely amount of time. Except when the controller is clocking
> >>> out data to the peripheral.
> >>>
> >>> Even if find out the amount of time MOSI would be required high prior to CS low,
> >>> then we would need some sort of MOSI high/low state set with a delay prior to
> >>> active CS. That might be enough to support the AD4000 series of devices but,
> >>> would it be worth the added complexity?
> >>>
> >>
> >> It needs to happen at the same time as setting CPOL for the SCLK line for the
> >> device that is about to have the CS asserted. So I don't think we are breaking
> >> new ground here. Typically, in most datasheets I've seen they tend to say
> >> something like 2 ns before the CS change. So in most cases, I don't think
> > which datasheets? Are any of those for devices supported by the ad4000 driver?
> 
> In the AD4000 datasheet, Figure 59, it shows the time needed for SDI setup
> before CS assertion, labeled as t_SSDICNV. Table 2 gives this value to be
> 2 ns.

That delay is for "4-wire" mode and it specifies the delay before bringing CS
high. In "3-wire" mode, we are bringing CS low to start transfers so it doesn't
look like t_SSDICNV applies to the "3-wire" mode setup.

> 
> So unless a SPI controller has a functional clock of > 500 MHz or somehow
> sets the SDI state and the CS assertion in the same register write this
> minimum delay will always be met. I highly suspect noting like this has
> happened before so no one ever needed to worry about the timing and it
> just works (for the similar case of CPOL).
> 
> > 
> >> anyone bothers adding a delay. But if a longer delay was really needed for
> >> a specific peripheral, we could add a SPI xfer with no read/write that has
> >> cs_off=1 and a delay to get the correct state of both MOSI and SCLK a longer
> >> time before the CS change.
> > 
> > I don't know if that would actually work. I have not tested doing something like that.
> > This also implies the controller will be able to start the next transfer right
> > after the first preparatory transfer ends and it will meet that inter-transfer
> > timing requirement (which I still didn't find documented anywhere).
> > I'm not convinced that would be the best way to support those devices.
> 
> I did something like this in the ad7944 driver where we needed an up to
> 500ns delay before asserting CS. On SPI controllers without a hardware
> sleep or FIFO, the delay will of course be much longer. But the delay
> is just a minimum delay, so longer doesn't hurt. It just affects the
> max sample rate that can be reliably achieved.
> 
In ad7944_3wire_cs_mode_init_msg(), xfers[1] is prepared with spi_transfer.delay
which is the "delay to be introduced after this transfer before
(optionally) changing the chipselect status, then starting the next transfer or
completing this @spi_message." That should work for ad7944 because
it has ADC SDI physically connected to VIO in that setup.
For ad4000, we would want to set MOSI high (by writing 1s) such that MOSI
is high when CS is brought high (if I'm getting what you are suggesting).
But spi_transfer.delay is introduced after the transfer and before changing CS
so I think MOSI may return to low if the controller idles MOSI low.
I can't see how this would work for ad4000.
Other delays we have for spi_transfer (cs_change_delay, word_delay) don't seem
to help for this particular case either.

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

[David Lechner]

On 6/20/24 1:21 PM, Marcelo Schmitt wrote:
> On 06/20, David Lechner wrote:
>> On 6/20/24 10:12 AM, Marcelo Schmitt wrote:
>>> On 06/19, David Lechner wrote:
>>>> On 6/19/24 1:58 PM, Marcelo Schmitt wrote:
>>>>> On 06/19, David Lechner wrote:
>>>>>> On 6/18/24 6:10 PM, Marcelo Schmitt wrote:
>>>>>>
>>>>>>
>>>>
>>>> ...
>>>>


>>> I don't know if that would actually work. I have not tested doing something like that.
>>> This also implies the controller will be able to start the next transfer right
>>> after the first preparatory transfer ends and it will meet that inter-transfer
>>> timing requirement (which I still didn't find documented anywhere).
>>> I'm not convinced that would be the best way to support those devices.
>>
>> I did something like this in the ad7944 driver where we needed an up to
>> 500ns delay before asserting CS. On SPI controllers without a hardware
>> sleep or FIFO, the delay will of course be much longer. But the delay
>> is just a minimum delay, so longer doesn't hurt. It just affects the
>> max sample rate that can be reliably achieved.
>>
> In ad7944_3wire_cs_mode_init_msg(), xfers[1] is prepared with spi_transfer.delay
> which is the "delay to be introduced after this transfer before
> (optionally) changing the chipselect status, then starting the next transfer or
> completing this @spi_message." That should work for ad7944 because
> it has ADC SDI physically connected to VIO in that setup.
> For ad4000, we would want to set MOSI high (by writing 1s) such that MOSI
> is high when CS is brought high (if I'm getting what you are suggesting).
> But spi_transfer.delay is introduced after the transfer and before changing CS
> so I think MOSI may return to low if the controller idles MOSI low.
> I can't see how this would work for ad4000.
> Other delays we have for spi_transfer (cs_change_delay, word_delay) don't seem
> to help for this particular case either.

I was actually referring to ad7944_4wire_mode_init_msg().

In the case of ad4000, the SPI controller will be required to support the
new SPI_MOSI_IDLE_HIGH flag. So at the beginning of the message, before any
of the individual xfers, the controller driver will configure SCLK base on
CPOL and MOSI based on SPI_MOSI_IDLE_HIGH. Then it will do whatever the
xfers say.

For most SPI controllers in Linux, this SCLK/MOSI config will happen in
ctlr->prepare_message() which happens before xfers are processed in
ctlr->transfer_one_message(). In the AXI SPI Engine, the SCLK/MOSI config
is in a separate instruction that happens before anything else in the
message.

So if the first xfer is just a delay with no read/write, the delay will
always happen after SCLK and MOSI are configured. We don't have to write
1s because SPI_MOSI_IDLE_HIGH already does the right thing.

Re: [PATCH v4 6/6] iio: adc: Add support for AD4000

[Jonathan Cameron]

On Tue, 18 Jun 2024 20:12:37 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> Add support for AD4000 series of low noise, low power, high speed,
> successive aproximation register (SAR) ADCs.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
Hi Marcelo,

A few comments inline.

Thanks,

Jonathan

> ---
>  MAINTAINERS              |   1 +
>  drivers/iio/adc/Kconfig  |  12 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad4000.c | 715 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 729 insertions(+)
>  create mode 100644 drivers/iio/adc/ad4000.c


> diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
> new file mode 100644
> index 000000000000..310f81a2a1d9
> --- /dev/null
> +++ b/drivers/iio/adc/ad4000.c

> +
> +/* AD4000 Configuration Register programmable bits */
> +#define AD4000_CFG_STATUS		BIT(4) /* Status bits output */
> +#define AD4000_CFG_SPAN_COMP		BIT(3) /* Input span compression  */
> +#define AD4000_CFG_HIGHZ		BIT(2) /* High impedance mode  */
> +#define AD4000_CFG_TURBO		BIT(1) /* Turbo mode */
> +
> +#define AD4000_TQUIET1_NS		190
> +#define AD4000_TQUIET2_NS		60
> +#define AD4000_TCONV_NS			320
> +
> +#define AD4000_18BIT_MSK	GENMASK(31, 14)
> +#define AD4000_20BIT_MSK	GENMASK(31, 12)

See below. These masks made me wonder what you were doing with them given everything
they represent is provided by other paths.

> +
> +#define AD4000_DIFF_CHANNEL(_sign, _real_bits, _3wire)				\
> +{										\
> +	.type = IIO_VOLTAGE,							\
> +	.indexed = 1,								\
> +	.differential = 1,							\
> +	.channel = 0,								\
> +	.channel2 = 1,								\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
> +			      BIT(IIO_CHAN_INFO_SCALE),				\
> +	.info_mask_separate_available = _3wire ? BIT(IIO_CHAN_INFO_SCALE) : 0,	\
> +	.scan_type = {								\
> +		.sign = _sign,							\
> +		.realbits = _real_bits,						\
> +		.storagebits = _real_bits > 16 ? 32 : 16,			\
> +		.shift = _real_bits > 16 ? 32 - _real_bits : 0,			\
> +		.endianness = IIO_BE,						\
> +	},									\
> +}
> +
> +#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _3wire)			\
> +{										\
> +	.type = IIO_VOLTAGE,							\
> +	.indexed = 1,								\
> +	.channel = 0,								\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
> +			      BIT(IIO_CHAN_INFO_SCALE) |			\
> +			      BIT(IIO_CHAN_INFO_OFFSET),			\
> +	.info_mask_separate_available = _3wire ? BIT(IIO_CHAN_INFO_SCALE) : 0,	\
> +	.scan_type = {								\
> +		.sign = _sign,							\
> +		.realbits = _real_bits,						\
> +		.storagebits = _real_bits > 16 ? 32 : 16,			\
> +		.shift = _real_bits > 16 ? 32 - _real_bits : 0,			\

I wonder if it's worth another level of macro so that you can compute storage bits
in one place.  The dance for shift then just becomes _storage_bits - _read_bits.
e.g.

#define __AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _storage_bits, _3wire)			\
{										\
	.type = IIO_VOLTAGE,							\
	.indexed = 1,								\
	.channel = 0,								\
	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
			      BIT(IIO_CHAN_INFO_SCALE) |			\
			      BIT(IIO_CHAN_INFO_OFFSET),			\
	.info_mask_separate_available = _3wire ? BIT(IIO_CHAN_INFO_SCALE) : 0,	\
	.scan_type = {								\
		.sign = _sign,							\
		.realbits = _real_bits,						\
		.storagebits = _storage_bits,					\
		.shift = _storage_bits - read_bits,				\
		.endianness = IIO_BE,						\
	},									\
}
#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _3wire) \
	__AD4000_PSEUDO_DIFF_CHANNEL((_sign), (_read_bits), (_real_bits) > 16 ? 32 : 16, (_3wire))
 
> +		.endianness = IIO_BE,						\
> +	},									\
> +}

> +struct ad4000_chip_info {
> +	const char *dev_name;
> +	struct iio_chan_spec chan_spec;
> +	struct iio_chan_spec three_w_chan_spec;
As mentioned below add

> +};

> +
> +struct ad4000_state {
> +	struct spi_device *spi;
> +	struct gpio_desc *cnv_gpio;
> +	struct spi_transfer xfers[2];
> +	struct spi_message msg;
> +	int vref_mv;
> +	enum ad4000_spi_mode spi_mode;
> +	bool span_comp;
> +	bool turbo_mode;
> +	u16 gain_milli;
> +	int scale_tbl[2][2];
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 */

Wrapped a bit sort. Go up to 80 chars.

> +	struct {
> +		union {
> +			__be16 sample_buf16;
> +			__be32 sample_buf32;
> +		} data;
> +		s64 timestamp __aligned(8);
> +	} scan __aligned(IIO_DMA_MINALIGN);
> +	__be16 tx_buf;
> +	__be16 rx_buf;
As below, I think these are u8 ...

> +};


> +
> +static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
> +{
> +	st->tx_buf = cpu_to_be16(AD4000_WRITE_COMMAND << BITS_PER_BYTE | val);

Use u8 tx_buf[2] and fill the two bytes separately given one is the command
and the other the data.

> +	return spi_write(st->spi, &st->tx_buf, sizeof(st->tx_buf));
> +}
> +
> +static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
> +{
> +	struct spi_transfer t[] = {
> +		{
> +			.tx_buf = &st->tx_buf,
> +			.rx_buf = &st->rx_buf,
> +			.len = 2,
> +		},
> +	};
> +	int ret;
> +
> +	st->tx_buf = cpu_to_be16(AD4000_READ_COMMAND << BITS_PER_BYTE);

Interesting dance. Really necessary?  This looks like a classic
write address byte then read register contents sequency.
	u8 tx = AD4000_READ_COMMAND;
	u8 rx;

	ret = spi_write_then_read(spi, tx, 1, rx, 1);
	if (ret < 0)
		return ret;

	*val = rx;

	return ret;


> +	ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = be16_to_cpu(st->rx_buf);
> +
> +	return ret;
> +}
> +
> +static void ad4000_unoptimize_msg(void *msg)
> +{
> +	spi_unoptimize_message(msg);
> +}

Makes me wonder if a devm_spi_optimize_msg() would make sense... 

+static int ad4000_single_conversion(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan, int *val)
+{
+	struct ad4000_state *st = iio_priv(indio_dev);
+	u32 sample;
+	int ret;
+
+	ret = ad4000_convert_and_acquire(st);
+	if (ret < 0)
+		return ret;
+
+	if (chan->scan_type.storagebits > 16)
+		sample = be32_to_cpu(st->scan.data.sample_buf32);
+	else
+		sample = be16_to_cpu(st->scan.data.sample_buf16);
+
	I'd probably just do
	sample >>= chan->scan_type.shift;

+	switch (chan->scan_type.realbits) {
+	case 16:
+		break;
+	case 18:
+		sample = FIELD_GET(AD4000_18BIT_MSK, sample);
+		break;
+	case 20:
+		sample = FIELD_GET(AD4000_20BIT_MSK, sample);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (chan->scan_type.sign == 's')
+		*val = sign_extend32(sample, chan->scan_type.realbits - 1);
+
+	return IIO_VAL_INT;
+}
+


> +static int ad4000_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long info)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = (int *)st->scale_tbl;
> +		*length = 2 * 2;
I'd use a define for the length of the array (of pairs) and reuse that
here.

> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static int ad4000_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val, int val2,
> +			    long mask)
> +{
> +	struct ad4000_state *st = iio_priv(indio_dev);
> +	unsigned int reg_val;
> +	bool span_comp_en;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
If you are going to do a read modify write cycle, you should hold a local
lock to avoid races.

Relying on the implementation of iio_device_claim_direct_coped() for that
isn't a good idea as that may change in the future.


> +			ret = ad4000_read_reg(st, &reg_val);
> +			if (ret < 0)
> +				return ret;
> +
> +			span_comp_en = val2 == st->scale_tbl[1][1];
> +			reg_val &= ~AD4000_CFG_SPAN_COMP;
> +			reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en);
> +
> +			ret = ad4000_write_reg(st, reg_val);
> +			if (ret < 0)
> +				return ret;
> +
> +			st->span_comp = span_comp_en;
> +			return 0;
> +		}
> +		unreachable();
> +	default:
> +		return -EINVAL;
> +	}
> +}


> +
> +static int ad4000_probe(struct spi_device *spi)
> +{
> +	const struct ad4000_chip_info *chip;
> +	struct iio_dev *indio_dev;
> +	struct ad4000_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = spi_get_device_match_data(spi);
> +	if (!chip)
> +		return -EINVAL;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	ret = devm_regulator_get_enable(&spi->dev, "vdd");

A lot of spi->dev, I'd add a
struct device *dev = spi->dev; and use that throughout.

> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "Failed to enable VDD supply\n");
> +
>
...

> +	indio_dev->name = chip->dev_name;
> +	indio_dev->num_channels = 1;
> +
> +	/* Hardware gain only applies to ADAQ devices */

Probably best not to rely on the DT binding sanity. So I'd add a flag for
support of this to your chip info structures and only try reading the
property if you have an adaq.  With that in place you can probably
drop the comment as well as it will be easy to tell by seeing who
as set st->has_hardware_gain (name up to you)
> +	st->gain_milli = 1000;
> +	if (device_property_present(&spi->dev, "adi,gain-milli")) {
> +		ret = device_property_read_u16(&spi->dev, "adi,gain-milli",
> +					       &st->gain_milli);
> +		if (ret)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Failed to read gain property\n");
> +	}
We are often lazy on parameters with a clear default and just do
	device_property_read_*()
without checking the return value.  That will only overwrite the output
parameter it it succeeds. Otherwise we get the default you set before
the call.

If you prefer the more protective form I don't mind that much.

> +
> +	ad4000_fill_scale_tbl(st, indio_dev->channels);
> +
> +	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> +					      &iio_pollfunc_store_time,
> +					      &ad4000_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}

Re: [PATCH v4 6/6] iio: adc: Add support for AD4000

[Jonathan Cameron]

> > +struct ad4000_chip_info {
> > +	const char *dev_name;
> > +	struct iio_chan_spec chan_spec;
> > +	struct iio_chan_spec three_w_chan_spec;
> > +};  
> 
> I understand the reason for doing this, but it still seems a bit weird
> to me to have two different sets of specs for the same chip. I guess
> we'll see what Jonathan has to say about this.


It's very common, though for a different reason.

Normally it's for cases where we have events (threshold crossing as similar)
and the interrupt is optional. In those case we have channel specs with
and without the event.  In this case the change is small so maybe
the code to set it up on a copy of the chan spec would be fine.

This is simple though so I'd keep it this way.

> 
> > +
> > +static const struct ad4000_chip_info ad4000_chip_info = {
> > +	.dev_name = "ad4000",
> > +	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
> > +	.three_w_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 1),
> > +};
> 
> or could just replace all of this this will spi_w8r8() and have
> a one-line function.
Good point. I'd forgotten that existed.  Better still than
spi_write_then_read.

Glad we spotted some of the same things. Sometimes it's weird
and two reviews are entirely unrelated issues throughout!

Jonathan