[PATCH 0/6] iio: adc: ad7606: add support for AD760{7,8,9} parts

[PATCH 0/6] iio: adc: ad7606: add support for AD760{7,8,9} parts

[Alexandru Ardelean]

This change-set adds support for AD7607, AD7608 and AD7609.
These parts are simpler parts for the AD7606x family. They support only
HW-only mode like AD7605, but they support oversampling like the other
AD7606x parts.

AD7607 has a 14-bit resolution.
AD7608 and AD7609 are 18-bit resolution.
AD7607 & AD7608 supports +/-5V and +/-10V ranges.
AD7609 supports +/-10V & +/-20V ranges.

The oversampling settings are the same.
Because of AD7607, the scales had to be reworked (again), but this time
doing away with the allocation at runtime for the scale-available-show
values. This time, the full IIO_VAL_INT_PLUS_MICRO values are stored
statically. AD7607 supports a scale of 1.220703, which is the first and
only (so far) scale that is above 1.

This patchset contains a few small bug-fixes found during more testing of
these parts.

Alexandru Ardelean (6):
  iio: adc: ad7606: fix/persist oversampling_ratio setting
  iio: adc: ad7606: fix issue/quirk with find_closest() for oversampling
  iio: adc: ad7606: use realbits for sign-extending in scan_direct
  iio: adc: ad7606: rework scale-available to be static
  dt-bindings: iio: adc: adi,ad7606: document AD760{7,8,9} parts
  iio: adc: ad7606: add support for AD760{7,8,9} parts

 .../bindings/iio/adc/adi,ad7606.yaml          |   9 +
 drivers/iio/adc/ad7606.c                      | 242 +++++++++++++-----
 drivers/iio/adc/ad7606.h                      |   9 +-
 drivers/iio/adc/ad7606_par.c                  |   6 +
 drivers/iio/adc/ad7606_spi.c                  |  42 +++
 5 files changed, 241 insertions(+), 67 deletions(-)

-- 
2.46.1

[PATCH 1/6] iio: adc: ad7606: fix/persist oversampling_ratio setting

[Alexandru Ardelean]

When the mutexes were reworked to guards, the caching of the
oversampling_ratio values was removed by accident.

The main effect of this change is that, after setting the
oversampling_ratio value, reading it back would result in the initial value
(of 1).
The value would get sent to the device correctly though.

Fixes 2956979dbd0d: ("iio: adc: ad7606: switch mutexes to guard")
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 0e830a17fc19..ae49f4ba50d9 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -753,6 +753,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 		ret = st->write_os(indio_dev, i);
 		if (ret < 0)
 			return ret;
+		st->oversampling = st->oversampling_avail[i];
 
 		return 0;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-- 
2.46.1

[PATCH 2/6] iio: adc: ad7606: fix issue/quirk with find_closest() for oversampling

[Alexandru Ardelean]

There's a small issue with setting oversampling-ratio that seems to have
been there since the driver was in staging.
Trying to set an oversampling value of '2' will set an oversampling value
of '1'. This is because find_closest() does an average + rounding of 1 + 2,
and we get '1'.

This is the only issue with find_closest(), at least in this setup. The
other values (above 2) work reasonably well. Setting 3, rounds to 2, so a
quick fix is to round 'val' to 3 (if userspace provides 2).

Fixes 41f71e5e7daf: ("staging: iio: adc: ad7606: Use find_closest() macro")
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index ae49f4ba50d9..d0fe9fd65f3f 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -748,6 +748,9 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		if (val2)
 			return -EINVAL;
+		/* Minor trick, so that OS of 2, doesn't get rounded to 1 */
+		if (val == 2)
+			val++;
 		i = find_closest(val, st->oversampling_avail,
 				 st->num_os_ratios);
 		ret = st->write_os(indio_dev, i);
-- 
2.46.1

[PATCH 3/6] iio: adc: ad7606: use realbits for sign-extending in scan_direct

[Alexandru Ardelean]

Currently the 'ad7606' driver supports parts with 18 and 16 bits
resolutions.
But when adding support for AD7607 (which has a 14-bit resolution) we
should check for the 'realbits' field, to be able to sign-extend correctly.

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

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index d0fe9fd65f3f..a1f0c2feb04a 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -568,7 +568,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;
+	unsigned int realbits = st->chip_info->channels[1].scan_type.realbits;
 	const struct iio_chan_spec *chan;
 	int ret;
 
