LinuxLists.cc - [PATCH RFC v6 00/10] iio: adc: add new ad7380 driver

[permalink] [raw]

Subject: [PATCH RFC v6 01/10] dt-bindings: iio: adc: Add binding for AD7380 ADCs

From: David Lechner <[email protected]>

This adds a binding specification for the Analog Devices Inc. AD7380
family of ADCs.

Signed-off-by: David Lechner <[email protected]>
Signed-off-by: Julien Stephan <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
.../devicetree/bindings/iio/adc/adi,ad7380.yaml | 82 ++++++++++++++++++++++
MAINTAINERS | 9 +++
2 files changed, 91 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
new file mode 100644
index 000000000000..5e1ee0ebe0a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7380.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices Simultaneous Sampling Analog to Digital Converters
+
+maintainers:
+ - Michael Hennerich <[email protected]>
+ - Nuno Sá <[email protected]>
+
+description: |
+ * https://www.analog.com/en/products/ad7380.html
+ * https://www.analog.com/en/products/ad7381.html
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - adi,ad7380
+ - adi,ad7381
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 80000000
+ spi-cpol: true
+ spi-cpha: true
+
+ vcc-supply:
+ description: A 3V to 3.6V supply that powers the chip.
+
+ vlogic-supply:
+ description:
+ A 1.65V to 3.6V supply for the logic pins.
+
+ refio-supply:
+ description:
+ A 2.5V to 3.3V supply for the external reference voltage. When omitted,
+ the internal 2.5V reference is used.
+
+ interrupts:
+ description:
+ When the device is using 1-wire mode, this property is used to optionally
+ specify the ALERT interrupt.
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - vcc-supply
+ - vlogic-supply
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ adc@0 {
+ compatible = "adi,ad7380";
+ reg = <0>;
+
+ spi-cpol;
+ spi-cpha;
+ spi-max-frequency = <80000000>;
+
+ interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
+ interrupt-parent = <&gpio0>;
+
+ vcc-supply = <&supply_3_3V>;
+ vlogic-supply = <&supply_3_3V>;
+ refio-supply = <&supply_2_5V>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 8ad79cf70552..5bb168937853 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -427,6 +427,15 @@ W: http://wiki.analog.com/AD7142
W: https://ez.analog.com/linux-software-drivers
F: drivers/input/misc/ad714x.c

+AD738X ADC DRIVER (AD7380/1/2/4)
+M: Michael Hennerich <[email protected]>
+M: Nuno Sá <[email protected]>
+R: David Lechner <[email protected]>
+S: Supported
+W: https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
+W: https://ez.analog.com/linux-software-drivers
+F: Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
+
AD7877 TOUCHSCREEN DRIVER
M: Michael Hennerich <[email protected]>
S: Supported

--
2.44.0

2024-05-01 14:56:33

[permalink] [raw]

Subject: [PATCH RFC v6 03/10] dt-bindings: iio: adc: ad7380: add pseudo-differential parts

From: David Lechner <[email protected]>

Adding AD7383 and AD7384 compatible parts that are pseudo-differential.

Pseudo-differential require common mode voltage supplies, so add them
conditionally

Signed-off-by: David Lechner <[email protected]>
Signed-off-by: Julien Stephan <[email protected]>
Acked-by: Conor Dooley <[email protected]>
---
.../devicetree/bindings/iio/adc/adi,ad7380.yaml | 32 ++++++++++++++++++++++
1 file changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
index 5e1ee0ebe0a2..de3d28a021ae 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
@@ -13,6 +13,8 @@ maintainers:
description: |
* https://www.analog.com/en/products/ad7380.html
* https://www.analog.com/en/products/ad7381.html
+ * https://www.analog.com/en/products/ad7383.html
+ * https://www.analog.com/en/products/ad7384.html

$ref: /schemas/spi/spi-peripheral-props.yaml#

@@ -21,6 +23,8 @@ properties:
enum:
- adi,ad7380
- adi,ad7381
+ - adi,ad7383
+ - adi,ad7384

reg:
maxItems: 1
@@ -42,6 +46,16 @@ properties:
A 2.5V to 3.3V supply for the external reference voltage. When omitted,
the internal 2.5V reference is used.

+ aina-supply:
+ description:
+ The common mode voltage supply for the AINA- pin on pseudo-differential
+ chips.
+
+ ainb-supply:
+ description:
+ The common mode voltage supply for the AINB- pin on pseudo-differential
+ chips.
+
interrupts:
description:
When the device is using 1-wire mode, this property is used to optionally
@@ -56,6 +70,24 @@ required:

unevaluatedProperties: false

+allOf:
+ # pseudo-differential chips require common mode voltage supplies,
+ # true differential chips don't use them
+ - if:
+ properties:
+ compatible:
+ enum:
+ - adi,ad7383
+ - adi,ad7384
+ then:
+ required:
+ - aina-supply
+ - ainb-supply
+ else:
+ properties:
+ aina-supply: false
+ ainb-supply: false
+
examples:
- |
#include <dt-bindings/interrupt-controller/irq.h>

--
2.44.0

2024-05-01 14:56:59

[permalink] [raw]

Subject: [PATCH RFC v6 04/10] iio: adc: ad7380: add support for pseudo-differential parts

From: David Lechner <[email protected]>

Add support for AD7383, AD7384 pseudo-differential compatible parts.
Pseudo differential parts require common mode voltage supplies so add
the support for them and add the support of IIO_CHAN_INFO_OFFSET to
retrieve the offset

Signed-off-by: David Lechner <[email protected]>
Signed-off-by: Julien Stephan <[email protected]>
---
drivers/iio/adc/ad7380.c | 110 ++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 94 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index a218f59c276e..d6abce6d45d3 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -7,6 +7,7 @@
*
* Datasheets of supported parts:
* ad7380/1 : https://www.analog.com/media/en/technical-documentation/data-sheets/AD7380-7381.pdf
+ * ad7383/4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7383-7384.pdf
*/

#include <linux/bitfield.h>
@@ -68,16 +69,19 @@ struct ad7380_chip_info {
const char *name;
const struct iio_chan_spec *channels;
unsigned int num_channels;
+ const char * const *vcm_supplies;
+ unsigned int num_vcm_supplies;
};

-#define AD7380_CHANNEL(index, bits) { \
+#define AD7380_CHANNEL(index, bits, diff) { \
.type = IIO_VOLTAGE, \
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ ((diff) ? 0 : BIT(IIO_CHAN_INFO_OFFSET)), \
.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
.indexed = 1, \
- .differential = 1, \
- .channel = 2 * (index), \
- .channel2 = 2 * (index) + 1, \
+ .differential = (diff), \
+ .channel = (diff) ? (2 * (index)) : (index), \
+ .channel2 = (diff) ? (2 * (index) + 1) : 0, \
.scan_index = (index), \
.scan_type = { \
.sign = 's', \
@@ -87,15 +91,23 @@ struct ad7380_chip_info {
}, \
}

-#define DEFINE_AD7380_2_CHANNEL(name, bits) \
-static const struct iio_chan_spec name[] = { \
- AD7380_CHANNEL(0, bits), \
- AD7380_CHANNEL(1, bits), \
- IIO_CHAN_SOFT_TIMESTAMP(2), \
+#define DEFINE_AD7380_2_CHANNEL(name, bits, diff) \
+static const struct iio_chan_spec name[] = { \
+ AD7380_CHANNEL(0, bits, diff), \
+ AD7380_CHANNEL(1, bits, diff), \
+ IIO_CHAN_SOFT_TIMESTAMP(2), \
}

-DEFINE_AD7380_2_CHANNEL(ad7380_channels, 16);
-DEFINE_AD7380_2_CHANNEL(ad7381_channels, 14);
+/* fully differential */
+DEFINE_AD7380_2_CHANNEL(ad7380_channels, 16, 1);
+DEFINE_AD7380_2_CHANNEL(ad7381_channels, 14, 1);
+/* pseudo differential */
+DEFINE_AD7380_2_CHANNEL(ad7383_channels, 16, 0);
+DEFINE_AD7380_2_CHANNEL(ad7384_channels, 14, 0);
+
+static const char * const ad7380_2_channel_vcm_supplies[] = {
+ "aina", "ainb",
+};

/* Since this is simultaneous sampling, we don't allow individual channels. */
static const unsigned long ad7380_2_channel_scan_masks[] = {
@@ -115,11 +127,28 @@ static const struct ad7380_chip_info ad7381_chip_info = {
.num_channels = ARRAY_SIZE(ad7381_channels),
};

+static const struct ad7380_chip_info ad7383_chip_info = {
+ .name = "ad7383",
+ .channels = ad7383_channels,
+ .num_channels = ARRAY_SIZE(ad7383_channels),
+ .vcm_supplies = ad7380_2_channel_vcm_supplies,
+ .num_vcm_supplies = ARRAY_SIZE(ad7380_2_channel_vcm_supplies),
+};
+
+static const struct ad7380_chip_info ad7384_chip_info = {
+ .name = "ad7384",
+ .channels = ad7384_channels,
+ .num_channels = ARRAY_SIZE(ad7384_channels),
+ .vcm_supplies = ad7380_2_channel_vcm_supplies,
+ .num_vcm_supplies = ARRAY_SIZE(ad7380_2_channel_vcm_supplies),
+};
+
struct ad7380_state {
const struct ad7380_chip_info *chip_info;
struct spi_device *spi;
struct regmap *regmap;
unsigned int vref_mv;
+ unsigned int vcm_mv[2];
/*
* DMA (thus cache coherency maintenance) requires the
* transfer buffers to live in their own cache lines.
@@ -293,13 +322,24 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
unreachable();
case IIO_CHAN_INFO_SCALE:
/*
- * According to the datasheet, the LSB size for fully differential ADC is
- * (2 × VREF) / 2^N, where N is the ADC resolution (i.e realbits)
+ * According to the datasheet, the LSB size is:
+ * * (2 × VREF) / 2^N, for differential chips
+ * * VREF / 2^N, for pseudo-differential chips
+ * where N is the ADC resolution (i.e realbits)
*/
*val = st->vref_mv;
- *val2 = chan->scan_type.realbits - 1;
+ *val2 = chan->scan_type.realbits - chan->differential;

return IIO_VAL_FRACTIONAL_LOG2;
+ case IIO_CHAN_INFO_OFFSET:
+ /*
+ * According to IIO ABI, offset is applied before scale,
+ * so offset is: vcm_mv / scale
+ */
+ *val = st->vcm_mv[chan->channel] * (1 << chan->scan_type.realbits)
+ / st->vref_mv;
+
+ return IIO_VAL_INT;
default:
return -EINVAL;
}
@@ -346,7 +386,7 @@ static int ad7380_probe(struct spi_device *spi)
struct iio_dev *indio_dev;
struct ad7380_state *st;
struct regulator *vref;
- int ret;
+ int ret, i;

indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
if (!indio_dev)
@@ -390,6 +430,40 @@ static int ad7380_probe(struct spi_device *spi)
st->vref_mv = AD7380_INTERNAL_REF_MV;
}

+ if (st->chip_info->num_vcm_supplies > ARRAY_SIZE(st->vcm_mv))
+ return dev_err_probe(&spi->dev, -EINVAL,
+ "invalid number of VCM supplies\n");
+
+ /*
+ * pseudo-differential chips have common mode supplies for the negative
+ * input pin.
+ */
+ for (i = 0; i < st->chip_info->num_vcm_supplies; i++) {
+ struct regulator *vcm;
+
+ vcm = devm_regulator_get(&spi->dev,
+ st->chip_info->vcm_supplies[i]);
+ if (IS_ERR(vcm))
+ return dev_err_probe(&spi->dev, PTR_ERR(vcm),
+ "Failed to get %s regulator\n",
+ st->chip_info->vcm_supplies[i]);
+
+ ret = regulator_enable(vcm);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(&spi->dev,
+ ad7380_regulator_disable, vcm);
+ if (ret)
+ return ret;
+
+ ret = regulator_get_voltage(vcm);
+ if (ret < 0)
+ return ret;
+
+ st->vcm_mv[i] = ret / 1000;
+ }
+
st->regmap = devm_regmap_init(&spi->dev, NULL, st, &ad7380_regmap_config);
if (IS_ERR(st->regmap))
return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
@@ -418,12 +492,16 @@ static int ad7380_probe(struct spi_device *spi)
static const struct of_device_id ad7380_of_match_table[] = {
{ .compatible = "adi,ad7380", .data = &ad7380_chip_info },
{ .compatible = "adi,ad7381", .data = &ad7381_chip_info },
+ { .compatible = "adi,ad7383", .data = &ad7383_chip_info },
+ { .compatible = "adi,ad7384", .data = &ad7384_chip_info },
{ }
};

static const struct spi_device_id ad7380_id_table[] = {
{ "ad7380", (kernel_ulong_t)&ad7380_chip_info },
{ "ad7381", (kernel_ulong_t)&ad7381_chip_info },
+ { "ad7383", (kernel_ulong_t)&ad7383_chip_info },
+ { "ad7384", (kernel_ulong_t)&ad7384_chip_info },
{ }
};
MODULE_DEVICE_TABLE(spi, ad7380_id_table);

--
2.44.0

2024-05-01 14:57:29

[permalink] [raw]

Subject: [PATCH RFC v6 06/10] dt-bindings: iio: adc: ad7380: add support for ad738x-4 4 channels variants

Add compatible support for ad7380/1/3/4-4 parts which are 4 channels
variants from ad7380/1/3/4

Signed-off-by: Julien Stephan <[email protected]>
Acked-by: Conor Dooley <[email protected]>
---
.../devicetree/bindings/iio/adc/adi,ad7380.yaml | 34 ++++++++++++++++++++++
1 file changed, 34 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
index de3d28a021ae..899b777017ce 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
@@ -15,6 +15,10 @@ description: |
* https://www.analog.com/en/products/ad7381.html
* https://www.analog.com/en/products/ad7383.html
* https://www.analog.com/en/products/ad7384.html
+ * https://www.analog.com/en/products/ad7380-4.html
+ * https://www.analog.com/en/products/ad7381-4.html
+ * https://www.analog.com/en/products/ad7383-4.html
+ * https://www.analog.com/en/products/ad7384-4.html

$ref: /schemas/spi/spi-peripheral-props.yaml#

@@ -25,6 +29,10 @@ properties:
- adi,ad7381
- adi,ad7383
- adi,ad7384
+ - adi,ad7380-4
+ - adi,ad7381-4
+ - adi,ad7383-4
+ - adi,ad7384-4

reg:
maxItems: 1
@@ -56,6 +64,16 @@ properties:
The common mode voltage supply for the AINB- pin on pseudo-differential
chips.

+ ainc-supply:
+ description:
+ The common mode voltage supply for the AINC- pin on pseudo-differential
+ chips.
+
+ aind-supply:
+ description:
+ The common mode voltage supply for the AIND- pin on pseudo-differential
+ chips.
+
interrupts:
description:
When the device is using 1-wire mode, this property is used to optionally
@@ -79,6 +97,8 @@ allOf:
enum:
- adi,ad7383
- adi,ad7384
+ - adi,ad7383-4
+ - adi,ad7384-4
then:
required:
- aina-supply
@@ -87,6 +107,20 @@ allOf:
properties:
aina-supply: false
ainb-supply: false
+ - if:
+ properties:
+ compatible:
+ enum:
+ - adi,ad7383-4
+ - adi,ad7384-4
+ then:
+ required:
+ - ainc-supply
+ - aind-supply
+ else:
+ properties:
+ ainc-supply: false
+ aind-supply: false

examples:
- |

--
2.44.0

2024-05-01 14:58:04

[permalink] [raw]

Subject: [PATCH RFC v6 07/10] iio: adc: ad7380: add support for ad738x-4 4 channels variants

Add support for ad7380/1/2/3-4 parts which are 4 channels
variants from ad7380/1/2/3

Signed-off-by: Julien Stephan <[email protected]>
---
drivers/iio/adc/ad7380.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index 201006d878f1..020959759170 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -8,6 +8,9 @@
* Datasheets of supported parts:
* ad7380/1 : https://www.analog.com/media/en/technical-documentation/data-sheets/AD7380-7381.pdf
* ad7383/4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7383-7384.pdf
+ * ad7380-4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7380-4.pdf
+ * ad7381-4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7381-4.pdf
+ * ad7383/4-4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7383-4-ad7384-4.pdf
*/

#include <linux/bitfield.h>
@@ -29,7 +32,7 @@
#include <linux/iio/trigger_consumer.h>
#include <linux/iio/triggered_buffer.h>

-#define MAX_NUM_CHANNELS 2
+#define MAX_NUM_CHANNELS 4
/* 2.5V internal reference voltage */
#define AD7380_INTERNAL_REF_MV 2500

@@ -106,27 +109,53 @@ static const struct iio_chan_spec name[] = { \
IIO_CHAN_SOFT_TIMESTAMP(2), \
}

+#define DEFINE_AD7380_4_CHANNEL(name, bits, diff) \
+static const struct iio_chan_spec name[] = { \
+ AD7380_CHANNEL(0, bits, diff), \
+ AD7380_CHANNEL(1, bits, diff), \
+ AD7380_CHANNEL(2, bits, diff), \
+ AD7380_CHANNEL(3, bits, diff), \
+ IIO_CHAN_SOFT_TIMESTAMP(4), \
+}
+
/* fully differential */
DEFINE_AD7380_2_CHANNEL(ad7380_channels, 16, 1);
DEFINE_AD7380_2_CHANNEL(ad7381_channels, 14, 1);
+DEFINE_AD7380_4_CHANNEL(ad7380_4_channels, 16, 1);
+DEFINE_AD7380_4_CHANNEL(ad7381_4_channels, 14, 1);
/* pseudo differential */
DEFINE_AD7380_2_CHANNEL(ad7383_channels, 16, 0);
DEFINE_AD7380_2_CHANNEL(ad7384_channels, 14, 0);
+DEFINE_AD7380_4_CHANNEL(ad7383_4_channels, 16, 0);
+DEFINE_AD7380_4_CHANNEL(ad7384_4_channels, 14, 0);

static const char * const ad7380_2_channel_vcm_supplies[] = {
"aina", "ainb",
};

+static const char * const ad7380_4_channel_vcm_supplies[] = {
+ "aina", "ainb", "ainc", "aind",
+};
+
/* Since this is simultaneous sampling, we don't allow individual channels. */
static const unsigned long ad7380_2_channel_scan_masks[] = {
GENMASK(1, 0),
0
};

+static const unsigned long ad7380_4_channel_scan_masks[] = {
+ GENMASK(3, 0),
+ 0
+};
+
static const struct ad7380_timing_specs ad7380_timing = {
.t_csh_ns = 10,
};

+static const struct ad7380_timing_specs ad7380_4_timing = {
+ .t_csh_ns = 20,
+};
+
static const struct ad7380_chip_info ad7380_chip_info = {
.name = "ad7380",
.channels = ad7380_channels,
@@ -163,6 +192,42 @@ static const struct ad7380_chip_info ad7384_chip_info = {
.timing_specs = &ad7380_timing,
};

+static const struct ad7380_chip_info ad7380_4_chip_info = {
+ .name = "ad7380-4",
+ .channels = ad7380_4_channels,
+ .num_channels = ARRAY_SIZE(ad7380_4_channels),
+ .available_scan_masks = ad7380_4_channel_scan_masks,
+ .timing_specs = &ad7380_4_timing,
+};
+
+static const struct ad7380_chip_info ad7381_4_chip_info = {
+ .name = "ad7381-4",
+ .channels = ad7381_4_channels,
+ .num_channels = ARRAY_SIZE(ad7381_4_channels),
+ .available_scan_masks = ad7380_4_channel_scan_masks,
+ .timing_specs = &ad7380_4_timing,
+};
+
+static const struct ad7380_chip_info ad7383_4_chip_info = {
+ .name = "ad7383-4",
+ .channels = ad7383_4_channels,
+ .num_channels = ARRAY_SIZE(ad7383_4_channels),
+ .vcm_supplies = ad7380_4_channel_vcm_supplies,
+ .num_vcm_supplies = ARRAY_SIZE(ad7380_4_channel_vcm_supplies),
+ .available_scan_masks = ad7380_4_channel_scan_masks,
+ .timing_specs = &ad7380_4_timing,
+};
+
+static const struct ad7380_chip_info ad7384_4_chip_info = {
+ .name = "ad7384-4",
+ .channels = ad7384_4_channels,
+ .num_channels = ARRAY_SIZE(ad7384_4_channels),
+ .vcm_supplies = ad7380_4_channel_vcm_supplies,
+ .num_vcm_supplies = ARRAY_SIZE(ad7380_4_channel_vcm_supplies),
+ .available_scan_masks = ad7380_4_channel_scan_masks,
+ .timing_specs = &ad7380_4_timing,
+};
+
struct ad7380_state {
const struct ad7380_chip_info *chip_info;
struct spi_device *spi;
@@ -174,7 +239,7 @@ struct ad7380_state {
* transfer buffers to live in their own cache lines.
* Make the buffer large enough for MAX_NUM_CHANNELS 16-bit samples and one 64-bit
* aligned 64 bit timestamp.
- * As MAX_NUM_CHANNELS is 2 the layout of the structure is the same for all parts
+ * As MAX_NUM_CHANNELS is 4 the layout of the structure is the same for all parts
*/
struct {
u16 raw[MAX_NUM_CHANNELS];
@@ -517,6 +582,10 @@ static const struct of_device_id ad7380_of_match_table[] = {
{ .compatible = "adi,ad7381", .data = &ad7381_chip_info },
{ .compatible = "adi,ad7383", .data = &ad7383_chip_info },
{ .compatible = "adi,ad7384", .data = &ad7384_chip_info },
+ { .compatible = "adi,ad7380-4", .data = &ad7380_4_chip_info },
+ { .compatible = "adi,ad7381-4", .data = &ad7381_4_chip_info },
+ { .compatible = "adi,ad7383-4", .data = &ad7383_4_chip_info },
+ { .compatible = "adi,ad7384-4", .data = &ad7384_4_chip_info },
{ }
};

@@ -525,6 +594,10 @@ static const struct spi_device_id ad7380_id_table[] = {
{ "ad7381", (kernel_ulong_t)&ad7381_chip_info },
{ "ad7383", (kernel_ulong_t)&ad7383_chip_info },
{ "ad7384", (kernel_ulong_t)&ad7384_chip_info },
+ { "ad7380-4", (kernel_ulong_t)&ad7380_4_chip_info },
+ { "ad7381-4", (kernel_ulong_t)&ad7381_4_chip_info },
+ { "ad7383-4", (kernel_ulong_t)&ad7383_4_chip_info },
+ { "ad7384-4", (kernel_ulong_t)&ad7384_4_chip_info },
{ }
};
MODULE_DEVICE_TABLE(spi, ad7380_id_table);

--
2.44.0

2024-05-01 14:58:18

[permalink] [raw]

Subject: [PATCH RFC v6 09/10] iio: adc: ad7380: add support for rolling average oversampling mode

Adds support for rolling average oversampling mode.

Rolling oversampling mode uses a first in, first out (FIFO) buffer of
the most recent samples in the averaging calculation, allowing the ADC
throughput rate and output data rate to stay the same, since we only need
to take only one sample for each new conversion.

The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8
in this mode (vs 1, 2, 4, 8, 16, 32 for the normal average)

In order to be able to change the averaging mode, this commit also adds
the new "oversampling_mode" and "oversampling_mode_available" custom
attributes along with the according documentation file in
Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard
attributes correspond to this use case.

Signed-off-by: Julien Stephan <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 | 38 ++++++
MAINTAINERS | 1 +
drivers/iio/adc/ad7380.c | 143 +++++++++++++++++++--
3 files changed, 174 insertions(+), 8 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380
new file mode 100644
index 000000000000..0a560ef3e32a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380
@@ -0,0 +1,38 @@
+What: /sys/bus/iio/devices/iio:deviceX/oversampling_mode
+KernelVersion: 6.9
+Contact: Michael Hennerich <[email protected]>
+Description:
+ Writing this attribute sets the oversampling average mode.
+ Reading it, shows the configured mode.
+ Available modes can be displayed using the oversampling_mode_available
+ attribute.
+ When writing this attribute to change the oversampling mode, this will
+ have the following side effects:
+
+ - soft reset the ADC to flush the oversampling block and FIFO
+
+ - the available oversampling ratios depend on the oversampling mode
+ configured so to avoid misconfiguration, changing the mode will disable
+ the oversampling by setting the ratio to 1.
+
+ - the list of available ratios (displayed by reading the
+ oversampling_ratio_available attribute) will be updated when changing
+ the oversampling mode.
+
+What: /sys/bus/iio/devices/iio:deviceX/oversampling_mode_available
+KernelVersion: 6.9
+Contact: Michael Hennerich <[email protected]>
+Description:
+ Display the available oversampling average modes. The two available modes
+ are "normal" and "rolling" where "normal" average mode is the default one.
+
+ - normal averaging involves taking a number of samples, adding them
+ together, and dividing the result by the number of samples taken.
+ This result is then output from the device. The sample data is cleared
+ when the process completes. Because we need more samples to output a
+ value, the data output rate decrease with the oversampling ratio.
+
+ - rolling oversampling mode uses a first in, first out (FIFO) buffer of
+ the most recent samples in the averaging calculation, allowing the ADC
+ throughput rate and output data rate to stay the same, since we only need
+ to take only one sample for each new conversion.
diff --git a/MAINTAINERS b/MAINTAINERS
index 87724a9e9f9f..ca1e115f2aff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -434,6 +434,7 @@ R: David Lechner <[email protected]>
S: Supported
W: https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
W: https://ez.analog.com/linux-software-drivers
+F: Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380
F: Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
F: drivers/iio/adc/ad7380.c

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index 1e3869f5e48c..7b021bb9cf87 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -51,6 +51,8 @@
#define AD7380_REG_ADDR_ALERT_HIGH_TH 0x5

#define AD7380_CONFIG1_OS_MODE BIT(9)
+#define OS_MODE_NORMAL_AVERAGE 0
+#define OS_MODE_ROLLING_AVERAGE 1
#define AD7380_CONFIG1_OSR GENMASK(8, 6)
#define AD7380_CONFIG1_CRC_W BIT(5)
#define AD7380_CONFIG1_CRC_R BIT(4)
@@ -159,16 +161,27 @@ static const struct ad7380_timing_specs ad7380_4_timing = {
.t_csh_ns = 20,
};

+/*
+ * Available oversampling modes.
+ */
+static const char * const ad7380_oversampling_average_modes[] = {
+ [OS_MODE_NORMAL_AVERAGE] = "normal",
+ [OS_MODE_ROLLING_AVERAGE] = "rolling",
+};
+
/*
* Available oversampling ratios. The indices correspond
* with the bit value expected by the chip.
- * The available ratios depend on the averaging mode,
- * only normal averaging is supported for now
+ * The available ratios depend on the averaging mode.
*/
static const int ad7380_normal_average_oversampling_ratios[] = {
1, 2, 4, 8, 16, 32,
};

+static const int ad7380_rolling_average_oversampling_ratios[] = {
+ 1, 2, 4, 8,
+};
+
static const struct ad7380_chip_info ad7380_chip_info = {
.name = "ad7380",
.channels = ad7380_channels,
@@ -244,6 +257,7 @@ static const struct ad7380_chip_info ad7384_4_chip_info = {
struct ad7380_state {
const struct ad7380_chip_info *chip_info;
struct spi_device *spi;
+ unsigned int oversampling_mode;
unsigned int oversampling_ratio;
struct regmap *regmap;
unsigned int vref_mv;
@@ -403,7 +417,7 @@ static int ad7380_read_direct(struct ad7380_state *st,
/*
* In normal average oversampling we need to wait for multiple conversions to be done
*/
- if (st->oversampling_ratio > 1)
+ if (st->oversampling_mode == OS_MODE_NORMAL_AVERAGE && st->oversampling_ratio > 1)
xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio;

ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
@@ -462,10 +476,22 @@ static int ad7380_read_avail(struct iio_dev *indio_dev,
const int **vals, int *type, int *length,
long mask)
{
+ struct ad7380_state *st = iio_priv(indio_dev);
+
switch (mask) {
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
- *vals = ad7380_normal_average_oversampling_ratios;
- *length = ARRAY_SIZE(ad7380_normal_average_oversampling_ratios);
+ switch (st->oversampling_mode) {
+ case OS_MODE_NORMAL_AVERAGE:
+ *vals = ad7380_normal_average_oversampling_ratios;
+ *length = ARRAY_SIZE(ad7380_normal_average_oversampling_ratios);
+ break;
+ case OS_MODE_ROLLING_AVERAGE:
+ *vals = ad7380_rolling_average_oversampling_ratios;
+ *length = ARRAY_SIZE(ad7380_rolling_average_oversampling_ratios);
+ break;
+ default:
+ return -EINVAL;
+ }
*type = IIO_VAL_INT;

return IIO_AVAIL_LIST;
@@ -505,9 +531,20 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,

switch (mask) {
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
- osr = check_osr(ad7380_normal_average_oversampling_ratios,
- ARRAY_SIZE(ad7380_normal_average_oversampling_ratios),
- val);
+ switch (st->oversampling_mode) {
+ case OS_MODE_NORMAL_AVERAGE:
+ osr = check_osr(ad7380_normal_average_oversampling_ratios,
+ ARRAY_SIZE(ad7380_normal_average_oversampling_ratios),
+ val);
+ break;
+ case OS_MODE_ROLLING_AVERAGE:
+ osr = check_osr(ad7380_rolling_average_oversampling_ratios,
+ ARRAY_SIZE(ad7380_rolling_average_oversampling_ratios),
+ val);
+ break;
+ default:
+ return -EINVAL;
+ }

if (osr < 0)
return osr;
@@ -538,7 +575,96 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
}
}

+static ssize_t oversampling_mode_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct ad7380_state *st = iio_priv(dev_to_iio_dev(dev));
+ unsigned int os_mode;
+
+ os_mode = st->oversampling_mode;
+
+ return sysfs_emit(buf, "%s\n", ad7380_oversampling_average_modes[os_mode]);
+}
+
+static ssize_t oversampling_mode_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct ad7380_state *st = iio_priv(indio_dev);
+ int os_mode, ret;
+
+ ret = sysfs_match_string(ad7380_oversampling_average_modes, buf);
+ if (ret < 0)
+ return ret;
+
+ os_mode = ret;
+
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
+ AD7380_CONFIG1_OS_MODE,
+ FIELD_PREP(AD7380_CONFIG1_OS_MODE, os_mode));
+
+ if (ret)
+ return ret;
+
+ st->oversampling_mode = os_mode;
+
+ /*
+ * Oversampling ratio depends on oversampling mode, to avoid
+ * misconfiguration when changing oversampling mode,
+ * disable oversampling by setting OSR to 0.
+ */
+ ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
+ AD7380_CONFIG1_OSR, FIELD_PREP(AD7380_CONFIG1_OSR, 0));
+
+ if (ret)
+ return ret;
+
+ st->oversampling_ratio = 1;
+
+ /*
+ * Perform a soft reset.
+ * This will flush the oversampling block and FIFO but will
+ * maintain the content of the configurable registers.
+ */
+ ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
+ AD7380_CONFIG2_RESET,
+ FIELD_PREP(AD7380_CONFIG2_RESET,
+ AD7380_CONFIG2_RESET_SOFT));
+ }
+ return ret ?: len;
+}
+
+static ssize_t oversampling_mode_available_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int i;
+ size_t len = 0;
+
+ for (i = 0; i < ARRAY_SIZE(ad7380_oversampling_average_modes); i++)
+ len += sysfs_emit_at(buf, len, "%s ", ad7380_oversampling_average_modes[i]);
+
+ buf[len - 1] = '\n';
+
+ return len;
+}
+
+static IIO_DEVICE_ATTR_RW(oversampling_mode, 0);
+static IIO_DEVICE_ATTR_RO(oversampling_mode_available, 0);
+
+static struct attribute *ad7380_attributes[] = {
+ &iio_dev_attr_oversampling_mode.dev_attr.attr,
+ &iio_dev_attr_oversampling_mode_available.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group ad7380_attribute_group = {
+ .attrs = ad7380_attributes,
+};
+
static const struct iio_info ad7380_info = {
+ .attrs = &ad7380_attribute_group,
.read_raw = &ad7380_read_raw,
.read_avail = &ad7380_read_avail,
.write_raw = &ad7380_write_raw,
@@ -569,6 +695,7 @@ static int ad7380_init(struct ad7380_state *st, struct regulator *vref)
* This is the default value after reset,
* so just initialize internal data
*/
+ st->oversampling_mode = OS_MODE_NORMAL_AVERAGE;
st->oversampling_ratio = 1;

/* SPI 1-wire mode */

--
2.44.0

2024-05-01 14:58:40

[permalink] [raw]

Subject: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for resolution boost

ad738x chips are able to use an additional 2 bits of resolution when
using oversampling. The 14-bits chips can have up to 16 bits of
resolution and the 16-bits chips can have up to 18 bits of resolution.

In order to dynamically allow to enable/disable the resolution boost
feature, we have to set iio realbits/storagebits to the maximum per chips.
This means that for iio, data will always be on the higher resolution
available, and to cope with that we adjust the iio scale and iio offset
depending on the resolution boost status.

The available scales can be displayed using the regular _scale_available
attributes and can be set by writing the regular _scale attribute.
The available scales depend on the oversampling status.

Signed-off-by: Julien Stephan <[email protected]>

---

In order to support the resolution boost (additional 2 bits of resolution)
we need to set realbits/storagebits to the maximum value i.e :
- realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
- realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips

For 14-bits chips this does not have a major impact, but this
has the drawback of forcing 16-bits chip users to always use 32-bits
words in buffers even if they are not using oversampling and resolution
boost. Is there a better way of implementing this? For example
implementing dynamic scan_type?

Another issue is the location of the timestamps. I understood the need
for ts to be consistent between chips, but right now I do not have a
better solution.. I was thinking of maybe adding the timestamps at the
beginning? That would imply to change the iio_chan_spec struct and maybe
add a iio_push_to_buffers_with_timestamp_first() wrapper function? Is
that an option?

Any suggestion would be very much appreciated.
---
drivers/iio/adc/ad7380.c | 226 ++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 194 insertions(+), 32 deletions(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index 7b021bb9cf87..e240098708e9 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -20,6 +20,7 @@
#include <linux/err.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/units.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
#include <linux/slab.h>
@@ -58,6 +59,8 @@
#define AD7380_CONFIG1_CRC_R BIT(4)
#define AD7380_CONFIG1_ALERTEN BIT(3)
#define AD7380_CONFIG1_RES BIT(2)
+#define RESOLUTION_BOOST_DISABLE 0
+#define RESOLUTION_BOOST_ENABLE 1
#define AD7380_CONFIG1_REFSEL BIT(1)
#define AD7380_CONFIG1_PMODE BIT(0)

@@ -86,6 +89,14 @@ struct ad7380_chip_info {
const struct ad7380_timing_specs *timing_specs;
};

+/*
+ * realbits/storagebits cannot be dynamically changed, so in order to
+ * support the resolution boost (additional 2 bits of resolution)
+ * we need to set realbits/storagebits to the maximum value i.e :
+ * - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
+ * - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
+ * We need to adjust the scale depending on resolution boost status
+ */
#define AD7380_CHANNEL(index, bits, diff) { \
.type = IIO_VOLTAGE, \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
@@ -93,6 +104,7 @@ struct ad7380_chip_info {
.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
.info_mask_shared_by_type_available = \
+ BIT(IIO_CHAN_INFO_SCALE) | \
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
.indexed = 1, \
.differential = (diff), \
@@ -101,8 +113,8 @@ struct ad7380_chip_info {
.scan_index = (index), \
.scan_type = { \
.sign = 's', \
- .realbits = (bits), \
- .storagebits = 16, \
+ .realbits = (bits) + 2, \
+ .storagebits = ((bits) + 2 > 16) ? 32 : 16, \
.endianness = IIO_CPU, \
}, \
}
@@ -259,6 +271,8 @@ struct ad7380_state {
struct spi_device *spi;
unsigned int oversampling_mode;
unsigned int oversampling_ratio;
+ unsigned int scales[2][2];
+ bool resolution_boost_enable;
struct regmap *regmap;
unsigned int vref_mv;
unsigned int vcm_mv[MAX_NUM_CHANNELS];
@@ -270,7 +284,10 @@ struct ad7380_state {
* As MAX_NUM_CHANNELS is 4 the layout of the structure is the same for all parts
*/
struct {
- u16 raw[MAX_NUM_CHANNELS];
+ union {
+ u16 u16[MAX_NUM_CHANNELS];
+ u32 u32[MAX_NUM_CHANNELS];
+ } raw;

s64 ts __aligned(8);
} scan_data __aligned(IIO_DMA_MINALIGN);
@@ -359,23 +376,67 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
unreachable();
}

+static int ad7380_prepare_spi_xfer(struct ad7380_state *st, struct spi_transfer *xfer)
+{
+ int bits_per_word;
+
+ memset(xfer, 0, sizeof(*xfer));
+
+ xfer->rx_buf = &st->scan_data.raw;
+
+ if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
+ bits_per_word = st->chip_info->channels[0].scan_type.realbits;
+ else
+ bits_per_word = st->chip_info->channels[0].scan_type.realbits - 2;
+
+ xfer->bits_per_word = bits_per_word;
+
+ xfer->len = (st->chip_info->num_channels - 1) * BITS_TO_BYTES(bits_per_word);
+
+ return bits_per_word;
+}
+
static irqreturn_t ad7380_trigger_handler(int irq, void *p)
{
struct iio_poll_func *pf = p;
struct iio_dev *indio_dev = pf->indio_dev;
struct ad7380_state *st = iio_priv(indio_dev);
- struct spi_transfer xfer = {
- .bits_per_word = st->chip_info->channels[0].scan_type.realbits,
- .len = (st->chip_info->num_channels - 1) *
- BITS_TO_BYTES(st->chip_info->channels->scan_type.storagebits),
- .rx_buf = st->scan_data.raw,
- };
- int ret;
+ struct spi_transfer xfer;
+ int bits_per_word, realbits, i, ret;
+
+ realbits = st->chip_info->channels[0].scan_type.realbits;
+ bits_per_word = ad7380_prepare_spi_xfer(st, &xfer);

ret = spi_sync_transfer(st->spi, &xfer, 1);
if (ret)
goto out;

+ /*
+ * If bits_per_word == realbits (resolution boost enabled), we don't
+ * need to manipulate the raw data, otherwise we may need to fix things
+ * up a bit to fit the scan_type specs
+ */
+ if (bits_per_word < realbits) {
+ if (realbits > 16 && bits_per_word <= 16) {
+ /*
+ * Here realbits > 16 so storagebits is 32 and bits_per_word is <= 16
+ * so we need to sign extend u16 to u32 using reverse order to
+ * avoid writing over union data
+ */
+ for (i = st->chip_info->num_channels - 2; i >= 0; i--)
+ st->scan_data.raw.u32[i] = sign_extend32(st->scan_data.raw.u16[i],
+ bits_per_word - 1);
+ } else if (bits_per_word < 16) {
+ /*
+ * Here realbits <= 16 so storagebits is 16.
+ * We only need to sign extend only if bits_per_word is < 16
+ */
+ for (i = 0; i < st->chip_info->num_channels - 1; i++)
+ st->scan_data.raw.u16[i] = sign_extend32(st->scan_data.raw.u16[i],
+ bits_per_word - 1);
+ }
+ }
+
iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
pf->timestamp);

@@ -388,7 +449,7 @@ static irqreturn_t ad7380_trigger_handler(int irq, void *p)
static int ad7380_read_direct(struct ad7380_state *st,
struct iio_chan_spec const *chan, int *val)
{
- struct spi_transfer xfers[] = {
+ struct spi_transfer xfers[2] = {
/* toggle CS (no data xfer) to trigger a conversion */
{
.speed_hz = AD7380_REG_WR_SPEED_HZ,
@@ -403,16 +464,11 @@ static int ad7380_read_direct(struct ad7380_state *st,
.unit = SPI_DELAY_UNIT_NSECS,
},
},
- /* then read all channels */
+ /* then read all channels, it will be filled by ad7380_prepare_spi_xfer */
{
- .speed_hz = AD7380_REG_WR_SPEED_HZ,
- .bits_per_word = chan->scan_type.realbits,
- .rx_buf = st->scan_data.raw,
- .len = (st->chip_info->num_channels - 1) *
- ((chan->scan_type.storagebits > 16) ? 4 : 2),
},
};
- int ret;
+ int bits_per_word, ret;

/*
* In normal average oversampling we need to wait for multiple conversions to be done
@@ -420,12 +476,18 @@ static int ad7380_read_direct(struct ad7380_state *st,
if (st->oversampling_mode == OS_MODE_NORMAL_AVERAGE && st->oversampling_ratio > 1)
xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio;

+ bits_per_word = ad7380_prepare_spi_xfer(st, &xfers[1]);
+
ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
if (ret < 0)
return ret;

- *val = sign_extend32(st->scan_data.raw[chan->scan_index],
- chan->scan_type.realbits - 1);
+ if (bits_per_word > 16)
+ *val = sign_extend32(st->scan_data.raw.u32[chan->scan_index],
+ bits_per_word - 1);
+ else
+ *val = sign_extend32(st->scan_data.raw.u16[chan->scan_index],
+ bits_per_word - 1);

return IIO_VAL_INT;
}
@@ -435,6 +497,12 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
int *val, int *val2, long info)
{
struct ad7380_state *st = iio_priv(indio_dev);
+ int realbits;
+
+ if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
+ realbits = chan->scan_type.realbits;
+ else
+ realbits = chan->scan_type.realbits - 2;

switch (info) {
case IIO_CHAN_INFO_RAW:
@@ -443,22 +511,16 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
}
unreachable();
case IIO_CHAN_INFO_SCALE:
- /*
- * According to the datasheet, the LSB size is:
- * * (2 × VREF) / 2^N, for differential chips
- * * VREF / 2^N, for pseudo-differential chips
- * where N is the ADC resolution (i.e realbits)
- */
- *val = st->vref_mv;
- *val2 = chan->scan_type.realbits - chan->differential;
+ *val = st->scales[st->resolution_boost_enable][0];
+ *val2 = st->scales[st->resolution_boost_enable][1];

- return IIO_VAL_FRACTIONAL_LOG2;
+ return IIO_VAL_INT_PLUS_MICRO;
case IIO_CHAN_INFO_OFFSET:
/*
* According to IIO ABI, offset is applied before scale,
* so offset is: vcm_mv / scale
*/
- *val = st->vcm_mv[chan->channel] * (1 << chan->scan_type.realbits)
+ *val = st->vcm_mv[chan->channel] * (1 << realbits)
/ st->vref_mv;

return IIO_VAL_INT;
@@ -494,6 +556,24 @@ static int ad7380_read_avail(struct iio_dev *indio_dev,
}
*type = IIO_VAL_INT;

+ return IIO_AVAIL_LIST;
+ case IIO_CHAN_INFO_SCALE:
+ *vals = (const int *)st->scales[0];
+ /*
+ * Values are stored into a 2D matrix.
+ * We have 2 available scales depending on the resolution boost status
+ * (enabled/disabled). Resolution boost can be enabled only when oversampling
+ * is enabled (i.e OSR > 1).
+ * So depending on the oversampling ratio we display only the currently
+ * available scales. First scale is for normal mode, second scale is for resolution
+ * boost enabled.
+ */
+ if (st->oversampling_ratio > 1)
+ *length = 4;
+ else
+ *length = 2;
+ *type = IIO_VAL_INT_PLUS_MICRO;
+
return IIO_AVAIL_LIST;
default:
return -EINVAL;
@@ -522,6 +602,25 @@ static inline int check_osr(const int *available_ratio, int size, int ratio)
return -EINVAL;
}

+static int ad7380_set_resolution_boost(struct iio_dev *indio_dev, bool enable)
+{
+ struct ad7380_state *st = iio_priv(indio_dev);
+ int ret;
+
+ if (st->resolution_boost_enable == enable)
+ return 0;
+
+ ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
+ AD7380_CONFIG1_RES,
+ FIELD_PREP(AD7380_CONFIG1_RES, enable));
+
+ if (ret)
+ return ret;
+
+ st->resolution_boost_enable = enable;
+ return 0;
+}
+
static int ad7380_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan, int val,
int val2, long mask)
@@ -559,6 +658,13 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,

st->oversampling_ratio = val;

+ /*
+ * If oversampling is disabled (OSR == 1),
+ * we need to disable resolution boost
+ */
+ if (st->oversampling_ratio == 1)
+ ad7380_set_resolution_boost(indio_dev, RESOLUTION_BOOST_DISABLE);
+
/*
* Perform a soft reset.
* This will flush the oversampling block and FIFO but will
@@ -570,6 +676,25 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
AD7380_CONFIG2_RESET_SOFT));
}
return 0;
+ case IIO_CHAN_INFO_SCALE:
+ if (val == st->scales[RESOLUTION_BOOST_DISABLE][0] &&
+ val2 == st->scales[RESOLUTION_BOOST_DISABLE][1]) {
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ return ad7380_set_resolution_boost(indio_dev,
+ RESOLUTION_BOOST_DISABLE);
+ }
+ unreachable();
+ }
+ if (st->oversampling_ratio > 1 &&
+ val == st->scales[RESOLUTION_BOOST_ENABLE][0] &&
+ val2 == st->scales[RESOLUTION_BOOST_ENABLE][1]) {
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ return ad7380_set_resolution_boost(indio_dev,
+ RESOLUTION_BOOST_ENABLE);
+ }
+ unreachable();
+ }
+ return -EINVAL;
default:
return -EINVAL;
}
@@ -614,6 +739,8 @@ static ssize_t oversampling_mode_store(struct device *dev,
* Oversampling ratio depends on oversampling mode, to avoid
* misconfiguration when changing oversampling mode,
* disable oversampling by setting OSR to 0.
+ * Since we disable the oversampling, we also need to
+ * disable the resolution boost
*/
ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
AD7380_CONFIG1_OSR, FIELD_PREP(AD7380_CONFIG1_OSR, 0));
@@ -622,6 +749,7 @@ static ssize_t oversampling_mode_store(struct device *dev,
return ret;

st->oversampling_ratio = 1;
+ ad7380_set_resolution_boost(indio_dev, RESOLUTION_BOOST_DISABLE);

/*
* Perform a soft reset.
@@ -671,6 +799,36 @@ static const struct iio_info ad7380_info = {
.debugfs_reg_access = &ad7380_debugfs_reg_access,
};

+static void ad7380_init_available_scales(struct ad7380_state *st)
+{
+ s64 tmp;
+ int value_micro, value_int, realbits, differential;
+
+ /*
+ * Resolution boost allow to enable 2 higher bits resolution
+ * when oversampling is enabled, so we can have only two
+ * scales depending on the resolution boost status.
+ */
+ realbits = st->chip_info->channels[0].scan_type.realbits;
+ differential = st->chip_info->channels[0].differential;
+
+ /*
+ * According to the datasheet, the LSB size is:
+ * * (2 × VREF) / 2^N, for differential chips
+ * * VREF / 2^N, for pseudo-differential chips
+ * where N is the ADC resolution (i.e realbits)
+ */
+ tmp = (s64)st->vref_mv * MEGA >> (realbits - 2 - differential);
+ value_int = div_s64_rem(tmp, MEGA, &value_micro);
+ st->scales[RESOLUTION_BOOST_DISABLE][0] = value_int;
+ st->scales[RESOLUTION_BOOST_DISABLE][1] = value_micro;
+
+ tmp = (s64)st->vref_mv * MEGA >> (realbits - differential);
+ value_int = div_s64_rem(tmp, MEGA, &value_micro);
+ st->scales[RESOLUTION_BOOST_ENABLE][0] = value_int;
+ st->scales[RESOLUTION_BOOST_ENABLE][1] = value_micro;
+}
+
static int ad7380_init(struct ad7380_state *st, struct regulator *vref)
{
int ret;
@@ -691,12 +849,16 @@ static int ad7380_init(struct ad7380_state *st, struct regulator *vref)
if (ret < 0)
return ret;

- /* Disable oversampling by default.
- * This is the default value after reset,
+ /* Disable oversampling and resolution boost by default.
+ * This are the default values after reset,
* so just initialize internal data
*/
st->oversampling_mode = OS_MODE_NORMAL_AVERAGE;
st->oversampling_ratio = 1;
+ st->resolution_boost_enable = RESOLUTION_BOOST_DISABLE;
+
+ /* initialize available scales */
+ ad7380_init_available_scales(st);

/* SPI 1-wire mode */
return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,

--
2.44.0

2024-05-01 14:58:55

[permalink] [raw]

Subject: [PATCH RFC v6 02/10] iio: adc: ad7380: new driver for AD7380 ADCs

From: David Lechner <[email protected]>

This adds a new driver for the AD7380 family ADCs.

The driver currently implements basic support for the AD7380, AD7381,
2-channel differential ADCs. Support for additional single-ended,
pseudo-differential and 4-channel chips that use the same register map
as well as additional features of the chip will be added in future patches.

Co-developed-by: Stefan Popa <[email protected]>
Signed-off-by: Stefan Popa <[email protected]>
Reviewed-by: Nuno Sa <[email protected]>
Signed-off-by: David Lechner <[email protected]>
[Julien Stephan: add datasheet links of supported parts]
[Julien Stephan: fix rx/tx buffer for regmap access]
[Julien Stephan: fix scale issue]
[Julien Stephan: use the new iio_device_claim_direct_scoped
instead of iio_device_claim_direct_mode]
Signed-off-by: Julien Stephan <[email protected]>
---
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 16 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad7380.c | 443 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 461 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5bb168937853..87724a9e9f9f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -435,6 +435,7 @@ S: Supported
W: https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
+F: drivers/iio/adc/ad7380.c

AD7877 TOUCHSCREEN DRIVER
M: Michael Hennerich <[email protected]>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..631386b037ae 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -155,6 +155,22 @@ config AD7298
To compile this driver as a module, choose M here: the
module will be called ad7298.

+config AD7380
+ tristate "Analog Devices AD7380 ADC driver"
+ depends on SPI_MASTER
+ select IIO_BUFFER
+ select IIO_TRIGGER
+ select IIO_TRIGGERED_BUFFER
+ help
+ AD7380 is a family of simultaneous sampling ADCs that share the same
+ SPI register map and have similar pinouts.
+
+ Say yes here to build support for Analog Devices AD7380 ADC and
+ similar chips.
+
+ To compile this driver as a module, choose M here: the module will be
+ called ad7380.
+
config AD7476
tristate "Analog Devices AD7476 1-channel ADCs driver and other similar devices from AD and TI"
depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index edb32ce2af02..bd3cbbb178fa 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_AD7291) += ad7291.o
obj-$(CONFIG_AD7292) += ad7292.o
obj-$(CONFIG_AD7298) += ad7298.o
obj-$(CONFIG_AD7923) += ad7923.o
+obj-$(CONFIG_AD7380) += ad7380.o
obj-$(CONFIG_AD7476) += ad7476.o
obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
new file mode 100644
index 000000000000..a218f59c276e
--- /dev/null
+++ b/drivers/iio/adc/ad7380.c
@@ -0,0 +1,443 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD738x Simultaneous Sampling SAR ADCs
+ *
+ * Copyright 2017 Analog Devices Inc.
+ * Copyright 2024 BayLibre, SAS
+ *
+ * Datasheets of supported parts:
+ * ad7380/1 : https://www.analog.com/media/en/technical-documentation/data-sheets/AD7380-7381.pdf
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+/* 2.5V internal reference voltage */
+#define AD7380_INTERNAL_REF_MV 2500
+
+/* reading and writing registers is more reliable at lower than max speed */
+#define AD7380_REG_WR_SPEED_HZ 10000000
+
+#define AD7380_REG_WR BIT(15)
+#define AD7380_REG_REGADDR GENMASK(14, 12)
+#define AD7380_REG_DATA GENMASK(11, 0)
+
+#define AD7380_REG_ADDR_NOP 0x0
+#define AD7380_REG_ADDR_CONFIG1 0x1
+#define AD7380_REG_ADDR_CONFIG2 0x2
+#define AD7380_REG_ADDR_ALERT 0x3
+#define AD7380_REG_ADDR_ALERT_LOW_TH 0x4
+#define AD7380_REG_ADDR_ALERT_HIGH_TH 0x5
+
+#define AD7380_CONFIG1_OS_MODE BIT(9)
+#define AD7380_CONFIG1_OSR GENMASK(8, 6)
+#define AD7380_CONFIG1_CRC_W BIT(5)
+#define AD7380_CONFIG1_CRC_R BIT(4)
+#define AD7380_CONFIG1_ALERTEN BIT(3)
+#define AD7380_CONFIG1_RES BIT(2)
+#define AD7380_CONFIG1_REFSEL BIT(1)
+#define AD7380_CONFIG1_PMODE BIT(0)
+
+#define AD7380_CONFIG2_SDO2 GENMASK(9, 8)
+#define AD7380_CONFIG2_SDO BIT(8)
+#define AD7380_CONFIG2_RESET GENMASK(7, 0)
+
+#define AD7380_CONFIG2_RESET_SOFT 0x3C
+#define AD7380_CONFIG2_RESET_HARD 0xFF
+
+#define AD7380_ALERT_LOW_TH GENMASK(11, 0)
+#define AD7380_ALERT_HIGH_TH GENMASK(11, 0)
+
+struct ad7380_chip_info {
+ const char *name;
+ const struct iio_chan_spec *channels;
+ unsigned int num_channels;
+};
+
+#define AD7380_CHANNEL(index, bits) { \
+ .type = IIO_VOLTAGE, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+ .indexed = 1, \
+ .differential = 1, \
+ .channel = 2 * (index), \
+ .channel2 = 2 * (index) + 1, \
+ .scan_index = (index), \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = (bits), \
+ .storagebits = 16, \
+ .endianness = IIO_CPU, \
+ }, \
+}
+
+#define DEFINE_AD7380_2_CHANNEL(name, bits) \
+static const struct iio_chan_spec name[] = { \
+ AD7380_CHANNEL(0, bits), \
+ AD7380_CHANNEL(1, bits), \
+ IIO_CHAN_SOFT_TIMESTAMP(2), \
+}
+
+DEFINE_AD7380_2_CHANNEL(ad7380_channels, 16);
+DEFINE_AD7380_2_CHANNEL(ad7381_channels, 14);
+
+/* Since this is simultaneous sampling, we don't allow individual channels. */
+static const unsigned long ad7380_2_channel_scan_masks[] = {
+ GENMASK(1, 0),
+ 0
+};
+
+static const struct ad7380_chip_info ad7380_chip_info = {
+ .name = "ad7380",
+ .channels = ad7380_channels,
+ .num_channels = ARRAY_SIZE(ad7380_channels),
+};
+
+static const struct ad7380_chip_info ad7381_chip_info = {
+ .name = "ad7381",
+ .channels = ad7381_channels,
+ .num_channels = ARRAY_SIZE(ad7381_channels),
+};
+
+struct ad7380_state {
+ const struct ad7380_chip_info *chip_info;
+ struct spi_device *spi;
+ struct regmap *regmap;
+ unsigned int vref_mv;
+ /*
+ * DMA (thus cache coherency maintenance) requires the
+ * transfer buffers to live in their own cache lines.
+ * Make the buffer large enough for 2 16-bit samples and one 64-bit
+ * aligned 64 bit timestamp.
+ */
+ struct {
+ u16 raw[2];
+
+ s64 ts __aligned(8);
+ } scan_data __aligned(IIO_DMA_MINALIGN);
+ u16 tx;
+ u16 rx;
+};
+
+static int ad7380_regmap_reg_write(void *context, unsigned int reg,
+ unsigned int val)
+{
+ struct ad7380_state *st = context;
+ struct spi_transfer xfer = {
+ .speed_hz = AD7380_REG_WR_SPEED_HZ,
+ .bits_per_word = 16,
+ .len = 2,
+ .tx_buf = &st->tx,
+ };
+
+ st->tx = FIELD_PREP(AD7380_REG_WR, 1) |
+ FIELD_PREP(AD7380_REG_REGADDR, reg) |
+ FIELD_PREP(AD7380_REG_DATA, val);
+
+ return spi_sync_transfer(st->spi, &xfer, 1);
+}
+
+static int ad7380_regmap_reg_read(void *context, unsigned int reg,
+ unsigned int *val)
+{
+ struct ad7380_state *st = context;
+ struct spi_transfer xfers[] = {
+ {
+ .speed_hz = AD7380_REG_WR_SPEED_HZ,
+ .bits_per_word = 16,
+ .len = 2,
+ .tx_buf = &st->tx,
+ .cs_change = 1,
+ .cs_change_delay = {
+ .value = 10, /* t[CSH] */
+ .unit = SPI_DELAY_UNIT_NSECS,
+ },
+ }, {
+ .speed_hz = AD7380_REG_WR_SPEED_HZ,
+ .bits_per_word = 16,
+ .len = 2,
+ .rx_buf = &st->rx,
+ },
+ };
+ int ret;
+
+ st->tx = FIELD_PREP(AD7380_REG_WR, 0) |
+ FIELD_PREP(AD7380_REG_REGADDR, reg) |
+ FIELD_PREP(AD7380_REG_DATA, 0);
+
+ ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+ if (ret < 0)
+ return ret;
+
+ *val = FIELD_GET(AD7380_REG_DATA, st->rx);
+
+ return 0;
+}
+
+static const struct regmap_config ad7380_regmap_config = {
+ .reg_bits = 3,
+ .val_bits = 12,
+ .reg_read = ad7380_regmap_reg_read,
+ .reg_write = ad7380_regmap_reg_write,
+ .max_register = AD7380_REG_ADDR_ALERT_HIGH_TH,
+ .can_sleep = true,
+};
+
+static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
+ u32 writeval, u32 *readval)
+{
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ struct ad7380_state *st = iio_priv(indio_dev);
+ int ret;
+
+ if (readval)
+ ret = regmap_read(st->regmap, reg, readval);
+ else
+ ret = regmap_write(st->regmap, reg, writeval);
+
+ return ret;
+ }
+ unreachable();
+}
+
+static irqreturn_t ad7380_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct ad7380_state *st = iio_priv(indio_dev);
+ struct spi_transfer xfer = {
+ .bits_per_word = st->chip_info->channels[0].scan_type.realbits,
+ .len = 4,
+ .rx_buf = st->scan_data.raw,
+ };
+ int ret;
+
+ ret = spi_sync_transfer(st->spi, &xfer, 1);
+ if (ret)
+ goto out;
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
+ pf->timestamp);
+
+out:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static int ad7380_read_direct(struct ad7380_state *st,
+ struct iio_chan_spec const *chan, int *val)
+{
+ struct spi_transfer xfers[] = {
+ /* toggle CS (no data xfer) to trigger a conversion */
+ {
+ .speed_hz = AD7380_REG_WR_SPEED_HZ,
+ .bits_per_word = chan->scan_type.realbits,
+ .delay = {
+ .value = 190, /* t[CONVERT] */
+ .unit = SPI_DELAY_UNIT_NSECS,
+ },
+ .cs_change = 1,
+ .cs_change_delay = {
+ .value = 10, /* t[CSH] */
+ .unit = SPI_DELAY_UNIT_NSECS,
+ },
+ },
+ /* then read both channels */
+ {
+ .speed_hz = AD7380_REG_WR_SPEED_HZ,
+ .bits_per_word = chan->scan_type.realbits,
+ .rx_buf = st->scan_data.raw,
+ .len = 4,
+ },
+ };
+ int ret;
+
+ ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+ if (ret < 0)
+ return ret;
+
+ *val = sign_extend32(st->scan_data.raw[chan->scan_index],
+ chan->scan_type.realbits - 1);
+
+ return IIO_VAL_INT;
+}
+
+static int ad7380_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long info)
+{
+ struct ad7380_state *st = iio_priv(indio_dev);
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ return ad7380_read_direct(st, chan, val);
+ }
+ unreachable();
+ case IIO_CHAN_INFO_SCALE:
+ /*
+ * According to the datasheet, the LSB size for fully differential ADC is
+ * (2 × VREF) / 2^N, where N is the ADC resolution (i.e realbits)
+ */
+ *val = st->vref_mv;
+ *val2 = chan->scan_type.realbits - 1;
+
+ return IIO_VAL_FRACTIONAL_LOG2;
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info ad7380_info = {
+ .read_raw = &ad7380_read_raw,
+ .debugfs_reg_access = &ad7380_debugfs_reg_access,
+};
+
+static int ad7380_init(struct ad7380_state *st, struct regulator *vref)
+{
+ int ret;
+
+ /* perform hard reset */
+ ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
+ AD7380_CONFIG2_RESET,
+ FIELD_PREP(AD7380_CONFIG2_RESET,
+ AD7380_CONFIG2_RESET_HARD));
+ if (ret < 0)
+ return ret;
+
+ /* select internal or external reference voltage */
+ ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
+ AD7380_CONFIG1_REFSEL,
+ FIELD_PREP(AD7380_CONFIG1_REFSEL,
+ vref ? 1 : 0));
+ if (ret < 0)
+ return ret;
+
+ /* SPI 1-wire mode */
+ return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
+ AD7380_CONFIG2_SDO,
+ FIELD_PREP(AD7380_CONFIG2_SDO, 1));
+}
+
+static void ad7380_regulator_disable(void *p)
+{
+ regulator_disable(p);
+}
+
+static int ad7380_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct ad7380_state *st;
+ struct regulator *vref;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ st = iio_priv(indio_dev);
+ st->spi = spi;
+ st->chip_info = spi_get_device_match_data(spi);
+ if (!st->chip_info)
+ return dev_err_probe(&spi->dev, -EINVAL, "missing match data\n");
+
+ vref = devm_regulator_get_optional(&spi->dev, "refio");
+ if (IS_ERR(vref)) {
+ if (PTR_ERR(vref) != -ENODEV)
+ return dev_err_probe(&spi->dev, PTR_ERR(vref),
+ "Failed to get refio regulator\n");
+
+ vref = NULL;
+ }
+
+ /*
+ * If there is no REFIO supply, then it means that we are using
+ * the internal 2.5V reference, otherwise REFIO is reference voltage.
+ */
+ if (vref) {
+ ret = regulator_enable(vref);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(&spi->dev,
+ ad7380_regulator_disable, vref);
+ if (ret)
+ return ret;
+
+ ret = regulator_get_voltage(vref);
+ if (ret < 0)
+ return ret;
+
+ st->vref_mv = ret / 1000;
+ } else {
+ st->vref_mv = AD7380_INTERNAL_REF_MV;
+ }
+
+ st->regmap = devm_regmap_init(&spi->dev, NULL, st, &ad7380_regmap_config);
+ if (IS_ERR(st->regmap))
+ return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
+ "failed to allocate register map\n");
+
+ indio_dev->channels = st->chip_info->channels;
+ indio_dev->num_channels = st->chip_info->num_channels;
+ indio_dev->name = st->chip_info->name;
+ indio_dev->info = &ad7380_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->available_scan_masks = ad7380_2_channel_scan_masks;
+
+ ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+ iio_pollfunc_store_time,
+ ad7380_trigger_handler, NULL);
+ if (ret)
+ return ret;
+
+ ret = ad7380_init(st, vref);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id ad7380_of_match_table[] = {
+ { .compatible = "adi,ad7380", .data = &ad7380_chip_info },
+ { .compatible = "adi,ad7381", .data = &ad7381_chip_info },
+ { }
+};
+
+static const struct spi_device_id ad7380_id_table[] = {
+ { "ad7380", (kernel_ulong_t)&ad7380_chip_info },
+ { "ad7381", (kernel_ulong_t)&ad7381_chip_info },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, ad7380_id_table);
+
+static struct spi_driver ad7380_driver = {
+ .driver = {
+ .name = "ad7380",
+ .of_match_table = ad7380_of_match_table,
+ },
+ .probe = ad7380_probe,
+ .id_table = ad7380_id_table,
+};
+module_spi_driver(ad7380_driver);
+
+MODULE_AUTHOR("Stefan Popa <[email protected]>");
+MODULE_DESCRIPTION("Analog Devices AD738x ADC driver");
+MODULE_LICENSE("GPL");

