[PATCH v5 0/7] Add support for AD4000 series of ADCs

devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v5 0/7] Add support for AD4000 series of ADCs
@ 2024-06-25 21:52 Marcelo Schmitt
  2024-06-25 21:53 ` [PATCH v5 1/7] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration Marcelo Schmitt
                   ` (6 more replies)
  0 siblings, 7 replies; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-25 21:52 UTC (permalink / raw)
  To: broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

This patch series extends the SPI bitbang, gpio, and spi-engine controllers to
support configurable MOSI line idle states.
It then introduces the ad4000 driver which uses the MOSI idle configuration to
provide improved support for the AD4000 series of ADCs.
Documentation is added describing the new extension to the SPI protocol.
The currently supported wiring modes for AD4000 devices were documented under
IIO documentation directory.

Change log v4 -> v5:

[SPI]
- spi: Fixed spi_setup() not failing if controller doesn't support requested MOSI config.
- spi-summary: CS active -> CS asserted; use of `` to highlight code elements.
- spi: spi-engine: renamed SPI_ENGINE_CONFIG_SDO_IDLE -> SPI_ENGINE_CONFIG_SDO_IDLE_HIGH.
[Device tree]
- Added missing compatibles for ADAQ4001 and ADAQ4003.
- Removed example comments.
- Split adi,spi-mode property into adi,sdi-pin and adi,cnv-pin properties.
- Updated property constraints.
[IIO]
- ad4000: Reviewed includes.
- ad4000: Fixed devm_regulator_get_enable_read_voltage() usage.
- ad4000: Used local lock to protect scale change read modify write cycle.
- ad4000: renamed _3wire -> _reg_access; three_w_chan_spec -> reg_access_chan_spec.
- ad4000: Use u8 tx_buf[2] and u8 rx_buf[2] for reg access transfers.
- ad4000: Simplified dt property read.
- ad4000: Use sample >>= chan->scan_type.shift;
- ad4000: Use devm_spi_optimize_message().
- ad4000: Improvement to macros.
[Documentation]
- [New patch] iio: Added documentation for AD4000 describing wiring connection modes.

Link to v4: https://lore.kernel.org/linux-iio/cover.1718749981.git.marcelo.schmitt@analog.com/
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/

I believe to have made everything suggested in v4, except for the following:

- Use of spi_w8r8().
I tried that but then the values I got from reg read alternated between 0x65 and
0xFF (sometimes 0x00).
I didn't find out why so I kept with spi_sync_transfer().
May investigate it better if re-spinning.

- Reducing sample buffer size
I agree we may save some memory reducing the sample buffer size from 32 to 24 bits.
I will probably use get_unaligned_be24() to get the data back but will have to
declare sample buffers as arrays. It was very tricky to get the buffer
declarations correct the first time and I didn't want to take much longer to
provide a v5.
May do it for v6 if re-spinning.


Thanks,
Marcelo

Marcelo Schmitt (7):
  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
  docs: iio: Add documentation for AD4000

 .../bindings/iio/adc/adi,ad4000.yaml          | 190 +++++
 Documentation/iio/ad4000.rst                  | 131 ++++
 Documentation/iio/index.rst                   |   1 +
 Documentation/spi/spi-summary.rst             |  83 ++
 MAINTAINERS                                   |   9 +
 drivers/iio/adc/Kconfig                       |  12 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ad4000.c                      | 711 ++++++++++++++++++
 drivers/spi/spi-axi-spi-engine.c              |   8 +
 drivers/spi/spi-bitbang.c                     |  24 +
 drivers/spi/spi-gpio.c                        |  12 +-
 drivers/spi/spi.c                             |   7 +
 include/linux/spi/spi.h                       |   6 +
 include/linux/spi/spi_bitbang.h               |   1 +
 include/uapi/linux/spi/spi.h                  |   5 +-
 15 files changed, 1198 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
 create mode 100644 Documentation/iio/ad4000.rst
 create mode 100644 drivers/iio/adc/ad4000.c

-- 
2.43.0


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH v5 1/7] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration
  2024-06-25 21:52 [PATCH v5 0/7] Add support for AD4000 series of ADCs Marcelo Schmitt
@ 2024-06-25 21:53 ` Marcelo Schmitt
  2024-06-26  9:37   ` Nuno Sá
  2024-06-26 14:57   ` David Lechner
  2024-06-25 21:53 ` [PATCH v5 2/7] spi: bitbang: Implement support for MOSI idle state configuration Marcelo Schmitt
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-25 21:53 UTC (permalink / raw)
  To: broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

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.

Conventional SPI controllers 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 demanding 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>
---
 Documentation/spi/spi-summary.rst | 83 +++++++++++++++++++++++++++++++
 drivers/spi/spi.c                 |  7 +++
 include/linux/spi/spi.h           |  6 +++
 include/uapi/linux/spi/spi.h      |  5 +-
 4 files changed, 99 insertions(+), 2 deletions(-)

diff --git a/Documentation/spi/spi-summary.rst b/Documentation/spi/spi-summary.rst
index 7f8accfae6f9..51dd8a105b7e 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 asserted but the controller is not clocking out data to
+the peripheral and also when CS is not asserted.
+
+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 9bc9fd10d538..341803110a06 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3942,6 +3942,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 idle low and high at the same time.\n");
+		return -EINVAL;
+	}
 	/*
 	 * Help drivers fail *cleanly* when they need options
 	 * that aren't supported with their current controller.
@@ -3950,6 +3956,7 @@ int spi_setup(struct spi_device *spi)
 	 */
 	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);
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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v5 2/7] spi: bitbang: Implement support for MOSI idle state configuration
  2024-06-25 21:52 [PATCH v5 0/7] Add support for AD4000 series of ADCs Marcelo Schmitt
  2024-06-25 21:53 ` [PATCH v5 1/7] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration Marcelo Schmitt
@ 2024-06-25 21:53 ` Marcelo Schmitt
  2024-06-26 15:01   ` David Lechner
  2024-06-25 21:54 ` [PATCH v5 3/7] spi: spi-gpio: Add " Marcelo Schmitt
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-25 21:53 UTC (permalink / raw)
  To: broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v5 3/7] spi: spi-gpio: Add support for MOSI idle state configuration
  2024-06-25 21:52 [PATCH v5 0/7] Add support for AD4000 series of ADCs Marcelo Schmitt
  2024-06-25 21:53 ` [PATCH v5 1/7] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration Marcelo Schmitt
  2024-06-25 21:53 ` [PATCH v5 2/7] spi: bitbang: Implement support for MOSI idle state configuration Marcelo Schmitt
@ 2024-06-25 21:54 ` Marcelo Schmitt
  2024-06-26 15:02   ` David Lechner
  2024-06-25 21:54 ` [PATCH v5 4/7] spi: spi-axi-spi-engine: Add support for MOSI idle configuration Marcelo Schmitt
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-25 21:54 UTC (permalink / raw)
  To: broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v5 4/7] spi: spi-axi-spi-engine: Add support for MOSI idle configuration
  2024-06-25 21:52 [PATCH v5 0/7] Add support for AD4000 series of ADCs Marcelo Schmitt
                   ` (2 preceding siblings ...)
  2024-06-25 21:54 ` [PATCH v5 3/7] spi: spi-gpio: Add " Marcelo Schmitt
@ 2024-06-25 21:54 ` Marcelo Schmitt
  2024-06-26  6:14   ` Alexandru Ardelean
                     ` (2 more replies)
  2024-06-25 21:55 ` [PATCH v5 5/7] dt-bindings: iio: adc: Add AD4000 Marcelo Schmitt
                   ` (2 subsequent siblings)
  6 siblings, 3 replies; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-25 21:54 UTC (permalink / raw)
  To: broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

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..5a88d31ca758 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_HIGH		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_HIGH;
+	if (spi->mode & SPI_MOSI_IDLE_LOW)
+		config &= ~SPI_ENGINE_CONFIG_SDO_IDLE_HIGH;
 
 	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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v5 5/7] dt-bindings: iio: adc: Add AD4000
  2024-06-25 21:52 [PATCH v5 0/7] Add support for AD4000 series of ADCs Marcelo Schmitt
                   ` (3 preceding siblings ...)
  2024-06-25 21:54 ` [PATCH v5 4/7] spi: spi-axi-spi-engine: Add support for MOSI idle configuration Marcelo Schmitt
@ 2024-06-25 21:55 ` Marcelo Schmitt
  2024-06-26 11:34   ` Conor Dooley
  2024-06-25 21:55 ` [PATCH v5 6/7] iio: adc: Add support for AD4000 Marcelo Schmitt
  2024-06-25 21:55 ` [PATCH v5 7/7] docs: iio: Add documentation " Marcelo Schmitt
  6 siblings, 1 reply; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-25 21:55 UTC (permalink / raw)
  To: broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

Add device tree documentation for AD4000 series of ADC devices.

Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
 .../bindings/iio/adc/adi,ad4000.yaml          | 190 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 197 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..76035dff5474
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
@@ -0,0 +1,190 @@
+# 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
+      - const: adi,adaq4001
+      - const: adi,adaq4003
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
+
+  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.
+
+  '#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:
+      When provided, this property indicates the GPIO that is connected to the
+      CNV pin.
+    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 ("3-wire" mode) or the
+      SDI line is low and the CNV line is high ("4-wire" 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:
+  # The configuration register can only be accessed if SDI is connected to MOSI
+  - if:
+      required:
+        - adi,sdi-pin
+    then:
+      properties:
+        adi,high-z-input: false
+  # chain mode has lower SCLK max rate
+  - if:
+      required:
+        - '#daisy-chained-devices'
+    then:
+      properties:
+        spi-max-frequency:
+          maximum: 50000000 # for VIO > 2.7 V, 40000000 for VIO > 1.7 V
+  # 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>;
+        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>;
+            adi,sdi-pin = "cs";
+            cnv-gpios = <&gpio0 88 GPIO_ACTIVE_HIGH>;
+        };
+    };
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        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>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 9517093d889d..9aa6531f7cf2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1199,6 +1199,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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v5 6/7] iio: adc: Add support for AD4000
  2024-06-25 21:52 [PATCH v5 0/7] Add support for AD4000 series of ADCs Marcelo Schmitt
                   ` (4 preceding siblings ...)
  2024-06-25 21:55 ` [PATCH v5 5/7] dt-bindings: iio: adc: Add AD4000 Marcelo Schmitt
@ 2024-06-25 21:55 ` Marcelo Schmitt
  2024-06-26  6:11   ` Alexandru Ardelean
                     ` (3 more replies)
  2024-06-25 21:55 ` [PATCH v5 7/7] docs: iio: Add documentation " Marcelo Schmitt
  6 siblings, 4 replies; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-25 21:55 UTC (permalink / raw)
  To: broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

Add support for AD4000 series of low noise, low power, high speed,
successive approximation 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 | 711 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 725 insertions(+)
 create mode 100644 drivers/iio/adc/ad4000.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 9aa6531f7cf2..f4ffedada8ea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1205,6 +1205,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 b8184706c7d1..5bbe843916a3 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 51298c52b223..f4361df40cca 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..0b6293db68dc