@@ -603,15 +603,29 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
 
 	chan = &indio_dev->channels[ch + 1];
 	if (chan->scan_type.sign == 'u') {
-		if (storagebits > 16)
+		switch (realbits) {
+		case 18:
 			*val = st->data.buf32[ch];
-		else
+			break;
+		case 16:
 			*val = st->data.buf16[ch];
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
 	} else {
-		if (storagebits > 16)
+		switch (realbits) {
+		case 18:
 			*val = sign_extend32(st->data.buf32[ch], 17);
-		else
+			break;
+		case 16:
 			*val = sign_extend32(st->data.buf16[ch], 15);
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
 	}
 
 error_ret:
-- 
2.46.1

[PATCH 4/6] iio: adc: ad7606: rework scale-available to be static

[Alexandru Ardelean]

The main driver for this change is the AD7607 part, which has a scale of
"1.220703" for the ±10V range. The AD7607 has a resolution of 14-bits.

So, just adding the scale-available list for that part would require some
quirks to handle just that scale value.
But to do it more neatly, the best approach is to rework the scale
available lists to have the same format as it is returned to userspace.
That way, we can also get rid of the allocation for the 'scale_avail_show'
array.

Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 106 ++++++++++++++++++---------------------
 drivers/iio/adc/ad7606.h |   6 +--
 2 files changed, 50 insertions(+), 62 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index a1f0c2feb04a..115c27ae02f3 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -33,42 +33,44 @@
 
 /*
  * Scales are computed as 5000/32768 and 10000/32768 respectively,
- * so that when applied to the raw values they provide mV values
+ * so that when applied to the raw values they provide mV values.
+ * The scale arrays are kept as IIO_VAL_INT_PLUS_MICRO, so index
+ * X is the integer part and X + 1 is the fractional part.
  */
-static const unsigned int ad7606_16bit_hw_scale_avail[2] = {
-	152588, 305176
+static const unsigned int ad7606_16bit_hw_scale_avail[2][2] = {
+	{ 0, 152588 }, { 0, 305176 }
 };
 
-static const unsigned int ad7606_18bit_hw_scale_avail[2] = {
-	38147, 76294
+static const unsigned int ad7606_18bit_hw_scale_avail[2][2] = {
+	{ 0, 38147 }, { 0, 76294 }
 };
 
-static const unsigned int ad7606c_16bit_single_ended_unipolar_scale_avail[3] = {
-	76294, 152588, 190735,
+static const unsigned int ad7606c_16bit_single_ended_unipolar_scale_avail[3][2] = {
+	{ 0, 76294 }, { 0, 152588 }, { 0, 190735 }
 };
 
-static const unsigned int ad7606c_16bit_single_ended_bipolar_scale_avail[5] = {
-	76294, 152588, 190735, 305176, 381470
+static const unsigned int ad7606c_16bit_single_ended_bipolar_scale_avail[5][2] = {
+	{ 0, 76294 }, { 0, 152588 }, { 0, 190735 }, { 0, 305176 }, { 0, 381470 }
 };
 
-static const unsigned int ad7606c_16bit_differential_bipolar_scale_avail[4] = {
-	152588, 305176, 381470, 610352
+static const unsigned int ad7606c_16bit_differential_bipolar_scale_avail[4][2] = {
+	{ 0, 152588 }, { 0, 305176 }, { 0, 381470 }, { 0, 610352 }
 };
 
-static const unsigned int ad7606c_18bit_single_ended_unipolar_scale_avail[3] = {
-	19073, 38147, 47684
+static const unsigned int ad7606c_18bit_single_ended_unipolar_scale_avail[3][2] = {
+	{ 0, 19073 }, { 0, 38147 }, { 0, 47684 }
 };
 
-static const unsigned int ad7606c_18bit_single_ended_bipolar_scale_avail[5] = {
-	19073, 38147, 47684, 76294, 95367
+static const unsigned int ad7606c_18bit_single_ended_bipolar_scale_avail[5][2] = {
+	{ 0, 19073 }, { 0, 38147 }, { 0, 47684 }, { 0, 76294 }, { 0, 95367 }
 };
 
-static const unsigned int ad7606c_18bit_differential_bipolar_scale_avail[4] = {
-	38147, 76294, 95367, 152588
+static const unsigned int ad7606c_18bit_differential_bipolar_scale_avail[4][2] = {
+	{ 0, 38147 }, { 0, 76294 }, { 0, 95367 }, { 0, 152588 }
 };
 
-static const unsigned int ad7606_16bit_sw_scale_avail[3] = {
-	76293, 152588, 305176
+static const unsigned int ad7606_16bit_sw_scale_avail[3][2] = {
+	{ 0, 76293 }, { 0, 152588 }, { 0, 305176 }
 };
 
 static const unsigned int ad7606_oversampling_avail[7] = {
@@ -663,8 +665,8 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 		if (st->sw_mode_en)
 			ch = chan->address;
 		cs = &st->chan_scales[ch];
-		*val = 0;
-		*val2 = cs->scale_avail[cs->range];
+		*val = cs->scale_avail[cs->range][0];
+		*val2 = cs->scale_avail[cs->range][1];
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		*val = st->oversampling;
@@ -681,21 +683,6 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
-static ssize_t ad7606_show_avail(char *buf, const unsigned int *vals,
-				 unsigned int n, bool micros)
-{
-	size_t len = 0;
-	int i;
-
-	for (i = 0; i < n; i++) {
-		len += scnprintf(buf + len, PAGE_SIZE - len,
-			micros ? "0.%06u " : "%u ", vals[i]);
-	}
-	buf[len - 1] = '\n';
-
-	return len;
-}
-
 static ssize_t in_voltage_scale_available_show(struct device *dev,
 					       struct device_attribute *attr,
 					       char *buf)
@@ -703,8 +690,16 @@ 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];
+	const unsigned int (*vals)[2] = cs->scale_avail;
+	unsigned int i;
+	size_t len = 0;
 
-	return ad7606_show_avail(buf, cs->scale_avail, cs->num_scales, true);
+	for (i = 0; i < cs->num_scales; i++)
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ",
+				 vals[i][0], vals[i][1]);
+	buf[len - 1] = '\n';
+
+	return len;
 }
 
 static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);
@@ -742,6 +737,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 			    long mask)
 {
 	struct ad7606_state *st = iio_priv(indio_dev);
+	unsigned int scale_avail[AD760X_MAX_SCALES];
 	struct ad7606_chan_scale *cs;
 	int i, ret, ch = 0;
 
@@ -752,7 +748,12 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 		if (st->sw_mode_en)
 			ch = chan->address;
 		cs = &st->chan_scales[ch];
-		i = find_closest(val2, cs->scale_avail, cs->num_scales);
+		for (i = 0; i < cs->num_scales; i++) {
+			scale_avail[i] = cs->scale_avail[i][0] * 1000000 +
+					 cs->scale_avail[i][1];
+		}
+		val = val * 1000000 + val2;
+		i = find_closest(val, scale_avail, cs->num_scales);
 		ret = st->write_scale(indio_dev, ch, i + cs->reg_offset);
 		if (ret < 0)
 			return ret;
@@ -788,9 +789,15 @@ static ssize_t ad7606_oversampling_ratio_avail(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7606_state *st = iio_priv(indio_dev);
+	const unsigned int *vals = st->oversampling_avail;
+	unsigned int i;
+	size_t len = 0;
 
-	return ad7606_show_avail(buf, st->oversampling_avail,
-				 st->num_os_ratios, false);
+	for (i = 0; i < st->num_os_ratios; i++)
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%u ", vals[i]);
+	buf[len - 1] = '\n';
+
+	return len;
 }
 
 static IIO_DEVICE_ATTR(oversampling_ratio_available, 0444,
@@ -944,8 +951,8 @@ static int ad7606_read_avail(struct iio_dev *indio_dev,
 			ch = chan->address;
 
 		cs = &st->chan_scales[ch];
-		*vals = cs->scale_avail_show;
-		*length = cs->num_scales * 2;
+		*vals = (int *)cs->scale_avail;
+		*length = cs->num_scales;
 		*type = IIO_VAL_INT_PLUS_MICRO;
 
 		return IIO_AVAIL_LIST;
@@ -1068,24 +1075,9 @@ static int ad7606_chan_scales_setup(struct iio_dev *indio_dev)
 	indio_dev->channels = chans;
 
 	for (ch = 0; ch < num_channels; ch++) {
-		struct ad7606_chan_scale *cs;
-		int i;
-
 		ret = st->chip_info->scale_setup_cb(st, &chans[ch + 1], ch);
 		if (ret)
 			return ret;
-
-		cs = &st->chan_scales[ch];
-
-		if (cs->num_scales * 2 > AD760X_MAX_SCALE_SHOW)
-			return dev_err_probe(st->dev, -ERANGE,
-					"Driver error: scale range too big");
-
-		/* 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;
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 2c629a15cc33..32c6f776c5df 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -103,8 +103,6 @@ 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
  * @reg_offset		offset for the register value, to be applied when
@@ -112,9 +110,7 @@ struct ad7606_chip_info {
  */
 struct ad7606_chan_scale {
 #define AD760X_MAX_SCALES		16
-#define AD760X_MAX_SCALE_SHOW		(AD760X_MAX_SCALES * 2)
-	const unsigned int		*scale_avail;
-	int				scale_avail_show[AD760X_MAX_SCALE_SHOW];
+	const unsigned int		(*scale_avail)[2];
 	unsigned int			num_scales;
 	unsigned int			range;
 	unsigned int			reg_offset;
-- 
2.46.1

[PATCH 5/6] dt-bindings: iio: adc: adi,ad7606: document AD760{7,8,9} parts

[Alexandru Ardelean]

This change adds the AD7607, AD7608 & AD7609 parts to the binding doc of
the ad7606 driver.

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

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
index 0a612844029a..ab5881d0d017 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
@@ -16,6 +16,9 @@ description: |
   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/ad7607.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7608.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7609.pdf
   https://www.analog.com/media/en/technical-documentation/data-sheets/AD7616.pdf
 
 properties:
@@ -28,6 +31,9 @@ properties:
       - adi,ad7606b
       - adi,ad7606c-16
       - adi,ad7606c-18
+      - adi,ad7607
+      - adi,ad7608
+      - adi,ad7609
       - adi,ad7616
 
   reg:
@@ -250,6 +256,9 @@ allOf:
               - adi,ad7606-4
               - adi,ad7606-6
               - adi,ad7606-8
+              - adi,ad7607
+              - adi,ad7608
+              - adi,ad7609
     then:
       properties:
         adi,sw-mode: false
-- 
2.46.1

[PATCH 6/6] iio: adc: ad7606: add support for AD760{7,8,9} parts

[Alexandru Ardelean]

The AD7607, AD7608 and AD7609 are some older parts of the AD7606 family.
They are hardware-only, meaning that they don't have any registers
accessible via SPI or Parallel interface.
They are more similar to the AD7605-4 part, which is supported by the
'ad7606' driver, and are configurable via GPIOs.

Like the AD7605-4 part, all 3 parts have 2 CONVST (Conversion Start) pins
(CONVST A and CONVST B). But in practice, these should be tied together to
make reading of samples easier via a serial line.

The AD7607 has an 14-bit resolution and AD7608 & AD7609 have an 18-bit
resolution. The main difference between the AD7608 & AD7609 is that the
AD7609 has a larger range (±10V & ±20V) vs the ±5V & ±10V ranges for AD7608.

However, unlike AD7605-4 part, these 3 parts have oversampling which is
configurable (like for the AD7606 in HW-mode) via GPIOs.

Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad7607.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad7608.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad7609.pdf

Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c     | 108 +++++++++++++++++++++++++++++++++++
 drivers/iio/adc/ad7606.h     |   3 +
 drivers/iio/adc/ad7606_par.c |   6 ++
 drivers/iio/adc/ad7606_spi.c |  42 ++++++++++++++
 4 files changed, 159 insertions(+)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 115c27ae02f3..4204b91da7db 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -73,6 +73,14 @@ static const unsigned int ad7606_16bit_sw_scale_avail[3][2] = {
 	{ 0, 76293 }, { 0, 152588 }, { 0, 305176 }
 };
 
+static const unsigned int ad7607_hw_scale_avail[2][2] = {
+	{ 0, 610352 }, { 1, 220703 }
+};
+
+static const unsigned int ad7609_hw_scale_avail[2][2] = {
+	{ 0, 152588 }, { 0, 305176 }
+};
+
 static const unsigned int ad7606_oversampling_avail[7] = {
 	1, 2, 4, 8, 16, 32, 64,
 };
@@ -113,6 +121,30 @@ static const struct iio_chan_spec ad7606_channels_18bit[] = {
 	AD7606_CHANNEL(7, 18),
 };
 
+static const struct iio_chan_spec ad7607_channels[] = {
+	IIO_CHAN_SOFT_TIMESTAMP(8),
+	AD7606_CHANNEL(0, 14),
+	AD7606_CHANNEL(1, 14),
+	AD7606_CHANNEL(2, 14),
+	AD7606_CHANNEL(3, 14),
+	AD7606_CHANNEL(4, 14),
+	AD7606_CHANNEL(5, 14),
+	AD7606_CHANNEL(6, 14),
+	AD7606_CHANNEL(7, 14),
+};
+
+static const struct iio_chan_spec ad7608_channels[] = {
+	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.
@@ -149,6 +181,12 @@ static int ad7606c_16bit_chan_scale_setup(struct ad7606_state *st,
 					  struct iio_chan_spec *chan, int ch);
 static int ad7606_16bit_chan_scale_setup(struct ad7606_state *st,
 					 struct iio_chan_spec *chan, int ch);
+static int ad7607_chan_scale_setup(struct ad7606_state *st,
+				   struct iio_chan_spec *chan, int ch);
+static int ad7608_chan_scale_setup(struct ad7606_state *st,
+				   struct iio_chan_spec *chan, int ch);
+static int ad7609_chan_scale_setup(struct ad7606_state *st,
+				   struct iio_chan_spec *chan, int ch);
 
 const struct ad7606_chip_info ad7605_4_info = {
 	.channels = ad7605_channels,
@@ -215,6 +253,39 @@ const struct ad7606_chip_info ad7606c_16_info = {
 };
 EXPORT_SYMBOL_NS_GPL(ad7606c_16_info, IIO_AD7606);
 
+const struct ad7606_chip_info ad7607_info = {
+	.channels = ad7607_channels,
+	.name = "ad7607",
+	.num_adc_channels = 8,
+	.num_channels = 9,
+	.oversampling_avail = ad7606_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	.scale_setup_cb = ad7607_chan_scale_setup,
+};
+EXPORT_SYMBOL_NS_GPL(ad7607_info, IIO_AD7606);
+
+const struct ad7606_chip_info ad7608_info = {
+	.channels = ad7608_channels,
+	.name = "ad7608",
+	.num_adc_channels = 8,
+	.num_channels = 9,
+	.oversampling_avail = ad7606_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	.scale_setup_cb = ad7608_chan_scale_setup,
+};
+EXPORT_SYMBOL_NS_GPL(ad7608_info, IIO_AD7606);
+
+const struct ad7606_chip_info ad7609_info = {
+	.channels = ad7608_channels,
+	.name = "ad7609",
+	.num_adc_channels = 8,
+	.num_channels = 9,
+	.oversampling_avail = ad7606_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	.scale_setup_cb = ad7609_chan_scale_setup,
+};
+EXPORT_SYMBOL_NS_GPL(ad7609_info, IIO_AD7606);
+
 const struct ad7606_chip_info ad7606c_18_info = {
 	.channels = ad7606_channels_18bit,
 	.name = "ad7606c18",
@@ -441,6 +512,39 @@ static int ad7606c_16bit_chan_scale_setup(struct ad7606_state *st,
 	return 0;
 }
 
+static int ad7607_chan_scale_setup(struct ad7606_state *st,
+				   struct iio_chan_spec *chan, int ch)
+{
+	struct ad7606_chan_scale *cs = &st->chan_scales[ch];
+
+	cs->range = 0;
+	cs->scale_avail = ad7607_hw_scale_avail;
+	cs->num_scales = ARRAY_SIZE(ad7607_hw_scale_avail);
+	return 0;
+}
+
+static int ad7608_chan_scale_setup(struct ad7606_state *st,
+				   struct iio_chan_spec *chan, int ch)
+{
+	struct ad7606_chan_scale *cs = &st->chan_scales[ch];
+
+	cs->range = 0;
+	cs->scale_avail = ad7606_18bit_hw_scale_avail;
+	cs->num_scales = ARRAY_SIZE(ad7606_18bit_hw_scale_avail);
+	return 0;
+}
+
+static int ad7609_chan_scale_setup(struct ad7606_state *st,
+				   struct iio_chan_spec *chan, int ch)
+{
+	struct ad7606_chan_scale *cs = &st->chan_scales[ch];
+
+	cs->range = 0;
+	cs->scale_avail = ad7609_hw_scale_avail;
+	cs->num_scales = ARRAY_SIZE(ad7609_hw_scale_avail);
+	return 0;
+}
+
 static int ad7606_reg_access(struct iio_dev *indio_dev,
 			     unsigned int reg,
 			     unsigned int writeval,
@@ -610,6 +714,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
 			*val = st->data.buf32[ch];
 			break;
 		case 16:
+		case 14:
 			*val = st->data.buf16[ch];
 			break;
 		default:
@@ -624,6 +729,9 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
 		case 16:
 			*val = sign_extend32(st->data.buf16[ch], 15);
 			break;
+		case 14:
+			*val = sign_extend32(st->data.buf16[ch], 13);
+			break;
 		default:
 			ret = -EINVAL;
 			break;
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 32c6f776c5df..998814a92b82 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -237,6 +237,9 @@ extern const struct ad7606_chip_info ad7606_4_info;
 extern const struct ad7606_chip_info ad7606b_info;
 extern const struct ad7606_chip_info ad7606c_16_info;
 extern const struct ad7606_chip_info ad7606c_18_info;
+extern const struct ad7606_chip_info ad7607_info;
+extern const struct ad7606_chip_info ad7608_info;
+extern const struct ad7606_chip_info ad7609_info;
 extern const struct ad7606_chip_info ad7616_info;
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
index 4e729777d373..a25182a3daa7 100644
--- a/drivers/iio/adc/ad7606_par.c
+++ b/drivers/iio/adc/ad7606_par.c
@@ -211,6 +211,9 @@ static const struct platform_device_id ad7606_driver_ids[] = {
 	{ .name	= "ad7606-6", .driver_data = (kernel_ulong_t)&ad7606_6_info, },
 	{ .name	= "ad7606-8", .driver_data = (kernel_ulong_t)&ad7606_8_info, },
 	{ .name	= "ad7606b", .driver_data = (kernel_ulong_t)&ad7606b_info, },
+	{ .name	= "ad7607", .driver_data = (kernel_ulong_t)&ad7607_info, },
+	{ .name	= "ad7608", .driver_data = (kernel_ulong_t)&ad7608_info, },
+	{ .name	= "ad7609", .driver_data = (kernel_ulong_t)&ad7609_info, },
 	{ }
 };
 MODULE_DEVICE_TABLE(platform, ad7606_driver_ids);
@@ -221,6 +224,9 @@ static const struct of_device_id ad7606_of_match[] = {
 	{ .compatible = "adi,ad7606-6", .data = &ad7606_6_info },
 	{ .compatible = "adi,ad7606-8", .data = &ad7606_8_info },
 	{ .compatible = "adi,ad7606b", .data = &ad7606b_info },
+	{ .compatible = "adi,ad7607", .data = &ad7607_info },
+	{ .compatible = "adi,ad7608", .data = &ad7608_info },
+	{ .compatible = "adi,ad7609", .data = &ad7609_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ad7606_of_match);
diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
index 44c6031e9e9a..0662300cde8d 100644
--- a/drivers/iio/adc/ad7606_spi.c
+++ b/drivers/iio/adc/ad7606_spi.c
@@ -132,6 +132,19 @@ static int ad7606_spi_read_block(struct device *dev,
 	return 0;
 }
 
+static int ad7606_spi_read_block14to16(struct device *dev,
+				       int count, void *buf)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct spi_transfer xfer = {
+		.bits_per_word = 14,
+		.len = count * sizeof(u16),
+		.rx_buf = buf,
+	};
+
+	return spi_sync_transfer(spi, &xfer, 1);
+}
+
 static int ad7606_spi_read_block18to32(struct device *dev,
 				       int count, void *buf)
 {
@@ -325,6 +338,14 @@ static const struct ad7606_bus_ops ad7606_spi_bops = {
 	.read_block = ad7606_spi_read_block,
 };
 
+static const struct ad7606_bus_ops ad7607_spi_bops = {
+	.read_block = ad7606_spi_read_block14to16,
+};
+
+static const struct ad7606_bus_ops ad7608_spi_bops = {
+	.read_block = ad7606_spi_read_block18to32,
+};
+
 static const struct ad7606_bus_ops ad7616_spi_bops = {
 	.read_block = ad7606_spi_read_block,
 	.reg_read = ad7606_spi_reg_read,
@@ -387,6 +408,21 @@ static const struct ad7606_bus_info ad7606c_18_bus_info = {
 	.bops = &ad7606c_18_spi_bops,
 };
 
+static const struct ad7606_bus_info ad7607_bus_info = {
+	.chip_info = &ad7607_info,
+	.bops = &ad7607_spi_bops,
+};
+
+static const struct ad7606_bus_info ad7608_bus_info = {
+	.chip_info = &ad7608_info,
+	.bops = &ad7608_spi_bops,
+};
+
+static const struct ad7606_bus_info ad7609_bus_info = {
+	.chip_info = &ad7609_info,
+	.bops = &ad7608_spi_bops,
+};
+
 static const struct ad7606_bus_info ad7616_bus_info = {
 	.chip_info = &ad7616_info,
 	.bops = &ad7616_spi_bops,
@@ -408,6 +444,9 @@ static const struct spi_device_id ad7606_id_table[] = {
 	{ "ad7606b",  (kernel_ulong_t)&ad7606b_bus_info },
 	{ "ad7606c-16", (kernel_ulong_t)&ad7606c_16_bus_info },
 	{ "ad7606c-18", (kernel_ulong_t)&ad7606c_18_bus_info },
+	{ "ad7607",   (kernel_ulong_t)&ad7607_bus_info },
+	{ "ad7608",   (kernel_ulong_t)&ad7608_bus_info },
+	{ "ad7609",   (kernel_ulong_t)&ad7609_bus_info },
 	{ "ad7616",   (kernel_ulong_t)&ad7616_bus_info },
 	{ }
 };
@@ -421,6 +460,9 @@ static const struct of_device_id ad7606_of_match[] = {
 	{ .compatible = "adi,ad7606b", .data = &ad7606b_bus_info },
 	{ .compatible = "adi,ad7606c-16", .data = &ad7606c_16_bus_info },
 	{ .compatible = "adi,ad7606c-18", .data = &ad7606c_18_bus_info },
+	{ .compatible = "adi,ad7607", .data = &ad7607_bus_info },
+	{ .compatible = "adi,ad7608", .data = &ad7608_bus_info },
+	{ .compatible = "adi,ad7609", .data = &ad7609_bus_info },
 	{ .compatible = "adi,ad7616", .data = &ad7616_bus_info },
 	{ }
 };
-- 
2.46.1

Re: [PATCH 5/6] dt-bindings: iio: adc: adi,ad7606: document AD760{7,8,9} parts

[Conor Dooley]

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

On Mon, Oct 21, 2024 at 04:02:20PM +0300, Alexandru Ardelean wrote:
> This change adds the AD7607, AD7608 & AD7609 parts to the binding doc of
> the ad7606 driver.
> 
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

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

Re: [PATCH 3/6] iio: adc: ad7606: use realbits for sign-extending in scan_direct

[David Lechner]

On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> Currently the 'ad7606' driver supports parts with 18 and 16 bits
> resolutions.
> But when adding support for AD7607 (which has a 14-bit resolution) we
> should check for the 'realbits' field, to be able to sign-extend correctly.
> 
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---
>  drivers/iio/adc/ad7606.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index d0fe9fd65f3f..a1f0c2feb04a 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -568,7 +568,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;
> +	unsigned int realbits = st->chip_info->channels[1].scan_type.realbits;
>  	const struct iio_chan_spec *chan;
>  	int ret;
>  
> @@ -603,15 +603,29 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
>  
>  	chan = &indio_dev->channels[ch + 1];
>  	if (chan->scan_type.sign == 'u') {
> -		if (storagebits > 16)

I think it would be simpler to keep this if statement for choosing
which data.bufXX to use since there are only 2 choices.


> +		switch (realbits) {
> +		case 18:
>  			*val = st->data.buf32[ch];
> -		else
> +			break;
> +		case 16:
>  			*val = st->data.buf16[ch];
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
>  	} else {
> -		if (storagebits > 16)
> +		switch (realbits) {
> +		case 18:
>  			*val = sign_extend32(st->data.buf32[ch], 17);
> -		else
> +			break;
> +		case 16:
>  			*val = sign_extend32(st->data.buf16[ch], 15);
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}

We can change this to:

	*val = sign_extend32(st->data.buf16[ch], reablbits - 1);

Then no additional changes should be needed to support 14-bit chips.

And similarly, we could avoid the need to use the more verbose
switch statement.

>  	}
>  
>  error_ret:

Re: [PATCH 4/6] iio: adc: ad7606: rework scale-available to be static

[David Lechner]

On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> The main driver for this change is the AD7607 part, which has a scale of
> "1.220703" for the ±10V range. The AD7607 has a resolution of 14-bits.
> 
> So, just adding the scale-available list for that part would require some
> quirks to handle just that scale value.
> But to do it more neatly, the best approach is to rework the scale
> available lists to have the same format as it is returned to userspace.
> That way, we can also get rid of the allocation for the 'scale_avail_show'
> array.
> 
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---

...


>  static ssize_t in_voltage_scale_available_show(struct device *dev,
>  					       struct device_attribute *attr,
>  					       char *buf)
> @@ -703,8 +690,16 @@ 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];
> +	const unsigned int (*vals)[2] = cs->scale_avail;
> +	unsigned int i;
> +	size_t len = 0;
>  
> -	return ad7606_show_avail(buf, cs->scale_avail, cs->num_scales, true);
> +	for (i = 0; i < cs->num_scales; i++)
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ",
> +				 vals[i][0], vals[i][1]);
> +	buf[len - 1] = '\n';
> +
> +	return len;
>  }
>  
>  static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);

Probably a reason for this that I forgot, but why is this handled in a
custom attribute instead of ad7606_read_avail()?

> @@ -742,6 +737,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>  			    long mask)
>  {
>  	struct ad7606_state *st = iio_priv(indio_dev);
> +	unsigned int scale_avail[AD760X_MAX_SCALES];

Calling this scale_avail_uv would make the code easier for me to understand.

>  	struct ad7606_chan_scale *cs;
>  	int i, ret, ch = 0;
>  
> @@ -752,7 +748,12 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>  		if (st->sw_mode_en)
>  			ch = chan->address;
>  		cs = &st->chan_scales[ch];
> -		i = find_closest(val2, cs->scale_avail, cs->num_scales);
> +		for (i = 0; i < cs->num_scales; i++) {
> +			scale_avail[i] = cs->scale_avail[i][0] * 1000000 +

								 MICRO

> +					 cs->scale_avail[i][1];
> +		}
> +		val = val * 1000000 + val2;
> +		i = find_closest(val, scale_avail, cs->num_scales);
>  		ret = st->write_scale(indio_dev, ch, i + cs->reg_offset);
>  		if (ret < 0)
>  			return ret;
> @@ -788,9 +789,15 @@ static ssize_t ad7606_oversampling_ratio_avail(struct device *dev,
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct ad7606_state *st = iio_priv(indio_dev);
> +	const unsigned int *vals = st->oversampling_avail;
> +	unsigned int i;
> +	size_t len = 0;
>  
> -	return ad7606_show_avail(buf, st->oversampling_avail,
> -				 st->num_os_ratios, false);
> +	for (i = 0; i < st->num_os_ratios; i++)
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%u ", vals[i]);
> +	buf[len - 1] = '\n';
> +
> +	return len;
>  }
>  
>  static IIO_DEVICE_ATTR(oversampling_ratio_available, 0444,

Same question about ad7606_read_avail() instead for this one.

Re: [PATCH 2/6] iio: adc: ad7606: fix issue/quirk with find_closest() for oversampling

[David Lechner]

On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> There's a small issue with setting oversampling-ratio that seems to have
> been there since the driver was in staging.
> Trying to set an oversampling value of '2' will set an oversampling value
> of '1'. This is because find_closest() does an average + rounding of 1 + 2,
> and we get '1'.
> 
> This is the only issue with find_closest(), at least in this setup. The
> other values (above 2) work reasonably well. Setting 3, rounds to 2, so a
> quick fix is to round 'val' to 3 (if userspace provides 2).

This sounds like a bug in find_closest() instead of in this driver.

If there is an exact match in the list, it seems reasonable to expect
that the exact match is returned by find_closest().

Re: [PATCH 2/6] iio: adc: ad7606: fix issue/quirk with find_closest() for oversampling

[David Lechner]

On 10/21/24 2:03 PM, David Lechner wrote:
> On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
>> There's a small issue with setting oversampling-ratio that seems to have
>> been there since the driver was in staging.
>> Trying to set an oversampling value of '2' will set an oversampling value
>> of '1'. This is because find_closest() does an average + rounding of 1 + 2,
>> and we get '1'.
>>
>> This is the only issue with find_closest(), at least in this setup. The
>> other values (above 2) work reasonably well. Setting 3, rounds to 2, so a
>> quick fix is to round 'val' to 3 (if userspace provides 2).
> 
> This sounds like a bug in find_closest() instead of in this driver.
> 
> If there is an exact match in the list, it seems reasonable to expect
> that the exact match is returned by find_closest().
> 

Likely also affected by this bug since they have values 1, 2 in the list:

* rtq6056_adc_set_average()
* si1133_scale_to_swgain()

Re: [PATCH 3/6] iio: adc: ad7606: use realbits for sign-extending in scan_direct

[Alexandru Ardelean]

On Mon, Oct 21, 2024 at 8:19 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> > Currently the 'ad7606' driver supports parts with 18 and 16 bits
> > resolutions.
> > But when adding support for AD7607 (which has a 14-bit resolution) we
> > should check for the 'realbits' field, to be able to sign-extend correctly.
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > ---
> >  drivers/iio/adc/ad7606.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> > index d0fe9fd65f3f..a1f0c2feb04a 100644
> > --- a/drivers/iio/adc/ad7606.c
> > +++ b/drivers/iio/adc/ad7606.c
> > @@ -568,7 +568,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;
> > +     unsigned int realbits = st->chip_info->channels[1].scan_type.realbits;
> >       const struct iio_chan_spec *chan;
> >       int ret;
> >
> > @@ -603,15 +603,29 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> >
> >       chan = &indio_dev->channels[ch + 1];
> >       if (chan->scan_type.sign == 'u') {
> > -             if (storagebits > 16)
>
> I think it would be simpler to keep this if statement for choosing
> which data.bufXX to use since there are only 2 choices.
>
>
> > +             switch (realbits) {
> > +             case 18:
> >                       *val = st->data.buf32[ch];
> > -             else
> > +                     break;
> > +             case 16:
> >                       *val = st->data.buf16[ch];
> > +                     break;
> > +             default:
> > +                     ret = -EINVAL;
> > +                     break;
> > +             }
> >       } else {
> > -             if (storagebits > 16)
> > +             switch (realbits) {
> > +             case 18:
> >                       *val = sign_extend32(st->data.buf32[ch], 17);
> > -             else
> > +                     break;
> > +             case 16:
> >                       *val = sign_extend32(st->data.buf16[ch], 15);
> > +                     break;
> > +             default:
> > +                     ret = -EINVAL;
> > +                     break;
> > +             }
>
> We can change this to:
>
>         *val = sign_extend32(st->data.buf16[ch], reablbits - 1);
>
> Then no additional changes should be needed to support 14-bit chips.
>
> And similarly, we could avoid the need to use the more verbose
> switch statement.

Ack.

>
> >       }
> >
> >  error_ret:
>
>

Re: [PATCH 2/6] iio: adc: ad7606: fix issue/quirk with find_closest() for oversampling

[Alexandru Ardelean]

On Mon, Oct 21, 2024 at 10:31 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 10/21/24 2:03 PM, David Lechner wrote:
> > On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> >> There's a small issue with setting oversampling-ratio that seems to have
> >> been there since the driver was in staging.
> >> Trying to set an oversampling value of '2' will set an oversampling value
> >> of '1'. This is because find_closest() does an average + rounding of 1 + 2,
> >> and we get '1'.
> >>
> >> This is the only issue with find_closest(), at least in this setup. The
> >> other values (above 2) work reasonably well. Setting 3, rounds to 2, so a
> >> quick fix is to round 'val' to 3 (if userspace provides 2).
> >
> > This sounds like a bug in find_closest() instead of in this driver.
> >

Adding Bart (the original author of find_closest()).

> > If there is an exact match in the list, it seems reasonable to expect
> > that the exact match is returned by find_closest().
> >
>
> Likely also affected by this bug since they have values 1, 2 in the list:
>
> * rtq6056_adc_set_average()
> * si1133_scale_to_swgain()

Yeah.
I forgot to mention this sooner.
But this patch is more of an RFC patch about how to handle this
situation with find_closest().

For monotonic values with an increment of 1, find_closest() is a bit buggy.
Will try to fix find_closest()

>

Re: [PATCH 4/6] iio: adc: ad7606: rework scale-available to be static

[Alexandru Ardelean]

On Mon, Oct 21, 2024 at 9:17 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> > The main driver for this change is the AD7607 part, which has a scale of
> > "1.220703" for the ±10V range. The AD7607 has a resolution of 14-bits.
> >
> > So, just adding the scale-available list for that part would require some
> > quirks to handle just that scale value.
> > But to do it more neatly, the best approach is to rework the scale
> > available lists to have the same format as it is returned to userspace.
> > That way, we can also get rid of the allocation for the 'scale_avail_show'
> > array.
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > ---
>
> ...
>
>
> >  static ssize_t in_voltage_scale_available_show(struct device *dev,
> >                                              struct device_attribute *attr,
> >                                              char *buf)
> > @@ -703,8 +690,16 @@ 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];
> > +     const unsigned int (*vals)[2] = cs->scale_avail;
> > +     unsigned int i;
> > +     size_t len = 0;
> >
> > -     return ad7606_show_avail(buf, cs->scale_avail, cs->num_scales, true);
> > +     for (i = 0; i < cs->num_scales; i++)
> > +             len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ",
> > +                              vals[i][0], vals[i][1]);
> > +     buf[len - 1] = '\n';
> > +
> > +     return len;
> >  }
> >
> >  static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);
>
> Probably a reason for this that I forgot, but why is this handled in a
> custom attribute instead of ad7606_read_avail()?

[1]
So, this is a quirk of this driver that would require a bigger cleanup.
It could be done as a different series.
Or (otherwise said) I would do it in a different series (unless
requested otherwise).

These device-level attributes are attached in the probe() of this
driver, based on the GPIO configurations provided via DT.
There's that bit of code

        if (st->gpio_os) {
                if (st->gpio_range)
                        indio_dev->info = &ad7606_info_os_and_range;
                else
                        indio_dev->info = &ad7606_info_os;
        } else {
                if (st->gpio_range)
                        indio_dev->info = &ad7606_info_range;
                else
                        indio_dev->info = &ad7606_info_no_os_or_range;
        }

The "range" there refers to "scale_available", which is only attached
like this, for HW mode.
A rework of HW-mode would be a good idea.

>
> > @@ -742,6 +737,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> >                           long mask)
> >  {
> >       struct ad7606_state *st = iio_priv(indio_dev);
> > +     unsigned int scale_avail[AD760X_MAX_SCALES];
>
> Calling this scale_avail_uv would make the code easier for me to understand.

Ack.

>
> >       struct ad7606_chan_scale *cs;
> >       int i, ret, ch = 0;
> >
> > @@ -752,7 +748,12 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> >               if (st->sw_mode_en)
> >                       ch = chan->address;
> >               cs = &st->chan_scales[ch];
> > -             i = find_closest(val2, cs->scale_avail, cs->num_scales);
> > +             for (i = 0; i < cs->num_scales; i++) {
> > +                     scale_avail[i] = cs->scale_avail[i][0] * 1000000 +
>
>                                                                  MICRO

Ack.

>
> > +                                      cs->scale_avail[i][1];
> > +             }
> > +             val = val * 1000000 + val2;
> > +             i = find_closest(val, scale_avail, cs->num_scales);
> >               ret = st->write_scale(indio_dev, ch, i + cs->reg_offset);
> >               if (ret < 0)
> >                       return ret;
> > @@ -788,9 +789,15 @@ static ssize_t ad7606_oversampling_ratio_avail(struct device *dev,
> >  {
> >       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >       struct ad7606_state *st = iio_priv(indio_dev);
> > +     const unsigned int *vals = st->oversampling_avail;
> > +     unsigned int i;
> > +     size_t len = 0;
> >
> > -     return ad7606_show_avail(buf, st->oversampling_avail,
> > -                              st->num_os_ratios, false);
> > +     for (i = 0; i < st->num_os_ratios; i++)
> > +             len += scnprintf(buf + len, PAGE_SIZE - len, "%u ", vals[i]);
> > +     buf[len - 1] = '\n';
> > +
> > +     return len;
> >  }
> >
> >  static IIO_DEVICE_ATTR(oversampling_ratio_available, 0444,
>
> Same question about ad7606_read_avail() instead for this one.

See [1]

>

Re: [PATCH 2/6] iio: adc: ad7606: fix issue/quirk with find_closest() for oversampling

[Nuno Sá]

On Tue, 2024-10-22 at 09:31 +0300, Alexandru Ardelean wrote:
> On Mon, Oct 21, 2024 at 10:31 PM David Lechner <dlechner@baylibre.com> wrote:
> > 
> > On 10/21/24 2:03 PM, David Lechner wrote:
> > > On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> > > > There's a small issue with setting oversampling-ratio that seems to have
> > > > been there since the driver was in staging.
> > > > Trying to set an oversampling value of '2' will set an oversampling
> > > > value
> > > > of '1'. This is because find_closest() does an average + rounding of 1 +
> > > > 2,
> > > > and we get '1'.
> > > > 
> > > > This is the only issue with find_closest(), at least in this setup. The
> > > > other values (above 2) work reasonably well. Setting 3, rounds to 2, so
> > > > a
> > > > quick fix is to round 'val' to 3 (if userspace provides 2).
> > > 
> > > This sounds like a bug in find_closest() instead of in this driver.
> > > 
> 
> Adding Bart (the original author of find_closest()).
> 
> > > If there is an exact match in the list, it seems reasonable to expect
> > > that the exact match is returned by find_closest().
> > > 
> > 
> > Likely also affected by this bug since they have values 1, 2 in the list:
> > 
> > * rtq6056_adc_set_average()
> > * si1133_scale_to_swgain()
> 
> Yeah.
> I forgot to mention this sooner.
> But this patch is more of an RFC patch about how to handle this
> situation with find_closest().
> 
> For monotonic values with an increment of 1, find_closest() is a bit buggy.
> Will try to fix find_closest()
> 
> > 

FWIW, I'm not a fan of using find_closest() in this situation. We have an
available attr wich outputs the supported values. To me, -EINVAL is the way to
go if some user writes an invalid value.

I feel the usage of find_closest() in these case is likely to make the code
easier. Maybe an helper analogous to match_string() would be seen with good eyes
(like match_value()).

But yeah, I guess that changing the behavior now could break some userspace
users.

- Nuno Sá

Re: [PATCH 4/6] iio: adc: ad7606: rework scale-available to be static

[Nuno Sá]

On Tue, 2024-10-22 at 09:59 +0300, Alexandru Ardelean wrote:
> On Mon, Oct 21, 2024 at 9:17 PM David Lechner <dlechner@baylibre.com> wrote:
> > 
> > On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> > > The main driver for this change is the AD7607 part, which has a scale of
> > > "1.220703" for the ±10V range. The AD7607 has a resolution of 14-bits.
> > > 
> > > So, just adding the scale-available list for that part would require some
> > > quirks to handle just that scale value.
> > > But to do it more neatly, the best approach is to rework the scale
> > > available lists to have the same format as it is returned to userspace.
> > > That way, we can also get rid of the allocation for the 'scale_avail_show'
> > > array.
> > > 
> > > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > > ---
> > 
> > ...
> > 
> > 
> > >  static ssize_t in_voltage_scale_available_show(struct device *dev,
> > >                                              struct device_attribute
> > > *attr,
> > >                                              char *buf)
> > > @@ -703,8 +690,16 @@ 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];
> > > +     const unsigned int (*vals)[2] = cs->scale_avail;
> > > +     unsigned int i;
> > > +     size_t len = 0;
> > > 
> > > -     return ad7606_show_avail(buf, cs->scale_avail, cs->num_scales,
> > > true);
> > > +     for (i = 0; i < cs->num_scales; i++)
> > > +             len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ",
> > > +                              vals[i][0], vals[i][1]);
> > > +     buf[len - 1] = '\n';
> > > +
> > > +     return len;
> > >  }
> > > 
> > >  static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);
> > 
> > Probably a reason for this that I forgot, but why is this handled in a
> > custom attribute instead of ad7606_read_avail()?
> 
> [1]
> So, this is a quirk of this driver that would require a bigger cleanup.
> It could be done as a different series.
> Or (otherwise said) I would do it in a different series (unless
> requested otherwise).

Agreed...

> 
> These device-level attributes are attached in the probe() of this
> driver, based on the GPIO configurations provided via DT.
> There's that bit of code
> 
>         if (st->gpio_os) {
>                 if (st->gpio_range)
>                         indio_dev->info = &ad7606_info_os_and_range;
>                 else
>                         indio_dev->info = &ad7606_info_os;
>         } else {
>                 if (st->gpio_range)
>                         indio_dev->info = &ad7606_info_range;
>                 else
>                         indio_dev->info = &ad7606_info_no_os_or_range;
>         }
> 
> The "range" there refers to "scale_available", which is only attached
> like this, for HW mode.
> A rework of HW-mode would be a good idea.
> 

Maybe it's also due to .read_avail() not being around when the driver was first
added (but not sure about that).

- Nuno Sá

>