--
2.44.0

2024-05-01 14:59:35

[permalink] [raw]

Subject: [PATCH RFC v6 05/10] iio: adc: ad7380: prepare for parts with more channels

The current driver supports only parts with 2 channels.
In order to prepare the support of new compatible ADCs with more
channels, this commit:
- defines MAX_NUM_CHANNEL to specify the maximum number of
channels currently supported by the driver
- adds available_scan_mask member in ad7380_chip_info structure
- fixes spi xfer struct len depending on number of channels
- fixes scan_data.raw buffer size to handle more channels
- adds a timing specifications structure in ad7380_chip_info structure

Signed-off-by: Julien Stephan <[email protected]>
---
drivers/iio/adc/ad7380.c | 43 +++++++++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index d6abce6d45d3..201006d878f1 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -29,6 +29,7 @@
#include <linux/iio/trigger_consumer.h>
#include <linux/iio/triggered_buffer.h>

+#define MAX_NUM_CHANNELS 2
/* 2.5V internal reference voltage */
#define AD7380_INTERNAL_REF_MV 2500

@@ -65,12 +66,19 @@
#define AD7380_ALERT_LOW_TH GENMASK(11, 0)
#define AD7380_ALERT_HIGH_TH GENMASK(11, 0)

+#define T_CONVERT_NS 190 /* conversion time */
+struct ad7380_timing_specs {
+ const unsigned int t_csh_ns; /* CS minimum high time */
+};
+
struct ad7380_chip_info {
const char *name;
const struct iio_chan_spec *channels;
unsigned int num_channels;
const char * const *vcm_supplies;
unsigned int num_vcm_supplies;
+ const unsigned long *available_scan_masks;
+ const struct ad7380_timing_specs *timing_specs;
};

