[PATCH v4 0/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts

[PATCH v4 0/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts

[Alexandru Ardelean]

The AD7606C-16 and AD7606C-18 are pretty similar with the AD7606B.
The main difference between AD7606C-16 & AD7606C-18 is the precision in
bits (16 vs 18).
Because of that, some scales need to be defined for the 18-bit variants, as
they need to be computed against 2**18 (vs 2**16 for the 16 bit-variants).

Because the AD7606C-16,18 also supports bipolar & differential channels,
for SW-mode, the default range of 10 V or ±10V should be set at probe.
On reset, the default range (in the registers) is set to value 0x3 which
corresponds to '±10 V single-ended range', regardless of bipolar or
differential configuration.

Aside from the scale/ranges, the AD7606C-16 is similar to the AD7606B.

This changeset, does a bit of rework to the existing ad7606 driver and then
adds support for the AD7606C-16 & AD7606C-18 parts.

Datasheet links:
  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf

Changelog v3 -> v4:
  - v3: https://lore.kernel.org/linux-iio/20240904072718.1143440-1-aardelean@baylibre.com/
  - For patch 'dt-bindings: iio: adc: document diff-channels corner case
    for some ADCs'
    - Added 'Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>'
  - Renamed patch 'dt-bindings: iio: adc: add adi,ad7606c-{16,18} compatible strings'
    to 'dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts'
    - Updated based on notes from Krzysztof Kozlowski (from v3)
      - Dropped ()
      - Re-ordered the patternProperties:oneOf:required specification
      - Unified match-pattern to '^channel@[1-8]$'

Changelog v2 -> v3:
  - v2: https://lore.kernel.org/linux-iio/20240902103638.686039-1-aardelean@baylibre.com/
  - Applied checkpatch.pl changes
  - Managed to setup and run 'make dt_binding_check DT_SCHEMA_FILES=adi,ad7606.yaml'
    - Found the winning combination for this setup
    - David Lechner also helped
  - For patch 'iio: adc: ad7606: rework available attributes for SW channels'
    - Removed an extra space that checkpatch found
  - For patch 'dt-bindings: iio: adc: document diff-channels corner case
    for some ADCs'
    - Removed 'the the' stutter (that I did in writing)
  - For patch 'dt-bindings: iio: adc: add adi,ad7606c-{16,18} compatible strings'
    - Updated binding with some description for 'diff-channels' & 'bipolar'
      properties
    - Channel definitions are counted from 1 to 8 to match datasheet
    - Added more bindings rules for 'diff-channels' & 'bipolar' for AD7606C
      - Adapted some ideas from adi,ad7192.yaml
  - For patch 'iio: adc: ad7606: add support for AD7606C-{16,18} parts'
    - Updated 'diff-channels' property with channel numbers (from 1 to 8)
      handling

Changelog v1 -> v2:
  - v1: https://lore.kernel.org/linux-iio/20240819064721.91494-1-aardelean@baylibre.com/
  - Fixed description in 'iio: adc: ad7606: add 'bits' parameter to channels macros'
  - Added patch 'dt-bindings: iio: adc: document diff-channels corner case
    for some ADCs'
    - diff-channels = <reg reg> can be used to define differential channels
      with dedicated positive + negative pins
  - Re-worked patch 'dt-bindings: iio: adc: add adi,ad7606c-{16,18} compatible strings'
    - Using standard 'diff-channels' & 'bipolar' properties from adc.yaml
  - Re-worked patch 'iio: adc: ad7606: add support for AD7606C-{16,18} parts'
    - Reading 18-bit samples now relies on SPI controllers being able to
      pad 18-bits to 32-bits.
    - Implemented 'diff-channels = <reg reg>' setting
    - Removed some bad/left-over channel configuration code which I forgot
      during development and rebasing.

Alexandru Ardelean (8):
  iio: adc: ad7606: add 'bits' parameter to channels macros
  iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct()
  iio: adc: ad7606: split a 'ad7606_sw_mode_setup()' from probe
  iio: adc: ad7606: wrap channel ranges & scales into struct
  iio: adc: ad7606: rework available attributes for SW channels
  dt-bindings: iio: adc: document diff-channels corner case for some
    ADCs
  dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts
  iio: adc: ad7606: add support for AD7606C-{16,18} parts

 .../devicetree/bindings/iio/adc/adc.yaml      |   4 +
 .../bindings/iio/adc/adi,ad7606.yaml          | 109 +++++
 drivers/iio/adc/ad7606.c                      | 433 +++++++++++++++---
 drivers/iio/adc/ad7606.h                      |  78 +++-
 drivers/iio/adc/ad7606_spi.c                  |  71 ++-
 5 files changed, 599 insertions(+), 96 deletions(-)

-- 
2.46.0

[PATCH v4 1/8] iio: adc: ad7606: add 'bits' parameter to channels macros

[Alexandru Ardelean]

There are some newer additions to the AD7606 family, which support 18 bit
precision.
Up until now, all chips were 16 bit.

This change adds a 'bits' parameter to the AD760X_CHANNEL macro and renames
'ad7606_channels' -> 'ad7606_channels_16bit' for the current devices.

The AD7606_SW_CHANNEL() macro is also introduced, as a short-hand for IIO
channels in SW mode.

Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c     | 58 ++++++++++++++++++------------------
 drivers/iio/adc/ad7606.h     | 18 ++++++-----
 drivers/iio/adc/ad7606_spi.c | 16 +++++-----
 3 files changed, 47 insertions(+), 45 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 539e4a8621fe..dba1f28782e4 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -333,16 +333,16 @@ static const struct iio_chan_spec ad7605_channels[] = {
 	AD7605_CHANNEL(3),
 };
 
-static const struct iio_chan_spec ad7606_channels[] = {
+static const struct iio_chan_spec ad7606_channels_16bit[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(8),
-	AD7606_CHANNEL(0),
-	AD7606_CHANNEL(1),
-	AD7606_CHANNEL(2),
-	AD7606_CHANNEL(3),
-	AD7606_CHANNEL(4),
-	AD7606_CHANNEL(5),
-	AD7606_CHANNEL(6),
-	AD7606_CHANNEL(7),
+	AD7606_CHANNEL(0, 16),
+	AD7606_CHANNEL(1, 16),
+	AD7606_CHANNEL(2, 16),
+	AD7606_CHANNEL(3, 16),
+	AD7606_CHANNEL(4, 16),
+	AD7606_CHANNEL(5, 16),
+	AD7606_CHANNEL(6, 16),
+	AD7606_CHANNEL(7, 16),
 };
 
 /*
@@ -357,22 +357,22 @@ static const struct iio_chan_spec ad7606_channels[] = {
  */
 static const struct iio_chan_spec ad7616_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(16),
-	AD7606_CHANNEL(0),
-	AD7606_CHANNEL(1),
-	AD7606_CHANNEL(2),
-	AD7606_CHANNEL(3),
-	AD7606_CHANNEL(4),
-	AD7606_CHANNEL(5),
-	AD7606_CHANNEL(6),
-	AD7606_CHANNEL(7),
-	AD7606_CHANNEL(8),
-	AD7606_CHANNEL(9),
-	AD7606_CHANNEL(10),
-	AD7606_CHANNEL(11),
-	AD7606_CHANNEL(12),
-	AD7606_CHANNEL(13),
-	AD7606_CHANNEL(14),
-	AD7606_CHANNEL(15),
+	AD7606_CHANNEL(0, 16),
+	AD7606_CHANNEL(1, 16),
+	AD7606_CHANNEL(2, 16),
+	AD7606_CHANNEL(3, 16),
+	AD7606_CHANNEL(4, 16),
+	AD7606_CHANNEL(5, 16),
+	AD7606_CHANNEL(6, 16),
+	AD7606_CHANNEL(7, 16),
+	AD7606_CHANNEL(8, 16),
+	AD7606_CHANNEL(9, 16),
+	AD7606_CHANNEL(10, 16),
+	AD7606_CHANNEL(11, 16),
+	AD7606_CHANNEL(12, 16),
+	AD7606_CHANNEL(13, 16),
+	AD7606_CHANNEL(14, 16),
+	AD7606_CHANNEL(15, 16),
 };
 
 static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
@@ -382,25 +382,25 @@ static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
 		.num_channels = 5,
 	},
 	[ID_AD7606_8] = {
-		.channels = ad7606_channels,
+		.channels = ad7606_channels_16bit,
 		.num_channels = 9,
 		.oversampling_avail = ad7606_oversampling_avail,
 		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
 	},
 	[ID_AD7606_6] = {
-		.channels = ad7606_channels,
+		.channels = ad7606_channels_16bit,
 		.num_channels = 7,
 		.oversampling_avail = ad7606_oversampling_avail,
 		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
 	},
 	[ID_AD7606_4] = {
-		.channels = ad7606_channels,
+		.channels = ad7606_channels_16bit,
 		.num_channels = 5,
 		.oversampling_avail = ad7606_oversampling_avail,
 		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
 	},
 	[ID_AD7606B] = {
-		.channels = ad7606_channels,
+		.channels = ad7606_channels_16bit,
 		.num_channels = 9,
 		.oversampling_avail = ad7606_oversampling_avail,
 		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 0c6a88cc4695..771121350f98 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -8,7 +8,7 @@
 #ifndef IIO_ADC_AD7606_H_
 #define IIO_ADC_AD7606_H_
 
-#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all) {	\
+#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all, bits) {	\
 		.type = IIO_VOLTAGE,				\
 		.indexed = 1,					\
 		.channel = num,					\
@@ -19,24 +19,26 @@
 		.scan_index = num,				\
 		.scan_type = {					\
 			.sign = 's',				\
-			.realbits = 16,				\
-			.storagebits = 16,			\
+			.realbits = (bits),			\
+			.storagebits = (bits),			\
 			.endianness = IIO_CPU,			\
 		},						\
 }
 
 #define AD7605_CHANNEL(num)				\
 	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW),	\
-		BIT(IIO_CHAN_INFO_SCALE), 0)
+		BIT(IIO_CHAN_INFO_SCALE), 0, 16)
 
-#define AD7606_CHANNEL(num)				\
+#define AD7606_CHANNEL(num, bits)			\
 	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW),	\
 		BIT(IIO_CHAN_INFO_SCALE),		\
-		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
 
-#define AD7616_CHANNEL(num)	\
+#define AD7606_SW_CHANNEL(num, bits)	\
 	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
-		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
+		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
+
+#define AD7616_CHANNEL(num)	AD7606_SW_CHANNEL(num, 16)
 
 /**
  * struct ad7606_chip_info - chip specific information
diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
index 62ec12195307..e00f58a6a0e9 100644
--- a/drivers/iio/adc/ad7606_spi.c
+++ b/drivers/iio/adc/ad7606_spi.c
@@ -67,14 +67,14 @@ static const struct iio_chan_spec ad7616_sw_channels[] = {
 
 static const struct iio_chan_spec ad7606b_sw_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(8),
-	AD7616_CHANNEL(0),
-	AD7616_CHANNEL(1),
-	AD7616_CHANNEL(2),
-	AD7616_CHANNEL(3),
-	AD7616_CHANNEL(4),
-	AD7616_CHANNEL(5),
-	AD7616_CHANNEL(6),
-	AD7616_CHANNEL(7),
+	AD7606_SW_CHANNEL(0, 16),
+	AD7606_SW_CHANNEL(1, 16),
+	AD7606_SW_CHANNEL(2, 16),
+	AD7606_SW_CHANNEL(3, 16),
+	AD7606_SW_CHANNEL(4, 16),
+	AD7606_SW_CHANNEL(5, 16),
+	AD7606_SW_CHANNEL(6, 16),
+	AD7606_SW_CHANNEL(7, 16),
 };
 
 static const unsigned int ad7606B_oversampling_avail[9] = {
-- 
2.46.0

[PATCH v4 2/8] iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct()

[Alexandru Ardelean]

The ad7606_scan_direct() function returns 'int', which is fine for 16-bit
samples.
But when going to 18-bit samples, these need to be implemented as 32-bit
(or int) type.

In that case when getting samples (which can be negative), we'd get random
error codes.
So, the easiest thing is to just move the 'val' pointer to
'ad7606_scan_direct()'. This doesn't qualify as a fix, it's just a
preparation for 18-bit ADCs (of the AD7606 family).

Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index dba1f28782e4..5049e37f8393 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -138,7 +138,8 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
-static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
+static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
+			      int *val)
 {
 	struct ad7606_state *st = iio_priv(indio_dev);
 	int ret;
@@ -153,7 +154,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
 
 	ret = ad7606_read_samples(st);
 	if (ret == 0)
-		ret = st->data[ch];
+		*val = sign_extend32(st->data[ch], 15);
 
 error_ret:
 	gpiod_set_value(st->gpio_convst, 0);
@@ -173,10 +174,9 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
 		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
-			ret = ad7606_scan_direct(indio_dev, chan->address);
+			ret = ad7606_scan_direct(indio_dev, chan->address, val);
 			if (ret < 0)
 				return ret;
-			*val = (short) ret;
 			return IIO_VAL_INT;
 		}
 		unreachable();
-- 
2.46.0

[PATCH v4 3/8] iio: adc: ad7606: split a 'ad7606_sw_mode_setup()' from probe

[Alexandru Ardelean]

This change moves the logic for setting up SW mode (during probe) into it's
own function.

With the addition of some newer parts, the SW-mode part can get a little
more complicated.
So it's a bit better to have a separate function for this.

Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 43 ++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 5049e37f8393..b400c9b2519d 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -545,6 +545,29 @@ static const struct iio_trigger_ops ad7606_trigger_ops = {
 	.validate_device = iio_trigger_validate_own_device,
 };
 
+static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
+{
+	struct ad7606_state *st = iio_priv(indio_dev);
+
+	if (!st->bops->sw_mode_config)
+		return 0;
+
+	st->sw_mode_en = device_property_present(st->dev, "adi,sw-mode");
+	if (!st->sw_mode_en)
+		return 0;
+
+	indio_dev->info = &ad7606_info_os_range_and_debug;
+
+	/* Scale of 0.076293 is only available in sw mode */
+	st->scale_avail = ad7616_sw_scale_avail;
+	st->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
+
+	/* After reset, in software mode, ±10 V is set by default */
+	memset32(st->range, 2, ARRAY_SIZE(st->range));
+
+	return st->bops->sw_mode_config(indio_dev);
+}
+
 int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 		 const char *name, unsigned int id,
 		 const struct ad7606_bus_ops *bops)
@@ -617,23 +640,9 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 	st->write_scale = ad7606_write_scale_hw;
 	st->write_os = ad7606_write_os_hw;
 
-	if (st->bops->sw_mode_config)
-		st->sw_mode_en = device_property_present(st->dev,
-							 "adi,sw-mode");
-
-	if (st->sw_mode_en) {
-		/* Scale of 0.076293 is only available in sw mode */
-		st->scale_avail = ad7616_sw_scale_avail;
-		st->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
-
-		/* After reset, in software mode, ±10 V is set by default */
-		memset32(st->range, 2, ARRAY_SIZE(st->range));
-		indio_dev->info = &ad7606_info_os_range_and_debug;
-
-		ret = st->bops->sw_mode_config(indio_dev);
-		if (ret < 0)
-			return ret;
-	}
+	ret = ad7606_sw_mode_setup(indio_dev);
+	if (ret)
+		return ret;
 
 	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
 					  indio_dev->name,
-- 
2.46.0

[PATCH v4 4/8] iio: adc: ad7606: wrap channel ranges & scales into struct

[Alexandru Ardelean]

With the addition of AD7606C-16,18 which have differential & bipolar
channels (and ranges), which can vary from channel to channel, we'll need
to keep more information about each channel range.

To do that, we'll add a 'struct ad7606_chan_scale' type to hold just
configuration for each channel.
This includes the scales per channel (which can be different with
AD7606C-16,18), as well as the range for each channel.
This driver was already keeping the range value for each channel before,
and since this is couple with the scales, it also makes sense to put them
in the same struct.

Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 37 +++++++++++++++++++++++++------------
 drivers/iio/adc/ad7606.h | 22 ++++++++++++++++------
 2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index b400c9b2519d..2554a4a4a9c0 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -170,6 +170,7 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 {
 	int ret, ch = 0;
 	struct ad7606_state *st = iio_priv(indio_dev);
+	struct ad7606_chan_scale *cs;
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
@@ -183,8 +184,9 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SCALE:
 		if (st->sw_mode_en)
 			ch = chan->address;
+		cs = &st->chan_scales[ch];
 		*val = 0;
-		*val2 = st->scale_avail[st->range[ch]];
+		*val2 = cs->scale_avail[cs->range];
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		*val = st->oversampling;
@@ -214,8 +216,9 @@ static ssize_t in_voltage_scale_available_show(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7606_state *st = iio_priv(indio_dev);
+	struct ad7606_chan_scale *cs = &st->chan_scales[0];
 
-	return ad7606_show_avail(buf, st->scale_avail, st->num_scales, true);
+	return ad7606_show_avail(buf, cs->scale_avail, cs->num_scales, true);
 }
 
 static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);