--- /dev/null
+++ b/drivers/iio/adc/ad4000.c
@@ -0,0 +1,711 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AD4000 SPI ADC driver
+ *
+ * Copyright 2024 Analog Devices Inc.
+ */
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/byteorder/generic.h>
+#include <linux/device.h>
+#include <linux/err.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/units.h>
+#include <linux/util_macros.h>
+#include <linux/iio/iio.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_SCALE_OPTIONS		2
+
+#define AD4000_TQUIET1_NS		190
+#define AD4000_TQUIET2_NS		60
+#define AD4000_TCONV_NS			320
+
+#define __AD4000_DIFF_CHANNEL(_sign, _real_bits, _storage_bits, _reg_access)	\
+{										\
+	.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 = _reg_access ? BIT(IIO_CHAN_INFO_SCALE) : 0,\
+	.scan_type = {								\
+		.sign = _sign,							\
+		.realbits = _real_bits,						\
+		.storagebits = _storage_bits,					\
+		.shift = _storage_bits - _real_bits,				\
+		.endianness = IIO_BE,						\
+	},									\
+}
+
+#define AD4000_DIFF_CHANNEL(_sign, _real_bits, _reg_access)			\
+	__AD4000_DIFF_CHANNEL((_sign), (_real_bits),				\
+				     ((_real_bits) > 16 ? 32 : 16), (_reg_access))
+
+#define __AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _storage_bits, _reg_access)\
+{										\
+	.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 = _reg_access ? BIT(IIO_CHAN_INFO_SCALE) : 0,\
+	.scan_type = {								\
+		.sign = _sign,							\
+		.realbits = _real_bits,						\
+		.storagebits = _storage_bits,					\
+		.shift = _storage_bits - _real_bits,				\
+		.endianness = IIO_BE,						\
+	},									\
+}
+
+#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _reg_access)		\
+	__AD4000_PSEUDO_DIFF_CHANNEL((_sign), (_real_bits),			\
+				     ((_real_bits) > 16 ? 32 : 16), (_reg_access))
+
+enum ad4000_sdi {
+	/* datasheet calls this "4-wire mode" (controller CS goes to ADC SDI!) */
+	AD4000_SDI_MOSI,
+	/* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
+	AD4000_SDI_VIO,
+	AD4000_SDI_CS,
+};
+
+/* maps adi,sdi-pin property value to enum */
+static const char * const ad4000_sdi_pin[] = {
+	[AD4000_SDI_MOSI] = "",
+	[AD4000_SDI_VIO] = "high",
+	[AD4000_SDI_CS] = "cs",
+};
+
+struct ad4000_chip_info {
+	const char *dev_name;
+	struct iio_chan_spec chan_spec;
+	struct iio_chan_spec reg_access_chan_spec;
+	bool has_hardware_gain;
+};
+
+static const struct ad4000_chip_info ad4000_chip_info = {
+	.dev_name = "ad4000",
+	.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_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),
+	.reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 16, 1),
+	.has_hardware_gain = true,
+};
+
+static const struct ad4000_chip_info adaq4003_chip_info = {
+	.dev_name = "adaq4003",
+	.chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
+	.reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
+	.has_hardware_gain = true,
+};
+
+struct ad4000_state {
+	struct spi_device *spi;
+	struct gpio_desc *cnv_gpio;
+	struct spi_transfer xfers[2];
+	struct spi_message msg;
+	struct mutex lock; /* Protect read modify write cycle */
+	int vref_mv;
+	enum ad4000_sdi sdi_pin;
+	bool span_comp;
+	bool turbo_mode;
+	u16 gain_milli;
+	int scale_tbl[AD4000_SCALE_OPTIONS][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);
+	u8 tx_buf[2];
+	u8 rx_buf[2];
+};
+
+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[0] = AD4000_WRITE_COMMAND;
+	st->tx_buf[1] = val;
+	return spi_write(st->spi, st->tx_buf, ARRAY_SIZE(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[0] = AD4000_READ_COMMAND;
+	ret = spi_sync_transfer(st->spi, &t, 1);
+	if (ret < 0)
+		return ret;
+
+	*val = st->tx_buf[1];
+	return ret;
+}
+
+/*
+ * This executes a data sample transfer for when the device connections are
+ * in "3-wire" mode, selected when the adi,sdi-pin device tree property is
+ * absent or set to "high". 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;
+
+	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);
+
+	return devm_spi_optimize_message(st->spi, &st->msg);
+}
+
+/*
+ * This executes a data sample transfer for when the device connections are
+ * in "4-wire" mode, selected when the adi,sdi-pin device tree property is
+ * set to "cs". 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;
+
+	/*
+	 * 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);
+
+	return devm_spi_optimize_message(st->spi, &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);
+
+	sample >>= chan->scan_type.shift;
+
+	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 = AD4000_SCALE_OPTIONS * 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:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret < 0)
+			return ret;
+
+		mutex_lock(&st->lock);
+		ret = ad4000_read_reg(st, &reg_val);
+		if (ret < 0)
+			goto err_unlock;
+
+		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)
+			goto err_unlock;
+
+		st->span_comp = span_comp_en;
+err_unlock:
+		iio_device_release_direct_mode(indio_dev);
+		mutex_unlock(&st->lock);
+		return ret;
+	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, pf->timestamp);
+
+err_out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info ad4000_reg_access_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 device *dev = &spi->dev;
+	struct iio_dev *indio_dev;
+	struct ad4000_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(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(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable VDD supply\n");
+
+	ret = devm_regulator_get_enable(dev, "vio");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable VIO supply\n");
+
+	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Failed to get ref regulator reference\n");
+	st->vref_mv = ret / 1000;
+
+	st->cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_HIGH);
+	if (IS_ERR(st->cnv_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
+				     "Failed to get CNV GPIO");
+
+	ret = device_property_match_property_string(dev, "adi,sdi-pin",
+						    ad4000_sdi_pin,
+						    ARRAY_SIZE(ad4000_sdi_pin));
+	if (ret < 0 && ret != -EINVAL)
+		return dev_err_probe(dev, ret,
+				     "getting adi,sdi-pin property failed\n");
+
+	/* Default to usual SPI connections if pin properties are not present */
+	st->sdi_pin = ret == -EINVAL ? AD4000_SDI_MOSI : ret;
+	switch (st->sdi_pin) {
+	case AD4000_SDI_MOSI:
+		indio_dev->info = &ad4000_reg_access_info;
+		indio_dev->channels = &chip->reg_access_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_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(dev, "Failed to config device\n");
+
+		break;
+	case AD4000_SDI_VIO:
+		indio_dev->info = &ad4000_info;
+		indio_dev->channels = &chip->chan_spec;
+		ret = ad4000_prepare_3wire_mode_message(st, indio_dev->channels);
+		if (ret)
+			return ret;
+
+		break;
+	case AD4000_SDI_CS:
+		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;
+	default:
+		return dev_err_probe(dev, -EINVAL, "Unrecognized connection mode\n");
+	}
+
+	indio_dev->name = chip->dev_name;
+	indio_dev->num_channels = 1;
+
+	devm_mutex_init(dev, &st->lock);
+
+	st->gain_milli = 1000;
+	if (chip->has_hardware_gain) {
+		if (device_property_present(dev, "adi,gain-milli")) {
+			ret = device_property_read_u16(dev, "adi,gain-milli",
+						       &st->gain_milli);
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "Failed to read gain property\n");
+		}
+	}
+
+	ad4000_fill_scale_tbl(st, indio_dev->channels);
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      &iio_pollfunc_store_time,
+					      &ad4000_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(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


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [PATCH v5 7/7] docs: iio: Add documentation for AD4000
  2024-06-25 21:52 [PATCH v5 0/7] Add support for AD4000 series of ADCs Marcelo Schmitt
                   ` (5 preceding siblings ...)
  2024-06-25 21:55 ` [PATCH v5 6/7] iio: adc: Add support for AD4000 Marcelo Schmitt
@ 2024-06-25 21:55 ` Marcelo Schmitt
  2024-06-26 15:43   ` David Lechner
  6 siblings, 1 reply; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-25 21:55 UTC (permalink / raw)
  To: broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

Document wiring configurations for the AD4000 series of ADCs.

Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
 Documentation/iio/ad4000.rst | 131 +++++++++++++++++++++++++++++++++++
 Documentation/iio/index.rst  |   1 +
 MAINTAINERS                  |   1 +
 3 files changed, 133 insertions(+)
 create mode 100644 Documentation/iio/ad4000.rst

diff --git a/Documentation/iio/ad4000.rst b/Documentation/iio/ad4000.rst
new file mode 100644
index 000000000000..de8fd3ae6e62
--- /dev/null
+++ b/Documentation/iio/ad4000.rst
@@ -0,0 +1,131 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+=============
+AD4000 driver
+=============
+
+Device driver for Analog Devices Inc. AD4000 series of ADCs.
+
+Supported devices
+=================
+
+* `AD4000 <https://www.analog.com/AD4000>`_
+* `AD4001 <https://www.analog.com/AD4001>`_
+* `AD4002 <https://www.analog.com/AD4002>`_
+* `AD4003 <https://www.analog.com/AD4003>`_
+* `AD4004 <https://www.analog.com/AD4004>`_
+* `AD4005 <https://www.analog.com/AD4005>`_
+* `AD4006 <https://www.analog.com/AD4006>`_
+* `AD4007 <https://www.analog.com/AD4007>`_
+* `AD4008 <https://www.analog.com/AD4008>`_
+* `AD4010 <https://www.analog.com/AD4010>`_
+* `AD4011 <https://www.analog.com/AD4011>`_
+* `AD4020 <https://www.analog.com/AD4020>`_
+* `AD4021 <https://www.analog.com/AD4021>`_
+* `AD4022 <https://www.analog.com/AD4022>`_
+* `ADAQ4001 <https://www.analog.com/ADAQ4001>`_
+* `ADAQ4003 <https://www.analog.com/ADAQ4003>`_
+
+Wiring connections
+------------------
+
+Devices of the AD4000 series can be connected to the SPI host controller in a
+few different modes.
+
+CS mode, 3-wire turbo mode
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Datasheet "3-wire" mode is what most resembles standard SPI connection which,
+for these devices, comprises of connecting the controller CS line to device CNV
+pin and other SPI lines as usual. This configuration is (misleadingly) called
+"CS Mode, 3-Wire Turbo Mode" connection in datasheets.
+NOTE: The datasheet definition of 3-wire mode for the AD4000 series is NOT the
+same of standard spi-3wire mode.
+This is the only connection mode that allows configuration register access but
+it requires the SPI controller to support the ``SPI_MOSI_IDLE_HIGH`` feature.
+
+Omit the ``adi,sdi-pin`` property in device tree to select this mode.
+
+::
+
+                                         +-------------+
+     + ----------------------------------| SDO         |
+     |                                   |             |
+     |               +-------------------| CS          |
+     |               v                   |             |
+     |    +--------------------+         |     HOST    |
+     |    |        CNV         |         |             |
+     +--->| SDI   AD4000   SDO |-------->| SDI         |
+          |        SCK         |         |             |
+          +--------------------+         |             |
+                    ^                    |             |
+                    +--------------------| SCLK        |
+                                         +-------------+
+
+CS mode, 3-wire, without busy indicator
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Another wiring configuration supported as "3-wire" mode has the SDI pin
+hard-wired to digital input/output interface supply (VIO). In this setup, the
+controller is not required to support ``SPI_MOSI_IDLE_HIGH`` but register access
+is not possible. This connection mode saves one wire and works with any SPI
+controller.
+
+Set the ``adi,sdi-pin`` device tree property to ``"high"`` to select this mode.
+
+::
+
+                                         +-------------+
+                    +--------------------| CS          |
+                    v                    |             |
+    VIO   +--------------------+         |     HOST    |
+     |    |        CNV         |         |             |
+     +--->| SDI   AD4000   SDO |-------->| SDI         |
+          |        SCK         |         |             |
+          +--------------------+         |             |
+                    ^                    |             |
+                    +--------------------| SCLK        |
+                                         +-------------+
+
+Alternatively, a GPIO may be connected to the device CNV pin. This is similar to
+the previous wiring configuration but saves the use of a CS line.
+
+::
+
+                                         +-------------+
+                    +--------------------| GPIO        |
+                    v                    |             |
+    VIO   +--------------------+         |     HOST    |
+     |    |        CNV         |         |             |
+     +--->| SDI   AD4000   SDO |-------->| SDI         |
+          |        SCK         |         |             |
+          +--------------------+         |             |
+                    ^                    |             |
+                    +--------------------| SCLK        |
+                                         +-------------+
+
+CS mode, 4-wire without busy indicator
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+In datasheet "4-wire" mode, the controller CS line is connected to the ADC SDI
+pin and a GPIO is connected to the ADC CNV pin. This connection mode may better
+suit scenarios where multiple ADCs can share one CNV trigger.
+
+Set ``adi,sdi-pin`` to ``"cs"`` to select this mode.
+
+
+::
+
+                                         +-------------+
+     + ----------------------------------| CS          |
+     |                                   |             |
+     |               +-------------------| GPIO        |
+     |               v                   |             |
+     |    +--------------------+         |     HOST    |
+     |    |        CNV         |         |             |
+     +--->| SDI   AD4000   SDO |-------->| SDI         |
+          |        SCK         |         |             |
+          +--------------------+         |             |
+                    ^                    |             |
+                    +--------------------| SCLK        |
+                                         +-------------+
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index 4c13bfa2865c..5df157a44923 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -17,6 +17,7 @@ Industrial I/O Kernel Drivers
 .. toctree::
    :maxdepth: 1
 
+   ad4000
    ad7944
    adis16475
    adis16480
diff --git a/MAINTAINERS b/MAINTAINERS
index f4ffedada8ea..a03b3db9157c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1205,6 +1205,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:	Documentation/iio/ad4000.rst
 F:	drivers/iio/adc/ad4000.c
 
 ANALOG DEVICES INC AD4130 DRIVER
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 6/7] iio: adc: Add support for AD4000
  2024-06-25 21:55 ` [PATCH v5 6/7] iio: adc: Add support for AD4000 Marcelo Schmitt
@ 2024-06-26  6:11   ` Alexandru Ardelean
  2024-06-26 13:25     ` Marcelo Schmitt
  2024-06-26 11:04   ` Nuno Sá
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 33+ messages in thread
From: Alexandru Ardelean @ 2024-06-26  6:11 UTC (permalink / raw)
  To: Marcelo Schmitt
  Cc: broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1, linux-iio, devicetree, linux-spi, linux-doc,
	linux-kernel