#define AD7380_CHANNEL(index, bits, diff) { \
@@ -115,16 +123,24 @@ static const unsigned long ad7380_2_channel_scan_masks[] = {
0
};

+static const struct ad7380_timing_specs ad7380_timing = {
+ .t_csh_ns = 10,
+};
+
static const struct ad7380_chip_info ad7380_chip_info = {
.name = "ad7380",
.channels = ad7380_channels,
.num_channels = ARRAY_SIZE(ad7380_channels),
+ .available_scan_masks = ad7380_2_channel_scan_masks,
+ .timing_specs = &ad7380_timing,
};

static const struct ad7380_chip_info ad7381_chip_info = {
.name = "ad7381",
.channels = ad7381_channels,
.num_channels = ARRAY_SIZE(ad7381_channels),
+ .available_scan_masks = ad7380_2_channel_scan_masks,
+ .timing_specs = &ad7380_timing,
};

static const struct ad7380_chip_info ad7383_chip_info = {
@@ -133,6 +149,8 @@ static const struct ad7380_chip_info ad7383_chip_info = {
.num_channels = ARRAY_SIZE(ad7383_channels),
.vcm_supplies = ad7380_2_channel_vcm_supplies,
.num_vcm_supplies = ARRAY_SIZE(ad7380_2_channel_vcm_supplies),
+ .available_scan_masks = ad7380_2_channel_scan_masks,
+ .timing_specs = &ad7380_timing,
};

static const struct ad7380_chip_info ad7384_chip_info = {
@@ -141,6 +159,8 @@ static const struct ad7380_chip_info ad7384_chip_info = {
.num_channels = ARRAY_SIZE(ad7384_channels),
.vcm_supplies = ad7380_2_channel_vcm_supplies,
.num_vcm_supplies = ARRAY_SIZE(ad7380_2_channel_vcm_supplies),
+ .available_scan_masks = ad7380_2_channel_scan_masks,
+ .timing_specs = &ad7380_timing,
};

struct ad7380_state {
@@ -148,15 +168,16 @@ struct ad7380_state {
struct spi_device *spi;
struct regmap *regmap;
unsigned int vref_mv;
- unsigned int vcm_mv[2];
+ unsigned int vcm_mv[MAX_NUM_CHANNELS];
/*
* DMA (thus cache coherency maintenance) requires the
* transfer buffers to live in their own cache lines.
- * Make the buffer large enough for 2 16-bit samples and one 64-bit
+ * Make the buffer large enough for MAX_NUM_CHANNELS 16-bit samples and one 64-bit
* aligned 64 bit timestamp.
+ * As MAX_NUM_CHANNELS is 2 the layout of the structure is the same for all parts
*/
struct {
- u16 raw[2];
+ u16 raw[MAX_NUM_CHANNELS];

s64 ts __aligned(8);
} scan_data __aligned(IIO_DMA_MINALIGN);
@@ -194,7 +215,7 @@ static int ad7380_regmap_reg_read(void *context, unsigned int reg,
.tx_buf = &st->tx,
.cs_change = 1,
.cs_change_delay = {
- .value = 10, /* t[CSH] */
+ .value = st->chip_info->timing_specs->t_csh_ns,
.unit = SPI_DELAY_UNIT_NSECS,
},
}, {
@@ -252,7 +273,8 @@ static irqreturn_t ad7380_trigger_handler(int irq, void *p)
struct ad7380_state *st = iio_priv(indio_dev);
struct spi_transfer xfer = {
.bits_per_word = st->chip_info->channels[0].scan_type.realbits,
- .len = 4,
+ .len = (st->chip_info->num_channels - 1) *
+ BITS_TO_BYTES(st->chip_info->channels->scan_type.storagebits),
.rx_buf = st->scan_data.raw,
};
int ret;
@@ -279,21 +301,22 @@ static int ad7380_read_direct(struct ad7380_state *st,
.speed_hz = AD7380_REG_WR_SPEED_HZ,
.bits_per_word = chan->scan_type.realbits,
.delay = {
- .value = 190, /* t[CONVERT] */
+ .value = T_CONVERT_NS,
.unit = SPI_DELAY_UNIT_NSECS,
},
.cs_change = 1,
.cs_change_delay = {
- .value = 10, /* t[CSH] */
+ .value = st->chip_info->timing_specs->t_csh_ns,
.unit = SPI_DELAY_UNIT_NSECS,
},
},
- /* then read both channels */
+ /* then read all channels */
{
.speed_hz = AD7380_REG_WR_SPEED_HZ,
.bits_per_word = chan->scan_type.realbits,
.rx_buf = st->scan_data.raw,
- .len = 4,
+ .len = (st->chip_info->num_channels - 1) *
+ ((chan->scan_type.storagebits > 16) ? 4 : 2),
},
};
int ret;
@@ -474,7 +497,7 @@ static int ad7380_probe(struct spi_device *spi)
indio_dev->name = st->chip_info->name;
indio_dev->info = &ad7380_info;
indio_dev->modes = INDIO_DIRECT_MODE;
- indio_dev->available_scan_masks = ad7380_2_channel_scan_masks;
+ indio_dev->available_scan_masks = st->chip_info->available_scan_masks;

ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
iio_pollfunc_store_time,

--
2.44.0

2024-05-01 15:00:28

[permalink] [raw]

Subject: [PATCH RFC v6 08/10] iio: adc: ad7380: add oversampling support

ad7380x(-4) parts are able to do oversampling to increase accuracy.
They support 2 average modes: normal average and rolling overage.
This commits focus on enabling normal average oversampling, which is the
default one.

Normal averaging involves taking a number of samples, adding them together,
and dividing the result by the number of samples taken.
This result is then output from the device. The sample data is cleared when
the process completes. Because we need more samples to output a value,
the data output rate decrease with the oversampling ratio.

Signed-off-by: Julien Stephan <[email protected]>
---
drivers/iio/adc/ad7380.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 114 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index 020959759170..1e3869f5e48c 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -88,7 +88,10 @@ struct ad7380_chip_info {
.type = IIO_VOLTAGE, \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
((diff) ? 0 : BIT(IIO_CHAN_INFO_OFFSET)), \
- .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+ .info_mask_shared_by_type_available = \
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
.indexed = 1, \
.differential = (diff), \
.channel = (diff) ? (2 * (index)) : (index), \
@@ -156,6 +159,16 @@ static const struct ad7380_timing_specs ad7380_4_timing = {
.t_csh_ns = 20,
};

+/*
+ * Available oversampling ratios. The indices correspond
+ * with the bit value expected by the chip.
+ * The available ratios depend on the averaging mode,
+ * only normal averaging is supported for now
+ */
+static const int ad7380_normal_average_oversampling_ratios[] = {
+ 1, 2, 4, 8, 16, 32,
+};
+
static const struct ad7380_chip_info ad7380_chip_info = {
.name = "ad7380",
.channels = ad7380_channels,
@@ -231,6 +244,7 @@ static const struct ad7380_chip_info ad7384_4_chip_info = {
struct ad7380_state {
const struct ad7380_chip_info *chip_info;
struct spi_device *spi;
+ unsigned int oversampling_ratio;
struct regmap *regmap;
unsigned int vref_mv;
unsigned int vcm_mv[MAX_NUM_CHANNELS];
@@ -386,6 +400,12 @@ static int ad7380_read_direct(struct ad7380_state *st,
};
int ret;

+ /*
+ * In normal average oversampling we need to wait for multiple conversions to be done
+ */
+ if (st->oversampling_ratio > 1)
+ xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio;
+
ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
if (ret < 0)
return ret;
@@ -428,6 +448,91 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
/ st->vref_mv;

return IIO_VAL_INT;
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ *val = st->oversampling_ratio;
+
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad7380_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ *vals = ad7380_normal_average_oversampling_ratios;
+ *length = ARRAY_SIZE(ad7380_normal_average_oversampling_ratios);
+ *type = IIO_VAL_INT;
+
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+}
+
+/**
+ * check_osr - Check the oversampling ratio
+ * @available_ratio: available ratios's array
+ * @size: size of the available_ratio array
+ * ratio: ratio to check
+ *
+ * Check if ratio is present in @available_ratio. Check for exact match.
+ * @available_ratio is an array of the available ratios (depending on the oversampling mode).
+ * The indices must correspond with the bit value expected by the chip.
+ */
+static inline int check_osr(const int *available_ratio, int size, int ratio)
+{
+ int i;
+
+ for (i = 0; i < size; i++) {
+ if (ratio == available_ratio[i])
+ return i;
+ }
+
+ return -EINVAL;
+}
+
+static int ad7380_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val,
+ int val2, long mask)
+{
+ struct ad7380_state *st = iio_priv(indio_dev);
+ int ret, osr;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ osr = check_osr(ad7380_normal_average_oversampling_ratios,
+ ARRAY_SIZE(ad7380_normal_average_oversampling_ratios),
+ val);
+
+ if (osr < 0)
+ return osr;
+
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
+ AD7380_CONFIG1_OSR,
+ FIELD_PREP(AD7380_CONFIG1_OSR, osr));
+
+ if (ret)
+ return ret;
+
+ st->oversampling_ratio = val;
+
+ /*
+ * Perform a soft reset.
+ * This will flush the oversampling block and FIFO but will
+ * maintain the content of the configurable registers.
+ */
+ ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
+ AD7380_CONFIG2_RESET,
+ FIELD_PREP(AD7380_CONFIG2_RESET,
+ AD7380_CONFIG2_RESET_SOFT));
+ }
+ return 0;
default:
return -EINVAL;
}
@@ -435,6 +540,8 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,

static const struct iio_info ad7380_info = {
.read_raw = &ad7380_read_raw,
+ .read_avail = &ad7380_read_avail,
+ .write_raw = &ad7380_write_raw,
.debugfs_reg_access = &ad7380_debugfs_reg_access,
};

@@ -458,6 +565,12 @@ static int ad7380_init(struct ad7380_state *st, struct regulator *vref)
if (ret < 0)
return ret;

+ /* Disable oversampling by default.
+ * This is the default value after reset,
+ * so just initialize internal data
+ */
+ st->oversampling_ratio = 1;
+
/* SPI 1-wire mode */
return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
AD7380_CONFIG2_SDO,

--
2.44.0

2024-05-06 08:20:31

by Nuno Sá

[permalink] [raw]

Subject: Re: [PATCH RFC v6 08/10] iio: adc: ad7380: add oversampling support

Hi Julien,

On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote:
> ad7380x(-4) parts are able to do oversampling to increase accuracy.
> They support 2 average modes: normal average and rolling overage.
> This commits focus on enabling normal average oversampling, which is the
> default one.
>
> Normal averaging involves taking a number of samples, adding them together,
> and dividing the result by the number of samples taken.
> This result is then output from the device. The sample data is cleared when
> the process completes. Because we need more samples to output a value,
> the data output rate decrease with the oversampling ratio.
>
> Signed-off-by: Julien Stephan <[email protected]>
> ---
> drivers/iio/adc/ad7380.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 114 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index 020959759170..1e3869f5e48c 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -88,7 +88,10 @@ struct ad7380_chip_info {
> .type = IIO_VOLTAGE, \
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> ((diff) ? 0 : BIT(IIO_CHAN_INFO_OFFSET)), \
> - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> + .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> .indexed = 1, \
> .differential = (diff), \
> .channel = (diff) ? (2 * (index)) : (index), \
> @@ -156,6 +159,16 @@ static const struct ad7380_timing_specs ad7380_4_timing = {
> .t_csh_ns = 20,
> };
>
> +/*
> + * Available oversampling ratios. The indices correspond
> + * with the bit value expected by the chip.
> + * The available ratios depend on the averaging mode,
> + * only normal averaging is supported for now
> + */
> +static const int ad7380_normal_average_oversampling_ratios[] = {
> + 1, 2, 4, 8, 16, 32,
> +};
> +
> static const struct ad7380_chip_info ad7380_chip_info = {
> .name = "ad7380",
> .channels = ad7380_channels,
> @@ -231,6 +244,7 @@ static const struct ad7380_chip_info ad7384_4_chip_info = {
> struct ad7380_state {
> const struct ad7380_chip_info *chip_info;
> struct spi_device *spi;
> + unsigned int oversampling_ratio;

nit: move this to the other 'unsigned int' fields...

> struct regmap *regmap;
> unsigned int vref_mv;
> unsigned int vcm_mv[MAX_NUM_CHANNELS];
> @@ -386,6 +400,12 @@ static int ad7380_read_direct(struct ad7380_state *st,
> };
> int ret;
>
> + /*
> + * In normal average oversampling we need to wait for multiple conversions
> to be done
> + */
> + if (st->oversampling_ratio > 1)
> + xfers[0].delay.value = T_CONVERT_NS + 500 * st-
> >oversampling_ratio;
> +
> ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> if (ret < 0)
> return ret;
> @@ -428,6 +448,91 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
> / st->vref_mv;
>
> return IIO_VAL_INT;
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + *val = st->oversampling_ratio;
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad7380_read_avail(struct iio_dev *indio_dev,
> +      struct iio_chan_spec const *chan,
> +      const int **vals, int *type, int *length,
> +      long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + *vals = ad7380_normal_average_oversampling_ratios;
> + *length = ARRAY_SIZE(ad7380_normal_average_oversampling_ratios);
> + *type = IIO_VAL_INT;
> +
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +/**
> + * check_osr - Check the oversampling ratio
> + * @available_ratio: available ratios's array
> + * @size: size of the available_ratio array
> + * ratio: ratio to check
> + *
> + * Check if ratio is present in @available_ratio. Check for exact match.
> + * @available_ratio is an array of the available ratios (depending on the
> oversampling mode).
> + * The indices must correspond with the bit value expected by the chip.
> + */
> +static inline int check_osr(const int *available_ratio, int size, int ratio)

Please name the function ad7380_check_osr(). Also, drop the inline... The compiler
should be smart enough to take of that for us.

> +{
> + int i;
> +
> + for (i = 0; i < size; i++) {
> + if (ratio == available_ratio[i])
> + return i;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ad7380_write_raw(struct iio_dev *indio_dev,
> +     struct iio_chan_spec const *chan, int val,
> +     int val2, long mask)
> +{
> + struct ad7380_state *st = iio_priv(indio_dev);
> + int ret, osr;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + osr = check_osr(ad7380_normal_average_oversampling_ratios,
> + ARRAY_SIZE(ad7380_normal_average_oversampling_rati
> os),
> + val);
> +
> + if (osr < 0)
> + return osr;
> +
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + ret = regmap_update_bits(st->regmap,
> AD7380_REG_ADDR_CONFIG1,
> + AD7380_CONFIG1_OSR,
> + FIELD_PREP(AD7380_CONFIG1_OSR,
> osr));
> +
> + if (ret)
> + return ret;
> +
> + st->oversampling_ratio = val;
> +
> + /*
> + * Perform a soft reset.
> + * This will flush the oversampling block and FIFO but
> will
> + * maintain the content of the configurable registers.
> + */
> + ret = regmap_update_bits(st->regmap,
> AD7380_REG_ADDR_CONFIG2,
> + AD7380_CONFIG2_RESET,
> + FIELD_PREP(AD7380_CONFIG2_RESET,
> +
> AD7380_CONFIG2_RESET_SOFT));
> + }
> + return 0;

return ret;

Or you may be asked to directly return in regmap_update_bits() and use unreachable()
here.

> default:
> return -EINVAL;
> }
> @@ -435,6 +540,8 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>
> static const struct iio_info ad7380_info = {
> .read_raw = &ad7380_read_raw,
> + .read_avail = &ad7380_read_avail,
> + .write_raw = &ad7380_write_raw,
> .debugfs_reg_access = &ad7380_debugfs_reg_access,
> };
>
> @@ -458,6 +565,12 @@ static int ad7380_init(struct ad7380_state *st, struct
> regulator *vref)
> if (ret < 0)
> return ret;
>
> + /* Disable oversampling by default.
> + * This is the default value after reset,
> + * so just initialize internal data
> + */

Your comment block is not in accordance with coding style. checkpatch should complain
about this.

> + st->oversampling_ratio = 1;
> +
> /* SPI 1-wire mode */
> return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> AD7380_CONFIG2_SDO,
>

2024-05-06 08:33:36

by Nuno Sá

[permalink] [raw]

Subject: Re: [PATCH RFC v6 09/10] iio: adc: ad7380: add support for rolling average oversampling mode

On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote:
> Adds support for rolling average oversampling mode.
>
> Rolling oversampling mode uses a first in, first out (FIFO) buffer of
> the most recent samples in the averaging calculation, allowing the ADC
> throughput rate and output data rate to stay the same, since we only need
> to take only one sample for each new conversion.
>
> The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8
> in this mode (vs 1, 2, 4, 8, 16, 32 for the normal average)
>
> In order to be able to change the averaging mode, this commit also adds
> the new "oversampling_mode" and "oversampling_mode_available" custom
> attributes along with the according documentation file in
> Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard
> attributes correspond to this use case.
>
> Signed-off-by: Julien Stephan <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 | 38 ++++++
> MAINTAINERS                                        |   1 +
> drivers/iio/adc/ad7380.c                           | 143 +++++++++++++++++++--
> 3 files changed, 174 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380
> b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380
> new file mode 100644
> index 000000000000..0a560ef3e32a
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380
> @@ -0,0 +1,38 @@
> +What: /sys/bus/iio/devices/iio:deviceX/oversampling_mode
> +KernelVersion: 6.9
> +Contact: Michael Hennerich <[email protected]>
> +Description:
> +    Writing this attribute sets the oversampling average mode.
> +    Reading it, shows the configured mode.
> +    Available modes can be displayed using the oversampling_mode_available
> +    attribute.
> +    When writing this attribute to change the oversampling mode, this will
> +    have the following side effects:
> +
> +      - soft reset the ADC to flush the oversampling block and FIFO
> +
> +      - the available oversampling ratios depend on the oversampling mode
> +        configured so to avoid misconfiguration, changing the mode will disable
> +        the oversampling by setting the ratio to 1.

Hmm, can we somehow re-enable it again with a proper configuration (after the mode
change)?

> +
> +      - the list of available ratios (displayed by reading the
> +        oversampling_ratio_available attribute) will be updated when changing
> +        the oversampling mode.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/oversampling_mode_available
> +KernelVersion: 6.9
> +Contact: Michael Hennerich <[email protected]>
> +Description:
> +    Display the available oversampling average modes. The two available modes
> +    are "normal" and "rolling" where "normal" average mode is the default one.
> +
> +      - normal averaging involves taking a number of samples, adding them
> +        together, and dividing the result by the number of samples taken.
> +        This result is then output from the device. The sample data is cleared
> +        when the process completes. Because we need more samples to output a
> +        value, the data output rate decrease with the oversampling ratio.
> +
> +      - rolling oversampling mode uses a first in, first out (FIFO) buffer of
> +        the most recent samples in the averaging calculation, allowing the ADC
> +        throughput rate and output data rate to stay the same, since we only need
> +        to take only one sample for each new conversion.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 87724a9e9f9f..ca1e115f2aff 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -434,6 +434,7 @@ R: David Lechner <[email protected]>
> S: Supported
> W:
> https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
> W: https://ez.analog.com/linux-software-drivers
> +F: Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380
> F: Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> F: drivers/iio/adc/ad7380.c
>
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index 1e3869f5e48c..7b021bb9cf87 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -51,6 +51,8 @@
> #define AD7380_REG_ADDR_ALERT_HIGH_TH 0x5
>
> #define AD7380_CONFIG1_OS_MODE BIT(9)
> +#define OS_MODE_NORMAL_AVERAGE 0
> +#define OS_MODE_ROLLING_AVERAGE 1

no AD7380 prefix?

> #define AD7380_CONFIG1_OSR GENMASK(8, 6)
> #define AD7380_CONFIG1_CRC_W BIT(5)
> #define AD7380_CONFIG1_CRC_R BIT(4)
> @@ -159,16 +161,27 @@ static const struct ad7380_timing_specs ad7380_4_timing = {
> .t_csh_ns = 20,
> };
>

..

>
> +static ssize_t oversampling_mode_store(struct device *dev,
> +        struct device_attribute *attr,
> +        const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct ad7380_state *st = iio_priv(indio_dev);
> + int os_mode, ret;
> +
> + ret = sysfs_match_string(ad7380_oversampling_average_modes, buf);
> + if (ret < 0)
> + return ret;
> +
> + os_mode = ret;
> +
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
> + AD7380_CONFIG1_OS_MODE,
> + FIELD_PREP(AD7380_CONFIG1_OS_MODE,
> os_mode));
> +
> + if (ret)
> + return ret;
> +
> + st->oversampling_mode = os_mode;
> +
> + /*
> + * Oversampling ratio depends on oversampling mode, to avoid
> + * misconfiguration when changing oversampling mode,
> + * disable oversampling by setting OSR to 0.
> + */

Given the comment, I would expect you to disable osr before changing the mode.

> + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
> + AD7380_CONFIG1_OSR,
> FIELD_PREP(AD7380_CONFIG1_OSR, 0));
> +
> + if (ret)
> + return ret;
> +
> + st->oversampling_ratio = 1;
> +

1?

Also, it would be nice if we could re-enabled osr. So the flow would maybe be:

1) disable osr;
2) change mode;
3) do whatever we need so configuration makes sense for new mode?
4) re-enable osr;
5) soft-reset;

Also not sure if 4) and 5) are in the correct order.

> + /*
> + * Perform a soft reset.
> + * This will flush the oversampling block and FIFO but will
> + * maintain the content of the configurable registers.
> + */
> + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> + AD7380_CONFIG2_RESET,
> + FIELD_PREP(AD7380_CONFIG2_RESET,
> +     AD7380_CONFIG2_RESET_SOFT));
> + }
> + return ret ?: len;
> +}
> +
> +static ssize_t oversampling_mode_available_show(struct device *dev,
> + struct device_attribute *attr,
> char *buf)
> +{
> + int i;
> + size_t len = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(ad7380_oversampling_average_modes); i++)
> + len += sysfs_emit_at(buf, len, "%s ",
> ad7380_oversampling_average_modes[i]);
> +
> + buf[len - 1] = '\n';
> +
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR_RW(oversampling_mode, 0);
> +static IIO_DEVICE_ATTR_RO(oversampling_mode_available, 0);
> +
> +static struct attribute *ad7380_attributes[] = {
> + &iio_dev_attr_oversampling_mode.dev_attr.attr,
> + &iio_dev_attr_oversampling_mode_available.dev_attr.attr,
> + NULL
> +};
> +