@@ -253,19 +256,21 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 			    long mask)
 {
 	struct ad7606_state *st = iio_priv(indio_dev);
+	struct ad7606_chan_scale *cs;
 	int i, ret, ch = 0;
 
 	guard(mutex)(&st->lock);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
-		i = find_closest(val2, st->scale_avail, st->num_scales);
 		if (st->sw_mode_en)
 			ch = chan->address;
+		cs = &st->chan_scales[ch];
+		i = find_closest(val2, cs->scale_avail, cs->num_scales);
 		ret = st->write_scale(indio_dev, ch, i);
 		if (ret < 0)
 			return ret;
-		st->range[ch] = i;
+		cs->range = i;
 
 		return 0;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
@@ -547,7 +552,9 @@ static const struct iio_trigger_ops ad7606_trigger_ops = {
 
 static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
 {
+	unsigned int num_channels = indio_dev->num_channels - 1;
 	struct ad7606_state *st = iio_priv(indio_dev);
+	int ch;
 
 	if (!st->bops->sw_mode_config)
 		return 0;
@@ -559,11 +566,14 @@ static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
 	indio_dev->info = &ad7606_info_os_range_and_debug;
 
 	/* Scale of 0.076293 is only available in sw mode */
-	st->scale_avail = ad7616_sw_scale_avail;
-	st->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
-
 	/* After reset, in software mode, ±10 V is set by default */
-	memset32(st->range, 2, ARRAY_SIZE(st->range));
+	for (ch = 0; ch < num_channels; ch++) {
+		struct ad7606_chan_scale *cs = &st->chan_scales[ch];
+
+		cs->scale_avail = ad7616_sw_scale_avail;
+		cs->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
+		cs->range = 2;
+	}
 
 	return st->bops->sw_mode_config(indio_dev);
 }
@@ -572,6 +582,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 		 const char *name, unsigned int id,
 		 const struct ad7606_bus_ops *bops)
 {
+	struct ad7606_chan_scale *cs;
 	struct ad7606_state *st;
 	int ret;
 	struct iio_dev *indio_dev;
@@ -588,10 +599,12 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 	st->bops = bops;
 	st->base_address = base_address;
 	/* tied to logic low, analog input range is +/- 5V */
-	st->range[0] = 0;
 	st->oversampling = 1;
-	st->scale_avail = ad7606_scale_avail;
-	st->num_scales = ARRAY_SIZE(ad7606_scale_avail);
+
+	cs = &st->chan_scales[0];
+	cs->range = 0;
+	cs->scale_avail = ad7606_scale_avail;
+	cs->num_scales = ARRAY_SIZE(ad7606_scale_avail);
 
 	ret = devm_regulator_get_enable(dev, "avcc");
 	if (ret)
@@ -698,7 +711,7 @@ static int ad7606_resume(struct device *dev)
 	struct ad7606_state *st = iio_priv(indio_dev);
 
 	if (st->gpio_standby) {
-		gpiod_set_value(st->gpio_range, st->range[0]);
+		gpiod_set_value(st->gpio_range, st->chan_scales[0].range);
 		gpiod_set_value(st->gpio_standby, 1);
 		ad7606_reset(st);
 	}
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 771121350f98..afe6a4030e0e 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -8,6 +8,8 @@
 #ifndef IIO_ADC_AD7606_H_
 #define IIO_ADC_AD7606_H_
 
+#define AD760X_MAX_CHANNELS	16
+
 #define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all, bits) {	\
 		.type = IIO_VOLTAGE,				\
 		.indexed = 1,					\
@@ -60,17 +62,27 @@ struct ad7606_chip_info {
 	unsigned long			init_delay_ms;
 };
 
+/**
+ * struct ad7606_chan_scale - channel scale configuration
+ * @scale_avail		pointer to the array which stores the available scales
+ * @num_scales		number of elements stored in the scale_avail array
+ * @range		voltage range selection, selects which scale to apply
+ */
+struct ad7606_chan_scale {
+	const unsigned int		*scale_avail;
+	unsigned int			num_scales;
+	unsigned int			range;
+};
+
 /**
  * struct ad7606_state - driver instance specific data
  * @dev		pointer to kernel device
  * @chip_info		entry in the table of chips that describes this device
  * @bops		bus operations (SPI or parallel)
- * @range		voltage range selection, selects which scale to apply
+ * @chan_scales		scale configuration for channels
  * @oversampling	oversampling selection
  * @base_address	address from where to read data in parallel operation
  * @sw_mode_en		software mode enabled
- * @scale_avail		pointer to the array which stores the available scales
- * @num_scales		number of elements stored in the scale_avail array
  * @oversampling_avail	pointer to the array which stores the available
  *			oversampling ratios.
  * @num_os_ratios	number of elements stored in oversampling_avail array
@@ -94,12 +106,10 @@ struct ad7606_state {
 	struct device			*dev;
 	const struct ad7606_chip_info	*chip_info;
 	const struct ad7606_bus_ops	*bops;
-	unsigned int			range[16];
+	struct ad7606_chan_scale	chan_scales[AD760X_MAX_CHANNELS];
 	unsigned int			oversampling;
 	void __iomem			*base_address;
 	bool				sw_mode_en;
-	const unsigned int		*scale_avail;
-	unsigned int			num_scales;
 	const unsigned int		*oversampling_avail;
 	unsigned int			num_os_ratios;
 	int (*write_scale)(struct iio_dev *indio_dev, int ch, int val);
-- 
2.46.0

[PATCH v4 5/8] iio: adc: ad7606: rework available attributes for SW channels

[Alexandru Ardelean]

For SW mode, the oversampling and scales attributes are always present.
So, they can be implemented via a 'read_avail' hook in iio_info.

For HW mode, it's a bit tricky, as these attributes get assigned based on
GPIO definitions.

So, for SW mode, we define a separate AD7606_SW_CHANNEL() macro, and use
that for the SW channels.
And 'ad7606_info_os_range_and_debug' can be renamed to
'ad7606_info_sw_mode' as it is only used for SW mode.

For the 'read_avail' hook, we'll need to allocate the SW scales, so that
they are just returned userspace without any extra processing.
The allocation will happen when then ad7606_state struct is allocated.
The oversampling available parameters don't need any extra processing; they
can just be passed back to userspace (as they are).

Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 63 ++++++++++++++++++++++++++++++++++++----
 drivers/iio/adc/ad7606.h | 31 +++++++++++++++++---
 2 files changed, 85 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 2554a4a4a9c0..4c3fbb28f790 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -507,6 +507,37 @@ static int ad7606_buffer_predisable(struct iio_dev *indio_dev)
 	return 0;
 }
 
+static int ad7606_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long info)
+{
+	struct ad7606_state *st = iio_priv(indio_dev);
+	struct ad7606_chan_scale *cs;
+	unsigned int ch = 0;
+
+	switch (info) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = st->oversampling_avail;
+		*length = st->num_os_ratios;
+		*type = IIO_VAL_INT;
+
+		return IIO_AVAIL_LIST;
+
+	case IIO_CHAN_INFO_SCALE:
+		if (st->sw_mode_en)
+			ch = chan->address;
+
+		cs = &st->chan_scales[ch];
+		*vals = cs->scale_avail_show;
+		*length = cs->num_scales * 2;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+
+		return IIO_AVAIL_LIST;
+	}
+	return -EINVAL;
+}
+
 static const struct iio_buffer_setup_ops ad7606_buffer_ops = {
 	.postenable = &ad7606_buffer_postenable,
 	.predisable = &ad7606_buffer_predisable,
@@ -524,11 +555,11 @@ static const struct iio_info ad7606_info_os_and_range = {
 	.validate_trigger = &ad7606_validate_trigger,
 };
 
-static const struct iio_info ad7606_info_os_range_and_debug = {
+static const struct iio_info ad7606_info_sw_mode = {
 	.read_raw = &ad7606_read_raw,
 	.write_raw = &ad7606_write_raw,
+	.read_avail = &ad7606_read_avail,
 	.debugfs_reg_access = &ad7606_reg_access,
-	.attrs = &ad7606_attribute_group_os_and_range,
 	.validate_trigger = &ad7606_validate_trigger,
 };
 
@@ -554,7 +585,7 @@ static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
 {
 	unsigned int num_channels = indio_dev->num_channels - 1;
 	struct ad7606_state *st = iio_priv(indio_dev);
-	int ch;
+	int ret, ch;
 
 	if (!st->bops->sw_mode_config)
 		return 0;
@@ -563,7 +594,7 @@ static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
 	if (!st->sw_mode_en)
 		return 0;
 
-	indio_dev->info = &ad7606_info_os_range_and_debug;
+	indio_dev->info = &ad7606_info_sw_mode;
 
 	/* Scale of 0.076293 is only available in sw mode */
 	/* After reset, in software mode, ±10 V is set by default */
@@ -575,7 +606,29 @@ static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
 		cs->range = 2;
 	}
 
-	return st->bops->sw_mode_config(indio_dev);
+	ret = st->bops->sw_mode_config(indio_dev);
+	if (ret)
+		return ret;
+
+	for (ch = 0; ch < num_channels; ch++) {
+		struct ad7606_chan_scale *cs = &st->chan_scales[ch];
+		int i;
+
+		cs = &st->chan_scales[ch];
+
+		if (cs->num_scales * 2 > AD760X_MAX_SCALE_SHOW) {
+			dev_err(st->dev, "Driver error: scale range too big");
+			return -ERANGE;
+		}
+
+		/* Generate a scale_avail list for showing to userspace */
+		for (i = 0; i < cs->num_scales; i++) {
+			cs->scale_avail_show[i * 2] = 0;
+			cs->scale_avail_show[i * 2 + 1] = cs->scale_avail[i];
+		}
+	}
+
+	return 0;
 }
 
 int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index afe6a4030e0e..2113ad460c0f 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -27,6 +27,29 @@
 		},						\
 }
 
+#define AD7606_SW_CHANNEL(num, bits) {				\
+		.type = IIO_VOLTAGE,				\
+		.indexed = 1,					\
+		.channel = num,					\
+		.address = num,					\
+		.info_mask_separate =				\
+			BIT(IIO_CHAN_INFO_RAW) |		\
+			BIT(IIO_CHAN_INFO_SCALE),		\
+		.info_mask_separate_available =			\
+			BIT(IIO_CHAN_INFO_SCALE),		\
+		.info_mask_shared_by_all =			\
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
+		.info_mask_shared_by_all_available =		\
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
+		.scan_index = num,				\
+		.scan_type = {					\
+			.sign = 's',				\
+			.realbits = (bits),			\
+			.storagebits = (bits),			\
+			.endianness = IIO_CPU,			\
+		},						\
+}
+
 #define AD7605_CHANNEL(num)				\
 	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW),	\
 		BIT(IIO_CHAN_INFO_SCALE), 0, 16)
@@ -36,10 +59,6 @@
 		BIT(IIO_CHAN_INFO_SCALE),		\
 		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
 
-#define AD7606_SW_CHANNEL(num, bits)	\
-	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
-		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
-
 #define AD7616_CHANNEL(num)	AD7606_SW_CHANNEL(num, 16)
 
 /**
@@ -65,11 +84,15 @@ struct ad7606_chip_info {
 /**
  * struct ad7606_chan_scale - channel scale configuration
  * @scale_avail		pointer to the array which stores the available scales
+ * @scale_avail_show	a duplicate of 'scale_avail' which is readily formatted
+ *			such that it can be read via the 'read_avail' hook
  * @num_scales		number of elements stored in the scale_avail array
  * @range		voltage range selection, selects which scale to apply
  */
 struct ad7606_chan_scale {
+#define AD760X_MAX_SCALE_SHOW		(AD760X_MAX_CHANNELS * 2)
 	const unsigned int		*scale_avail;
+	int				scale_avail_show[AD760X_MAX_SCALE_SHOW];
 	unsigned int			num_scales;
 	unsigned int			range;
 };
-- 
2.46.0

[PATCH v4 6/8] dt-bindings: iio: adc: document diff-channels corner case for some ADCs

[Alexandru Ardelean]

Some ADCs have channels with negative and positive inputs, which can be
used to measure differential voltage levels. These inputs/pins are
dedicated (to the given channel) and cannot be muxed as with other ADCs.

For those types of setups, the 'diff-channels' property can be specified to
be used with the channel number (or reg property) for both negative and
positive inputs/pins.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 Documentation/devicetree/bindings/iio/adc/adc.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adc.yaml b/Documentation/devicetree/bindings/iio/adc/adc.yaml
index 8e7835cf36fd..b9bc02b5b07a 100644
--- a/Documentation/devicetree/bindings/iio/adc/adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adc.yaml
@@ -37,6 +37,10 @@ properties:
       to both the positive and negative inputs of a differential ADC.
       The first value specifies the positive input pin, the second
       specifies the negative input pin.
+      There are also some ADCs, where the differential channel has dedicated
+      positive and negative inputs which can be used to measure differential
+      voltage levels. For those setups, this property can be configured with
+      the 'reg' property for both inputs (i.e. diff-channels = <reg reg>).
 
   single-channel:
     $ref: /schemas/types.yaml#/definitions/uint32
-- 
2.46.0

[PATCH v4 7/8] dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts

[Alexandru Ardelean]

The driver will support the AD7606C-16 and AD7606C-18.
This change adds the compatible strings for these devices.

The AD7606C-16,18 channels also support these (individually configurable)
types of channels:
 - bipolar single-ended
 - unipolar single-ended
 - bipolar differential

Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 .../bindings/iio/adc/adi,ad7606.yaml          | 109 ++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
index 69408cae3db9..57537ab0ec82 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
@@ -14,6 +14,8 @@ description: |
   https://www.analog.com/media/en/technical-documentation/data-sheets/AD7605-4.pdf
   https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606_7606-6_7606-4.pdf
   https://www.analog.com/media/en/technical-documentation/data-sheets/AD7606B.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
   https://www.analog.com/media/en/technical-documentation/data-sheets/AD7616.pdf
 
 properties:
@@ -24,11 +26,19 @@ properties:
       - adi,ad7606-6
       - adi,ad7606-8  # Referred to as AD7606 (without -8) in the datasheet
       - adi,ad7606b
+      - adi,ad7606c-16
+      - adi,ad7606c-18
       - adi,ad7616
 
   reg:
     maxItems: 1
 
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
   spi-cpha: true
 
   spi-cpol: true
@@ -114,6 +124,46 @@ properties:
       assumed that the pins are hardwired to VDD.
     type: boolean
 
+patternProperties:
+  "^channel@[1-8]$":
+    type: object
+    $ref: adc.yaml
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        description:
+          The channel number, as specified in the datasheet (from 1 to 8).
+        minimum: 1
+        maximum: 8
+
+      diff-channels:
+        description:
+          Each channel can be configured as a differential bipolar channel.
+          The ADC uses the same positive and negative inputs for this.
+          This property must be specified as 'reg' (or the channel number) for
+          both positive and negative inputs (i.e. diff-channels = <reg reg>).
+        items:
+          minimum: 1
+          maximum: 8
+
+      bipolar:
+        description:
+          Each channel can be configured as a unipolar or bipolar single-ended.
+          When this property is not specified, it's unipolar, so the ADC will
+          have only the positive input wired.
+          For this ADC the 'diff-channels' & 'bipolar' properties are mutually
+          exclusive.
+
+    required:
+      - reg
+
+    oneOf:
+      - required:
+          - diff-channels
+      - required:
+          - bipolar
+
 required:
   - compatible
   - reg
@@ -170,6 +220,17 @@ allOf:
         adi,conversion-start-gpios:
           maxItems: 1
 
+  - if:
+      not:
+        properties:
+          compatible:
+            enum:
+              - adi,ad7606c-16
+              - adi,ad7606c-18
+    then:
+      patternProperties:
+        "^channel@[1-8]$": false
+
 unevaluatedProperties: false
 
 examples:
@@ -202,4 +263,52 @@ examples:
             standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>;
         };
     };
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "adi,ad7606c-18";
+            reg = <0>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            spi-max-frequency = <1000000>;
+            spi-cpol;
+            spi-cpha;
+
+            avcc-supply = <&adc_vref>;
+            vdrive-supply = <&vdd_supply>;
+
+            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+            interrupt-parent = <&gpio>;
+
+            adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>;
+            reset-gpios = <&gpio 27 GPIO_ACTIVE_HIGH>;
+            adi,first-data-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>;
+            standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>;
+
+            adi,sw-mode;
+
+            channel@1 {
+                reg = <1>;
+                diff-channels = <1 1>;
+            };
+
+            channel@3 {
+                reg = <3>;
+                bipolar;
+            };
+
+            channel@8 {
+                reg = <8>;
+                diff-channels = <8 8>;
+            };
+
+        };
+    };
 ...
-- 
2.46.0