On Wed, Jun 26, 2024 at 12:56 AM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> Add support for AD4000 series of low noise, low power, high speed,
> successive approximation register (SAR) ADCs.
>

Hello :)

Looks good overall.
Just a few comments.
The only one where I am not sure is about the enum-to-string mapping.
If that's fine, we can leave this unchanged (from my side).

> 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 | 711 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 725 insertions(+)
>  create mode 100644 drivers/iio/adc/ad4000.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9aa6531f7cf2..f4ffedada8ea 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1205,6 +1205,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 b8184706c7d1..5bbe843916a3 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 51298c52b223..f4361df40cca 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..0b6293db68dc
> --- /dev/null
> +++ b/drivers/iio/adc/ad4000.c
> @@ -0,0 +1,711 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD4000 SPI ADC driver
> + *
> + * Copyright 2024 Analog Devices Inc.
> + */
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/byteorder/generic.h>
> +#include <linux/device.h>
> +#include <linux/err.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/units.h>
> +#include <linux/util_macros.h>
> +#include <linux/iio/iio.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_SCALE_OPTIONS           2
> +
> +#define AD4000_TQUIET1_NS              190
> +#define AD4000_TQUIET2_NS              60
> +#define AD4000_TCONV_NS                        320
> +
> +#define __AD4000_DIFF_CHANNEL(_sign, _real_bits, _storage_bits, _reg_access)   \
> +{                                                                              \
> +       .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 = _reg_access ? BIT(IIO_CHAN_INFO_SCALE) : 0,\
> +       .scan_type = {                                                          \
> +               .sign = _sign,                                                  \
> +               .realbits = _real_bits,                                         \
> +               .storagebits = _storage_bits,                                   \
> +               .shift = _storage_bits - _real_bits,                            \
> +               .endianness = IIO_BE,                                           \
> +       },                                                                      \
> +}
> +
> +#define AD4000_DIFF_CHANNEL(_sign, _real_bits, _reg_access)                    \
> +       __AD4000_DIFF_CHANNEL((_sign), (_real_bits),                            \
> +                                    ((_real_bits) > 16 ? 32 : 16), (_reg_access))
> +
> +#define __AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _storage_bits, _reg_access)\
> +{                                                                              \
> +       .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 = _reg_access ? BIT(IIO_CHAN_INFO_SCALE) : 0,\
> +       .scan_type = {                                                          \
> +               .sign = _sign,                                                  \
> +               .realbits = _real_bits,                                         \
> +               .storagebits = _storage_bits,                                   \
> +               .shift = _storage_bits - _real_bits,                            \
> +               .endianness = IIO_BE,                                           \
> +       },                                                                      \
> +}
> +
> +#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _reg_access)             \
> +       __AD4000_PSEUDO_DIFF_CHANNEL((_sign), (_real_bits),                     \
> +                                    ((_real_bits) > 16 ? 32 : 16), (_reg_access))
> +
> +enum ad4000_sdi {
> +       /* datasheet calls this "4-wire mode" (controller CS goes to ADC SDI!) */
> +       AD4000_SDI_MOSI,
> +       /* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
> +       AD4000_SDI_VIO,
> +       AD4000_SDI_CS,
> +};
> +
> +/* maps adi,sdi-pin property value to enum */
> +static const char * const ad4000_sdi_pin[] = {
> +       [AD4000_SDI_MOSI] = "",

Maybe I missed a previous comment.
And I'm also a little fuzzy on the details here, but in the DT this
property has "high", "low", "cs".
Is "low" the default if unspecified?
Or should this string be "low"?

> +       [AD4000_SDI_VIO] = "high",
> +       [AD4000_SDI_CS] = "cs",
> +};
> +
> +struct ad4000_chip_info {
> +       const char *dev_name;
> +       struct iio_chan_spec chan_spec;
> +       struct iio_chan_spec reg_access_chan_spec;
> +       bool has_hardware_gain;
> +};
> +
> +static const struct ad4000_chip_info ad4000_chip_info = {
> +       .dev_name = "ad4000",
> +       .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_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),
> +       .reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 16, 1),
> +       .has_hardware_gain = true,
> +};
> +
> +static const struct ad4000_chip_info adaq4003_chip_info = {
> +       .dev_name = "adaq4003",
> +       .chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
> +       .reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
> +       .has_hardware_gain = true,
> +};
> +
> +struct ad4000_state {
> +       struct spi_device *spi;
> +       struct gpio_desc *cnv_gpio;
> +       struct spi_transfer xfers[2];
> +       struct spi_message msg;
> +       struct mutex lock; /* Protect read modify write cycle */
> +       int vref_mv;
> +       enum ad4000_sdi sdi_pin;
> +       bool span_comp;
> +       bool turbo_mode;
> +       u16 gain_milli;
> +       int scale_tbl[AD4000_SCALE_OPTIONS][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);
> +       u8 tx_buf[2];
> +       u8 rx_buf[2];
> +};
> +
> +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[0] = AD4000_WRITE_COMMAND;
> +       st->tx_buf[1] = val;
> +       return spi_write(st->spi, st->tx_buf, ARRAY_SIZE(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[0] = AD4000_READ_COMMAND;
> +       ret = spi_sync_transfer(st->spi, &t, 1);
> +       if (ret < 0)
> +               return ret;
> +
> +       *val = st->tx_buf[1];
> +       return ret;
> +}
> +
> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "3-wire" mode, selected when the adi,sdi-pin device tree property is
> + * absent or set to "high". 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;
> +
> +       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);
> +
> +       return devm_spi_optimize_message(st->spi, &st->msg);
> +}
> +
> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "4-wire" mode, selected when the adi,sdi-pin device tree property is
> + * set to "cs". 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;
> +
> +       /*
> +        * 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);
> +
> +       return devm_spi_optimize_message(st->spi, &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);
> +
> +       sample >>= chan->scan_type.shift;
> +
> +       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 = AD4000_SCALE_OPTIONS * 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:
> +               ret = iio_device_claim_direct_mode(indio_dev);
> +               if (ret < 0)
> +                       return ret;
> +
> +               mutex_lock(&st->lock);
> +               ret = ad4000_read_reg(st, &reg_val);
> +               if (ret < 0)
> +                       goto err_unlock;
> +
> +               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)
> +                       goto err_unlock;
> +
> +               st->span_comp = span_comp_en;
> +err_unlock:
> +               iio_device_release_direct_mode(indio_dev);
> +               mutex_unlock(&st->lock);
> +               return ret;
> +       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, pf->timestamp);
> +
> +err_out:
> +       iio_trigger_notify_done(indio_dev->trig);
> +       return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info ad4000_reg_access_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 device *dev = &spi->dev;
> +       struct iio_dev *indio_dev;
> +       struct ad4000_state *st;
> +       int ret;
> +
> +       indio_dev = devm_iio_device_alloc(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(dev, "vdd");
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to enable VDD supply\n");
> +
> +       ret = devm_regulator_get_enable(dev, "vio");
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to enable VIO supply\n");
> +
> +       ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret,
> +                                    "Failed to get ref regulator reference\n");
> +       st->vref_mv = ret / 1000;
> +
> +       st->cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_HIGH);
> +       if (IS_ERR(st->cnv_gpio))
> +               return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
> +                                    "Failed to get CNV GPIO");
> +
> +       ret = device_property_match_property_string(dev, "adi,sdi-pin",
> +                                                   ad4000_sdi_pin,
> +                                                   ARRAY_SIZE(ad4000_sdi_pin));
> +       if (ret < 0 && ret != -EINVAL)
> +               return dev_err_probe(dev, ret,
> +                                    "getting adi,sdi-pin property failed\n");
> +
> +       /* Default to usual SPI connections if pin properties are not present */
> +       st->sdi_pin = ret == -EINVAL ? AD4000_SDI_MOSI : ret;
> +       switch (st->sdi_pin) {
> +       case AD4000_SDI_MOSI:
> +               indio_dev->info = &ad4000_reg_access_info;
> +               indio_dev->channels = &chip->reg_access_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_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(dev, "Failed to config device\n");
> +
> +               break;
> +       case AD4000_SDI_VIO:
> +               indio_dev->info = &ad4000_info;
> +               indio_dev->channels = &chip->chan_spec;
> +               ret = ad4000_prepare_3wire_mode_message(st, indio_dev->channels);
> +               if (ret)
> +                       return ret;
> +
> +               break;
> +       case AD4000_SDI_CS:
> +               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;
> +       default:
> +               return dev_err_probe(dev, -EINVAL, "Unrecognized connection mode\n");
> +       }
> +
> +       indio_dev->name = chip->dev_name;
> +       indio_dev->num_channels = 1;
> +
> +       devm_mutex_init(dev, &st->lock);
> +
> +       st->gain_milli = 1000;
> +       if (chip->has_hardware_gain) {
> +               if (device_property_present(dev, "adi,gain-milli")) {

Only if there is another version, it may be neat to reduce indentation
here (a bit).
Something like:
        if (chip->has_hardware_gain &&
            device_property_present(dev, "adi,gain-milli")) {

        }

> +                       ret = device_property_read_u16(dev, "adi,gain-milli",
> +                                                      &st->gain_milli);
> +                       if (ret)
> +                               return dev_err_probe(dev, ret,
> +                                                    "Failed to read gain property\n");
> +               }
> +       }
> +
> +       ad4000_fill_scale_tbl(st, indio_dev->channels);
> +
> +       ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +                                             &iio_pollfunc_store_time,
> +                                             &ad4000_trigger_handler, NULL);
> +       if (ret)
> +               return ret;
> +
> +       return devm_iio_device_register(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
>
>

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 4/7] spi: spi-axi-spi-engine: Add support for MOSI idle configuration
  2024-06-25 21:54 ` [PATCH v5 4/7] spi: spi-axi-spi-engine: Add support for MOSI idle configuration Marcelo Schmitt
@ 2024-06-26  6:14   ` Alexandru Ardelean
  2024-06-26 13:27     ` Marcelo Schmitt
  2024-06-26  9:56   ` Nuno Sá
  2024-06-26 15:06   ` David Lechner
  2 siblings, 1 reply; 33+ messages in thread
From: Alexandru Ardelean @ 2024-06-26  6:14 UTC (permalink / raw)
  To: Marcelo Schmitt
  Cc: broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1, linux-iio, devicetree, linux-spi, linux-doc,
	linux-kernel

On Wed, Jun 26, 2024 at 12:55 AM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> Implement MOSI idle low and MOSI idle high to better support peripherals
> that request specific MOSI behavior.
>

One minor nitpick.
Feel free to ignore, if there won't be a re-spin.

> 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..5a88d31ca758 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_HIGH                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_HIGH;
> +       if (spi->mode & SPI_MOSI_IDLE_LOW)
> +               config &= ~SPI_ENGINE_CONFIG_SDO_IDLE_HIGH;
>
>         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;

There's a second space after the assignment.
               host->mode_bits |=<2 spaces here>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
>
>

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 1/7] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration
  2024-06-25 21:53 ` [PATCH v5 1/7] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration Marcelo Schmitt