Maybe use IIO_ENUM... It allows the core to do some of the stuff you're doing for
you.

- Nuno Sá

2024-05-06 08:56:00

by Nuno Sá

[permalink] [raw]

Subject: Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for resolution boost

On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote:
> ad738x chips are able to use an additional 2 bits of resolution when
> using oversampling. The 14-bits chips can have up to 16 bits of
> resolution and the 16-bits chips can have up to 18 bits of resolution.
>
> In order to dynamically allow to enable/disable the resolution boost
> feature, we have to set iio realbits/storagebits to the maximum per chips.
> This means that for iio, data will always be on the higher resolution
> available, and to cope with that we adjust the iio scale and iio offset
> depending on the resolution boost status.
>
> The available scales can be displayed using the regular _scale_available
> attributes and can be set by writing the regular _scale attribute.
> The available scales depend on the oversampling status.
>
> Signed-off-by: Julien Stephan <[email protected]>
>
> ---
>
> In order to support the resolution boost (additional 2 bits of resolution)
> we need to set realbits/storagebits to the maximum value i.e :
> - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
>
> For 14-bits chips this does not have a major impact, but this
> has the drawback of forcing 16-bits chip users to always use 32-bits
> words in buffers even if they are not using oversampling and resolution
> boost. Is there a better way of implementing this? For example
> implementing dynamic scan_type?
>

