[PATCH 0/6] iio: adc: ad4030: new driver for AD4030 and similar ADCs

[PATCH 0/6] iio: adc: ad4030: new driver for AD4030 and similar ADCs

[Esteban Blanc]

This is adding DT bindings and a new driver for AD4030, AD4630 and
AD4632 ADCs.

This work is being done in collaboration with Analog Devices Inc.,
hence they are listed as maintainers rather than me.

The code has been tested on a Zedboard with an EVAL-AD4030-24FMCZ,
an EVAL-AD4630-24FMCZ and an EVAL-AD4630-16FMCZ. As there is no eval
board for AD4632 the support can't be tested at the moment. The main
difference is the reduced throughput.

To: Lars-Peter Clausen <lars@metafoo.de>
To: Michael Hennerich <Michael.Hennerich@analog.com>
To: Jonathan Cameron <jic23@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Nuno Sa <nuno.sa@analog.com>
To: Jonathan Corbet <corbet@lwn.net>
Cc: Michael Hennerich <michael.hennerich@analog.com>
Cc: linux-iio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: David Lechner <dlechner@baylibre.com>
Cc: linux-doc@vger.kernel.org
Signed-off-by: Esteban Blanc <eblanc@baylibre.com>

Changes since RFC:
- Reorder IIO channels to have the common byte channel next to its differential
  channel.
- Extended names for IIO channels.
- Diffrential data channels are marked as differential channels on IIO
  side.
- Use get/put_unaligned_be24 for offset and sign extend it.
- Common byte channel now has 32 realbits. This will be the same as what the
  FPGA will return, avoiding different channel layouts.
- Fix missing newline in some error messages.
- Add comment for the use of spi_sync_transfer instead of
  spi_write_then_read in ad4030_spi_read.
- Use DMA safe buffers for regmap operations.
- Clarify calculation for number of bytes to read from the device during
  conversion.
- Formating fixes.
- Add documentation page.
- Link to RFC: https://lore.kernel.org/r/20240627-eblanc-ad4630_v1-v1-0-fdc0610c23b0@baylibre.com

---
Esteban Blanc (6):
      dt-bindings: iio: adc: add ADI ad4030, ad4630 and ad4632
      iio: adc: ad4030: add driver for ad4030-24
      iio: adc: ad4030: add averaging support
      iio: adc: ad4030: add support for ad4630-24 and ad4630-16
      iio: adc: ad4030: add support for ad4632-16 and ad4632-24
      docs: iio: ad4030: add documentation

 .../devicetree/bindings/iio/adc/adi,ad4030.yaml    |  113 ++
 Documentation/iio/ad4030.rst                       |  129 +++
 Documentation/iio/index.rst                        |    1 +
 MAINTAINERS                                        |   10 +
 drivers/iio/adc/Kconfig                            |   13 +
 drivers/iio/adc/Makefile                           |    1 +
 drivers/iio/adc/ad4030.c                           | 1141 ++++++++++++++++++++
 7 files changed, 1408 insertions(+)
---
base-commit: 3545ef5c627c761c76bbdf71e4b4d423fe30e51b
change-id: 20240624-eblanc-ad4630_v1-1a074097eb91

Best regards,
-- 
Esteban Blanc <eblanc@baylibre.com>

[PATCH 1/6] dt-bindings: iio: adc: add ADI ad4030, ad4630 and ad4632

[Esteban Blanc]

This adds a binding specification for the Analog Devices Inc. AD4030,
AD4630 and AD4632 families of ADCs.

- ad4030-24 is a 1 channel SAR ADC with 24 bits of precision and a
  sampling rate of 2M samples per second
- ad4630-16 is a 2 channels SAR ADC with 16 bits of precision and a
  sampling rate of 2M samples per second
- ad4630-24 is a 2 channels SAR ADC with 24 bits of precision and a
  sampling rate of 2M samples per second
- ad4632-16 is a 2 channels SAR ADC with 16 bits of precision and a
  sampling rate of 500K samples per second
- ad4632-24 is a 2 channels SAR ADC with 24 bits of precision and a
  sampling rate of 500K samples per second

Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
---
 .../devicetree/bindings/iio/adc/adi,ad4030.yaml    | 113 +++++++++++++++++++++
 MAINTAINERS                                        |   8 ++
 2 files changed, 121 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
new file mode 100644
index 000000000000..7957c0c0ac7a
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
@@ -0,0 +1,113 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2024 Analog Devices Inc.
+# Copyright 2024 BayLibre, SAS.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4030.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4030 and AD4630 ADC family device driver
+
+maintainers:
+  - Nuno Sa <nuno.sa@analog.com>
+  - Michael Hennerich <michael.hennerich@analog.com>
+
+description: |
+  Analog Devices AD4030 single channel and AD4630 dual channel precision SAR ADC
+  family
+
+  * https://www.analog.com/media/en/technical-documentation/data-sheets/ad4030-24-4032-24.pdf
+  * https://www.analog.com/media/en/technical-documentation/data-sheets/ad4630-24_ad4632-24.pdf
+  * https://www.analog.com/media/en/technical-documentation/data-sheets/ad4630-16-4632-16.pdf
+
+properties:
+
+  compatible:
+    enum:
+      - adi,ad4030-24
+      - adi,ad4630-16
+      - adi,ad4630-24
+      - adi,ad4632-16
+      - adi,ad4632-24
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 100000000
+
+  spi-rx-bus-width:
+    enum: [1, 2, 4]
+
+  vdd-5v-supply: true
+  vdd-1v8-supply: true
+  vio-supply: true
+
+  ref-supply:
+    description:
+      Optional External unbuffered reference. Used when refin-supply is not
+      connected.
+
+  refin-supply:
+    description:
+      Internal buffered Reference. Used when ref-supply is not connected.
+
+  cnv-gpio:
+    description:
+      The Convert Input (CNV). It initiates the sampling conversions.
+    maxItems: 1
+
+  reset-gpio:
+    description:
+      Reset Input (Active Low). Used for asynchronous device reset.
+    maxItems: 1
+
+  interrupts:
+    description:
+      The BUSY pin is used to signal that the conversions results are available
+      to be transferred when in SPI Clocking Mode. This nodes should be connected
+      to an interrupt that is triggered when the BUSY line goes low.
+    maxItems: 1
+
+  interrupt-names:
+    const: busy
+
+required:
+  - compatible
+  - reg
+  - vdd-5v-supply
+  - vdd-1v8-supply
+  - vio-supply
+  - cnv-gpio
+
+oneOf:
+  - required:
+      - ref-supply
+  - required:
+      - refin-supply
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "adi,ad4030-24";
+            reg = <0>;
+            spi-max-frequency = <80000000>;
+            vdd-5v-supply = <&supply_5V>;
+            vdd-1v8-supply = <&supply_1_8V>;
+            vio-supply = <&supply_1_8V>;
+            ref-supply = <&supply_5V>;
+            cnv-gpio = <&gpio0 0 GPIO_ACTIVE_HIGH>;
+        };
+    };
+
diff --git a/MAINTAINERS b/MAINTAINERS
index 417c6751c0dc..f17c42bea19c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -413,6 +413,14 @@ S:	Maintained
 W:	https://parisc.wiki.kernel.org/index.php/AD1889
 F:	sound/pci/ad1889.*
 
+AD4030 ADC DRIVER (AD4030-24/AD4630-16/AD4630-24/AD4632-16/AD4632-24)
+M:	Michael Hennerich <michael.hennerich@analog.com>
+M:	Nuno Sá <nuno.sa@analog.com>
+R:	Esteban Blanc <eblanc@baylibre.com>
+S:	Supported
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
+
 AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER
 M:	Mugilraj Dhavachelvan <dmugil2000@gmail.com>
 L:	linux-iio@vger.kernel.org

-- 
2.44.1

[PATCH 2/6] iio: adc: ad4030: add driver for ad4030-24

[Esteban Blanc]

This adds a new driver for the Analog Devices INC. AD4030-24 ADC.

The driver implements basic support for the AD4030-24 1 channel
differential ADC with hardware gain and offset control.

Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
---
 MAINTAINERS              |   1 +
 drivers/iio/adc/Kconfig  |  13 +
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad4030.c | 854 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 869 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f17c42bea19c..6a5a0e7b7a51 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -420,6 +420,7 @@ R:	Esteban Blanc <eblanc@baylibre.com>
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
+F:	drivers/iio/adc/ad4030.c
 
 AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER
 M:	Mugilraj Dhavachelvan <dmugil2000@gmail.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 88e8ce2e78b3..f4bd05780f6f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -33,6 +33,19 @@ config AD4000
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad4000.
 
+config AD4030
+	tristate "Analog Device AD4630 ADC Driver"
+	depends on SPI
+	depends on GPIOLIB
+	select REGMAP_SPI
+	select IIO_BUFFER
+	help
+	  Say yes here to build support for Analog Devices AD4030 and AD4630 high speed
+	  SPI analog to digital converters (ADC).
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad4030.
+
 config AD4130
 	tristate "Analog Device AD4130 ADC Driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 8b80664c6d6b..0e4f833abf0b 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -7,6 +7,7 @@
 obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4000) += ad4000.o
+obj-$(CONFIG_AD4030) += ad4030.o
 obj-$(CONFIG_AD4130) += ad4130.o
 obj-$(CONFIG_AD4695) += ad4695.o
 obj-$(CONFIG_AD7091R) += ad7091r-base.o
diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
new file mode 100644
index 000000000000..a981dce988e5
--- /dev/null
+++ b/drivers/iio/adc/ad4030.c
@@ -0,0 +1,854 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD4030 and AD4630 ADC family driver.
+ *
+ * Copyright 2024 Analog Devices, Inc.
+ * Copyright 2024 BayLibre, SAS
+ *
+ * based on code from:
+ *	Analog Devices, Inc.
+ *	  Sergiu Cuciurean <sergiu.cuciurean@analog.com>
+ *	  Nuno Sa <nuno.sa@analog.com>
+ *	  Marcelo Schmitt <marcelo.schmitt@analog.com>
+ *	  Liviu Adace <liviu.adace@analog.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/units.h>
+#include <linux/clk.h>
+#include <linux/spi/spi.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define AD4030_REG_INTERFACE_CONFIG_A			0x00
+#define     AD4030_REG_INTERFACE_CONFIG_A_SW_RESET	(BIT(0) | BIT(7))
+#define AD4030_REG_INTERFACE_CONFIG_B			0x01
+#define AD4030_REG_DEVICE_CONFIG			0x02
+#define AD4030_REG_CHIP_TYPE				0x03
+#define AD4030_REG_PRODUCT_ID_L				0x04
+#define AD4030_REG_PRODUCT_ID_H				0x05
+#define AD4030_REG_CHIP_GRADE				0x06
+#define     AD4030_REG_CHIP_GRADE_AD4030_24_GRADE	0x10
+#define     AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE	GENMASK(7, 3)
+#define AD4030_REG_SCRATCH_PAD			0x0A
+#define AD4030_REG_SPI_REVISION			0x0B
+#define AD4030_REG_VENDOR_L			0x0C
+#define AD4030_REG_VENDOR_H			0x0D
+#define AD4030_REG_STREAM_MODE			0x0E
+#define AD4030_REG_INTERFACE_CONFIG_C		0x10
+#define AD4030_REG_INTERFACE_STATUS_A		0x11
+#define AD4030_REG_EXIT_CFG_MODE		0x14
+#define     AD4030_REG_EXIT_CFG_MODE_EXIT_MSK	BIT(0)
+#define AD4030_REG_AVG				0x15
+#define     AD4030_REG_AVG_MASK_AVG_SYNC	BIT(7)
+#define     AD4030_REG_AVG_MASK_AVG_VAL		GENMASK(4, 0)
+#define AD4030_REG_OFFSET_X0_0			0x16
+#define AD4030_REG_OFFSET_X0_1			0x17
+#define AD4030_REG_OFFSET_X0_2			0x18
+#define AD4030_REG_OFFSET_X1_0			0x19
+#define AD4030_REG_OFFSET_X1_1			0x1A
+#define AD4030_REG_OFFSET_X1_2			0x1B
+#define     AD4030_REG_OFFSET_BYTES_NB		3
+#define     AD4030_REG_OFFSET_CHAN(ch)		(AD4030_REG_OFFSET_X0_2 +	\
+						(AD4030_REG_OFFSET_BYTES_NB *	\
+						(ch)))
+#define AD4030_REG_GAIN_X0_LSB			0x1C
+#define AD4030_REG_GAIN_X0_MSB			0x1D
+#define AD4030_REG_GAIN_X1_LSB			0x1E
+#define AD4030_REG_GAIN_X1_MSB			0x1F
+#define     AD4030_REG_GAIN_MAX_GAIN		1999970
+#define     AD4030_REG_GAIN_BYTES_NB		2
+#define     AD4030_REG_GAIN_CHAN(ch)		(AD4030_REG_GAIN_X0_MSB +	\
+						(AD4030_REG_GAIN_BYTES_NB *	\
+						(ch)))
+#define AD4030_REG_MODES			0x20
+#define     AD4030_REG_MODES_MASK_OUT_DATA_MODE	GENMASK(2, 0)
+#define     AD4030_REG_MODES_MASK_LANE_MODE	GENMASK(7, 6)
+#define AD4030_REG_OSCILATOR			0x21
+#define AD4030_REG_IO				0x22
+#define     AD4030_REG_IO_MASK_IO2X		BIT(1)
+#define AD4030_REG_PAT0				0x23
+#define AD4030_REG_PAT1				0x24
+#define AD4030_REG_PAT2				0x25
+#define AD4030_REG_PAT3				0x26
+#define AD4030_REG_DIG_DIAG			0x34
+#define AD4030_REG_DIG_ERR			0x35
+
+/* Sequence starting with "1 0 1" to enable reg access */
+#define AD4030_REG_ACCESS			0xa0
+
+#define AD4030_MAX_IIO_SAMPLE_SIZE_BUFFERED	BITS_TO_BYTES(64)
+#define AD4030_MAX_HARDWARE_CHANNEL_NB		2
+#define AD4030_MAX_IIO_CHANNEL_NB		5
+#define AD4030_COMMON_BYTE_CHANNELS_FILTER	0xA
+/*
+ * This accounts for for 1 sample per channel plus one u64 for the timestamp,
+ * aligned on a u64 boundary
+ */
+#define AD4030_MAXIMUM_RX_BUFFER_SIZE			\
+	ALIGN(AD4030_MAX_IIO_SAMPLE_SIZE_BUFFERED *	\
+	      AD4030_MAX_HARDWARE_CHANNEL_NB,		\
+	      sizeof(u64)) + sizeof(u64)
+
+#define AD4030_VREF_MIN_UV		(4096 * MILLI)
+#define AD4030_VREF_MAX_UV		(5000 * MILLI)
+#define AD4030_VIO_THRESHOLD_UV		(1400 * MILLI)
+
+#define AD4030_SPI_MAX_XFER_LEN		8
+#define AD4030_SPI_MAX_REG_XFER_SPEED	(80 * MEGA)
+#define AD4030_TCNVH_NS			10
+#define AD4030_TCNVL_NS			20
+#define AD4030_TCYC_NS			500
+#define AD4030_TCYC_ADJUSTED_NS		(AD4030_TCYC_NS - AD4030_TCNVL_NS)
+#define AD4030_TRESET_PW_NS		50
+#define AD4030_TRESET_COM_DELAY_MS	750
+
+enum ad4030_out_mode {
+	AD4030_OUT_DATA_MD_16_DIFF = 0x00,
+	AD4030_OUT_DATA_MD_24_DIFF = 0x00,
+	AD4030_OUT_DATA_MD_16_DIFF_8_COM = 0x01,
+	AD4030_OUT_DATA_MD_24_DIFF_8_COM = 0x02,
+	AD4030_OUT_DATA_MD_30_AVERAGED_DIFF = 0x03,
+	AD4030_OUT_DATA_MD_32_PATTERN = 0x04
+};
+
+struct ad4030_chip_info {
+	const char *name;
+	const unsigned long *available_masks;
+	unsigned int available_masks_len;
+	const struct iio_chan_spec channels[AD4030_MAX_IIO_CHANNEL_NB];
+	u8 grade;
+	u8 precision_bits;
+	/* Number of hardware channels */
+	int num_channels;
+	unsigned int tcyc;
+};
+
+struct ad4030_state {
+	struct spi_device *spi;
+	struct regmap *regmap;
+	const struct ad4030_chip_info *chip;
+	struct gpio_desc *cnv_gpio;
+	int vref_uv;
+	int vio_uv;
+	int offset_avail[3];
+	u32 conversion_speed_hz;
+	enum ad4030_out_mode mode;
+
+	/*
+	 * DMA (thus cache coherency maintenance) requires the transfer buffers
+	 * to live in their own cache lines.
+	 */
+	u8 tx_data[AD4030_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
+	struct {
+		union {
+			u8 raw[AD4030_MAXIMUM_RX_BUFFER_SIZE];
+			struct {
+				s32 val;
+				u32 common;
+			} __packed buffered[AD4030_MAX_HARDWARE_CHANNEL_NB];
+		};
+	} rx_data __aligned(IIO_DMA_MINALIGN);
+};
+
+#define AD4030_CHAN_CMO(_idx)  {					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.type = IIO_VOLTAGE,						\
+	.indexed = 1,							\
+	.channel = _idx * 2 + 2,					\
+	.scan_index = _idx * 2 + 1,					\
+	.extend_name = "Channel" #_idx " common byte part",		\
+	.scan_type = {							\
+		.sign = 'u',						\
+		.storagebits = 32,					\
+		.realbits = 8,						\
+		.endianness = IIO_BE,					\
+	},								\
+}
+
+#define AD4030_CHAN_IN(_idx, _storage, _real, _shift) {			\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
+		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
+		BIT(IIO_CHAN_INFO_RAW),					\
+	.info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
+		BIT(IIO_CHAN_INFO_CALIBSCALE),				\
+	.type = IIO_VOLTAGE,						\
+	.indexed = 1,							\
+	.channel = _idx * 2,						\
+	.channel2 = _idx * 2 + 1,					\
+	.scan_index = _idx * 2,						\
+	.extend_name = "Channel" #_idx " differential part",		\
+	.differential = true,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.storagebits = _storage,				\
+		.realbits = _real,					\
+		.shift = _shift,					\
+		.endianness = IIO_BE,					\
+	},								\
+}
+
+static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
+			   void *val, size_t val_size)
+{
+	struct ad4030_state *st = context;
+
+	struct spi_transfer xfer = {
+		.tx_buf = st->tx_data,
+		.rx_buf = st->rx_data.raw,
+		.len = reg_size + val_size,
+	};
+	int ret;
+
+	memcpy(st->tx_data, reg, reg_size);
+
+	/*
+	 * This should use spi_write_the_read but when doing so, CS never get
+	 * deasserted.
+	 */
+	ret = spi_sync_transfer(st->spi, &xfer, 1);
+	if (ret)
+		return ret;
+
+	memcpy(val, &st->rx_data.raw[reg_size], val_size);
+
+	return ret;
+}
+
+static int ad4030_spi_write(void *context, const void *data, size_t count)
+{
+	const struct ad4030_state *st = context;
+
+	return spi_write(st->spi, data, count);
+}
+
+static const struct regmap_bus ad4030_regmap_bus = {
+	.read = ad4030_spi_read,
+	.write = ad4030_spi_write,
+	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
+};
+
+static const struct regmap_range ad4030_regmap_rd_range[] = {
+	regmap_reg_range(AD4030_REG_INTERFACE_CONFIG_A, AD4030_REG_CHIP_GRADE),
+	regmap_reg_range(AD4030_REG_SCRATCH_PAD, AD4030_REG_STREAM_MODE),
+	regmap_reg_range(AD4030_REG_INTERFACE_CONFIG_C, AD4030_REG_INTERFACE_STATUS_A),
+	regmap_reg_range(AD4030_REG_EXIT_CFG_MODE, AD4030_REG_PAT3),
+	regmap_reg_range(AD4030_REG_DIG_DIAG, AD4030_REG_DIG_ERR),
+};
+
+static const struct regmap_range ad4030_regmap_wr_range[] = {
+	regmap_reg_range(AD4030_REG_CHIP_TYPE, AD4030_REG_CHIP_GRADE),
+	regmap_reg_range(AD4030_REG_SPI_REVISION, AD4030_REG_VENDOR_H),
+};
+
+static const struct regmap_access_table ad4030_regmap_rd_table = {
+	.yes_ranges = ad4030_regmap_rd_range,
+	.n_yes_ranges = ARRAY_SIZE(ad4030_regmap_rd_range),
+};
+
+static const struct regmap_access_table ad4030_regmap_wr_table = {
+	.no_ranges = ad4030_regmap_wr_range,
+	.n_no_ranges = ARRAY_SIZE(ad4030_regmap_wr_range),
+};
+
+static const struct regmap_config ad4030_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+	.read_flag_mask = 0x80,
+	.rd_table = &ad4030_regmap_rd_table,
+	.wr_table = &ad4030_regmap_wr_table,
+	.max_register = AD4030_REG_DIG_ERR,
+};
+
+static int ad4030_enter_config_mode(struct ad4030_state *st)
+{
+	st->rx_data.raw[0] = AD4030_REG_ACCESS;
+
+	return spi_write(st->spi, st->rx_data.raw, 1);
+}
+
+static int ad4030_exit_config_mode(struct ad4030_state *st)
+{
+	return regmap_write(st->regmap, AD4030_REG_EXIT_CFG_MODE,
+		     AD4030_REG_EXIT_CFG_MODE_EXIT_MSK);
+}
+
+static int ad4030_get_chan_gain(struct iio_dev *indio_dev, int ch, int *val,
+				int *val2)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	u16 gain;
+	int ret;
+
+	ret = regmap_bulk_read(st->regmap, AD4030_REG_GAIN_CHAN(ch),
+			       st->rx_data.raw, AD4030_REG_GAIN_BYTES_NB);
+	if (ret)
+		return ret;
+
+	gain = be16_to_cpu(*(u16 *)st->rx_data.raw);
+
+	/* From datasheet: multiplied output = input × gain word/0x8000 */
+	*val = gain / 0x8000;
+	*val2 = mul_u64_u32_div(gain % 0x8000, MICRO, 0x8000);
+
+	return 0;
+}
+
+/*
+ * @brief Returns the offset where 1 LSB = (VREF/2^precision_bits - 1)/gain
+ */
+static int ad4030_get_chan_offset(struct iio_dev *indio_dev, int ch, int *val)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = regmap_bulk_read(st->regmap, AD4030_REG_OFFSET_CHAN(ch),
+			       st->rx_data.raw, AD4030_REG_OFFSET_BYTES_NB);
+	if (ret)
+		return ret;
+
+	switch (st->chip->precision_bits) {
+	case 16:
+		*val = sign_extend32(get_unaligned_be16(st->rx_data.raw), 15);
+		break;
+
+	case 24:
+		*val = sign_extend32(get_unaligned_be24(st->rx_data.raw), 23);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int gain_int,
+				int gain_frac)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	__be16 *val = (__be16 *)st->rx_data.raw;
+	u64 gain;
+
+	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
+
+	if (gain > AD4030_REG_GAIN_MAX_GAIN)
+		return -EINVAL;
+
+	*val = cpu_to_be16(DIV_ROUND_CLOSEST_ULL(gain * 0x8000, MICRO));
+
+	return regmap_bulk_write(st->regmap, AD4030_REG_GAIN_CHAN(ch), val,
+			  AD4030_REG_GAIN_BYTES_NB);
+}
+
+static int ad4030_set_chan_offset(struct iio_dev *indio_dev, int ch, int offset)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+
+	if (offset < st->offset_avail[0] || offset > st->offset_avail[2])
+		return -EINVAL;
+
+	switch (st->chip->precision_bits) {
+	case 16:
+		put_unaligned_be16(offset, st->rx_data.raw);
+		break;
+
+	case 24:
+		put_unaligned_be24(offset, st->rx_data.raw);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return regmap_bulk_write(st->regmap, AD4030_REG_OFFSET_CHAN(ch),
+			  st->rx_data.raw, AD4030_REG_OFFSET_BYTES_NB);
+}
+
+static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
+					unsigned int mask)
+{
+	/* Common byte channel is after the "real" differential sample channel */
+	return mask & AD4030_COMMON_BYTE_CHANNELS_FILTER;
+}
+
+static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+
+	if (ad4030_is_common_byte_asked(st, mask))
+		st->mode = AD4030_OUT_DATA_MD_24_DIFF_8_COM;
+	else
+		st->mode = AD4030_OUT_DATA_MD_24_DIFF;
+
+	return regmap_update_bits(st->regmap, AD4030_REG_MODES,
+				AD4030_REG_MODES_MASK_OUT_DATA_MODE,
+				st->mode);
+}
+
+static int ad4030_conversion(struct ad4030_state *st,
+			     const struct iio_chan_spec *chan)
+{
+	unsigned int bytes_to_read;
+	unsigned char byte_index;
+	unsigned int i;
+	int ret;
+
+	/* Number of bytes for one differential channel */
+	bytes_to_read = BITS_TO_BYTES(chan->scan_type.realbits);
+	/* Add one byte if we are using a differential + common byte mode */
+	bytes_to_read += (st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM ||
+			st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM) ? 1 : 0;
+	/* Mulitiply by the number of hardware channels */
+	bytes_to_read *= st->chip->num_channels;
+
+	gpiod_set_value_cansleep(st->cnv_gpio, 1);
+	ndelay(AD4030_TCNVH_NS);
+	gpiod_set_value_cansleep(st->cnv_gpio, 0);
+	ndelay(st->chip->tcyc);
+
+	ret = spi_read(st->spi, st->rx_data.raw, bytes_to_read);
+	if (ret)
+		return ret;
+
+	if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
+		return 0;
+
+	byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
+	for (i = 0; i < st->chip->num_channels; i++)
+		st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
+
+	return 0;
+}
+
+static int ad4030_single_conversion(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan, int *val)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad4030_set_mode(indio_dev, BIT(chan->channel));
+	if (ret)
+		return ret;
+
+	ret = ad4030_exit_config_mode(st);
+	if (ret)
+		goto out_exit_config_mode_error;
+
+	ret = ad4030_conversion(st, chan);
+	if (ret)
+		goto out_error;
+
+	if (chan->channel % 2)
+		*val = st->rx_data.buffered[chan->channel / 2].common;
+	else
+		*val = st->rx_data.buffered[chan->channel / 2].val;
+
+out_error:
+	ad4030_enter_config_mode(st);
+
+out_exit_config_mode_error:
+
+	if (ret)
+		return ret;
+
+	return IIO_VAL_INT;
+}
+
+static irqreturn_t ad4030_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ad4030_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad4030_conversion(st, indio_dev->channels);
+	if (ret)
+		goto out;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, st->rx_data.raw,
+					   pf->timestamp);
+
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static const int *ad4030_get_offset_avail(struct ad4030_state *st)
+{
+	return st->offset_avail;
+}
+
+static const int ad4030_gain_avail[3][2] = {
+	{ 0, 0 },
+	{ 0, 30 },
+	{ 1, 999969 },
+};
+
+static int ad4030_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *channel,
+			     const int **vals, int *type,
+			     int *length, long mask)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBBIAS:
+		*vals = ad4030_get_offset_avail(st);
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_RANGE;
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		*vals = (void *)ad4030_gain_avail;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_RANGE;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4030_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long info)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	int ret;
+
+	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+		switch (info) {
+		case IIO_CHAN_INFO_RAW:
+			return ad4030_single_conversion(indio_dev, chan, val);
+
+		case IIO_CHAN_INFO_SCALE:
+			*val = (st->vref_uv * 2) / MILLI;
+			*val2 = st->chip->precision_bits;
+			return IIO_VAL_FRACTIONAL_LOG2;
+
+		case IIO_CHAN_INFO_CALIBSCALE:
+			ret = ad4030_get_chan_gain(indio_dev, chan->channel,
+						   val, val2);
+			if (ret)
+				return ret;
+			return IIO_VAL_INT_PLUS_MICRO;
+
+		case IIO_CHAN_INFO_CALIBBIAS:
+			ret = ad4030_get_chan_offset(indio_dev, chan->channel,
+						     val);
+			if (ret)
+				return ret;
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}
+	}
+
+	unreachable();
+}
+
+static int ad4030_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val,
+			    int val2, long info)
+{
+	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+		switch (info) {
+		case IIO_CHAN_INFO_CALIBSCALE:
+			return ad4030_set_chan_gain(indio_dev, chan->channel,
+						    val, val2);
+
+		case IIO_CHAN_INFO_CALIBBIAS:
+			return ad4030_set_chan_offset(indio_dev, chan->channel,
+						      val);
+
+		default:
+			return -EINVAL;
+		}
+	}
+
+	unreachable();
+}
+
+static int ad4030_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			     unsigned int writeval, unsigned int *readval)
+{
+	const struct ad4030_state *st = iio_priv(indio_dev);
+
+	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+		if (readval)
+			return regmap_read(st->regmap, reg, readval);
+
+		return regmap_write(st->regmap, reg, writeval);
+	}
+
+	unreachable();
+}
+
+static const struct iio_info ad4030_iio_info = {
+	.read_avail = ad4030_read_avail,
+	.read_raw = ad4030_read_raw,
+	.write_raw = ad4030_write_raw,
+	.debugfs_reg_access = ad4030_reg_access,
+};
+
+static int ad4030_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad4030_set_mode(indio_dev, *indio_dev->active_scan_mask);
+	if (ret)
+		return ret;
+
+	ret = ad4030_exit_config_mode(st);
+	if (ret)
+		return ret;
+
+	/* Restore SPI max speed for conversion */
+	st->spi->max_speed_hz = st->conversion_speed_hz;
+
+	return 0;
+}
+
+static int ad4030_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+
+	/* Make sure the SPI clock is within range to read register */
+	st->spi->max_speed_hz = min(st->spi->max_speed_hz,
+				    AD4030_SPI_MAX_REG_XFER_SPEED);
+
+	return ad4030_enter_config_mode(st);
+}
+
+static const struct iio_buffer_setup_ops ad4030_buffer_setup_ops = {
+	.preenable = ad4030_buffer_preenable,
+	.postdisable = ad4030_buffer_postdisable,
+};
+
+static int ad4030_regulators_get(struct ad4030_state *st)
+{
+	struct device *dev = &st->spi->dev;
+	static const char * const ids[] = { "vdd-5v", "vdd-1v8" };
+	int ret;
+
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ids), ids);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
+
+	st->vio_uv = devm_regulator_get_enable_read_voltage(dev, "vio");
+	if (st->vio_uv < 0)
+		return dev_err_probe(dev, st->vio_uv,
+				     "Failed to enable and read vio voltage\n");
+
+	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
+	if (st->vref_uv < 0) {
+		if (st->vref_uv != -ENODEV)
+			return dev_err_probe(dev, st->vref_uv,
+					     "Failed to read vref voltage\n");
+
+		/* if not using optional REF, the internal REFIN must be used */
+		st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "refin");
+		if (st->vref_uv < 0)
+			return dev_err_probe(dev, st->vref_uv,
+					     "Failed to read vrefin voltage\n");
+	}
+
+	if (st->vref_uv < AD4030_VREF_MIN_UV || st->vref_uv > AD4030_VREF_MAX_UV)
+		return dev_err_probe(dev, -EINVAL,
+				     "vref(%d) must be in the range [%lu %lu]\n",
+				     st->vref_uv, AD4030_VREF_MIN_UV,
+				     AD4030_VREF_MAX_UV);
+
+	return 0;
+}
+
+static int ad4030_reset(struct ad4030_state *st)
+{
+	struct device *dev = &st->spi->dev;
+	struct gpio_desc *reset;
+	int ret;
+
+	/* Use GPIO if available ... */
+	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset))
+		return dev_err_probe(dev, PTR_ERR(reset),
+				"Failed to get reset GPIO\n");
+
+	if (reset) {
+		ndelay(50);
+		gpiod_set_value_cansleep(reset, 0);
+	} else {
+		/* ... falback to software reset otherwise */
+		ret = ad4030_enter_config_mode(st);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, AD4030_REG_INTERFACE_CONFIG_A,
+				   AD4030_REG_INTERFACE_CONFIG_A_SW_RESET);
+		if (ret)
+			return ret;
+	}
+
+	/* Wait for reset to complete before communicating to it */
+	fsleep(AD4030_TRESET_COM_DELAY_MS);
+
+	/* After reset, conversion mode is enabled. Switch to reg access */
+	return ad4030_enter_config_mode(st);
+}
+
+static int ad4030_detect_chip_info(const struct ad4030_state *st)
+{
+	unsigned int grade;
+	int ret;
+
+	ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
+	if (ret)
+		return ret;
+
+	grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
+	if (grade != st->chip->grade)
+		return dev_err_probe(&st->spi->dev, -EINVAL,
+					"Unknown grade(0x%x) for %s\n", grade,
+					st->chip->name);
+
+	return 0;
+}
+
+static int ad4030_config(struct ad4030_state *st)
+{
+	st->offset_avail[0] = (int)BIT(st->chip->precision_bits - 1) * -1;
+	st->offset_avail[1] = 1;
+	st->offset_avail[2] = BIT(st->chip->precision_bits - 1) - 1;
+
+	if (st->vio_uv < AD4030_VIO_THRESHOLD_UV)
+		return regmap_write(st->regmap, AD4030_REG_IO,
+				AD4030_REG_IO_MASK_IO2X);
+
+	return 0;
+}
+
+static int ad4030_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct iio_dev *indio_dev;
+	struct ad4030_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+
+	/* Make sure the SPI clock is within range to read register */
+	st->conversion_speed_hz = min(spi->max_speed_hz,
+				      AD4030_SPI_MAX_REG_XFER_SPEED);
+
+	st->regmap = devm_regmap_init(dev, &ad4030_regmap_bus, st,
+				      &ad4030_regmap_config);
+	if (IS_ERR(st->regmap))
+		dev_err_probe(dev, PTR_ERR(st->regmap),
+			      "Failed to initialize regmap\n");
+
+	st->chip = spi_get_device_match_data(spi);
+	if (!st->chip)
+		return -EINVAL;
+
+	ret = ad4030_regulators_get(st);
+	if (ret)
+		return ret;
+
+	ret = ad4030_reset(st);
+	if (ret)
+		return ret;
+
+	ret = ad4030_detect_chip_info(st);
+	if (ret)
+		return ret;
+
+	ret = ad4030_config(st);
+	if (ret)
+		return ret;
+
+	st->cnv_gpio = devm_gpiod_get(dev, "cnv", GPIOD_OUT_LOW);
+	if (IS_ERR(st->cnv_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
+				     "Failed to get cnv gpio\n");
+
+	/*
+	 * One hardware channel is split in two software channels when using
+	 * common byte mode. Add one more channel for the timestamp.
+	 */
+	indio_dev->num_channels = 2 * st->chip->num_channels + 1;
+	indio_dev->name = st->chip->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ad4030_iio_info;
+	indio_dev->channels = st->chip->channels;
+	indio_dev->available_scan_masks = st->chip->available_masks;
+	indio_dev->masklength = st->chip->available_masks_len;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      ad4030_trigger_handler,
+					      &ad4030_buffer_setup_ops);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const unsigned long ad4030_channel_masks[] = {
+	/* Differential only */
+	BIT(0),
+	/* Differential with common byte */
+	GENMASK(1, 0),
+	0,
+};
+
+static const struct ad4030_chip_info ad4030_24_chip_info = {
+	.name = "ad4030-24",
+	.available_masks = ad4030_channel_masks,
+	.available_masks_len = ARRAY_SIZE(ad4030_channel_masks),
+	.channels = {
+		AD4030_CHAN_IN(0, 32, 24, 8),
+		AD4030_CHAN_CMO(0),
+		IIO_CHAN_SOFT_TIMESTAMP(2),
+	},
+	.grade = AD4030_REG_CHIP_GRADE_AD4030_24_GRADE,
+	.precision_bits = 24,
+	.num_channels = 1,
+	.tcyc = AD4030_TCYC_ADJUSTED_NS,
+};
+
+static const struct spi_device_id ad4030_id_table[] = {
+	{ "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ad4030_id_table);
+
+static const struct of_device_id ad4030_of_match[] = {
+	{ .compatible = "adi,ad4030-24", .data = &ad4030_24_chip_info },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ad4030_of_match);
+
+static struct spi_driver ad4030_driver = {
+	.driver = {
+		.name = "ad4030",
+		.of_match_table = ad4030_of_match,
+	},
+	.probe = ad4030_probe,
+	.id_table = ad4030_id_table,
+};
+module_spi_driver(ad4030_driver);
+
+MODULE_AUTHOR("Esteban Blanc <eblanc@baylibre.com>");
+MODULE_DESCRIPTION("Analog Devices AD4630 ADC family driver");
+MODULE_LICENSE("GPL");

-- 
2.44.1

[PATCH 3/6] iio: adc: ad4030: add averaging support

[Esteban Blanc]

This add support for the averaging mode of AD4030 using oversampling IIO
attribute

Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
---
 drivers/iio/adc/ad4030.c | 126 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 111 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
index a981dce988e5..e1e1dbf0565c 100644
--- a/drivers/iio/adc/ad4030.c
+++ b/drivers/iio/adc/ad4030.c
@@ -115,6 +115,18 @@ enum ad4030_out_mode {
 	AD4030_OUT_DATA_MD_32_PATTERN = 0x04
 };
 
+enum {
+	AD4030_LANE_MD_1_PER_CH,
+	AD4030_LANE_MD_2_PER_CH,
+	AD4030_LANE_MD_4_PER_CH,
+	AD4030_LANE_MD_INTERLEAVED = 0b11,
+};
+
+enum {
+	AD4030_SCAN_TYPE_NORMAL,
+	AD4030_SCAN_TYPE_AVG,
+};
+
 struct ad4030_chip_info {
 	const char *name;
 	const unsigned long *available_masks;
@@ -135,6 +147,7 @@ struct ad4030_state {
 	int vref_uv;
 	int vio_uv;
 	int offset_avail[3];
+	unsigned int avg_len;
 	u32 conversion_speed_hz;
 	enum ad4030_out_mode mode;
 
@@ -169,8 +182,11 @@ struct ad4030_state {
 	},								\
 }
 
-#define AD4030_CHAN_IN(_idx, _storage, _real, _shift) {			\
-	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),		\
+#define AD4030_CHAN_IN(_idx, _scan_type) {				\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE) |		\
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
+	.info_mask_shared_by_all_available =				\
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
 		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
 		BIT(IIO_CHAN_INFO_RAW),					\
@@ -183,15 +199,16 @@ struct ad4030_state {
 	.scan_index = _idx * 2,						\
 	.extend_name = "Channel" #_idx " differential part",		\
 	.differential = true,						\
-	.scan_type = {							\
-		.sign = 's',						\
-		.storagebits = _storage,				\
-		.realbits = _real,					\
-		.shift = _shift,					\
-		.endianness = IIO_BE,					\
-	},								\
+	.has_ext_scan_type = 1,						\
+	.ext_scan_type = _scan_type,					\
+	.num_ext_scan_type = ARRAY_SIZE(_scan_type),			\
 }
 
+static const int ad4030_average_modes[] = {
+	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192, 16384,
+	32768, 65536
+};
+
 static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
 			   void *val, size_t val_size)
 {
@@ -327,6 +344,13 @@ static int ad4030_get_chan_offset(struct iio_dev *indio_dev, int ch, int *val)
 	return 0;
 }
 
+static int ad4030_get_avg_frame_len(struct iio_dev *dev)
+{
+	const struct ad4030_state *st = iio_priv(dev);
+
+	return 1L << st->avg_len;
+}
+
 static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int gain_int,
 				int gain_frac)
 {
@@ -369,6 +393,22 @@ static int ad4030_set_chan_offset(struct iio_dev *indio_dev, int ch, int offset)
 			  st->rx_data.raw, AD4030_REG_OFFSET_BYTES_NB);
 }
 