@ 2024-06-26  9:37   ` Nuno Sá
  2024-06-26 14:57   ` David Lechner
  1 sibling, 0 replies; 33+ messages in thread
From: Nuno Sá @ 2024-06-26  9:37 UTC (permalink / raw)
  To: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel



On Tue, 2024-06-25 at 18:53 -0300, Marcelo Schmitt wrote:
> 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.
> 
> Conventional SPI controllers 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 demanding 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>
> ---

One minor nit... With that:

Acked-by: Nuno Sa <nuno.sa@analog.com>

>  Documentation/spi/spi-summary.rst | 83 +++++++++++++++++++++++++++++++
>  drivers/spi/spi.c                 |  7 +++
>  include/linux/spi/spi.h           |  6 +++
>  include/uapi/linux/spi/spi.h      |  5 +-
>  4 files changed, 99 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/spi/spi-summary.rst b/Documentation/spi/spi-
> summary.rst
> index 7f8accfae6f9..51dd8a105b7e 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).

Forgot this one... inactive -> deasserted (or not asserted)

- Nuno Sá



^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 4/7] spi: spi-axi-spi-engine: Add support for MOSI idle configuration
  2024-06-25 21:54 ` [PATCH v5 4/7] spi: spi-axi-spi-engine: Add support for MOSI idle configuration Marcelo Schmitt
  2024-06-26  6:14   ` Alexandru Ardelean
@ 2024-06-26  9:56   ` Nuno Sá
  2024-06-26 15:06   ` David Lechner
  2 siblings, 0 replies; 33+ messages in thread
From: Nuno Sá @ 2024-06-26  9:56 UTC (permalink / raw)
  To: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On Tue, 2024-06-25 at 18:54 -0300, 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>
> ---

Acked-by: Nuno Sa <nuno.sa@analog.com>




^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 6/7] iio: adc: Add support for AD4000
  2024-06-25 21:55 ` [PATCH v5 6/7] iio: adc: Add support for AD4000 Marcelo Schmitt
  2024-06-26  6:11   ` Alexandru Ardelean
@ 2024-06-26 11:04   ` Nuno Sá
  2024-06-26 13:17     ` Marcelo Schmitt
  2024-06-26 16:56   ` David Lechner
  2024-06-29 18:05   ` Jonathan Cameron
  3 siblings, 1 reply; 33+ messages in thread
From: Nuno Sá @ 2024-06-26 11:04 UTC (permalink / raw)
  To: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On Tue, 2024-06-25 at 18:55 -0300, Marcelo Schmitt wrote:
> Add support for AD4000 series of low noise, low power, high speed,
> successive approximation 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 | 711 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 725 insertions(+)
>  create mode 100644 drivers/iio/adc/ad4000.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9aa6531f7cf2..f4ffedada8ea 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1205,6 +1205,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 b8184706c7d1..5bbe843916a3 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 51298c52b223..f4361df40cca 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..0b6293db68dc
> --- /dev/null
> +++ b/drivers/iio/adc/ad4000.c
> @@ -0,0 +1,711 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD4000 SPI ADC driver
> + *
> + * Copyright 2024 Analog Devices Inc.
> + */
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/byteorder/generic.h>
> +#include <linux/device.h>
> +#include <linux/err.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/units.h>
> +#include <linux/util_macros.h>
> +#include <linux/iio/iio.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> 

...

> +
> +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[0] = AD4000_READ_COMMAND;
> +	ret = spi_sync_transfer(st->spi, &t, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = st->tx_buf[1];

I'm puzzled... tx_buf?

> +	return ret;
> +}
> +
> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "3-wire" mode, selected when the adi,sdi-pin device tree property is
> + * absent or set to "high". 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;
> +
> +	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);
> +
> +	return devm_spi_optimize_message(st->spi, &st->msg);
> +}
> +
> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "4-wire" mode, selected when the adi,sdi-pin device tree property is
> + * set to "cs". 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;
> +
> +	/*
> +	 * 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);
> +
> +	return devm_spi_optimize_message(st->spi, &st->msg);
> +}

nit: you could reduce the scope of the above prepare functions...

> +
> +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);

Just a minor note regarding your comment in the cover. FWIW, I prefer you leave
it like this. Yes, with 24 bits you save some space but then you need an
unaligned access... To me that space savings are really a micro optimization so
I would definitely go with the simpler form.

> +	else
> +		sample = be16_to_cpu(st->scan.data.sample_buf16);
> +
> +	sample >>= chan->scan_type.shift;
> +
> +	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 = AD4000_SCALE_OPTIONS * 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:
> +		ret = iio_device_claim_direct_mode(indio_dev);

iio_device_claim_direct_scoped()?

> +		if (ret < 0)
> +			return ret;
> +
> +		mutex_lock(&st->lock);

guard()?

> +		ret = ad4000_read_reg(st, &reg_val);
> +		if (ret < 0)
> +			goto err_unlock;
> +
> +		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)
> +			goto err_unlock;
> +
> +		st->span_comp = span_comp_en;
> +err_unlock:
> +		iio_device_release_direct_mode(indio_dev);
> +		mutex_unlock(&st->lock);
> +		return ret;
> +	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, pf-
> >timestamp);
> +
> +err_out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info ad4000_reg_access_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 device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad4000_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(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(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable VDD
> supply\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vio");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable VIO
> supply\n");

devm_regulator_bulk_get_enable()? Do we have any ordering constrains?

> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get ref regulator
> reference\n");
> +	st->vref_mv = ret / 1000;
> +
> +	st->cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->cnv_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
> +				     "Failed to get CNV GPIO");
> +
> +	ret = device_property_match_property_string(dev, "adi,sdi-pin",
> +						    ad4000_sdi_pin,
> +						   
> ARRAY_SIZE(ad4000_sdi_pin));
> +	if (ret < 0 && ret != -EINVAL)
> +		return dev_err_probe(dev, ret,
> +				     "getting adi,sdi-pin property
> failed\n");
> +
> +	/* Default to usual SPI connections if pin properties are not present
> */
> +	st->sdi_pin = ret == -EINVAL ? AD4000_SDI_MOSI : ret;
> +	switch (st->sdi_pin) {
> +	case AD4000_SDI_MOSI:
> +		indio_dev->info = &ad4000_reg_access_info;
> +		indio_dev->channels = &chip->reg_access_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_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(dev, "Failed to config device\n");
> +

Should this be a warning? Very suspicious :)

> +		break;
> +	case AD4000_SDI_VIO:
> +		indio_dev->info = &ad4000_info;
> +		indio_dev->channels = &chip->chan_spec;
> +		ret = ad4000_prepare_3wire_mode_message(st, indio_dev-
> >channels);
> +		if (ret)
> +			return ret;
> +
> +		break;
> +	case AD4000_SDI_CS:
> +		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;
> +	default:
> +		return dev_err_probe(dev, -EINVAL, "Unrecognized connection
> mode\n");
> +	}
> +
> +	indio_dev->name = chip->dev_name;
> +	indio_dev->num_channels = 1;
> +
> +	devm_mutex_init(dev, &st->lock);
> +
> +	st->gain_milli = 1000;
> +	if (chip->has_hardware_gain) {
> +		if (device_property_present(dev, "adi,gain-milli")) {
> +			ret = device_property_read_u16(dev, "adi,gain-milli",
> +						       &st->gain_milli);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "Failed to read gain
> property\n");
> +		}
> 

the above looks odd. Why not?

ret = device_property_read_u16(dev, "adi,gain-milli", &st->gain_milli);
if (!ret) {
	...
}

Note that you're also allowing any value for gain_milli when we just allow some
of them (according to the bindings). Hence you should make sure we get supported
values from FW.

- Nuno Sá

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 5/7] dt-bindings: iio: adc: Add AD4000
  2024-06-25 21:55 ` [PATCH v5 5/7] dt-bindings: iio: adc: Add AD4000 Marcelo Schmitt
@ 2024-06-26 11:34   ` Conor Dooley
  2024-06-26 13:34     ` Marcelo Schmitt
  0 siblings, 1 reply; 33+ messages in thread
From: Conor Dooley @ 2024-06-26 11:34 UTC (permalink / raw)
  To: Marcelo Schmitt
  Cc: broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	marcelo.schmitt1, linux-iio, devicetree, linux-spi, linux-doc,
	linux-kernel

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

On Tue, Jun 25, 2024 at 06:55:03PM -0300, Marcelo Schmitt wrote:
> Add device tree documentation for AD4000 series of ADC devices.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad4000.yaml          | 190 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 197 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..76035dff5474
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> @@ -0,0 +1,190 @@
> +# 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

> +      - const: adi,adaq4001

> +      - const: adi,adaq4003

I think some blank lines, maybe like the above, would go a long way with
this list of compatibles.

> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
> +
> +  adi,sdi-pin:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ high, low, cs ]

    enum: [ high, low, cs, sdi ]
    default: sdi

I'd do this, so that the default is documented in the binding, not in
the description text.

Otherwise, this looks good to me.

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 6/7] iio: adc: Add support for AD4000
  2024-06-26 11:04   ` Nuno Sá
@ 2024-06-26 13:17     ` Marcelo Schmitt
  2024-06-26 13:45       ` Nuno Sá
  0 siblings, 1 reply; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-26 13:17 UTC (permalink / raw)
  To: Nuno Sá
  Cc: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 06/26, Nuno Sá wrote:
> On Tue, 2024-06-25 at 18:55 -0300, Marcelo Schmitt wrote:
> > Add support for AD4000 series of low noise, low power, high speed,
> > successive approximation 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 | 711 +++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 725 insertions(+)
> >  create mode 100644 drivers/iio/adc/ad4000.c
> > 
...
> ...
> 
> > +
> > +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[0] = AD4000_READ_COMMAND;
> > +	ret = spi_sync_transfer(st->spi, &t, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*val = st->tx_buf[1];
> 
> I'm puzzled... tx_buf?
> 
Oh my, I must have messed up when changing to array buffers.
Looks like v6 will be coming :)