Yeah, I don't think it's that bad in this case. But maybe dynamic scan types is
something we may need at some point yes (or IOW that I would like to see supported
:)). We do some ADCs (eg: ad4630) where we use questionably use FW properties to set
a specific operating mode exactly because we have a different data layout (scan
elements) depending on the mode.

> Another issue is the location of the timestamps. I understood the need
> for ts to be consistent between chips, but right now I do not have a
> better solution.. I was thinking of maybe adding the timestamps at the
> beginning? That would imply to change the iio_chan_spec struct and maybe
> add a iio_push_to_buffers_with_timestamp_first() wrapper function? Is
> that an option?
>
> Any suggestion would be very much appreciated.
> ---
> drivers/iio/adc/ad7380.c | 226 ++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 194 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index 7b021bb9cf87..e240098708e9 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -20,6 +20,7 @@
> #include <linux/err.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/units.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> @@ -58,6 +59,8 @@
> #define AD7380_CONFIG1_CRC_R BIT(4)
> #define AD7380_CONFIG1_ALERTEN BIT(3)
> #define AD7380_CONFIG1_RES BIT(2)
> +#define RESOLUTION_BOOST_DISABLE 0
> +#define RESOLUTION_BOOST_ENABLE 1

No ad7390 prefix?

> #define AD7380_CONFIG1_REFSEL BIT(1)
> #define AD7380_CONFIG1_PMODE BIT(0)
>
> @@ -86,6 +89,14 @@ struct ad7380_chip_info {
> const struct ad7380_timing_specs *timing_specs;
> };
>
> +/*
> + * realbits/storagebits cannot be dynamically changed, so in order to
> + * support the resolution boost (additional 2 bits of resolution)
> + * we need to set realbits/storagebits to the maximum value i.e :
> + *   - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> + *   - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
> + * We need to adjust the scale depending on resolution boost status
> + */
> #define AD7380_CHANNEL(index, bits, diff) { \
> .type = IIO_VOLTAGE, \
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> @@ -93,6 +104,7 @@ struct ad7380_chip_info {
> .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> .indexed = 1, \
> .differential = (diff), \
> @@ -101,8 +113,8 @@ struct ad7380_chip_info {
> .scan_index = (index), \
> .scan_type = { \
> .sign = 's', \
> - .realbits = (bits), \
> - .storagebits = 16, \
> + .realbits = (bits) + 2, \
> + .storagebits = ((bits) + 2 > 16) ? 32 : 16, \
> .endianness = IIO_CPU, \
> }, \
> }
> @@ -259,6 +271,8 @@ struct ad7380_state {
> struct spi_device *spi;
> unsigned int oversampling_mode;
> unsigned int oversampling_ratio;
> + unsigned int scales[2][2];
> + bool resolution_boost_enable;
> struct regmap *regmap;
> unsigned int vref_mv;
> unsigned int vcm_mv[MAX_NUM_CHANNELS];
> @@ -270,7 +284,10 @@ struct ad7380_state {
> * As MAX_NUM_CHANNELS is 4 the layout of the structure is the same for
> all parts
> */
> struct {
> - u16 raw[MAX_NUM_CHANNELS];
> + union {
> + u16 u16[MAX_NUM_CHANNELS];
> + u32 u32[MAX_NUM_CHANNELS];
> + } raw;
>
> s64 ts __aligned(8);
> } scan_data __aligned(IIO_DMA_MINALIGN);
> @@ -359,23 +376,67 @@ static int ad7380_debugfs_reg_access(struct iio_dev
> *indio_dev, u32 reg,
> unreachable();
> }
>
> +static int ad7380_prepare_spi_xfer(struct ad7380_state *st, struct spi_transfer
> *xfer)
> +{
> + int bits_per_word;
> +
> + memset(xfer, 0, sizeof(*xfer));
> +
> + xfer->rx_buf = &st->scan_data.raw;
> +
> + if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
> + bits_per_word = st->chip_info->channels[0].scan_type.realbits;
> + else
> + bits_per_word = st->chip_info->channels[0].scan_type.realbits - 2;
> +
> + xfer->bits_per_word = bits_per_word;
> +
> + xfer->len = (st->chip_info->num_channels - 1) *
> BITS_TO_BYTES(bits_per_word);
> +
> + return bits_per_word;

I think this may very well be something we can do once during buffer enablement... I
would expect that enabling/disabling resolution boost during buffering not to be a
great idea.

> +}
> +
> static irqreturn_t ad7380_trigger_handler(int irq, void *p)
> {
> struct iio_poll_func *pf = p;
> struct iio_dev *indio_dev = pf->indio_dev;
> struct ad7380_state *st = iio_priv(indio_dev);
> - struct spi_transfer xfer = {
> - .bits_per_word = st->chip_info->channels[0].scan_type.realbits,
> - .len = (st->chip_info->num_channels - 1) *
> -        BITS_TO_BYTES(st->chip_info->channels-
> >scan_type.storagebits),
> - .rx_buf = st->scan_data.raw,
> - };
> - int ret;
> + struct spi_transfer xfer;
> + int bits_per_word, realbits, i, ret;
> +
> + realbits = st->chip_info->channels[0].scan_type.realbits;
> + bits_per_word = ad7380_prepare_spi_xfer(st, &xfer);
>
> ret = spi_sync_transfer(st->spi, &xfer, 1);
> if (ret)
> goto out;
>
> + /*
> + * If bits_per_word == realbits (resolution boost enabled), we don't
> + * need to manipulate the raw data, otherwise we may need to fix things
> + * up a bit to fit the scan_type specs
> + */
> + if (bits_per_word < realbits) {
> + if (realbits > 16 && bits_per_word <= 16) {
> + /*
> + * Here realbits > 16 so storagebits is 32 and
> bits_per_word is <= 16
> + * so we need to sign extend u16 to u32 using reverse
> order to
> + * avoid writing over union data
> + */
> + for (i = st->chip_info->num_channels - 2; i >= 0; i--)
> + st->scan_data.raw.u32[i] = sign_extend32(st-
> >scan_data.raw.u16[i],
> +
> bits_per_word - 1);
> + } else if (bits_per_word < 16) {

Can't we have bits_per_word = 16 in case realbits <= 16?

- Nuno Sá

2024-05-06 13:46:38

[permalink] [raw]

Subject: Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for resolution boost

On Mon, 06 May 2024 10:55:46 +0200
Nuno Sá <[email protected]> wrote:

> On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote:
> > ad738x chips are able to use an additional 2 bits of resolution when
> > using oversampling. The 14-bits chips can have up to 16 bits of
> > resolution and the 16-bits chips can have up to 18 bits of resolution.
> >
> > In order to dynamically allow to enable/disable the resolution boost
> > feature, we have to set iio realbits/storagebits to the maximum per chips.
> > This means that for iio, data will always be on the higher resolution
> > available, and to cope with that we adjust the iio scale and iio offset
> > depending on the resolution boost status.
> >
> > The available scales can be displayed using the regular _scale_available
> > attributes and can be set by writing the regular _scale attribute.
> > The available scales depend on the oversampling status.
> >
> > Signed-off-by: Julien Stephan <[email protected]>
> >
> > ---
> >
> > In order to support the resolution boost (additional 2 bits of resolution)
> > we need to set realbits/storagebits to the maximum value i.e :
> > - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> > - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
> >
> > For 14-bits chips this does not have a major impact, but this
> > has the drawback of forcing 16-bits chip users to always use 32-bits
> > words in buffers even if they are not using oversampling and resolution
> > boost. Is there a better way of implementing this? For example
> > implementing dynamic scan_type?
> >
>
> Yeah, I don't think it's that bad in this case. But maybe dynamic scan types is
> something we may need at some point yes (or IOW that I would like to see supported
> :)). We do some ADCs (eg: ad4630) where we use questionably use FW properties to set
> a specific operating mode exactly because we have a different data layout (scan
> elements) depending on the mode.

Yeah. Fixed scan modes were somewhat of a bad design decision a long time back.
However, the big advantage is that it got people to think hard about whether it is
worth supporting low precision modes. For slow devices it very rarely is and
forcing people to make a decision and the advantage we never supported
low precision on those parts.

Having said that there are good reasons for dynamic resolution changing
(the main one being the storage case you have here) so I'd be happy to
see some patches adding it. It might be easier than I've always thought
to bolt on.

This and inkernel event consumers have been the two significant features
where I keep expecting it to happen, but every time people decide they really
don't care enough to make them work :(

>
> > Another issue is the location of the timestamps. I understood the need
> > for ts to be consistent between chips, but right now I do not have a
> > better solution.. I was thinking of maybe adding the timestamps at the
> > beginning? That would imply to change the iio_chan_spec struct and maybe
> > add a iio_push_to_buffers_with_timestamp_first() wrapper function? Is
> > that an option?

You have lost me on this one. Why does the timestamp need to be in a consistent
location? We have lots of drivers where it moves about between different
chips they support, or indeed what channels are currently turned on.

I haven't actually looked at the latest code yet, so maybe it will become
obvious!

Jonathan

2024-05-06 13:56:41

[permalink] [raw]

Subject: Re: [PATCH RFC v6 02/10] iio: adc: ad7380: new driver for AD7380 ADCs

On Wed, 01 May 2024 16:55:35 +0200
Julien Stephan <[email protected]> wrote:

> From: David Lechner <[email protected]>
>
> This adds a new driver for the AD7380 family ADCs.
>
> The driver currently implements basic support for the AD7380, AD7381,
> 2-channel differential ADCs. Support for additional single-ended,
> pseudo-differential and 4-channel chips that use the same register map
> as well as additional features of the chip will be added in future patches.
>
> Co-developed-by: Stefan Popa <[email protected]>
> Signed-off-by: Stefan Popa <[email protected]>
> Reviewed-by: Nuno Sa <[email protected]>
> Signed-off-by: David Lechner <[email protected]>
> [Julien Stephan: add datasheet links of supported parts]
> [Julien Stephan: fix rx/tx buffer for regmap access]
> [Julien Stephan: fix scale issue]
> [Julien Stephan: use the new iio_device_claim_direct_scoped
> instead of iio_device_claim_direct_mode]
> Signed-off-by: Julien Stephan <[email protected]>

I took another look, mostly so I could refresh my memory of the driver
before getting to the newer patches.

A few minor things inline, + I wondering if the structure for the
scan is what was meant about keeping timestamps in the same place.

> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> new file mode 100644
> index 000000000000..a218f59c276e
> --- /dev/null
> +++ b/drivers/iio/adc/ad7380.c
> @@ -0,0 +1,443 @@

> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

This and linux/sysfs.h are are normally only needed it you are providing
custom attrs. I don't see those here so not sure those headers are used
directly by this driver.

> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +

> +struct ad7380_state {
> + const struct ad7380_chip_info *chip_info;
> + struct spi_device *spi;
> + struct regmap *regmap;
> + unsigned int vref_mv;
> + /*
> + * DMA (thus cache coherency maintenance) requires the
> + * transfer buffers to live in their own cache lines.
> + * Make the buffer large enough for 2 16-bit samples and one 64-bit
> + * aligned 64 bit timestamp.
> + */
> + struct {
> + u16 raw[2];
> +
> + s64 ts __aligned(8);

Ah is the comment in patch 10 about consistent timestamps coming from this?
If so then don't worry about that - you just loose the pretty option of
using a structure to define the data layout.

union {
u16 raw[4 + 4] __aligned(8); /* include the ts*/
u32 raw[4 + 2] __aligned(8); /* include ts */
} scan_data __aligned(IIO_DMA_MINALIGN);

There are lots of drivers that are highly flexible in how many
channels we have so they always have to take this more manual path
to data sizing. Some do magic with ALIGN() so if you want you could
do something like that here. That magic ensures the right amount of
padding to have the naturally aligned 8 byte timestamp in an array of
smaller type.

Note you can't solve this by putting the timestamp first because it
is always optional whether it is enabled (that bit is just dealt with
by the IIO core rather than a driver).

> + } scan_data __aligned(IIO_DMA_MINALIGN);
> + u16 tx;
> + u16 rx;
> +};

> +static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
> + u32 writeval, u32 *readval)
> +{
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + struct ad7380_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (readval)
> + ret = regmap_read(st->regmap, reg, readval);
Trivial, but
return regmap_read()
> + else
> + ret = regmap_write(st->regmap, reg, writeval);
return regmap_write()

unless something gets added here later in the series.

> +
> + return ret;
> + }
> + unreachable();
> +}

2024-05-06 14:06:23

[permalink] [raw]

Subject: Re: [PATCH RFC v6 08/10] iio: adc: ad7380: add oversampling support

On Wed, 01 May 2024 16:55:41 +0200
Julien Stephan <[email protected]> wrote:

> ad7380x(-4) parts are able to do oversampling to increase accuracy.
> They support 2 average modes: normal average and rolling overage.
> This commits focus on enabling normal average oversampling, which is the
> default one.

The other case got me curious. If you do want to support the rolling average
in future, it could probably be handled as a low pass filter control rather
than a form of oversampling. Anyhow, not relevant here!

>
> Normal averaging involves taking a number of samples, adding them together,
> and dividing the result by the number of samples taken.
> This result is then output from the device. The sample data is cleared when
> the process completes. Because we need more samples to output a value,
> the data output rate decrease with the oversampling ratio.
>
> Signed-off-by: Julien Stephan <[email protected]>
Hi Julien.

A few additional comments inline.

Jonathan

> ---
> drivers/iio/adc/ad7380.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 114 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index 020959759170..1e3869f5e48c 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -88,7 +88,10 @@ struct ad7380_chip_info {
> .type = IIO_VOLTAGE, \
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> ((diff) ? 0 : BIT(IIO_CHAN_INFO_OFFSET)), \
> - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> + .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> .indexed = 1, \
> .differential = (diff), \
> .channel = (diff) ? (2 * (index)) : (index), \
> @@ -156,6 +159,16 @@ static const struct ad7380_timing_specs ad7380_4_timing = {
> .t_csh_ns = 20,
> };
>
> +/*
> + * Available oversampling ratios. The indices correspond
> + * with the bit value expected by the chip.
> + * The available ratios depend on the averaging mode,
> + * only normal averaging is supported for now
> + */
> +static const int ad7380_normal_average_oversampling_ratios[] = {
> + 1, 2, 4, 8, 16, 32,
> +};
> +
> static const struct ad7380_chip_info ad7380_chip_info = {
> .name = "ad7380",
> .channels = ad7380_channels,
> @@ -231,6 +244,7 @@ static const struct ad7380_chip_info ad7384_4_chip_info = {
> struct ad7380_state {
> const struct ad7380_chip_info *chip_info;
> struct spi_device *spi;
> + unsigned int oversampling_ratio;
> struct regmap *regmap;
> unsigned int vref_mv;
> unsigned int vcm_mv[MAX_NUM_CHANNELS];
> @@ -386,6 +400,12 @@ static int ad7380_read_direct(struct ad7380_state *st,
> };
> int ret;
>
> + /*
> + * In normal average oversampling we need to wait for multiple conversions to be done
Wrap comment at 80 chars. Generally I prefer we keep to old limit of 80 unless
there is a readability advantage. I don't see such an advantage in this case.

> + */
> + if (st->oversampling_ratio > 1)
> + xfers[0].delay.value = T_CONVERT_NS + 500 * st->oversampling_ratio;
> +

> +
> +/**
> + * check_osr - Check the oversampling ratio
> + * @available_ratio: available ratios's array
> + * @size: size of the available_ratio array
> + * ratio: ratio to check
> + *
> + * Check if ratio is present in @available_ratio. Check for exact match.
> + * @available_ratio is an array of the available ratios (depending on the oversampling mode).
> + * The indices must correspond with the bit value expected by the chip.
> + */
> +static inline int check_osr(const int *available_ratio, int size, int ratio)
> +{
> + int i;
> +
> + for (i = 0; i < size; i++) {
> + if (ratio == available_ratio[i])
> + return i;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ad7380_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct ad7380_state *st = iio_priv(indio_dev);
> + int ret, osr;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + osr = check_osr(ad7380_normal_average_oversampling_ratios,

Nuno already pointed out function name should be prefixed.

> + ARRAY_SIZE(ad7380_normal_average_oversampling_ratios),
> + val);

If this is just checking, why does it return osr? Feels like name needs
to be ad7380_osr_to_regval() or something like that.

> +
> + if (osr < 0)
> + return osr;
> +
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
> + AD7380_CONFIG1_OSR,
> + FIELD_PREP(AD7380_CONFIG1_OSR, osr));
> +
> + if (ret)
> + return ret;
> +
> + st->oversampling_ratio = val;
> +
> + /*
> + * Perform a soft reset.
> + * This will flush the oversampling block and FIFO but will
> + * maintain the content of the configurable registers.
> + */
> + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> + AD7380_CONFIG2_RESET,
> + FIELD_PREP(AD7380_CONFIG2_RESET,
> + AD7380_CONFIG2_RESET_SOFT));
> + }
> + return 0;
> default:
> return -EINVAL;
> }
> @@ -435,6 +540,8 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>
> static const struct iio_info ad7380_info = {
> .read_raw = &ad7380_read_raw,
> + .read_avail = &ad7380_read_avail,
> + .write_raw = &ad7380_write_raw,
> .debugfs_reg_access = &ad7380_debugfs_reg_access,
> };
>
> @@ -458,6 +565,12 @@ static int ad7380_init(struct ad7380_state *st, struct regulator *vref)
> if (ret < 0)
> return ret;
>
> + /* Disable oversampling by default.
IIO comment syntax is
/*
* Disable oversampling by default.

Also, curiously short lines that could definitely be wrapped nearer 80 chars!

> + * This is the default value after reset,
> + * so just initialize internal data
> + */
> + st->oversampling_ratio = 1;
> +
> /* SPI 1-wire mode */
> return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> AD7380_CONFIG2_SDO,
>

2024-05-06 14:17:49

[permalink] [raw]

Subject: Re: [PATCH RFC v6 09/10] iio: adc: ad7380: add support for rolling average oversampling mode

On Wed, 01 May 2024 16:55:42 +0200
Julien Stephan <[email protected]> wrote:

> Adds support for rolling average oversampling mode.
>
> Rolling oversampling mode uses a first in, first out (FIFO) buffer of
> the most recent samples in the averaging calculation, allowing the ADC
> throughput rate and output data rate to stay the same, since we only need
> to take only one sample for each new conversion.
>
> The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8
> in this mode (vs 1, 2, 4, 8, 16, 32 for the normal average)

Ah. I should have read on!

>
> In order to be able to change the averaging mode, this commit also adds
> the new "oversampling_mode" and "oversampling_mode_available" custom
> attributes along with the according documentation file in
> Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard
> attributes correspond to this use case.

This comes to the comment I stuck in the previous patch.

To most people this is not a form of oversampling because the data rate
remains unchanged. It's a cheap low pass filter (boxcar) Google pointed me at:
https://dsp.stackexchange.com/questions/9966/what-is-the-cut-off-frequency-of-a-moving-average-filter

in_voltage_low_pass_3db_frequency would be the most appropriate standard
ABI for this if we do treat it as a low pass filter control.

I'm not necessarily saying we don't want new ABI for this, but I would
like to consider the pros and cons of just using the 3db frequency.

So would that work for this part or am I missing something?

>
> Signed-off-by: Julien Stephan <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 | 38 ++++++
> MAINTAINERS | 1 +
> drivers/iio/adc/ad7380.c | 143 +++++++++++++++++++--
> 3 files changed, 174 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380
> new file mode 100644
> index 000000000000..0a560ef3e32a
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380
> @@ -0,0 +1,38 @@
> +What: /sys/bus/iio/devices/iio:deviceX/oversampling_mode
> +KernelVersion: 6.9
> +Contact: Michael Hennerich <[email protected]>
> +Description:
> + Writing this attribute sets the oversampling average mode.
> + Reading it, shows the configured mode.
> + Available modes can be displayed using the oversampling_mode_available
> + attribute.
> + When writing this attribute to change the oversampling mode, this will
> + have the following side effects:
Where possible, write ABI docs with the assumption we will generalize
them in future. Annoyingly the documentation system doesn't allow for
multiple descriptions. As such, additional information like this doesn't
belong in the ABI docs.

> +
> + - soft reset the ADC to flush the oversampling block and FIFO
I think this was already picked up on in another review, but my inclination is
make this something you can't change with the buffer enabled. The results
will be rather unpredictable anyway and it will simplify the handling a little
to just block that corner with a claim (or failure to claim) direct mode
when setting this.

> +
> + - the available oversampling ratios depend on the oversampling mode
> + configured so to avoid misconfiguration, changing the mode will disable
> + the oversampling by setting the ratio to 1.

Better to get a close as possible. If they've configured it to something we can't
do then it's user error. If they have picked a value that is still possible then
allowing that is a nice usability improvement.

> +
> + - the list of available ratios (displayed by reading the
> + oversampling_ratio_available attribute) will be updated when changing
> + the oversampling mode.

In general an ABI element is allowed to modify any other (because this sort of
chaining is common.) As such I don't think this needs to be in the ABI docs
but would be a useful detail to add to a chip specific main document elsewhere.

> +
> +What: /sys/bus/iio/devices/iio:deviceX/oversampling_mode_available
> +KernelVersion: 6.9
> +Contact: Michael Hennerich <[email protected]>
> +Description:
> + Display the available oversampling average modes. The two available modes
> + are "normal" and "rolling" where "normal" average mode is the default one.
> +
> + - normal averaging involves taking a number of samples, adding them
> + together, and dividing the result by the number of samples taken.
> + This result is then output from the device. The sample data is cleared
> + when the process completes. Because we need more samples to output a
> + value, the data output rate decrease with the oversampling ratio.
> +
> + - rolling oversampling mode uses a first in, first out (FIFO) buffer of
> + the most recent samples in the averaging calculation, allowing the ADC
> + throughput rate and output data rate to stay the same, since we only need
> + to take only one sample for each new conversion.

If we keep this, I wonder if "moving" or "rolling" is the more common term for this.

> +
> +static IIO_DEVICE_ATTR_RW(oversampling_mode, 0);
> +static IIO_DEVICE_ATTR_RO(oversampling_mode_available, 0);
> +
> +static struct attribute *ad7380_attributes[] = {
> + &iio_dev_attr_oversampling_mode.dev_attr.attr,
> + &iio_dev_attr_oversampling_mode_available.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group ad7380_attribute_group = {
> + .attrs = ad7380_attributes,
> +};

Bring the sysfs includes in here rather than in the original driver patch.

Thanks,

Jonathan

2024-05-06 14:23:35

[permalink] [raw]

Subject: Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for resolution boost

On Mon, 6 May 2024 14:46:16 +0100
Jonathan Cameron <[email protected]> wrote:

> On Mon, 06 May 2024 10:55:46 +0200
> Nuno Sá <[email protected]> wrote:
>
> > On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote:
> > > ad738x chips are able to use an additional 2 bits of resolution when
> > > using oversampling. The 14-bits chips can have up to 16 bits of
> > > resolution and the 16-bits chips can have up to 18 bits of resolution.
> > >
> > > In order to dynamically allow to enable/disable the resolution boost
> > > feature, we have to set iio realbits/storagebits to the maximum per chips.
> > > This means that for iio, data will always be on the higher resolution
> > > available, and to cope with that we adjust the iio scale and iio offset
> > > depending on the resolution boost status.
> > >
> > > The available scales can be displayed using the regular _scale_available
> > > attributes and can be set by writing the regular _scale attribute.
> > > The available scales depend on the oversampling status.
> > >
> > > Signed-off-by: Julien Stephan <[email protected]>
> > >
> > > ---
> > >
> > > In order to support the resolution boost (additional 2 bits of resolution)
> > > we need to set realbits/storagebits to the maximum value i.e :
> > > - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> > > - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
> > >
> > > For 14-bits chips this does not have a major impact, but this
> > > has the drawback of forcing 16-bits chip users to always use 32-bits
> > > words in buffers even if they are not using oversampling and resolution
> > > boost. Is there a better way of implementing this? For example
> > > implementing dynamic scan_type?
> > >
> >
> > Yeah, I don't think it's that bad in this case. But maybe dynamic scan types is
> > something we may need at some point yes (or IOW that I would like to see supported
> > :)). We do some ADCs (eg: ad4630) where we use questionably use FW properties to set
> > a specific operating mode exactly because we have a different data layout (scan
> > elements) depending on the mode.
>
> Yeah. Fixed scan modes were somewhat of a bad design decision a long time back.
> However, the big advantage is that it got people to think hard about whether it is
> worth supporting low precision modes. For slow devices it very rarely is and
> forcing people to make a decision and the advantage we never supported
> low precision on those parts.
>
> Having said that there are good reasons for dynamic resolution changing
> (the main one being the storage case you have here) so I'd be happy to
> see some patches adding it. It might be easier than I've always thought
> to bolt on.
>
> This and inkernel event consumers have been the two significant features
> where I keep expecting it to happen, but every time people decide they really
> don't care enough to make them work :(
>
> >
> > > Another issue is the location of the timestamps. I understood the need
> > > for ts to be consistent between chips, but right now I do not have a
> > > better solution.. I was thinking of maybe adding the timestamps at the
> > > beginning? That would imply to change the iio_chan_spec struct and maybe
> > > add a iio_push_to_buffers_with_timestamp_first() wrapper function? Is
> > > that an option?
>
> You have lost me on this one. Why does the timestamp need to be in a consistent
> location? We have lots of drivers where it moves about between different
> chips they support, or indeed what channels are currently turned on.
Maybe I now understand this?
The concern is the structure used for the iio_push_to_buffers_with_timestamp()
That doesn't need to be a structure and if you look at drivers where it isn't
the most common reason is because the timestamp needs to be able to move around.

So do something similar here.

Jonathan

>
> I haven't actually looked at the latest code yet, so maybe it will become
> obvious!
>
> Jonathan
>
>
>

2024-05-06 14:30:37

[permalink] [raw]

Subject: Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for resolution boost

On Wed, 01 May 2024 16:55:43 +0200
Julien Stephan <[email protected]> wrote:

> ad738x chips are able to use an additional 2 bits of resolution when
> using oversampling. The 14-bits chips can have up to 16 bits of
> resolution and the 16-bits chips can have up to 18 bits of resolution.
>
> In order to dynamically allow to enable/disable the resolution boost
> feature, we have to set iio realbits/storagebits to the maximum per chips.
> This means that for iio, data will always be on the higher resolution
> available, and to cope with that we adjust the iio scale and iio offset
> depending on the resolution boost status.
>
> The available scales can be displayed using the regular _scale_available
> attributes and can be set by writing the regular _scale attribute.
> The available scales depend on the oversampling status.
>
> Signed-off-by: Julien Stephan <[email protected]>
>
> ---
>
> In order to support the resolution boost (additional 2 bits of resolution)
> we need to set realbits/storagebits to the maximum value i.e :
> - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
>
> For 14-bits chips this does not have a major impact, but this
> has the drawback of forcing 16-bits chip users to always use 32-bits
> words in buffers even if they are not using oversampling and resolution
> boost. Is there a better way of implementing this? For example
> implementing dynamic scan_type?
>
> Another issue is the location of the timestamps. I understood the need
> for ts to be consistent between chips, but right now I do not have a
> better solution.. I was thinking of maybe adding the timestamps at the
> beginning? That would imply to change the iio_chan_spec struct and maybe
> add a iio_push_to_buffers_with_timestamp_first() wrapper function? Is
> that an option?

Questions discussed in another branch of the thread.

Jonathan

>
> Any suggestion would be very much appreciated.
> ---
> drivers/iio/adc/ad7380.c | 226 ++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 194 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index 7b021bb9cf87..e240098708e9 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -20,6 +20,7 @@
> #include <linux/err.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/units.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> @@ -58,6 +59,8 @@
> #define AD7380_CONFIG1_CRC_R BIT(4)
> #define AD7380_CONFIG1_ALERTEN BIT(3)
> #define AD7380_CONFIG1_RES BIT(2)
> +#define RESOLUTION_BOOST_DISABLE 0
> +#define RESOLUTION_BOOST_ENABLE 1
If the field is defined, the values should be obvious.
Also you used this as a boolean where simply passing in true
or false would be less confusing.

> #define AD7380_CONFIG1_REFSEL BIT(1)
> #define AD7380_CONFIG1_PMODE BIT(0)
>
> @@ -86,6 +89,14 @@ struct ad7380_chip_info {
> const struct ad7380_timing_specs *timing_specs;
> };

> @@ -259,6 +271,8 @@ struct ad7380_state {
> struct spi_device *spi;
> unsigned int oversampling_mode;
> unsigned int oversampling_ratio;
> + unsigned int scales[2][2];
> + bool resolution_boost_enable;
> struct regmap *regmap;
> unsigned int vref_mv;
> unsigned int vcm_mv[MAX_NUM_CHANNELS];
> @@ -270,7 +284,10 @@ struct ad7380_state {
> * As MAX_NUM_CHANNELS is 4 the layout of the structure is the same for all parts
> */
> struct {
> - u16 raw[MAX_NUM_CHANNELS];
> + union {
> + u16 u16[MAX_NUM_CHANNELS];
> + u32 u32[MAX_NUM_CHANNELS];
> + } raw;
>
> s64 ts __aligned(8);

As per earlier comments, roll this timestamp into the union as well
because it will move around.

> } scan_data __aligned(IIO_DMA_MINALIGN);
> @@ -359,23 +376,67 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
> unreachable();
> }

>
> +static int ad7380_set_resolution_boost(struct iio_dev *indio_dev, bool enable)
You pass 1 or 0 in here rather than true or false which would make more sense.
> +{
> + struct ad7380_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (st->resolution_boost_enable == enable)
> + return 0;
> +
> + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
> + AD7380_CONFIG1_RES,
> + FIELD_PREP(AD7380_CONFIG1_RES, enable));
Mapping true / false to 1 / 0 whilst correct doesn't give particularly readable
code. So useful to just have an
enable ? 1 : 0
in there to make the mapping more obvious.
> +
> + if (ret)
> + return ret;
> +
> + st->resolution_boost_enable = enable;

Trivial: blank line here.

> + return 0;
> +}
>
> static int ad7380_init(struct ad7380_state *st, struct regulator *vref)
> {
> int ret;
> @@ -691,12 +849,16 @@ static int ad7380_init(struct ad7380_state *st, struct regulator *vref)
> if (ret < 0)
> return ret;
>
> - /* Disable oversampling by default.
> - * This is the default value after reset,
> + /* Disable oversampling and resolution boost by default.

Follow through from earlier. Wrong comment syntax + wrap lines nearer 80 chars.

> + * This are the default values after reset,
> * so just initialize internal data
> */

2024-05-06 14:56:10

[permalink] [raw]

Subject: Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for resolution boost

FYI, Julien is AFK for a bit so I'll try to respond to the non-trivial comments.

On Mon, May 6, 2024 at 8:46 AM Jonathan Cameron <[email protected]> wrote:
>
> On Mon, 06 May 2024 10:55:46 +0200
> Nuno Sá <[email protected]> wrote:
>
> > On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote:
> > > ad738x chips are able to use an additional 2 bits of resolution when
> > > using oversampling. The 14-bits chips can have up to 16 bits of
> > > resolution and the 16-bits chips can have up to 18 bits of resolution.
> > >
> > > In order to dynamically allow to enable/disable the resolution boost
> > > feature, we have to set iio realbits/storagebits to the maximum per chips.
> > > This means that for iio, data will always be on the higher resolution
> > > available, and to cope with that we adjust the iio scale and iio offset
> > > depending on the resolution boost status.
> > >
> > > The available scales can be displayed using the regular _scale_available
> > > attributes and can be set by writing the regular _scale attribute.
> > > The available scales depend on the oversampling status.
> > >
> > > Signed-off-by: Julien Stephan <[email protected]>
> > >
> > > ---
> > >
> > > In order to support the resolution boost (additional 2 bits of resolution)
> > > we need to set realbits/storagebits to the maximum value i.e :
> > > - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> > > - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
> > >
> > > For 14-bits chips this does not have a major impact, but this
> > > has the drawback of forcing 16-bits chip users to always use 32-bits
> > > words in buffers even if they are not using oversampling and resolution
> > > boost. Is there a better way of implementing this? For example
> > > implementing dynamic scan_type?
> > >
> >
> > Yeah, I don't think it's that bad in this case. But maybe dynamic scan types is
> > something we may need at some point yes (or IOW that I would like to see supported
> > :)). We do some ADCs (eg: ad4630) where we use questionably use FW properties to set
> > a specific operating mode exactly because we have a different data layout (scan
> > elements) depending on the mode.
>
> Yeah. Fixed scan modes were somewhat of a bad design decision a long time back.
> However, the big advantage is that it got people to think hard about whether it is
> worth supporting low precision modes. For slow devices it very rarely is and
> forcing people to make a decision and the advantage we never supported
> low precision on those parts.
>
> Having said that there are good reasons for dynamic resolution changing
> (the main one being the storage case you have here) so I'd be happy to
> see some patches adding it. It might be easier than I've always thought
> to bolt on.
>
> This and inkernel event consumers have been the two significant features
> where I keep expecting it to happen, but every time people decide they really
> don't care enough to make them work :(
>

Supposing we knew someone willing and able :-) ...

Do you have any specific requirements for how dynamic resolution
changing should work? Any particular sticky points we should watch out
for?

I'm assuming this would just affect the bufferY/*_type attributes,
i.e. if you write a channel scale attribute to change the resolution,
then the scan_type info may change and the bufferY/*_type would need
to be read again.

2024-05-06 15:04:33

[permalink] [raw]

Subject: Re: [PATCH RFC v6 09/10] iio: adc: ad7380: add support for rolling average oversampling mode

On Mon, May 6, 2024 at 9:17 AM Jonathan Cameron <[email protected]> wrote:
>
> On Wed, 01 May 2024 16:55:42 +0200
> Julien Stephan <[email protected]> wrote:
>
> > Adds support for rolling average oversampling mode.
> >
> > Rolling oversampling mode uses a first in, first out (FIFO) buffer of
> > the most recent samples in the averaging calculation, allowing the ADC
> > throughput rate and output data rate to stay the same, since we only need
> > to take only one sample for each new conversion.
> >
> > The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8
> > in this mode (vs 1, 2, 4, 8, 16, 32 for the normal average)
>
> Ah. I should have read on!
>
> >
> > In order to be able to change the averaging mode, this commit also adds
> > the new "oversampling_mode" and "oversampling_mode_available" custom
> > attributes along with the according documentation file in
> > Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard
> > attributes correspond to this use case.
>
> This comes to the comment I stuck in the previous patch.
>
> To most people this is not a form of oversampling because the data rate
> remains unchanged. It's a cheap low pass filter (boxcar) Google pointed me at:
> https://dsp.stackexchange.com/questions/9966/what-is-the-cut-off-frequency-of-a-moving-average-filter
>
> in_voltage_low_pass_3db_frequency would be the most appropriate standard
> ABI for this if we do treat it as a low pass filter control.
>
> I'm not necessarily saying we don't want new ABI for this, but I would
> like to consider the pros and cons of just using the 3db frequency.
>
> So would that work for this part or am I missing something?
>

I like the idea. But from the link, it looks like the 3dB frequency
depends on the sampling frequency which is unknown (e.g. could come
from hrtimer trigger).

Would it be reasonable to calculate the 3db frequency at the max
sample rate that the chip allows and just use those numbers?

2024-05-06 15:09:33

[permalink] [raw]

Subject: Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for resolution boost

On Mon, May 6, 2024 at 3:55 AM Nuno Sá <[email protected]> wrote:
>
> On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote:

..

> > + /*
> > + * If bits_per_word == realbits (resolution boost enabled), we don't
> > + * need to manipulate the raw data, otherwise we may need to fix things
> > + * up a bit to fit the scan_type specs
> > + */
> > + if (bits_per_word < realbits) {
> > + if (realbits > 16 && bits_per_word <= 16) {
> > + /*
> > + * Here realbits > 16 so storagebits is 32 and
> > bits_per_word is <= 16
> > + * so we need to sign extend u16 to u32 using reverse
> > order to
> > + * avoid writing over union data
> > + */
> > + for (i = st->chip_info->num_channels - 2; i >= 0; i--)
> > + st->scan_data.raw.u32[i] = sign_extend32(st-
> > >scan_data.raw.u16[i],
> > +
> > bits_per_word - 1);
> > + } else if (bits_per_word < 16) {
>
> Can't we have bits_per_word = 16 in case realbits <= 16?
>
This case is handled by the outermost if, so can't have that here. (In
that case, no manipulation is required so the whole big if statement
is skipped). realbits will never be < bits_per_word.

2024-05-06 21:45:30

[permalink] [raw]

Subject: Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for resolution boost

On Wed, May 1, 2024 at 9:56 AM Julien Stephan <[email protected]> wrote:
>

..

>
> +static int ad7380_prepare_spi_xfer(struct ad7380_state *st, struct spi_transfer *xfer)
> +{
> + int bits_per_word;
> +
> + memset(xfer, 0, sizeof(*xfer));
> +
> + xfer->rx_buf = &st->scan_data.raw;
> +
> + if (st->resolution_boost_enable == RESOLUTION_BOOST_ENABLE)
> + bits_per_word = st->chip_info->channels[0].scan_type.realbits;
> + else
> + bits_per_word = st->chip_info->channels[0].scan_type.realbits - 2;
> +
> + xfer->bits_per_word = bits_per_word;
> +
> + xfer->len = (st->chip_info->num_channels - 1) * BITS_TO_BYTES(bits_per_word);

This needs to be based on storagebits, not realbits.

> +
> + return bits_per_word;
> +}
> +

2024-05-09 22:01:42

[permalink] [raw]

Subject: Re: [PATCH RFC v6 09/10] iio: adc: ad7380: add support for rolling average oversampling mode

On Wed, May 8, 2024 at 6:26 AM Jonathan Cameron
<[email protected]> wrote:
>
> On Mon, 6 May 2024 10:04:10 -0500
> David Lechner <[email protected]> wrote:
>
> > On Mon, May 6, 2024 at 9:17 AM Jonathan Cameron <[email protected]> wrote:
> > >
> > > On Wed, 01 May 2024 16:55:42 +0200
> > > Julien Stephan <[email protected]> wrote:
> > >
> > > > Adds support for rolling average oversampling mode.
> > > >
> > > > Rolling oversampling mode uses a first in, first out (FIFO) buffer of
> > > > the most recent samples in the averaging calculation, allowing the ADC
> > > > throughput rate and output data rate to stay the same, since we only need
> > > > to take only one sample for each new conversion.
> > > >
> > > > The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8
> > > > in this mode (vs 1, 2, 4, 8, 16, 32 for the normal average)
> > >
> > > Ah. I should have read on!
> > >
> > > >
> > > > In order to be able to change the averaging mode, this commit also adds
> > > > the new "oversampling_mode" and "oversampling_mode_available" custom
> > > > attributes along with the according documentation file in
> > > > Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard
> > > > attributes correspond to this use case.
> > >
> > > This comes to the comment I stuck in the previous patch.
> > >
> > > To most people this is not a form of oversampling because the data rate
> > > remains unchanged. It's a cheap low pass filter (boxcar) Google pointed me at:
> > > https://dsp.stackexchange.com/questions/9966/what-is-the-cut-off-frequency-of-a-moving-average-filter
> > >
> > > in_voltage_low_pass_3db_frequency would be the most appropriate standard
> > > ABI for this if we do treat it as a low pass filter control.
> > >
> > > I'm not necessarily saying we don't want new ABI for this, but I would
> > > like to consider the pros and cons of just using the 3db frequency.
> > >
> > > So would that work for this part or am I missing something?
> > >
> >
> > I like the idea. But from the link, it looks like the 3dB frequency
> > depends on the sampling frequency which is unknown (e.g. could come
> > from hrtimer trigger).
> >
> > Would it be reasonable to calculate the 3db frequency at the max
> > sample rate that the chip allows and just use those numbers?
> >
> Ah. So looking at datasheet the normal average oversampling is
> self clocked, but this version is not.
>
> So, I'll ask the dumb question. What is this feature for?
> We have to pump the SPI bus anyway why not just do the maths in
> userspace? Oversampling is normally about data rate reduction
> with a bonus in precision obtained.
>
> Jonathan
>

I asked the apps engineers and the answer I got is that it a way to
enable oversampling while still maintaining a high sample rate.

Another thing to consider here is that we can only enable the extra
resolution bits if oversampling is enabled (normal or rolling mode).
The chip might not work right if we try to enable the extra bits
without oversampling enabled.

So my thinking is perhaps it is better to keep the rolling mode as
oversampling rather than trying to call it a low pass filter. As you
said, normal mode is about data rate reduction with bonus precision.
Rolling average oversampling mode then would then just be for the
bonus precision.

2024-05-11 11:47:45