[PATCH v4 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts

[Alexandru Ardelean]

The AD7606C-16 and AD7606C-18 are pretty similar with the AD7606B.
The main difference between AD7606C-16 & AD7606C-18 is the precision in
bits (16 vs 18).
Because of that, some scales need to be defined for the 18-bit variants, as
they need to be computed against 2**18 (vs 2**16 for the 16 bit-variants).

Because the AD7606C-16,18 also supports bipolar & differential channels,
for SW-mode, the default range of 10 V or ±10V should be set at probe.
On reset, the default range (in the registers) is set to value 0x3 which
corresponds to '±10 V single-ended range', regardless of bipolar or
differential configuration.

Aside from the scale/ranges, the AD7606C-16 is similar to the AD7606B.

The AD7606C-18 variant offers 18-bit precision. Because of this, the
requirement to use this chip is that the SPI controller supports padding
of 18-bit sequences to 32-bit arrays.

Datasheet links:
  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf

Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c     | 266 +++++++++++++++++++++++++++++++----
 drivers/iio/adc/ad7606.h     |  17 ++-
 drivers/iio/adc/ad7606_spi.c |  55 ++++++++
 3 files changed, 309 insertions(+), 29 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 4c3fbb28f790..999c4411859e 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -28,14 +28,44 @@
 
 #include "ad7606.h"
 
+typedef void (*ad7606c_chan_setup_cb_t)(struct ad7606_state *st, int ch,
+					bool bipolar, bool differential);
+
 /*
  * Scales are computed as 5000/32768 and 10000/32768 respectively,
  * so that when applied to the raw values they provide mV values
  */
-static const unsigned int ad7606_scale_avail[2] = {
+static const unsigned int ad7606_16bit_hw_scale_avail[2] = {
 	152588, 305176
 };
 
+static const unsigned int ad7606_18bit_hw_scale_avail[2] = {
+	38147, 76294
+};
+
+static const unsigned int ad7606c_16_scale_single_ended_unipolar_avail[3] = {
+	76294, 152588, 190735,
+};
+
+static const unsigned int ad7606c_16_scale_single_ended_bipolar_avail[5] = {
+	76294, 152588, 190735, 305176, 381470
+};
+
+static const unsigned int ad7606c_16_scale_differential_bipolar_avail[4] = {
+	152588, 305176, 381470, 610352
+};
+
+static const unsigned int ad7606c_18_scale_single_ended_unipolar_avail[3] = {
+	19073, 38147, 47684
+};
+
+static const unsigned int ad7606c_18_scale_single_ended_bipolar_avail[5] = {
+	19073, 38147, 47684, 76294, 95367
+};
+
+static const unsigned int ad7606c_18_scale_differential_bipolar_avail[4] = {
+	38147, 76294, 95367, 152588
+};
 
 static const unsigned int ad7616_sw_scale_avail[3] = {
 	76293, 152588, 305176
@@ -82,11 +112,19 @@ static int ad7606_reg_access(struct iio_dev *indio_dev,
 	}
 }
 
-static int ad7606_read_samples(struct ad7606_state *st)
+static int ad7606_read_samples(struct ad7606_state *st, bool sign_extend_samples)
 {
+	unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
 	unsigned int num = st->chip_info->num_channels - 1;
-	u16 *data = st->data;
-	int ret;
+	u32 *data32 = st->data.d32;
+	u16 *data16 = st->data.d16;
+	void *data;
+	int i, ret;
+
+	if (storagebits > 16)
+		data = data32;
+	else
+		data = data16;
 
 	/*
 	 * The frstdata signal is set to high while and after reading the sample
@@ -108,11 +146,25 @@ static int ad7606_read_samples(struct ad7606_state *st)
 			return -EIO;
 		}
 
-		data++;
+		if (storagebits > 16)
+			data32++;
+		else
+			data16++;
 		num--;
 	}
 
-	return st->bops->read_block(st->dev, num, data);
+	ret = st->bops->read_block(st->dev, num, data);
+	if (ret)
+		return ret;
+
+	if (storagebits == 16 || !sign_extend_samples)
+		return 0;
+
+	/* For 18 bit samples, we need to sign-extend samples to 32 bits */
+	for (i = 0; i < num; i++)
+		data32[i] = sign_extend32(data32[i], 17);
+
+	return 0;
 }
 
 static irqreturn_t ad7606_trigger_handler(int irq, void *p)
@@ -124,11 +176,11 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
 
 	guard(mutex)(&st->lock);
 
-	ret = ad7606_read_samples(st);
+	ret = ad7606_read_samples(st, true);
 	if (ret)
 		goto error_ret;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, st->data,
+	iio_push_to_buffers_with_timestamp(indio_dev, st->data.d16,
 					   iio_get_time_ns(indio_dev));
 error_ret:
 	iio_trigger_notify_done(indio_dev->trig);
@@ -142,6 +194,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
 			      int *val)
 {
 	struct ad7606_state *st = iio_priv(indio_dev);
+	unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
 	int ret;
 
 	gpiod_set_value(st->gpio_convst, 1);
@@ -152,9 +205,13 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
 		goto error_ret;
 	}
 
-	ret = ad7606_read_samples(st);
-	if (ret == 0)
-		*val = sign_extend32(st->data[ch], 15);
+	ret = ad7606_read_samples(st, false);
+	if (ret == 0) {
+		if (storagebits > 16)
+			*val = sign_extend32(st->data.d32[ch], 17);
+		else
+			*val = sign_extend32(st->data.d16[ch], 15);
+	}
 
 error_ret:
 	gpiod_set_value(st->gpio_convst, 0);
@@ -267,7 +324,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 			ch = chan->address;
 		cs = &st->chan_scales[ch];
 		i = find_closest(val2, cs->scale_avail, cs->num_scales);
-		ret = st->write_scale(indio_dev, ch, i);
+		ret = st->write_scale(indio_dev, ch, i + cs->reg_offset);
 		if (ret < 0)
 			return ret;
 		cs->range = i;
@@ -350,6 +407,18 @@ static const struct iio_chan_spec ad7606_channels_16bit[] = {
 	AD7606_CHANNEL(7, 16),
 };
 
+static const struct iio_chan_spec ad7606_channels_18bit[] = {
+	IIO_CHAN_SOFT_TIMESTAMP(8),
+	AD7606_CHANNEL(0, 18),
+	AD7606_CHANNEL(1, 18),
+	AD7606_CHANNEL(2, 18),
+	AD7606_CHANNEL(3, 18),
+	AD7606_CHANNEL(4, 18),
+	AD7606_CHANNEL(5, 18),
+	AD7606_CHANNEL(6, 18),
+	AD7606_CHANNEL(7, 18),
+};
+
 /*
  * The current assumption that this driver makes for AD7616, is that it's
  * working in Hardware Mode with Serial, Burst and Sequencer modes activated.
@@ -410,6 +479,18 @@ static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
 		.oversampling_avail = ad7606_oversampling_avail,
 		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
 	},
+	[ID_AD7606C_16] = {
+		.channels = ad7606_channels_16bit,
+		.num_channels = 9,
+		.oversampling_avail = ad7606_oversampling_avail,
+		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	},
+	[ID_AD7606C_18] = {
+		.channels = ad7606_channels_18bit,
+		.num_channels = 9,
+		.oversampling_avail = ad7606_oversampling_avail,
+		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	},
 	[ID_AD7616] = {
 		.channels = ad7616_channels,
 		.num_channels = 17,
@@ -581,7 +662,122 @@ static const struct iio_trigger_ops ad7606_trigger_ops = {
 	.validate_device = iio_trigger_validate_own_device,
 };
 
-static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
+static void ad7606c_18_chan_setup(struct ad7606_state *st, int ch,
+				  bool bipolar, bool differential)
+{
+	struct ad7606_chan_scale *cs = &st->chan_scales[ch];
+
+	if (differential) {
+		cs->scale_avail =
+			ad7606c_18_scale_differential_bipolar_avail;
+		cs->num_scales =
+			ARRAY_SIZE(ad7606c_18_scale_differential_bipolar_avail);
+		/* Bipolar differential ranges start at 8 (b1000) */
+		cs->reg_offset = 8;
+		cs->range = 1;
+	} else if (bipolar) {
+		cs->scale_avail =
+			ad7606c_18_scale_single_ended_bipolar_avail;
+		cs->num_scales =
+			ARRAY_SIZE(ad7606c_18_scale_single_ended_bipolar_avail);
+		cs->range = 3;
+	} else {
+		cs->scale_avail =
+			ad7606c_18_scale_single_ended_unipolar_avail;
+		cs->num_scales =
+			ARRAY_SIZE(ad7606c_18_scale_single_ended_unipolar_avail);
+		/* Unipolar single-ended ranges start at 5 (b0101) */
+		cs->reg_offset = 5;
+		cs->range = 1;
+	}
+}
+
+static void ad7606c_16_chan_setup(struct ad7606_state *st, int ch,
+				  bool bipolar, bool differential)
+{
+	struct ad7606_chan_scale *cs = &st->chan_scales[ch];
+
+	if (differential) {
+		cs->scale_avail =
+			ad7606c_16_scale_differential_bipolar_avail;
+		cs->num_scales =
+			ARRAY_SIZE(ad7606c_16_scale_differential_bipolar_avail);
+		/* Bipolar differential ranges start at 8 (b1000) */
+		cs->reg_offset = 8;
+		cs->range = 1;
+	} else if (bipolar) {
+		cs->scale_avail =
+			ad7606c_16_scale_single_ended_bipolar_avail;
+		cs->num_scales =
+			ARRAY_SIZE(ad7606c_16_scale_single_ended_bipolar_avail);
+		cs->range = 3;
+	} else {
+		cs->scale_avail =
+			ad7606c_16_scale_single_ended_unipolar_avail;
+		cs->num_scales =
+			ARRAY_SIZE(ad7606c_16_scale_single_ended_unipolar_avail);
+		/* Unipolar single-ended ranges start at 5 (b0101) */
+		cs->reg_offset = 5;
+		cs->range = 1;
+	}
+}
+
+static int ad7606c_sw_mode_setup_channels(struct iio_dev *indio_dev,
+					  ad7606c_chan_setup_cb_t chan_setup_cb)
+{
+	unsigned int num_channels = indio_dev->num_channels - 1;
+	struct ad7606_state *st = iio_priv(indio_dev);
+	bool chan_configured[AD760X_MAX_CHANNELS] = {};
+	struct device *dev = st->dev;
+	int ret;
+	u32 ch;
+
+	/* We need to hook this first */
+	ret = st->bops->sw_mode_config(indio_dev);
+	if (ret)
+		return ret;
+
+	device_for_each_child_node_scoped(dev, child) {
+		bool bipolar, differential;
+		u32 pins[2];
+
+		ret = fwnode_property_read_u32(child, "reg", &ch);
+		if (ret)
+			continue;
+
+		/* channel number (here) is from 1 to num_channels */
+		if (ch == 0 || ch > num_channels) {
+			dev_warn(st->dev,
+				 "Invalid channel number (ignoring): %d\n", ch);
+			continue;
+		}
+
+		bipolar = fwnode_property_present(child, "bipolar");
+
+		ret = fwnode_property_read_u32_array(child, "diff-channels",
+						     pins, ARRAY_SIZE(pins));
+		/* Channel is differential, if pins are the same as 'reg' */
+		if (ret == 0 && pins[0] == ch && pins[1] == ch)
+			differential = true;
+		else
+			differential = false;
+
+		ch--;
+
+		chan_setup_cb(st, ch, bipolar, differential);
+		chan_configured[ch] = true;
+	}
+
+	/* Apply default configuration to unconfigured (via DT) channels */
+	for (ch = 0; ch < num_channels; ch++) {
+		if (!chan_configured[ch])
+			chan_setup_cb(st, ch, false, false);
+	}
+
+	return 0;
+}
+
+static int ad7606_sw_mode_setup(struct iio_dev *indio_dev, unsigned int id)
 {
 	unsigned int num_channels = indio_dev->num_channels - 1;
 	struct ad7606_state *st = iio_priv(indio_dev);
@@ -596,17 +792,30 @@ static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
 
 	indio_dev->info = &ad7606_info_sw_mode;
 
-	/* Scale of 0.076293 is only available in sw mode */
-	/* After reset, in software mode, ±10 V is set by default */
-	for (ch = 0; ch < num_channels; ch++) {
-		struct ad7606_chan_scale *cs = &st->chan_scales[ch];
+	switch (id) {
+	case ID_AD7606C_18:
+		ret = ad7606c_sw_mode_setup_channels(indio_dev,
+						     ad7606c_18_chan_setup);
+		break;
+	case ID_AD7606C_16:
+		ret = ad7606c_sw_mode_setup_channels(indio_dev,
+						     ad7606c_16_chan_setup);
+		break;
+	default:
+		/* Scale of 0.076293 is only available in sw mode */
+		/* After reset, in software mode, ±10 V is set by default */
+		for (ch = 0; ch < num_channels; ch++) {
+			struct ad7606_chan_scale *cs = &st->chan_scales[ch];
+
+			cs->scale_avail = ad7616_sw_scale_avail;
+			cs->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
+			cs->range = 2;
+		}
 
-		cs->scale_avail = ad7616_sw_scale_avail;
-		cs->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
-		cs->range = 2;
+		ret = st->bops->sw_mode_config(indio_dev);
+		break;
 	}
 
-	ret = st->bops->sw_mode_config(indio_dev);
 	if (ret)
 		return ret;
 
@@ -655,9 +864,16 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 	st->oversampling = 1;
 
 	cs = &st->chan_scales[0];
-	cs->range = 0;
-	cs->scale_avail = ad7606_scale_avail;
-	cs->num_scales = ARRAY_SIZE(ad7606_scale_avail);
+	switch (id) {
+	case ID_AD7606C_18:
+		cs->scale_avail = ad7606_18bit_hw_scale_avail;
+		cs->num_scales = ARRAY_SIZE(ad7606_18bit_hw_scale_avail);
+		break;
+	default:
+		cs->scale_avail = ad7606_16bit_hw_scale_avail;
+		cs->num_scales = ARRAY_SIZE(ad7606_16bit_hw_scale_avail);
+		break;
+	}
 
 	ret = devm_regulator_get_enable(dev, "avcc");
 	if (ret)
@@ -706,7 +922,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 	st->write_scale = ad7606_write_scale_hw;
 	st->write_os = ad7606_write_os_hw;
 
-	ret = ad7606_sw_mode_setup(indio_dev);
+	ret = ad7606_sw_mode_setup(indio_dev, id);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 2113ad460c0f..6b0897aa2dc7 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -22,7 +22,7 @@
 		.scan_type = {					\
 			.sign = 's',				\
 			.realbits = (bits),			\
-			.storagebits = (bits),			\
+			.storagebits = (bits) > 16 ? 32 : 16,	\
 			.endianness = IIO_CPU,			\
 		},						\
 }
@@ -45,7 +45,7 @@
 		.scan_type = {					\
 			.sign = 's',				\
 			.realbits = (bits),			\
-			.storagebits = (bits),			\
+			.storagebits = (bits) > 16 ? 32 : 16,	\
 			.endianness = IIO_CPU,			\
 		},						\
 }
@@ -88,6 +88,8 @@ struct ad7606_chip_info {
  *			such that it can be read via the 'read_avail' hook
  * @num_scales		number of elements stored in the scale_avail array
  * @range		voltage range selection, selects which scale to apply
+ * @reg_offset		offset for the register value, to be applied when
+ *			writing the value of 'range' to the register value
  */
 struct ad7606_chan_scale {
 #define AD760X_MAX_SCALE_SHOW		(AD760X_MAX_CHANNELS * 2)
@@ -95,6 +97,7 @@ struct ad7606_chan_scale {
 	int				scale_avail_show[AD760X_MAX_SCALE_SHOW];
 	unsigned int			num_scales;
 	unsigned int			range;
+	unsigned int			reg_offset;
 };
 
 /**
@@ -151,9 +154,13 @@ struct ad7606_state {
 	/*
 	 * DMA (thus cache coherency maintenance) may require the
 	 * transfer buffers to live in their own cache lines.
-	 * 16 * 16-bit samples + 64-bit timestamp
+	 * 16 * 16-bit samples + 64-bit timestamp - for AD7616
+	 * 8 * 32-bit samples + 64-bit timestamp - for AD7616C-18 (and similar)
 	 */
-	unsigned short			data[20] __aligned(IIO_DMA_MINALIGN);
+	union {
+		unsigned short d16[20];
+		unsigned int d32[10];
+	} data __aligned(IIO_DMA_MINALIGN);
 	__be16				d16[2];
 };
 
@@ -192,6 +199,8 @@ enum ad7606_supported_device_ids {
 	ID_AD7606_6,
 	ID_AD7606_4,
 	ID_AD7606B,
+	ID_AD7606C_16,
+	ID_AD7606C_18,
 	ID_AD7616,
 };
 
diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
index e00f58a6a0e9..b8d630ad156d 100644
--- a/drivers/iio/adc/ad7606_spi.c
+++ b/drivers/iio/adc/ad7606_spi.c
@@ -77,6 +77,18 @@ static const struct iio_chan_spec ad7606b_sw_channels[] = {
 	AD7606_SW_CHANNEL(7, 16),
 };
 
+static const struct iio_chan_spec ad7606c_18_sw_channels[] = {
+	IIO_CHAN_SOFT_TIMESTAMP(8),
+	AD7606_SW_CHANNEL(0, 18),
+	AD7606_SW_CHANNEL(1, 18),
+	AD7606_SW_CHANNEL(2, 18),
+	AD7606_SW_CHANNEL(3, 18),
+	AD7606_SW_CHANNEL(4, 18),
+	AD7606_SW_CHANNEL(5, 18),
+	AD7606_SW_CHANNEL(6, 18),
+	AD7606_SW_CHANNEL(7, 18),
+};
+
 static const unsigned int ad7606B_oversampling_avail[9] = {
 	1, 2, 4, 8, 16, 32, 64, 128, 256
 };
@@ -120,6 +132,19 @@ static int ad7606_spi_read_block(struct device *dev,
 	return 0;
 }
 
+static int ad7606_spi_read_block18to32(struct device *dev,
+				       int count, void *buf)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct spi_transfer xfer = {
+		.bits_per_word = 18,
+		.len = count,
+		.rx_buf = buf,
+	};
+
+	return spi_sync_transfer(spi, &xfer, 1);
+}
+
 static int ad7606_spi_reg_read(struct ad7606_state *st, unsigned int addr)
 {
 	struct spi_device *spi = to_spi_device(st->dev);
@@ -283,6 +308,19 @@ static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
 	return 0;
 }
 
+static int ad7606c_18_sw_mode_config(struct iio_dev *indio_dev)
+{
+	int ret;
+
+	ret = ad7606B_sw_mode_config(indio_dev);
+	if (ret)
+		return ret;
+
+	indio_dev->channels = ad7606c_18_sw_channels;
+
+	return 0;
+}
+
 static const struct ad7606_bus_ops ad7606_spi_bops = {
 	.read_block = ad7606_spi_read_block,
 };
@@ -305,6 +343,15 @@ static const struct ad7606_bus_ops ad7606B_spi_bops = {
 	.sw_mode_config = ad7606B_sw_mode_config,
 };
 
+static const struct ad7606_bus_ops ad7606c_18_spi_bops = {
+	.read_block = ad7606_spi_read_block18to32,
+	.reg_read = ad7606_spi_reg_read,
+	.reg_write = ad7606_spi_reg_write,
+	.write_mask = ad7606_spi_write_mask,
+	.rd_wr_cmd = ad7606B_spi_rd_wr_cmd,
+	.sw_mode_config = ad7606c_18_sw_mode_config,
+};
+
 static int ad7606_spi_probe(struct spi_device *spi)
 {
 	const struct spi_device_id *id = spi_get_device_id(spi);
@@ -315,8 +362,12 @@ static int ad7606_spi_probe(struct spi_device *spi)
 		bops = &ad7616_spi_bops;
 		break;
 	case ID_AD7606B:
+	case ID_AD7606C_16:
 		bops = &ad7606B_spi_bops;
 		break;
+	case ID_AD7606C_18:
+		bops = &ad7606c_18_spi_bops;
+		break;
 	default:
 		bops = &ad7606_spi_bops;
 		break;
@@ -333,6 +384,8 @@ static const struct spi_device_id ad7606_id_table[] = {
 	{ "ad7606-6", ID_AD7606_6 },
 	{ "ad7606-8", ID_AD7606_8 },
 	{ "ad7606b",  ID_AD7606B },
+	{ "ad7606c-16",  ID_AD7606C_16 },
+	{ "ad7606c-18",  ID_AD7606C_18 },
 	{ "ad7616",   ID_AD7616 },
 	{ }
 };
@@ -344,6 +397,8 @@ static const struct of_device_id ad7606_of_match[] = {
 	{ .compatible = "adi,ad7606-6" },
 	{ .compatible = "adi,ad7606-8" },
 	{ .compatible = "adi,ad7606b" },
+	{ .compatible = "adi,ad7606c-16" },
+	{ .compatible = "adi,ad7606c-18" },
 	{ .compatible = "adi,ad7616" },
 	{ }
 };
-- 
2.46.0

Re: [PATCH v4 7/8] dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts

[Krzysztof Kozlowski]

On 05/09/2024 10:24, Alexandru Ardelean wrote:
> The driver will support the AD7606C-16 and AD7606C-18.
> This change adds the compatible strings for these devices.
> 
> The AD7606C-16,18 channels also support these (individually configurable)
> types of channels:
>  - bipolar single-ended
>  - unipolar single-ended
>  - bipolar differential
> 
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v4 1/8] iio: adc: ad7606: add 'bits' parameter to channels macros

[David Lechner]

On 9/5/24 3:23 AM, Alexandru Ardelean wrote:
> There are some newer additions to the AD7606 family, which support 18 bit
> precision.
> Up until now, all chips were 16 bit.
> 
> This change adds a 'bits' parameter to the AD760X_CHANNEL macro and renames
> 'ad7606_channels' -> 'ad7606_channels_16bit' for the current devices.
> 
> The AD7606_SW_CHANNEL() macro is also introduced, as a short-hand for IIO
> channels in SW mode.
> 
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---

...

> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index 0c6a88cc4695..771121350f98 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> @@ -8,7 +8,7 @@
>  #ifndef IIO_ADC_AD7606_H_
>  #define IIO_ADC_AD7606_H_
>  
> -#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all) {	\
> +#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all, bits) {	\
>  		.type = IIO_VOLTAGE,				\
>  		.indexed = 1,					\
>  		.channel = num,					\
> @@ -19,24 +19,26 @@
>  		.scan_index = num,				\
>  		.scan_type = {					\
>  			.sign = 's',				\
> -			.realbits = 16,				\
> -			.storagebits = 16,			\
> +			.realbits = (bits),			\
> +			.storagebits = (bits),			\

Technically OK in this patch since bits is still always 16 but we
can avoid changing the same line again later to:

	(bits) > 16 ? 32 : 16

if we just do that in this patch.


>  			.endianness = IIO_CPU,			\
>  		},						\
>  }

Re: [PATCH v4 2/8] iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct()

[David Lechner]

On 9/5/24 3:23 AM, Alexandru Ardelean wrote:
> The ad7606_scan_direct() function returns 'int', which is fine for 16-bit
> samples.
> But when going to 18-bit samples, these need to be implemented as 32-bit
> (or int) type.
> 
> In that case when getting samples (which can be negative), we'd get random
> error codes.
> So, the easiest thing is to just move the 'val' pointer to
> 'ad7606_scan_direct()'. This doesn't qualify as a fix, it's just a
> preparation for 18-bit ADCs (of the AD7606 family).
> 
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---

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

Re: [PATCH v4 7/8] dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts

[David Lechner]

On 9/5/24 3:24 AM, Alexandru Ardelean wrote:
> The driver will support the AD7606C-16 and AD7606C-18.
> This change adds the compatible strings for these devices.
> 
> The AD7606C-16,18 channels also support these (individually configurable)
> types of channels:
>  - bipolar single-ended
>  - unipolar single-ended
>  - bipolar differential
> 
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---
>  .../bindings/iio/adc/adi,ad7606.yaml          | 109 ++++++++++++++++++
>  1 file changed, 109 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> index 69408cae3db9..57537ab0ec82 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> @@ -14,6 +14,8 @@ description: |
>    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7605-4.pdf
>    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606_7606-6_7606-4.pdf
>    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7606B.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
>    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7616.pdf
>  
>  properties:
> @@ -24,11 +26,19 @@ properties:
>        - adi,ad7606-6
>        - adi,ad7606-8  # Referred to as AD7606 (without -8) in the datasheet
>        - adi,ad7606b
> +      - adi,ad7606c-16
> +      - adi,ad7606c-18
>        - adi,ad7616
>  
>    reg:
>      maxItems: 1
>  
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
>    spi-cpha: true
>  
>    spi-cpol: true
> @@ -114,6 +124,46 @@ properties:
>        assumed that the pins are hardwired to VDD.
>      type: boolean
>  
> +patternProperties:
> +  "^channel@[1-8]$":
> +    type: object
> +    $ref: adc.yaml
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        description:
> +          The channel number, as specified in the datasheet (from 1 to 8).
> +        minimum: 1
> +        maximum: 8
> +
> +      diff-channels:
> +        description:
> +          Each channel can be configured as a differential bipolar channel.
> +          The ADC uses the same positive and negative inputs for this.
> +          This property must be specified as 'reg' (or the channel number) for
> +          both positive and negative inputs (i.e. diff-channels = <reg reg>).
> +        items:
> +          minimum: 1
> +          maximum: 8
> +
> +      bipolar:
> +        description:
> +          Each channel can be configured as a unipolar or bipolar single-ended.
> +          When this property is not specified, it's unipolar, so the ADC will
> +          have only the positive input wired.
> +          For this ADC the 'diff-channels' & 'bipolar' properties are mutually
> +          exclusive.
> +
> +    required:
> +      - reg
> +
> +    oneOf:
> +      - required:
> +          - diff-channels
> +      - required:
> +          - bipolar