> > +	return ret;
> > +}
> > +
> > +/*
> > + * This executes a data sample transfer for when the device connections are
> > + * in "3-wire" mode, selected when the adi,sdi-pin device tree property is
> > + * absent or set to "high". 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;
> > +
> > +	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);
> > +
> > +	return devm_spi_optimize_message(st->spi, &st->msg);
> > +}
> > +
> > +/*
> > + * This executes a data sample transfer for when the device connections are
> > + * in "4-wire" mode, selected when the adi,sdi-pin device tree property is
> > + * set to "cs". 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;
> > +
> > +	/*
> > +	 * 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);
> > +
> > +	return devm_spi_optimize_message(st->spi, &st->msg);
> > +}
> 
> nit: you could reduce the scope of the above prepare functions...

Not sure I got what you mean with this comment Nuno.
Would it be preferable to prepare the 3-wire/4-wire transfers within the switch
cases in probe?

> 
> > +
> > +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);
> 
> Just a minor note regarding your comment in the cover. FWIW, I prefer you leave
> it like this. Yes, with 24 bits you save some space but then you need an
> unaligned access... To me that space savings are really a micro optimization so
> I would definitely go with the simpler form.
> 
I'm no expert on this. Will go with what maintainers say.

> > +	else
> > +		sample = be16_to_cpu(st->scan.data.sample_buf16);
> > +
> > +	sample >>= chan->scan_type.shift;
> > +
> > +	if (chan->scan_type.sign == 's')
> > +		*val = sign_extend32(sample, chan->scan_type.realbits - 1);
> > +
> > +	return IIO_VAL_INT;
> > +}
> > +
...
> > +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:
> > +		ret = iio_device_claim_direct_mode(indio_dev);
> 
> iio_device_claim_direct_scoped()?

I had iio_device_claim_direct_scoped() in v4 but was asked to use a local
lock to protect the read modify write cycle here.
> 
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		mutex_lock(&st->lock);
> 
> guard()?

This guard() stuff is somewhat new to me.
Will check out if can use it here.

> 
> > +		ret = ad4000_read_reg(st, &reg_val);
> > +		if (ret < 0)
> > +			goto err_unlock;
> > +
> > +		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)
> > +			goto err_unlock;
> > +
> > +		st->span_comp = span_comp_en;
> > +err_unlock:
> > +		iio_device_release_direct_mode(indio_dev);
> > +		mutex_unlock(&st->lock);
> > +		return ret;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
...
> > +
> > +static int ad4000_probe(struct spi_device *spi)
> > +{
> > +	const struct ad4000_chip_info *chip;
> > +	struct device *dev = &spi->dev;
> > +	struct iio_dev *indio_dev;
> > +	struct ad4000_state *st;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(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(dev, "vdd");
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to enable VDD
> > supply\n");
> > +
> > +	ret = devm_regulator_get_enable(dev, "vio");
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to enable VIO
> > supply\n");
> 
> devm_regulator_bulk_get_enable()? Do we have any ordering constrains?

No ordering constraints, but vdd and vio are optional while ref is required and
we need to get the voltage of ref.
devm_regulator_bulk_get_enable_read_voltage()? and discard vdd and vio voltages?

> 
> > +
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret,
> > +				     "Failed to get ref regulator
> > reference\n");
> > +	st->vref_mv = ret / 1000;
> > +
> > +	st->cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(st->cnv_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
> > +				     "Failed to get CNV GPIO");
> > +
> > +	ret = device_property_match_property_string(dev, "adi,sdi-pin",
> > +						    ad4000_sdi_pin,
> > +						   
> > ARRAY_SIZE(ad4000_sdi_pin));
> > +	if (ret < 0 && ret != -EINVAL)
> > +		return dev_err_probe(dev, ret,
> > +				     "getting adi,sdi-pin property
> > failed\n");
> > +
> > +	/* Default to usual SPI connections if pin properties are not present
> > */
> > +	st->sdi_pin = ret == -EINVAL ? AD4000_SDI_MOSI : ret;
> > +	switch (st->sdi_pin) {
> > +	case AD4000_SDI_MOSI:
> > +		indio_dev->info = &ad4000_reg_access_info;
> > +		indio_dev->channels = &chip->reg_access_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_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(dev, "Failed to config device\n");
> > +
> 
> Should this be a warning? Very suspicious :)

This devices have some many possible wiring configurations.
I didn't want to fail just because reg access fail.
Maybe ADC SDI was wired to VIO but dt don't have adi,sdi-pin = "high".
Reg access will fail but sample read should work.

> 
> > +		break;
> > +	case AD4000_SDI_VIO:
> > +		indio_dev->info = &ad4000_info;
> > +		indio_dev->channels = &chip->chan_spec;
> > +		ret = ad4000_prepare_3wire_mode_message(st, indio_dev-
> > >channels);
> > +		if (ret)
> > +			return ret;
> > +
> > +		break;
> > +	case AD4000_SDI_CS:
> > +		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;
> > +	default:
> > +		return dev_err_probe(dev, -EINVAL, "Unrecognized connection
> > mode\n");
> > +	}
> > +
> > +	indio_dev->name = chip->dev_name;
> > +	indio_dev->num_channels = 1;
> > +
> > +	devm_mutex_init(dev, &st->lock);
> > +
> > +	st->gain_milli = 1000;
> > +	if (chip->has_hardware_gain) {
> > +		if (device_property_present(dev, "adi,gain-milli")) {
> > +			ret = device_property_read_u16(dev, "adi,gain-milli",
> > +						       &st->gain_milli);
> > +			if (ret)
> > +				return dev_err_probe(dev, ret,
> > +						     "Failed to read gain
> > property\n");
> > +		}
> > 
> 
> the above looks odd. Why not?
> 
> ret = device_property_read_u16(dev, "adi,gain-milli", &st->gain_milli);
> if (!ret) {
> 	...
> }

I wanted to be more protective in case anything strange comes from dt.

> 
> Note that you're also allowing any value for gain_milli when we just allow some
> of them (according to the bindings). Hence you should make sure we get supported
> values from FW.

Yes, but anything different from what is specified in the binding should make
dtbs_check fail, no?
can use device_property_match_property_string() so we assure only supported
gain-milli values in the driver as well.

> 
> - Nuno Sá

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 6/7] iio: adc: Add support for AD4000
  2024-06-26  6:11   ` Alexandru Ardelean
@ 2024-06-26 13:25     ` Marcelo Schmitt
  0 siblings, 0 replies; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-26 13:25 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 06/26, Alexandru Ardelean wrote:
> On Wed, Jun 26, 2024 at 12:56 AM Marcelo Schmitt
> <marcelo.schmitt@analog.com> wrote:
> >
> > Add support for AD4000 series of low noise, low power, high speed,
> > successive approximation register (SAR) ADCs.
> >
> 
> Hello :)

Hey Alexandru, nice to hear from you.

> 
> Looks good overall.
> Just a few comments.
> The only one where I am not sure is about the enum-to-string mapping.
> If that's fine, we can leave this unchanged (from my side).
> 
> > 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 | 711 +++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 725 insertions(+)
> >  create mode 100644 drivers/iio/adc/ad4000.c
> >
...
> > +enum ad4000_sdi {
> > +       /* datasheet calls this "4-wire mode" (controller CS goes to ADC SDI!) */
> > +       AD4000_SDI_MOSI,
> > +       /* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
> > +       AD4000_SDI_VIO,
> > +       AD4000_SDI_CS,
> > +};
> > +
> > +/* maps adi,sdi-pin property value to enum */
> > +static const char * const ad4000_sdi_pin[] = {
> > +       [AD4000_SDI_MOSI] = "",
> 
> Maybe I missed a previous comment.
> And I'm also a little fuzzy on the details here, but in the DT this
> property has "high", "low", "cs".
> Is "low" the default if unspecified?
> Or should this string be "low"?

The default is to have MOSI connected to ADC SDI pin which was empty adi,sdi-pin
dt property in v5.
Will make the defalut explicit as "sdi" as suggested in dt-binding review.

> 
> > +       [AD4000_SDI_VIO] = "high",
> > +       [AD4000_SDI_CS] = "cs",
> > +};
> > +
...
> > +
> > +       st->gain_milli = 1000;
> > +       if (chip->has_hardware_gain) {
> > +               if (device_property_present(dev, "adi,gain-milli")) {
> 
> Only if there is another version, it may be neat to reduce indentation
> here (a bit).
> Something like:
>         if (chip->has_hardware_gain &&
>             device_property_present(dev, "adi,gain-milli")) {
> 
>         }
> 
looks good, will do.

Thanks

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 4/7] spi: spi-axi-spi-engine: Add support for MOSI idle configuration
  2024-06-26  6:14   ` Alexandru Ardelean
@ 2024-06-26 13:27     ` Marcelo Schmitt
  0 siblings, 0 replies; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-26 13:27 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 06/26, Alexandru Ardelean wrote:
> On Wed, Jun 26, 2024 at 12:55 AM Marcelo Schmitt
> <marcelo.schmitt@analog.com> wrote:
> >
> > Implement MOSI idle low and MOSI idle high to better support peripherals
> > that request specific MOSI behavior.
> >
> 
> One minor nitpick.
> Feel free to ignore, if there won't be a re-spin.
> 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > ---
> >  drivers/spi/spi-axi-spi-engine.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
...
> > @@ -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;
> 
> There's a second space after the assignment.
>                host->mode_bits |=<2 spaces here>SPI_MOSI_IDLE_LOW |
> SPI_MOSI_IDLE_HIGH;
ack

thanks
> 
> 
> >         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
> >
> >

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 5/7] dt-bindings: iio: adc: Add AD4000
  2024-06-26 11:34   ` Conor Dooley
@ 2024-06-26 13:34     ` Marcelo Schmitt
  2024-06-26 15:55       ` Conor Dooley
  0 siblings, 1 reply; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-26 13:34 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 06/26, Conor Dooley wrote:
> On Tue, Jun 25, 2024 at 06:55:03PM -0300, Marcelo Schmitt wrote:
> > Add device tree documentation for AD4000 series of ADC devices.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > ---
> >  .../bindings/iio/adc/adi,ad4000.yaml          | 190 ++++++++++++++++++
> >  MAINTAINERS                                   |   7 +
...
> > +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
> 
> > +      - const: adi,adaq4001
> 
> > +      - const: adi,adaq4003
> 
> I think some blank lines, maybe like the above, would go a long way with
> this list of compatibles.
> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
> > +
> > +  adi,sdi-pin:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [ high, low, cs ]
> 
>     enum: [ high, low, cs, sdi ]
>     default: sdi
> 
> I'd do this, so that the default is documented in the binding, not in
> the description text.
> 
> Otherwise, this looks good to me.

Ack, will do.
Thanks

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 6/7] iio: adc: Add support for AD4000
  2024-06-26 13:17     ` Marcelo Schmitt
@ 2024-06-26 13:45       ` Nuno Sá
  2024-06-27 17:09         ` Marcelo Schmitt
  0 siblings, 1 reply; 33+ messages in thread
From: Nuno Sá @ 2024-06-26 13:45 UTC (permalink / raw)
  To: Marcelo Schmitt
  Cc: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On Wed, 2024-06-26 at 10:17 -0300, Marcelo Schmitt wrote:
> On 06/26, Nuno Sá wrote:
> > On Tue, 2024-06-25 at 18:55 -0300, Marcelo Schmitt wrote:
> > > Add support for AD4000 series of low noise, low power, high speed,
> > > successive approximation 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 | 711 +++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 725 insertions(+)
> > >  create mode 100644 drivers/iio/adc/ad4000.c
> > > 
> 

...

> > 
> > nit: you could reduce the scope of the above prepare functions...
> 
> Not sure I got what you mean with this comment Nuno.
> Would it be preferable to prepare the 3-wire/4-wire transfers within the
> switch
> cases in probe?
> 

These functions are only called from probe() right? So they could closer to the
probe function. Anyways a nitpick comment :)