+static int ad4030_set_avg_frame_len(struct iio_dev *dev, unsigned int avg_len)
+{
+	struct ad4030_state *st = iio_priv(dev);
+	unsigned int avg_val = ilog2(avg_len);
+	int ret;
+
+	ret = regmap_write(st->regmap, AD4030_REG_AVG, AD4030_REG_AVG_MASK_AVG_SYNC |
+		    FIELD_PREP(AD4030_REG_AVG_MASK_AVG_VAL, avg_val));
+	if (ret)
+		return ret;
+
+	st->avg_len = avg_val;
+
+	return 0;
+}
+
 static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
 					unsigned int mask)
 {
@@ -380,7 +420,9 @@ static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
 {
 	struct ad4030_state *st = iio_priv(indio_dev);
 
-	if (ad4030_is_common_byte_asked(st, mask))
+	if (st->avg_len)
+		st->mode = AD4030_OUT_DATA_MD_30_AVERAGED_DIFF;
+	else if (ad4030_is_common_byte_asked(st, mask))
 		st->mode = AD4030_OUT_DATA_MD_24_DIFF_8_COM;
 	else
 		st->mode = AD4030_OUT_DATA_MD_24_DIFF;
@@ -394,6 +436,7 @@ static int ad4030_conversion(struct ad4030_state *st,
 			     const struct iio_chan_spec *chan)
 {
 	unsigned int bytes_to_read;
+	unsigned long cnv_nb = 1UL << st->avg_len;
 	unsigned char byte_index;
 	unsigned int i;
 	int ret;
@@ -406,10 +449,12 @@ static int ad4030_conversion(struct ad4030_state *st,
 	/* Mulitiply by the number of hardware channels */
 	bytes_to_read *= st->chip->num_channels;
 
-	gpiod_set_value_cansleep(st->cnv_gpio, 1);
-	ndelay(AD4030_TCNVH_NS);
-	gpiod_set_value_cansleep(st->cnv_gpio, 0);
-	ndelay(st->chip->tcyc);
+	for (i = 0; i < cnv_nb; i++) {
+		gpiod_set_value_cansleep(st->cnv_gpio, 1);
+		ndelay(AD4030_TCNVH_NS);
+		gpiod_set_value_cansleep(st->cnv_gpio, 0);
+		ndelay(st->chip->tcyc);
+	}
 
 	ret = spi_read(st->spi, st->rx_data.raw, bytes_to_read);
 	if (ret)
@@ -508,6 +553,12 @@ static int ad4030_read_avail(struct iio_dev *indio_dev,
 		*type = IIO_VAL_INT_PLUS_MICRO;
 		return IIO_AVAIL_RANGE;
 
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = ad4030_average_modes;
+		*type = IIO_VAL_INT;
+		*length = ARRAY_SIZE(ad4030_average_modes);
+		return IIO_AVAIL_LIST;
+
 	default:
 		return -EINVAL;
 	}
@@ -544,6 +595,10 @@ static int ad4030_read_raw(struct iio_dev *indio_dev,
 				return ret;
 			return IIO_VAL_INT;
 
+		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+			*val = ad4030_get_avg_frame_len(indio_dev);
+			return IIO_VAL_INT;
+
 		default:
 			return -EINVAL;
 		}
@@ -566,6 +621,9 @@ static int ad4030_write_raw(struct iio_dev *indio_dev,
 			return ad4030_set_chan_offset(indio_dev, chan->channel,
 						      val);
 
+		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+			return ad4030_set_avg_frame_len(indio_dev, val);
+
 		default:
 			return -EINVAL;
 		}
@@ -589,11 +647,20 @@ static int ad4030_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 	unreachable();
 }
 
+static int ad4030_get_current_scan_type(const struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+
+	return st->avg_len ? AD4030_SCAN_TYPE_AVG : AD4030_SCAN_TYPE_NORMAL;
+}
+
 static const struct iio_info ad4030_iio_info = {
 	.read_avail = ad4030_read_avail,
 	.read_raw = ad4030_read_raw,
 	.write_raw = ad4030_write_raw,
 	.debugfs_reg_access = ad4030_reg_access,
+	.get_current_scan_type = ad4030_get_current_scan_type,
 };
 
 static int ad4030_buffer_preenable(struct iio_dev *indio_dev)
@@ -626,9 +693,21 @@ static int ad4030_buffer_postdisable(struct iio_dev *indio_dev)
 	return ad4030_enter_config_mode(st);
 }
 
+static bool ad4030_validate_scan_mask(struct iio_dev *indio_dev, const unsigned long *scan_mask)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+
+	/* Asking for both common channels and averaging */
+	if (st->avg_len && ad4030_is_common_byte_asked(st, *scan_mask))
+		return false;
+
+	return true;
+}
+
 static const struct iio_buffer_setup_ops ad4030_buffer_setup_ops = {
 	.preenable = ad4030_buffer_preenable,
 	.postdisable = ad4030_buffer_postdisable,
+	.validate_scan_mask = ad4030_validate_scan_mask,
 };
 
 static int ad4030_regulators_get(struct ad4030_state *st)
@@ -812,12 +891,29 @@ static const unsigned long ad4030_channel_masks[] = {
 	0,
 };
 
+static const struct iio_scan_type ad4030_24_scan_types[] = {
+	[AD4030_SCAN_TYPE_NORMAL] = {
+		.sign = 's',
+		.storagebits = 32,
+		.realbits = 24,
+		.shift = 8,
+		.endianness = IIO_BE,
+	},
+	[AD4030_SCAN_TYPE_AVG] = {
+		.sign = 's',
+		.storagebits = 32,
+		.realbits = 30,
+		.shift = 2,
+		.endianness = IIO_BE,
+	},
+};
+
 static const struct ad4030_chip_info ad4030_24_chip_info = {
 	.name = "ad4030-24",
 	.available_masks = ad4030_channel_masks,
 	.available_masks_len = ARRAY_SIZE(ad4030_channel_masks),
 	.channels = {
-		AD4030_CHAN_IN(0, 32, 24, 8),
+		AD4030_CHAN_IN(0, ad4030_24_scan_types),
 		AD4030_CHAN_CMO(0),
 		IIO_CHAN_SOFT_TIMESTAMP(2),
 	},

-- 
2.44.1

[PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Esteban Blanc]

AD4630-24 and AD4630-16 are 2 channels ADCs. Both channels are
interleaved bit per bit on SDO line.

Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
---
 drivers/iio/adc/ad4030.c | 197 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 173 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
index e1e1dbf0565c..dbba5287b630 100644
--- a/drivers/iio/adc/ad4030.c
+++ b/drivers/iio/adc/ad4030.c
@@ -32,6 +32,8 @@
 #define AD4030_REG_PRODUCT_ID_H				0x05
 #define AD4030_REG_CHIP_GRADE				0x06
 #define     AD4030_REG_CHIP_GRADE_AD4030_24_GRADE	0x10
+#define     AD4030_REG_CHIP_GRADE_AD4630_16_GRADE	0x03
+#define     AD4030_REG_CHIP_GRADE_AD4630_24_GRADE	0x00
 #define     AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE	GENMASK(7, 3)
 #define AD4030_REG_SCRATCH_PAD			0x0A
 #define AD4030_REG_SPI_REVISION			0x0B
@@ -159,10 +161,14 @@ struct ad4030_state {
 	struct {
 		union {
 			u8 raw[AD4030_MAXIMUM_RX_BUFFER_SIZE];
-			struct {
-				s32 val;
-				u32 common;
-			} __packed buffered[AD4030_MAX_HARDWARE_CHANNEL_NB];
+			union {
+				s32 diff[AD4030_MAX_HARDWARE_CHANNEL_NB];
+				struct {
+					s32 diff;
+					u32 common;
+				} __packed
+				buffered_common[AD4030_MAX_HARDWARE_CHANNEL_NB];
+			};
 		};
 	} rx_data __aligned(IIO_DMA_MINALIGN);
 };
@@ -171,7 +177,7 @@ struct ad4030_state {
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
 	.type = IIO_VOLTAGE,						\
 	.indexed = 1,							\
-	.channel = _idx * 2 + 2,					\
+	.channel = _idx * 3 + 2,					\
 	.scan_index = _idx * 2 + 1,					\
 	.extend_name = "Channel" #_idx " common byte part",		\
 	.scan_type = {							\
@@ -194,8 +200,8 @@ struct ad4030_state {
 		BIT(IIO_CHAN_INFO_CALIBSCALE),				\
 	.type = IIO_VOLTAGE,						\
 	.indexed = 1,							\
-	.channel = _idx * 2,						\
-	.channel2 = _idx * 2 + 1,					\
+	.channel = _idx * 3,						\
+	.channel2 = _idx * 3 + 1,					\
 	.scan_index = _idx * 2,						\
 	.extend_name = "Channel" #_idx " differential part",		\
 	.differential = true,						\
@@ -412,7 +418,7 @@ static int ad4030_set_avg_frame_len(struct iio_dev *dev, unsigned int avg_len)
 static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
 					unsigned int mask)
 {
-	/* Common byte channel is after the "real" differential sample channel */
+	/* Common byte channels are after each differential channel */
 	return mask & AD4030_COMMON_BYTE_CHANNELS_FILTER;
 }
 
@@ -420,18 +426,69 @@ static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
 {
 	struct ad4030_state *st = iio_priv(indio_dev);
 
-	if (st->avg_len)
+	if (st->avg_len) {
 		st->mode = AD4030_OUT_DATA_MD_30_AVERAGED_DIFF;
-	else if (ad4030_is_common_byte_asked(st, mask))
-		st->mode = AD4030_OUT_DATA_MD_24_DIFF_8_COM;
-	else
+	} else if (ad4030_is_common_byte_asked(st, mask)) {
+		switch (st->chip->precision_bits) {
+		case 16:
+			st->mode = AD4030_OUT_DATA_MD_16_DIFF_8_COM;
+			break;
+
+		case 24:
+			st->mode = AD4030_OUT_DATA_MD_24_DIFF_8_COM;
+			break;
+
+		default:
+			return -EINVAL;
+		}
+	} else {
 		st->mode = AD4030_OUT_DATA_MD_24_DIFF;
+	}
 
 	return regmap_update_bits(st->regmap, AD4030_REG_MODES,
 				AD4030_REG_MODES_MASK_OUT_DATA_MODE,
 				st->mode);
 }
 
+/*
+ * @brief Descramble 2 32bits numbers out of a 64bits. The bits are interleaved:
+ * 1 bit for first number, 1 bit for the second, and so on...
+ */
+static void ad4030_extract_interleaved(u8 *src, u32 *ch0, u32 *ch1)
+{
+	u8 h0, h1, l0, l1;
+	u32 out0, out1;
+	u8 *out0_raw = (u8 *)&out0;
+	u8 *out1_raw = (u8 *)&out1;
+
+	for (int i = 0; i < 4; i++) {
+		h0 = src[i * 2];
+		l1 = src[i * 2 + 1];
+		h1 = h0 << 1;
+		l0 = l1 >> 1;
+
+		h0 &= 0xAA;
+		l0 &= 0x55;
+		h1 &= 0xAA;
+		l1 &= 0x55;
+
+		h0 = (h0 | h0 << 001) & 0xCC;
+		h1 = (h1 | h1 << 001) & 0xCC;
+		l0 = (l0 | l0 >> 001) & 0x33;
+		l1 = (l1 | l1 >> 001) & 0x33;
+		h0 = (h0 | h0 << 002) & 0xF0;
+		h1 = (h1 | h1 << 002) & 0xF0;
+		l0 = (l0 | l0 >> 002) & 0x0F;
+		l1 = (l1 | l1 >> 002) & 0x0F;
+
+		out0_raw[i] = h0 | l0;
+		out1_raw[i] = h1 | l1;
+	}
+
+	*ch0 = out0;
+	*ch1 = out1;
+}
+
 static int ad4030_conversion(struct ad4030_state *st,
 			     const struct iio_chan_spec *chan)
 {
@@ -460,12 +517,21 @@ static int ad4030_conversion(struct ad4030_state *st,
 	if (ret)
 		return ret;
 
-	if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
+	if (st->chip->num_channels == 2)
+		ad4030_extract_interleaved(st->rx_data.raw,
+					   &st->rx_data.diff[0],
+					   &st->rx_data.diff[1]);
+
+	if (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
+	    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
 		return 0;
 
 	byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
-	for (i = 0; i < st->chip->num_channels; i++)
-		st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
+	/* Doing it backward to avoid overlap when reordering */
+	for (i = st->chip->num_channels - 1; i > 0; i--) {
+		st->rx_data.buffered_common[i].diff = st->rx_data.diff[i];
+		st->rx_data.buffered_common[i].common = ((u8 *)&st->rx_data.diff[i])[byte_index];
+	}
 
 	return 0;
 }
@@ -489,9 +555,9 @@ static int ad4030_single_conversion(struct iio_dev *indio_dev,
 		goto out_error;
 
 	if (chan->channel % 2)
-		*val = st->rx_data.buffered[chan->channel / 2].common;
+		*val = st->rx_data.buffered_common[chan->channel / 2].common;
 	else
-		*val = st->rx_data.buffered[chan->channel / 2].val;
+		*val = st->rx_data.diff[chan->channel / 2];
 
 out_error:
 	ad4030_enter_config_mode(st);
@@ -582,14 +648,17 @@ static int ad4030_read_raw(struct iio_dev *indio_dev,
 			return IIO_VAL_FRACTIONAL_LOG2;
 
 		case IIO_CHAN_INFO_CALIBSCALE:
-			ret = ad4030_get_chan_gain(indio_dev, chan->channel,
-						   val, val2);
+			ret = ad4030_get_chan_gain(indio_dev,
+						   chan->scan_index / 2,
+						   val,
+						   val2);
 			if (ret)
 				return ret;
 			return IIO_VAL_INT_PLUS_MICRO;
 
 		case IIO_CHAN_INFO_CALIBBIAS:
-			ret = ad4030_get_chan_offset(indio_dev, chan->channel,
+			ret = ad4030_get_chan_offset(indio_dev,
+						     chan->scan_index / 2,
 						     val);
 			if (ret)
 				return ret;
@@ -614,11 +683,14 @@ static int ad4030_write_raw(struct iio_dev *indio_dev,
 	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
 		switch (info) {
 		case IIO_CHAN_INFO_CALIBSCALE:
-			return ad4030_set_chan_gain(indio_dev, chan->channel,
-						    val, val2);
+			return ad4030_set_chan_gain(indio_dev,
+						    chan->scan_index / 2,
+						    val,
+						    val2);
 
 		case IIO_CHAN_INFO_CALIBBIAS:
-			return ad4030_set_chan_offset(indio_dev, chan->channel,
+			return ad4030_set_chan_offset(indio_dev,
+						      chan->scan_index / 2,
 						      val);
 
 		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
@@ -801,10 +873,24 @@ static int ad4030_detect_chip_info(const struct ad4030_state *st)
 
 static int ad4030_config(struct ad4030_state *st)
 {
+	int ret;
+	u8 reg_modes;
+
 	st->offset_avail[0] = (int)BIT(st->chip->precision_bits - 1) * -1;
 	st->offset_avail[1] = 1;
 	st->offset_avail[2] = BIT(st->chip->precision_bits - 1) - 1;
 
+	if (st->chip->num_channels > 1)
+		reg_modes = FIELD_PREP(AD4030_REG_MODES_MASK_LANE_MODE,
+				       AD4030_LANE_MD_INTERLEAVED);
+	else
+		reg_modes = FIELD_PREP(AD4030_REG_MODES_MASK_LANE_MODE,
+				       AD4030_LANE_MD_1_PER_CH);
+
+	ret = regmap_write(st->regmap, AD4030_REG_MODES, reg_modes);
+	if (ret)
+		return ret;
+
 	if (st->vio_uv < AD4030_VIO_THRESHOLD_UV)
 		return regmap_write(st->regmap, AD4030_REG_IO,
 				AD4030_REG_IO_MASK_IO2X);
@@ -891,8 +977,16 @@ static const unsigned long ad4030_channel_masks[] = {
 	0,
 };
 
+static const unsigned long ad4630_channel_masks[] = {
+	/* Differential only */
+	BIT(0) | BIT(2),
+	/* Differential with common byte */
+	GENMASK(3, 0),
+	0,
+};
+
 static const struct iio_scan_type ad4030_24_scan_types[] = {
-	[AD4030_SCAN_TYPE_NORMAL] = {
+	[AD4030_OUT_DATA_MD_24_DIFF] = {
 		.sign = 's',
 		.storagebits = 32,
 		.realbits = 24,
@@ -908,6 +1002,23 @@ static const struct iio_scan_type ad4030_24_scan_types[] = {
 	},
 };
 
+static const struct iio_scan_type ad4030_16_scan_types[] = {
+	[AD4030_SCAN_TYPE_NORMAL] = {
+		.sign = 's',
+		.storagebits = 32,
+		.realbits = 16,
+		.shift = 16,
+		.endianness = IIO_BE,
+	},
+	[AD4030_SCAN_TYPE_AVG] = {
+		.sign = 's',
+		.storagebits = 32,
+		.realbits = 30,
+		.shift = 2,
+		.endianness = IIO_BE,
+	}
+};
+
 static const struct ad4030_chip_info ad4030_24_chip_info = {
 	.name = "ad4030-24",
 	.available_masks = ad4030_channel_masks,
@@ -923,14 +1034,52 @@ static const struct ad4030_chip_info ad4030_24_chip_info = {
 	.tcyc = AD4030_TCYC_ADJUSTED_NS,
 };
 
+static const struct ad4030_chip_info ad4630_16_chip_info = {
+	.name = "ad4630-16",
+	.available_masks = ad4630_channel_masks,
+	.available_masks_len = ARRAY_SIZE(ad4630_channel_masks),
+	.channels = {
+		AD4030_CHAN_IN(0, ad4030_16_scan_types),
+		AD4030_CHAN_CMO(0),
+		AD4030_CHAN_IN(1, ad4030_16_scan_types),
+		AD4030_CHAN_CMO(1),
+		IIO_CHAN_SOFT_TIMESTAMP(4),
+	},
+	.grade = AD4030_REG_CHIP_GRADE_AD4630_16_GRADE,
+	.precision_bits = 16,
+	.num_channels = 2,
+	.tcyc = AD4030_TCYC_ADJUSTED_NS,
+};
+
+static const struct ad4030_chip_info ad4630_24_chip_info = {
+	.name = "ad4630-24",
+	.available_masks = ad4630_channel_masks,
+	.available_masks_len = ARRAY_SIZE(ad4630_channel_masks),
+	.channels = {
+		AD4030_CHAN_IN(0, ad4030_24_scan_types),
+		AD4030_CHAN_CMO(0),
+		AD4030_CHAN_IN(1, ad4030_24_scan_types),
+		AD4030_CHAN_CMO(1),
+		IIO_CHAN_SOFT_TIMESTAMP(4),
+	},
+	.grade = AD4030_REG_CHIP_GRADE_AD4630_24_GRADE,
+	.precision_bits = 24,
+	.num_channels = 2,
+	.tcyc = AD4030_TCYC_ADJUSTED_NS,
+};
+
 static const struct spi_device_id ad4030_id_table[] = {
 	{ "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
+	{ "ad4630-16", (kernel_ulong_t)&ad4630_16_chip_info },
+	{ "ad4630-24", (kernel_ulong_t)&ad4630_24_chip_info },
 	{}
 };
 MODULE_DEVICE_TABLE(spi, ad4030_id_table);
 
 static const struct of_device_id ad4030_of_match[] = {
 	{ .compatible = "adi,ad4030-24", .data = &ad4030_24_chip_info },
+	{ .compatible = "adi,ad4630-16", .data = &ad4630_16_chip_info },
+	{ .compatible = "adi,ad4630-24", .data = &ad4630_24_chip_info },
 	{}
 };
 MODULE_DEVICE_TABLE(of, ad4030_of_match);

-- 
2.44.1

[PATCH 5/6] iio: adc: ad4030: add support for ad4632-16 and ad4632-24

[Esteban Blanc]

AD4632-24 and AD4632-16 are 2 channels ADCs. Both channels are
interleaved bit per bit on SDO line.

Both of them do not have evaluation board. As such, the support added
here can't be tested. Support is provided as best effort until someone get
their hands on one.

Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
---
 drivers/iio/adc/ad4030.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
index dbba5287b630..d42eb53b5e56 100644
--- a/drivers/iio/adc/ad4030.c
+++ b/drivers/iio/adc/ad4030.c
@@ -34,6 +34,8 @@
 #define     AD4030_REG_CHIP_GRADE_AD4030_24_GRADE	0x10
 #define     AD4030_REG_CHIP_GRADE_AD4630_16_GRADE	0x03
 #define     AD4030_REG_CHIP_GRADE_AD4630_24_GRADE	0x00
+#define     AD4030_REG_CHIP_GRADE_AD4632_16_GRADE	0x05
+#define     AD4030_REG_CHIP_GRADE_AD4632_24_GRADE	0x02
 #define     AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE	GENMASK(7, 3)
 #define AD4030_REG_SCRATCH_PAD			0x0A
 #define AD4030_REG_SPI_REVISION			0x0B
@@ -106,6 +108,8 @@
 #define AD4030_TCYC_NS			500
 #define AD4030_TCYC_ADJUSTED_NS		(AD4030_TCYC_NS - AD4030_TCNVL_NS)
 #define AD4030_TRESET_PW_NS		50
+#define AD4632_TCYC_NS			2000
+#define AD4632_TCYC_ADJUSTED_NS		(AD4632_TCYC_NS - AD4030_TCNVL_NS)
 #define AD4030_TRESET_COM_DELAY_MS	750
 
 enum ad4030_out_mode {
@@ -1068,10 +1072,46 @@ static const struct ad4030_chip_info ad4630_24_chip_info = {
 	.tcyc = AD4030_TCYC_ADJUSTED_NS,
 };
 
+static const struct ad4030_chip_info ad4632_16_chip_info = {
+	.name = "ad4632-16",
+	.available_masks = ad4630_channel_masks,
+	.available_masks_len = ARRAY_SIZE(ad4630_channel_masks),
+	.channels = {
+		AD4030_CHAN_IN(0, ad4030_16_scan_types),
+		AD4030_CHAN_CMO(0),
+		AD4030_CHAN_IN(1, ad4030_16_scan_types),
+		AD4030_CHAN_CMO(1),
+		IIO_CHAN_SOFT_TIMESTAMP(4),
+	},
+	.grade = AD4030_REG_CHIP_GRADE_AD4632_16_GRADE,
+	.precision_bits = 16,
+	.num_channels = 2,
+	.tcyc = AD4632_TCYC_ADJUSTED_NS,
+};
+
+static const struct ad4030_chip_info ad4632_24_chip_info = {
+	.name = "ad4632-24",
+	.available_masks = ad4630_channel_masks,
+	.available_masks_len = ARRAY_SIZE(ad4630_channel_masks),
+	.channels = {
+		AD4030_CHAN_IN(0, ad4030_24_scan_types),
+		AD4030_CHAN_CMO(0),
+		AD4030_CHAN_IN(1, ad4030_24_scan_types),
+		AD4030_CHAN_CMO(1),
+		IIO_CHAN_SOFT_TIMESTAMP(4),
+	},
+	.grade = AD4030_REG_CHIP_GRADE_AD4632_24_GRADE,
+	.precision_bits = 24,
+	.num_channels = 2,
+	.tcyc = AD4632_TCYC_ADJUSTED_NS,
+};
+
 static const struct spi_device_id ad4030_id_table[] = {
 	{ "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
 	{ "ad4630-16", (kernel_ulong_t)&ad4630_16_chip_info },
 	{ "ad4630-24", (kernel_ulong_t)&ad4630_24_chip_info },
+	{ "ad4632-16", (kernel_ulong_t)&ad4632_16_chip_info },
+	{ "ad4632-24", (kernel_ulong_t)&ad4632_24_chip_info },
 	{}
 };
 MODULE_DEVICE_TABLE(spi, ad4030_id_table);
@@ -1080,6 +1120,8 @@ static const struct of_device_id ad4030_of_match[] = {
 	{ .compatible = "adi,ad4030-24", .data = &ad4030_24_chip_info },
 	{ .compatible = "adi,ad4630-16", .data = &ad4630_16_chip_info },
 	{ .compatible = "adi,ad4630-24", .data = &ad4630_24_chip_info },
+	{ .compatible = "adi,ad4632-16", .data = &ad4632_16_chip_info },
+	{ .compatible = "adi,ad4632-24", .data = &ad4632_24_chip_info },
 	{}
 };
 MODULE_DEVICE_TABLE(of, ad4030_of_match);

-- 
2.44.1

[PATCH 6/6] docs: iio: ad4030: add documentation

[Esteban Blanc]

This adds a new page to document how to use the ad4030 ADC driver

Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
---
 Documentation/iio/ad4030.rst | 129 +++++++++++++++++++++++++++++++++++++++++++
 Documentation/iio/index.rst  |   1 +
 MAINTAINERS                  |   1 +
 3 files changed, 131 insertions(+)

diff --git a/Documentation/iio/ad4030.rst b/Documentation/iio/ad4030.rst
new file mode 100644
index 000000000000..56e0ba58b127
--- /dev/null
+++ b/Documentation/iio/ad4030.rst
@@ -0,0 +1,129 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+=============
+AD4030 driver
+=============
+
+ADC driver for Analog Devices Inc. AD4030 and similar devices. The module name
+is ``ad4030``.
+
+
+Supported devices
+=================
+
+The following chips are supported by this driver:
+
+* `AD4030-24 <https://www.analog.com/AD4030-24>`_
+* `AD4630-16 <https://www.analog.com/AD4630-16>`_
+* `AD4630-24 <https://www.analog.com/AD4630-24>`_
+* `AD4632-16 <https://www.analog.com/AD4632-16>`_
+* `AD4632-24 <https://www.analog.com/AD4632-24>`_
+
+IIO channels
+============
+
+Each "device" channel as described in the datasheet is split in 2 IIO channels,
+in the following order:
+
+- One channel for the differential data
+- One channel for the common byte.
+
+Supported features
+==================
+
+SPI wiring modes
+----------------
+
+The driver currently supports the following SPI wiring configurations:
+
+One lane mode
+^^^^^^^^^^^^^
+
+In this mode, each channel has its own SDO line to send the conversion results.
+At the moment this mode can only be used on AD4030 which has one channel so only
+one SDO line is used.
+
+.. code-block::
+
+    +-------------+         +-------------+
+    |     ADC     |         |     HOST    |
+    |             |         |             |
+    |         CNV |<--------| CNV         |
+    |          CS |<--------| CS          |
+    |         SDI |<--------| SDO         |
+    |        SDO0 |-------->| SDI         |
+    |        SCLK |<--------| SCLK        |
+    +-------------+         +-------------+
+
+Interleaved mode
+^^^^^^^^^^^^^^^^
+
+In this mode, both channels conversion results are bit interleaved one SDO line.
+As such the wiring is the same as `One lane mode`.
+
+SPI Clock mode
+--------------
+
+Only the SPI clocking mode is supported.
+
+Output modes
+------------
+
+There is more exposed IIO channels than channels as describe in the devices
+datasheet. This is due to the `Differential data + 8-bit common-mode` encoding
+2 types of information in one conversion result. As such a "device" channel
+provides 2 IIO channels, one for the differential data and one for the common
+byte.
+
+Differential data
+^^^^^^^^^^^^^^^^^
+
+This mode is selected when:
+
+- Only differential channels are selected
+- Oversampling attribute is set to 1
+
+Differential data + common-mode
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+This mode is selected when:
+
+- Differential and common-mode channels are selected
+- Oversampling attribute is set to 1
+
+For the 24-bits chips, this mode is also available with 16-bits differential
+data but is not selectable yet.
+
+Averaged differential data
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+This mode is selected when:
+
+- Only differential channels are selected
+- Oversampling attribute is greater than 1
+
+Reference voltage
+-----------------
+
+The chip supports an external reference voltage via the ``REF`` input or an
+internal buffered reference voltage via the ``REFIN`` input. The driver looks
+at the device tree to determine which is being used. If ``ref-supply`` is
+present, then the external reference voltage is used and the internal buffer is
+disabled. If ``refin-supply`` is present, then the internal buffered reference
+voltage is used.
+
+Reset
+-----
+
+Both hardware and software reset are supported. The driver looks first at the
+device tree to see if the `reset-gpio` is populated. If not present, the driver
+will fallback to a software reset by wiring to the device's registers.
+
+Unimplemented features
+----------------------
+
+- ``BUSY`` indication
+- Additional wiring modes
+- Additional clock modes
+- Differential data 16-bits + common-mode for 24-bits chips
+
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index dfcf9618568a..61faf3a60da6 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -19,6 +19,7 @@ Industrial I/O Kernel Drivers
    :maxdepth: 1
 
    ad4000
+   ad4030
    ad4695
    ad7380
    ad7944
diff --git a/MAINTAINERS b/MAINTAINERS
index 6a5a0e7b7a51..4a076a48648a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -420,6 +420,7 @@ R:	Esteban Blanc <eblanc@baylibre.com>
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
+F:	Documentation/iio/ad4030.c
 F:	drivers/iio/adc/ad4030.c
 
 AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER

-- 
2.44.1

Re: [PATCH 0/6] iio: adc: ad4030: new driver for AD4030 and similar ADCs

[Conor Dooley]

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

On Thu, Aug 22, 2024 at 02:45:16PM +0200, Esteban Blanc wrote:
> This is adding DT bindings and a new driver for AD4030, AD4630 and
> AD4632 ADCs.
> 
> This work is being done in collaboration with Analog Devices Inc.,
> hence they are listed as maintainers rather than me.
> 
> The code has been tested on a Zedboard with an EVAL-AD4030-24FMCZ,
> an EVAL-AD4630-24FMCZ and an EVAL-AD4630-16FMCZ. As there is no eval
> board for AD4632 the support can't be tested at the moment. The main
> difference is the reduced throughput.
> 
> To: Lars-Peter Clausen <lars@metafoo.de>
> To: Michael Hennerich <Michael.Hennerich@analog.com>
> To: Jonathan Cameron <jic23@kernel.org>
> To: Rob Herring <robh@kernel.org>
> To: Krzysztof Kozlowski <krzk+dt@kernel.org>
> To: Conor Dooley <conor+dt@kernel.org>
> To: Nuno Sa <nuno.sa@analog.com>
> To: Jonathan Corbet <corbet@lwn.net>
> Cc: Michael Hennerich <michael.hennerich@analog.com>
> Cc: linux-iio@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: David Lechner <dlechner@baylibre.com>
> Cc: linux-doc@vger.kernel.org
> Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> 
> Changes since RFC:

FYI, RFC is a state, not a version so this should be v2.

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

Re: [PATCH 1/6] dt-bindings: iio: adc: add ADI ad4030, ad4630 and ad4632

[Conor Dooley]

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

On Thu, Aug 22, 2024 at 02:45:17PM +0200, Esteban Blanc wrote:
> This adds a binding specification for the Analog Devices Inc. AD4030,
> AD4630 and AD4632 families of ADCs.
> 
> - ad4030-24 is a 1 channel SAR ADC with 24 bits of precision and a
>   sampling rate of 2M samples per second
> - ad4630-16 is a 2 channels SAR ADC with 16 bits of precision and a
>   sampling rate of 2M samples per second
> - ad4630-24 is a 2 channels SAR ADC with 24 bits of precision and a
>   sampling rate of 2M samples per second
> - ad4632-16 is a 2 channels SAR ADC with 16 bits of precision and a
>   sampling rate of 500K samples per second
> - ad4632-24 is a 2 channels SAR ADC with 24 bits of precision and a
>   sampling rate of 500K samples per second
> 
> Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad4030.yaml    | 113 +++++++++++++++++++++
>  MAINTAINERS                                        |   8 ++
>  2 files changed, 121 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
> new file mode 100644
> index 000000000000..7957c0c0ac7a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
> @@ -0,0 +1,113 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2024 Analog Devices Inc.
> +# Copyright 2024 BayLibre, SAS.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4030.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD4030 and AD4630 ADC family device driver
> +
> +maintainers:
> +  - Nuno Sa <nuno.sa@analog.com>
> +  - Michael Hennerich <michael.hennerich@analog.com>

This doesn't match the MAINTAINERS file entry below FYI.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

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

Re: [PATCH 2/6] iio: adc: ad4030: add driver for ad4030-24

[David Lechner]

On 8/22/24 7:45 AM, Esteban Blanc wrote:
> This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> 
> The driver implements basic support for the AD4030-24 1 channel
> differential ADC with hardware gain and offset control.
> 
> Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> ---
>  MAINTAINERS              |   1 +
>  drivers/iio/adc/Kconfig  |  13 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad4030.c | 854 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 869 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f17c42bea19c..6a5a0e7b7a51 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -420,6 +420,7 @@ R:	Esteban Blanc <eblanc@baylibre.com>
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
> +F:	drivers/iio/adc/ad4030.c
>  
>  AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER
>  M:	Mugilraj Dhavachelvan <dmugil2000@gmail.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 88e8ce2e78b3..f4bd05780f6f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -33,6 +33,19 @@ config AD4000
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad4000.
>  
> +config AD4030
> +	tristate "Analog Device AD4630 ADC Driver"
> +	depends on SPI
> +	depends on GPIOLIB
> +	select REGMAP_SPI

It looks like we are just using REGMAP, not REGMAP_SPI.

> +	select IIO_BUFFER

And also select IIO_TRIGGERED_BUFFER?

> +	help
> +	  Say yes here to build support for Analog Devices AD4030 and AD4630 high speed
> +	  SPI analog to digital converters (ADC).
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ad4030.
> +
>  config AD4130
>  	tristate "Analog Device AD4130 ADC Driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 8b80664c6d6b..0e4f833abf0b 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -7,6 +7,7 @@
>  obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
>  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
>  obj-$(CONFIG_AD4000) += ad4000.o
> +obj-$(CONFIG_AD4030) += ad4030.o
>  obj-$(CONFIG_AD4130) += ad4130.o
>  obj-$(CONFIG_AD4695) += ad4695.o
>  obj-$(CONFIG_AD7091R) += ad7091r-base.o
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> new file mode 100644
> index 000000000000..a981dce988e5
> --- /dev/null
> +++ b/drivers/iio/adc/ad4030.c
> @@ -0,0 +1,854 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD4030 and AD4630 ADC family driver.
> + *
> + * Copyright 2024 Analog Devices, Inc.
> + * Copyright 2024 BayLibre, SAS
> + *
> + * based on code from:
> + *	Analog Devices, Inc.
> + *	  Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> + *	  Nuno Sa <nuno.sa@analog.com>
> + *	  Marcelo Schmitt <marcelo.schmitt@analog.com>
> + *	  Liviu Adace <liviu.adace@analog.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/units.h>

alphabetical order?

> +#include <linux/clk.h>
> +#include <linux/spi/spi.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define AD4030_REG_INTERFACE_CONFIG_A			0x00
> +#define     AD4030_REG_INTERFACE_CONFIG_A_SW_RESET	(BIT(0) | BIT(7))
> +#define AD4030_REG_INTERFACE_CONFIG_B			0x01
> +#define AD4030_REG_DEVICE_CONFIG			0x02
> +#define AD4030_REG_CHIP_TYPE				0x03
> +#define AD4030_REG_PRODUCT_ID_L				0x04
> +#define AD4030_REG_PRODUCT_ID_H				0x05
> +#define AD4030_REG_CHIP_GRADE				0x06
> +#define     AD4030_REG_CHIP_GRADE_AD4030_24_GRADE	0x10
> +#define     AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE	GENMASK(7, 3)
> +#define AD4030_REG_SCRATCH_PAD			0x0A
> +#define AD4030_REG_SPI_REVISION			0x0B
> +#define AD4030_REG_VENDOR_L			0x0C
> +#define AD4030_REG_VENDOR_H			0x0D
> +#define AD4030_REG_STREAM_MODE			0x0E
> +#define AD4030_REG_INTERFACE_CONFIG_C		0x10
> +#define AD4030_REG_INTERFACE_STATUS_A		0x11
> +#define AD4030_REG_EXIT_CFG_MODE		0x14
> +#define     AD4030_REG_EXIT_CFG_MODE_EXIT_MSK	BIT(0)
> +#define AD4030_REG_AVG				0x15
> +#define     AD4030_REG_AVG_MASK_AVG_SYNC	BIT(7)
> +#define     AD4030_REG_AVG_MASK_AVG_VAL		GENMASK(4, 0)
> +#define AD4030_REG_OFFSET_X0_0			0x16
> +#define AD4030_REG_OFFSET_X0_1			0x17
> +#define AD4030_REG_OFFSET_X0_2			0x18
> +#define AD4030_REG_OFFSET_X1_0			0x19
> +#define AD4030_REG_OFFSET_X1_1			0x1A
> +#define AD4030_REG_OFFSET_X1_2			0x1B
> +#define     AD4030_REG_OFFSET_BYTES_NB		3
> +#define     AD4030_REG_OFFSET_CHAN(ch)		(AD4030_REG_OFFSET_X0_2 +	\
> +						(AD4030_REG_OFFSET_BYTES_NB *	\
> +						(ch)))
> +#define AD4030_REG_GAIN_X0_LSB			0x1C
> +#define AD4030_REG_GAIN_X0_MSB			0x1D
> +#define AD4030_REG_GAIN_X1_LSB			0x1E
> +#define AD4030_REG_GAIN_X1_MSB			0x1F
> +#define     AD4030_REG_GAIN_MAX_GAIN		1999970
> +#define     AD4030_REG_GAIN_BYTES_NB		2
> +#define     AD4030_REG_GAIN_CHAN(ch)		(AD4030_REG_GAIN_X0_MSB +	\
> +						(AD4030_REG_GAIN_BYTES_NB *	\
> +						(ch)))
> +#define AD4030_REG_MODES			0x20
> +#define     AD4030_REG_MODES_MASK_OUT_DATA_MODE	GENMASK(2, 0)
> +#define     AD4030_REG_MODES_MASK_LANE_MODE	GENMASK(7, 6)
> +#define AD4030_REG_OSCILATOR			0x21
> +#define AD4030_REG_IO				0x22
> +#define     AD4030_REG_IO_MASK_IO2X		BIT(1)
> +#define AD4030_REG_PAT0				0x23
> +#define AD4030_REG_PAT1				0x24
> +#define AD4030_REG_PAT2				0x25
> +#define AD4030_REG_PAT3				0x26
> +#define AD4030_REG_DIG_DIAG			0x34
> +#define AD4030_REG_DIG_ERR			0x35
> +
> +/* Sequence starting with "1 0 1" to enable reg access */
> +#define AD4030_REG_ACCESS			0xa0

nite: for case-consistency: 0xA0

> +
> +#define AD4030_MAX_IIO_SAMPLE_SIZE_BUFFERED	BITS_TO_BYTES(64)
> +#define AD4030_MAX_HARDWARE_CHANNEL_NB		2
> +#define AD4030_MAX_IIO_CHANNEL_NB		5
> +#define AD4030_COMMON_BYTE_CHANNELS_FILTER	0xA
> +/*
> + * This accounts for for 1 sample per channel plus one u64 for the timestamp,
> + * aligned on a u64 boundary

Technically, soft timestamp is s64.

> + */
> +#define AD4030_MAXIMUM_RX_BUFFER_SIZE			\
> +	ALIGN(AD4030_MAX_IIO_SAMPLE_SIZE_BUFFERED *	\
> +	      AD4030_MAX_HARDWARE_CHANNEL_NB,		\
> +	      sizeof(u64)) + sizeof(u64)
> +
> +#define AD4030_VREF_MIN_UV		(4096 * MILLI)
> +#define AD4030_VREF_MAX_UV		(5000 * MILLI)
> +#define AD4030_VIO_THRESHOLD_UV		(1400 * MILLI)
> +
> +#define AD4030_SPI_MAX_XFER_LEN		8
> +#define AD4030_SPI_MAX_REG_XFER_SPEED	(80 * MEGA)
> +#define AD4030_TCNVH_NS			10
> +#define AD4030_TCNVL_NS			20
> +#define AD4030_TCYC_NS			500
> +#define AD4030_TCYC_ADJUSTED_NS		(AD4030_TCYC_NS - AD4030_TCNVL_NS)
> +#define AD4030_TRESET_PW_NS		50
> +#define AD4030_TRESET_COM_DELAY_MS	750
> +
> +enum ad4030_out_mode {
> +	AD4030_OUT_DATA_MD_16_DIFF = 0x00,
> +	AD4030_OUT_DATA_MD_24_DIFF = 0x00,
> +	AD4030_OUT_DATA_MD_16_DIFF_8_COM = 0x01,
> +	AD4030_OUT_DATA_MD_24_DIFF_8_COM = 0x02,
> +	AD4030_OUT_DATA_MD_30_AVERAGED_DIFF = 0x03,
> +	AD4030_OUT_DATA_MD_32_PATTERN = 0x04
> +};
> +
> +struct ad4030_chip_info {
> +	const char *name;
> +	const unsigned long *available_masks;
> +	unsigned int available_masks_len;
> +	const struct iio_chan_spec channels[AD4030_MAX_IIO_CHANNEL_NB];> +	u8 grade;
> +	u8 precision_bits;
> +	/* Number of hardware channels */
> +	int num_channels;
> +	unsigned int tcyc;

Always nice to have the units in the variable name: tcyc_ns

> +};
> +
> +struct ad4030_state {
> +	struct spi_device *spi;
> +	struct regmap *regmap;
> +	const struct ad4030_chip_info *chip;
> +	struct gpio_desc *cnv_gpio;
> +	int vref_uv;
> +	int vio_uv;
> +	int offset_avail[3];
> +	u32 conversion_speed_hz;
> +	enum ad4030_out_mode mode;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the transfer buffers
> +	 * to live in their own cache lines.
> +	 */
> +	u8 tx_data[AD4030_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> +	struct {
> +		union {
> +			u8 raw[AD4030_MAXIMUM_RX_BUFFER_SIZE];
> +			struct {
> +				s32 val;
> +				u32 common;
> +			} __packed buffered[AD4030_MAX_HARDWARE_CHANNEL_NB];

__packed isn't going to make a difference here since everything is 32-bit.

> +		};
> +	} rx_data __aligned(IIO_DMA_MINALIGN);
> +};
> +
> +#define AD4030_CHAN_CMO(_idx)  {					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\

Datasheet says LSB for the 8-bit common mode output value is Vref/256
so we also need IIO_CHAN_INFO_SCALE here.

> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _idx * 2 + 2,					\
> +	.scan_index = _idx * 2 + 1,					\
> +	.extend_name = "Channel" #_idx " common byte part",		\

Labels are usually one word and reflect the datasheet name.

Suggest `"common-mode" #_idx` or `"CM" #_idx` for this one.

> +	.scan_type = {							\
> +		.sign = 'u',						\
> +		.storagebits = 32,					\

Using SPI offload is going to require different channel info structs
anyway since we will need a samping frequency attribute and there won't
be a soft timestamp.

So I think it would be OK to make storagebits 8 here. And we can make it
whatever it needs to be in the new struct when we actually add SPI
offload support.

> +		.realbits = 8,						\
> +		.endianness = IIO_BE,					\
> +	},								\
> +}
> +
> +#define AD4030_CHAN_IN(_idx, _storage, _real, _shift) {			\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),		\

As mentioned above scale should be info_mask_separate.

> +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
> +		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
> +		BIT(IIO_CHAN_INFO_RAW),					\
> +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
> +		BIT(IIO_CHAN_INFO_CALIBSCALE),				\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _idx * 2,						\
> +	.channel2 = _idx * 2 + 1,					\
> +	.scan_index = _idx * 2,						\
> +	.extend_name = "Channel" #_idx " differential part",		\

As above, suggest `"IN" #_idx` for this.

> +	.differential = true,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.storagebits = _storage,				\
> +		.realbits = _real,					\
> +		.shift = _shift,					\
> +		.endianness = IIO_BE,					\
> +	},								\
> +}
> +
> +static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
> +			   void *val, size_t val_size)
> +{
> +	struct ad4030_state *st = context;
> +
> +	struct spi_transfer xfer = {
> +		.tx_buf = st->tx_data,
> +		.rx_buf = st->rx_data.raw,
> +		.len = reg_size + val_size,
> +	};
> +	int ret;
> +
> +	memcpy(st->tx_data, reg, reg_size);
> +
> +	/*
> +	 * This should use spi_write_the_read but when doing so, CS never get
> +	 * deasserted.
> +	 */
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	memcpy(val, &st->rx_data.raw[reg_size], val_size);
> +
> +	return ret;
> +}
> +
> +static int ad4030_spi_write(void *context, const void *data, size_t count)
> +{
> +	const struct ad4030_state *st = context;
> +
> +	return spi_write(st->spi, data, count);
> +}
> +
> +static const struct regmap_bus ad4030_regmap_bus = {
> +	.read = ad4030_spi_read,
> +	.write = ad4030_spi_write,
> +	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
> +};
> +
> +static const struct regmap_range ad4030_regmap_rd_range[] = {
> +	regmap_reg_range(AD4030_REG_INTERFACE_CONFIG_A, AD4030_REG_CHIP_GRADE),
> +	regmap_reg_range(AD4030_REG_SCRATCH_PAD, AD4030_REG_STREAM_MODE),
> +	regmap_reg_range(AD4030_REG_INTERFACE_CONFIG_C, AD4030_REG_INTERFACE_STATUS_A),
> +	regmap_reg_range(AD4030_REG_EXIT_CFG_MODE, AD4030_REG_PAT3),
> +	regmap_reg_range(AD4030_REG_DIG_DIAG, AD4030_REG_DIG_ERR),
> +};
> +
> +static const struct regmap_range ad4030_regmap_wr_range[] = {
> +	regmap_reg_range(AD4030_REG_CHIP_TYPE, AD4030_REG_CHIP_GRADE),
> +	regmap_reg_range(AD4030_REG_SPI_REVISION, AD4030_REG_VENDOR_H),
> +};
> +
> +static const struct regmap_access_table ad4030_regmap_rd_table = {
> +	.yes_ranges = ad4030_regmap_rd_range,
> +	.n_yes_ranges = ARRAY_SIZE(ad4030_regmap_rd_range),
> +};
> +
> +static const struct regmap_access_table ad4030_regmap_wr_table = {
> +	.no_ranges = ad4030_regmap_wr_range,
> +	.n_no_ranges = ARRAY_SIZE(ad4030_regmap_wr_range),
> +};
> +
> +static const struct regmap_config ad4030_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.read_flag_mask = 0x80,
> +	.rd_table = &ad4030_regmap_rd_table,
> +	.wr_table = &ad4030_regmap_wr_table,
> +	.max_register = AD4030_REG_DIG_ERR,
> +};
> +
> +static int ad4030_enter_config_mode(struct ad4030_state *st)
> +{
> +	st->rx_data.raw[0] = AD4030_REG_ACCESS;
> +
> +	return spi_write(st->spi, st->rx_data.raw, 1);
> +}
> +
> +static int ad4030_exit_config_mode(struct ad4030_state *st)
> +{
> +	return regmap_write(st->regmap, AD4030_REG_EXIT_CFG_MODE,
> +		     AD4030_REG_EXIT_CFG_MODE_EXIT_MSK);
> +}
> +
> +static int ad4030_get_chan_gain(struct iio_dev *indio_dev, int ch, int *val,
> +				int *val2)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	u16 gain;
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4030_REG_GAIN_CHAN(ch),
> +			       st->rx_data.raw, AD4030_REG_GAIN_BYTES_NB);
> +	if (ret)
> +		return ret;
> +
> +	gain = be16_to_cpu(*(u16 *)st->rx_data.raw);
> +
> +	/* From datasheet: multiplied output = input × gain word/0x8000 */
> +	*val = gain / 0x8000;
> +	*val2 = mul_u64_u32_div(gain % 0x8000, MICRO, 0x8000);
> +
> +	return 0;
> +}
> +
> +/*
> + * @brief Returns the offset where 1 LSB = (VREF/2^precision_bits - 1)/gain
> + */
> +static int ad4030_get_chan_offset(struct iio_dev *indio_dev, int ch, int *val)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4030_REG_OFFSET_CHAN(ch),
> +			       st->rx_data.raw, AD4030_REG_OFFSET_BYTES_NB);
> +	if (ret)
> +		return ret;
> +
> +	switch (st->chip->precision_bits) {
> +	case 16:
> +		*val = sign_extend32(get_unaligned_be16(st->rx_data.raw), 15);
> +		break;
> +
> +	case 24:
> +		*val = sign_extend32(get_unaligned_be24(st->rx_data.raw), 23);
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int gain_int,
> +				int gain_frac)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	__be16 *val = (__be16 *)st->rx_data.raw;
> +	u64 gain;

Should check for negative values here (have to check both gain_int and gain_val
due to the way IIO_VAL_INT_PLUS_MICRO works). Otherwise negative numbers will
be turned positive in the unsigned multiply below.

	if (gain_int < 0 || gain_frac < 0)
		return -EINVAL;

> +
> +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> +
> +	if (gain > AD4030_REG_GAIN_MAX_GAIN)
> +		return -EINVAL;
> +
> +	*val = cpu_to_be16(DIV_ROUND_CLOSEST_ULL(gain * 0x8000, MICRO));
> +
> +	return regmap_bulk_write(st->regmap, AD4030_REG_GAIN_CHAN(ch), val,
> +			  AD4030_REG_GAIN_BYTES_NB);
> +}
> +
> +static int ad4030_set_chan_offset(struct iio_dev *indio_dev, int ch, int offset)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	if (offset < st->offset_avail[0] || offset > st->offset_avail[2])
> +		return -EINVAL;
> +
> +	switch (st->chip->precision_bits) {
> +	case 16:
> +		put_unaligned_be16(offset, st->rx_data.raw);
> +		break;
> +
> +	case 24:
> +		put_unaligned_be24(offset, st->rx_data.raw);
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return regmap_bulk_write(st->regmap, AD4030_REG_OFFSET_CHAN(ch),
> +			  st->rx_data.raw, AD4030_REG_OFFSET_BYTES_NB);
> +}
> +
> +static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
> +					unsigned int mask)
> +{
> +	/* Common byte channel is after the "real" differential sample channel */
> +	return mask & AD4030_COMMON_BYTE_CHANNELS_FILTER;
> +}
> +
> +static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	if (ad4030_is_common_byte_asked(st, mask))
> +		st->mode = AD4030_OUT_DATA_MD_24_DIFF_8_COM;
> +	else
> +		st->mode = AD4030_OUT_DATA_MD_24_DIFF;
> +
> +	return regmap_update_bits(st->regmap, AD4030_REG_MODES,
> +				AD4030_REG_MODES_MASK_OUT_DATA_MODE,
> +				st->mode);
> +}
> +
> +static int ad4030_conversion(struct ad4030_state *st,
> +			     const struct iio_chan_spec *chan)
> +{
> +	unsigned int bytes_to_read;
> +	unsigned char byte_index;
> +	unsigned int i;
> +	int ret;
> +
> +	/* Number of bytes for one differential channel */
> +	bytes_to_read = BITS_TO_BYTES(chan->scan_type.realbits);
> +	/* Add one byte if we are using a differential + common byte mode */
> +	bytes_to_read += (st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM ||
> +			st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM) ? 1 : 0;
> +	/* Mulitiply by the number of hardware channels */
> +	bytes_to_read *= st->chip->num_channels;
> +
> +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> +	ndelay(AD4030_TCNVH_NS);
> +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +	ndelay(st->chip->tcyc);
> +
> +	ret = spi_read(st->spi, st->rx_data.raw, bytes_to_read);
> +	if (ret)
> +		return ret;
> +
> +	if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> +		return 0;
> +
> +	byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
> +	for (i = 0; i < st->chip->num_channels; i++)
> +		st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
> +
> +	return 0;
> +}
> +
> +static int ad4030_single_conversion(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan, int *val)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4030_set_mode(indio_dev, BIT(chan->channel));
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_exit_config_mode(st);
> +	if (ret)
> +		goto out_exit_config_mode_error;

Looks like we could just return ret here.

> +
> +	ret = ad4030_conversion(st, chan);
> +	if (ret)
> +		goto out_error;
> +
> +	if (chan->channel % 2)
> +		*val = st->rx_data.buffered[chan->channel / 2].common;
> +	else
> +		*val = st->rx_data.buffered[chan->channel / 2].val;
> +
> +out_error:
> +	ad4030_enter_config_mode(st);
> +
> +out_exit_config_mode_error:
> +
> +	if (ret)
> +		return ret;
> +
> +	return IIO_VAL_INT;

This can be moved before out_error:, then we can just have
return ret here and leave out the if.

> +}
> +
> +static irqreturn_t ad4030_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4030_conversion(st, indio_dev->channels);
> +	if (ret)
> +		goto out;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->rx_data.raw,
> +					   pf->timestamp);
> +
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const int *ad4030_get_offset_avail(struct ad4030_state *st)
> +{
> +	return st->offset_avail;
> +}
> +
> +static const int ad4030_gain_avail[3][2] = {
> +	{ 0, 0 },
> +	{ 0, 30 },
> +	{ 1, 999969 },
> +};

Could use IIO_VAL_FRACTIONAL_LOG2 or IIO_VAL_INT_PLUS_NANO here
to get more precise values. Actual range is: [0 1/2^15 U16_MAX/2^15]

To 9 decimal places: [0.000000000 0.000030517 1.999969482]

> +
> +static int ad4030_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *channel,
> +			     const int **vals, int *type,
> +			     int *length, long mask)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		*vals = ad4030_get_offset_avail(st);
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_RANGE;
> +
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		*vals = (void *)ad4030_gain_avail;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_RANGE;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4030_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long info)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +		switch (info) {
> +		case IIO_CHAN_INFO_RAW:
> +			return ad4030_single_conversion(indio_dev, chan, val);
> +
> +		case IIO_CHAN_INFO_SCALE:
> +			*val = (st->vref_uv * 2) / MILLI;
> +			*val2 = st->chip->precision_bits;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +
> +		case IIO_CHAN_INFO_CALIBSCALE:
> +			ret = ad4030_get_chan_gain(indio_dev, chan->channel,
> +						   val, val2);
> +			if (ret)
> +				return ret;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +
> +		case IIO_CHAN_INFO_CALIBBIAS:
> +			ret = ad4030_get_chan_offset(indio_dev, chan->channel,
> +						     val);
> +			if (ret)
> +				return ret;
> +			return IIO_VAL_INT;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	unreachable();
> +}
> +
> +static int ad4030_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val,
> +			    int val2, long info)
> +{
> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +		switch (info) {
> +		case IIO_CHAN_INFO_CALIBSCALE:
> +			return ad4030_set_chan_gain(indio_dev, chan->channel,
> +						    val, val2);
> +
> +		case IIO_CHAN_INFO_CALIBBIAS:
> +			return ad4030_set_chan_offset(indio_dev, chan->channel,
> +						      val);

Need to add .write_raw_get_fmt to struct iio_info below to set
IIO_CHAN_INFO_CALIBBIAS to IIO_VAL_INT. Othwerwise, the defualt
IIO_VAL_INT_PLUS_MICRO is used and val2 would have considered
for handling negative values.

> +
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	unreachable();
> +}
> +
> +static int ad4030_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +			     unsigned int writeval, unsigned int *readval)
> +{
> +	const struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +		if (readval)
> +			return regmap_read(st->regmap, reg, readval);
> +
> +		return regmap_write(st->regmap, reg, writeval);
> +	}
> +
> +	unreachable();
> +}
> +
> +static const struct iio_info ad4030_iio_info = {
> +	.read_avail = ad4030_read_avail,
> +	.read_raw = ad4030_read_raw,
> +	.write_raw = ad4030_write_raw,
> +	.debugfs_reg_access = ad4030_reg_access,
> +};
> +
> +static int ad4030_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4030_set_mode(indio_dev, *indio_dev->active_scan_mask);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_exit_config_mode(st);
> +	if (ret)
> +		return ret;
> +
> +	/* Restore SPI max speed for conversion */
> +	st->spi->max_speed_hz = st->conversion_speed_hz;
> +
> +	return 0;
> +}
> +
> +static int ad4030_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	/* Make sure the SPI clock is within range to read register */
> +	st->spi->max_speed_hz = min(st->spi->max_speed_hz,
> +				    AD4030_SPI_MAX_REG_XFER_SPEED);
> +
> +	return ad4030_enter_config_mode(st);
> +}
> +
> +static const struct iio_buffer_setup_ops ad4030_buffer_setup_ops = {
> +	.preenable = ad4030_buffer_preenable,
> +	.postdisable = ad4030_buffer_postdisable,
> +};
> +
> +static int ad4030_regulators_get(struct ad4030_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	static const char * const ids[] = { "vdd-5v", "vdd-1v8" };
> +	int ret;
> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ids), ids);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> +
> +	st->vio_uv = devm_regulator_get_enable_read_voltage(dev, "vio");
> +	if (st->vio_uv < 0)
> +		return dev_err_probe(dev, st->vio_uv,
> +				     "Failed to enable and read vio voltage\n");
> +
> +	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (st->vref_uv < 0) {
> +		if (st->vref_uv != -ENODEV)
> +			return dev_err_probe(dev, st->vref_uv,
> +					     "Failed to read vref voltage\n");
> +
> +		/* if not using optional REF, the internal REFIN must be used */
> +		st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "refin");
> +		if (st->vref_uv < 0)
> +			return dev_err_probe(dev, st->vref_uv,
> +					     "Failed to read vrefin voltage\n");
> +	}
> +
> +	if (st->vref_uv < AD4030_VREF_MIN_UV || st->vref_uv > AD4030_VREF_MAX_UV)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "vref(%d) must be in the range [%lu %lu]\n",
> +				     st->vref_uv, AD4030_VREF_MIN_UV,
> +				     AD4030_VREF_MAX_UV);
> +
> +	return 0;
> +}
> +
> +static int ad4030_reset(struct ad4030_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	struct gpio_desc *reset;
> +	int ret;
> +
> +	/* Use GPIO if available ... */
> +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset))
> +		return dev_err_probe(dev, PTR_ERR(reset),
> +				"Failed to get reset GPIO\n");
> +
> +	if (reset) {
> +		ndelay(50);
> +		gpiod_set_value_cansleep(reset, 0);
> +	} else {
> +		/* ... falback to software reset otherwise */
> +		ret = ad4030_enter_config_mode(st);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(st->regmap, AD4030_REG_INTERFACE_CONFIG_A,
> +				   AD4030_REG_INTERFACE_CONFIG_A_SW_RESET);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Wait for reset to complete before communicating to it */
> +	fsleep(AD4030_TRESET_COM_DELAY_MS);

Should be renamed to AD4030_TRESET_COM_DELAY_US since it is microseconds.

> +
> +	/* After reset, conversion mode is enabled. Switch to reg access */
> +	return ad4030_enter_config_mode(st);
> +}
> +
> +static int ad4030_detect_chip_info(const struct ad4030_state *st)
> +{
> +	unsigned int grade;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
> +	if (ret)
> +		return ret;
> +
> +	grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
> +	if (grade != st->chip->grade)
> +		return dev_err_probe(&st->spi->dev, -EINVAL,
> +					"Unknown grade(0x%x) for %s\n", grade,
> +					st->chip->name);
> +
> +	return 0;
> +}
> +
> +static int ad4030_config(struct ad4030_state *st)
> +{
> +	st->offset_avail[0] = (int)BIT(st->chip->precision_bits - 1) * -1;
> +	st->offset_avail[1] = 1;
> +	st->offset_avail[2] = BIT(st->chip->precision_bits - 1) - 1;
> +
> +	if (st->vio_uv < AD4030_VIO_THRESHOLD_UV)
> +		return regmap_write(st->regmap, AD4030_REG_IO,
> +				AD4030_REG_IO_MASK_IO2X);
> +
> +	return 0;
> +}
> +
> +static int ad4030_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad4030_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	/* Make sure the SPI clock is within range to read register */
> +	st->conversion_speed_hz = min(spi->max_speed_hz,
> +				      AD4030_SPI_MAX_REG_XFER_SPEED);
> +
> +	st->regmap = devm_regmap_init(dev, &ad4030_regmap_bus, st,
> +				      &ad4030_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		dev_err_probe(dev, PTR_ERR(st->regmap),
> +			      "Failed to initialize regmap\n");
> +
> +	st->chip = spi_get_device_match_data(spi);
> +	if (!st->chip)
> +		return -EINVAL;
> +
> +	ret = ad4030_regulators_get(st);
> +	if (ret)
> +		return ret;

Datasheet says:

	Perform a reset no sooner than 3 ms after the power
	supplies are valid and stable

So might be a good idea to have mdelay(3) here.

> +
> +	ret = ad4030_reset(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_detect_chip_info(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_config(st);
> +	if (ret)
> +		return ret;
> +
> +	st->cnv_gpio = devm_gpiod_get(dev, "cnv", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->cnv_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
> +				     "Failed to get cnv gpio\n");
> +
> +	/*
> +	 * One hardware channel is split in two software channels when using
> +	 * common byte mode. Add one more channel for the timestamp.
> +	 */
> +	indio_dev->num_channels = 2 * st->chip->num_channels + 1;

Might make more sense to rename chip->num_channels to num_voltage_inputs
or something like that to avoid confusion.

> +	indio_dev->name = st->chip->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ad4030_iio_info;
> +	indio_dev->channels = st->chip->channels;
> +	indio_dev->available_scan_masks = st->chip->available_masks;
> +	indio_dev->masklength = st->chip->available_masks_len;

indio_dev->masklengh is marked as [INTERN] so should not be set by drivers.

> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      ad4030_trigger_handler,
> +					      &ad4030_buffer_setup_ops);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const unsigned long ad4030_channel_masks[] = {
> +	/* Differential only */
> +	BIT(0),
> +	/* Differential with common byte */

Suggest: /* Differential and common-mode voltage */

> +	GENMASK(1, 0),
> +	0,
> +};
> +
> +static const struct ad4030_chip_info ad4030_24_chip_info = {
> +	.name = "ad4030-24",
> +	.available_masks = ad4030_channel_masks,
> +	.available_masks_len = ARRAY_SIZE(ad4030_channel_masks),
> +	.channels = {
> +		AD4030_CHAN_IN(0, 32, 24, 8),
> +		AD4030_CHAN_CMO(0),
> +		IIO_CHAN_SOFT_TIMESTAMP(2),
> +	},
> +	.grade = AD4030_REG_CHIP_GRADE_AD4030_24_GRADE,
> +	.precision_bits = 24,
> +	.num_channels = 1,
> +	.tcyc = AD4030_TCYC_ADJUSTED_NS,
> +};
> +
> +static const struct spi_device_id ad4030_id_table[] = {
> +	{ "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, ad4030_id_table);
> +
> +static const struct of_device_id ad4030_of_match[] = {
> +	{ .compatible = "adi,ad4030-24", .data = &ad4030_24_chip_info },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ad4030_of_match);
> +
> +static struct spi_driver ad4030_driver = {
> +	.driver = {
> +		.name = "ad4030",
> +		.of_match_table = ad4030_of_match,
> +	},
> +	.probe = ad4030_probe,
> +	.id_table = ad4030_id_table,
> +};
> +module_spi_driver(ad4030_driver);
> +
> +MODULE_AUTHOR("Esteban Blanc <eblanc@baylibre.com>");
> +MODULE_DESCRIPTION("Analog Devices AD4630 ADC family driver");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 3/6] iio: adc: ad4030: add averaging support

[David Lechner]

On 8/22/24 7:45 AM, Esteban Blanc wrote:
> This add support for the averaging mode of AD4030 using oversampling IIO
> attribute
> 
> Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> ---
>  drivers/iio/adc/ad4030.c | 126 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 111 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> index a981dce988e5..e1e1dbf0565c 100644
> --- a/drivers/iio/adc/ad4030.c
> +++ b/drivers/iio/adc/ad4030.c
> @@ -115,6 +115,18 @@ enum ad4030_out_mode {
>  	AD4030_OUT_DATA_MD_32_PATTERN = 0x04
>  };
>  
> +enum {
> +	AD4030_LANE_MD_1_PER_CH,
> +	AD4030_LANE_MD_2_PER_CH,
> +	AD4030_LANE_MD_4_PER_CH,
> +	AD4030_LANE_MD_INTERLEAVED = 0b11,

nit: value 0f AD4030_LANE_MD_INTERLEAVED is already 3, so
shouldn't need to specify it.

> +};
> +
> +enum {
> +	AD4030_SCAN_TYPE_NORMAL,
> +	AD4030_SCAN_TYPE_AVG,
> +};
> +
>  struct ad4030_chip_info {
>  	const char *name;
>  	const unsigned long *available_masks;
> @@ -135,6 +147,7 @@ struct ad4030_state {
>  	int vref_uv;
>  	int vio_uv;
>  	int offset_avail[3];
> +	unsigned int avg_len;
>  	u32 conversion_speed_hz;
>  	enum ad4030_out_mode mode;
>  
> @@ -169,8 +182,11 @@ struct ad4030_state {
>  	},								\
>  }
>  
> -#define AD4030_CHAN_IN(_idx, _storage, _real, _shift) {			\
> -	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),		\
> +#define AD4030_CHAN_IN(_idx, _scan_type) {				\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE) |		\
> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
> +	.info_mask_shared_by_all_available =				\
> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
>  		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
>  		BIT(IIO_CHAN_INFO_RAW),					\
> @@ -183,15 +199,16 @@ struct ad4030_state {
>  	.scan_index = _idx * 2,						\
>  	.extend_name = "Channel" #_idx " differential part",		\
>  	.differential = true,						\
> -	.scan_type = {							\
> -		.sign = 's',						\
> -		.storagebits = _storage,				\
> -		.realbits = _real,					\
> -		.shift = _shift,					\
> -		.endianness = IIO_BE,					\
> -	},								\
> +	.has_ext_scan_type = 1,						\
> +	.ext_scan_type = _scan_type,					\
> +	.num_ext_scan_type = ARRAY_SIZE(_scan_type),			\
>  }
>  
> +static const int ad4030_average_modes[] = {
> +	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192, 16384,
> +	32768, 65536
> +};
> +
>  static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
>  			   void *val, size_t val_size)
>  {
> @@ -327,6 +344,13 @@ static int ad4030_get_chan_offset(struct iio_dev *indio_dev, int ch, int *val)
>  	return 0;
>  }
>  
> +static int ad4030_get_avg_frame_len(struct iio_dev *dev)
> +{
> +	const struct ad4030_state *st = iio_priv(dev);
> +
> +	return 1L << st->avg_len;
> +}
> +
>  static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int gain_int,
>  				int gain_frac)
>  {
> @@ -369,6 +393,22 @@ static int ad4030_set_chan_offset(struct iio_dev *indio_dev, int ch, int offset)
>  			  st->rx_data.raw, AD4030_REG_OFFSET_BYTES_NB);
>  }
>  
> +static int ad4030_set_avg_frame_len(struct iio_dev *dev, unsigned int avg_len)
> +{
> +	struct ad4030_state *st = iio_priv(dev);
> +	unsigned int avg_val = ilog2(avg_len);
> +	int ret;

Should probably limit avg_len max to 65536 to avoid weird roll
around when higher values are given.

> +
> +	ret = regmap_write(st->regmap, AD4030_REG_AVG, AD4030_REG_AVG_MASK_AVG_SYNC |
> +		    FIELD_PREP(AD4030_REG_AVG_MASK_AVG_VAL, avg_val));
> +	if (ret)
> +		return ret;
> +
> +	st->avg_len = avg_val;
> +
> +	return 0;
> +}
> +
>  static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
>  					unsigned int mask)
>  {
> @@ -380,7 +420,9 @@ static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
>  {
>  	struct ad4030_state *st = iio_priv(indio_dev);
>  
> -	if (ad4030_is_common_byte_asked(st, mask))
> +	if (st->avg_len)
> +		st->mode = AD4030_OUT_DATA_MD_30_AVERAGED_DIFF;
> +	else if (ad4030_is_common_byte_asked(st, mask))
>  		st->mode = AD4030_OUT_DATA_MD_24_DIFF_8_COM;
>  	else
>  		st->mode = AD4030_OUT_DATA_MD_24_DIFF;
> @@ -394,6 +436,7 @@ static int ad4030_conversion(struct ad4030_state *st,
>  			     const struct iio_chan_spec *chan)
>  {
>  	unsigned int bytes_to_read;
> +	unsigned long cnv_nb = 1UL << st->avg_len;
>  	unsigned char byte_index;
>  	unsigned int i;
>  	int ret;
> @@ -406,10 +449,12 @@ static int ad4030_conversion(struct ad4030_state *st,
>  	/* Mulitiply by the number of hardware channels */
>  	bytes_to_read *= st->chip->num_channels;
>  
> -	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> -	ndelay(AD4030_TCNVH_NS);
> -	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> -	ndelay(st->chip->tcyc);
> +	for (i = 0; i < cnv_nb; i++) {
> +		gpiod_set_value_cansleep(st->cnv_gpio, 1);
> +		ndelay(AD4030_TCNVH_NS);
> +		gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +		ndelay(st->chip->tcyc);
> +	}
>  
>  	ret = spi_read(st->spi, st->rx_data.raw, bytes_to_read);
>  	if (ret)
> @@ -508,6 +553,12 @@ static int ad4030_read_avail(struct iio_dev *indio_dev,
>  		*type = IIO_VAL_INT_PLUS_MICRO;
>  		return IIO_AVAIL_RANGE;
>  
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = ad4030_average_modes;
> +		*type = IIO_VAL_INT;
> +		*length = ARRAY_SIZE(ad4030_average_modes);
> +		return IIO_AVAIL_LIST;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -544,6 +595,10 @@ static int ad4030_read_raw(struct iio_dev *indio_dev,
>  				return ret;
>  			return IIO_VAL_INT;
>  
> +		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +			*val = ad4030_get_avg_frame_len(indio_dev);
> +			return IIO_VAL_INT;
> +
>  		default:
>  			return -EINVAL;
>  		}
> @@ -566,6 +621,9 @@ static int ad4030_write_raw(struct iio_dev *indio_dev,
>  			return ad4030_set_chan_offset(indio_dev, chan->channel,
>  						      val);
>  
> +		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:

Might want to check for negative values before implicitly casting val
to unsigned.

> +			return ad4030_set_avg_frame_len(indio_dev, val);
> +
>  		default:
>  			return -EINVAL;
>  		}
> @@ -589,11 +647,20 @@ static int ad4030_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>  	unreachable();
>  }
>  
> +static int ad4030_get_current_scan_type(const struct iio_dev *indio_dev,
> +					const struct iio_chan_spec *chan)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	return st->avg_len ? AD4030_SCAN_TYPE_AVG : AD4030_SCAN_TYPE_NORMAL;
> +}
> +
>  static const struct iio_info ad4030_iio_info = {
>  	.read_avail = ad4030_read_avail,
>  	.read_raw = ad4030_read_raw,
>  	.write_raw = ad4030_write_raw,
>  	.debugfs_reg_access = ad4030_reg_access,
> +	.get_current_scan_type = ad4030_get_current_scan_type,
>  };
>  
>  static int ad4030_buffer_preenable(struct iio_dev *indio_dev)
> @@ -626,9 +693,21 @@ static int ad4030_buffer_postdisable(struct iio_dev *indio_dev)
>  	return ad4030_enter_config_mode(st);
>  }
>  
> +static bool ad4030_validate_scan_mask(struct iio_dev *indio_dev, const unsigned long *scan_mask)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	/* Asking for both common channels and averaging */
> +	if (st->avg_len && ad4030_is_common_byte_asked(st, *scan_mask))
> +		return false;
> +
> +	return true;
> +}
> +
>  static const struct iio_buffer_setup_ops ad4030_buffer_setup_ops = {
>  	.preenable = ad4030_buffer_preenable,
>  	.postdisable = ad4030_buffer_postdisable,
> +	.validate_scan_mask = ad4030_validate_scan_mask,
>  };
>  
>  static int ad4030_regulators_get(struct ad4030_state *st)
> @@ -812,12 +891,29 @@ static const unsigned long ad4030_channel_masks[] = {
>  	0,
>  };
>  
> +static const struct iio_scan_type ad4030_24_scan_types[] = {
> +	[AD4030_SCAN_TYPE_NORMAL] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 24,
> +		.shift = 8,
> +		.endianness = IIO_BE,
> +	},
> +	[AD4030_SCAN_TYPE_AVG] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 30,
> +		.shift = 2,
> +		.endianness = IIO_BE,
> +	},
> +};
> +
>  static const struct ad4030_chip_info ad4030_24_chip_info = {
>  	.name = "ad4030-24",
>  	.available_masks = ad4030_channel_masks,
>  	.available_masks_len = ARRAY_SIZE(ad4030_channel_masks),
>  	.channels = {
> -		AD4030_CHAN_IN(0, 32, 24, 8),
> +		AD4030_CHAN_IN(0, ad4030_24_scan_types),
>  		AD4030_CHAN_CMO(0),
>  		IIO_CHAN_SOFT_TIMESTAMP(2),
>  	},
> 

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[David Lechner]

On 8/22/24 7:45 AM, Esteban Blanc wrote:
> AD4630-24 and AD4630-16 are 2 channels ADCs. Both channels are
> interleaved bit per bit on SDO line.
> 
> Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> ---
>  drivers/iio/adc/ad4030.c | 197 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 173 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> index e1e1dbf0565c..dbba5287b630 100644
> --- a/drivers/iio/adc/ad4030.c
> +++ b/drivers/iio/adc/ad4030.c
> @@ -32,6 +32,8 @@
>  #define AD4030_REG_PRODUCT_ID_H				0x05
>  #define AD4030_REG_CHIP_GRADE				0x06
>  #define     AD4030_REG_CHIP_GRADE_AD4030_24_GRADE	0x10
> +#define     AD4030_REG_CHIP_GRADE_AD4630_16_GRADE	0x03
> +#define     AD4030_REG_CHIP_GRADE_AD4630_24_GRADE	0x00
>  #define     AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE	GENMASK(7, 3)
>  #define AD4030_REG_SCRATCH_PAD			0x0A
>  #define AD4030_REG_SPI_REVISION			0x0B
> @@ -159,10 +161,14 @@ struct ad4030_state {
>  	struct {
>  		union {
>  			u8 raw[AD4030_MAXIMUM_RX_BUFFER_SIZE];
> -			struct {
> -				s32 val;
> -				u32 common;
> -			} __packed buffered[AD4030_MAX_HARDWARE_CHANNEL_NB];
> +			union {
> +				s32 diff[AD4030_MAX_HARDWARE_CHANNEL_NB];
> +				struct {
> +					s32 diff;
> +					u32 common;
> +				} __packed
> +				buffered_common[AD4030_MAX_HARDWARE_CHANNEL_NB];
> +			};
>  		};
>  	} rx_data __aligned(IIO_DMA_MINALIGN);
>  };
> @@ -171,7 +177,7 @@ struct ad4030_state {
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
>  	.type = IIO_VOLTAGE,						\
>  	.indexed = 1,							\
> -	.channel = _idx * 2 + 2,					\
> +	.channel = _idx * 3 + 2,					\

I'm guessing * 3 should have actually been in the first patch?
Seems odd to change it now.

>  	.scan_index = _idx * 2 + 1,					\
>  	.extend_name = "Channel" #_idx " common byte part",		\
>  	.scan_type = {							\
> @@ -194,8 +200,8 @@ struct ad4030_state {
>  		BIT(IIO_CHAN_INFO_CALIBSCALE),				\
>  	.type = IIO_VOLTAGE,						\
>  	.indexed = 1,							\
> -	.channel = _idx * 2,						\
> -	.channel2 = _idx * 2 + 1,					\
> +	.channel = _idx * 3,						\
> +	.channel2 = _idx * 3 + 1,					\
>  	.scan_index = _idx * 2,						\
>  	.extend_name = "Channel" #_idx " differential part",		\
>  	.differential = true,						\
> @@ -412,7 +418,7 @@ static int ad4030_set_avg_frame_len(struct iio_dev *dev, unsigned int avg_len)
>  static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
>  					unsigned int mask)
>  {
> -	/* Common byte channel is after the "real" differential sample channel */
> +	/* Common byte channels are after each differential channel */
>  	return mask & AD4030_COMMON_BYTE_CHANNELS_FILTER;
>  }
>  
> @@ -420,18 +426,69 @@ static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
>  {
>  	struct ad4030_state *st = iio_priv(indio_dev);
>  
> -	if (st->avg_len)
> +	if (st->avg_len) {
>  		st->mode = AD4030_OUT_DATA_MD_30_AVERAGED_DIFF;
> -	else if (ad4030_is_common_byte_asked(st, mask))
> -		st->mode = AD4030_OUT_DATA_MD_24_DIFF_8_COM;
> -	else
> +	} else if (ad4030_is_common_byte_asked(st, mask)) {
> +		switch (st->chip->precision_bits) {
> +		case 16:
> +			st->mode = AD4030_OUT_DATA_MD_16_DIFF_8_COM;
> +			break;
> +
> +		case 24:
> +			st->mode = AD4030_OUT_DATA_MD_24_DIFF_8_COM;
> +			break;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +	} else {
>  		st->mode = AD4030_OUT_DATA_MD_24_DIFF;

nit: maybe better to rename this one to AD4030_OUT_DATA_MD_DIFF
or AD4030_OUT_DATA_MD_DIFF_ONLY since it can be 16 or 24 bits
depending on the chip?

> +	}
>  
>  	return regmap_update_bits(st->regmap, AD4030_REG_MODES,
>  				AD4030_REG_MODES_MASK_OUT_DATA_MODE,
>  				st->mode);
>  }
>  
> +/*
> + * @brief Descramble 2 32bits numbers out of a 64bits. The bits are interleaved:
> + * 1 bit for first number, 1 bit for the second, and so on...
> + */
> +static void ad4030_extract_interleaved(u8 *src, u32 *ch0, u32 *ch1)
> +{
> +	u8 h0, h1, l0, l1;
> +	u32 out0, out1;
> +	u8 *out0_raw = (u8 *)&out0;
> +	u8 *out1_raw = (u8 *)&out1;
> +
> +	for (int i = 0; i < 4; i++) {
> +		h0 = src[i * 2];
> +		l1 = src[i * 2 + 1];
> +		h1 = h0 << 1;
> +		l0 = l1 >> 1;
> +
> +		h0 &= 0xAA;
> +		l0 &= 0x55;
> +		h1 &= 0xAA;
> +		l1 &= 0x55;
> +
> +		h0 = (h0 | h0 << 001) & 0xCC;
> +		h1 = (h1 | h1 << 001) & 0xCC;
> +		l0 = (l0 | l0 >> 001) & 0x33;
> +		l1 = (l1 | l1 >> 001) & 0x33;
> +		h0 = (h0 | h0 << 002) & 0xF0;
> +		h1 = (h1 | h1 << 002) & 0xF0;
> +		l0 = (l0 | l0 >> 002) & 0x0F;
> +		l1 = (l1 | l1 >> 002) & 0x0F;
> +
> +		out0_raw[i] = h0 | l0;
> +		out1_raw[i] = h1 | l1;
> +	}
> +
> +	*ch0 = out0;
> +	*ch1 = out1;
> +}
> +
>  static int ad4030_conversion(struct ad4030_state *st,
>  			     const struct iio_chan_spec *chan)
>  {
> @@ -460,12 +517,21 @@ static int ad4030_conversion(struct ad4030_state *st,
>  	if (ret)
>  		return ret;
>  
> -	if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> +	if (st->chip->num_channels == 2)
> +		ad4030_extract_interleaved(st->rx_data.raw,
> +					   &st->rx_data.diff[0],
> +					   &st->rx_data.diff[1]);
> +
> +	if (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> +	    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
>  		return 0;
>  
>  	byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
> -	for (i = 0; i < st->chip->num_channels; i++)
> -		st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
> +	/* Doing it backward to avoid overlap when reordering */
> +	for (i = st->chip->num_channels - 1; i > 0; i--) {
> +		st->rx_data.buffered_common[i].diff = st->rx_data.diff[i];
> +		st->rx_data.buffered_common[i].common = ((u8 *)&st->rx_data.diff[i])[byte_index];
> +	}
>  
>  	return 0;
>  }
> @@ -489,9 +555,9 @@ static int ad4030_single_conversion(struct iio_dev *indio_dev,
>  		goto out_error;
>  
>  	if (chan->channel % 2)
> -		*val = st->rx_data.buffered[chan->channel / 2].common;
> +		*val = st->rx_data.buffered_common[chan->channel / 2].common;
>  	else
> -		*val = st->rx_data.buffered[chan->channel / 2].val;
> +		*val = st->rx_data.diff[chan->channel / 2];

Doesn't this need to change since `* 2` was changed to `* 3` for the channel
value?

Better would be to make use of chan->address to store the actual number we need
and use that directly instead of reverse engineering the relation of chan->channel
to input pin number.

>  
>  out_error:
>  	ad4030_enter_config_mode(st);
> @@ -582,14 +648,17 @@ static int ad4030_read_raw(struct iio_dev *indio_dev,
>  			return IIO_VAL_FRACTIONAL_LOG2;
>  
>  		case IIO_CHAN_INFO_CALIBSCALE:
> -			ret = ad4030_get_chan_gain(indio_dev, chan->channel,
> -						   val, val2);
> +			ret = ad4030_get_chan_gain(indio_dev,
> +						   chan->scan_index / 2,
> +						   val,
> +						   val2);
>  			if (ret)
>  				return ret;
>  			return IIO_VAL_INT_PLUS_MICRO;
>  
>  		case IIO_CHAN_INFO_CALIBBIAS:
> -			ret = ad4030_get_chan_offset(indio_dev, chan->channel,
> +			ret = ad4030_get_chan_offset(indio_dev,
> +						     chan->scan_index / 2,

Similar to above, we could use chan->address here instead of having to
know the relationship between scan_index and input pin.

>  						     val);
>  			if (ret)
>  				return ret;
> @@ -614,11 +683,14 @@ static int ad4030_write_raw(struct iio_dev *indio_dev,
>  	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
>  		switch (info) {
>  		case IIO_CHAN_INFO_CALIBSCALE:
> -			return ad4030_set_chan_gain(indio_dev, chan->channel,
> -						    val, val2);
> +			return ad4030_set_chan_gain(indio_dev,
> +						    chan->scan_index / 2,
> +						    val,
> +						    val2);
>  
>  		case IIO_CHAN_INFO_CALIBBIAS:
> -			return ad4030_set_chan_offset(indio_dev, chan->channel,
> +			return ad4030_set_chan_offset(indio_dev,
> +						      chan->scan_index / 2,
>  						      val);
>  
>  		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> @@ -801,10 +873,24 @@ static int ad4030_detect_chip_info(const struct ad4030_state *st)
>  
>  static int ad4030_config(struct ad4030_state *st)
>  {
> +	int ret;
> +	u8 reg_modes;
> +
>  	st->offset_avail[0] = (int)BIT(st->chip->precision_bits - 1) * -1;
>  	st->offset_avail[1] = 1;
>  	st->offset_avail[2] = BIT(st->chip->precision_bits - 1) - 1;
>  
> +	if (st->chip->num_channels > 1)
> +		reg_modes = FIELD_PREP(AD4030_REG_MODES_MASK_LANE_MODE,
> +				       AD4030_LANE_MD_INTERLEAVED);
> +	else
> +		reg_modes = FIELD_PREP(AD4030_REG_MODES_MASK_LANE_MODE,
> +				       AD4030_LANE_MD_1_PER_CH);
> +
> +	ret = regmap_write(st->regmap, AD4030_REG_MODES, reg_modes);
> +	if (ret)
> +		return ret;
> +
>  	if (st->vio_uv < AD4030_VIO_THRESHOLD_UV)
>  		return regmap_write(st->regmap, AD4030_REG_IO,
>  				AD4030_REG_IO_MASK_IO2X);
> @@ -891,8 +977,16 @@ static const unsigned long ad4030_channel_masks[] = {
>  	0,
>  };
>  
> +static const unsigned long ad4630_channel_masks[] = {
> +	/* Differential only */
> +	BIT(0) | BIT(2),

nit: for order consistency with GENMASK

	BIT(2) | BIT(0),

> +	/* Differential with common byte */
> +	GENMASK(3, 0),
> +	0,
> +};
> +
>  static const struct iio_scan_type ad4030_24_scan_types[] = {
> -	[AD4030_SCAN_TYPE_NORMAL] = {
> +	[AD4030_OUT_DATA_MD_24_DIFF] = {

Accidental replacement?

>  		.sign = 's',
>  		.storagebits = 32,
>  		.realbits = 24,
> @@ -908,6 +1002,23 @@ static const struct iio_scan_type ad4030_24_scan_types[] = {
>  	},
>  };
>  
> +static const struct iio_scan_type ad4030_16_scan_types[] = {
> +	[AD4030_SCAN_TYPE_NORMAL] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 16,
> +		.shift = 16,
> +		.endianness = IIO_BE,
> +	},
> +	[AD4030_SCAN_TYPE_AVG] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 30,
> +		.shift = 2,
> +		.endianness = IIO_BE,
> +	}
> +};
> +
>  static const struct ad4030_chip_info ad4030_24_chip_info = {
>  	.name = "ad4030-24",
>  	.available_masks = ad4030_channel_masks,
> @@ -923,14 +1034,52 @@ static const struct ad4030_chip_info ad4030_24_chip_info = {
>  	.tcyc = AD4030_TCYC_ADJUSTED_NS,
>  };
>  
> +static const struct ad4030_chip_info ad4630_16_chip_info = {
> +	.name = "ad4630-16",
> +	.available_masks = ad4630_channel_masks,
> +	.available_masks_len = ARRAY_SIZE(ad4630_channel_masks),
> +	.channels = {
> +		AD4030_CHAN_IN(0, ad4030_16_scan_types),
> +		AD4030_CHAN_CMO(0),
> +		AD4030_CHAN_IN(1, ad4030_16_scan_types),
> +		AD4030_CHAN_CMO(1),
> +		IIO_CHAN_SOFT_TIMESTAMP(4),
> +	},
> +	.grade = AD4030_REG_CHIP_GRADE_AD4630_16_GRADE,
> +	.precision_bits = 16,
> +	.num_channels = 2,
> +	.tcyc = AD4030_TCYC_ADJUSTED_NS,
> +};
> +
> +static const struct ad4030_chip_info ad4630_24_chip_info = {
> +	.name = "ad4630-24",
> +	.available_masks = ad4630_channel_masks,
> +	.available_masks_len = ARRAY_SIZE(ad4630_channel_masks),
> +	.channels = {
> +		AD4030_CHAN_IN(0, ad4030_24_scan_types),
> +		AD4030_CHAN_CMO(0),
> +		AD4030_CHAN_IN(1, ad4030_24_scan_types),
> +		AD4030_CHAN_CMO(1),
> +		IIO_CHAN_SOFT_TIMESTAMP(4),
> +	},
> +	.grade = AD4030_REG_CHIP_GRADE_AD4630_24_GRADE,
> +	.precision_bits = 24,
> +	.num_channels = 2,
> +	.tcyc = AD4030_TCYC_ADJUSTED_NS,
> +};
> +
>  static const struct spi_device_id ad4030_id_table[] = {
>  	{ "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
> +	{ "ad4630-16", (kernel_ulong_t)&ad4630_16_chip_info },
> +	{ "ad4630-24", (kernel_ulong_t)&ad4630_24_chip_info },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(spi, ad4030_id_table);
>  
>  static const struct of_device_id ad4030_of_match[] = {
>  	{ .compatible = "adi,ad4030-24", .data = &ad4030_24_chip_info },
> +	{ .compatible = "adi,ad4630-16", .data = &ad4630_16_chip_info },
> +	{ .compatible = "adi,ad4630-24", .data = &ad4630_24_chip_info },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, ad4030_of_match);
> 

Re: [PATCH 6/6] docs: iio: ad4030: add documentation

[David Lechner]

On 8/22/24 7:45 AM, Esteban Blanc wrote:
> This adds a new page to document how to use the ad4030 ADC driver
> 
> Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> ---
>  Documentation/iio/ad4030.rst | 129 +++++++++++++++++++++++++++++++++++++++++++
>  Documentation/iio/index.rst  |   1 +
>  MAINTAINERS                  |   1 +
>  3 files changed, 131 insertions(+)
> 
> diff --git a/Documentation/iio/ad4030.rst b/Documentation/iio/ad4030.rst
> new file mode 100644
> index 000000000000..56e0ba58b127
> --- /dev/null
> +++ b/Documentation/iio/ad4030.rst
> @@ -0,0 +1,129 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +=============
> +AD4030 driver
> +=============
> +
> +ADC driver for Analog Devices Inc. AD4030 and similar devices. The module name
> +is ``ad4030``.
> +
> +
> +Supported devices
> +=================
> +
> +The following chips are supported by this driver:
> +
> +* `AD4030-24 <https://www.analog.com/AD4030-24>`_
> +* `AD4630-16 <https://www.analog.com/AD4630-16>`_
> +* `AD4630-24 <https://www.analog.com/AD4630-24>`_
> +* `AD4632-16 <https://www.analog.com/AD4632-16>`_
> +* `AD4632-24 <https://www.analog.com/AD4632-24>`_
> +
> +IIO channels
> +============
> +
> +Each "device" channel as described in the datasheet is split in 2 IIO channels,
> +in the following order:
> +
> +- One channel for the differential data
> +- One channel for the common byte.
> +
> +Supported features
> +==================
> +
> +SPI wiring modes
> +----------------
> +
> +The driver currently supports the following SPI wiring configurations:
> +
> +One lane mode
> +^^^^^^^^^^^^^
> +
> +In this mode, each channel has its own SDO line to send the conversion results.
> +At the moment this mode can only be used on AD4030 which has one channel so only
> +one SDO line is used.
> +
> +.. code-block::
> +
> +    +-------------+         +-------------+
> +    |     ADC     |         |     HOST    |
> +    |             |         |             |
> +    |         CNV |<--------| CNV         |
> +    |          CS |<--------| CS          |
> +    |         SDI |<--------| SDO         |
> +    |        SDO0 |-------->| SDI         |
> +    |        SCLK |<--------| SCLK        |
> +    +-------------+         +-------------+
> +
> +Interleaved mode
> +^^^^^^^^^^^^^^^^
> +
> +In this mode, both channels conversion results are bit interleaved one SDO line.
> +As such the wiring is the same as `One lane mode`.
> +
> +SPI Clock mode
> +--------------
> +
> +Only the SPI clocking mode is supported.
> +
> +Output modes
> +------------
> +
> +There is more exposed IIO channels than channels as describe in the devices
> +datasheet. This is due to the `Differential data + 8-bit common-mode` encoding
> +2 types of information in one conversion result. As such a "device" channel
> +provides 2 IIO channels, one for the differential data and one for the common
> +byte.
> +
> +Differential data
> +^^^^^^^^^^^^^^^^^
> +
> +This mode is selected when:
> +
> +- Only differential channels are selected
> +- Oversampling attribute is set to 1
> +
> +Differential data + common-mode
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +This mode is selected when:
> +
> +- Differential and common-mode channels are selected
> +- Oversampling attribute is set to 1
> +
> +For the 24-bits chips, this mode is also available with 16-bits differential
> +data but is not selectable yet.
> +
> +Averaged differential data
> +^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +This mode is selected when:
> +
> +- Only differential channels are selected
> +- Oversampling attribute is greater than 1
> +

Worth mentioning calibration (gain/offset) feature?

> +Reference voltage
> +-----------------
> +
> +The chip supports an external reference voltage via the ``REF`` input or an
> +internal buffered reference voltage via the ``REFIN`` input. The driver looks
> +at the device tree to determine which is being used. If ``ref-supply`` is
> +present, then the external reference voltage is used and the internal buffer is
> +disabled. If ``refin-supply`` is present, then the internal buffered reference
> +voltage is used.
> +
> +Reset
> +-----
> +
> +Both hardware and software reset are supported. The driver looks first at the
> +device tree to see if the `reset-gpio` is populated. If not present, the driver
> +will fallback to a software reset by wiring to the device's registers.
> +
> +Unimplemented features
> +----------------------
> +
> +- ``BUSY`` indication
> +- Additional wiring modes
> +- Additional clock modes
> +- Differential data 16-bits + common-mode for 24-bits chips

- overrange event
- test patterns

> +
> diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
> index dfcf9618568a..61faf3a60da6 100644
> --- a/Documentation/iio/index.rst
> +++ b/Documentation/iio/index.rst
> @@ -19,6 +19,7 @@ Industrial I/O Kernel Drivers
>     :maxdepth: 1
>  
>     ad4000
> +   ad4030
>     ad4695
>     ad7380
>     ad7944
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6a5a0e7b7a51..4a076a48648a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -420,6 +420,7 @@ R:	Esteban Blanc <eblanc@baylibre.com>
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
> +F:	Documentation/iio/ad4030.c
>  F:	drivers/iio/adc/ad4030.c
>  
>  AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER
> 

Re: [PATCH 2/6] iio: adc: ad4030: add driver for ad4030-24

[Jonathan Cameron]

On Thu, 22 Aug 2024 14:39:55 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 8/22/24 7:45 AM, Esteban Blanc wrote:
> > This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> > 
> > The driver implements basic support for the AD4030-24 1 channel
> > differential ADC with hardware gain and offset control.
> > 
> > Signed-off-by: Esteban Blanc <eblanc@baylibre.com>

A couple of comments on comments inline mainly to point out
one 'lazy' alternative that is very common for the IIO_VAL_INT
write case.

> > +static int ad4030_single_conversion(struct iio_dev *indio_dev,
> > +				    const struct iio_chan_spec *chan, int *val)
> > +{
> > +	struct ad4030_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = ad4030_set_mode(indio_dev, BIT(chan->channel));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4030_exit_config_mode(st);
> > +	if (ret)
> > +		goto out_exit_config_mode_error;  
> 
> Looks like we could just return ret here.
> 
> > +
> > +	ret = ad4030_conversion(st, chan);
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	if (chan->channel % 2)
> > +		*val = st->rx_data.buffered[chan->channel / 2].common;
> > +	else
> > +		*val = st->rx_data.buffered[chan->channel / 2].val;
> > +
> > +out_error:
> > +	ad4030_enter_config_mode(st);
> > +
> > +out_exit_config_mode_error:
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	return IIO_VAL_INT;  
> 
> This can be moved before out_error:, then we can just have
> return ret here and leave out the if.
I'd assume not quite because we need to go back into config mode
even on error.

I'd be tempted to have separate error block and just duplicate
that one call.
> 
> > +}

> > +static int ad4030_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int val,
> > +			    int val2, long info)
> > +{
> > +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > +		switch (info) {
> > +		case IIO_CHAN_INFO_CALIBSCALE:
> > +			return ad4030_set_chan_gain(indio_dev, chan->channel,
> > +						    val, val2);
> > +
> > +		case IIO_CHAN_INFO_CALIBBIAS:
> > +			return ad4030_set_chan_offset(indio_dev, chan->channel,
> > +						      val);  
> 
> Need to add .write_raw_get_fmt to struct iio_info below to set
> IIO_CHAN_INFO_CALIBBIAS to IIO_VAL_INT. Othwerwise, the defualt
> IIO_VAL_INT_PLUS_MICRO is used and val2 would have considered
> for handling negative values.

Lazy approach is 
	if (val2 != 0)
		return -EINVAL;
We do this a fair bit in drivers to avoid a very minimal write_raw_fmt
callback.

But 'right way' is to tell the core that it's an int.

> 
> > +
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	unreachable();
> > +}

> 
> > +	indio_dev->name = st->chip->name;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->info = &ad4030_iio_info;
> > +	indio_dev->channels = st->chip->channels;
> > +	indio_dev->available_scan_masks = st->chip->available_masks;
> > +	indio_dev->masklength = st->chip->available_masks_len;  
> 
> indio_dev->masklengh is marked as [INTERN] so should not be set by drivers.

It will now give a compile error if you try this on linux-next or
the iio.git/togreg tree.

Re: [PATCH 2/6] iio: adc: ad4030: add driver for ad4030-24

[Jonathan Cameron]

On Thu, 22 Aug 2024 14:45:18 +0200
Esteban Blanc <eblanc@baylibre.com> wrote:

> This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> 
> The driver implements basic support for the AD4030-24 1 channel
> differential ADC with hardware gain and offset control.
> 
> Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
Hi Esteban

Some additional comments.  David did a good review already so
I've tried not to duplicate too much of that.

The big one in here is don't use extended_name.
It's effectively deprecated for new drivers plus
it would have required you add a lot of ABI docs as every
sysfs file would have a strange name.

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

> +struct ad4030_state {
> +	struct spi_device *spi;
> +	struct regmap *regmap;
> +	const struct ad4030_chip_info *chip;
> +	struct gpio_desc *cnv_gpio;
> +	int vref_uv;
> +	int vio_uv;
> +	int offset_avail[3];
> +	u32 conversion_speed_hz;
> +	enum ad4030_out_mode mode;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the transfer buffers
> +	 * to live in their own cache lines.
> +	 */
> +	u8 tx_data[AD4030_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> +	struct {
> +		union {
> +			u8 raw[AD4030_MAXIMUM_RX_BUFFER_SIZE];
> +			struct {
> +				s32 val;
> +				u32 common;
> +			} __packed buffered[AD4030_MAX_HARDWARE_CHANNEL_NB];

David pointed out this doesn't need to be packed.
Given you have a union here, add __beXX as needed to avoid casts below.

> +		};
> +	} rx_data __aligned(IIO_DMA_MINALIGN);

This one is always fun.  You shouldn't need to mark this second buffer
because normally SPI controllers don't shoot themselves in the foot
and it isn't normal software flow to fill rx_data whilst the SPI
controller is working on tx_data (I didn't notice you doing that
here either).

As such, we don't need tx_buf and rx_buf to be in different cachelines.
It is sufficient to mark just the first one (as that forces these
to start at a new cacheline) the second buffer can then sit later
in that same cacheline if the sizes end up so that happens.

So drop __aligned(IIO_DMA_MINALIGN from rx_data.


> +};
> +
> +#define AD4030_CHAN_CMO(_idx)  {					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _idx * 2 + 2,					\
> +	.scan_index = _idx * 2 + 1,					\
> +	.extend_name = "Channel" #_idx " common byte part",		\

We more or less never use extend name any more because it makes writing
userspace code much harder.  Use the label callback to assign a label instead.

If we were still using this, it would need to be a lot simpler than that
and no spaces etc as it ends up int he sysfs file names.


> +	.scan_type = {							\
> +		.sign = 'u',						\
> +		.storagebits = 32,					\
> +		.realbits = 8,						\
> +		.endianness = IIO_BE,					\
> +	},								\
> +}
> +
> +#define AD4030_CHAN_IN(_idx, _storage, _real, _shift) {			\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
> +		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
> +		BIT(IIO_CHAN_INFO_RAW),					\
> +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
> +		BIT(IIO_CHAN_INFO_CALIBSCALE),				\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _idx * 2,						\
> +	.channel2 = _idx * 2 + 1,					\
> +	.scan_index = _idx * 2,						\
> +	.extend_name = "Channel" #_idx " differential part",		\

As above, no to this for same reason.
This will generate a crazy ABI so I'm a bit surprised that didn't show
up in your testing.  Would have needed a lot of docs even if we did
still do things this way.

> +	.differential = true,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.storagebits = _storage,				\
> +		.realbits = _real,					\
> +		.shift = _shift,					\
> +		.endianness = IIO_BE,					\
> +	},								\
> +}
> +
> +static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
> +			   void *val, size_t val_size)
> +{
> +	struct ad4030_state *st = context;
> +
> +	struct spi_transfer xfer = {
> +		.tx_buf = st->tx_data,
> +		.rx_buf = st->rx_data.raw,
> +		.len = reg_size + val_size,
> +	};
> +	int ret;
> +
> +	memcpy(st->tx_data, reg, reg_size);
> +
> +	/*
> +	 * This should use spi_write_the_read but when doing so, CS never get
> +	 * deasserted.

I'm confused.  As a single transfer it won't be deasserted in the transfer
whereas spi_write_then_read() will. So is this comment backwards or
is it referring to something else?

> +	 */
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	memcpy(val, &st->rx_data.raw[reg_size], val_size);
> +
> +	return ret;
> +}

> +
> +static int ad4030_get_chan_gain(struct iio_dev *indio_dev, int ch, int *val,
> +				int *val2)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	u16 gain;
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4030_REG_GAIN_CHAN(ch),
> +			       st->rx_data.raw, AD4030_REG_GAIN_BYTES_NB);
> +	if (ret)
> +		return ret;
> +
> +	gain = be16_to_cpu(*(u16 *)st->rx_data.raw);

As below, I'd be tempted to use the unaligned getters to avoid the cast.
This isn't a performance path afterall so even if it's painful on
some arch, we don't really care.

> +
> +	/* From datasheet: multiplied output = input × gain word/0x8000 */
> +	*val = gain / 0x8000;
> +	*val2 = mul_u64_u32_div(gain % 0x8000, MICRO, 0x8000);
> +
> +	return 0;
> +}
> +
> +/*
> + * @brief Returns the offset where 1 LSB = (VREF/2^precision_bits - 1)/gain

Not kernel, doc so drop the @brief, or fully document it.

> + */
> +static int ad4030_get_chan_offset(struct iio_dev *indio_dev, int ch, int *val)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4030_REG_OFFSET_CHAN(ch),
> +			       st->rx_data.raw, AD4030_REG_OFFSET_BYTES_NB);
> +	if (ret)
> +		return ret;
> +
> +	switch (st->chip->precision_bits) {
> +	case 16:
> +		*val = sign_extend32(get_unaligned_be16(st->rx_data.raw), 15);
> +		break;
return 0;
> +
> +	case 24:
> +		*val = sign_extend32(get_unaligned_be24(st->rx_data.raw), 23);
> +		break;
return 0;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
I'll always prefer early returns because I don't like scrolling
to check what happens after switch statements.
So move the returns up and drop this.

> +}
> +
> +static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int gain_int,
> +				int gain_frac)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	__be16 *val = (__be16 *)st->rx_data.raw;

Hmm. This is a bit ugly. Maybe a union for that
buffer so you can reference it directly without the type casting?
Or be lazy and do a put_unaligned_be16 even though it's aligned ;)

> +	u64 gain;
> +
> +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> +
> +	if (gain > AD4030_REG_GAIN_MAX_GAIN)
> +		return -EINVAL;
> +
> +	*val = cpu_to_be16(DIV_ROUND_CLOSEST_ULL(gain * 0x8000, MICRO));
> +
> +	return regmap_bulk_write(st->regmap, AD4030_REG_GAIN_CHAN(ch), val,
> +			  AD4030_REG_GAIN_BYTES_NB);
> +}

> +
> +static int ad4030_conversion(struct ad4030_state *st,
> +			     const struct iio_chan_spec *chan)
> +{
> +	unsigned int bytes_to_read;
> +	unsigned char byte_index;
> +	unsigned int i;
> +	int ret;
> +
> +	/* Number of bytes for one differential channel */
> +	bytes_to_read = BITS_TO_BYTES(chan->scan_type.realbits);
> +	/* Add one byte if we are using a differential + common byte mode */
> +	bytes_to_read += (st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM ||
> +			st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM) ? 1 : 0;
> +	/* Mulitiply by the number of hardware channels */
> +	bytes_to_read *= st->chip->num_channels;
> +
> +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> +	ndelay(AD4030_TCNVH_NS);
> +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +	ndelay(st->chip->tcyc);
> +
> +	ret = spi_read(st->spi, st->rx_data.raw, bytes_to_read);
> +	if (ret)
> +		return ret;
> +
> +	if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> +		return 0;
> +
> +	byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
> +	for (i = 0; i < st->chip->num_channels; i++)
> +		st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
break line after =.

When it doesn't significantly hurt readability we still try to keep to 80
chars for IIO drivers.  People have big screens but a lot of kernel devs
love to have lots of windows across them - or have bad eyesight due to
years of code review!


> +
> +	return 0;
> +}

> +
> +static int ad4030_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long info)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +		switch (info) {
> +		case IIO_CHAN_INFO_RAW:
> +			return ad4030_single_conversion(indio_dev, chan, val);
> +
> +		case IIO_CHAN_INFO_SCALE:
> +			*val = (st->vref_uv * 2) / MILLI;
> +			*val2 = st->chip->precision_bits;
> +			return IIO_VAL_FRACTIONAL_LOG2;

No reason you can't read this whilst buffered capture in progress.
Maybe it's not worth the effort of special casing though.

It is the one thing people do read whilst doing buffered capture
though because they didn't cache it before starting the buffer
and it's needed for data interpretation unlike all the other controls.

Maybe just do a
	if (info == IIO_CHAN_INFO_SCALE) {
	}
block at top of function?



> +
> +		case IIO_CHAN_INFO_CALIBSCALE:
> +			ret = ad4030_get_chan_gain(indio_dev, chan->channel,
> +						   val, val2);
> +			if (ret)
> +				return ret;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +
> +		case IIO_CHAN_INFO_CALIBBIAS:
> +			ret = ad4030_get_chan_offset(indio_dev, chan->channel,
> +						     val);
> +			if (ret)
> +				return ret;
> +			return IIO_VAL_INT;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	unreachable();
> +}
> +



> +
> +static int ad4030_reset(struct ad4030_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	struct gpio_desc *reset;
> +	int ret;
> +
> +	/* Use GPIO if available ... */

Obvious comment given code. I'd drop it.

> +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset))
> +		return dev_err_probe(dev, PTR_ERR(reset),
> +				"Failed to get reset GPIO\n");
> +
> +	if (reset) {
> +		ndelay(50);
> +		gpiod_set_value_cansleep(reset, 0);
> +	} else {
> +		/* ... falback to software reset otherwise */
likewise. Feels obvious enough to not benefit from comment.
Maybe that's just me having seen far too many drivers though
so if you really think it is useful then fine to leave these.

> +		ret = ad4030_enter_config_mode(st);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(st->regmap, AD4030_REG_INTERFACE_CONFIG_A,
> +				   AD4030_REG_INTERFACE_CONFIG_A_SW_RESET);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Wait for reset to complete before communicating to it */

I'd rather see a reference for the value than a generic comment
like this.  Also pull the actual value down here. Not particularly
useful to have a define for what is a real time unless you are going
 to have some combined docs for a bunch of timings (i.e a datasheet
table reference)


> +	fsleep(AD4030_TRESET_COM_DELAY_MS);
> +
> +	/* After reset, conversion mode is enabled. Switch to reg access */

This comment is good as not obvious without datasheet diving.
So one to definitely keep.


> +	return ad4030_enter_config_mode(st);
> +}
> +
> +static int ad4030_detect_chip_info(const struct ad4030_state *st)
> +{
> +	unsigned int grade;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
> +	if (ret)
> +		return ret;
> +
> +	grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
> +	if (grade != st->chip->grade)
> +		return dev_err_probe(&st->spi->dev, -EINVAL,
> +					"Unknown grade(0x%x) for %s\n", grade,
> +					st->chip->name);

Is this similar to a missmatch on a whoami value?
I.e. should we print a message and carry on in the interests of providing
some degree of support for newer devices on older kernel?
(fallback compatibles in DT)

Superficially it seems to be matched with a particular compatible
so it seems likely.

> +
> +	return 0;
> +}
> +


>
> +static const struct spi_device_id ad4030_id_table[] = {
> +	{ "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
> +	{}
As below.

I'm going to assume you have a bunch of other parts you plan to
support soon. Otherwise we normally don't add the chip specific
support until it is needed.  It tends to complicate initial driver
review a little and experience says that sometimes no other devices
are ever added.


> +};
> +MODULE_DEVICE_TABLE(spi, ad4030_id_table);
> +
> +static const struct of_device_id ad4030_of_match[] = {
> +	{ .compatible = "adi,ad4030-24", .data = &ad4030_24_chip_info },
> +	{}

I'm aiming for a bit more consistency in formatting of these.
As such made an arbitrary choice to prefer
 	{ }
for the terminators. Please go with that for new code.

> +};
> +MODULE_DEVICE_TABLE(of, ad4030_of_match);

Re: [PATCH 1/6] dt-bindings: iio: adc: add ADI ad4030, ad4630 and ad4632

[Krzysztof Kozlowski]

On 22/08/2024 14:45, Esteban Blanc wrote:
> This adds a binding specification for the Analog Devices Inc. AD4030,
> AD4630 and AD4632 families of ADCs.
> 
> - ad4030-24 is a 1 channel SAR ADC with 24 bits of precision and a
>   sampling rate of 2M samples per second
> - ad4630-16 is a 2 channels SAR ADC with 16 bits of precision and a
>   sampling rate of 2M samples per second
> - ad4630-24 is a 2 channels SAR ADC with 24 bits of precision and a
>   sampling rate of 2M samples per second
> - ad4632-16 is a 2 channels SAR ADC with 16 bits of precision and a
>   sampling rate of 500K samples per second
> - ad4632-24 is a 2 channels SAR ADC with 24 bits of precision and a
>   sampling rate of 500K samples per second
> 
> Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad4030.yaml    | 113 +++++++++++++++++++++
>  MAINTAINERS                                        |   8 ++
>  2 files changed, 121 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
> new file mode 100644
> index 000000000000..7957c0c0ac7a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
> @@ -0,0 +1,113 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2024 Analog Devices Inc.
> +# Copyright 2024 BayLibre, SAS.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4030.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD4030 and AD4630 ADC family device driver

"device driver"? Bindings are for hardware. Explain the hardware, not
driver.

> +
> +maintainers:
> +  - Nuno Sa <nuno.sa@analog.com>
> +  - Michael Hennerich <michael.hennerich@analog.com>
> +
> +description: |
> +  Analog Devices AD4030 single channel and AD4630 dual channel precision SAR ADC
> +  family

Does not look like wrapped according to Linux coding style. Read the
coding style (not checkpatch).

> +
> +  * https://www.analog.com/media/en/technical-documentation/data-sheets/ad4030-24-4032-24.pdf
> +  * https://www.analog.com/media/en/technical-documentation/data-sheets/ad4630-24_ad4632-24.pdf
> +  * https://www.analog.com/media/en/technical-documentation/data-sheets/ad4630-16-4632-16.pdf
> +
> +properties:
> +

Drop blank line

> +  compatible:
> +    enum:
> +      - adi,ad4030-24
> +      - adi,ad4630-16
> +      - adi,ad4630-24
> +      - adi,ad4632-16
> +      - adi,ad4632-24
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 100000000
> +
> +  spi-rx-bus-width:
> +    enum: [1, 2, 4]
> +
> +  vdd-5v-supply: true
> +  vdd-1v8-supply: true
> +  vio-supply: true
> +
> +  ref-supply:
> +    description:
> +      Optional External unbuffered reference. Used when refin-supply is not
> +      connected.
> +
> +  refin-supply:
> +    description:
> +      Internal buffered Reference. Used when ref-supply is not connected.
> +
> +  cnv-gpio:

Nope, there is no "gpio" property. It IS ALWAYS gpios. Look at other
examples.


> +    description:
> +      The Convert Input (CNV). It initiates the sampling conversions.
> +    maxItems: 1
> +
> +  reset-gpio:

Same problem.

> +    description:
> +      Reset Input (Active Low). Used for asynchronous device reset.
> +    maxItems: 1
> +
> +  interrupts:
> +    description:
> +      The BUSY pin is used to signal that the conversions results are available
> +      to be transferred when in SPI Clocking Mode. This nodes should be connected
> +      to an interrupt that is triggered when the BUSY line goes low.
> +    maxItems: 1
> +


> 

Best regards,
Krzysztof

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Jonathan Cameron]

On Thu, 22 Aug 2024 14:45:20 +0200
Esteban Blanc <eblanc@baylibre.com> wrote:

> AD4630-24 and AD4630-16 are 2 channels ADCs. Both channels are
> interleaved bit per bit on SDO line.
> 
> Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
Ah. I should have looked on in the patch set.  This obviously answers
question about plans to support more parts.

It feels like you made various improvements to naming etc
in this patch and then forgot to push them back to the earlier
patches.

Jonathan

> ---
>  drivers/iio/adc/ad4030.c | 197 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 173 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> index e1e1dbf0565c..dbba5287b630 100644
> --- a/drivers/iio/adc/ad4030.c
> +++ b/drivers/iio/adc/ad4030.c
> @@ -32,6 +32,8 @@
>  #define AD4030_REG_PRODUCT_ID_H				0x05
>  #define AD4030_REG_CHIP_GRADE				0x06
>  #define     AD4030_REG_CHIP_GRADE_AD4030_24_GRADE	0x10
> +#define     AD4030_REG_CHIP_GRADE_AD4630_16_GRADE	0x03
> +#define     AD4030_REG_CHIP_GRADE_AD4630_24_GRADE	0x00
>  #define     AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE	GENMASK(7, 3)
>  #define AD4030_REG_SCRATCH_PAD			0x0A
>  #define AD4030_REG_SPI_REVISION			0x0B
> @@ -159,10 +161,14 @@ struct ad4030_state {
>  	struct {
>  		union {
>  			u8 raw[AD4030_MAXIMUM_RX_BUFFER_SIZE];
> -			struct {
> -				s32 val;
> -				u32 common;
> -			} __packed buffered[AD4030_MAX_HARDWARE_CHANNEL_NB];
> +			union {
> +				s32 diff[AD4030_MAX_HARDWARE_CHANNEL_NB];
> +				struct {
> +					s32 diff;
> +					u32 common;
> +				} __packed
Unless you are going to do something complex later, a union of unions is
just a union so can flatten this I think.  Also no need for __packed
for array of single size as those are always packed.

> +				buffered_common[AD4030_MAX_HARDWARE_CHANNEL_NB];

This style is confusing to read.  If you did need the __packed then it would be
worth pushing to previous line, but you don't.
In general feels like some of these renames should have been in the earlier
patch.

> +			};
>  		};
>  	} rx_data __aligned(IIO_DMA_MINALIGN);
>  };
> @@ -171,7 +177,7 @@ struct ad4030_state {
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
>  	.type = IIO_VOLTAGE,						\
>  	.indexed = 1,							\
> -	.channel = _idx * 2 + 2,					\
> +	.channel = _idx * 3 + 2,					\
Odd change. Wrong patch? 
>  	.scan_index = _idx * 2 + 1,					\
>  	.extend_name = "Channel" #_idx " common byte part",		\
>  	.scan_type = {							\
> @@ -194,8 +200,8 @@ struct ad4030_state {
>  		BIT(IIO_CHAN_INFO_CALIBSCALE),				\
>  	.type = IIO_VOLTAGE,						\
>  	.indexed = 1,							\
> -	.channel = _idx * 2,						\
> -	.channel2 = _idx * 2 + 1,					\
> +	.channel = _idx * 3,						\
> +	.channel2 = _idx * 3 + 1,					\
>  	.scan_index = _idx * 2,						\
>  	.extend_name = "Channel" #_idx " differential part",		\
>  	.differential = true,						\
> @@ -412,7 +418,7 @@ static int ad4030_set_avg_frame_len(struct iio_dev *dev, unsigned int avg_len)
>  static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
>  					unsigned int mask)
>  {
> -	/* Common byte channel is after the "real" differential sample channel */
> +	/* Common byte channels are after each differential channel */

Just use that comment in earlier patch to avoid noise.
Each can incorporate 1 even if it's a bit odd to read.

>  	return mask & AD4030_COMMON_BYTE_CHANNELS_FILTER;
>  }
>  

> +/*
> + * @brief Descramble 2 32bits numbers out of a 64bits. The bits are interleaved:
> + * 1 bit for first number, 1 bit for the second, and so on...
> + */
> +static void ad4030_extract_interleaved(u8 *src, u32 *ch0, u32 *ch1)
> +{
> +	u8 h0, h1, l0, l1;
> +	u32 out0, out1;
> +	u8 *out0_raw = (u8 *)&out0;
> +	u8 *out1_raw = (u8 *)&out1;
> +
> +	for (int i = 0; i < 4; i++) {
> +		h0 = src[i * 2];
> +		l1 = src[i * 2 + 1];
> +		h1 = h0 << 1;
> +		l0 = l1 >> 1;
> +
> +		h0 &= 0xAA;
> +		l0 &= 0x55;
> +		h1 &= 0xAA;
> +		l1 &= 0x55;
> +
> +		h0 = (h0 | h0 << 001) & 0xCC;
> +		h1 = (h1 | h1 << 001) & 0xCC;
> +		l0 = (l0 | l0 >> 001) & 0x33;
> +		l1 = (l1 | l1 >> 001) & 0x33;
> +		h0 = (h0 | h0 << 002) & 0xF0;
> +		h1 = (h1 | h1 << 002) & 0xF0;
> +		l0 = (l0 | l0 >> 002) & 0x0F;
> +		l1 = (l1 | l1 >> 002) & 0x0F;
> +
> +		out0_raw[i] = h0 | l0;
> +		out1_raw[i] = h1 | l1;
> +	}
> +
> +	*ch0 = out0;
> +	*ch1 = out1;
> +}
> +
>  static int ad4030_conversion(struct ad4030_state *st,
>  			     const struct iio_chan_spec *chan)
>  {
> @@ -460,12 +517,21 @@ static int ad4030_conversion(struct ad4030_state *st,
>  	if (ret)
>  		return ret;
>  
> -	if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> +	if (st->chip->num_channels == 2)
> +		ad4030_extract_interleaved(st->rx_data.raw,
> +					   &st->rx_data.diff[0],
> +					   &st->rx_data.diff[1]);
> +
> +	if (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> +	    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
>  		return 0;
>  
>  	byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
> -	for (i = 0; i < st->chip->num_channels; i++)
> -		st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
> +	/* Doing it backward to avoid overlap when reordering */
> +	for (i = st->chip->num_channels - 1; i > 0; i--) {
> +		st->rx_data.buffered_common[i].diff = st->rx_data.diff[i];
> +		st->rx_data.buffered_common[i].common = ((u8 *)&st->rx_data.diff[i])[byte_index];
> +	}

I wonder if doing it in place is actually worthwhile.  Maybe unpack into a second
array? That is still fairly small and may make code easier to read.


>  
>  	return 0;
>  }

...

>  
> +static const unsigned long ad4630_channel_masks[] = {
> +	/* Differential only */
> +	BIT(0) | BIT(2),
> +	/* Differential with common byte */
> +	GENMASK(3, 0),
The packing of data isn't going to be good. How bad to shuffle
to put the two small channels next to each other?
Seems like it means you will want to combine your deinterleave
and channel specific handling above, which is a bit fiddly but
not much worse than current code.


It only matters if you have timestamps though as otherwise
the 8 byte alignment will force 6 bytes of packing in one
place vs the 3 this gives in two places.
I guess maybe not worth doing unless you plan to combine
this with an offload engine in which case the timestamps will
probably be disabled and the 12 byte scan length will be preferable
to 16 bytes.

> +	0,
> +};
> +
>  static const struct iio_scan_type ad4030_24_scan_types[] = {
> -	[AD4030_SCAN_TYPE_NORMAL] = {
> +	[AD4030_OUT_DATA_MD_24_DIFF] = {
Name it this way in the first place.

>  		.sign = 's',
>  		.storagebits = 32,
>  		.realbits = 24,
> @@ -908,6 +1002,23 @@ static const struct iio_scan_type ad4030_24_scan_types[] = {
>  	},
>  };
>  
> +static const struct iio_scan_type ad4030_16_scan_types[] = {
> +	[AD4030_SCAN_TYPE_NORMAL] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 16,
> +		.shift = 16,
> +		.endianness = IIO_BE,
> +	},
> +	[AD4030_SCAN_TYPE_AVG] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 30,
> +		.shift = 2,
> +		.endianness = IIO_BE,
> +	}
> +};
> +

Re: [PATCH 5/6] iio: adc: ad4030: add support for ad4632-16 and ad4632-24

[Jonathan Cameron]

On Thu, 22 Aug 2024 14:45:21 +0200
Esteban Blanc <eblanc@baylibre.com> wrote:

> AD4632-24 and AD4632-16 are 2 channels ADCs. Both channels are
> interleaved bit per bit on SDO line.
> 
> Both of them do not have evaluation board. As such, the support added
> here can't be tested. Support is provided as best effort until someone get
> their hands on one.
> 
> Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
This looks fine to me.

Re: [PATCH 2/6] iio: adc: ad4030: add driver for ad4030-24

[Esteban Blanc]

On Sat Aug 24, 2024 at 1:21 PM CEST, Jonathan Cameron wrote:
> On Thu, 22 Aug 2024 14:45:18 +0200
> Esteban Blanc <eblanc@baylibre.com> wrote:
>
> > This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> > 
> > The driver implements basic support for the AD4030-24 1 channel
> > differential ADC with hardware gain and offset control.
> > 
> > Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> Hi Esteban
>
> Some additional comments.  David did a good review already so
> I've tried not to duplicate too much of that.
>
> The big one in here is don't use extended_name.
> It's effectively deprecated for new drivers plus
> it would have required you add a lot of ABI docs as every
> sysfs file would have a strange name.
>
> > diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> > new file mode 100644
> > index 000000000000..a981dce988e5
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4030.c
>
> > +struct ad4030_state {
> > +	struct spi_device *spi;
> > +	struct regmap *regmap;
> > +	const struct ad4030_chip_info *chip;
> > +	struct gpio_desc *cnv_gpio;
> > +	int vref_uv;
> > +	int vio_uv;
> > +	int offset_avail[3];
> > +	u32 conversion_speed_hz;
> > +	enum ad4030_out_mode mode;
> > +
> > +	/*
> > +	 * DMA (thus cache coherency maintenance) requires the transfer buffers
> > +	 * to live in their own cache lines.
> > +	 */
> > +	u8 tx_data[AD4030_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> > +	struct {
> > +		union {
> > +			u8 raw[AD4030_MAXIMUM_RX_BUFFER_SIZE];
> > +			struct {
> > +				s32 val;
> > +				u32 common;
> > +			} __packed buffered[AD4030_MAX_HARDWARE_CHANNEL_NB];
>
> David pointed out this doesn't need to be packed.
> Given you have a union here, add __beXX as needed to avoid casts below.

They also pointed out that I should reduce the size for the common field.
I was planing to use an u32 bitfield here, 8 bits for common and 24 bits for
padding. As far as I understood, the C standard is quite flexible on the
size used for bitfield, so I should probably keep the __packed, right?

> > +};
> > +
> > +#define AD4030_CHAN_CMO(_idx)  {					\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> > +	.type = IIO_VOLTAGE,						\
> > +	.indexed = 1,							\
> > +	.channel = _idx * 2 + 2,					\
> > +	.scan_index = _idx * 2 + 1,					\
> > +	.extend_name = "Channel" #_idx " common byte part",		\
>
> We more or less never use extend name any more because it makes writing
> userspace code much harder.  Use the label callback to assign a label instead.
>
> If we were still using this, it would need to be a lot simpler than that
> and no spaces etc as it ends up int he sysfs file names.

> > +	.scan_type = {							\
> > +		.sign = 'u',						\
> > +		.storagebits = 32,					\
> > +		.realbits = 8,						\
> > +		.endianness = IIO_BE,					\
> > +	},								\
> > +}
> > +
> > +#define AD4030_CHAN_IN(_idx, _storage, _real, _shift) {			\
> > +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),		\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
> > +		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
> > +		BIT(IIO_CHAN_INFO_RAW),					\
> > +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
> > +		BIT(IIO_CHAN_INFO_CALIBSCALE),				\
> > +	.type = IIO_VOLTAGE,						\
> > +	.indexed = 1,							\
> > +	.channel = _idx * 2,						\
> > +	.channel2 = _idx * 2 + 1,					\
> > +	.scan_index = _idx * 2,						\
> > +	.extend_name = "Channel" #_idx " differential part",		\
>
> As above, no to this for same reason.
> This will generate a crazy ABI so I'm a bit surprised that didn't show
> up in your testing.  Would have needed a lot of docs even if we did
> still do things this way.

I'm using ADI IIO oscilloscope to check the signals so I didn't get
impacted by the sysfs change. Anyway I will use labels.

> > +	.differential = true,						\
> > +	.scan_type = {							\
> > +		.sign = 's',						\
> > +		.storagebits = _storage,				\
> > +		.realbits = _real,					\
> > +		.shift = _shift,					\
> > +		.endianness = IIO_BE,					\
> > +	},								\
> > +}
> > +
> > +static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
> > +			   void *val, size_t val_size)
> > +{
> > +	struct ad4030_state *st = context;
> > +
> > +	struct spi_transfer xfer = {
> > +		.tx_buf = st->tx_data,
> > +		.rx_buf = st->rx_data.raw,
> > +		.len = reg_size + val_size,
> > +	};
> > +	int ret;
> > +
> > +	memcpy(st->tx_data, reg, reg_size);
> > +
> > +	/*
> > +	 * This should use spi_write_the_read but when doing so, CS never get
> > +	 * deasserted.
>
> I'm confused.  As a single transfer it won't be deasserted in the transfer
> whereas spi_write_then_read() will. So is this comment backwards or
> is it referring to something else?

So, with a single transfer (what is done now), the transfer is working
as expected: CS goes low, the data is transferred, CS goes high again.
With spi_write_then_read(), CS goes low, data is transferred but CS never
goes high again. After some time I get a timeout error in the kernel logs.

> > +static int ad4030_conversion(struct ad4030_state *st,
> > +			     const struct iio_chan_spec *chan)
> > +{
> > +	unsigned int bytes_to_read;
> > +	unsigned char byte_index;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	/* Number of bytes for one differential channel */
> > +	bytes_to_read = BITS_TO_BYTES(chan->scan_type.realbits);
> > +	/* Add one byte if we are using a differential + common byte mode */
> > +	bytes_to_read += (st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM ||
> > +			st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM) ? 1 : 0;
> > +	/* Mulitiply by the number of hardware channels */
> > +	bytes_to_read *= st->chip->num_channels;
> > +
> > +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> > +	ndelay(AD4030_TCNVH_NS);
> > +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> > +	ndelay(st->chip->tcyc);
> > +
> > +	ret = spi_read(st->spi, st->rx_data.raw, bytes_to_read);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> > +		return 0;
> > +
> > +	byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
> > +	for (i = 0; i < st->chip->num_channels; i++)
> > +		st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
> break line after =.
>
> When it doesn't significantly hurt readability we still try to keep to 80
> chars for IIO drivers.  People have big screens but a lot of kernel devs
> love to have lots of windows across them - or have bad eyesight due to
> years of code review!

I keep forgeting that checkpatch now defaults at 100 chars...

> > +static int ad4030_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan, int *val,
> > +			   int *val2, long info)
> > +{
> > +	struct ad4030_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > +		switch (info) {
> > +		case IIO_CHAN_INFO_RAW:
> > +			return ad4030_single_conversion(indio_dev, chan, val);
> > +
> > +		case IIO_CHAN_INFO_SCALE:
> > +			*val = (st->vref_uv * 2) / MILLI;
> > +			*val2 = st->chip->precision_bits;
> > +			return IIO_VAL_FRACTIONAL_LOG2;
>
> No reason you can't read this whilst buffered capture in progress.
> Maybe it's not worth the effort of special casing though.
>
> It is the one thing people do read whilst doing buffered capture
> though because they didn't cache it before starting the buffer
> and it's needed for data interpretation unlike all the other controls.
>
> Maybe just do a
> 	if (info == IIO_CHAN_INFO_SCALE) {
> 	}
> block at top of function?

Good catch. I will check for IIO_CHAN_INFO_SCALE before the whole block

> > +static int ad4030_reset(struct ad4030_state *st)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	struct gpio_desc *reset;
> > +	int ret;
> > +
> > +	/* Use GPIO if available ... */
> > +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(reset))
> > +		return dev_err_probe(dev, PTR_ERR(reset),
> > +				"Failed to get reset GPIO\n");
> > +
> > +	if (reset) {
> > +		ndelay(50);
> > +		gpiod_set_value_cansleep(reset, 0);
> > +	} else {
> > +		/* ... falback to software reset otherwise */
> > +		ret = ad4030_enter_config_mode(st);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_write(st->regmap, AD4030_REG_INTERFACE_CONFIG_A,
> > +				   AD4030_REG_INTERFACE_CONFIG_A_SW_RESET);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	/* Wait for reset to complete before communicating to it */
>
> I'd rather see a reference for the value than a generic comment
> like this.  Also pull the actual value down here. Not particularly
> useful to have a define for what is a real time unless you are going
>  to have some combined docs for a bunch of timings (i.e a datasheet
> table reference)

I will put the real value in fsleep call directly. When you say "I'd
rather see a reference for the value", you ment a reference to the place
the value is defined in the datasheet, right?

> > +static int ad4030_detect_chip_info(const struct ad4030_state *st)
> > +{
> > +	unsigned int grade;
> > +	int ret;
> > +
> > +	ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
> > +	if (ret)
> > +		return ret;
> > +
> > +	grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
> > +	if (grade != st->chip->grade)
> > +		return dev_err_probe(&st->spi->dev, -EINVAL,
> > +					"Unknown grade(0x%x) for %s\n", grade,
> > +					st->chip->name);
>
> Is this similar to a missmatch on a whoami value?

Yes. It also saved me multiple hours of debuging when the eval board
was not connected porperly and the SPI link was just not working.

> I.e. should we print a message and carry on in the interests of providing
> some degree of support for newer devices on older kernel?
> (fallback compatibles in DT)

Ok, let's go with a warning then.

> > +static const struct spi_device_id ad4030_id_table[] = {
> > +	{ "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
> > +	{}
>
> I'm going to assume you have a bunch of other parts you plan to
> support soon. Otherwise we normally don't add the chip specific
> support until it is needed.  It tends to complicate initial driver
> review a little and experience says that sometimes no other devices
> are ever added.

I'm sending the other devices in the same series (patch 4 and 5).
For the sake of reducing noise in the later patches, I've put it in
the initial driver. If you feel like I should wait and do it in the
following patch (patch 4), I can do that.

Thanks for your time,

-- 
Esteban Blanc
BayLibre

Re: [PATCH 2/6] iio: adc: ad4030: add driver for ad4030-24

[Jonathan Cameron]

On Tue, 27 Aug 2024 18:45:49 +0200
Esteban Blanc <eblanc@baylibre.com> wrote:

> On Sat Aug 24, 2024 at 1:21 PM CEST, Jonathan Cameron wrote:
> > On Thu, 22 Aug 2024 14:45:18 +0200
> > Esteban Blanc <eblanc@baylibre.com> wrote:
> >  
> > > This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> > > 
> > > The driver implements basic support for the AD4030-24 1 channel
> > > differential ADC with hardware gain and offset control.
> > > 
> > > Signed-off-by: Esteban Blanc <eblanc@baylibre.com>  
> > Hi Esteban
> >
> > Some additional comments.  David did a good review already so
> > I've tried not to duplicate too much of that.
> >
> > The big one in here is don't use extended_name.
> > It's effectively deprecated for new drivers plus
> > it would have required you add a lot of ABI docs as every
> > sysfs file would have a strange name.
> >  
> > > diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> > > new file mode 100644
> > > index 000000000000..a981dce988e5
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/ad4030.c  
> >  
> > > +struct ad4030_state {
> > > +	struct spi_device *spi;
> > > +	struct regmap *regmap;
> > > +	const struct ad4030_chip_info *chip;
> > > +	struct gpio_desc *cnv_gpio;
> > > +	int vref_uv;
> > > +	int vio_uv;
> > > +	int offset_avail[3];
> > > +	u32 conversion_speed_hz;
> > > +	enum ad4030_out_mode mode;
> > > +
> > > +	/*
> > > +	 * DMA (thus cache coherency maintenance) requires the transfer buffers
> > > +	 * to live in their own cache lines.
> > > +	 */
> > > +	u8 tx_data[AD4030_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> > > +	struct {
> > > +		union {
> > > +			u8 raw[AD4030_MAXIMUM_RX_BUFFER_SIZE];
> > > +			struct {
> > > +				s32 val;
> > > +				u32 common;
> > > +			} __packed buffered[AD4030_MAX_HARDWARE_CHANNEL_NB];  
> >
> > David pointed out this doesn't need to be packed.
> > Given you have a union here, add __beXX as needed to avoid casts below.  
> 
> They also pointed out that I should reduce the size for the common field.
> I was planing to use an u32 bitfield here, 8 bits for common and 24 bits for
> padding. As far as I understood, the C standard is quite flexible on the
> size used for bitfield, so I should probably keep the __packed, right?
Don't use a bitfield - they are a pain as the C standard is very vague
on the data arrangement. Just use big enough storage and #define x GENMASK()
etc to extract the dta.

> 

> > > +static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
> > > +			   void *val, size_t val_size)
> > > +{
> > > +	struct ad4030_state *st = context;
> > > +
> > > +	struct spi_transfer xfer = {
> > > +		.tx_buf = st->tx_data,
> > > +		.rx_buf = st->rx_data.raw,
> > > +		.len = reg_size + val_size,
> > > +	};
> > > +	int ret;
> > > +
> > > +	memcpy(st->tx_data, reg, reg_size);
> > > +
> > > +	/*
> > > +	 * This should use spi_write_the_read but when doing so, CS never get
> > > +	 * deasserted.  
> >
> > I'm confused.  As a single transfer it won't be deasserted in the transfer
> > whereas spi_write_then_read() will. So is this comment backwards or
> > is it referring to something else?  
> 
> So, with a single transfer (what is done now), the transfer is working
> as expected: CS goes low, the data is transferred, CS goes high again.
> With spi_write_then_read(), CS goes low, data is transferred but CS never
> goes high again. After some time I get a timeout error in the kernel logs.

That's odd.  spi_write_then_read() should not behave differently.
Perhaps a quirk of your SPI controller?

That one is worth digging into as in both cases we should have some
transactions and after that the chip select should behave the same.



> > > +static int ad4030_reset(struct ad4030_state *st)
> > > +{
> > > +	struct device *dev = &st->spi->dev;
> > > +	struct gpio_desc *reset;
> > > +	int ret;
> > > +
> > > +	/* Use GPIO if available ... */
> > > +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(reset))
> > > +		return dev_err_probe(dev, PTR_ERR(reset),
> > > +				"Failed to get reset GPIO\n");
> > > +
> > > +	if (reset) {
> > > +		ndelay(50);
> > > +		gpiod_set_value_cansleep(reset, 0);
> > > +	} else {
> > > +		/* ... falback to software reset otherwise */
> > > +		ret = ad4030_enter_config_mode(st);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		ret = regmap_write(st->regmap, AD4030_REG_INTERFACE_CONFIG_A,
> > > +				   AD4030_REG_INTERFACE_CONFIG_A_SW_RESET);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	/* Wait for reset to complete before communicating to it */  
> >
> > I'd rather see a reference for the value than a generic comment
> > like this.  Also pull the actual value down here. Not particularly
> > useful to have a define for what is a real time unless you are going
> >  to have some combined docs for a bunch of timings (i.e a datasheet
> > table reference)  
> 
> I will put the real value in fsleep call directly. When you say "I'd
> rather see a reference for the value", you ment a reference to the place
> the value is defined in the datasheet, right?
Exactly.

> 
> > > +static int ad4030_detect_chip_info(const struct ad4030_state *st)
> > > +{
> > > +	unsigned int grade;
> > > +	int ret;
> > > +
> > > +	ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
> > > +	if (grade != st->chip->grade)
> > > +		return dev_err_probe(&st->spi->dev, -EINVAL,
> > > +					"Unknown grade(0x%x) for %s\n", grade,
> > > +					st->chip->name);  
> >
> > Is this similar to a missmatch on a whoami value?  
> 
> Yes. It also saved me multiple hours of debuging when the eval board
> was not connected porperly and the SPI link was just not working.
> 
> > I.e. should we print a message and carry on in the interests of providing
> > some degree of support for newer devices on older kernel?
> > (fallback compatibles in DT)  
> 
> Ok, let's go with a warning then.
> 
> > > +static const struct spi_device_id ad4030_id_table[] = {
> > > +	{ "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
> > > +	{}  
> >
> > I'm going to assume you have a bunch of other parts you plan to
> > support soon. Otherwise we normally don't add the chip specific
> > support until it is needed.  It tends to complicate initial driver
> > review a little and experience says that sometimes no other devices
> > are ever added.  
> 
> I'm sending the other devices in the same series (patch 4 and 5).
> For the sake of reducing noise in the later patches, I've put it in
> the initial driver. If you feel like I should wait and do it in the
> following patch (patch 4), I can do that.

Oops. I didn't ready on ;)  Absolutely fine to have this here.

Jonathan

> 
> Thanks for your time,
> 

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Esteban Blanc]

On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote:
> On Thu, 22 Aug 2024 14:45:20 +0200
> Esteban Blanc <eblanc@baylibre.com> wrote:
> > @@ -460,12 +517,21 @@ static int ad4030_conversion(struct ad4030_state *st,
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> > +	if (st->chip->num_channels == 2)
> > +		ad4030_extract_interleaved(st->rx_data.raw,
> > +					   &st->rx_data.diff[0],
> > +					   &st->rx_data.diff[1]);
> > +
> > +	if (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> > +	    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> >  		return 0;
> >  
> >  	byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
> > -	for (i = 0; i < st->chip->num_channels; i++)
> > -		st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
> > +	/* Doing it backward to avoid overlap when reordering */
> > +	for (i = st->chip->num_channels - 1; i > 0; i--) {
> > +		st->rx_data.buffered_common[i].diff = st->rx_data.diff[i];
> > +		st->rx_data.buffered_common[i].common = ((u8 *)&st->rx_data.diff[i])[byte_index];
> > +	}
>
> I wonder if doing it in place is actually worthwhile.  Maybe unpack into a second
> array? That is still fairly small and may make code easier to read.

Okay sure

> > +static const unsigned long ad4630_channel_masks[] = {
> > +	/* Differential only */
> > +	BIT(0) | BIT(2),
> > +	/* Differential with common byte */
> > +	GENMASK(3, 0),
> The packing of data isn't going to be good. How bad to shuffle
> to put the two small channels next to each other?
> Seems like it means you will want to combine your deinterleave
> and channel specific handling above, which is a bit fiddly but
> not much worse than current code.

I can do it since that was what I had done in the RFC in the first place.
Nuno asked for in this email https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/:

>>> * You're pushing the CM channels into the end. So when we a 2 channel device
>>> we'll have:

>>> in_voltage0 - diff
>>> in_voltage1 - diff
>>> in_voltage2 - CM associated with chan0
>>> in_voltage0 - CM associated with chan1
>>>
>>> I think we could make it so the CM channel comes right after the channel where
>>> it's data belongs too. So for example, odd channels would be CM channels (and
>>> labels could also make sense).

So that's what I did here :D

For the software side off things here it doesn't change a lot of things
since we have to manipulate the data anyway, putting the extra byte at the
end or in between is no extra work.
For the offload engine however, it should be easier to ask for 24 bits
then 8 bits for each channel as it would return two u32 per "hardware
channel".

In order to avoid having two different layouts, I was kind of sold by
Nuno's idea of having the CM in between each diff channel.

Thanks for your time,

-- 
Esteban Blanc
BayLibre

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Nuno Sá]

On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote:
> On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote:
> > On Thu, 22 Aug 2024 14:45:20 +0200
> > Esteban Blanc <eblanc@baylibre.com> wrote:
> > > @@ -460,12 +517,21 @@ static int ad4030_conversion(struct ad4030_state *st,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> > > +	if (st->chip->num_channels == 2)
> > > +		ad4030_extract_interleaved(st->rx_data.raw,
> > > +					   &st->rx_data.diff[0],
> > > +					   &st->rx_data.diff[1]);
> > > +
> > > +	if (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> > > +	    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> > >  		return 0;
> > >  
> > >  	byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
> > > -	for (i = 0; i < st->chip->num_channels; i++)
> > > -		st->rx_data.buffered[i].common = ((u8 *)&st-
> > > >rx_data.buffered[i].val)[byte_index];
> > > +	/* Doing it backward to avoid overlap when reordering */
> > > +	for (i = st->chip->num_channels - 1; i > 0; i--) {
> > > +		st->rx_data.buffered_common[i].diff = st->rx_data.diff[i];
> > > +		st->rx_data.buffered_common[i].common = ((u8 *)&st-
> > > >rx_data.diff[i])[byte_index];
> > > +	}
> > 
> > I wonder if doing it in place is actually worthwhile.  Maybe unpack into a second
> > array? That is still fairly small and may make code easier to read.
> 
> Okay sure
> 
> > > +static const unsigned long ad4630_channel_masks[] = {
> > > +	/* Differential only */
> > > +	BIT(0) | BIT(2),
> > > +	/* Differential with common byte */
> > > +	GENMASK(3, 0),
> > The packing of data isn't going to be good. How bad to shuffle
> > to put the two small channels next to each other?
> > Seems like it means you will want to combine your deinterleave
> > and channel specific handling above, which is a bit fiddly but
> > not much worse than current code.
> 
> I can do it since that was what I had done in the RFC in the first place.
> Nuno asked for in this email
> https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/
> :
> 
> > > > * You're pushing the CM channels into the end. So when we a 2 channel device
> > > > we'll have:
> 
> > > > in_voltage0 - diff
> > > > in_voltage1 - diff
> > > > in_voltage2 - CM associated with chan0
> > > > in_voltage0 - CM associated with chan1
> > > > 
> > > > I think we could make it so the CM channel comes right after the channel
> > > > where
> > > > it's data belongs too. So for example, odd channels would be CM channels (and
> > > > labels could also make sense).
> 
> So that's what I did here :D
> 
> For the software side off things here it doesn't change a lot of things
> since we have to manipulate the data anyway, putting the extra byte at the
> end or in between is no extra work.
> For the offload engine however, it should be easier to ask for 24 bits
> then 8 bits for each channel as it would return two u32 per "hardware
> channel".
> 
> In order to avoid having two different layouts, I was kind of sold by
> Nuno's idea of having the CM in between each diff channel.
> 

Tbh, I was not even thinking about the layout when I proposed the arrangement. Just
made sense to me (from a logical point of view) to have them together as they relate
to the same physical channel. FWIW, we're also speaking bytes in here so not sure if
it's that important (or bad).

That said, as we should have labels anyways, I'm not being the blocker if everyone
else prefers to have the RFC layout :)

- Nuno Sá

Re: [PATCH 2/6] iio: adc: ad4030: add driver for ad4030-24

[Nuno Sá]

Hi Esteban,

Just one remark...

On Thu, 2024-08-22 at 14:39 -0500, David Lechner wrote:
> On 8/22/24 7:45 AM, Esteban Blanc wrote:
> > This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> > 
> > The driver implements basic support for the AD4030-24 1 channel
> > differential ADC with hardware gain and offset control.
> > 
> > Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> > ---
> >  MAINTAINERS              |   1 +
> >  drivers/iio/adc/Kconfig  |  13 +
> >  drivers/iio/adc/Makefile |   1 +
> >  drivers/iio/adc/ad4030.c | 854 +++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 869 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f17c42bea19c..6a5a0e7b7a51 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -420,6 +420,7 @@ R:	Esteban Blanc <eblanc@baylibre.com>
> >  S:	Supported
> >  W:	https://ez.analog.com/linux-software-drivers
> >  F:	Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
> > +F:	drivers/iio/adc/ad4030.c
> >  
> >  AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER
> >  M:	Mugilraj Dhavachelvan <dmugil2000@gmail.com>
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 88e8ce2e78b3..f4bd05780f6f 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -33,6 +33,19 @@ config AD4000
> >  	  To compile this driver as a module, choose M here: the module will be
> >  	  called ad4000.
> >  
> > +config AD4030
> > +	tristate "Analog Device AD4630 ADC Driver"
> > +	depends on SPI
> > +	depends on GPIOLIB
> > +	select REGMAP_SPI
> 
> It looks like we are just using REGMAP, not REGMAP_SPI.
> 
> > +	select IIO_BUFFER
> 
> And also select IIO_TRIGGERED_BUFFER?
> 
> > +	help
> > +	  Say yes here to build support for Analog Devices AD4030 and AD4630
> > high speed
> > +	  SPI analog to digital converters (ADC).
> > +
> > +	  To compile this driver as a module, choose M here: the module will be
> > +	  called ad4030.
> > +
> >  config AD4130
> >  	tristate "Analog Device AD4130 ADC Driver"
> >  	depends on SPI
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 8b80664c6d6b..0e4f833abf0b 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -7,6 +7,7 @@
> >  obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
> >  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> >  obj-$(CONFIG_AD4000) += ad4000.o
> > +obj-$(CONFIG_AD4030) += ad4030.o
> >  obj-$(CONFIG_AD4130) += ad4130.o
> >  obj-$(CONFIG_AD4695) += ad4695.o
> >  obj-$(CONFIG_AD7091R) += ad7091r-base.o
> > diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> > new file mode 100644
> > index 000000000000..a981dce988e5
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4030.c
> > @@ -0,0 +1,854 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Analog Devices AD4030 and AD4630 ADC family driver.
> > + *
> > + * Copyright 2024 Analog Devices, Inc.
> > + * Copyright 2024 BayLibre, SAS
> > + *
> > + * based on code from:
> > + *	Analog Devices, Inc.
> > + *	  Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > + *	  Nuno Sa <nuno.sa@analog.com>
> > + *	  Marcelo Schmitt <marcelo.schmitt@analog.com>
> > + *	  Liviu Adace <liviu.adace@analog.com>
> > + */
> > +	.type = IIO_VOLTAGE,						\
> > +	.indexed = 1,							\
> > +	.channel = _idx * 2 + 2,					\
> > +	.scan_index = _idx * 2 + 1,					\
> > +	.extend_name = "Channel" #_idx " common byte part",		\
> 
> Labels are usually one word and reflect the datasheet name.
> 
> Suggest `"common-mode" #_idx` or `"CM" #_idx` for this one.
> 

Also, .extend_name is not to be used anymore... In the end of the day IIO will create
label files anyways but from what I remember about this, extend_name is not to be
directly used this anymore (so other think it's still fine). Instead, use the label
callback.

- Nuno Sá
> 
> 

Re: [PATCH 2/6] iio: adc: ad4030: add driver for ad4030-24

[Esteban Blanc]

On Fri Sep 13, 2024 at 10:22 AM UTC, Nuno Sá wrote:
> Hi Esteban,
>
> Just one remark...
>
> On Thu, 2024-08-22 at 14:39 -0500, David Lechner wrote:
> > On 8/22/24 7:45 AM, Esteban Blanc wrote:
> > > This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> > > 
> > > The driver implements basic support for the AD4030-24 1 channel
> > > differential ADC with hardware gain and offset control.
> > > 
> > > Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> > > ---
> > >  MAINTAINERS              |   1 +
> > >  drivers/iio/adc/Kconfig  |  13 +
> > >  drivers/iio/adc/Makefile |   1 +
> > >  drivers/iio/adc/ad4030.c | 854 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 869 insertions(+)
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index f17c42bea19c..6a5a0e7b7a51 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -420,6 +420,7 @@ R:	Esteban Blanc <eblanc@baylibre.com>
> > >  S:	Supported
> > >  W:	https://ez.analog.com/linux-software-drivers
> > >  F:	Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
> > > +F:	drivers/iio/adc/ad4030.c
> > >  
> > >  AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER
> > >  M:	Mugilraj Dhavachelvan <dmugil2000@gmail.com>
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 88e8ce2e78b3..f4bd05780f6f 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -33,6 +33,19 @@ config AD4000
> > >  	  To compile this driver as a module, choose M here: the module will be
> > >  	  called ad4000.
> > >  
> > > +config AD4030
> > > +	tristate "Analog Device AD4630 ADC Driver"
> > > +	depends on SPI
> > > +	depends on GPIOLIB
> > > +	select REGMAP_SPI
> > 
> > It looks like we are just using REGMAP, not REGMAP_SPI.
> > 
> > > +	select IIO_BUFFER
> > 
> > And also select IIO_TRIGGERED_BUFFER?
> > 
> > > +	help
> > > +	  Say yes here to build support for Analog Devices AD4030 and AD4630
> > > high speed
> > > +	  SPI analog to digital converters (ADC).
> > > +
> > > +	  To compile this driver as a module, choose M here: the module will be
> > > +	  called ad4030.
> > > +
> > >  config AD4130
> > >  	tristate "Analog Device AD4130 ADC Driver"
> > >  	depends on SPI
> > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > > index 8b80664c6d6b..0e4f833abf0b 100644
> > > --- a/drivers/iio/adc/Makefile
> > > +++ b/drivers/iio/adc/Makefile
> > > @@ -7,6 +7,7 @@
> > >  obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
> > >  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> > >  obj-$(CONFIG_AD4000) += ad4000.o
> > > +obj-$(CONFIG_AD4030) += ad4030.o
> > >  obj-$(CONFIG_AD4130) += ad4130.o
> > >  obj-$(CONFIG_AD4695) += ad4695.o
> > >  obj-$(CONFIG_AD7091R) += ad7091r-base.o
> > > diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> > > new file mode 100644
> > > index 000000000000..a981dce988e5
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/ad4030.c
> > > @@ -0,0 +1,854 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Analog Devices AD4030 and AD4630 ADC family driver.
> > > + *
> > > + * Copyright 2024 Analog Devices, Inc.
> > > + * Copyright 2024 BayLibre, SAS
> > > + *
> > > + * based on code from:
> > > + *	Analog Devices, Inc.
> > > + *	  Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > > + *	  Nuno Sa <nuno.sa@analog.com>
> > > + *	  Marcelo Schmitt <marcelo.schmitt@analog.com>
> > > + *	  Liviu Adace <liviu.adace@analog.com>
> > > + */
> > > +	.type = IIO_VOLTAGE,						\
> > > +	.indexed = 1,							\
> > > +	.channel = _idx * 2 + 2,					\
> > > +	.scan_index = _idx * 2 + 1,					\
> > > +	.extend_name = "Channel" #_idx " common byte part",		\
> > 
> > Labels are usually one word and reflect the datasheet name.
> > 
> > Suggest `"common-mode" #_idx` or `"CM" #_idx` for this one.
> > 
>
> Also, .extend_name is not to be used anymore... In the end of the day IIO will create
> label files anyways but from what I remember about this, extend_name is not to be
> directly used this anymore (so other think it's still fine). Instead, use the label
> callback.

Yeah, Jonathan or David mentioned it. I'm using `.read_label` callback
now.

Thanks for your time,

-- 
Esteban Blanc
BayLibre

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Esteban Blanc]

On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote:
> On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote:
> > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote:
> > > On Thu, 22 Aug 2024 14:45:20 +0200
> > > Esteban Blanc <eblanc@baylibre.com> wrote:
> > > > +static const unsigned long ad4630_channel_masks[] = {
> > > > +	/* Differential only */
> > > > +	BIT(0) | BIT(2),
> > > > +	/* Differential with common byte */
> > > > +	GENMASK(3, 0),
> > > The packing of data isn't going to be good. How bad to shuffle
> > > to put the two small channels next to each other?
> > > Seems like it means you will want to combine your deinterleave
> > > and channel specific handling above, which is a bit fiddly but
> > > not much worse than current code.
> > 
> > I can do it since that was what I had done in the RFC in the first place.
> > Nuno asked for in this email
> > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/
> > :
> > 
> > > > > * You're pushing the CM channels into the end. So when we a 2 channel device
> > > > > we'll have:
> > 
> > > > > in_voltage0 - diff
> > > > > in_voltage1 - diff
> > > > > in_voltage2 - CM associated with chan0
> > > > > in_voltage0 - CM associated with chan1
> > > > > 
> > > > > I think we could make it so the CM channel comes right after the channel
> > > > > where
> > > > > it's data belongs too. So for example, odd channels would be CM channels (and
> > > > > labels could also make sense).
> > 
> > So that's what I did here :D
> > 
> > For the software side off things here it doesn't change a lot of things
> > since we have to manipulate the data anyway, putting the extra byte at the
> > end or in between is no extra work.
> > For the offload engine however, it should be easier to ask for 24 bits
> > then 8 bits for each channel as it would return two u32 per "hardware
> > channel".
> > 
> > In order to avoid having two different layouts, I was kind of sold by
> > Nuno's idea of having the CM in between each diff channel.
> > 
>
> Tbh, I was not even thinking about the layout when I proposed the arrangement. Just
> made sense to me (from a logical point of view) to have them together as they relate
> to the same physical channel. FWIW, we're also speaking bytes in here so not sure if
> it's that important (or bad).

The best we can do (if we managed to do it HDL wise) is to reorder the
data to get both CM byte in a single u32 after the 2 u32 of both diff
channel. That would be 3 u32 instead of 4.

I don't have a strong opinion other than what we have with the V1 should
be simpler to rework for the offload engine since we can manage to
get the same layout from the offload engine. And if there is
some performance issue we could still try to rework the HDL but I can't
say for sure if there will be some drawback with that.

If you or Jonathan prefers the RFC version let's do that, no problem I
already have the code for it :D

Best regards,

-- 
Esteban "Skallwar" Blanc
BayLibre

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Nuno Sá]

On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote:
> On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote:
> > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote:
> > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote:
> > > > On Thu, 22 Aug 2024 14:45:20 +0200
> > > > Esteban Blanc <eblanc@baylibre.com> wrote:
> > > > > +static const unsigned long ad4630_channel_masks[] = {
> > > > > +	/* Differential only */
> > > > > +	BIT(0) | BIT(2),
> > > > > +	/* Differential with common byte */
> > > > > +	GENMASK(3, 0),
> > > > The packing of data isn't going to be good. How bad to shuffle
> > > > to put the two small channels next to each other?
> > > > Seems like it means you will want to combine your deinterleave
> > > > and channel specific handling above, which is a bit fiddly but
> > > > not much worse than current code.
> > > 
> > > I can do it since that was what I had done in the RFC in the first place.
> > > Nuno asked for in this email
> > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/
> > > :
> > > 
> > > > > > * You're pushing the CM channels into the end. So when we a 2 channel
> > > > > > device
> > > > > > we'll have:
> > > 
> > > > > > in_voltage0 - diff
> > > > > > in_voltage1 - diff
> > > > > > in_voltage2 - CM associated with chan0
> > > > > > in_voltage0 - CM associated with chan1
> > > > > > 
> > > > > > I think we could make it so the CM channel comes right after the channel
> > > > > > where
> > > > > > it's data belongs too. So for example, odd channels would be CM channels
> > > > > > (and
> > > > > > labels could also make sense).
> > > 
> > > So that's what I did here :D
> > > 
> > > For the software side off things here it doesn't change a lot of things
> > > since we have to manipulate the data anyway, putting the extra byte at the
> > > end or in between is no extra work.
> > > For the offload engine however, it should be easier to ask for 24 bits
> > > then 8 bits for each channel as it would return two u32 per "hardware
> > > channel".
> > > 
> > > In order to avoid having two different layouts, I was kind of sold by
> > > Nuno's idea of having the CM in between each diff channel.
> > > 
> > 
> > Tbh, I was not even thinking about the layout when I proposed the arrangement.
> > Just
> > made sense to me (from a logical point of view) to have them together as they
> > relate
> > to the same physical channel. FWIW, we're also speaking bytes in here so not sure
> > if
> > it's that important (or bad).
> 
> The best we can do (if we managed to do it HDL wise) is to reorder the
> data to get both CM byte in a single u32 after the 2 u32 of both diff
> channel. That would be 3 u32 instead of 4.
> 

We are starting to see more and more devices that do stuff like this. Have one
physical channel that reflects in more than one IIO channel. For SW buffering it's
not really a big deal but for HW buffering it's not ideal. 

I feel that at some point we should think about having a way to map a channel scan
element (being kind of a virtual scan element) into the storage_bits of another one.
So in this case, one sample (for one channel) would be the 32bits and things should
work the same either in SW or HW buffering.

That said, it's probably easier said than done in practice :)

- Nuno Sá

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Jonathan Cameron]

On Fri, 13 Sep 2024 15:46:17 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote:
> > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote:  
> > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote:  
> > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote:  
> > > > > On Thu, 22 Aug 2024 14:45:20 +0200
> > > > > Esteban Blanc <eblanc@baylibre.com> wrote:  
> > > > > > +static const unsigned long ad4630_channel_masks[] = {
> > > > > > +	/* Differential only */
> > > > > > +	BIT(0) | BIT(2),
> > > > > > +	/* Differential with common byte */
> > > > > > +	GENMASK(3, 0),  
> > > > > The packing of data isn't going to be good. How bad to shuffle
> > > > > to put the two small channels next to each other?
> > > > > Seems like it means you will want to combine your deinterleave
> > > > > and channel specific handling above, which is a bit fiddly but
> > > > > not much worse than current code.  
> > > > 
> > > > I can do it since that was what I had done in the RFC in the first place.
> > > > Nuno asked for in this email
> > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/
> > > > :
> > > >   
> > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel
> > > > > > > device
> > > > > > > we'll have:  
> > > >   
> > > > > > > in_voltage0 - diff
> > > > > > > in_voltage1 - diff
> > > > > > > in_voltage2 - CM associated with chan0
> > > > > > > in_voltage0 - CM associated with chan1
> > > > > > > 
> > > > > > > I think we could make it so the CM channel comes right after the channel
> > > > > > > where
> > > > > > > it's data belongs too. So for example, odd channels would be CM channels
> > > > > > > (and
> > > > > > > labels could also make sense).  
> > > > 
> > > > So that's what I did here :D
> > > > 
> > > > For the software side off things here it doesn't change a lot of things
> > > > since we have to manipulate the data anyway, putting the extra byte at the
> > > > end or in between is no extra work.
> > > > For the offload engine however, it should be easier to ask for 24 bits
> > > > then 8 bits for each channel as it would return two u32 per "hardware
> > > > channel".
> > > > 
> > > > In order to avoid having two different layouts, I was kind of sold by
> > > > Nuno's idea of having the CM in between each diff channel.
> > > >   
> > > 
> > > Tbh, I was not even thinking about the layout when I proposed the arrangement.
> > > Just
> > > made sense to me (from a logical point of view) to have them together as they
> > > relate
> > > to the same physical channel. FWIW, we're also speaking bytes in here so not sure
> > > if
> > > it's that important (or bad).  
> > 
> > The best we can do (if we managed to do it HDL wise) is to reorder the
> > data to get both CM byte in a single u32 after the 2 u32 of both diff
> > channel. That would be 3 u32 instead of 4.

Entirely up to you. :)
> >   
> 
> We are starting to see more and more devices that do stuff like this. Have one
> physical channel that reflects in more than one IIO channel. For SW buffering it's
> not really a big deal but for HW buffering it's not ideal. 
> 
> I feel that at some point we should think about having a way to map a channel scan
> element (being kind of a virtual scan element) into the storage_bits of another one.
> So in this case, one sample (for one channel) would be the 32bits and things should
> work the same either in SW or HW buffering.
> 
> That said, it's probably easier said than done in practice :)

Yeah. That could get ugly fast + All existing userspace will fail to handle it
so I'm not keen. Maybe it's doable if we assume the 'virtual channels' are all
meta data we don't mind loosing with existing software stacks and define
a non overlapping ABI to identify the metadata.  Still smells bad to me so
I'll take quite a bit of convincing!

Adding something to clearly 'associate' multiple related channels would be fine
as that wouldn't change the data interpretation, just provide more info on top.
Kind of a structured _label 

Maybe a _channelgroup attribute?   Would be const and all the channels with
the same index would reflect that they were measured on same 'thing'.
Typically thing might be a pin or differential pair, but we might be measuring
different types of signals - e.g. current and power.

Joanthan

> 
> - Nuno Sá
> 

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Nuno Sá]

On Sat, 2024-09-14 at 12:25 +0100, Jonathan Cameron wrote:
> On Fri, 13 Sep 2024 15:46:17 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote:
> > > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote:  
> > > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote:  
> > > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote:  
> > > > > > On Thu, 22 Aug 2024 14:45:20 +0200
> > > > > > Esteban Blanc <eblanc@baylibre.com> wrote:  
> > > > > > > +static const unsigned long ad4630_channel_masks[] = {
> > > > > > > +	/* Differential only */
> > > > > > > +	BIT(0) | BIT(2),
> > > > > > > +	/* Differential with common byte */
> > > > > > > +	GENMASK(3, 0),  
> > > > > > The packing of data isn't going to be good. How bad to shuffle
> > > > > > to put the two small channels next to each other?
> > > > > > Seems like it means you will want to combine your deinterleave
> > > > > > and channel specific handling above, which is a bit fiddly but
> > > > > > not much worse than current code.  
> > > > > 
> > > > > I can do it since that was what I had done in the RFC in the first place.
> > > > > Nuno asked for in this email
> > > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/
> > > > > :
> > > > >   
> > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel
> > > > > > > > device
> > > > > > > > we'll have:  
> > > > >   
> > > > > > > > in_voltage0 - diff
> > > > > > > > in_voltage1 - diff
> > > > > > > > in_voltage2 - CM associated with chan0
> > > > > > > > in_voltage0 - CM associated with chan1
> > > > > > > > 
> > > > > > > > I think we could make it so the CM channel comes right after the
> > > > > > > > channel
> > > > > > > > where
> > > > > > > > it's data belongs too. So for example, odd channels would be CM
> > > > > > > > channels
> > > > > > > > (and
> > > > > > > > labels could also make sense).  
> > > > > 
> > > > > So that's what I did here :D
> > > > > 
> > > > > For the software side off things here it doesn't change a lot of things
> > > > > since we have to manipulate the data anyway, putting the extra byte at the
> > > > > end or in between is no extra work.
> > > > > For the offload engine however, it should be easier to ask for 24 bits
> > > > > then 8 bits for each channel as it would return two u32 per "hardware
> > > > > channel".
> > > > > 
> > > > > In order to avoid having two different layouts, I was kind of sold by
> > > > > Nuno's idea of having the CM in between each diff channel.
> > > > >   
> > > > 
> > > > Tbh, I was not even thinking about the layout when I proposed the
> > > > arrangement.
> > > > Just
> > > > made sense to me (from a logical point of view) to have them together as they
> > > > relate
> > > > to the same physical channel. FWIW, we're also speaking bytes in here so not
> > > > sure
> > > > if
> > > > it's that important (or bad).  
> > > 
> > > The best we can do (if we managed to do it HDL wise) is to reorder the
> > > data to get both CM byte in a single u32 after the 2 u32 of both diff
> > > channel. That would be 3 u32 instead of 4.
> 
> Entirely up to you. :)
> > >   
> > 
> > We are starting to see more and more devices that do stuff like this. Have one
> > physical channel that reflects in more than one IIO channel. For SW buffering
> > it's
> > not really a big deal but for HW buffering it's not ideal. 
> > 
> > I feel that at some point we should think about having a way to map a channel
> > scan
> > element (being kind of a virtual scan element) into the storage_bits of another
> > one.
> > So in this case, one sample (for one channel) would be the 32bits and things
> > should
> > work the same either in SW or HW buffering.
> > 
> > That said, it's probably easier said than done in practice :)
> 
> Yeah. That could get ugly fast + All existing userspace will fail to handle it
> so I'm not keen. Maybe it's doable if we assume the 'virtual channels' are all
> meta data we don't mind loosing with existing software stacks and define
> a non overlapping ABI to identify the metadata.  Still smells bad to me so
> I'll take quite a bit of convincing!

Naturally it would have to be done in a way that drivers not defining the "special"
scan elements would not be affected.

> 
> Adding something to clearly 'associate' multiple related channels would be fine
> as that wouldn't change the data interpretation, just provide more info on top.
> Kind of a structured _label 
> 
> Maybe a _channelgroup attribute?   Would be const and all the channels with
> the same index would reflect that they were measured on same 'thing'.
> Typically thing might be a pin or differential pair, but we might be measuring
> different types of signals - e.g. current and power.
> 

Sounds reasonable but I think the tricky part is always to have a sane way of saying
that multiple scan elements relate to just one storage_bits so we could say something
like (taking this as example):

scan0: //diff channel which describing the physical HW in terms of real size
 .storage_bits = 32
 .real_bits = 24
 .shift = 8

scan1: //CM data
 //.storage - relates to scan0 so should add nothing to the sample size if both
enabled
 .real_bits = 8

Likely not what you meant but one thing I took from your '_channelgroup' idea was to
have something similar to extended_info maybe with a small top level description and
then an array of channels (that would form the group/aggregated channel). Only on the
top level description we would be allowed to define the size of the scan element (in
case of buffering). Still seems tricky to me :).

Anyways, Right now, I have no time for something like this but eventually would like
to try something. But if someone wants to propose something sooner, please :)

- Nuno Sá 

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Esteban Blanc]

On Sat Sep 14, 2024 at 11:25 AM UTC, Jonathan Cameron wrote:
> On Fri, 13 Sep 2024 15:46:17 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> > On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote:
> > > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote:  
> > > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote:  
> > > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote:  
> > > > > > On Thu, 22 Aug 2024 14:45:20 +0200
> > > > > > Esteban Blanc <eblanc@baylibre.com> wrote:  
> > > > > > > +static const unsigned long ad4630_channel_masks[] = {
> > > > > > > +	/* Differential only */
> > > > > > > +	BIT(0) | BIT(2),
> > > > > > > +	/* Differential with common byte */
> > > > > > > +	GENMASK(3, 0),  
> > > > > > The packing of data isn't going to be good. How bad to shuffle
> > > > > > to put the two small channels next to each other?
> > > > > > Seems like it means you will want to combine your deinterleave
> > > > > > and channel specific handling above, which is a bit fiddly but
> > > > > > not much worse than current code.  
> > > > > 
> > > > > I can do it since that was what I had done in the RFC in the first place.
> > > > > Nuno asked for in this email
> > > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/
> > > > > :
> > > > >   
> > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel
> > > > > > > > device
> > > > > > > > we'll have:  
> > > > >   
> > > > > > > > in_voltage0 - diff
> > > > > > > > in_voltage1 - diff
> > > > > > > > in_voltage2 - CM associated with chan0
> > > > > > > > in_voltage0 - CM associated with chan1
> > > > > > > > 
> > > > > > > > I think we could make it so the CM channel comes right after the channel
> > > > > > > > where
> > > > > > > > it's data belongs too. So for example, odd channels would be CM channels
> > > > > > > > (and
> > > > > > > > labels could also make sense).  
> > > > > 
> > > > > So that's what I did here :D
> > > > > 
> > > > > For the software side off things here it doesn't change a lot of things
> > > > > since we have to manipulate the data anyway, putting the extra byte at the
> > > > > end or in between is no extra work.
> > > > > For the offload engine however, it should be easier to ask for 24 bits
> > > > > then 8 bits for each channel as it would return two u32 per "hardware
> > > > > channel".
> > > > > 
> > > > > In order to avoid having two different layouts, I was kind of sold by
> > > > > Nuno's idea of having the CM in between each diff channel.
> > > > >   
> > > > 
> > > > Tbh, I was not even thinking about the layout when I proposed the arrangement.
> > > > Just
> > > > made sense to me (from a logical point of view) to have them together as they
> > > > relate
> > > > to the same physical channel. FWIW, we're also speaking bytes in here so not sure
> > > > if
> > > > it's that important (or bad).  
> > > 
> > > The best we can do (if we managed to do it HDL wise) is to reorder the
> > > data to get both CM byte in a single u32 after the 2 u32 of both diff
> > > channel. That would be 3 u32 instead of 4.
>
> Entirely up to you. :)

Ok so here is the plan I propose:
 1. Use the layout of this patch (common byte channels just after their
 respective diff channel) as it should work out of the box for the offload
 engine (once it's merged [1]).
 2. In case of performance issue, switch to the RFC layout (both diff
 channels then both common byte channels) and try to modify the HDL for
 the offload engine to reduce the memory footprint by one byte for the 2
 hardware channels case.

[1]: https://lore.kernel.org/lkml/20240722-dlech-mainline-spi-engine-offload-2-v3-0-7420e45df69b@baylibre.com/

Best regards,

-- 
Esteban Blanc
BayLibre

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Esteban Blanc]

On Fri Sep 13, 2024 at 9:55 AM UTC, Esteban Blanc wrote:
> On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote:
> > On Thu, 22 Aug 2024 14:45:20 +0200
> > Esteban Blanc <eblanc@baylibre.com> wrote:
> > > @@ -460,12 +517,21 @@ static int ad4030_conversion(struct ad4030_state *st,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> > > +	if (st->chip->num_channels == 2)
> > > +		ad4030_extract_interleaved(st->rx_data.raw,
> > > +					   &st->rx_data.diff[0],
> > > +					   &st->rx_data.diff[1]);
> > > +
> > > +	if (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> > > +	    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> > >  		return 0;
> > >  
> > >  	byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
> > > -	for (i = 0; i < st->chip->num_channels; i++)
> > > -		st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
> > > +	/* Doing it backward to avoid overlap when reordering */
> > > +	for (i = st->chip->num_channels - 1; i > 0; i--) {
> > > +		st->rx_data.buffered_common[i].diff = st->rx_data.diff[i];
> > > +		st->rx_data.buffered_common[i].common = ((u8 *)&st->rx_data.diff[i])[byte_index];
> > > +	}
> >
> > I wonder if doing it in place is actually worthwhile.  Maybe unpack into a second
> > array? That is still fairly small and may make code easier to read.
>
> Okay sure

Actually I can't consolidate the differential only mode and the common
byte mode without having to create a bunch of if/else or having a
memcpy. The best I can do is this, but I don't like it:

```
static int ad4030_conversion(struct ad4030_state *st,
			     const struct iio_chan_spec *chan)
{
	...
	u32 tmp[AD4030_MAX_HARDWARE_CHANNEL_NB];
	u32 *diff;

	...

	if (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
	    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM) {
		if (st->chip->num_voltage_inputs == 2)
			ad4030_extract_interleaved(st->rx_data.raw,
						   &st->rx_data.diff[0],
						   &st->rx_data.diff[1]);
		return 0;
	}

	if (st->chip->num_voltage_inputs == 2) {
		ad4030_extract_interleaved(st->rx_data.raw,
					   &tmp[0],
					   &tmp[1]);
		diff = tmp;
	} else {
		diff = st->rx_data.diff;
	}

	common_byte_mask = BITS_TO_BYTES(chan->scan_type.realbits);
	for (i = 0; i < st->chip->num_voltage_inputs; i++) {
		st->rx_data.buffered[i].val = diff[i];
		st->rx_data.buffered[i].common =
			((u8 *)(diff + i))[common_byte_mask];
	}

	return 0;
```

The root cause is that when we are in a differential only mode we are
leaving before the for loop and we want the data to be in the rx_data.

It fells clunky and brain consuming IMAO, I prefer a reversed loop with
a good comment (the one we have now is not explicit enough, I will
update it).

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Jonathan Cameron]

On Mon, 16 Sep 2024 08:12:24 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sat, 2024-09-14 at 12:25 +0100, Jonathan Cameron wrote:
> > On Fri, 13 Sep 2024 15:46:17 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >   
> > > On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote:  
> > > > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote:    
> > > > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote:    
> > > > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote:    
> > > > > > > On Thu, 22 Aug 2024 14:45:20 +0200
> > > > > > > Esteban Blanc <eblanc@baylibre.com> wrote:    
> > > > > > > > +static const unsigned long ad4630_channel_masks[] = {
> > > > > > > > +	/* Differential only */
> > > > > > > > +	BIT(0) | BIT(2),
> > > > > > > > +	/* Differential with common byte */
> > > > > > > > +	GENMASK(3, 0),    
> > > > > > > The packing of data isn't going to be good. How bad to shuffle
> > > > > > > to put the two small channels next to each other?
> > > > > > > Seems like it means you will want to combine your deinterleave
> > > > > > > and channel specific handling above, which is a bit fiddly but
> > > > > > > not much worse than current code.    
> > > > > > 
> > > > > > I can do it since that was what I had done in the RFC in the first place.
> > > > > > Nuno asked for in this email
> > > > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/
> > > > > > :
> > > > > >     
> > > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel
> > > > > > > > > device
> > > > > > > > > we'll have:    
> > > > > >     
> > > > > > > > > in_voltage0 - diff
> > > > > > > > > in_voltage1 - diff
> > > > > > > > > in_voltage2 - CM associated with chan0
> > > > > > > > > in_voltage0 - CM associated with chan1
> > > > > > > > > 
> > > > > > > > > I think we could make it so the CM channel comes right after the
> > > > > > > > > channel
> > > > > > > > > where
> > > > > > > > > it's data belongs too. So for example, odd channels would be CM
> > > > > > > > > channels
> > > > > > > > > (and
> > > > > > > > > labels could also make sense).    
> > > > > > 
> > > > > > So that's what I did here :D
> > > > > > 
> > > > > > For the software side off things here it doesn't change a lot of things
> > > > > > since we have to manipulate the data anyway, putting the extra byte at the
> > > > > > end or in between is no extra work.
> > > > > > For the offload engine however, it should be easier to ask for 24 bits
> > > > > > then 8 bits for each channel as it would return two u32 per "hardware
> > > > > > channel".
> > > > > > 
> > > > > > In order to avoid having two different layouts, I was kind of sold by
> > > > > > Nuno's idea of having the CM in between each diff channel.
> > > > > >     
> > > > > 
> > > > > Tbh, I was not even thinking about the layout when I proposed the
> > > > > arrangement.
> > > > > Just
> > > > > made sense to me (from a logical point of view) to have them together as they
> > > > > relate
> > > > > to the same physical channel. FWIW, we're also speaking bytes in here so not
> > > > > sure
> > > > > if
> > > > > it's that important (or bad).    
> > > > 
> > > > The best we can do (if we managed to do it HDL wise) is to reorder the
> > > > data to get both CM byte in a single u32 after the 2 u32 of both diff
> > > > channel. That would be 3 u32 instead of 4.  
> > 
> > Entirely up to you. :)  
> > > >     
> > > 
> > > We are starting to see more and more devices that do stuff like this. Have one
> > > physical channel that reflects in more than one IIO channel. For SW buffering
> > > it's
> > > not really a big deal but for HW buffering it's not ideal. 
> > > 
> > > I feel that at some point we should think about having a way to map a channel
> > > scan
> > > element (being kind of a virtual scan element) into the storage_bits of another
> > > one.
> > > So in this case, one sample (for one channel) would be the 32bits and things
> > > should
> > > work the same either in SW or HW buffering.
> > > 
> > > That said, it's probably easier said than done in practice :)  
> > 
> > Yeah. That could get ugly fast + All existing userspace will fail to handle it
> > so I'm not keen. Maybe it's doable if we assume the 'virtual channels' are all
> > meta data we don't mind loosing with existing software stacks and define
> > a non overlapping ABI to identify the metadata.  Still smells bad to me so
> > I'll take quite a bit of convincing!  
> 
> Naturally it would have to be done in a way that drivers not defining the "special"
> scan elements would not be affected.

It's worse than that - it would need be defined so userspace running
against the devices with the special channels would have to work without
knowing anything about them. So we couldn't do the really nasty thing
of setting scan_index the same for both of them with same storage size and
different shifts and real_bits.  That would be the sort of things that might
crash userspace code.

Driver effects are less of an issue than ABI breakage - or even just
ABI a userspace author would not expect.

> 
> > 
> > Adding something to clearly 'associate' multiple related channels would be fine
> > as that wouldn't change the data interpretation, just provide more info on top.
> > Kind of a structured _label 
> > 
> > Maybe a _channelgroup attribute?   Would be const and all the channels with
> > the same index would reflect that they were measured on same 'thing'.
> > Typically thing might be a pin or differential pair, but we might be measuring
> > different types of signals - e.g. current and power.
> >   
> 
> Sounds reasonable but I think the tricky part is always to have a sane way of saying
> that multiple scan elements relate to just one storage_bits so we could say something
> like (taking this as example):
> 
> scan0: //diff channel which describing the physical HW in terms of real size
>  .storage_bits = 32
>  .real_bits = 24
>  .shift = 8
> 
> scan1: //CM data
>  //.storage - relates to scan0 so should add nothing to the sample size if both
> enabled
>  .real_bits = 8
> 
Indeed - I get the concept, but don't like it. 
In general it's a dead end for general purpose channels - because of that
pile of legacy userspace.  It 'might' just about be acceptable for 'meta data' channels
or where we are adding significant new interface for functionality purposes (e.g.
when we did the newer DMA buffer stuff).

> Likely not what you meant but one thing I took from your '_channelgroup' idea was to
> have something similar to extended_info maybe with a small top level description and
> then an array of channels (that would form the group/aggregated channel). Only on the
> top level description we would be allowed to define the size of the scan element (in
> case of buffering). Still seems tricky to me :).

Yeah.  The channel group thing was for normal naturally aligned packing, not
data backed tighter than that.

> 
> Anyways, Right now, I have no time for something like this but eventually would like
> to try something. But if someone wants to propose something sooner, please :)

*looks doubtful*  Maybe I can be convinced.  We'll see.

Jonathan
> 
> - Nuno Sá 
> 

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Jonathan Cameron]

On Mon, 16 Sep 2024 09:19:02 +0000
"Esteban Blanc" <eblanc@baylibre.com> wrote:

> On Sat Sep 14, 2024 at 11:25 AM UTC, Jonathan Cameron wrote:
> > On Fri, 13 Sep 2024 15:46:17 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >  
> > > On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote:  
> > > > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote:    
> > > > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote:    
> > > > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote:    
> > > > > > > On Thu, 22 Aug 2024 14:45:20 +0200
> > > > > > > Esteban Blanc <eblanc@baylibre.com> wrote:    
> > > > > > > > +static const unsigned long ad4630_channel_masks[] = {
> > > > > > > > +	/* Differential only */
> > > > > > > > +	BIT(0) | BIT(2),
> > > > > > > > +	/* Differential with common byte */
> > > > > > > > +	GENMASK(3, 0),    
> > > > > > > The packing of data isn't going to be good. How bad to shuffle
> > > > > > > to put the two small channels next to each other?
> > > > > > > Seems like it means you will want to combine your deinterleave
> > > > > > > and channel specific handling above, which is a bit fiddly but
> > > > > > > not much worse than current code.    
> > > > > > 
> > > > > > I can do it since that was what I had done in the RFC in the first place.
> > > > > > Nuno asked for in this email
> > > > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/
> > > > > > :
> > > > > >     
> > > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel
> > > > > > > > > device
> > > > > > > > > we'll have:    
> > > > > >     
> > > > > > > > > in_voltage0 - diff
> > > > > > > > > in_voltage1 - diff
> > > > > > > > > in_voltage2 - CM associated with chan0
> > > > > > > > > in_voltage0 - CM associated with chan1
> > > > > > > > > 
> > > > > > > > > I think we could make it so the CM channel comes right after the channel
> > > > > > > > > where
> > > > > > > > > it's data belongs too. So for example, odd channels would be CM channels
> > > > > > > > > (and
> > > > > > > > > labels could also make sense).    
> > > > > > 
> > > > > > So that's what I did here :D
> > > > > > 
> > > > > > For the software side off things here it doesn't change a lot of things
> > > > > > since we have to manipulate the data anyway, putting the extra byte at the
> > > > > > end or in between is no extra work.
> > > > > > For the offload engine however, it should be easier to ask for 24 bits
> > > > > > then 8 bits for each channel as it would return two u32 per "hardware
> > > > > > channel".
> > > > > > 
> > > > > > In order to avoid having two different layouts, I was kind of sold by
> > > > > > Nuno's idea of having the CM in between each diff channel.
> > > > > >     
> > > > > 
> > > > > Tbh, I was not even thinking about the layout when I proposed the arrangement.
> > > > > Just
> > > > > made sense to me (from a logical point of view) to have them together as they
> > > > > relate
> > > > > to the same physical channel. FWIW, we're also speaking bytes in here so not sure
> > > > > if
> > > > > it's that important (or bad).    
> > > > 
> > > > The best we can do (if we managed to do it HDL wise) is to reorder the
> > > > data to get both CM byte in a single u32 after the 2 u32 of both diff
> > > > channel. That would be 3 u32 instead of 4.  
> >
> > Entirely up to you. :)  
> 
> Ok so here is the plan I propose:
>  1. Use the layout of this patch (common byte channels just after their
>  respective diff channel) as it should work out of the box for the offload
>  engine (once it's merged [1]).
>  2. In case of performance issue, switch to the RFC layout (both diff
>  channels then both common byte channels) and try to modify the HDL for
>  the offload engine to reduce the memory footprint by one byte for the 2
>  hardware channels case.
It's a bit of a risk as you might get someone making too many assumptions about
channel ordering in some specific purpose code - at that point this change
is an ABI break that someone noticed.

So I'd guess we'll stay with 1 for ever which is find.

Jonathan


> 
> [1]: https://lore.kernel.org/lkml/20240722-dlech-mainline-spi-engine-offload-2-v3-0-7420e45df69b@baylibre.com/
> 
> Best regards,
> 

Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Jonathan Cameron]

On Mon, 16 Sep 2024 08:12:24 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sat, 2024-09-14 at 12:25 +0100, Jonathan Cameron wrote:
> > On Fri, 13 Sep 2024 15:46:17 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >   
> > > On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote:  
> > > > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote:    
> > > > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote:    
> > > > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote:    
> > > > > > > On Thu, 22 Aug 2024 14:45:20 +0200
> > > > > > > Esteban Blanc <eblanc@baylibre.com> wrote:    
> > > > > > > > +static const unsigned long ad4630_channel_masks[] = {
> > > > > > > > +	/* Differential only */
> > > > > > > > +	BIT(0) | BIT(2),
> > > > > > > > +	/* Differential with common byte */
> > > > > > > > +	GENMASK(3, 0),    
> > > > > > > The packing of data isn't going to be good. How bad to shuffle
> > > > > > > to put the two small channels next to each other?
> > > > > > > Seems like it means you will want to combine your deinterleave
> > > > > > > and channel specific handling above, which is a bit fiddly but
> > > > > > > not much worse than current code.    
> > > > > > 
> > > > > > I can do it since that was what I had done in the RFC in the first place.
> > > > > > Nuno asked for in this email
> > > > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/
> > > > > > :
> > > > > >     
> > > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel
> > > > > > > > > device
> > > > > > > > > we'll have:    
> > > > > >     
> > > > > > > > > in_voltage0 - diff
> > > > > > > > > in_voltage1 - diff
> > > > > > > > > in_voltage2 - CM associated with chan0
> > > > > > > > > in_voltage0 - CM associated with chan1
> > > > > > > > > 
> > > > > > > > > I think we could make it so the CM channel comes right after the
> > > > > > > > > channel
> > > > > > > > > where
> > > > > > > > > it's data belongs too. So for example, odd channels would be CM
> > > > > > > > > channels
> > > > > > > > > (and
> > > > > > > > > labels could also make sense).    
> > > > > > 
> > > > > > So that's what I did here :D
> > > > > > 
> > > > > > For the software side off things here it doesn't change a lot of things
> > > > > > since we have to manipulate the data anyway, putting the extra byte at the
> > > > > > end or in between is no extra work.
> > > > > > For the offload engine however, it should be easier to ask for 24 bits
> > > > > > then 8 bits for each channel as it would return two u32 per "hardware
> > > > > > channel".
> > > > > > 
> > > > > > In order to avoid having two different layouts, I was kind of sold by
> > > > > > Nuno's idea of having the CM in between each diff channel.
> > > > > >     
> > > > > 
> > > > > Tbh, I was not even thinking about the layout when I proposed the
> > > > > arrangement.
> > > > > Just
> > > > > made sense to me (from a logical point of view) to have them together as they
> > > > > relate
> > > > > to the same physical channel. FWIW, we're also speaking bytes in here so not
> > > > > sure
> > > > > if
> > > > > it's that important (or bad).    
> > > > 
> > > > The best we can do (if we managed to do it HDL wise) is to reorder the
> > > > data to get both CM byte in a single u32 after the 2 u32 of both diff
> > > > channel. That would be 3 u32 instead of 4.  
> > 
> > Entirely up to you. :)  
> > > >     
> > > 
> > > We are starting to see more and more devices that do stuff like this. Have one
> > > physical channel that reflects in more than one IIO channel. For SW buffering
> > > it's
> > > not really a big deal but for HW buffering it's not ideal. 
> > > 
> > > I feel that at some point we should think about having a way to map a channel
> > > scan
> > > element (being kind of a virtual scan element) into the storage_bits of another
> > > one.
> > > So in this case, one sample (for one channel) would be the 32bits and things
> > > should
> > > work the same either in SW or HW buffering.
> > > 
> > > That said, it's probably easier said than done in practice :)  
> > 
> > Yeah. That could get ugly fast + All existing userspace will fail to handle it
> > so I'm not keen. Maybe it's doable if we assume the 'virtual channels' are all
> > meta data we don't mind loosing with existing software stacks and define
> > a non overlapping ABI to identify the metadata.  Still smells bad to me so
> > I'll take quite a bit of convincing!  
> 
> Naturally it would have to be done in a way that drivers not defining the "special"
> scan elements would not be affected.

It's worse than that - it would need be defined so userspace running
against the devices with the special channels would have to work without
knowing anything about them. So we couldn't do the really nasty thing
of setting scan_index the same for both of them with same storage size and
different shifts and real_bits.  That would be the sort of things that might
crash userspace code.

Driver effects are less of an issue than ABI breakage - or even just
ABI a userspace author would not expect.

> 
> > 
> > Adding something to clearly 'associate' multiple related channels would be fine
> > as that wouldn't change the data interpretation, just provide more info on top.
> > Kind of a structured _label 
> > 
> > Maybe a _channelgroup attribute?   Would be const and all the channels with
> > the same index would reflect that they were measured on same 'thing'.
> > Typically thing might be a pin or differential pair, but we might be measuring
> > different types of signals - e.g. current and power.
> >   
> 
> Sounds reasonable but I think the tricky part is always to have a sane way of saying
> that multiple scan elements relate to just one storage_bits so we could say something
> like (taking this as example):
> 
> scan0: //diff channel which describing the physical HW in terms of real size
>  .storage_bits = 32
>  .real_bits = 24
>  .shift = 8
> 
> scan1: //CM data
>  //.storage - relates to scan0 so should add nothing to the sample size if both
> enabled
>  .real_bits = 8
> 
Indeed - I get the concept, but don't like it. 
In general it's a dead end for general purpose channels - because of that
pile of legacy userspace.  It 'might' just about be acceptable for 'meta data' channels
or where we are adding significant new interface for functionality purposes (e.g.
when we did the newer DMA buffer stuff).

> Likely not what you meant but one thing I took from your '_channelgroup' idea was to
> have something similar to extended_info maybe with a small top level description and
> then an array of channels (that would form the group/aggregated channel). Only on the
> top level description we would be allowed to define the size of the scan element (in
> case of buffering). Still seems tricky to me :).

Yeah.  The channel group thing was for normal naturally aligned packing, not
data backed tighter than that.

> 
> Anyways, Right now, I have no time for something like this but eventually would like
> to try something. But if someone wants to propose something sooner, please :)

*looks doubtful*  Maybe I can be convinced.  We'll see.

Jonathan
> 
> - Nuno Sá 
>