The datasheet (ad7606c-18.pdf) lists the following combinations:

* Bipolar single-ended
* Unipolar single-ended
* Bipolar differential

The logic in the oneOf: doesn't match this.

This I think this would be sufficient:

- if:
    required: [diff-channels]
  then:
    required: [bipolar]

> +
>  required:
>    - compatible
>    - reg
> @@ -170,6 +220,17 @@ allOf:
>          adi,conversion-start-gpios:
>            maxItems: 1
>  
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            enum:
> +              - adi,ad7606c-16
> +              - adi,ad7606c-18
> +    then:
> +      patternProperties:
> +        "^channel@[1-8]$": false
> +
>  unevaluatedProperties: false
>  
>  examples:
> @@ -202,4 +263,52 @@ examples:
>              standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>;
>          };
>      };
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0 {
> +            compatible = "adi,ad7606c-18";
> +            reg = <0>;
> +
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            spi-max-frequency = <1000000>;
> +            spi-cpol;
> +            spi-cpha;
> +
> +            avcc-supply = <&adc_vref>;
> +            vdrive-supply = <&vdd_supply>;
> +
> +            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
> +            interrupt-parent = <&gpio>;
> +
> +            adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>;
> +            reset-gpios = <&gpio 27 GPIO_ACTIVE_HIGH>;
> +            adi,first-data-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>;
> +            standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>;
> +
> +            adi,sw-mode;
> +
> +            channel@1 {
> +                reg = <1>;
> +                diff-channels = <1 1>;

                   bipolar;

> +            };
> +
> +            channel@3 {
> +                reg = <3>;
> +                bipolar;
> +            };
> +
> +            channel@8 {
> +                reg = <8>;
> +                diff-channels = <8 8>;

                   bipolar;

> +            };
> +
> +        };
> +    };
>  ...

Re: [PATCH v4 7/8] dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts

[David Lechner]

On 9/5/24 3:24 AM, Alexandru Ardelean wrote:
> The driver will support the AD7606C-16 and AD7606C-18.
> This change adds the compatible strings for these devices.
> 
> The AD7606C-16,18 channels also support these (individually configurable)
> types of channels:
>  - bipolar single-ended
>  - unipolar single-ended
>  - bipolar differential
> 
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---

...

> @@ -170,6 +220,17 @@ allOf:
>          adi,conversion-start-gpios:
>            maxItems: 1
>  
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            enum:
> +              - adi,ad7606c-16
> +              - adi,ad7606c-18
> +    then:
> +      patternProperties:
> +        "^channel@[1-8]$": false
> +

Technically, we should also have "^channel@[1-8]$": false
if adi,sw-mode is not set (not: required: [adi,sw-mode]).

Re: [PATCH v4 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts

[David Lechner]

On 9/5/24 3:24 AM, Alexandru Ardelean wrote:
> The AD7606C-16 and AD7606C-18 are pretty similar with the AD7606B.
> The main difference between AD7606C-16 & AD7606C-18 is the precision in
> bits (16 vs 18).
> Because of that, some scales need to be defined for the 18-bit variants, as
> they need to be computed against 2**18 (vs 2**16 for the 16 bit-variants).
> 
> Because the AD7606C-16,18 also supports bipolar & differential channels,
> for SW-mode, the default range of 10 V or ±10V should be set at probe.
> On reset, the default range (in the registers) is set to value 0x3 which
> corresponds to '±10 V single-ended range', regardless of bipolar or
> differential configuration.
> 
> Aside from the scale/ranges, the AD7606C-16 is similar to the AD7606B.
> 
> The AD7606C-18 variant offers 18-bit precision. Because of this, the
> requirement to use this chip is that the SPI controller supports padding
> of 18-bit sequences to 32-bit arrays.
> 
> Datasheet links:
>   https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
>   https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
> 
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---
>  drivers/iio/adc/ad7606.c     | 266 +++++++++++++++++++++++++++++++----
>  drivers/iio/adc/ad7606.h     |  17 ++-
>  drivers/iio/adc/ad7606_spi.c |  55 ++++++++
>  3 files changed, 309 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index 4c3fbb28f790..999c4411859e 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -28,14 +28,44 @@
>  
>  #include "ad7606.h"
>  
> +typedef void (*ad7606c_chan_setup_cb_t)(struct ad7606_state *st, int ch,
> +					bool bipolar, bool differential);
> +
>  /*
>   * Scales are computed as 5000/32768 and 10000/32768 respectively,
>   * so that when applied to the raw values they provide mV values
>   */
> -static const unsigned int ad7606_scale_avail[2] = {
> +static const unsigned int ad7606_16bit_hw_scale_avail[2] = {
>  	152588, 305176
>  };
>  
> +static const unsigned int ad7606_18bit_hw_scale_avail[2] = {
> +	38147, 76294
> +};
> +
> +static const unsigned int ad7606c_16_scale_single_ended_unipolar_avail[3] = {
> +	76294, 152588, 190735,
> +};
> +
> +static const unsigned int ad7606c_16_scale_single_ended_bipolar_avail[5] = {
> +	76294, 152588, 190735, 305176, 381470
> +};
> +
> +static const unsigned int ad7606c_16_scale_differential_bipolar_avail[4] = {
> +	152588, 305176, 381470, 610352
> +};
> +
> +static const unsigned int ad7606c_18_scale_single_ended_unipolar_avail[3] = {
> +	19073, 38147, 47684
> +};
> +
> +static const unsigned int ad7606c_18_scale_single_ended_bipolar_avail[5] = {
> +	19073, 38147, 47684, 76294, 95367
> +};
> +
> +static const unsigned int ad7606c_18_scale_differential_bipolar_avail[4] = {
> +	38147, 76294, 95367, 152588
> +};
>  
>  static const unsigned int ad7616_sw_scale_avail[3] = {
>  	76293, 152588, 305176
> @@ -82,11 +112,19 @@ static int ad7606_reg_access(struct iio_dev *indio_dev,
>  	}
>  }
>  
> -static int ad7606_read_samples(struct ad7606_state *st)
> +static int ad7606_read_samples(struct ad7606_state *st, bool sign_extend_samples)
>  {
> +	unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;

Why [1]? Sure, they are all the same, but [0] would seem less arbitrary.

>  	unsigned int num = st->chip_info->num_channels - 1;
> -	u16 *data = st->data;
> -	int ret;
> +	u32 *data32 = st->data.d32;
> +	u16 *data16 = st->data.d16;
> +	void *data;
> +	int i, ret;
> +
> +	if (storagebits > 16)
> +		data = data32;
> +	else
> +		data = data16;
>  
>  	/*
>  	 * The frstdata signal is set to high while and after reading the sample
> @@ -108,11 +146,25 @@ static int ad7606_read_samples(struct ad7606_state *st)
>  			return -EIO;
>  		}
>  
> -		data++;
> +		if (storagebits > 16)
> +			data32++;
> +		else
> +			data16++;
>  		num--;
>  	}
>  
> -	return st->bops->read_block(st->dev, num, data);
> +	ret = st->bops->read_block(st->dev, num, data);

Since data++ was removed, this looks broken now as well as the
other read_block() not visible in the diff.

Maybe better to drop data32 and data16, keep the change of data
to void*, and change data++ to data += BITS_TO_BYTES(storagebits)?

Although, all of this might be moot since it looks like this
needs to be rebased on [1].

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=fixes-togreg&id=90826e08468ba7fb35d8b39645b22d9e80004afe

> +	if (ret)
> +		return ret;
> +
> +	if (storagebits == 16 || !sign_extend_samples)
> +		return 0;
> +
> +	/* For 18 bit samples, we need to sign-extend samples to 32 bits */
> +	for (i = 0; i < num; i++)
> +		data32[i] = sign_extend32(data32[i], 17);> +
> +	return 0;
>  }
>  
>  static irqreturn_t ad7606_trigger_handler(int irq, void *p)
> @@ -124,11 +176,11 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>  
>  	guard(mutex)(&st->lock);
>  
> -	ret = ad7606_read_samples(st);
> +	ret = ad7606_read_samples(st, true);

Shouldn't the sign_extend parameter depend on if the data is unipolar or bipolar?

>  	if (ret)
>  		goto error_ret;
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, st->data,
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->data.d16,
>  					   iio_get_time_ns(indio_dev));
>  error_ret:
>  	iio_trigger_notify_done(indio_dev->trig);
> @@ -142,6 +194,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
>  			      int *val)
>  {
>  	struct ad7606_state *st = iio_priv(indio_dev);
> +	unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
>  	int ret;
>  
>  	gpiod_set_value(st->gpio_convst, 1);
> @@ -152,9 +205,13 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
>  		goto error_ret;
>  	}
>  
> -	ret = ad7606_read_samples(st);
> -	if (ret == 0)
> -		*val = sign_extend32(st->data[ch], 15);
> +	ret = ad7606_read_samples(st, false);

Why not let ad7606_read_samples() do the sign extending since
it can do that now?