...

> 
> > 
> > 
> > iio_device_claim_direct_scoped()?
> 
> I had iio_device_claim_direct_scoped() in v4 but was asked to use a local
> lock to protect the read modify write cycle here.
> > 
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		mutex_lock(&st->lock);
> > 
> > guard()?
> 
> This guard() stuff is somewhat new to me.
> Will check out if can use it here.

should be doable... 

> 
> > 
> > > +		ret = ad4000_read_reg(st, &reg_val);
> > > +		if (ret < 0)
> > > +			goto err_unlock;
> > > +
> > > +		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)
> > > +			goto err_unlock;
> > > +
> > > +		st->span_comp = span_comp_en;
> > > +err_unlock:
> > > +		iio_device_release_direct_mode(indio_dev);
> > > +		mutex_unlock(&st->lock);
> > > +		return ret;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> ...
> > > +
> > > +static int ad4000_probe(struct spi_device *spi)
> > > +{
> > > +	const struct ad4000_chip_info *chip;
> > > +	struct device *dev = &spi->dev;
> > > +	struct iio_dev *indio_dev;
> > > +	struct ad4000_state *st;
> > > +	int ret;
> > > +
> > > +	indio_dev = devm_iio_device_alloc(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(dev, "vdd");
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "Failed to enable VDD
> > > supply\n");
> > > +
> > > +	ret = devm_regulator_get_enable(dev, "vio");
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "Failed to enable VIO
> > > supply\n");
> > 
> > devm_regulator_bulk_get_enable()? Do we have any ordering constrains?
> 
> No ordering constraints, but vdd and vio are optional while ref is required
> and
> we need to get the voltage of ref.
> devm_regulator_bulk_get_enable_read_voltage()? and discard vdd and vio
> voltages?

Hmmm, vdd and vio do not look like optional to me :). Anyways I meant
devm_regulator_bulk_get_enable() only for vdd and vio and still treat ref
separately.

> 
> > 
> > > +
> > > +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> > > +	if (ret < 0)
> > > +		return dev_err_probe(dev, ret,
> > > +				     "Failed to get ref regulator
> > > reference\n");
> > > +	st->vref_mv = ret / 1000;
> > > +
> > > +	st->cnv_gpio = devm_gpiod_get_optional(dev, "cnv",
> > > GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(st->cnv_gpio))
> > > +		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
> > > +				     "Failed to get CNV GPIO");
> > > +
> > > +	ret = device_property_match_property_string(dev, "adi,sdi-pin",
> > > +						    ad4000_sdi_pin,
> > > +						   
> > > ARRAY_SIZE(ad4000_sdi_pin));
> > > +	if (ret < 0 && ret != -EINVAL)
> > > +		return dev_err_probe(dev, ret,
> > > +				     "getting adi,sdi-pin property
> > > failed\n");
> > > +
> > > +	/* Default to usual SPI connections if pin properties are not
> > > present
> > > */
> > > +	st->sdi_pin = ret == -EINVAL ? AD4000_SDI_MOSI : ret;
> > > +	switch (st->sdi_pin) {
> > > +	case AD4000_SDI_MOSI:
> > > +		indio_dev->info = &ad4000_reg_access_info;
> > > +		indio_dev->channels = &chip->reg_access_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_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(dev, "Failed to config device\n");
> > > +
> > 
> > Should this be a warning? Very suspicious :)
> 
> This devices have some many possible wiring configurations.
> I didn't want to fail just because reg access fail.
> Maybe ADC SDI was wired to VIO but dt don't have adi,sdi-pin = "high".
> Reg access will fail but sample read should work.

Well, to me that really is a configuration failure and we should treat it as
such. If we are in the so called "reg_access_info" which I read as "we can
access registers", failing to do so should be treated as an error. 

So, setting scale would also fail and we then have a broken interface :)

- Nuno Sá
> 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 1/7] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration
  2024-06-25 21:53 ` [PATCH v5 1/7] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration Marcelo Schmitt
  2024-06-26  9:37   ` Nuno Sá
@ 2024-06-26 14:57   ` David Lechner
  2024-06-26 15:08     ` Mark Brown
  1 sibling, 1 reply; 33+ messages in thread
From: David Lechner @ 2024-06-26 14:57 UTC (permalink / raw)
  To: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 6/25/24 4:53 PM, Marcelo Schmitt wrote:
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index e8e1e798924f..8e50a8559225 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -599,6 +599,12 @@ struct spi_controller {
>  	 * assert/de-assert more than one chip select at once.
>  	 */
>  #define SPI_CONTROLLER_MULTI_CS		BIT(7)
> +	/*
> +	 * The spi-controller is capable of keeping the MOSI line low or high
> +	 * when not clocking out data.
> +	 */
> +#define SPI_CONTROLLER_MOSI_IDLE_LOW    BIT(8)  /* Can idle MOSI low */
> +#define SPI_CONTROLLER_MOSI_IDLE_HIGH   BIT(9)  /* Can idle MOSI high */

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

>  
>  	/* Flag indicating if the allocation of this struct is devres-managed */
>  	bool			devm_allocated;
> diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h
> index ca56e477d161..ee4ac812b8f8 100644
> --- a/include/uapi/linux/spi/spi.h
> +++ b/include/uapi/linux/spi/spi.h
> @@ -28,7 +28,8 @@
>  #define	SPI_RX_OCTAL		_BITUL(14)	/* receive with 8 wires */
>  #define	SPI_3WIRE_HIZ		_BITUL(15)	/* high impedance turnaround */
>  #define	SPI_RX_CPHA_FLIP	_BITUL(16)	/* flip CPHA on Rx only xfer */
> -#define SPI_MOSI_IDLE_LOW	_BITUL(17)	/* leave mosi line low when idle */
> +#define SPI_MOSI_IDLE_LOW	_BITUL(17)	/* leave MOSI line low when idle */
> +#define SPI_MOSI_IDLE_HIGH	_BITUL(18)	/* leave MOSI line high when idle */

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

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

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

>  
>  /*
>   * All the bits defined above should be covered by SPI_MODE_USER_MASK.
> @@ -38,6 +39,6 @@
>   * These bits must not overlap. A static assert check should make sure of that.
>   * If adding extra bits, make sure to increase the bit index below as well.
>   */
> -#define SPI_MODE_USER_MASK	(_BITUL(18) - 1)
> +#define SPI_MODE_USER_MASK	(_BITUL(19) - 1)
>  
>  #endif /* _UAPI_SPI_H */


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 2/7] spi: bitbang: Implement support for MOSI idle state configuration
  2024-06-25 21:53 ` [PATCH v5 2/7] spi: bitbang: Implement support for MOSI idle state configuration Marcelo Schmitt
@ 2024-06-26 15:01   ` David Lechner
  0 siblings, 0 replies; 33+ messages in thread
From: David Lechner @ 2024-06-26 15:01 UTC (permalink / raw)
  To: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 6/25/24 4:53 PM, Marcelo Schmitt wrote:
> 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>
> ---

Reviewed-by: David Lechner <dlechner@baylibre.com>


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 3/7] spi: spi-gpio: Add support for MOSI idle state configuration
  2024-06-25 21:54 ` [PATCH v5 3/7] spi: spi-gpio: Add " Marcelo Schmitt
@ 2024-06-26 15:02   ` David Lechner
  0 siblings, 0 replies; 33+ messages in thread
From: David Lechner @ 2024-06-26 15:02 UTC (permalink / raw)
  To: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 6/25/24 4:54 PM, Marcelo Schmitt wrote:
> 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>
> ---

Reviewed-by: David Lechner <dlechner@baylibre.com>



^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 4/7] spi: spi-axi-spi-engine: Add support for MOSI idle configuration
  2024-06-25 21:54 ` [PATCH v5 4/7] spi: spi-axi-spi-engine: Add support for MOSI idle configuration Marcelo Schmitt
  2024-06-26  6:14   ` Alexandru Ardelean
  2024-06-26  9:56   ` Nuno Sá
@ 2024-06-26 15:06   ` David Lechner
  2 siblings, 0 replies; 33+ messages in thread
From: David Lechner @ 2024-06-26 15:06 UTC (permalink / raw)
  To: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 6/25/24 4:54 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..5a88d31ca758 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_HIGH		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_HIGH;
> +	if (spi->mode & SPI_MOSI_IDLE_LOW)
> +		config &= ~SPI_ENGINE_CONFIG_SDO_IDLE_HIGH;
>  
>  	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;

The driver already has a section:

	/* Some features depend of the IP core version. */
	if (ADI_AXI_PCORE_VER_MINOR(version) >= 2) {
		host->mode_bits |= SPI_CS_HIGH;
		host->setup = spi_engine_setup;
	}

So I would prefer to add the version check there instead.

With that change:

Reviewed-by: David Lechner <dlechner@baylibre.com>


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 1/7] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration
  2024-06-26 14:57   ` David Lechner
@ 2024-06-26 15:08     ` Mark Brown
  2024-06-27 17:41       ` Marcelo Schmitt
  0 siblings, 1 reply; 33+ messages in thread
From: Mark Brown @ 2024-06-26 15:08 UTC (permalink / raw)
  To: David Lechner
  Cc: Marcelo Schmitt, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, corbet,
	marcelo.schmitt1, linux-iio, devicetree, linux-spi, linux-doc,
	linux-kernel

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

On Wed, Jun 26, 2024 at 09:57:32AM -0500, David Lechner wrote:
> On 6/25/24 4:53 PM, Marcelo Schmitt wrote:

> > +#define SPI_CONTROLLER_MOSI_IDLE_LOW    BIT(8)  /* Can idle MOSI low */
> > +#define SPI_CONTROLLER_MOSI_IDLE_HIGH   BIT(9)  /* Can idle MOSI high */

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

Yes.

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

I don't think we need a specific plan there, obviously it'd be nice for
people to go through and enable but it's also fine to just leave this
for someone who needs the support to implement.

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 7/7] docs: iio: Add documentation for AD4000
  2024-06-25 21:55 ` [PATCH v5 7/7] docs: iio: Add documentation " Marcelo Schmitt
@ 2024-06-26 15:43   ` David Lechner
  2024-06-28 14:40     ` Marcelo Schmitt
  0 siblings, 1 reply; 33+ messages in thread
From: David Lechner @ 2024-06-26 15:43 UTC (permalink / raw)
  To: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 6/25/24 4:55 PM, Marcelo Schmitt wrote:
> Document wiring configurations for the AD4000 series of ADCs.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
>  Documentation/iio/ad4000.rst | 131 +++++++++++++++++++++++++++++++++++
>  Documentation/iio/index.rst  |   1 +
>  MAINTAINERS                  |   1 +
>  3 files changed, 133 insertions(+)
>  create mode 100644 Documentation/iio/ad4000.rst
> 
> diff --git a/Documentation/iio/ad4000.rst b/Documentation/iio/ad4000.rst
> new file mode 100644
> index 000000000000..de8fd3ae6e62
> --- /dev/null
> +++ b/Documentation/iio/ad4000.rst
> @@ -0,0 +1,131 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +=============
> +AD4000 driver
> +=============
> +
> +Device driver for Analog Devices Inc. AD4000 series of ADCs.
> +
> +Supported devices
> +=================
> +
> +* `AD4000 <https://www.analog.com/AD4000>`_
> +* `AD4001 <https://www.analog.com/AD4001>`_
> +* `AD4002 <https://www.analog.com/AD4002>`_
> +* `AD4003 <https://www.analog.com/AD4003>`_
> +* `AD4004 <https://www.analog.com/AD4004>`_
> +* `AD4005 <https://www.analog.com/AD4005>`_
> +* `AD4006 <https://www.analog.com/AD4006>`_
> +* `AD4007 <https://www.analog.com/AD4007>`_
> +* `AD4008 <https://www.analog.com/AD4008>`_
> +* `AD4010 <https://www.analog.com/AD4010>`_
> +* `AD4011 <https://www.analog.com/AD4011>`_
> +* `AD4020 <https://www.analog.com/AD4020>`_
> +* `AD4021 <https://www.analog.com/AD4021>`_
> +* `AD4022 <https://www.analog.com/AD4022>`_
> +* `ADAQ4001 <https://www.analog.com/ADAQ4001>`_
> +* `ADAQ4003 <https://www.analog.com/ADAQ4003>`_
> +
> +Wiring connections
> +------------------
> +
> +Devices of the AD4000 series can be connected to the SPI host controller in a
> +few different modes.
> +
> +CS mode, 3-wire turbo mode
> +^^^^^^^^^^^^^^^^^^^^^^^^^^

The datasheet also has the same diagram in _Figure 55. CS Mode, 4-Wire Turbo Mode
Connection Diagram_. So maybe we should call this "register support mode" or
something like that instead of mentioning 3 or 4-wire?

> +
> +Datasheet "3-wire" mode is what most resembles standard SPI connection which,
> +for these devices, comprises of connecting the controller CS line to device CNV
> +pin and other SPI lines as usual. This configuration is (misleadingly) called
> +"CS Mode, 3-Wire Turbo Mode" connection in datasheets.
> +NOTE: The datasheet definition of 3-wire mode for the AD4000 series is NOT the
> +same of standard spi-3wire mode.
> +This is the only connection mode that allows configuration register access but
> +it requires the SPI controller to support the ``SPI_MOSI_IDLE_HIGH`` feature.
> +
> +Omit the ``adi,sdi-pin`` property in device tree to select this mode.
> +
> +::
> +
> +                                         +-------------+
> +     + ----------------------------------| SDO         |
> +     |                                   |             |
> +     |               +-------------------| CS          |
> +     |               v                   |             |
> +     |    +--------------------+         |     HOST    |
> +     |    |        CNV         |         |             |
> +     +--->| SDI   AD4000   SDO |-------->| SDI         |
> +          |        SCK         |         |             |
> +          +--------------------+         |             |
> +                    ^                    |             |
> +                    +--------------------| SCLK        |
> +                                         +-------------+
> +

I think the rest of the explanations are good.


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 5/7] dt-bindings: iio: adc: Add AD4000
  2024-06-26 13:34     ` Marcelo Schmitt
@ 2024-06-26 15:55       ` Conor Dooley
  0 siblings, 0 replies; 33+ messages in thread
From: Conor Dooley @ 2024-06-26 15:55 UTC (permalink / raw)
  To: Marcelo Schmitt
  Cc: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

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

On Wed, Jun 26, 2024 at 10:34:24AM -0300, Marcelo Schmitt wrote:
> On 06/26, Conor Dooley wrote:
> > On Tue, Jun 25, 2024 at 06:55:03PM -0300, Marcelo Schmitt wrote:
> > > Add device tree documentation for AD4000 series of ADC devices.
> > > 
> > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > > ---
> > >  .../bindings/iio/adc/adi,ad4000.yaml          | 190 ++++++++++++++++++
> > >  MAINTAINERS                                   |   7 +
> ...
> > > +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
> > 
> > > +      - const: adi,adaq4001
> > 
> > > +      - const: adi,adaq4003
> > 
> > I think some blank lines, maybe like the above, would go a long way with
> > this list of compatibles.
> > 
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  spi-max-frequency:
> > > +    maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
> > > +
> > > +  adi,sdi-pin:
> > > +    $ref: /schemas/types.yaml#/definitions/string
> > > +    enum: [ high, low, cs ]
> > 
> >     enum: [ high, low, cs, sdi ]
> >     default: sdi
> > 
> > I'd do this, so that the default is documented in the binding, not in
> > the description text.
> > 
> > Otherwise, this looks good to me.
> 
> Ack, will do.

With those,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 6/7] iio: adc: Add support for AD4000
  2024-06-25 21:55 ` [PATCH v5 6/7] iio: adc: Add support for AD4000 Marcelo Schmitt
  2024-06-26  6:11   ` Alexandru Ardelean
  2024-06-26 11:04   ` Nuno Sá
@ 2024-06-26 16:56   ` David Lechner
  2024-06-27 23:34     ` Marcelo Schmitt
  2024-06-29 18:05   ` Jonathan Cameron
  3 siblings, 1 reply; 33+ messages in thread
From: David Lechner @ 2024-06-26 16:56 UTC (permalink / raw)
  To: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, corbet,
	marcelo.schmitt1
  Cc: linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 6/25/24 4:55 PM, Marcelo Schmitt wrote:

> +
> +enum ad4000_sdi {
> +	/* datasheet calls this "4-wire mode" (controller CS goes to ADC SDI!) */

It looks like this comment was meant for AD4000_SDI_CS.

> +	AD4000_SDI_MOSI,
> +	/* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
> +	AD4000_SDI_VIO,
> +	AD4000_SDI_CS,
> +};
> +
> +/* maps adi,sdi-pin property value to enum */
> +static const char * const ad4000_sdi_pin[] = {
> +	[AD4000_SDI_MOSI] = "",
> +	[AD4000_SDI_VIO] = "high",
> +	[AD4000_SDI_CS] = "cs",
> +};

Should we go ahead and add "low" here too even though it isn't supported
yet? We could give a different error message in this case. (not supported
mode vs. invalid value).

> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "3-wire" mode, selected when the adi,sdi-pin device tree property is
> + * absent or set to "high". 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.

This description doesn't sound quite correct. When st->turbo_mode is true
the shorter delay will cause a read during conversion, so we would be 
reading the sample from the previous conversion trigger, not the current one.

The description sounds like this function always does a read during
aquisition. So if that is the actual intent (and I agree it should be),
maybe the best thing to do would be to just remove st->turbo_mode for
now? Then we can add it back when we do SPI offload support that actually
needs it to achieve max sample rate. Then the function will match the
description as-is.

st->turbo_mode is never set to true currently anyway. So removing it
for now seems best.

> + */
> +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;
> +
> +	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);
> +
> +	return devm_spi_optimize_message(st->spi, &st->msg);

In the cover letter or after --- in this patch we should mention the
dependency since this is a new API and depends on the tag from Mark.

> +}
> +

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 6/7] iio: adc: Add support for AD4000
  2024-06-26 13:45       ` Nuno Sá
@ 2024-06-27 17:09         ` Marcelo Schmitt
  0 siblings, 0 replies; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-27 17:09 UTC (permalink / raw)
  To: Nuno Sá
  Cc: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 06/26, Nuno Sá wrote:
> On Wed, 2024-06-26 at 10:17 -0300, Marcelo Schmitt wrote:
> > On 06/26, Nuno Sá wrote:
> > > On Tue, 2024-06-25 at 18:55 -0300, Marcelo Schmitt wrote:
> > > > Add support for AD4000 series of low noise, low power, high speed,
> > > > successive approximation register (SAR) ADCs.
> > > > 
> > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > > > ---
...
> > > > +	ret = devm_regulator_get_enable(dev, "vdd");
> > > > +	if (ret)
> > > > +		return dev_err_probe(dev, ret, "Failed to enable VDD
> > > > supply\n");
> > > > +
> > > > +	ret = devm_regulator_get_enable(dev, "vio");
> > > > +	if (ret)
> > > > +		return dev_err_probe(dev, ret, "Failed to enable VIO
> > > > supply\n");
> > > 
> > > devm_regulator_bulk_get_enable()? Do we have any ordering constrains?
> > 
> > No ordering constraints, but vdd and vio are optional while ref is required
> > and
> > we need to get the voltage of ref.
> > devm_regulator_bulk_get_enable_read_voltage()? and discard vdd and vio
> > voltages?
> 
> Hmmm, vdd and vio do not look like optional to me :). Anyways I meant
> devm_regulator_bulk_get_enable() only for vdd and vio and still treat ref
> separately.
> 

I've mistaken these supplies with supplies for a different device.
Yes, vdd and vio are required and devm_regulator_bulk_get_enable() is useful
to init them.

> > 
> > > 
> > > > +
...
> > > > +		/*
> > > > +		 * 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_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(dev, "Failed to config device\n");
> > > > +
> > > 
> > > Should this be a warning? Very suspicious :)
> > 
> > This devices have some many possible wiring configurations.
> > I didn't want to fail just because reg access fail.
> > Maybe ADC SDI was wired to VIO but dt don't have adi,sdi-pin = "high".
> > Reg access will fail but sample read should work.
> 
> Well, to me that really is a configuration failure and we should treat it as
> such. If we are in the so called "reg_access_info" which I read as "we can
> access registers", failing to do so should be treated as an error. 
> 
> So, setting scale would also fail and we then have a broken interface :)

Drat, that's right. 
Okay, will make probe fail if config fails.

Thanks,
Marcelo

> 
> - Nuno Sá
> > 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 1/7] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration
  2024-06-26 15:08     ` Mark Brown
@ 2024-06-27 17:41       ` Marcelo Schmitt
  0 siblings, 0 replies; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-27 17:41 UTC (permalink / raw)
  To: Mark Brown
  Cc: David Lechner, Marcelo Schmitt, lars, Michael.Hennerich, jic23,
	robh+dt, krzysztof.kozlowski+dt, conor+dt, nuno.sa, corbet,
	linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 06/26, Mark Brown wrote:
> On Wed, Jun 26, 2024 at 09:57:32AM -0500, David Lechner wrote:
> > On 6/25/24 4:53 PM, Marcelo Schmitt wrote:
> 
> > > +#define SPI_CONTROLLER_MOSI_IDLE_LOW    BIT(8)  /* Can idle MOSI low */
> > > +#define SPI_CONTROLLER_MOSI_IDLE_HIGH   BIT(9)  /* Can idle MOSI high */
> 
> > These two flags above are still not used anywhere and are redundant with
> > the SPI_MOSI_IDLE_LOW/HIGH flags below so I don't think we should be adding
> > these.
> 
> Yes.

Oops, my bad. Removed them now for v6.

> 
> > Also, what is the plan for adding these flags to other SPI controllers. For
> > example, the IMX controller in [1] sounds like it should also support 
> > SPI_MOSI_IDLE_HIGH. And your comments on an earlier version of this series
> > made it sound like Raspberry Pi is always SPI_MOSI_IDLE_LOW, so should
> > have that flag.
> 
> I don't think we need a specific plan there, obviously it'd be nice for
> people to go through and enable but it's also fine to just leave this
> for someone who needs the support to implement.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 6/7] iio: adc: Add support for AD4000
  2024-06-26 16:56   ` David Lechner
@ 2024-06-27 23:34     ` Marcelo Schmitt
  0 siblings, 0 replies; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-27 23:34 UTC (permalink / raw)
  To: David Lechner
  Cc: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, corbet, linux-iio,
	devicetree, linux-spi, linux-doc, linux-kernel