> +	if (ret == 0) {
> +		if (storagebits > 16)
> +			*val = sign_extend32(st->data.d32[ch], 17);
> +		else
> +			*val = sign_extend32(st->data.d16[ch], 15);
> +	}
>  
>  error_ret:
>  	gpiod_set_value(st->gpio_convst, 0);
> @@ -267,7 +324,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>  			ch = chan->address;
>  		cs = &st->chan_scales[ch];
>  		i = find_closest(val2, cs->scale_avail, cs->num_scales);
> -		ret = st->write_scale(indio_dev, ch, i);
> +		ret = st->write_scale(indio_dev, ch, i + cs->reg_offset);
>  		if (ret < 0)
>  			return ret;
>  		cs->range = i;
> @@ -350,6 +407,18 @@ static const struct iio_chan_spec ad7606_channels_16bit[] = {
>  	AD7606_CHANNEL(7, 16),
>  };
>  
> +static const struct iio_chan_spec ad7606_channels_18bit[] = {
> +	IIO_CHAN_SOFT_TIMESTAMP(8),
> +	AD7606_CHANNEL(0, 18),
> +	AD7606_CHANNEL(1, 18),
> +	AD7606_CHANNEL(2, 18),
> +	AD7606_CHANNEL(3, 18),
> +	AD7606_CHANNEL(4, 18),
> +	AD7606_CHANNEL(5, 18),
> +	AD7606_CHANNEL(6, 18),
> +	AD7606_CHANNEL(7, 18),
> +};
> +
>  /*
>   * The current assumption that this driver makes for AD7616, is that it's
>   * working in Hardware Mode with Serial, Burst and Sequencer modes activated.
> @@ -410,6 +479,18 @@ static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
>  		.oversampling_avail = ad7606_oversampling_avail,
>  		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
>  	},
> +	[ID_AD7606C_16] = {
> +		.channels = ad7606_channels_16bit,
> +		.num_channels = 9,

Could be nice to have a cleanup patch before this to convert others to
use ARRAY_SIZE(), then use ARRAY_SIZE(ad7606_channels_16bit) here
instead of 9.

> +		.oversampling_avail = ad7606_oversampling_avail,
> +		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
> +	},
> +	[ID_AD7606C_18] = {
> +		.channels = ad7606_channels_18bit,
> +		.num_channels = 9,
> +		.oversampling_avail = ad7606_oversampling_avail,
> +		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
> +	},
>  	[ID_AD7616] = {
>  		.channels = ad7616_channels,
>  		.num_channels = 17,
> @@ -581,7 +662,122 @@ static const struct iio_trigger_ops ad7606_trigger_ops = {
>  	.validate_device = iio_trigger_validate_own_device,
>  };
>  
> -static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
> +static void ad7606c_18_chan_setup(struct ad7606_state *st, int ch,
> +				  bool bipolar, bool differential)
> +{
> +	struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> +
> +	if (differential) {
> +		cs->scale_avail =
> +			ad7606c_18_scale_differential_bipolar_avail;
> +		cs->num_scales =
> +			ARRAY_SIZE(ad7606c_18_scale_differential_bipolar_avail);
> +		/* Bipolar differential ranges start at 8 (b1000) */
> +		cs->reg_offset = 8;
> +		cs->range = 1;
> +	} else if (bipolar) {
> +		cs->scale_avail =
> +			ad7606c_18_scale_single_ended_bipolar_avail;
> +		cs->num_scales =
> +			ARRAY_SIZE(ad7606c_18_scale_single_ended_bipolar_avail);

I guess cs->reg_offset is 0 for this one?

> +		cs->range = 3;
> +	} else {
> +		cs->scale_avail =
> +			ad7606c_18_scale_single_ended_unipolar_avail;
> +		cs->num_scales =
> +			ARRAY_SIZE(ad7606c_18_scale_single_ended_unipolar_avail);
> +		/* Unipolar single-ended ranges start at 5 (b0101) */
> +		cs->reg_offset = 5;
> +		cs->range = 1;
> +	}
> +}
> +
> +static void ad7606c_16_chan_setup(struct ad7606_state *st, int ch,
> +				  bool bipolar, bool differential)
> +{
> +	struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> +
> +	if (differential) {
> +		cs->scale_avail =
> +			ad7606c_16_scale_differential_bipolar_avail;
> +		cs->num_scales =
> +			ARRAY_SIZE(ad7606c_16_scale_differential_bipolar_avail);
> +		/* Bipolar differential ranges start at 8 (b1000) */
> +		cs->reg_offset = 8;
> +		cs->range = 1;
> +	} else if (bipolar) {
> +		cs->scale_avail =
> +			ad7606c_16_scale_single_ended_bipolar_avail;
> +		cs->num_scales =
> +			ARRAY_SIZE(ad7606c_16_scale_single_ended_bipolar_avail);
> +		cs->range = 3;
> +	} else {
> +		cs->scale_avail =
> +			ad7606c_16_scale_single_ended_unipolar_avail;
> +		cs->num_scales =
> +			ARRAY_SIZE(ad7606c_16_scale_single_ended_unipolar_avail);
> +		/* Unipolar single-ended ranges start at 5 (b0101) */
> +		cs->reg_offset = 5;
> +		cs->range = 1;
> +	}
> +}
> +
> +static int ad7606c_sw_mode_setup_channels(struct iio_dev *indio_dev,
> +					  ad7606c_chan_setup_cb_t chan_setup_cb)
> +{
> +	unsigned int num_channels = indio_dev->num_channels - 1;
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +	bool chan_configured[AD760X_MAX_CHANNELS] = {};
> +	struct device *dev = st->dev;
> +	int ret;
> +	u32 ch;
> +
> +	/* We need to hook this first */

Comment would be more useful if it said why.

> +	ret = st->bops->sw_mode_config(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		bool bipolar, differential;
> +		u32 pins[2];
> +
> +		ret = fwnode_property_read_u32(child, "reg", &ch);
> +		if (ret)
> +			continue;
> +
> +		/* channel number (here) is from 1 to num_channels */
> +		if (ch == 0 || ch > num_channels) {
> +			dev_warn(st->dev,
> +				 "Invalid channel number (ignoring): %d\n", ch);
> +			continue;
> +		}
> +
> +		bipolar = fwnode_property_present(child, "bipolar");

IIRC, fwnode_property_read_bool() is preferred for bool/flag properties.

> +
> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
> +						     pins, ARRAY_SIZE(pins));
> +		/* Channel is differential, if pins are the same as 'reg' */
> +		if (ret == 0 && pins[0] == ch && pins[1] == ch)
> +			differential = true;
> +		else
> +			differential = false;

Would probably better to error on bad pin numbers rather than default to
not differential.

> +
> +		ch--;
> +
> +		chan_setup_cb(st, ch, bipolar, differential);
> +		chan_configured[ch] = true;
> +	}
> +
> +	/* Apply default configuration to unconfigured (via DT) channels */
> +	for (ch = 0; ch < num_channels; ch++) {
> +		if (!chan_configured[ch])
> +			chan_setup_cb(st, ch, false, false);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7606_sw_mode_setup(struct iio_dev *indio_dev, unsigned int id)
>  {
>  	unsigned int num_channels = indio_dev->num_channels - 1;
>  	struct ad7606_state *st = iio_priv(indio_dev);
> @@ -596,17 +792,30 @@ static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
>  
>  	indio_dev->info = &ad7606_info_sw_mode;
>  
> -	/* Scale of 0.076293 is only available in sw mode */
> -	/* After reset, in software mode, ±10 V is set by default */
> -	for (ch = 0; ch < num_channels; ch++) {
> -		struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> +	switch (id) {
> +	case ID_AD7606C_18:
> +		ret = ad7606c_sw_mode_setup_channels(indio_dev,
> +						     ad7606c_18_chan_setup);
> +		break;
> +	case ID_AD7606C_16:
> +		ret = ad7606c_sw_mode_setup_channels(indio_dev,
> +						     ad7606c_16_chan_setup);
> +		break;
> +	default:
> +		/* Scale of 0.076293 is only available in sw mode */
> +		/* After reset, in software mode, ±10 V is set by default */
> +		for (ch = 0; ch < num_channels; ch++) {
> +			struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> +
> +			cs->scale_avail = ad7616_sw_scale_avail;
> +			cs->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
> +			cs->range = 2;
> +		}
>  
> -		cs->scale_avail = ad7616_sw_scale_avail;
> -		cs->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
> -		cs->range = 2;
> +		ret = st->bops->sw_mode_config(indio_dev);
> +		break;
>  	}
>  
> -	ret = st->bops->sw_mode_config(indio_dev);
>  	if (ret)
>  		return ret;
>  
> @@ -655,9 +864,16 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  	st->oversampling = 1;
>  
>  	cs = &st->chan_scales[0];
> -	cs->range = 0;
> -	cs->scale_avail = ad7606_scale_avail;
> -	cs->num_scales = ARRAY_SIZE(ad7606_scale_avail);
> +	switch (id) {
> +	case ID_AD7606C_18:
> +		cs->scale_avail = ad7606_18bit_hw_scale_avail;
> +		cs->num_scales = ARRAY_SIZE(ad7606_18bit_hw_scale_avail);
> +		break;
> +	default:
> +		cs->scale_avail = ad7606_16bit_hw_scale_avail;
> +		cs->num_scales = ARRAY_SIZE(ad7606_16bit_hw_scale_avail);
> +		break;
> +	}
>  
>  	ret = devm_regulator_get_enable(dev, "avcc");
>  	if (ret)
> @@ -706,7 +922,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  	st->write_scale = ad7606_write_scale_hw;
>  	st->write_os = ad7606_write_os_hw;
>  
> -	ret = ad7606_sw_mode_setup(indio_dev);
> +	ret = ad7606_sw_mode_setup(indio_dev, id);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index 2113ad460c0f..6b0897aa2dc7 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> @@ -22,7 +22,7 @@
>  		.scan_type = {					\
>  			.sign = 's',				\
>  			.realbits = (bits),			\
> -			.storagebits = (bits),			\
> +			.storagebits = (bits) > 16 ? 32 : 16,	\
>  			.endianness = IIO_CPU,			\
>  		},						\
>  }
> @@ -45,7 +45,7 @@
>  		.scan_type = {					\
>  			.sign = 's',				\
>  			.realbits = (bits),			\
> -			.storagebits = (bits),			\
> +			.storagebits = (bits) > 16 ? 32 : 16,	\
>  			.endianness = IIO_CPU,			\
>  		},						\
>  }
> @@ -88,6 +88,8 @@ struct ad7606_chip_info {
>   *			such that it can be read via the 'read_avail' hook
>   * @num_scales		number of elements stored in the scale_avail array
>   * @range		voltage range selection, selects which scale to apply
> + * @reg_offset		offset for the register value, to be applied when
> + *			writing the value of 'range' to the register value
>   */
>  struct ad7606_chan_scale {
>  #define AD760X_MAX_SCALE_SHOW		(AD760X_MAX_CHANNELS * 2)
> @@ -95,6 +97,7 @@ struct ad7606_chan_scale {
>  	int				scale_avail_show[AD760X_MAX_SCALE_SHOW];
>  	unsigned int			num_scales;
>  	unsigned int			range;
> +	unsigned int			reg_offset;
>  };
>  
>  /**
> @@ -151,9 +154,13 @@ struct ad7606_state {
>  	/*
>  	 * DMA (thus cache coherency maintenance) may require the
>  	 * transfer buffers to live in their own cache lines.
> -	 * 16 * 16-bit samples + 64-bit timestamp
> +	 * 16 * 16-bit samples + 64-bit timestamp - for AD7616
> +	 * 8 * 32-bit samples + 64-bit timestamp - for AD7616C-18 (and similar)
>  	 */
> -	unsigned short			data[20] __aligned(IIO_DMA_MINALIGN);
> +	union {
> +		unsigned short d16[20];
> +		unsigned int d32[10];
> +	} data __aligned(IIO_DMA_MINALIGN);
>  	__be16				d16[2];
>  };
>  
> @@ -192,6 +199,8 @@ enum ad7606_supported_device_ids {
>  	ID_AD7606_6,
>  	ID_AD7606_4,
>  	ID_AD7606B,
> +	ID_AD7606C_16,
> +	ID_AD7606C_18,
>  	ID_AD7616,
>  };
>  
> diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
> index e00f58a6a0e9..b8d630ad156d 100644
> --- a/drivers/iio/adc/ad7606_spi.c
> +++ b/drivers/iio/adc/ad7606_spi.c
> @@ -77,6 +77,18 @@ static const struct iio_chan_spec ad7606b_sw_channels[] = {
>  	AD7606_SW_CHANNEL(7, 16),
>  };
>  
> +static const struct iio_chan_spec ad7606c_18_sw_channels[] = {
> +	IIO_CHAN_SOFT_TIMESTAMP(8),
> +	AD7606_SW_CHANNEL(0, 18),
> +	AD7606_SW_CHANNEL(1, 18),
> +	AD7606_SW_CHANNEL(2, 18),
> +	AD7606_SW_CHANNEL(3, 18),
> +	AD7606_SW_CHANNEL(4, 18),
> +	AD7606_SW_CHANNEL(5, 18),
> +	AD7606_SW_CHANNEL(6, 18),
> +	AD7606_SW_CHANNEL(7, 18),
> +};
> +
>  static const unsigned int ad7606B_oversampling_avail[9] = {
>  	1, 2, 4, 8, 16, 32, 64, 128, 256
>  };
> @@ -120,6 +132,19 @@ static int ad7606_spi_read_block(struct device *dev,
>  	return 0;
>  }
>  
> +static int ad7606_spi_read_block18to32(struct device *dev,
> +				       int count, void *buf)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct spi_transfer xfer = {
> +		.bits_per_word = 18,
> +		.len = count,

Isn't count the number of words? .len needs to be the number
of bytes, so 4 * count.

> +		.rx_buf = buf,
> +	};
> +
> +	return spi_sync_transfer(spi, &xfer, 1);
> +}
> +
>  static int ad7606_spi_reg_read(struct ad7606_state *st, unsigned int addr)
>  {
>  	struct spi_device *spi = to_spi_device(st->dev);
> @@ -283,6 +308,19 @@ static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
>  	return 0;
>  }
>  
> +static int ad7606c_18_sw_mode_config(struct iio_dev *indio_dev)
> +{
> +	int ret;
> +
> +	ret = ad7606B_sw_mode_config(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->channels = ad7606c_18_sw_channels;
> +
> +	return 0;
> +}
> +
>  static const struct ad7606_bus_ops ad7606_spi_bops = {
>  	.read_block = ad7606_spi_read_block,
>  };
> @@ -305,6 +343,15 @@ static const struct ad7606_bus_ops ad7606B_spi_bops = {
>  	.sw_mode_config = ad7606B_sw_mode_config,
>  };
>  
> +static const struct ad7606_bus_ops ad7606c_18_spi_bops = {
> +	.read_block = ad7606_spi_read_block18to32,
> +	.reg_read = ad7606_spi_reg_read,
> +	.reg_write = ad7606_spi_reg_write,
> +	.write_mask = ad7606_spi_write_mask,
> +	.rd_wr_cmd = ad7606B_spi_rd_wr_cmd,
> +	.sw_mode_config = ad7606c_18_sw_mode_config,
> +};
> +
>  static int ad7606_spi_probe(struct spi_device *spi)
>  {
>  	const struct spi_device_id *id = spi_get_device_id(spi);
> @@ -315,8 +362,12 @@ static int ad7606_spi_probe(struct spi_device *spi)
>  		bops = &ad7616_spi_bops;
>  		break;
>  	case ID_AD7606B:
> +	case ID_AD7606C_16:
>  		bops = &ad7606B_spi_bops;
>  		break;
> +	case ID_AD7606C_18:
> +		bops = &ad7606c_18_spi_bops;
> +		break;
>  	default:
>  		bops = &ad7606_spi_bops;
>  		break;
> @@ -333,6 +384,8 @@ static const struct spi_device_id ad7606_id_table[] = {
>  	{ "ad7606-6", ID_AD7606_6 },
>  	{ "ad7606-8", ID_AD7606_8 },
>  	{ "ad7606b",  ID_AD7606B },
> +	{ "ad7606c-16",  ID_AD7606C_16 },
> +	{ "ad7606c-18",  ID_AD7606C_18 },
>  	{ "ad7616",   ID_AD7616 },
>  	{ }
>  };
> @@ -344,6 +397,8 @@ static const struct of_device_id ad7606_of_match[] = {
>  	{ .compatible = "adi,ad7606-6" },
>  	{ .compatible = "adi,ad7606-8" },
>  	{ .compatible = "adi,ad7606b" },
> +	{ .compatible = "adi,ad7606c-16" },
> +	{ .compatible = "adi,ad7606c-18" },
>  	{ .compatible = "adi,ad7616" },
>  	{ }
>  };

Re: [PATCH v4 1/8] iio: adc: ad7606: add 'bits' parameter to channels macros

[Alexandru Ardelean]

On Fri, Sep 6, 2024 at 12:25 AM David Lechner <dlechner@baylibre.com> wrote:
>
> On 9/5/24 3:23 AM, Alexandru Ardelean wrote:
> > There are some newer additions to the AD7606 family, which support 18 bit
> > precision.
> > Up until now, all chips were 16 bit.
> >
> > This change adds a 'bits' parameter to the AD760X_CHANNEL macro and renames
> > 'ad7606_channels' -> 'ad7606_channels_16bit' for the current devices.
> >
> > The AD7606_SW_CHANNEL() macro is also introduced, as a short-hand for IIO
> > channels in SW mode.
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > ---
>
> ...
>
> > diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> > index 0c6a88cc4695..771121350f98 100644
> > --- a/drivers/iio/adc/ad7606.h
> > +++ b/drivers/iio/adc/ad7606.h
> > @@ -8,7 +8,7 @@
> >  #ifndef IIO_ADC_AD7606_H_
> >  #define IIO_ADC_AD7606_H_
> >
> > -#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all) { \
> > +#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all, bits) {   \
> >               .type = IIO_VOLTAGE,                            \
> >               .indexed = 1,                                   \
> >               .channel = num,                                 \
> > @@ -19,24 +19,26 @@
> >               .scan_index = num,                              \
> >               .scan_type = {                                  \
> >                       .sign = 's',                            \
> > -                     .realbits = 16,                         \
> > -                     .storagebits = 16,                      \
> > +                     .realbits = (bits),                     \
> > +                     .storagebits = (bits),                  \
>
> Technically OK in this patch since bits is still always 16 but we
> can avoid changing the same line again later to:
>
>         (bits) > 16 ? 32 : 16
>
> if we just do that in this patch.

sure
will update

>
>
> >                       .endianness = IIO_CPU,                  \
> >               },                                              \
> >  }

Re: [PATCH v4 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts

[Alexandru Ardelean]

On Fri, Sep 6, 2024 at 2:30 AM David Lechner <dlechner@baylibre.com> wrote:
>
> On 9/5/24 3:24 AM, Alexandru Ardelean wrote:
> > The AD7606C-16 and AD7606C-18 are pretty similar with the AD7606B.
> > The main difference between AD7606C-16 & AD7606C-18 is the precision in
> > bits (16 vs 18).
> > Because of that, some scales need to be defined for the 18-bit variants, as
> > they need to be computed against 2**18 (vs 2**16 for the 16 bit-variants).
> >
> > Because the AD7606C-16,18 also supports bipolar & differential channels,
> > for SW-mode, the default range of 10 V or ±10V should be set at probe.
> > On reset, the default range (in the registers) is set to value 0x3 which
> > corresponds to '±10 V single-ended range', regardless of bipolar or
> > differential configuration.
> >
> > Aside from the scale/ranges, the AD7606C-16 is similar to the AD7606B.
> >
> > The AD7606C-18 variant offers 18-bit precision. Because of this, the
> > requirement to use this chip is that the SPI controller supports padding
> > of 18-bit sequences to 32-bit arrays.
> >
> > Datasheet links:
> >   https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
> >   https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > ---
> >  drivers/iio/adc/ad7606.c     | 266 +++++++++++++++++++++++++++++++----
> >  drivers/iio/adc/ad7606.h     |  17 ++-
> >  drivers/iio/adc/ad7606_spi.c |  55 ++++++++
> >  3 files changed, 309 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> > index 4c3fbb28f790..999c4411859e 100644
> > --- a/drivers/iio/adc/ad7606.c
> > +++ b/drivers/iio/adc/ad7606.c
> > @@ -28,14 +28,44 @@
> >
> >  #include "ad7606.h"
> >
> > +typedef void (*ad7606c_chan_setup_cb_t)(struct ad7606_state *st, int ch,
> > +                                     bool bipolar, bool differential);
> > +
> >  /*
> >   * Scales are computed as 5000/32768 and 10000/32768 respectively,
> >   * so that when applied to the raw values they provide mV values
> >   */
> > -static const unsigned int ad7606_scale_avail[2] = {
> > +static const unsigned int ad7606_16bit_hw_scale_avail[2] = {
> >       152588, 305176
> >  };
> >
> > +static const unsigned int ad7606_18bit_hw_scale_avail[2] = {
> > +     38147, 76294
> > +};
> > +
> > +static const unsigned int ad7606c_16_scale_single_ended_unipolar_avail[3] = {
> > +     76294, 152588, 190735,
> > +};
> > +
> > +static const unsigned int ad7606c_16_scale_single_ended_bipolar_avail[5] = {
> > +     76294, 152588, 190735, 305176, 381470
> > +};
> > +
> > +static const unsigned int ad7606c_16_scale_differential_bipolar_avail[4] = {
> > +     152588, 305176, 381470, 610352
> > +};
> > +
> > +static const unsigned int ad7606c_18_scale_single_ended_unipolar_avail[3] = {
> > +     19073, 38147, 47684
> > +};
> > +
> > +static const unsigned int ad7606c_18_scale_single_ended_bipolar_avail[5] = {
> > +     19073, 38147, 47684, 76294, 95367
> > +};
> > +
> > +static const unsigned int ad7606c_18_scale_differential_bipolar_avail[4] = {
> > +     38147, 76294, 95367, 152588
> > +};
> >
> >  static const unsigned int ad7616_sw_scale_avail[3] = {
> >       76293, 152588, 305176
> > @@ -82,11 +112,19 @@ static int ad7606_reg_access(struct iio_dev *indio_dev,
> >       }
> >  }
> >
> > -static int ad7606_read_samples(struct ad7606_state *st)
> > +static int ad7606_read_samples(struct ad7606_state *st, bool sign_extend_samples)
> >  {
> > +     unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
>
> Why [1]? Sure, they are all the same, but [0] would seem less arbitrary.

[0] is the timestamp channel.


>
> >       unsigned int num = st->chip_info->num_channels - 1;
> > -     u16 *data = st->data;
> > -     int ret;
> > +     u32 *data32 = st->data.d32;
> > +     u16 *data16 = st->data.d16;
> > +     void *data;
> > +     int i, ret;
> > +
> > +     if (storagebits > 16)
> > +             data = data32;
> > +     else
> > +             data = data16;
> >
> >       /*
> >        * The frstdata signal is set to high while and after reading the sample
> > @@ -108,11 +146,25 @@ static int ad7606_read_samples(struct ad7606_state *st)
> >                       return -EIO;
> >               }
> >
> > -             data++;
> > +             if (storagebits > 16)
> > +                     data32++;
> > +             else
> > +                     data16++;
> >               num--;
> >       }
> >
> > -     return st->bops->read_block(st->dev, num, data);
> > +     ret = st->bops->read_block(st->dev, num, data);
>
> Since data++ was removed, this looks broken now as well as the
> other read_block() not visible in the diff.
>
> Maybe better to drop data32 and data16, keep the change of data
> to void*, and change data++ to data += BITS_TO_BYTES(storagebits)?
>
> Although, all of this might be moot since it looks like this
> needs to be rebased on [1].
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=fixes-togreg&id=90826e08468ba7fb35d8b39645b22d9e80004afe

Oh.
Omitted that patch.
I forgot that fixes-togreg has a different cadence in another branch.

>
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (storagebits == 16 || !sign_extend_samples)
> > +             return 0;
> > +
> > +     /* For 18 bit samples, we need to sign-extend samples to 32 bits */
> > +     for (i = 0; i < num; i++)
> > +             data32[i] = sign_extend32(data32[i], 17);> +
> > +     return 0;
> >  }
> >
> >  static irqreturn_t ad7606_trigger_handler(int irq, void *p)
> > @@ -124,11 +176,11 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
> >
> >       guard(mutex)(&st->lock);
> >
> > -     ret = ad7606_read_samples(st);
> > +     ret = ad7606_read_samples(st, true);
>
> Shouldn't the sign_extend parameter depend on if the data is unipolar or bipolar?

[c1]
Sign-extension is only needed for 18-bit samples.
16-bit samples are already properly sign(ed), but to 16-bits.

It's a slight performance improvement, that may look quirky here.
The idea here, is that for ad7606_scan_direct() we only need to
sign-extend 1 sample of the 8 samples we get.
And we need to sign-extend it to 32 bits regardless of it being 16-bit
or 18-bit.

In ad7606_trigger_handler(), the 16-bit samples were pushed as-is.
Which means that we need to sign-extend the samples at least for
18-bits (as it is a new part)
The question now becomes if we should sign-extend to 32-bits, 16-bit
samples in ad7606_trigger_handler(), as that may break some ABI.

>
> >       if (ret)
> >               goto error_ret;
> >
> > -     iio_push_to_buffers_with_timestamp(indio_dev, st->data,
> > +     iio_push_to_buffers_with_timestamp(indio_dev, st->data.d16,
> >                                          iio_get_time_ns(indio_dev));
> >  error_ret:
> >       iio_trigger_notify_done(indio_dev->trig);
> > @@ -142,6 +194,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> >                             int *val)
> >  {
> >       struct ad7606_state *st = iio_priv(indio_dev);
> > +     unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
> >       int ret;
> >
> >       gpiod_set_value(st->gpio_convst, 1);
> > @@ -152,9 +205,13 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> >               goto error_ret;
> >       }
> >
> > -     ret = ad7606_read_samples(st);
> > -     if (ret == 0)
> > -             *val = sign_extend32(st->data[ch], 15);
> > +     ret = ad7606_read_samples(st, false);
>
> Why not let ad7606_read_samples() do the sign extending since
> it can do that now?

Related to comment [c1]

>
> > +     if (ret == 0) {
> > +             if (storagebits > 16)
> > +                     *val = sign_extend32(st->data.d32[ch], 17);
> > +             else
> > +                     *val = sign_extend32(st->data.d16[ch], 15);
> > +     }
> >
> >  error_ret:
> >       gpiod_set_value(st->gpio_convst, 0);
> > @@ -267,7 +324,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> >                       ch = chan->address;
> >               cs = &st->chan_scales[ch];
> >               i = find_closest(val2, cs->scale_avail, cs->num_scales);
> > -             ret = st->write_scale(indio_dev, ch, i);
> > +             ret = st->write_scale(indio_dev, ch, i + cs->reg_offset);
> >               if (ret < 0)
> >                       return ret;
> >               cs->range = i;
> > @@ -350,6 +407,18 @@ static const struct iio_chan_spec ad7606_channels_16bit[] = {
> >       AD7606_CHANNEL(7, 16),
> >  };
> >
> > +static const struct iio_chan_spec ad7606_channels_18bit[] = {
> > +     IIO_CHAN_SOFT_TIMESTAMP(8),
> > +     AD7606_CHANNEL(0, 18),
> > +     AD7606_CHANNEL(1, 18),
> > +     AD7606_CHANNEL(2, 18),
> > +     AD7606_CHANNEL(3, 18),
> > +     AD7606_CHANNEL(4, 18),
> > +     AD7606_CHANNEL(5, 18),
> > +     AD7606_CHANNEL(6, 18),
> > +     AD7606_CHANNEL(7, 18),
> > +};
> > +
> >  /*
> >   * The current assumption that this driver makes for AD7616, is that it's
> >   * working in Hardware Mode with Serial, Burst and Sequencer modes activated.
> > @@ -410,6 +479,18 @@ static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
> >               .oversampling_avail = ad7606_oversampling_avail,
> >               .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
> >       },
> > +     [ID_AD7606C_16] = {
> > +             .channels = ad7606_channels_16bit,
> > +             .num_channels = 9,
>
> Could be nice to have a cleanup patch before this to convert others to
> use ARRAY_SIZE(), then use ARRAY_SIZE(ad7606_channels_16bit) here
> instead of 9.

Ack.

>
> > +             .oversampling_avail = ad7606_oversampling_avail,
> > +             .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
> > +     },
> > +     [ID_AD7606C_18] = {
> > +             .channels = ad7606_channels_18bit,
> > +             .num_channels = 9,
> > +             .oversampling_avail = ad7606_oversampling_avail,
> > +             .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
> > +     },
> >       [ID_AD7616] = {
> >               .channels = ad7616_channels,
> >               .num_channels = 17,
> > @@ -581,7 +662,122 @@ static const struct iio_trigger_ops ad7606_trigger_ops = {
> >       .validate_device = iio_trigger_validate_own_device,
> >  };
> >
> > -static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
> > +static void ad7606c_18_chan_setup(struct ad7606_state *st, int ch,
> > +                               bool bipolar, bool differential)
> > +{
> > +     struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> > +
> > +     if (differential) {
> > +             cs->scale_avail =
> > +                     ad7606c_18_scale_differential_bipolar_avail;
> > +             cs->num_scales =
> > +                     ARRAY_SIZE(ad7606c_18_scale_differential_bipolar_avail);
> > +             /* Bipolar differential ranges start at 8 (b1000) */
> > +             cs->reg_offset = 8;
> > +             cs->range = 1;
> > +     } else if (bipolar) {
> > +             cs->scale_avail =
> > +                     ad7606c_18_scale_single_ended_bipolar_avail;
> > +             cs->num_scales =
> > +                     ARRAY_SIZE(ad7606c_18_scale_single_ended_bipolar_avail);
>
> I guess cs->reg_offset is 0 for this one?

Yes.
I will make it explicit.

>
> > +             cs->range = 3;
> > +     } else {
> > +             cs->scale_avail =
> > +                     ad7606c_18_scale_single_ended_unipolar_avail;
> > +             cs->num_scales =
> > +                     ARRAY_SIZE(ad7606c_18_scale_single_ended_unipolar_avail);
> > +             /* Unipolar single-ended ranges start at 5 (b0101) */
> > +             cs->reg_offset = 5;
> > +             cs->range = 1;
> > +     }
> > +}
> > +
> > +static void ad7606c_16_chan_setup(struct ad7606_state *st, int ch,
> > +                               bool bipolar, bool differential)
> > +{
> > +     struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> > +
> > +     if (differential) {
> > +             cs->scale_avail =
> > +                     ad7606c_16_scale_differential_bipolar_avail;
> > +             cs->num_scales =
> > +                     ARRAY_SIZE(ad7606c_16_scale_differential_bipolar_avail);
> > +             /* Bipolar differential ranges start at 8 (b1000) */
> > +             cs->reg_offset = 8;
> > +             cs->range = 1;
> > +     } else if (bipolar) {
> > +             cs->scale_avail =
> > +                     ad7606c_16_scale_single_ended_bipolar_avail;
> > +             cs->num_scales =
> > +                     ARRAY_SIZE(ad7606c_16_scale_single_ended_bipolar_avail);
> > +             cs->range = 3;
> > +     } else {
> > +             cs->scale_avail =
> > +                     ad7606c_16_scale_single_ended_unipolar_avail;
> > +             cs->num_scales =
> > +                     ARRAY_SIZE(ad7606c_16_scale_single_ended_unipolar_avail);
> > +             /* Unipolar single-ended ranges start at 5 (b0101) */
> > +             cs->reg_offset = 5;
> > +             cs->range = 1;
> > +     }
> > +}
> > +
> > +static int ad7606c_sw_mode_setup_channels(struct iio_dev *indio_dev,
> > +                                       ad7606c_chan_setup_cb_t chan_setup_cb)
> > +{
> > +     unsigned int num_channels = indio_dev->num_channels - 1;
> > +     struct ad7606_state *st = iio_priv(indio_dev);
> > +     bool chan_configured[AD760X_MAX_CHANNELS] = {};
> > +     struct device *dev = st->dev;
> > +     int ret;
> > +     u32 ch;
> > +
> > +     /* We need to hook this first */
>
> Comment would be more useful if it said why.

Ack.
Will add.

>
> > +     ret = st->bops->sw_mode_config(indio_dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     device_for_each_child_node_scoped(dev, child) {
> > +             bool bipolar, differential;
> > +             u32 pins[2];
> > +
> > +             ret = fwnode_property_read_u32(child, "reg", &ch);
> > +             if (ret)
> > +                     continue;
> > +
> > +             /* channel number (here) is from 1 to num_channels */
> > +             if (ch == 0 || ch > num_channels) {
> > +                     dev_warn(st->dev,
> > +                              "Invalid channel number (ignoring): %d\n", ch);
> > +                     continue;
> > +             }
> > +
> > +             bipolar = fwnode_property_present(child, "bipolar");
>
> IIRC, fwnode_property_read_bool() is preferred for bool/flag properties.

Ack.

>
> > +
> > +             ret = fwnode_property_read_u32_array(child, "diff-channels",
> > +                                                  pins, ARRAY_SIZE(pins));
> > +             /* Channel is differential, if pins are the same as 'reg' */
> > +             if (ret == 0 && pins[0] == ch && pins[1] == ch)
> > +                     differential = true;
> > +             else
> > +                     differential = false;
>
> Would probably better to error on bad pin numbers rather than default to
> not differential.

No strong preference from my side.
Will implement failure/error case.

>
> > +
> > +             ch--;
> > +
> > +             chan_setup_cb(st, ch, bipolar, differential);
> > +             chan_configured[ch] = true;
> > +     }
> > +
> > +     /* Apply default configuration to unconfigured (via DT) channels */
> > +     for (ch = 0; ch < num_channels; ch++) {
> > +             if (!chan_configured[ch])
> > +                     chan_setup_cb(st, ch, false, false);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int ad7606_sw_mode_setup(struct iio_dev *indio_dev, unsigned int id)
> >  {
> >       unsigned int num_channels = indio_dev->num_channels - 1;
> >       struct ad7606_state *st = iio_priv(indio_dev);
> > @@ -596,17 +792,30 @@ static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
> >
> >       indio_dev->info = &ad7606_info_sw_mode;
> >
> > -     /* Scale of 0.076293 is only available in sw mode */
> > -     /* After reset, in software mode, ±10 V is set by default */
> > -     for (ch = 0; ch < num_channels; ch++) {
> > -             struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> > +     switch (id) {
> > +     case ID_AD7606C_18:
> > +             ret = ad7606c_sw_mode_setup_channels(indio_dev,
> > +                                                  ad7606c_18_chan_setup);
> > +             break;
> > +     case ID_AD7606C_16:
> > +             ret = ad7606c_sw_mode_setup_channels(indio_dev,
> > +                                                  ad7606c_16_chan_setup);
> > +             break;
> > +     default:
> > +             /* Scale of 0.076293 is only available in sw mode */
> > +             /* After reset, in software mode, ±10 V is set by default */
> > +             for (ch = 0; ch < num_channels; ch++) {
> > +                     struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> > +
> > +                     cs->scale_avail = ad7616_sw_scale_avail;
> > +                     cs->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
> > +                     cs->range = 2;
> > +             }
> >
> > -             cs->scale_avail = ad7616_sw_scale_avail;
> > -             cs->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
> > -             cs->range = 2;
> > +             ret = st->bops->sw_mode_config(indio_dev);
> > +             break;
> >       }
> >
> > -     ret = st->bops->sw_mode_config(indio_dev);
> >       if (ret)
> >               return ret;
> >
> > @@ -655,9 +864,16 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
> >       st->oversampling = 1;
> >
> >       cs = &st->chan_scales[0];
> > -     cs->range = 0;
> > -     cs->scale_avail = ad7606_scale_avail;
> > -     cs->num_scales = ARRAY_SIZE(ad7606_scale_avail);
> > +     switch (id) {
> > +     case ID_AD7606C_18:
> > +             cs->scale_avail = ad7606_18bit_hw_scale_avail;
> > +             cs->num_scales = ARRAY_SIZE(ad7606_18bit_hw_scale_avail);
> > +             break;
> > +     default:
> > +             cs->scale_avail = ad7606_16bit_hw_scale_avail;
> > +             cs->num_scales = ARRAY_SIZE(ad7606_16bit_hw_scale_avail);
> > +             break;
> > +     }
> >
> >       ret = devm_regulator_get_enable(dev, "avcc");
> >       if (ret)
> > @@ -706,7 +922,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
> >       st->write_scale = ad7606_write_scale_hw;
> >       st->write_os = ad7606_write_os_hw;
> >
> > -     ret = ad7606_sw_mode_setup(indio_dev);
> > +     ret = ad7606_sw_mode_setup(indio_dev, id);
> >       if (ret)
> >               return ret;
> >
> > diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> > index 2113ad460c0f..6b0897aa2dc7 100644
> > --- a/drivers/iio/adc/ad7606.h
> > +++ b/drivers/iio/adc/ad7606.h
> > @@ -22,7 +22,7 @@
> >               .scan_type = {                                  \
> >                       .sign = 's',                            \
> >                       .realbits = (bits),                     \
> > -                     .storagebits = (bits),                  \
> > +                     .storagebits = (bits) > 16 ? 32 : 16,   \
> >                       .endianness = IIO_CPU,                  \
> >               },                                              \
> >  }
> > @@ -45,7 +45,7 @@
> >               .scan_type = {                                  \
> >                       .sign = 's',                            \
> >                       .realbits = (bits),                     \
> > -                     .storagebits = (bits),                  \
> > +                     .storagebits = (bits) > 16 ? 32 : 16,   \
> >                       .endianness = IIO_CPU,                  \
> >               },                                              \
> >  }
> > @@ -88,6 +88,8 @@ struct ad7606_chip_info {
> >   *                   such that it can be read via the 'read_avail' hook
> >   * @num_scales               number of elements stored in the scale_avail array
> >   * @range            voltage range selection, selects which scale to apply
> > + * @reg_offset               offset for the register value, to be applied when
> > + *                   writing the value of 'range' to the register value
> >   */
> >  struct ad7606_chan_scale {
> >  #define AD760X_MAX_SCALE_SHOW                (AD760X_MAX_CHANNELS * 2)
> > @@ -95,6 +97,7 @@ struct ad7606_chan_scale {
> >       int                             scale_avail_show[AD760X_MAX_SCALE_SHOW];
> >       unsigned int                    num_scales;
> >       unsigned int                    range;
> > +     unsigned int                    reg_offset;
> >  };
> >
> >  /**
> > @@ -151,9 +154,13 @@ struct ad7606_state {
> >       /*
> >        * DMA (thus cache coherency maintenance) may require the
> >        * transfer buffers to live in their own cache lines.
> > -      * 16 * 16-bit samples + 64-bit timestamp
> > +      * 16 * 16-bit samples + 64-bit timestamp - for AD7616
> > +      * 8 * 32-bit samples + 64-bit timestamp - for AD7616C-18 (and similar)
> >        */
> > -     unsigned short                  data[20] __aligned(IIO_DMA_MINALIGN);
> > +     union {
> > +             unsigned short d16[20];
> > +             unsigned int d32[10];
> > +     } data __aligned(IIO_DMA_MINALIGN);
> >       __be16                          d16[2];
> >  };
> >
> > @@ -192,6 +199,8 @@ enum ad7606_supported_device_ids {
> >       ID_AD7606_6,
> >       ID_AD7606_4,
> >       ID_AD7606B,
> > +     ID_AD7606C_16,
> > +     ID_AD7606C_18,
> >       ID_AD7616,
> >  };
> >
> > diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
> > index e00f58a6a0e9..b8d630ad156d 100644
> > --- a/drivers/iio/adc/ad7606_spi.c
> > +++ b/drivers/iio/adc/ad7606_spi.c
> > @@ -77,6 +77,18 @@ static const struct iio_chan_spec ad7606b_sw_channels[] = {
> >       AD7606_SW_CHANNEL(7, 16),
> >  };
> >
> > +static const struct iio_chan_spec ad7606c_18_sw_channels[] = {
> > +     IIO_CHAN_SOFT_TIMESTAMP(8),
> > +     AD7606_SW_CHANNEL(0, 18),
> > +     AD7606_SW_CHANNEL(1, 18),
> > +     AD7606_SW_CHANNEL(2, 18),
> > +     AD7606_SW_CHANNEL(3, 18),
> > +     AD7606_SW_CHANNEL(4, 18),
> > +     AD7606_SW_CHANNEL(5, 18),
> > +     AD7606_SW_CHANNEL(6, 18),
> > +     AD7606_SW_CHANNEL(7, 18),
> > +};
> > +
> >  static const unsigned int ad7606B_oversampling_avail[9] = {
> >       1, 2, 4, 8, 16, 32, 64, 128, 256
> >  };
> > @@ -120,6 +132,19 @@ static int ad7606_spi_read_block(struct device *dev,
> >       return 0;
> >  }
> >
> > +static int ad7606_spi_read_block18to32(struct device *dev,
> > +                                    int count, void *buf)
> > +{
> > +     struct spi_device *spi = to_spi_device(dev);
> > +     struct spi_transfer xfer = {
> > +             .bits_per_word = 18,
> > +             .len = count,
>
> Isn't count the number of words? .len needs to be the number
> of bytes, so 4 * count.

Oh.
Hmm, I need to check.
I assumed .len is the count of elements, not of bytes.

>
> > +             .rx_buf = buf,
> > +     };
> > +
> > +     return spi_sync_transfer(spi, &xfer, 1);
> > +}
> > +
> >  static int ad7606_spi_reg_read(struct ad7606_state *st, unsigned int addr)
> >  {
> >       struct spi_device *spi = to_spi_device(st->dev);
> > @@ -283,6 +308,19 @@ static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
> >       return 0;
> >  }
> >
> > +static int ad7606c_18_sw_mode_config(struct iio_dev *indio_dev)
> > +{
> > +     int ret;
> > +
> > +     ret = ad7606B_sw_mode_config(indio_dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     indio_dev->channels = ad7606c_18_sw_channels;
> > +
> > +     return 0;
> > +}
> > +
> >  static const struct ad7606_bus_ops ad7606_spi_bops = {
> >       .read_block = ad7606_spi_read_block,
> >  };
> > @@ -305,6 +343,15 @@ static const struct ad7606_bus_ops ad7606B_spi_bops = {
> >       .sw_mode_config = ad7606B_sw_mode_config,
> >  };
> >
> > +static const struct ad7606_bus_ops ad7606c_18_spi_bops = {
> > +     .read_block = ad7606_spi_read_block18to32,
> > +     .reg_read = ad7606_spi_reg_read,
> > +     .reg_write = ad7606_spi_reg_write,
> > +     .write_mask = ad7606_spi_write_mask,
> > +     .rd_wr_cmd = ad7606B_spi_rd_wr_cmd,
> > +     .sw_mode_config = ad7606c_18_sw_mode_config,
> > +};
> > +
> >  static int ad7606_spi_probe(struct spi_device *spi)
> >  {
> >       const struct spi_device_id *id = spi_get_device_id(spi);
> > @@ -315,8 +362,12 @@ static int ad7606_spi_probe(struct spi_device *spi)
> >               bops = &ad7616_spi_bops;
> >               break;
> >       case ID_AD7606B:
> > +     case ID_AD7606C_16:
> >               bops = &ad7606B_spi_bops;
> >               break;
> > +     case ID_AD7606C_18:
> > +             bops = &ad7606c_18_spi_bops;
> > +             break;
> >       default:
> >               bops = &ad7606_spi_bops;
> >               break;
> > @@ -333,6 +384,8 @@ static const struct spi_device_id ad7606_id_table[] = {
> >       { "ad7606-6", ID_AD7606_6 },
> >       { "ad7606-8", ID_AD7606_8 },
> >       { "ad7606b",  ID_AD7606B },
> > +     { "ad7606c-16",  ID_AD7606C_16 },
> > +     { "ad7606c-18",  ID_AD7606C_18 },
> >       { "ad7616",   ID_AD7616 },
> >       { }
> >  };
> > @@ -344,6 +397,8 @@ static const struct of_device_id ad7606_of_match[] = {
> >       { .compatible = "adi,ad7606-6" },
> >       { .compatible = "adi,ad7606-8" },
> >       { .compatible = "adi,ad7606b" },
> > +     { .compatible = "adi,ad7606c-16" },
> > +     { .compatible = "adi,ad7606c-18" },
> >       { .compatible = "adi,ad7616" },
> >       { }
> >  };
>

Re: [PATCH v4 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts

[David Lechner]

On 9/6/24 12:34 AM, Alexandru Ardelean wrote:
> On Fri, Sep 6, 2024 at 2:30 AM David Lechner <dlechner@baylibre.com> wrote:
>>
>> On 9/5/24 3:24 AM, Alexandru Ardelean wrote:


>>> -static int ad7606_read_samples(struct ad7606_state *st)
>>> +static int ad7606_read_samples(struct ad7606_state *st, bool sign_extend_samples)
>>>  {
>>> +     unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
>>
>> Why [1]? Sure, they are all the same, but [0] would seem less arbitrary.
> 
> [0] is the timestamp channel.

Oh, that's weird. First channel but last scan index!?


>>
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (storagebits == 16 || !sign_extend_samples)
>>> +             return 0;
>>> +
>>> +     /* For 18 bit samples, we need to sign-extend samples to 32 bits */
>>> +     for (i = 0; i < num; i++)
>>> +             data32[i] = sign_extend32(data32[i], 17);> +
>>> +     return 0;
>>>  }
>>>
>>>  static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>>> @@ -124,11 +176,11 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>>>
>>>       guard(mutex)(&st->lock);
>>>
>>> -     ret = ad7606_read_samples(st);
>>> +     ret = ad7606_read_samples(st, true);
>>
>> Shouldn't the sign_extend parameter depend on if the data is unipolar or bipolar?
> 
> [c1]
> Sign-extension is only needed for 18-bit samples.
> 16-bit samples are already properly sign(ed), but to 16-bits.
> 
> It's a slight performance improvement, that may look quirky here.
> The idea here, is that for ad7606_scan_direct() we only need to
> sign-extend 1 sample of the 8 samples we get.
> And we need to sign-extend it to 32 bits regardless of it being 16-bit
> or 18-bit.
> 
> In ad7606_trigger_handler(), the 16-bit samples were pushed as-is.
> Which means that we need to sign-extend the samples at least for
> 18-bits (as it is a new part)
> The question now becomes if we should sign-extend to 32-bits, 16-bit
> samples in ad7606_trigger_handler(), as that may break some ABI.
> 

Sign extension should not be needed at all for buffered reads (that is
what scan_type is for). So sign extension should only be needed for
the direct read when returning a raw value via sysfs (raw read).

Re: [PATCH v4 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts

[Alexandru Ardelean]

On Fri, Sep 6, 2024 at 4:33 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 9/6/24 12:34 AM, Alexandru Ardelean wrote:
> > On Fri, Sep 6, 2024 at 2:30 AM David Lechner <dlechner@baylibre.com> wrote:
> >>
> >> On 9/5/24 3:24 AM, Alexandru Ardelean wrote:
>
>
> >>> -static int ad7606_read_samples(struct ad7606_state *st)
> >>> +static int ad7606_read_samples(struct ad7606_state *st, bool sign_extend_samples)
> >>>  {
> >>> +     unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
> >>
> >> Why [1]? Sure, they are all the same, but [0] would seem less arbitrary.
> >
> > [0] is the timestamp channel.
>
> Oh, that's weird. First channel but last scan index!?

Yep
¯\_(ツ)_/¯

>
>
> >>
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>> +     if (storagebits == 16 || !sign_extend_samples)
> >>> +             return 0;
> >>> +
> >>> +     /* For 18 bit samples, we need to sign-extend samples to 32 bits */
> >>> +     for (i = 0; i < num; i++)
> >>> +             data32[i] = sign_extend32(data32[i], 17);> +
> >>> +     return 0;
> >>>  }
> >>>
> >>>  static irqreturn_t ad7606_trigger_handler(int irq, void *p)
> >>> @@ -124,11 +176,11 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
> >>>
> >>>       guard(mutex)(&st->lock);
> >>>
> >>> -     ret = ad7606_read_samples(st);
> >>> +     ret = ad7606_read_samples(st, true);
> >>
> >> Shouldn't the sign_extend parameter depend on if the data is unipolar or bipolar?
> >
> > [c1]
> > Sign-extension is only needed for 18-bit samples.
> > 16-bit samples are already properly sign(ed), but to 16-bits.
> >
> > It's a slight performance improvement, that may look quirky here.
> > The idea here, is that for ad7606_scan_direct() we only need to
> > sign-extend 1 sample of the 8 samples we get.
> > And we need to sign-extend it to 32 bits regardless of it being 16-bit
> > or 18-bit.
> >
> > In ad7606_trigger_handler(), the 16-bit samples were pushed as-is.
> > Which means that we need to sign-extend the samples at least for
> > 18-bits (as it is a new part)
> > The question now becomes if we should sign-extend to 32-bits, 16-bit
> > samples in ad7606_trigger_handler(), as that may break some ABI.
> >
>
> Sign extension should not be needed at all for buffered reads (that is
> what scan_type is for). So sign extension should only be needed for
> the direct read when returning a raw value via sysfs (raw read).

ack;
will remove it then from ad7606_read_samples()

Re: [PATCH v4 7/8] dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts

[Alexandru Ardelean]

On Fri, Sep 6, 2024 at 12:54 AM David Lechner <dlechner@baylibre.com> wrote:
>
> On 9/5/24 3:24 AM, Alexandru Ardelean wrote:
> > The driver will support the AD7606C-16 and AD7606C-18.
> > This change adds the compatible strings for these devices.
> >
> > The AD7606C-16,18 channels also support these (individually configurable)
> > types of channels:
> >  - bipolar single-ended
> >  - unipolar single-ended
> >  - bipolar differential
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > ---
> >  .../bindings/iio/adc/adi,ad7606.yaml          | 109 ++++++++++++++++++
> >  1 file changed, 109 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > index 69408cae3db9..57537ab0ec82 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > @@ -14,6 +14,8 @@ description: |
> >    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7605-4.pdf
> >    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606_7606-6_7606-4.pdf
> >    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7606B.pdf
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
> >    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7616.pdf
> >
> >  properties:
> > @@ -24,11 +26,19 @@ properties:
> >        - adi,ad7606-6
> >        - adi,ad7606-8  # Referred to as AD7606 (without -8) in the datasheet
> >        - adi,ad7606b
> > +      - adi,ad7606c-16
> > +      - adi,ad7606c-18
> >        - adi,ad7616
> >
> >    reg:
> >      maxItems: 1
> >
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> >    spi-cpha: true
> >
> >    spi-cpol: true
> > @@ -114,6 +124,46 @@ properties:
> >        assumed that the pins are hardwired to VDD.
> >      type: boolean
> >
> > +patternProperties:
> > +  "^channel@[1-8]$":
> > +    type: object
> > +    $ref: adc.yaml
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        description:
> > +          The channel number, as specified in the datasheet (from 1 to 8).
> > +        minimum: 1
> > +        maximum: 8
> > +
> > +      diff-channels:
> > +        description:
> > +          Each channel can be configured as a differential bipolar channel.
> > +          The ADC uses the same positive and negative inputs for this.
> > +          This property must be specified as 'reg' (or the channel number) for
> > +          both positive and negative inputs (i.e. diff-channels = <reg reg>).
> > +        items:
> > +          minimum: 1
> > +          maximum: 8
> > +
> > +      bipolar:
> > +        description:
> > +          Each channel can be configured as a unipolar or bipolar single-ended.
> > +          When this property is not specified, it's unipolar, so the ADC will
> > +          have only the positive input wired.
> > +          For this ADC the 'diff-channels' & 'bipolar' properties are mutually
> > +          exclusive.
> > +
> > +    required:
> > +      - reg
> > +
> > +    oneOf:
> > +      - required:
> > +          - diff-channels
> > +      - required:
> > +          - bipolar
>
> The datasheet (ad7606c-18.pdf) lists the following combinations:
>
> * Bipolar single-ended
> * Unipolar single-ended
> * Bipolar differential
>
> The logic in the oneOf: doesn't match this.
>
> This I think this would be sufficient:
>
> - if:
>     required: [diff-channels]
>   then:
>     required: [bipolar]

So here, I am a bit vague.
This makes 'bipolar' mandatory if 'diff-channels' is mandatory, right?
But then 'bipolar' (on its own) becomes optional?
The way I understood the oneOf case is that:
1. if it's 'diff-channels' then it's specified 'bipolar differential'.
2. if it's 'bipolar' then it's specified as 'bipolar single-ended'
3. otherwise it's unipolar
4. oneOf enforces that at least 'diff-channels' or 'bipolar' is
specified if there is a channel node


>
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -170,6 +220,17 @@ allOf:
> >          adi,conversion-start-gpios:
> >            maxItems: 1
> >
> > +  - if:
> > +      not:
> > +        properties:
> > +          compatible:
> > +            enum:
> > +              - adi,ad7606c-16
> > +              - adi,ad7606c-18
> > +    then:
> > +      patternProperties:
> > +        "^channel@[1-8]$": false
> > +
> >  unevaluatedProperties: false
> >
> >  examples:
> > @@ -202,4 +263,52 @@ examples:
> >              standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>;
> >          };
> >      };
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        adc@0 {
> > +            compatible = "adi,ad7606c-18";
> > +            reg = <0>;
> > +
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            spi-max-frequency = <1000000>;
> > +            spi-cpol;
> > +            spi-cpha;
> > +
> > +            avcc-supply = <&adc_vref>;
> > +            vdrive-supply = <&vdd_supply>;
> > +
> > +            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
> > +            interrupt-parent = <&gpio>;
> > +
> > +            adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>;
> > +            reset-gpios = <&gpio 27 GPIO_ACTIVE_HIGH>;
> > +            adi,first-data-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>;
> > +            standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>;
> > +
> > +            adi,sw-mode;
> > +
> > +            channel@1 {
> > +                reg = <1>;
> > +                diff-channels = <1 1>;
>
>                    bipolar;
>
> > +            };
> > +
> > +            channel@3 {
> > +                reg = <3>;
> > +                bipolar;
> > +            };
> > +
> > +            channel@8 {
> > +                reg = <8>;
> > +                diff-channels = <8 8>;
>
>                    bipolar;
>
> > +            };
> > +
> > +        };
> > +    };
> >  ...
>

Re: [PATCH v4 7/8] dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts

[Alexandru Ardelean]

On Fri, Sep 6, 2024 at 1:02 AM David Lechner <dlechner@baylibre.com> wrote:
>
> On 9/5/24 3:24 AM, Alexandru Ardelean wrote:
> > The driver will support the AD7606C-16 and AD7606C-18.
> > This change adds the compatible strings for these devices.
> >
> > The AD7606C-16,18 channels also support these (individually configurable)
> > types of channels:
> >  - bipolar single-ended
> >  - unipolar single-ended
> >  - bipolar differential
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > ---
>
> ...
>
> > @@ -170,6 +220,17 @@ allOf:
> >          adi,conversion-start-gpios:
> >            maxItems: 1
> >
> > +  - if:
> > +      not:
> > +        properties:
> > +          compatible:
> > +            enum:
> > +              - adi,ad7606c-16
> > +              - adi,ad7606c-18
> > +    then:
> > +      patternProperties:
> > +        "^channel@[1-8]$": false
> > +
>
> Technically, we should also have "^channel@[1-8]$": false
> if adi,sw-mode is not set (not: required: [adi,sw-mode]).

Yes.
That's true.
Maybe I'll add another condition here for that.

Re: [PATCH v4 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts

[Alexandru Ardelean]

On Fri, Sep 6, 2024 at 8:34 AM Alexandru Ardelean
<aardelean@baylibre.com> wrote:
>
> On Fri, Sep 6, 2024 at 2:30 AM David Lechner <dlechner@baylibre.com> wrote:
> >
> > On 9/5/24 3:24 AM, Alexandru Ardelean wrote:
> > > The AD7606C-16 and AD7606C-18 are pretty similar with the AD7606B.
> > > The main difference between AD7606C-16 & AD7606C-18 is the precision in
> > > bits (16 vs 18).
> > > Because of that, some scales need to be defined for the 18-bit variants, as
> > > they need to be computed against 2**18 (vs 2**16 for the 16 bit-variants).
> > >
> > > Because the AD7606C-16,18 also supports bipolar & differential channels,
> > > for SW-mode, the default range of 10 V or ±10V should be set at probe.
> > > On reset, the default range (in the registers) is set to value 0x3 which
> > > corresponds to '±10 V single-ended range', regardless of bipolar or
> > > differential configuration.
> > >
> > > Aside from the scale/ranges, the AD7606C-16 is similar to the AD7606B.
> > >
> > > The AD7606C-18 variant offers 18-bit precision. Because of this, the
> > > requirement to use this chip is that the SPI controller supports padding
> > > of 18-bit sequences to 32-bit arrays.
> > >
> > > Datasheet links:
> > >   https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
> > >   https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
> > >
> > > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > > ---
> > >  drivers/iio/adc/ad7606.c     | 266 +++++++++++++++++++++++++++++++----
> > >  drivers/iio/adc/ad7606.h     |  17 ++-
> > >  drivers/iio/adc/ad7606_spi.c |  55 ++++++++
> > >  3 files changed, 309 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> > > index 4c3fbb28f790..999c4411859e 100644
> > > --- a/drivers/iio/adc/ad7606.c
> > > +++ b/drivers/iio/adc/ad7606.c
> > > @@ -28,14 +28,44 @@
> > >
> > >  #include "ad7606.h"
> > >
> > > +typedef void (*ad7606c_chan_setup_cb_t)(struct ad7606_state *st, int ch,
> > > +                                     bool bipolar, bool differential);
> > > +
> > >  /*
> > >   * Scales are computed as 5000/32768 and 10000/32768 respectively,
> > >   * so that when applied to the raw values they provide mV values
> > >   */
> > > -static const unsigned int ad7606_scale_avail[2] = {
> > > +static const unsigned int ad7606_16bit_hw_scale_avail[2] = {
> > >       152588, 305176
> > >  };
> > >
> > > +static const unsigned int ad7606_18bit_hw_scale_avail[2] = {
> > > +     38147, 76294
> > > +};
> > > +
> > > +static const unsigned int ad7606c_16_scale_single_ended_unipolar_avail[3] = {
> > > +     76294, 152588, 190735,
> > > +};
> > > +
> > > +static const unsigned int ad7606c_16_scale_single_ended_bipolar_avail[5] = {
> > > +     76294, 152588, 190735, 305176, 381470
> > > +};
> > > +
> > > +static const unsigned int ad7606c_16_scale_differential_bipolar_avail[4] = {
> > > +     152588, 305176, 381470, 610352
> > > +};
> > > +
> > > +static const unsigned int ad7606c_18_scale_single_ended_unipolar_avail[3] = {
> > > +     19073, 38147, 47684
> > > +};
> > > +
> > > +static const unsigned int ad7606c_18_scale_single_ended_bipolar_avail[5] = {
> > > +     19073, 38147, 47684, 76294, 95367
> > > +};
> > > +
> > > +static const unsigned int ad7606c_18_scale_differential_bipolar_avail[4] = {
> > > +     38147, 76294, 95367, 152588
> > > +};
> > >
> > >  static const unsigned int ad7616_sw_scale_avail[3] = {
> > >       76293, 152588, 305176
> > > @@ -82,11 +112,19 @@ static int ad7606_reg_access(struct iio_dev *indio_dev,
> > >       }
> > >  }
> > >
> > > -static int ad7606_read_samples(struct ad7606_state *st)
> > > +static int ad7606_read_samples(struct ad7606_state *st, bool sign_extend_samples)
> > >  {
> > > +     unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
> >
> > Why [1]? Sure, they are all the same, but [0] would seem less arbitrary.
>
> [0] is the timestamp channel.
>
>
> >
> > >       unsigned int num = st->chip_info->num_channels - 1;
> > > -     u16 *data = st->data;
> > > -     int ret;
> > > +     u32 *data32 = st->data.d32;
> > > +     u16 *data16 = st->data.d16;
> > > +     void *data;
> > > +     int i, ret;
> > > +
> > > +     if (storagebits > 16)
> > > +             data = data32;
> > > +     else
> > > +             data = data16;
> > >
> > >       /*
> > >        * The frstdata signal is set to high while and after reading the sample
> > > @@ -108,11 +146,25 @@ static int ad7606_read_samples(struct ad7606_state *st)
> > >                       return -EIO;
> > >               }
> > >
> > > -             data++;
> > > +             if (storagebits > 16)
> > > +                     data32++;
> > > +             else
> > > +                     data16++;
> > >               num--;
> > >       }
> > >
> > > -     return st->bops->read_block(st->dev, num, data);
> > > +     ret = st->bops->read_block(st->dev, num, data);
> >
> > Since data++ was removed, this looks broken now as well as the
> > other read_block() not visible in the diff.
> >
> > Maybe better to drop data32 and data16, keep the change of data
> > to void*, and change data++ to data += BITS_TO_BYTES(storagebits)?
> >
> > Although, all of this might be moot since it looks like this
> > needs to be rebased on [1].
> >
> > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=fixes-togreg&id=90826e08468ba7fb35d8b39645b22d9e80004afe
>
> Oh.
> Omitted that patch.
> I forgot that fixes-togreg has a different cadence in another branch.
>
> >
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     if (storagebits == 16 || !sign_extend_samples)
> > > +             return 0;
> > > +
> > > +     /* For 18 bit samples, we need to sign-extend samples to 32 bits */
> > > +     for (i = 0; i < num; i++)
> > > +             data32[i] = sign_extend32(data32[i], 17);> +
> > > +     return 0;
> > >  }
> > >
> > >  static irqreturn_t ad7606_trigger_handler(int irq, void *p)
> > > @@ -124,11 +176,11 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
> > >
> > >       guard(mutex)(&st->lock);
> > >
> > > -     ret = ad7606_read_samples(st);
> > > +     ret = ad7606_read_samples(st, true);
> >
> > Shouldn't the sign_extend parameter depend on if the data is unipolar or bipolar?
>
> [c1]
> Sign-extension is only needed for 18-bit samples.
> 16-bit samples are already properly sign(ed), but to 16-bits.
>
> It's a slight performance improvement, that may look quirky here.
> The idea here, is that for ad7606_scan_direct() we only need to
> sign-extend 1 sample of the 8 samples we get.
> And we need to sign-extend it to 32 bits regardless of it being 16-bit
> or 18-bit.
>
> In ad7606_trigger_handler(), the 16-bit samples were pushed as-is.
> Which means that we need to sign-extend the samples at least for
> 18-bits (as it is a new part)
> The question now becomes if we should sign-extend to 32-bits, 16-bit
> samples in ad7606_trigger_handler(), as that may break some ABI.
>
> >
> > >       if (ret)
> > >               goto error_ret;
> > >
> > > -     iio_push_to_buffers_with_timestamp(indio_dev, st->data,
> > > +     iio_push_to_buffers_with_timestamp(indio_dev, st->data.d16,
> > >                                          iio_get_time_ns(indio_dev));
> > >  error_ret:
> > >       iio_trigger_notify_done(indio_dev->trig);
> > > @@ -142,6 +194,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> > >                             int *val)
> > >  {
> > >       struct ad7606_state *st = iio_priv(indio_dev);
> > > +     unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
> > >       int ret;
> > >
> > >       gpiod_set_value(st->gpio_convst, 1);
> > > @@ -152,9 +205,13 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> > >               goto error_ret;
> > >       }
> > >
> > > -     ret = ad7606_read_samples(st);
> > > -     if (ret == 0)
> > > -             *val = sign_extend32(st->data[ch], 15);
> > > +     ret = ad7606_read_samples(st, false);
> >
> > Why not let ad7606_read_samples() do the sign extending since
> > it can do that now?
>
> Related to comment [c1]
>
> >
> > > +     if (ret == 0) {
> > > +             if (storagebits > 16)
> > > +                     *val = sign_extend32(st->data.d32[ch], 17);
> > > +             else
> > > +                     *val = sign_extend32(st->data.d16[ch], 15);
> > > +     }
> > >
> > >  error_ret:
> > >       gpiod_set_value(st->gpio_convst, 0);
> > > @@ -267,7 +324,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> > >                       ch = chan->address;
> > >               cs = &st->chan_scales[ch];
> > >               i = find_closest(val2, cs->scale_avail, cs->num_scales);
> > > -             ret = st->write_scale(indio_dev, ch, i);
> > > +             ret = st->write_scale(indio_dev, ch, i + cs->reg_offset);
> > >               if (ret < 0)
> > >                       return ret;
> > >               cs->range = i;
> > > @@ -350,6 +407,18 @@ static const struct iio_chan_spec ad7606_channels_16bit[] = {
> > >       AD7606_CHANNEL(7, 16),
> > >  };
> > >
> > > +static const struct iio_chan_spec ad7606_channels_18bit[] = {
> > > +     IIO_CHAN_SOFT_TIMESTAMP(8),
> > > +     AD7606_CHANNEL(0, 18),
> > > +     AD7606_CHANNEL(1, 18),
> > > +     AD7606_CHANNEL(2, 18),
> > > +     AD7606_CHANNEL(3, 18),
> > > +     AD7606_CHANNEL(4, 18),
> > > +     AD7606_CHANNEL(5, 18),
> > > +     AD7606_CHANNEL(6, 18),
> > > +     AD7606_CHANNEL(7, 18),
> > > +};
> > > +
> > >  /*
> > >   * The current assumption that this driver makes for AD7616, is that it's
> > >   * working in Hardware Mode with Serial, Burst and Sequencer modes activated.
> > > @@ -410,6 +479,18 @@ static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
> > >               .oversampling_avail = ad7606_oversampling_avail,
> > >               .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
> > >       },
> > > +     [ID_AD7606C_16] = {
> > > +             .channels = ad7606_channels_16bit,
> > > +             .num_channels = 9,
> >
> > Could be nice to have a cleanup patch before this to convert others to
> > use ARRAY_SIZE(), then use ARRAY_SIZE(ad7606_channels_16bit) here
> > instead of 9.
>
> Ack.

I'll need to revert this to a NACK.

"ad7606_channels{_16bit}" has 9 elements.
But for some ADC variants, only 7 are used.
So, num_channels is specified as 7 there, and references this same array.
Which also explains why the timestamp channel is the first.

>
> >
> > > +             .oversampling_avail = ad7606_oversampling_avail,
> > > +             .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
> > > +     },
> > > +     [ID_AD7606C_18] = {
> > > +             .channels = ad7606_channels_18bit,
> > > +             .num_channels = 9,

Continuing here: I could make it ARRAY_SIZE(ad7606_channels_18bit),
but that would make it less consistent with the other "num_channels"
styles.
And maybe there will be a 4 or 6 channel 18 bit part later.

> > > +             .oversampling_avail = ad7606_oversampling_avail,
> > > +             .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
> > > +     },
> > >       [ID_AD7616] = {
> > >               .channels = ad7616_channels,
> > >               .num_channels = 17,
> > > @@ -581,7 +662,122 @@ static const struct iio_trigger_ops ad7606_trigger_ops = {
> > >       .validate_device = iio_trigger_validate_own_device,
> > >  };
> > >
> > > -static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
> > > +static void ad7606c_18_chan_setup(struct ad7606_state *st, int ch,
> > > +                               bool bipolar, bool differential)
> > > +{
> > > +     struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> > > +
> > > +     if (differential) {
> > > +             cs->scale_avail =
> > > +                     ad7606c_18_scale_differential_bipolar_avail;
> > > +             cs->num_scales =
> > > +                     ARRAY_SIZE(ad7606c_18_scale_differential_bipolar_avail);
> > > +             /* Bipolar differential ranges start at 8 (b1000) */
> > > +             cs->reg_offset = 8;
> > > +             cs->range = 1;
> > > +     } else if (bipolar) {
> > > +             cs->scale_avail =
> > > +                     ad7606c_18_scale_single_ended_bipolar_avail;
> > > +             cs->num_scales =
> > > +                     ARRAY_SIZE(ad7606c_18_scale_single_ended_bipolar_avail);
> >
> > I guess cs->reg_offset is 0 for this one?
>
> Yes.
> I will make it explicit.
>
> >
> > > +             cs->range = 3;
> > > +     } else {
> > > +             cs->scale_avail =
> > > +                     ad7606c_18_scale_single_ended_unipolar_avail;
> > > +             cs->num_scales =
> > > +                     ARRAY_SIZE(ad7606c_18_scale_single_ended_unipolar_avail);
> > > +             /* Unipolar single-ended ranges start at 5 (b0101) */
> > > +             cs->reg_offset = 5;
> > > +             cs->range = 1;
> > > +     }
> > > +}
> > > +
> > > +static void ad7606c_16_chan_setup(struct ad7606_state *st, int ch,
> > > +                               bool bipolar, bool differential)
> > > +{
> > > +     struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> > > +
> > > +     if (differential) {
> > > +             cs->scale_avail =
> > > +                     ad7606c_16_scale_differential_bipolar_avail;
> > > +             cs->num_scales =
> > > +                     ARRAY_SIZE(ad7606c_16_scale_differential_bipolar_avail);
> > > +             /* Bipolar differential ranges start at 8 (b1000) */
> > > +             cs->reg_offset = 8;
> > > +             cs->range = 1;
> > > +     } else if (bipolar) {
> > > +             cs->scale_avail =
> > > +                     ad7606c_16_scale_single_ended_bipolar_avail;
> > > +             cs->num_scales =
> > > +                     ARRAY_SIZE(ad7606c_16_scale_single_ended_bipolar_avail);
> > > +             cs->range = 3;
> > > +     } else {
> > > +             cs->scale_avail =
> > > +                     ad7606c_16_scale_single_ended_unipolar_avail;
> > > +             cs->num_scales =
> > > +                     ARRAY_SIZE(ad7606c_16_scale_single_ended_unipolar_avail);
> > > +             /* Unipolar single-ended ranges start at 5 (b0101) */
> > > +             cs->reg_offset = 5;
> > > +             cs->range = 1;
> > > +     }
> > > +}
> > > +
> > > +static int ad7606c_sw_mode_setup_channels(struct iio_dev *indio_dev,
> > > +                                       ad7606c_chan_setup_cb_t chan_setup_cb)
> > > +{
> > > +     unsigned int num_channels = indio_dev->num_channels - 1;
> > > +     struct ad7606_state *st = iio_priv(indio_dev);
> > > +     bool chan_configured[AD760X_MAX_CHANNELS] = {};
> > > +     struct device *dev = st->dev;
> > > +     int ret;
> > > +     u32 ch;
> > > +
> > > +     /* We need to hook this first */
> >
> > Comment would be more useful if it said why.
>
> Ack.
> Will add.
>
> >
> > > +     ret = st->bops->sw_mode_config(indio_dev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     device_for_each_child_node_scoped(dev, child) {
> > > +             bool bipolar, differential;
> > > +             u32 pins[2];
> > > +
> > > +             ret = fwnode_property_read_u32(child, "reg", &ch);
> > > +             if (ret)
> > > +                     continue;
> > > +
> > > +             /* channel number (here) is from 1 to num_channels */
> > > +             if (ch == 0 || ch > num_channels) {
> > > +                     dev_warn(st->dev,
> > > +                              "Invalid channel number (ignoring): %d\n", ch);
> > > +                     continue;
> > > +             }
> > > +
> > > +             bipolar = fwnode_property_present(child, "bipolar");
> >
> > IIRC, fwnode_property_read_bool() is preferred for bool/flag properties.
>
> Ack.
>
> >
> > > +
> > > +             ret = fwnode_property_read_u32_array(child, "diff-channels",
> > > +                                                  pins, ARRAY_SIZE(pins));
> > > +             /* Channel is differential, if pins are the same as 'reg' */
> > > +             if (ret == 0 && pins[0] == ch && pins[1] == ch)
> > > +                     differential = true;
> > > +             else
> > > +                     differential = false;
> >
> > Would probably better to error on bad pin numbers rather than default to
> > not differential.
>
> No strong preference from my side.
> Will implement failure/error case.
>
> >
> > > +
> > > +             ch--;
> > > +
> > > +             chan_setup_cb(st, ch, bipolar, differential);
> > > +             chan_configured[ch] = true;
> > > +     }
> > > +
> > > +     /* Apply default configuration to unconfigured (via DT) channels */
> > > +     for (ch = 0; ch < num_channels; ch++) {
> > > +             if (!chan_configured[ch])
> > > +                     chan_setup_cb(st, ch, false, false);
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int ad7606_sw_mode_setup(struct iio_dev *indio_dev, unsigned int id)
> > >  {
> > >       unsigned int num_channels = indio_dev->num_channels - 1;
> > >       struct ad7606_state *st = iio_priv(indio_dev);
> > > @@ -596,17 +792,30 @@ static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
> > >
> > >       indio_dev->info = &ad7606_info_sw_mode;
> > >
> > > -     /* Scale of 0.076293 is only available in sw mode */
> > > -     /* After reset, in software mode, ±10 V is set by default */
> > > -     for (ch = 0; ch < num_channels; ch++) {
> > > -             struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> > > +     switch (id) {
> > > +     case ID_AD7606C_18:
> > > +             ret = ad7606c_sw_mode_setup_channels(indio_dev,
> > > +                                                  ad7606c_18_chan_setup);
> > > +             break;
> > > +     case ID_AD7606C_16:
> > > +             ret = ad7606c_sw_mode_setup_channels(indio_dev,
> > > +                                                  ad7606c_16_chan_setup);
> > > +             break;
> > > +     default:
> > > +             /* Scale of 0.076293 is only available in sw mode */
> > > +             /* After reset, in software mode, ±10 V is set by default */
> > > +             for (ch = 0; ch < num_channels; ch++) {
> > > +                     struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> > > +
> > > +                     cs->scale_avail = ad7616_sw_scale_avail;
> > > +                     cs->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
> > > +                     cs->range = 2;
> > > +             }
> > >
> > > -             cs->scale_avail = ad7616_sw_scale_avail;
> > > -             cs->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
> > > -             cs->range = 2;
> > > +             ret = st->bops->sw_mode_config(indio_dev);
> > > +             break;
> > >       }
> > >
> > > -     ret = st->bops->sw_mode_config(indio_dev);
> > >       if (ret)
> > >               return ret;
> > >
> > > @@ -655,9 +864,16 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
> > >       st->oversampling = 1;
> > >
> > >       cs = &st->chan_scales[0];
> > > -     cs->range = 0;
> > > -     cs->scale_avail = ad7606_scale_avail;
> > > -     cs->num_scales = ARRAY_SIZE(ad7606_scale_avail);
> > > +     switch (id) {
> > > +     case ID_AD7606C_18:
> > > +             cs->scale_avail = ad7606_18bit_hw_scale_avail;
> > > +             cs->num_scales = ARRAY_SIZE(ad7606_18bit_hw_scale_avail);
> > > +             break;
> > > +     default:
> > > +             cs->scale_avail = ad7606_16bit_hw_scale_avail;
> > > +             cs->num_scales = ARRAY_SIZE(ad7606_16bit_hw_scale_avail);
> > > +             break;
> > > +     }
> > >
> > >       ret = devm_regulator_get_enable(dev, "avcc");
> > >       if (ret)
> > > @@ -706,7 +922,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
> > >       st->write_scale = ad7606_write_scale_hw;
> > >       st->write_os = ad7606_write_os_hw;
> > >
> > > -     ret = ad7606_sw_mode_setup(indio_dev);
> > > +     ret = ad7606_sw_mode_setup(indio_dev, id);
> > >       if (ret)
> > >               return ret;
> > >
> > > diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> > > index 2113ad460c0f..6b0897aa2dc7 100644
> > > --- a/drivers/iio/adc/ad7606.h
> > > +++ b/drivers/iio/adc/ad7606.h
> > > @@ -22,7 +22,7 @@
> > >               .scan_type = {                                  \
> > >                       .sign = 's',                            \
> > >                       .realbits = (bits),                     \
> > > -                     .storagebits = (bits),                  \
> > > +                     .storagebits = (bits) > 16 ? 32 : 16,   \
> > >                       .endianness = IIO_CPU,                  \
> > >               },                                              \
> > >  }
> > > @@ -45,7 +45,7 @@
> > >               .scan_type = {                                  \
> > >                       .sign = 's',                            \
> > >                       .realbits = (bits),                     \
> > > -                     .storagebits = (bits),                  \
> > > +                     .storagebits = (bits) > 16 ? 32 : 16,   \
> > >                       .endianness = IIO_CPU,                  \
> > >               },                                              \
> > >  }
> > > @@ -88,6 +88,8 @@ struct ad7606_chip_info {
> > >   *                   such that it can be read via the 'read_avail' hook
> > >   * @num_scales               number of elements stored in the scale_avail array
> > >   * @range            voltage range selection, selects which scale to apply
> > > + * @reg_offset               offset for the register value, to be applied when
> > > + *                   writing the value of 'range' to the register value
> > >   */
> > >  struct ad7606_chan_scale {
> > >  #define AD760X_MAX_SCALE_SHOW                (AD760X_MAX_CHANNELS * 2)
> > > @@ -95,6 +97,7 @@ struct ad7606_chan_scale {
> > >       int                             scale_avail_show[AD760X_MAX_SCALE_SHOW];
> > >       unsigned int                    num_scales;
> > >       unsigned int                    range;
> > > +     unsigned int                    reg_offset;
> > >  };
> > >
> > >  /**
> > > @@ -151,9 +154,13 @@ struct ad7606_state {
> > >       /*
> > >        * DMA (thus cache coherency maintenance) may require the
> > >        * transfer buffers to live in their own cache lines.
> > > -      * 16 * 16-bit samples + 64-bit timestamp
> > > +      * 16 * 16-bit samples + 64-bit timestamp - for AD7616
> > > +      * 8 * 32-bit samples + 64-bit timestamp - for AD7616C-18 (and similar)
> > >        */
> > > -     unsigned short                  data[20] __aligned(IIO_DMA_MINALIGN);
> > > +     union {
> > > +             unsigned short d16[20];
> > > +             unsigned int d32[10];
> > > +     } data __aligned(IIO_DMA_MINALIGN);
> > >       __be16                          d16[2];
> > >  };
> > >
> > > @@ -192,6 +199,8 @@ enum ad7606_supported_device_ids {
> > >       ID_AD7606_6,
> > >       ID_AD7606_4,
> > >       ID_AD7606B,
> > > +     ID_AD7606C_16,
> > > +     ID_AD7606C_18,
> > >       ID_AD7616,
> > >  };
> > >
> > > diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
> > > index e00f58a6a0e9..b8d630ad156d 100644
> > > --- a/drivers/iio/adc/ad7606_spi.c
> > > +++ b/drivers/iio/adc/ad7606_spi.c
> > > @@ -77,6 +77,18 @@ static const struct iio_chan_spec ad7606b_sw_channels[] = {
> > >       AD7606_SW_CHANNEL(7, 16),
> > >  };
> > >
> > > +static const struct iio_chan_spec ad7606c_18_sw_channels[] = {
> > > +     IIO_CHAN_SOFT_TIMESTAMP(8),
> > > +     AD7606_SW_CHANNEL(0, 18),
> > > +     AD7606_SW_CHANNEL(1, 18),
> > > +     AD7606_SW_CHANNEL(2, 18),
> > > +     AD7606_SW_CHANNEL(3, 18),
> > > +     AD7606_SW_CHANNEL(4, 18),
> > > +     AD7606_SW_CHANNEL(5, 18),
> > > +     AD7606_SW_CHANNEL(6, 18),
> > > +     AD7606_SW_CHANNEL(7, 18),
> > > +};
> > > +
> > >  static const unsigned int ad7606B_oversampling_avail[9] = {
> > >       1, 2, 4, 8, 16, 32, 64, 128, 256
> > >  };
> > > @@ -120,6 +132,19 @@ static int ad7606_spi_read_block(struct device *dev,
> > >       return 0;
> > >  }
> > >
> > > +static int ad7606_spi_read_block18to32(struct device *dev,
> > > +                                    int count, void *buf)
> > > +{
> > > +     struct spi_device *spi = to_spi_device(dev);
> > > +     struct spi_transfer xfer = {
> > > +             .bits_per_word = 18,
> > > +             .len = count,
> >
> > Isn't count the number of words? .len needs to be the number
> > of bytes, so 4 * count.
>
> Oh.
> Hmm, I need to check.
> I assumed .len is the count of elements, not of bytes.
>
> >
> > > +             .rx_buf = buf,
> > > +     };
> > > +
> > > +     return spi_sync_transfer(spi, &xfer, 1);
> > > +}
> > > +
> > >  static int ad7606_spi_reg_read(struct ad7606_state *st, unsigned int addr)
> > >  {
> > >       struct spi_device *spi = to_spi_device(st->dev);
> > > @@ -283,6 +308,19 @@ static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
> > >       return 0;
> > >  }
> > >
> > > +static int ad7606c_18_sw_mode_config(struct iio_dev *indio_dev)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = ad7606B_sw_mode_config(indio_dev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     indio_dev->channels = ad7606c_18_sw_channels;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  static const struct ad7606_bus_ops ad7606_spi_bops = {
> > >       .read_block = ad7606_spi_read_block,
> > >  };
> > > @@ -305,6 +343,15 @@ static const struct ad7606_bus_ops ad7606B_spi_bops = {
> > >       .sw_mode_config = ad7606B_sw_mode_config,
> > >  };
> > >
> > > +static const struct ad7606_bus_ops ad7606c_18_spi_bops = {
> > > +     .read_block = ad7606_spi_read_block18to32,
> > > +     .reg_read = ad7606_spi_reg_read,
> > > +     .reg_write = ad7606_spi_reg_write,
> > > +     .write_mask = ad7606_spi_write_mask,
> > > +     .rd_wr_cmd = ad7606B_spi_rd_wr_cmd,
> > > +     .sw_mode_config = ad7606c_18_sw_mode_config,
> > > +};
> > > +
> > >  static int ad7606_spi_probe(struct spi_device *spi)
> > >  {
> > >       const struct spi_device_id *id = spi_get_device_id(spi);
> > > @@ -315,8 +362,12 @@ static int ad7606_spi_probe(struct spi_device *spi)
> > >               bops = &ad7616_spi_bops;
> > >               break;
> > >       case ID_AD7606B:
> > > +     case ID_AD7606C_16:
> > >               bops = &ad7606B_spi_bops;
> > >               break;
> > > +     case ID_AD7606C_18:
> > > +             bops = &ad7606c_18_spi_bops;
> > > +             break;
> > >       default:
> > >               bops = &ad7606_spi_bops;
> > >               break;
> > > @@ -333,6 +384,8 @@ static const struct spi_device_id ad7606_id_table[] = {
> > >       { "ad7606-6", ID_AD7606_6 },
> > >       { "ad7606-8", ID_AD7606_8 },
> > >       { "ad7606b",  ID_AD7606B },
> > > +     { "ad7606c-16",  ID_AD7606C_16 },
> > > +     { "ad7606c-18",  ID_AD7606C_18 },
> > >       { "ad7616",   ID_AD7616 },
> > >       { }
> > >  };
> > > @@ -344,6 +397,8 @@ static const struct of_device_id ad7606_of_match[] = {
> > >       { .compatible = "adi,ad7606-6" },
> > >       { .compatible = "adi,ad7606-8" },
> > >       { .compatible = "adi,ad7606b" },
> > > +     { .compatible = "adi,ad7606c-16" },
> > > +     { .compatible = "adi,ad7606c-18" },
> > >       { .compatible = "adi,ad7616" },
> > >       { }
> > >  };
> >

Re: [PATCH v4 7/8] dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts

[David Lechner]

On 9/6/24 11:59 PM, Alexandru Ardelean wrote:
> On Fri, Sep 6, 2024 at 12:54 AM David Lechner <dlechner@baylibre.com> wrote:
>>
>> On 9/5/24 3:24 AM, Alexandru Ardelean wrote:

...

>>> +patternProperties:
>>> +  "^channel@[1-8]$":
>>> +    type: object
>>> +    $ref: adc.yaml
>>> +    unevaluatedProperties: false
>>> +
>>> +    properties:
>>> +      reg:
>>> +        description:
>>> +          The channel number, as specified in the datasheet (from 1 to 8).
>>> +        minimum: 1
>>> +        maximum: 8
>>> +
>>> +      diff-channels:
>>> +        description:
>>> +          Each channel can be configured as a differential bipolar channel.
>>> +          The ADC uses the same positive and negative inputs for this.
>>> +          This property must be specified as 'reg' (or the channel number) for
>>> +          both positive and negative inputs (i.e. diff-channels = <reg reg>).
>>> +        items:
>>> +          minimum: 1
>>> +          maximum: 8
>>> +
>>> +      bipolar:
>>> +        description:
>>> +          Each channel can be configured as a unipolar or bipolar single-ended.
>>> +          When this property is not specified, it's unipolar, so the ADC will
>>> +          have only the positive input wired.
>>> +          For this ADC the 'diff-channels' & 'bipolar' properties are mutually
>>> +          exclusive.
>>> +
>>> +    required:
>>> +      - reg
>>> +
>>> +    oneOf:
>>> +      - required:
>>> +          - diff-channels
>>> +      - required:
>>> +          - bipolar
>>
>> The datasheet (ad7606c-18.pdf) lists the following combinations:
>>
>> * Bipolar single-ended
>> * Unipolar single-ended
>> * Bipolar differential
>>
>> The logic in the oneOf: doesn't match this.
>>
>> This I think this would be sufficient:
>>
>> - if:
>>     required: [diff-channels]
>>   then:
>>     required: [bipolar]
> 
> So here, I am a bit vague.
> This makes 'bipolar' mandatory if 'diff-channels' is mandatory, right?
> But then 'bipolar' (on its own) becomes optional?
> The way I understood the oneOf case is that:
> 1. if it's 'diff-channels' then it's specified 'bipolar differential'.

diff-channels does not imply bipolar in DT, so we need both properties
set to specify "bipolar differential".

> 2. if it's 'bipolar' then it's specified as 'bipolar single-ended'
> 3. otherwise it's unipolar
> 4. oneOf enforces that at least 'diff-channels' or 'bipolar' is
> specified if there is a channel node
> 
> 

Re: [PATCH v4 7/8] dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts

[Alexandru Ardelean]

On Sat, Sep 7, 2024 at 5:09 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 9/6/24 11:59 PM, Alexandru Ardelean wrote:
> > On Fri, Sep 6, 2024 at 12:54 AM David Lechner <dlechner@baylibre.com> wrote:
> >>
> >> On 9/5/24 3:24 AM, Alexandru Ardelean wrote:
>
> ...
>
> >>> +patternProperties:
> >>> +  "^channel@[1-8]$":
> >>> +    type: object
> >>> +    $ref: adc.yaml
> >>> +    unevaluatedProperties: false
> >>> +
> >>> +    properties:
> >>> +      reg:
> >>> +        description:
> >>> +          The channel number, as specified in the datasheet (from 1 to 8).
> >>> +        minimum: 1
> >>> +        maximum: 8
> >>> +
> >>> +      diff-channels:
> >>> +        description:
> >>> +          Each channel can be configured as a differential bipolar channel.
> >>> +          The ADC uses the same positive and negative inputs for this.
> >>> +          This property must be specified as 'reg' (or the channel number) for
> >>> +          both positive and negative inputs (i.e. diff-channels = <reg reg>).
> >>> +        items:
> >>> +          minimum: 1
> >>> +          maximum: 8
> >>> +
> >>> +      bipolar:
> >>> +        description:
> >>> +          Each channel can be configured as a unipolar or bipolar single-ended.
> >>> +          When this property is not specified, it's unipolar, so the ADC will
> >>> +          have only the positive input wired.
> >>> +          For this ADC the 'diff-channels' & 'bipolar' properties are mutually
> >>> +          exclusive.
> >>> +
> >>> +    required:
> >>> +      - reg
> >>> +
> >>> +    oneOf:
> >>> +      - required:
> >>> +          - diff-channels
> >>> +      - required:
> >>> +          - bipolar
> >>
> >> The datasheet (ad7606c-18.pdf) lists the following combinations:
> >>
> >> * Bipolar single-ended
> >> * Unipolar single-ended
> >> * Bipolar differential
> >>
> >> The logic in the oneOf: doesn't match this.
> >>
> >> This I think this would be sufficient:
> >>
> >> - if:
> >>     required: [diff-channels]
> >>   then:
> >>     required: [bipolar]
> >
> > So here, I am a bit vague.
> > This makes 'bipolar' mandatory if 'diff-channels' is mandatory, right?
> > But then 'bipolar' (on its own) becomes optional?
> > The way I understood the oneOf case is that:
> > 1. if it's 'diff-channels' then it's specified 'bipolar differential'.
>
> diff-channels does not imply bipolar in DT, so we need both properties
> set to specify "bipolar differential".
>

ack
will update then

> > 2. if it's 'bipolar' then it's specified as 'bipolar single-ended'
> > 3. otherwise it's unipolar
> > 4. oneOf enforces that at least 'diff-channels' or 'bipolar' is
> > specified if there is a channel node
> >
> >