On 06/26, David Lechner wrote:
> On 6/25/24 4:55 PM, Marcelo Schmitt wrote:
> 
> > +
> > +enum ad4000_sdi {
> > +	/* datasheet calls this "4-wire mode" (controller CS goes to ADC SDI!) */
> 
> It looks like this comment was meant for AD4000_SDI_CS.
> 

Yes, but I'm thinking maybe this is not the best place to have a comment about
that at all. I'm removing this comment for v6. Maybe better to have it well
documented in dt and close to the transfer functions than having partial
explanation here.

> > +	AD4000_SDI_MOSI,
> > +	/* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
removing this comment too
> > +	AD4000_SDI_VIO,
> > +	AD4000_SDI_CS,
> > +};
> > +
> > +/* maps adi,sdi-pin property value to enum */
> > +static const char * const ad4000_sdi_pin[] = {
> > +	[AD4000_SDI_MOSI] = "",
> > +	[AD4000_SDI_VIO] = "high",
> > +	[AD4000_SDI_CS] = "cs",
> > +};
> 
> Should we go ahead and add "low" here too even though it isn't supported
> yet? We could give a different error message in this case. (not supported
> mode vs. invalid value).
> 
Okay.
I have added for v6:
	case AD4000_SDI_GND:
		return dev_err_probe(dev, -EPROTONOSUPPORT,
				     "Unsupported connection mode\n");

> > +/*
> > + * This executes a data sample transfer for when the device connections are
> > + * in "3-wire" mode, selected when the adi,sdi-pin device tree property is
> > + * absent or set to "high". 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.
> 
> This description doesn't sound quite correct. When st->turbo_mode is true
> the shorter delay will cause a read during conversion, so we would be 
> reading the sample from the previous conversion trigger, not the current one.

If I'm correctly understanding the datasheet diagrams for 3-wire mode,
we should be reading the conversion triggered by the rising CS edge of the
preparatory/dummy transfer (that should happen when the dummy transfer finishes). 
So this is actually doing two samples. One sample gets triggered when the
dummy transfer ends and the other one is triggered when xfers[1] completes (this
second data sample is wasted because we are doing the extra dummy transfer to
avoid having data that might have been sampled long ago and to keep timestamps
close to the actual sampling time). 

> 
> The description sounds like this function always does a read during
> aquisition. So if that is the actual intent (and I agree it should be),
> maybe the best thing to do would be to just remove st->turbo_mode for
> now? Then we can add it back when we do SPI offload support that actually
> needs it to achieve max sample rate. Then the function will match the
> description as-is.
> 
> st->turbo_mode is never set to true currently anyway. So removing it
> for now seems best.

I always thought we should not add to the kernel code that does nothing, even if
it might do something in the future. For example, when adding new drivers, I
think it is preferred only to add defines for registers that are used, in
contrast to adding defines for all registers a chip has. So, yeah, removed
turbo_mode for v6 and also some reg defines I noticed were unused too.

> 
> > + */
> > +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;
> > +
> > +	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);
> > +
> > +	return devm_spi_optimize_message(st->spi, &st->msg);
> 
> In the cover letter or after --- in this patch we should mention the
> dependency since this is a new API and depends on the tag from Mark.

I got
d4a0055fdc22381fa256e345095e88d134e354c5 "spi: add devm_spi_optimize_message() helper"
7e74a45c7afdd8a9f82d14fd79ae0383bbaaed1e "spi: add EXPORT_SYMBOL_GPL(devm_spi_optimize_message)"
6ecdb0aa4dca62d236a659426e11e6cf302e8f18 "spi: axi-spi-engine: Add SPI_CS_HIGH support"
from https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/log/?h=for-6.11
to apply and test the changes for v6.
Will mention those in the cover letter.

Thanks,
Marcelo

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 7/7] docs: iio: Add documentation for AD4000
  2024-06-26 15:43   ` David Lechner
@ 2024-06-28 14:40     ` Marcelo Schmitt
  0 siblings, 0 replies; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-28 14:40 UTC (permalink / raw)
  To: David Lechner
  Cc: Marcelo Schmitt, broonie, lars, Michael.Hennerich, jic23, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, corbet, linux-iio,
	devicetree, linux-spi, linux-doc, linux-kernel

On 06/26, David Lechner wrote:
> On 6/25/24 4:55 PM, Marcelo Schmitt wrote:
> > Document wiring configurations for the AD4000 series of ADCs.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > ---
> >  Documentation/iio/ad4000.rst | 131 +++++++++++++++++++++++++++++++++++
> >  Documentation/iio/index.rst  |   1 +
> >  MAINTAINERS                  |   1 +
...
> > +Wiring connections
> > +------------------
> > +
> > +Devices of the AD4000 series can be connected to the SPI host controller in a
> > +few different modes.
> > +
> > +CS mode, 3-wire turbo mode
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> The datasheet also has the same diagram in _Figure 55. CS Mode, 4-Wire Turbo Mode
> Connection Diagram_. So maybe we should call this "register support mode" or
> something like that instead of mentioning 3 or 4-wire?
> 
Humm, ADAQ4003 datasheet has the same figure for CS Mode, 4-wire turbo mode
and CS mode, 4-wire with busy indicator although other datasheets have MOSI
to SDI in 4-wire turbo as you said. The register read/write functionality
sections say CNV must be brought low to access the configuration register.
I'll leave it as it is for now as I have not tested reg access with CNV high.
We can update this in the future if find out reg access to work in 4-wire mode.

> > +
> > +Datasheet "3-wire" mode is what most resembles standard SPI connection which,
> > +for these devices, comprises of connecting the controller CS line to device CNV
> > +pin and other SPI lines as usual. This configuration is (misleadingly) called
> > +"CS Mode, 3-Wire Turbo Mode" connection in datasheets.
> > +NOTE: The datasheet definition of 3-wire mode for the AD4000 series is NOT the
> > +same of standard spi-3wire mode.
> > +This is the only connection mode that allows configuration register access but
> > +it requires the SPI controller to support the ``SPI_MOSI_IDLE_HIGH`` feature.
> > +
> > +Omit the ``adi,sdi-pin`` property in device tree to select this mode.
> > +
> > +::
> > +
> > +                                         +-------------+
> > +     + ----------------------------------| SDO         |
> > +     |                                   |             |
> > +     |               +-------------------| CS          |
> > +     |               v                   |             |
> > +     |    +--------------------+         |     HOST    |
> > +     |    |        CNV         |         |             |
> > +     +--->| SDI   AD4000   SDO |-------->| SDI         |
> > +          |        SCK         |         |             |
> > +          +--------------------+         |             |
> > +                    ^                    |             |
> > +                    +--------------------| SCLK        |
> > +                                         +-------------+
> > +
> 
> I think the rest of the explanations are good.
> 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 6/7] iio: adc: Add support for AD4000
  2024-06-25 21:55 ` [PATCH v5 6/7] iio: adc: Add support for AD4000 Marcelo Schmitt
                     ` (2 preceding siblings ...)
  2024-06-26 16:56   ` David Lechner
@ 2024-06-29 18:05   ` Jonathan Cameron
  2024-06-29 18:13     ` Marcelo Schmitt
  3 siblings, 1 reply; 33+ messages in thread
From: Jonathan Cameron @ 2024-06-29 18:05 UTC (permalink / raw)
  To: Marcelo Schmitt
  Cc: broonie, lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, nuno.sa, dlechner, corbet, marcelo.schmitt1, linux-iio,
	devicetree, linux-spi, linux-doc, linux-kernel

On Tue, 25 Jun 2024 18:55:27 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

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

You've clearly gotten some good review for this version so I only
had a quick scan through.  One thing did jump out at me though.

> +
> +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:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret < 0)
> +			return ret;
> +
> +		mutex_lock(&st->lock);
> +		ret = ad4000_read_reg(st, &reg_val);
> +		if (ret < 0)
> +			goto err_unlock;
> +
> +		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)
> +			goto err_unlock;
> +
> +		st->span_comp = span_comp_en;
> +err_unlock:
> +		iio_device_release_direct_mode(indio_dev);
> +		mutex_unlock(&st->lock);

Lock ordering needs another look. I'm not sure we an trigger
a deadlock but it definitely looks problematic. 

J



^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH v5 6/7] iio: adc: Add support for AD4000
  2024-06-29 18:05   ` Jonathan Cameron
@ 2024-06-29 18:13     ` Marcelo Schmitt
  0 siblings, 0 replies; 33+ messages in thread
From: Marcelo Schmitt @ 2024-06-29 18:13 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Marcelo Schmitt, broonie, lars, Michael.Hennerich, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, nuno.sa, dlechner, corbet,
	linux-iio, devicetree, linux-spi, linux-doc, linux-kernel

On 06/29, Jonathan Cameron wrote:
> On Tue, 25 Jun 2024 18:55:27 -0300
> Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:
> 
> > Add support for AD4000 series of low noise, low power, high speed,
> > successive approximation register (SAR) ADCs.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> Hi Marcelo,
> 
> You've clearly gotten some good review for this version so I only
> had a quick scan through.  One thing did jump out at me though.
> 
> > +
> > +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:
> > +		ret = iio_device_claim_direct_mode(indio_dev);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		mutex_lock(&st->lock);
> > +		ret = ad4000_read_reg(st, &reg_val);
> > +		if (ret < 0)
> > +			goto err_unlock;
> > +
> > +		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)
> > +			goto err_unlock;
> > +
> > +		st->span_comp = span_comp_en;
> > +err_unlock:
> > +		iio_device_release_direct_mode(indio_dev);
> > +		mutex_unlock(&st->lock);
> 
> Lock ordering needs another look. I'm not sure we an trigger
> a deadlock but it definitely looks problematic. 

Oops. Oh, that's inddeed back lock release ordering.
I've changed to scoped and guard for v6 and will send the updated version soon.

Anyway, thanks for having a look at it.
Marcelo

> 
> J
> 
> 

^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2024-06-29 18:11 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-25 21:52 [PATCH v5 0/7] Add support for AD4000 series of ADCs Marcelo Schmitt
2024-06-25 21:53 ` [PATCH v5 1/7] spi: Enable controllers to extend the SPI protocol with MOSI idle configuration Marcelo Schmitt
2024-06-26  9:37   ` Nuno Sá
2024-06-26 14:57   ` David Lechner
2024-06-26 15:08     ` Mark Brown
2024-06-27 17:41       ` Marcelo Schmitt
2024-06-25 21:53 ` [PATCH v5 2/7] spi: bitbang: Implement support for MOSI idle state configuration Marcelo Schmitt
2024-06-26 15:01   ` David Lechner
2024-06-25 21:54 ` [PATCH v5 3/7] spi: spi-gpio: Add " Marcelo Schmitt
2024-06-26 15:02   ` David Lechner
2024-06-25 21:54 ` [PATCH v5 4/7] spi: spi-axi-spi-engine: Add support for MOSI idle configuration Marcelo Schmitt
2024-06-26  6:14   ` Alexandru Ardelean
2024-06-26 13:27     ` Marcelo Schmitt
2024-06-26  9:56   ` Nuno Sá
2024-06-26 15:06   ` David Lechner
2024-06-25 21:55 ` [PATCH v5 5/7] dt-bindings: iio: adc: Add AD4000 Marcelo Schmitt
2024-06-26 11:34   ` Conor Dooley
2024-06-26 13:34     ` Marcelo Schmitt
2024-06-26 15:55       ` Conor Dooley
2024-06-25 21:55 ` [PATCH v5 6/7] iio: adc: Add support for AD4000 Marcelo Schmitt
2024-06-26  6:11   ` Alexandru Ardelean
2024-06-26 13:25     ` Marcelo Schmitt
2024-06-26 11:04   ` Nuno Sá
2024-06-26 13:17     ` Marcelo Schmitt
2024-06-26 13:45       ` Nuno Sá
2024-06-27 17:09         ` Marcelo Schmitt
2024-06-26 16:56   ` David Lechner
2024-06-27 23:34     ` Marcelo Schmitt
2024-06-29 18:05   ` Jonathan Cameron
2024-06-29 18:13     ` Marcelo Schmitt
2024-06-25 21:55 ` [PATCH v5 7/7] docs: iio: Add documentation " Marcelo Schmitt
2024-06-26 15:43   ` David Lechner
2024-06-28 14:40     ` Marcelo Schmitt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).