LinuxLists.cc - [PATCH v2 0/2] Add support for AD4000 series

[permalink] [raw]

Subject: [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000

Add device tree documentation for AD4000 family of ADC devices.

Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf

Signed-off-by: Marcelo Schmitt <[email protected]>
---
.../bindings/iio/adc/adi,ad4000.yaml | 201 ++++++++++++++++++
MAINTAINERS | 7 +
2 files changed, 208 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
new file mode 100644
index 000000000000..ca06afb5149e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
@@ -0,0 +1,201 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4000 and similar Analog to Digital Converters
+
+maintainers:
+ - Marcelo Schmitt <[email protected]>
+
+description: |
+ Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
+ Specifications can be found at:
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - adi,ad4000
+ - adi,ad4001
+ - adi,ad4002
+ - adi,ad4003
+ - adi,ad4004
+ - adi,ad4005
+ - adi,ad4006
+ - adi,ad4007
+ - adi,ad4008
+ - adi,ad4010
+ - adi,ad4011
+ - adi,ad4020
+ - adi,ad4021
+ - adi,ad4022
+ - adi,adaq4001
+ - adi,adaq4003
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
+
+ spi-cpha: true
+
+ adi,spi-mode:
+ $ref: /schemas/types.yaml#/definitions/string
+ enum: [ single, chain ]
+ description: |
+ This property indicates the SPI wiring configuration.
+
+ When this property is omitted, it is assumed that the device is using what
+ the datasheet calls "4-wire mode". This is the conventional SPI mode used
+ when there are multiple devices on the same bus. In this mode, the CNV
+ line is used to initiate the conversion and the SDI line is connected to
+ CS on the SPI controller.
+
+ When this property is present, it indicates that the device is using one
+ of the following alternative wiring configurations:
+
+ * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
+ definition of 3-wire mode is NOT at all related to the standard
+ spi-3wire property!) This mode is often used when the ADC is the only
+ device on the bus. In this mode, SDI is tied to VIO, and the CNV line
+ can be connected to the CS line of the SPI controller or to a GPIO, in
+ which case the CS line of the controller is unused.
+ * chain: The datasheet calls this "chain mode". This mode is used to save
+ on wiring when multiple ADCs are used. In this mode, the SDI line of
+ one chip is tied to the SDO of the next chip in the chain and the SDI of
+ the last chip in the chain is tied to GND. Only the first chip in the
+ chain is connected to the SPI bus. The CNV line of all chips are tied
+ together. The CS line of the SPI controller can be used as the CNV line
+ only if it is active high.
+
+ '#daisy-chained-devices': true
+
+ vdd-supply:
+ description: A 1.8V supply that powers the chip (VDD).
+
+ vio-supply:
+ description:
+ A 1.8V to 5.5V supply for the digital inputs and outputs (VIO).
+
+ ref-supply:
+ description:
+ A 2.5 to 5V supply for the external reference voltage (REF).
+
+ cnv-gpios:
+ description:
+ The Convert Input (CNV). This input has multiple functions. It initiates
+ the conversions and selects the SPI mode of the device (chain or CS). In
+ 'single' mode, this property is omitted if the CNV pin is connected to the
+ CS line of the SPI controller. If 'single' mode is selected and this GPIO
+ is provided, it must be active low.
+ maxItems: 1
+
+ adi,high-z-input:
+ type: boolean
+ description:
+ High-Z mode allows the amplifier and RC filter in front of the ADC to be
+ chosen based on the signal bandwidth of interest, rather than the settling
+ requirements of the switched capacitor SAR ADC inputs.
+
+ adi,gain-milli:
+ description: |
+ The hardware gain applied to the ADC input (in milli units).
+ The gain provided by the ADC input scaler is defined by the hardware
+ connections between chip pins OUT+, R1K-, R1K1-, R1K+, R1K1+, and OUT-.
+ If not present, default to 1000 (no actual gain applied).
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [454, 909, 1000, 1900]
+ default: 1000
+
+ interrupts:
+ description:
+ The SDO pin can also function as a busy indicator. This node should be
+ connected to an interrupt that is triggered when the SDO line goes low
+ while the SDI line is high and the CNV line is low ('single' mode) or the
+ SDI line is low and the CNV line is high ('multi' mode); or when the SDO
+ line goes high while the SDI and CNV lines are high (chain mode),
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - spi-cpha
+ - vdd-supply
+ - vio-supply
+ - ref-supply
+
+allOf:
+ # in '4-wire' mode, cnv-gpios is required, for other modes it is optional
+ - if:
+ not:
+ required:
+ - adi,spi-mode
+ then:
+ required:
+ - cnv-gpios
+ # chain mode has lower SCLK max rate
+ - if:
+ required:
+ - adi,spi-mode
+ properties:
+ adi,spi-mode:
+ const: chain
+ then:
+ properties:
+ spi-max-frequency:
+ maximum: 50000000 # for VIO > 2.7 V, 40000000 for VIO > 1.7 V
+ required:
+ - '#daisy-chained-devices'
+ else:
+ properties:
+ '#daisy-chained-devices': false
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ /* Example for a AD devices */
+ adc@0 {
+ compatible = "adi,ad4020";
+ reg = <0>;
+ spi-cpha;
+ spi-max-frequency = <71000000>;
+ vdd-supply = <&supply_1_8V>;
+ vio-supply = <&supply_1_8V>;
+ ref-supply = <&supply_5V>;
+ cnv-gpios = <&gpio0 88 GPIO_ACTIVE_HIGH>;
+ };
+ };
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ /* Example for a ADAQ devices */
+ adc@0 {
+ compatible = "adi,adaq4003";
+ reg = <0>;
+ spi-cpha;
+ adi,spi-mode = "single";
+ spi-max-frequency = <80000000>;
+ vdd-supply = <&supply_1_8V>;
+ vio-supply = <&supply_1_8V>;
+ ref-supply = <&supply_5V>;
+ adi,high-z-input;
+ adi,gain-milli = <454>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index a7287cf44869..5dfe118a5dd3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1159,6 +1159,13 @@ W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
F: drivers/iio/dac/ad3552r.c

+ANALOG DEVICES INC AD4000 DRIVER
+M: Marcelo Schmitt <[email protected]>
+L: [email protected]
+S: Supported
+W: https://ez.analog.com/linux-software-drivers
+F: Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
+
ANALOG DEVICES INC AD4130 DRIVER
M: Cosmin Tanislav <[email protected]>
L: [email protected]
--
2.43.0

2024-04-08 14:32:28

[permalink] [raw]

Subject: [PATCH v2 2/2] iio: adc: Add support for AD4000

Add support for AD4000 family of low noise, low power, high speed,
successive aproximation register (SAR) ADCs.

Signed-off-by: Marcelo Schmitt <[email protected]>
---
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 12 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad4000.c | 649 +++++++++++++++++++++++++++++++++++++++
4 files changed, 663 insertions(+)
create mode 100644 drivers/iio/adc/ad4000.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5dfe118a5dd3..86aa96115f5a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1165,6 +1165,7 @@ L: [email protected]
S: Supported
W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
+F: drivers/iio/adc/ad4000.c

ANALOG DEVICES INC AD4130 DRIVER
M: Cosmin Tanislav <[email protected]>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..9c9d13d4b74f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -21,6 +21,18 @@ config AD_SIGMA_DELTA
select IIO_BUFFER
select IIO_TRIGGERED_BUFFER

+config AD4000
+ tristate "Analog Devices AD4000 ADC Driver"
+ depends on SPI
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ Say yes here to build support for Analog Devices AD4000 high speed
+ SPI analog to digital converters (ADC).
+
+ To compile this driver as a module, choose M here: the module will be
+ called ad4000.
+
config AD4130
tristate "Analog Device AD4130 ADC Driver"
depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index edb32ce2af02..aa52068d864b 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -6,6 +6,7 @@
# When adding new entries keep the list in alphabetical order
obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
+obj-$(CONFIG_AD4000) += ad4000.o
obj-$(CONFIG_AD4130) += ad4130.o
obj-$(CONFIG_AD7091R) += ad7091r-base.o
obj-$(CONFIG_AD7091R5) += ad7091r5.o
diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
new file mode 100644
index 000000000000..7997d9d98743
--- /dev/null
+++ b/drivers/iio/adc/ad4000.c
@@ -0,0 +1,649 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AD4000 SPI ADC driver
+ *
+ * Copyright 2024 Analog Devices Inc.
+ */
+#include <asm/unaligned.h>
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+#include <linux/units.h>
+#include <linux/util_macros.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+#define AD400X_READ_COMMAND 0x54
+#define AD400X_WRITE_COMMAND 0x14
+
+/* AD4000 Configuration Register programmable bits */
+#define AD4000_STATUS BIT(4) /* Status bits output */
+#define AD4000_SPAN_COMP BIT(3) /* Input span compression */
+#define AD4000_HIGHZ BIT(2) /* High impedance mode */
+#define AD4000_TURBO BIT(1) /* Turbo mode */
+
+#define AD4000_TQUIET2_NS 60
+
+#define AD4000_18BIT_MSK GENMASK(31, 14)
+#define AD4000_20BIT_MSK GENMASK(31, 12)
+
+#define AD4000_DIFF_CHANNEL(_sign, _real_bits) \
+ { \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .differential = 1, \
+ .channel = 0, \
+ .channel2 = 1, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
+ .scan_type = { \
+ .sign = _sign, \
+ .realbits = _real_bits, \
+ .storagebits = _real_bits > 16 ? 32 : 16, \
+ .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
+ .endianness = IIO_BE, \
+ }, \
+ } \
+
+#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits) \
+ { \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = 0, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_OFFSET), \
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
+ .scan_type = { \
+ .sign = _sign, \
+ .realbits = _real_bits, \
+ .storagebits = _real_bits > 16 ? 32 : 16, \
+ .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
+ .endianness = IIO_BE, \
+ }, \
+ } \
+
+enum ad4000_ids {
+ ID_AD4000,
+ ID_AD4001,
+ ID_AD4002,
+ ID_AD4003,
+ ID_AD4004,
+ ID_AD4005,
+ ID_AD4006,
+ ID_AD4007,
+ ID_AD4008,
+ ID_AD4010,
+ ID_AD4011,
+ ID_AD4020,
+ ID_AD4021,
+ ID_AD4022,
+ ID_ADAQ4001,
+ ID_ADAQ4003,
+};
+
+struct ad4000_chip_info {
+ const char *dev_name;
+ struct iio_chan_spec chan_spec;
+};
+
+static const struct ad4000_chip_info ad4000_chips[] = {
+ [ID_AD4000] = {
+ .dev_name = "ad4000",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
+ },
+ [ID_AD4001] = {
+ .dev_name = "ad4001",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
+ },
+ [ID_AD4002] = {
+ .dev_name = "ad4002",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
+ },
+ [ID_AD4003] = {
+ .dev_name = "ad4003",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+ },
+ [ID_AD4004] = {
+ .dev_name = "ad4004",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
+ },
+ [ID_AD4005] = {
+ .dev_name = "ad4005",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
+ },
+ [ID_AD4006] = {
+ .dev_name = "ad4006",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
+ },
+ [ID_AD4007] = {
+ .dev_name = "ad4007",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+ },
+ [ID_AD4008] = {
+ .dev_name = "ad4008",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
+ },
+ [ID_AD4010] = {
+ .dev_name = "ad4010",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
+ },
+ [ID_AD4011] = {
+ .dev_name = "ad4011",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+ },
+ [ID_AD4020] = {
+ .dev_name = "ad4020",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
+ },
+ [ID_AD4021] = {
+ .dev_name = "ad4021",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
+ },
+ [ID_AD4022] = {
+ .dev_name = "ad4022",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
+ },
+ [ID_ADAQ4001] = {
+ .dev_name = "adaq4001",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
+ },
+ [ID_ADAQ4003] = {
+ .dev_name = "adaq4003",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+ },
+};
+
+enum ad4000_gains {
+ AD4000_0454_GAIN = 0,
+ AD4000_0909_GAIN = 1,
+ AD4000_1_GAIN = 2,
+ AD4000_1900_GAIN = 3,
+ AD4000_GAIN_LEN
+};
+
+/*
+ * Gains stored and computed as fractions to avoid introducing rounding errors.
+ */
+static const int ad4000_gains_frac[AD4000_GAIN_LEN][2] = {
+ [AD4000_0454_GAIN] = { 227, 500 },
+ [AD4000_0909_GAIN] = { 909, 1000 },
+ [AD4000_1_GAIN] = { 1, 1 },
+ [AD4000_1900_GAIN] = { 19, 10 },
+};
+
+struct ad4000_state {
+ struct spi_device *spi;
+ struct gpio_desc *cnv_gpio;
+ int vref;
+ bool status_bits;
+ bool span_comp;
+ bool turbo_mode;
+ bool high_z_mode;
+
+ enum ad4000_gains pin_gain;
+ int scale_tbl[AD4000_GAIN_LEN][2][2];
+
+ /*
+ * DMA (thus cache coherency maintenance) requires the
+ * transfer buffers to live in their own cache lines.
+ */
+ struct {
+ union {
+ u16 sample_buf16;
+ u32 sample_buf32;
+ } data;
+ s64 timestamp __aligned(8);
+ } scan;
+ __be16 tx_buf __aligned(IIO_DMA_MINALIGN);
+ __be16 rx_buf;
+};
+
+static void ad4000_fill_scale_tbl(struct ad4000_state *st, int scale_bits,
+ const struct ad4000_chip_info *chip)
+{
+ int diff = chip->chan_spec.differential;
+ int val, val2, tmp0, tmp1, i;
+ u64 tmp2;
+
+ val2 = scale_bits;
+ for (i = 0; i < AD4000_GAIN_LEN; i++) {
+ val = st->vref / 1000;
+ /* Multiply by MILLI here to avoid losing precision */
+ val = mult_frac(val, ad4000_gains_frac[i][1] * MILLI,
+ ad4000_gains_frac[i][0]);
+ /* Would multiply by NANO here but we already multiplied by MILLI */
+ tmp2 = shift_right((u64)val * MICRO, val2);
+ tmp0 = (int)div_s64_rem(tmp2, NANO, &tmp1);
+ /* Store scale for when span compression is disabled */
+ st->scale_tbl[i][0][0] = tmp0; /* Integer part */
+ st->scale_tbl[i][0][1] = abs(tmp1); /* Fractional part */
+ /* Store scale for when span compression is enabled */
+ st->scale_tbl[i][1][0] = tmp0;
+ if (diff)
+ st->scale_tbl[i][1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
+ else
+ st->scale_tbl[i][1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
+ }
+}
+
+static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
+{
+ put_unaligned_be16(AD400X_WRITE_COMMAND << BITS_PER_BYTE | val,
+ &st->tx_buf);
+ return spi_write(st->spi, &st->tx_buf, 2);
+}
+
+static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
+{
+ struct spi_transfer t[] = {
+ {
+ .tx_buf = &st->tx_buf,
+ .rx_buf = &st->rx_buf,
+ .len = 2,
+ },
+ };
+ int ret;
+
+ put_unaligned_be16(AD400X_READ_COMMAND << BITS_PER_BYTE, &st->tx_buf);
+ ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
+ if (ret < 0)
+ return ret;
+
+ *val = get_unaligned_be16(&st->rx_buf);
+
+ return ret;
+}
+
+static int ad4000_read_sample(struct ad4000_state *st,
+ const struct iio_chan_spec *chan)
+{
+ struct spi_transfer t[] = {
+ {
+ .rx_buf = &st->scan.data,
+ .len = BITS_TO_BYTES(chan->scan_type.storagebits),
+ .delay = {
+ .value = AD4000_TQUIET2_NS,
+ .unit = SPI_DELAY_UNIT_NSECS,
+ },
+ },
+ };
+ int ret;
+
+ ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int ad4000_single_conversion(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan, int *val)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+ u32 sample;
+ int ret;
+
+ if (st->cnv_gpio)
+ gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_HIGH);
+
+ ret = ad4000_read_sample(st, chan);
+ if (ret)
+ return ret;
+
+ if (st->cnv_gpio)
+ gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_LOW);
+
+ if (chan->scan_type.storagebits > 16)
+ sample = get_unaligned_be32(&st->scan.data);
+ else
+ sample = get_unaligned_be16(&st->scan.data);
+
+ switch (chan->scan_type.realbits) {
+ case 16:
+ break;
+ case 18:
+ sample = FIELD_GET(AD4000_18BIT_MSK, sample);
+ break;
+ case 20:
+ sample = FIELD_GET(AD4000_20BIT_MSK, sample);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (chan->scan_type.sign == 's')
+ *val = sign_extend32(sample, chan->scan_type.realbits - 1);
+
+ return IIO_VAL_INT;
+}
+
+static int ad4000_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long info)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return ad4000_single_conversion(indio_dev, chan, val);
+ unreachable();
+ case IIO_CHAN_INFO_SCALE:
+ *val = st->scale_tbl[st->pin_gain][st->span_comp][0];
+ *val2 = st->scale_tbl[st->pin_gain][st->span_comp][1];
+ return IIO_VAL_INT_PLUS_NANO;
+ case IIO_CHAN_INFO_OFFSET:
+ *val = 0;
+ if (st->span_comp)
+ *val = mult_frac(st->vref / 1000, 1, 10);
+
+ return IIO_VAL_INT;
+ default:
+ break;
+ }
+
+ return -EINVAL;
+}
+
+static int ad4000_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long info)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+
+ switch (info) {
+ case IIO_CHAN_INFO_SCALE:
+ *vals = (int *)st->scale_tbl[st->pin_gain];
+ *length = 2 * 2;
+ *type = IIO_VAL_INT_PLUS_NANO;
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ return IIO_VAL_INT_PLUS_NANO;
+ default:
+ return IIO_VAL_INT_PLUS_MICRO;
+ }
+ return -EINVAL;
+}
+
+static int ad4000_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val, int val2,
+ long mask)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+ unsigned int reg_val;
+ bool span_comp_en;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ ret = ad4000_read_reg(st, &reg_val);
+ if (ret < 0)
+ return ret;
+
+ span_comp_en = (val2 == st->scale_tbl[st->pin_gain][1][1]);
+ reg_val &= ~AD4000_SPAN_COMP;
+ reg_val |= FIELD_PREP(AD4000_SPAN_COMP, span_comp_en);
+
+ ret = ad4000_write_reg(st, reg_val);
+ if (ret < 0)
+ return ret;
+
+ st->span_comp = span_comp_en;
+ return 0;
+ }
+ unreachable();
+ default:
+ break;
+ }
+
+ return -EINVAL;
+}
+
+static irqreturn_t ad4000_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct ad4000_state *st = iio_priv(indio_dev);
+ int ret;
+
+ if (st->cnv_gpio)
+ gpiod_set_value(st->cnv_gpio, GPIOD_OUT_HIGH);
+
+ ret = ad4000_read_sample(st, &indio_dev->channels[0]);
+ if (ret < 0)
+ goto err_out;
+
+ if (st->cnv_gpio)
+ gpiod_set_value(st->cnv_gpio, GPIOD_OUT_LOW);
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
+ iio_get_time_ns(indio_dev));
+
+err_out:
+ iio_trigger_notify_done(indio_dev->trig);
+ return IRQ_HANDLED;
+}
+
+static const struct iio_info ad4000_info = {
+ .read_raw = &ad4000_read_raw,
+ .read_avail = &ad4000_read_avail,
+ .write_raw = &ad4000_write_raw,
+ .write_raw_get_fmt = &ad4000_write_raw_get_fmt,
+};
+
+static void ad4000_config(struct ad4000_state *st)
+{
+ unsigned int reg_val;
+ int ret;
+
+ reg_val = FIELD_PREP(AD4000_TURBO, 1);
+
+ if (device_property_present(&st->spi->dev, "adi,high-z-input"))
+ reg_val |= FIELD_PREP(AD4000_HIGHZ, 1);
+
+ /*
+ * The ADC SDI pin might be connected to controller CS line in which
+ * case the write might fail. This, however, does not prevent the device
+ * from functioning even though in a configuration other than the
+ * requested one.
+ */
+ ret = ad4000_write_reg(st, reg_val);
+ if (ret < 0)
+ dev_dbg(&st->spi->dev, "Failed to config device\n");
+}
+
+static void ad4000_regulator_disable(void *reg)
+{
+ regulator_disable(reg);
+}
+
+static int ad4000_probe(struct spi_device *spi)
+{
+ const struct ad4000_chip_info *chip;
+ struct regulator *vref_reg;
+ struct iio_dev *indio_dev;
+ struct ad4000_state *st;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ chip = spi_get_device_match_data(spi);
+ if (!chip)
+ return -EINVAL;
+
+ st = iio_priv(indio_dev);
+ st->spi = spi;
+
+ ret = devm_regulator_get_enable(&spi->dev, "vdd");
+ if (ret)
+ return dev_err_probe(&spi->dev, ret, "Failed to enable VDD supply\n");
+
+ ret = devm_regulator_get_enable(&spi->dev, "vio");
+ if (ret)
+ return dev_err_probe(&spi->dev, ret, "Failed to enable VIO supply\n");
+
+ vref_reg = devm_regulator_get(&spi->dev, "ref");
+ if (IS_ERR(vref_reg))
+ return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
+ "Failed to get vref regulator\n");
+
+ ret = regulator_enable(vref_reg);
+ if (ret < 0)
+ return dev_err_probe(&spi->dev, ret,
+ "Failed to enable voltage regulator\n");
+
+ ret = devm_add_action_or_reset(&spi->dev, ad4000_regulator_disable, vref_reg);
+ if (ret)
+ return dev_err_probe(&spi->dev, ret,
+ "Failed to add regulator disable action\n");
+
+ st->vref = regulator_get_voltage(vref_reg);
+ if (st->vref < 0)
+ return dev_err_probe(&spi->dev, st->vref, "Failed to get vref\n");
+
+ st->cnv_gpio = devm_gpiod_get_optional(&spi->dev, "cnv", GPIOD_OUT_HIGH);
+ if (IS_ERR(st->cnv_gpio)) {
+ if (PTR_ERR(st->cnv_gpio) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio),
+ "Failed to get CNV GPIO");
+ }
+
+ ad4000_config(st);
+
+ indio_dev->name = chip->dev_name;
+ indio_dev->info = &ad4000_info;
+ indio_dev->channels = &chip->chan_spec;
+ indio_dev->num_channels = 1;
+
+ st->pin_gain = AD4000_1_GAIN;
+ if (device_property_present(&spi->dev, "adi,gain-milli")) {
+ u32 val;
+
+ ret = device_property_read_u32(&spi->dev, "adi,gain-milli", &val);
+ if (ret)
+ return ret;
+
+ switch (val) {
+ case 454:
+ st->pin_gain = AD4000_0454_GAIN;
+ break;
+ case 909:
+ st->pin_gain = AD4000_0909_GAIN;
+ break;
+ case 1000:
+ st->pin_gain = AD4000_1_GAIN;
+ break;
+ case 1900:
+ st->pin_gain = AD4000_1900_GAIN;
+ break;
+ default:
+ return dev_err_probe(&spi->dev, -EINVAL,
+ "Invalid firmware provided gain\n");
+ }
+ }
+
+ /*
+ * ADCs that output twos complement code have one less bit to express
+ * voltage magnitude.
+ */
+ if (chip->chan_spec.scan_type.sign == 's')
+ ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits - 1,
+ chip);
+ else
+ ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits,
+ chip);
+
+ ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+ &iio_pollfunc_store_time,
+ &ad4000_trigger_handler, NULL);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id ad4000_id[] = {
+ { "ad4000", (kernel_ulong_t)&ad4000_chips[ID_AD4000] },
+ { "ad4001", (kernel_ulong_t)&ad4000_chips[ID_AD4001] },
+ { "ad4002", (kernel_ulong_t)&ad4000_chips[ID_AD4002] },
+ { "ad4003", (kernel_ulong_t)&ad4000_chips[ID_AD4003] },
+ { "ad4004", (kernel_ulong_t)&ad4000_chips[ID_AD4004] },
+ { "ad4005", (kernel_ulong_t)&ad4000_chips[ID_AD4005] },
+ { "ad4006", (kernel_ulong_t)&ad4000_chips[ID_AD4006] },
+ { "ad4007", (kernel_ulong_t)&ad4000_chips[ID_AD4007] },
+ { "ad4008", (kernel_ulong_t)&ad4000_chips[ID_AD4008] },
+ { "ad4010", (kernel_ulong_t)&ad4000_chips[ID_AD4010] },
+ { "ad4011", (kernel_ulong_t)&ad4000_chips[ID_AD4011] },
+ { "ad4020", (kernel_ulong_t)&ad4000_chips[ID_AD4020] },
+ { "ad4021", (kernel_ulong_t)&ad4000_chips[ID_AD4021] },
+ { "ad4022", (kernel_ulong_t)&ad4000_chips[ID_AD4022] },
+ { "adaq4001", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4001] },
+ { "adaq4003", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4003] },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, ad4000_id);
+
+static const struct of_device_id ad4000_of_match[] = {
+ { .compatible = "adi,ad4000", .data = &ad4000_chips[ID_AD4000] },
+ { .compatible = "adi,ad4001", .data = &ad4000_chips[ID_AD4001] },
+ { .compatible = "adi,ad4002", .data = &ad4000_chips[ID_AD4002] },
+ { .compatible = "adi,ad4003", .data = &ad4000_chips[ID_AD4003] },
+ { .compatible = "adi,ad4004", .data = &ad4000_chips[ID_AD4004] },
+ { .compatible = "adi,ad4005", .data = &ad4000_chips[ID_AD4005] },
+ { .compatible = "adi,ad4006", .data = &ad4000_chips[ID_AD4006] },
+ { .compatible = "adi,ad4007", .data = &ad4000_chips[ID_AD4007] },
+ { .compatible = "adi,ad4008", .data = &ad4000_chips[ID_AD4008] },
+ { .compatible = "adi,ad4010", .data = &ad4000_chips[ID_AD4010] },
+ { .compatible = "adi,ad4011", .data = &ad4000_chips[ID_AD4011] },
+ { .compatible = "adi,ad4020", .data = &ad4000_chips[ID_AD4020] },
+ { .compatible = "adi,ad4021", .data = &ad4000_chips[ID_AD4021] },
+ { .compatible = "adi,ad4022", .data = &ad4000_chips[ID_AD4022] },
+ { .compatible = "adi,adaq4001", .data = &ad4000_chips[ID_ADAQ4001] },
+ { .compatible = "adi,adaq4003", .data = &ad4000_chips[ID_ADAQ4003] },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ad4000_of_match);
+
+static struct spi_driver ad4000_driver = {
+ .driver = {
+ .name = "ad4000",
+ .of_match_table = ad4000_of_match,
+ },
+ .probe = ad4000_probe,
+ .id_table = ad4000_id,
+};
+module_spi_driver(ad4000_driver);
+
+MODULE_AUTHOR("Mircea Caprioru <[email protected]>");
+MODULE_AUTHOR("Marcelo Schmitt <[email protected]>");
+MODULE_DESCRIPTION("Analog Devices AD4000 ADC driver");
+MODULE_LICENSE("GPL");
--
2.43.0

2024-04-09 02:55:24

[permalink] [raw]

Subject: Re: [PATCH v2 0/2] Add support for AD4000 series

On Mon, Apr 8, 2024 at 9:31 AM Marcelo Schmitt
<[email protected]> wrote:
>
> This is more like an RFC patch set since configuration read/write is currently
> buggy.
>
> Change log v1 -> v2:
> - Took device tree provided by David.
> - Dropped ABI additions in favor of device tree properties.
> - Set differential IIO channel subtype for differential ADCs.
> - Set scan_type shift bits to mask out correct real bits from buffer.
> - Added __aligned(8) to buffer timestamp.
> - Used union to reduce buffer memory usage for 16-bit devices.
> - Used SPI transfer functions rather than SPI message.
> - Used c99 style structure initialization.
> - Used iio_device_claim_direct_scoped().
> - Removed unneeded pointer casts.
> - Added other power supplies (VDD and VIO).
>
> Link to v1: https://lore.kernel.org/linux-iio/[email protected]/
>
> Additional topics:
>
> - Why there is no different handling for the different SPI wiring modes?
> It looks like there is no need for different handling of "4-wire" and "3-wire"
> modes.
> If in "4-wire" (dt default mode), SDI is connected to SPI controller CS and
> CNV is active high. We can activate the CNV GPIO then let the SPI controller
> bring CS (connected to SDI) down when starting the transfer.
> If in "3-wire" (dt single mode), if we have a CNV (active low) GPIO we activate
> it and then proceed with with the transfer. If controller CS is connected to
> CNV it works the same way.
> I'm thinking it's better if we can support these devices in similar way
> other SPI ADCs are supported. Does that make sense?

In the AD7944 driver, I handled the "3-wire" mode separately because
the sample conversion is triggered on the rising edge of the CNV line.
In "4-wire" mode, since we have a GPIO connected to CNV, we can just
toggle the GPIO from low to high, wait for the conversion time
(t_CONV) and then read the sample (SPI xfer) then toggle the CNV line
low again. In 3-wire mode, the CS line is connected to the CNV pin, so
in order to get an up-to-date sample, we need to to toggle the CS line
from low to high to trigger a conversion (spi xfer with no data, only
delay), then wait for the conversion time, then read the sample (2nd
spi xfer). So in "4-wire" mode, the CS line is getting toggled once
per sample, but in "3-wire" mode, it is getting toggled twice per
sample. I didn't add support for "3-wire" mode where CNV is connected
to GPIO because we can't get max sample rate that way and it is
unusual to not have CS connected to something. But if we do that here,
the timing has to be different from 4-wire mode in order to not get
stale data.

> To me, the "3-wire" mode with controller CS to ADC CNV is what most resembles
> conventional SPI. The only important distinction is that the
> controller must be able to keep ADC SDI line high during conversions.
> Although, while the spi-engine implementation provided to me can keep SDI up
> during conversions, I'm not sure its a thing all SPI controllers can do.
> I tried a raspberry pi 4 some time ago and it was leaving the SDI line low if
> no tx buffer was provided. Even with a tx full of 1s the controller would
> bring SDI down between each 8 bits of transfer.

This is a good point. It sounds like additional bindings are needed to
describe the various wiring cases of the SDI line.

It sounds like possibilities are:

1. SDI is hard-wired high -> can't write to registers, CNV is
connected to SPI controller CS, chip is in "3-wire" mode. Currently
adi,spi-mode="single"
2. SDI is connected to SDO of another chip, SDI of last chip is
hard-wired low -> can't write to registers, CNV is connected to SPI
controller CS, chips are in daisy chain mode. Currently
adi,spi-mode="chain"
3. SDI is connected to SPI controller CS -> can't write registers,
chip can operate in 4-wire mode with CNV connected to GPIO, Currently
adi,spi-mode omitted.
4. SDI is connected to SPI controller SDO -> can write registers, and
support all writing modes (3-wire, 4-wire, daisy chain) as long as SPI
controller SDO line can be kept high or low at the appropriate time.
Currently not handled.
5. There could be a pin mux that switches between the one of the first
three and the 4th option (needed to avoid the issue with SPI
controller not being able to place the SDI pin in the correct state
during conversion trigger as described above).

On AD7944, the proposed adi,spi-mode property was sufficient to
describe what was wired to the SDI pin because we only had the first 3
options (the AD7944 doesn't have SPI registers to write to).

Also see related comments in my reply to the DT bindings patch.

(From the complete bindings point of view, we should probably also
consider the possibility of variations of 1. and 2. where CS of the
SPI controller is not wired and CNV is connected to a GPIO - this can
be determined by the combination of the adi,spi-mode property and the
presence or absence of the cnv-gpios property.)

> Anyway, single-shot and buffered reads work with the spi-engine controller
> with ADC in "3-wire"/single mode with controller CS line connected to ADC CNV
> pin which is how I've been testing it.

Technically, yes data can be captured in "3-wire" mode with a single
CS toggle, but then the data is stale and doesn't correspond to the
soft timestamp because it is reading the data from the previous
conversion triggered by the last SPI xfer, whenever that was. Since it
is trivial to avoid this by adding the extra CS/CNV toggle I describe
above, I don't see any reason not to.

But the way the driver is written now, it is actually only supporting
the unnamed wiring option 4 from above, so now I understand the
confusion about 3-wire vs. 4-wire mode in that context.

>
> - Why did not make vref regulator optional?
> Other SAR ADCs I've seen needed a voltage reference otherwise they simply
> could not provide any reasonable readings. Isn't it preferable to fail rather
> than having a device that can't provide reliable data?

In the device tree bindings, making vref-supply required makes sense
since there is no internal reference. In the driver, as discussed in
V1, it will fail if vref-supply in regulator_get_voltage() if
vref-supply is missing and we use devm_regulator_get() instead of
devm_regulator_get_optional(). So leaving it as-is is fine. We have a
plan to clean this up later anyway.

>
> - Why did not split into AD and ADAQ patches?
> The main difference between AD and ADAQ is the amplifier in front of the ADC.
> If only supporting AD, we could probably avoid the scale table since it would
> only have two possible values per ADC. But then the handling of span compression
> scale would need refactoring to be in the scale table when adding ADAQ.
> I'm not excited to implement something knowing it will need rework in the
> following patch. Will do if required.

If it isn't that much work, it seems worth it to me. If the driver
work is too much, maybe just split the DT patch?

>
> - Span compression and offset.
> For non-differential ADCs, enabling the span compression requires an input offset.
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/AD4000-4004-4008.pdf
> page 18
> and
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> page 19
> I updated the _offset attribute for those ADCs according to span compression
> being enabled or not. Is it okay to have an attribute update cause an update to
> another one?
> Maybe also make the span compression a dt property and have it fixed after probe?

This doesn't sound like something that belongs in DT since it doesn't
depend on the physical properties of what is wired to the input.

But the fact that offset should not be read until after scale is set
sounds like a quirk that would be worth documenting in some
chip-specific docs.

>
> - Configuration register
> Despite it doing single-shot and buffered captures, read and writes to the
> configuration register are currently buggy. It is as if the register was
> "floating". I tried setting up buffers like ad7768-1, adxl355_core, bma220_spi,
> bma400_core, and mcp3911.

If the ADC CNV pin is connected to a GPIO and the ADC SDI pin is
connected to SDO of the SPI controller, then nothing is connected to
CS of the SPI controller, so that might be the problem.

>
>
> Thanks,
> Marcelo
>
> Marcelo Schmitt (2):
> dt-bindings: iio: adc: Add AD4000
> iio: adc: Add support for AD4000
>
> .../bindings/iio/adc/adi,ad4000.yaml | 201 ++++++
> MAINTAINERS | 8 +
> drivers/iio/adc/Kconfig | 12 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ad4000.c | 649 ++++++++++++++++++
> 5 files changed, 871 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> create mode 100644 drivers/iio/adc/ad4000.c
>
> --
> 2.43.0
>
>

2024-04-09 02:57:23

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000

On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
<[email protected]> wrote:
>
> Add device tree documentation for AD4000 family of ADC devices.
>
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
>

Suggested-by: David Lechner <[email protected]>

(if you still use mostly my suggestions in the end)

> Signed-off-by: Marcelo Schmitt <[email protected]>
> ---
> .../bindings/iio/adc/adi,ad4000.yaml | 201 ++++++++++++++++++
> MAINTAINERS | 7 +
> 2 files changed, 208 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> new file mode 100644
> index 000000000000..ca06afb5149e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> @@ -0,0 +1,201 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD4000 and similar Analog to Digital Converters
> +
> +maintainers:
> + - Marcelo Schmitt <[email protected]>
> +
> +description: |
> + Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
> + Specifications can be found at:
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> +
> +$ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - adi,ad4000
> + - adi,ad4001
> + - adi,ad4002
> + - adi,ad4003
> + - adi,ad4004
> + - adi,ad4005
> + - adi,ad4006
> + - adi,ad4007
> + - adi,ad4008
> + - adi,ad4010
> + - adi,ad4011
> + - adi,ad4020
> + - adi,ad4021
> + - adi,ad4022
> + - adi,adaq4001
> + - adi,adaq4003
> +
> + reg:
> + maxItems: 1
> +
> + spi-max-frequency:
> + maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
> +
> + spi-cpha: true
> +
> + adi,spi-mode:
> + $ref: /schemas/types.yaml#/definitions/string
> + enum: [ single, chain ]

It sounds like there are more possible wiring configurations for these
chips that I thought when suggesting reusing this binding from AD7944
so we probably need more options here. (see my reply to the cover
letter for the complete context of these remarks)

We identified A) an additional wiring configuration where SDI of the
ADC chip is wired to SDO of the SPI controller and B) a potential need
to pin mux between wiring modes to work around SPI controller
limitations perhaps we could omit the adi,spi-mode property and just
use the standard pinctrl properties.

pinctrl-names:
description: |
Names for possible ways the SDI line of the controller is wired.

* default: The SDI line of the ADC is connected to the SDO line of the
SPI controller. CNV line of the ADC is connected to CS of the SPI
controller.
* single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
definition of 3-wire mode is NOT at all related to the standard
spi-3wire property!) In this mode, SDI is tied to VIO, and the CNV line
can be connected to the CS line of the SPI controller (typical) or to a
GPIO, in which case the CS line of the controller is unused. The SDO
line of the SPI controller is not connected.
* multi: The datasheet calls this "4-wire mode" and is used when multiple
chips are connected in parallel. In this mode, the ADC SDI line is tied
to the CS line on the SPI controller and the CNV line is connected to
a GPIO. The SDO line of the SPI controller is not connected.
* chain: The datasheet calls this "chain mode". This mode is used to save
on wiring when multiple ADCs are used. In this mode, the SDI line of
one chip is tied to the SDO of the next chip in the chain and the SDI of
the last chip in the chain is tied to GND. Only the first chip in the
chain is connected to the SPI bus. The CNV line of all chips are tied
together. The CS line of the SPI controller can be used as the CNV line
only if it is active high.

If one name is specified, it is assumed the chip is hard-wired in this
configuration.

If two names are specified, it is assumed that a pinmux can switch between
the two wiring configurations. The first is the default mode for reading
and writing registers on the chip and the second is the mode for reading
the conversion data from the chip.
oneOf:
- items:
- enum:
- default
- single
- multi
- chain
- items:
- const: default
- enum:
- single
- multi
- chain

pinctrl-0:
maxItems: 1

pinctrl-1:
maxItems: 1

> + description: |
> + This property indicates the SPI wiring configuration.
> +
> + When this property is omitted, it is assumed that the device is using what
> + the datasheet calls "4-wire mode". This is the conventional SPI mode used
> + when there are multiple devices on the same bus. In this mode, the CNV
> + line is used to initiate the conversion and the SDI line is connected to
> + CS on the SPI controller.
> +
> + When this property is present, it indicates that the device is using one
> + of the following alternative wiring configurations:
> +
> + * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
> + definition of 3-wire mode is NOT at all related to the standard
> + spi-3wire property!) This mode is often used when the ADC is the only
> + device on the bus. In this mode, SDI is tied to VIO, and the CNV line
> + can be connected to the CS line of the SPI controller or to a GPIO, in
> + which case the CS line of the controller is unused.
> + * chain: The datasheet calls this "chain mode". This mode is used to save
> + on wiring when multiple ADCs are used. In this mode, the SDI line of
> + one chip is tied to the SDO of the next chip in the chain and the SDI of
> + the last chip in the chain is tied to GND. Only the first chip in the
> + chain is connected to the SPI bus. The CNV line of all chips are tied
> + together. The CS line of the SPI controller can be used as the CNV line
> + only if it is active high.
> +
> + '#daisy-chained-devices': true
> +
> + vdd-supply:
> + description: A 1.8V supply that powers the chip (VDD).
> +
> + vio-supply:
> + description:
> + A 1.8V to 5.5V supply for the digital inputs and outputs (VIO).
> +
> + ref-supply:
> + description:
> + A 2.5 to 5V supply for the external reference voltage (REF).
> +
> + cnv-gpios:
> + description:
> + The Convert Input (CNV). This input has multiple functions. It initiates
> + the conversions and selects the SPI mode of the device (chain or CS). In
> + 'single' mode, this property is omitted if the CNV pin is connected to the
> + CS line of the SPI controller. If 'single' mode is selected and this GPIO
> + is provided, it must be active low.

Since the conversion is triggered on the low to high transition of
CNV, I think it only makes sense to have it active high and not active
low.

> + maxItems: 1
> +
> + adi,high-z-input:
> + type: boolean
> + description:
> + High-Z mode allows the amplifier and RC filter in front of the ADC to be
> + chosen based on the signal bandwidth of interest, rather than the settling
> + requirements of the switched capacitor SAR ADC inputs.
> +
> + adi,gain-milli:
> + description: |
> + The hardware gain applied to the ADC input (in milli units).
> + The gain provided by the ADC input scaler is defined by the hardware
> + connections between chip pins OUT+, R1K-, R1K1-, R1K+, R1K1+, and OUT-.
> + If not present, default to 1000 (no actual gain applied).
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [454, 909, 1000, 1900]
> + default: 1000

Same suggestion as in V1 - we should make it clear that this property
only applies to ADAQ chips (in the description and also a -if: for the
bindings validator). Also, looking at the datasheet, it looks like
there are a lot more pins on the ADAQ chips, so I think there are more
properties missing here.

Some trivial ones:

vs-pos-supply (VS+ pin, 0 to 11V supply) and vs-neg-supply (VS- pin,
-11 to 0V supply)

pd-amp-gpios (active low) and pd-ref-gpios (active low) for optional
runtime power management.

Also the datasheet says the ADAQ chips supports "Single-ended to
differential conversion". So it seems like we might need some extra
properties to describe that case (a flag for indicating single-ended
wiring and an optional voltage supply to describe what is connected to
the negative input if it isn't tied to GND)

2024-04-09 03:05:35

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] iio: adc: Add support for AD4000

On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
<[email protected]> wrote:
>
> Add support for AD4000 family of low noise, low power, high speed,
> successive aproximation register (SAR) ADCs.
>
> Signed-off-by: Marcelo Schmitt <[email protected]>
> ---
> MAINTAINERS | 1 +
> drivers/iio/adc/Kconfig | 12 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ad4000.c | 649 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 663 insertions(+)
> create mode 100644 drivers/iio/adc/ad4000.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5dfe118a5dd3..86aa96115f5a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1165,6 +1165,7 @@ L: [email protected]
> S: Supported
> W: https://ez.analog.com/linux-software-drivers
> F: Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> +F: drivers/iio/adc/ad4000.c
>
> ANALOG DEVICES INC AD4130 DRIVER
> M: Cosmin Tanislav <[email protected]>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 8db68b80b391..9c9d13d4b74f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -21,6 +21,18 @@ config AD_SIGMA_DELTA
> select IIO_BUFFER
> select IIO_TRIGGERED_BUFFER
>
> +config AD4000
> + tristate "Analog Devices AD4000 ADC Driver"
> + depends on SPI
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + Say yes here to build support for Analog Devices AD4000 high speed
> + SPI analog to digital converters (ADC).
> +
> + To compile this driver as a module, choose M here: the module will be
> + called ad4000.
> +
> config AD4130
> tristate "Analog Device AD4130 ADC Driver"
> depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index edb32ce2af02..aa52068d864b 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -6,6 +6,7 @@
> # When adding new entries keep the list in alphabetical order
> obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
> obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> +obj-$(CONFIG_AD4000) += ad4000.o
> obj-$(CONFIG_AD4130) += ad4130.o
> obj-$(CONFIG_AD7091R) += ad7091r-base.o
> obj-$(CONFIG_AD7091R5) += ad7091r5.o
> diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
> new file mode 100644
> index 000000000000..7997d9d98743
> --- /dev/null
> +++ b/drivers/iio/adc/ad4000.c
> @@ -0,0 +1,649 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD4000 SPI ADC driver
> + *
> + * Copyright 2024 Analog Devices Inc.
> + */
> +#include <asm/unaligned.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +#include <linux/units.h>
> +#include <linux/util_macros.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#define AD400X_READ_COMMAND 0x54
> +#define AD400X_WRITE_COMMAND 0x14
> +
> +/* AD4000 Configuration Register programmable bits */
> +#define AD4000_STATUS BIT(4) /* Status bits output */
> +#define AD4000_SPAN_COMP BIT(3) /* Input span compression */
> +#define AD4000_HIGHZ BIT(2) /* High impedance mode */
> +#define AD4000_TURBO BIT(1) /* Turbo mode */

Usually bits of the same register share a similar prefix, e.g.
AD4000_CFG_TURBO, AD4000_CFG_HIGHZ, etc.

> +
> +#define AD4000_TQUIET2_NS 60
> +
> +#define AD4000_18BIT_MSK GENMASK(31, 14)
> +#define AD4000_20BIT_MSK GENMASK(31, 12)
> +
> +#define AD4000_DIFF_CHANNEL(_sign, _real_bits) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .differential = 1, \
> + .channel = 0, \
> + .channel2 = 1, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
> + .scan_type = { \
> + .sign = _sign, \
> + .realbits = _real_bits, \
> + .storagebits = _real_bits > 16 ? 32 : 16, \
> + .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
> + .endianness = IIO_BE, \
> + }, \
> + } \
> +
> +#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = 0, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
> + .scan_type = { \
> + .sign = _sign, \
> + .realbits = _real_bits, \
> + .storagebits = _real_bits > 16 ? 32 : 16, \
> + .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
> + .endianness = IIO_BE, \
> + }, \
> + } \

It looks like all differential chips are signed and all
pseduo-differential chips are unsigned, so I don't think we need the
_sign parameter in these macros.

I also still have doubts about using IIO_BE and 8-bit xfers when it
comes to adding support later to achieve max sample rate with a SPI
offload. For example to get 2MSPS with an 18-bit chip, it will require
an approx 33% faster SPI clock than the actual slowest clock possible
because it will have to read 6 extra bits per sample. I didn't check
the specs, but this may not even be physically possible without
exceeding the datasheet max SPI clock rate. Also errors could be
reduced if we could actually use the slowest allowable SPI clock rate.
Furthermore, the offload hardware would have to be capable of adding
an extra byte per sample for 18 and 20-bit chips when piping the data
to DMA in order to get the 32-bit alignment in the buffer required by
IIO scan_type and the natural alignment requirements of IIO buffers in
general.

> +
> +enum ad4000_ids {
> + ID_AD4000,
> + ID_AD4001,
> + ID_AD4002,
> + ID_AD4003,
> + ID_AD4004,
> + ID_AD4005,
> + ID_AD4006,
> + ID_AD4007,
> + ID_AD4008,
> + ID_AD4010,
> + ID_AD4011,
> + ID_AD4020,
> + ID_AD4021,
> + ID_AD4022,
> + ID_ADAQ4001,
> + ID_ADAQ4003,
> +};
> +
> +struct ad4000_chip_info {
> + const char *dev_name;
> + struct iio_chan_spec chan_spec;
> +};
> +
> +static const struct ad4000_chip_info ad4000_chips[] = {
> + [ID_AD4000] = {
> + .dev_name = "ad4000",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> + },
> + [ID_AD4001] = {
> + .dev_name = "ad4001",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> + },
> + [ID_AD4002] = {
> + .dev_name = "ad4002",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> + },
> + [ID_AD4003] = {
> + .dev_name = "ad4003",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> + },
> + [ID_AD4004] = {
> + .dev_name = "ad4004",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> + },
> + [ID_AD4005] = {
> + .dev_name = "ad4005",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> + },
> + [ID_AD4006] = {
> + .dev_name = "ad4006",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> + },
> + [ID_AD4007] = {
> + .dev_name = "ad4007",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> + },
> + [ID_AD4008] = {
> + .dev_name = "ad4008",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> + },
> + [ID_AD4010] = {
> + .dev_name = "ad4010",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> + },
> + [ID_AD4011] = {
> + .dev_name = "ad4011",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> + },
> + [ID_AD4020] = {
> + .dev_name = "ad4020",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> + },
> + [ID_AD4021] = {
> + .dev_name = "ad4021",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> + },
> + [ID_AD4022] = {
> + .dev_name = "ad4022",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> + },
> + [ID_ADAQ4001] = {
> + .dev_name = "adaq4001",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> + },
> + [ID_ADAQ4003] = {
> + .dev_name = "adaq4003",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> + },
> +};
> +
> +enum ad4000_gains {
> + AD4000_0454_GAIN = 0,
> + AD4000_0909_GAIN = 1,
> + AD4000_1_GAIN = 2,

AD4000_1000_GAIN would be more consistent with the others.

> + AD4000_1900_GAIN = 3,
> + AD4000_GAIN_LEN
> +};
> +
> +/*
> + * Gains stored and computed as fractions to avoid introducing rounding errors.
> + */
> +static const int ad4000_gains_frac[AD4000_GAIN_LEN][2] = {
> + [AD4000_0454_GAIN] = { 227, 500 },
> + [AD4000_0909_GAIN] = { 909, 1000 },
> + [AD4000_1_GAIN] = { 1, 1 },
> + [AD4000_1900_GAIN] = { 19, 10 },
> +};

Why not just store the numerator in milli units and always use 1000
for the denominator? It seems like it would simplify the code and make
it easier to read and understand. Also, these values are coming from
the adi,gain-milli property already, so we could avoid the enum and
the lookup table entirely and simplify things even more.

> +
> +struct ad4000_state {
> + struct spi_device *spi;
> + struct gpio_desc *cnv_gpio;
> + int vref;
> + bool status_bits;
> + bool span_comp;
> + bool turbo_mode;
> + bool high_z_mode;
> +
> + enum ad4000_gains pin_gain;
> + int scale_tbl[AD4000_GAIN_LEN][2][2];
> +
> + /*
> + * DMA (thus cache coherency maintenance) requires the
> + * transfer buffers to live in their own cache lines.
> + */
> + struct {
> + union {
> + u16 sample_buf16;
> + u32 sample_buf32;

Technically, these are holding big-endian data, so __be16 and __be32
would be more correct.

> + } data;
> + s64 timestamp __aligned(8);
> + } scan;
> + __be16 tx_buf __aligned(IIO_DMA_MINALIGN);
> + __be16 rx_buf;
> +};

scan.data is used as SPI rx_buf so __aligned(IIO_DMA_MINALIGN); needs
to be moved to the scan field.

> +
> +static void ad4000_fill_scale_tbl(struct ad4000_state *st, int scale_bits,
> + const struct ad4000_chip_info *chip)
> +{
> + int diff = chip->chan_spec.differential;
> + int val, val2, tmp0, tmp1, i;
> + u64 tmp2;
> +
> + val2 = scale_bits;
> + for (i = 0; i < AD4000_GAIN_LEN; i++) {

Only one gain is selected by the devicetree, so why do we need to do
this for all 4 gains?

> + val = st->vref / 1000;
> + /* Multiply by MILLI here to avoid losing precision */
> + val = mult_frac(val, ad4000_gains_frac[i][1] * MILLI,
> + ad4000_gains_frac[i][0]);
> + /* Would multiply by NANO here but we already multiplied by MILLI */
> + tmp2 = shift_right((u64)val * MICRO, val2);
> + tmp0 = (int)div_s64_rem(tmp2, NANO, &tmp1);
> + /* Store scale for when span compression is disabled */
> + st->scale_tbl[i][0][0] = tmp0; /* Integer part */
> + st->scale_tbl[i][0][1] = abs(tmp1); /* Fractional part */
> + /* Store scale for when span compression is enabled */
> + st->scale_tbl[i][1][0] = tmp0;
> + if (diff)
> + st->scale_tbl[i][1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
> + else
> + st->scale_tbl[i][1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
> + }
> +}
> +
> +static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
> +{
> + put_unaligned_be16(AD400X_WRITE_COMMAND << BITS_PER_BYTE | val,
> + &st->tx_buf);
> + return spi_write(st->spi, &st->tx_buf, 2);
> +}
> +
> +static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
> +{
> + struct spi_transfer t[] = {
> + {
> + .tx_buf = &st->tx_buf,
> + .rx_buf = &st->rx_buf,
> + .len = 2,
> + },
> + };
> + int ret;
> +
> + put_unaligned_be16(AD400X_READ_COMMAND << BITS_PER_BYTE, &st->tx_buf);
> + ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> + if (ret < 0)
> + return ret;
> +
> + *val = get_unaligned_be16(&st->rx_buf);
> +
> + return ret;
> +}
> +

It would be very helpful to have comments here explaining the exact
expected wiring configuration and signal timing here since there are
so many possibilities for this chip.

> +static int ad4000_read_sample(struct ad4000_state *st,
> + const struct iio_chan_spec *chan)
> +{
> + struct spi_transfer t[] = {

Don't really need [] here since there is only one xfer.

> + {
> + .rx_buf = &st->scan.data,
> + .len = BITS_TO_BYTES(chan->scan_type.storagebits),
> + .delay = {
> + .value = AD4000_TQUIET2_NS,
> + .unit = SPI_DELAY_UNIT_NSECS,
> + },
> + },
> + };
> + int ret;
> +
> + ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int ad4000_single_conversion(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan, int *val)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> + u32 sample;
> + int ret;
> +
> + if (st->cnv_gpio)
> + gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_HIGH);

It would make more sense and be less redundant to move the gpio code
into ad4000_read_sample().

Also, gpiod_set_value_cansleep() checks for NULL, so the if () is redundant.

> +
> + ret = ad4000_read_sample(st, chan);
> + if (ret)
> + return ret;
> +
> + if (st->cnv_gpio)
> + gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_LOW);
> +
> + if (chan->scan_type.storagebits > 16)
> + sample = get_unaligned_be32(&st->scan.data);
> + else
> + sample = get_unaligned_be16(&st->scan.data);

data is aligned, so be32/16_to_cpu() should be fine. Also, Jonathan
will suggest to use &st->scan.data.sample_b32/16 here too. :-)

> +
> + switch (chan->scan_type.realbits) {
> + case 16:
> + break;
> + case 18:
> + sample = FIELD_GET(AD4000_18BIT_MSK, sample);
> + break;
> + case 20:
> + sample = FIELD_GET(AD4000_20BIT_MSK, sample);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (chan->scan_type.sign == 's')
> + *val = sign_extend32(sample, chan->scan_type.realbits - 1);
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int ad4000_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long info)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> + return ad4000_single_conversion(indio_dev, chan, val);
> + unreachable();
> + case IIO_CHAN_INFO_SCALE:
> + *val = st->scale_tbl[st->pin_gain][st->span_comp][0];
> + *val2 = st->scale_tbl[st->pin_gain][st->span_comp][1];
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_OFFSET:
> + *val = 0;
> + if (st->span_comp)
> + *val = mult_frac(st->vref / 1000, 1, 10);
> +
> + return IIO_VAL_INT;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ad4000_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long info)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SCALE:
> + *vals = (int *)st->scale_tbl[st->pin_gain];
> + *length = 2 * 2;
> + *type = IIO_VAL_INT_PLUS_NANO;
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> + return -EINVAL;

not reachable because of default, so can be left out

> +}
> +
> +static int ad4000_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val, int val2,
> + long mask)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> + unsigned int reg_val;
> + bool span_comp_en;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + ret = ad4000_read_reg(st, &reg_val);
> + if (ret < 0)
> + return ret;
> +
> + span_comp_en = (val2 == st->scale_tbl[st->pin_gain][1][1]);
> + reg_val &= ~AD4000_SPAN_COMP;
> + reg_val |= FIELD_PREP(AD4000_SPAN_COMP, span_comp_en);
> +
> + ret = ad4000_write_reg(st, reg_val);
> + if (ret < 0)
> + return ret;
> +
> + st->span_comp = span_comp_en;
> + return 0;
> + }
> + unreachable();

Can bring out the return 0 to avoid unreachable.

> + default:
> + break;

Can return -EINVAL to avoid break;

> + }
> +
> + return -EINVAL;
> +}
> +
> +static irqreturn_t ad4000_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct ad4000_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (st->cnv_gpio)
> + gpiod_set_value(st->cnv_gpio, GPIOD_OUT_HIGH);
> +
> + ret = ad4000_read_sample(st, &indio_dev->channels[0]);
> + if (ret < 0)
> + goto err_out;
> +
> + if (st->cnv_gpio)
> + gpiod_set_value(st->cnv_gpio, GPIOD_OUT_LOW);
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
> + iio_get_time_ns(indio_dev));
> +
> +err_out:
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info ad4000_info = {
> + .read_raw = &ad4000_read_raw,
> + .read_avail = &ad4000_read_avail,
> + .write_raw = &ad4000_write_raw,
> + .write_raw_get_fmt = &ad4000_write_raw_get_fmt,
> +};
> +
> +static void ad4000_config(struct ad4000_state *st)
> +{
> + unsigned int reg_val;
> + int ret;
> +
> + reg_val = FIELD_PREP(AD4000_TURBO, 1);

Since the driver in it's current state can get anywhere near the max
sample rate of ~1MSPS, I don't think it makes sense to enable turbo at
this point.

> +
> + if (device_property_present(&st->spi->dev, "adi,high-z-input"))
> + reg_val |= FIELD_PREP(AD4000_HIGHZ, 1);
> +
> + /*
> + * The ADC SDI pin might be connected to controller CS line in which
> + * case the write might fail. This, however, does not prevent the device
> + * from functioning even though in a configuration other than the
> + * requested one.
> + */
> + ret = ad4000_write_reg(st, reg_val);
> + if (ret < 0)
> + dev_dbg(&st->spi->dev, "Failed to config device\n");

If writing fails because there is no CS line wired up, we won't get an
error returned here. The SPI controller has no way of knowing this
happened, so it can only assume the write was successful and return 0.
So this should return ret.

Ideally, the devicetree should tell us if CS is wired up or not.

> +}
> +
> +static void ad4000_regulator_disable(void *reg)
> +{
> + regulator_disable(reg);
> +}
> +
> +static int ad4000_probe(struct spi_device *spi)
> +{
> + const struct ad4000_chip_info *chip;
> + struct regulator *vref_reg;
> + struct iio_dev *indio_dev;
> + struct ad4000_state *st;
> + int ret;

We need a check somewhere in here to make sure that adi,spi-mode is in
a supported configuration. E.g. chain mode is not currently
implemented.

> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + chip = spi_get_device_match_data(spi);
> + if (!chip)
> + return -EINVAL;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> +
> + ret = devm_regulator_get_enable(&spi->dev, "vdd");
> + if (ret)
> + return dev_err_probe(&spi->dev, ret, "Failed to enable VDD supply\n");
> +
> + ret = devm_regulator_get_enable(&spi->dev, "vio");
> + if (ret)
> + return dev_err_probe(&spi->dev, ret, "Failed to enable VIO supply\n");
> +
> + vref_reg = devm_regulator_get(&spi->dev, "ref");
> + if (IS_ERR(vref_reg))
> + return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
> + "Failed to get vref regulator\n");
> +
> + ret = regulator_enable(vref_reg);
> + if (ret < 0)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to enable voltage regulator\n");
> +
> + ret = devm_add_action_or_reset(&spi->dev, ad4000_regulator_disable, vref_reg);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to add regulator disable action\n");
> +
> + st->vref = regulator_get_voltage(vref_reg);
> + if (st->vref < 0)
> + return dev_err_probe(&spi->dev, st->vref, "Failed to get vref\n");
> +
> + st->cnv_gpio = devm_gpiod_get_optional(&spi->dev, "cnv", GPIOD_OUT_HIGH);
> + if (IS_ERR(st->cnv_gpio)) {
> + if (PTR_ERR(st->cnv_gpio) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;

EPROBE_DEFER check is not needed with dev_err_probe();, it already does that.

> +
> + return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio),
> + "Failed to get CNV GPIO");
> + }
> +
> + ad4000_config(st);
> +
> + indio_dev->name = chip->dev_name;
> + indio_dev->info = &ad4000_info;
> + indio_dev->channels = &chip->chan_spec;
> + indio_dev->num_channels = 1;
> +
> + st->pin_gain = AD4000_1_GAIN;
> + if (device_property_present(&spi->dev, "adi,gain-milli")) {
> + u32 val;

Should it be an error if adi,gain-milli is set on non-adaq chips?

> +
> + ret = device_property_read_u32(&spi->dev, "adi,gain-milli", &val);
> + if (ret)
> + return ret;
> +
> + switch (val) {
> + case 454:
> + st->pin_gain = AD4000_0454_GAIN;
> + break;
> + case 909:
> + st->pin_gain = AD4000_0909_GAIN;
> + break;
> + case 1000:
> + st->pin_gain = AD4000_1_GAIN;
> + break;
> + case 1900:
> + st->pin_gain = AD4000_1900_GAIN;
> + break;
> + default:
> + return dev_err_probe(&spi->dev, -EINVAL,
> + "Invalid firmware provided gain\n");

Could help debugging if val is included in the error message.

> + }
> + }
> +
> + /*
> + * ADCs that output twos complement code have one less bit to express
> + * voltage magnitude.
> + */
> + if (chip->chan_spec.scan_type.sign == 's')
> + ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits - 1,
> + chip);
> + else
> + ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits,
> + chip);
> +
> + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> + &iio_pollfunc_store_time,
> + &ad4000_trigger_handler, NULL);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct spi_device_id ad4000_id[] = {
> + { "ad4000", (kernel_ulong_t)&ad4000_chips[ID_AD4000] },
> + { "ad4001", (kernel_ulong_t)&ad4000_chips[ID_AD4001] },
> + { "ad4002", (kernel_ulong_t)&ad4000_chips[ID_AD4002] },
> + { "ad4003", (kernel_ulong_t)&ad4000_chips[ID_AD4003] },
> + { "ad4004", (kernel_ulong_t)&ad4000_chips[ID_AD4004] },
> + { "ad4005", (kernel_ulong_t)&ad4000_chips[ID_AD4005] },
> + { "ad4006", (kernel_ulong_t)&ad4000_chips[ID_AD4006] },
> + { "ad4007", (kernel_ulong_t)&ad4000_chips[ID_AD4007] },
> + { "ad4008", (kernel_ulong_t)&ad4000_chips[ID_AD4008] },
> + { "ad4010", (kernel_ulong_t)&ad4000_chips[ID_AD4010] },
> + { "ad4011", (kernel_ulong_t)&ad4000_chips[ID_AD4011] },
> + { "ad4020", (kernel_ulong_t)&ad4000_chips[ID_AD4020] },
> + { "ad4021", (kernel_ulong_t)&ad4000_chips[ID_AD4021] },
> + { "ad4022", (kernel_ulong_t)&ad4000_chips[ID_AD4022] },
> + { "adaq4001", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4001] },
> + { "adaq4003", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4003] },
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, ad4000_id);
> +
> +static const struct of_device_id ad4000_of_match[] = {
> + { .compatible = "adi,ad4000", .data = &ad4000_chips[ID_AD4000] },
> + { .compatible = "adi,ad4001", .data = &ad4000_chips[ID_AD4001] },
> + { .compatible = "adi,ad4002", .data = &ad4000_chips[ID_AD4002] },
> + { .compatible = "adi,ad4003", .data = &ad4000_chips[ID_AD4003] },
> + { .compatible = "adi,ad4004", .data = &ad4000_chips[ID_AD4004] },
> + { .compatible = "adi,ad4005", .data = &ad4000_chips[ID_AD4005] },
> + { .compatible = "adi,ad4006", .data = &ad4000_chips[ID_AD4006] },
> + { .compatible = "adi,ad4007", .data = &ad4000_chips[ID_AD4007] },
> + { .compatible = "adi,ad4008", .data = &ad4000_chips[ID_AD4008] },
> + { .compatible = "adi,ad4010", .data = &ad4000_chips[ID_AD4010] },
> + { .compatible = "adi,ad4011", .data = &ad4000_chips[ID_AD4011] },
> + { .compatible = "adi,ad4020", .data = &ad4000_chips[ID_AD4020] },
> + { .compatible = "adi,ad4021", .data = &ad4000_chips[ID_AD4021] },
> + { .compatible = "adi,ad4022", .data = &ad4000_chips[ID_AD4022] },
> + { .compatible = "adi,adaq4001", .data = &ad4000_chips[ID_ADAQ4001] },
> + { .compatible = "adi,adaq4003", .data = &ad4000_chips[ID_ADAQ4003] },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad4000_of_match);
> +
> +static struct spi_driver ad4000_driver = {
> + .driver = {
> + .name = "ad4000",
> + .of_match_table = ad4000_of_match,
> + },
> + .probe = ad4000_probe,
> + .id_table = ad4000_id,
> +};
> +module_spi_driver(ad4000_driver);
> +
> +MODULE_AUTHOR("Mircea Caprioru <[email protected]>");
> +MODULE_AUTHOR("Marcelo Schmitt <[email protected]>");
> +MODULE_DESCRIPTION("Analog Devices AD4000 ADC driver");
> +MODULE_LICENSE("GPL");
> --
> 2.43.0
>
>

2024-04-09 15:34:28

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000

On 04/08, David Lechner wrote:
> On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
> <[email protected]> wrote:
> >
> > Add device tree documentation for AD4000 family of ADC devices.
> >
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> >
>
> Suggested-by: David Lechner <[email protected]>
>
> (if you still use mostly my suggestions in the end)

Yes, it's been of great help. Will include the tag in future ad4000 DT patches.

>
> > Signed-off-by: Marcelo Schmitt <[email protected]>
> > ---
> > .../bindings/iio/adc/adi,ad4000.yaml | 201 ++++++++++++++++++
> > MAINTAINERS | 7 +
> > 2 files changed, 208 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > new file mode 100644
> > index 000000000000..ca06afb5149e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > @@ -0,0 +1,201 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices AD4000 and similar Analog to Digital Converters
> > +
> > +maintainers:
> > + - Marcelo Schmitt <[email protected]>
> > +
> > +description: |
> > + Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
> > + Specifications can be found at:
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> > +
> > +$ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - adi,ad4000
> > + - adi,ad4001
> > + - adi,ad4002
> > + - adi,ad4003
> > + - adi,ad4004
> > + - adi,ad4005
> > + - adi,ad4006
> > + - adi,ad4007
> > + - adi,ad4008
> > + - adi,ad4010
> > + - adi,ad4011
> > + - adi,ad4020
> > + - adi,ad4021
> > + - adi,ad4022
> > + - adi,adaq4001
> > + - adi,adaq4003
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + spi-max-frequency:
> > + maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
> > +
> > + spi-cpha: true
> > +
> > + adi,spi-mode:
> > + $ref: /schemas/types.yaml#/definitions/string
> > + enum: [ single, chain ]
>
> It sounds like there are more possible wiring configurations for these
> chips that I thought when suggesting reusing this binding from AD7944
> so we probably need more options here. (see my reply to the cover
> letter for the complete context of these remarks)
>
> We identified A) an additional wiring configuration where SDI of the
> ADC chip is wired to SDO of the SPI controller and B) a potential need
> to pin mux between wiring modes to work around SPI controller
> limitations perhaps we could omit the adi,spi-mode property and just
> use the standard pinctrl properties.
>
> pinctrl-names:
> description: |
> Names for possible ways the SDI line of the controller is wired.
>
> * default: The SDI line of the ADC is connected to the SDO line of the
> SPI controller. CNV line of the ADC is connected to CS of the SPI
> controller.
Not sure if should be DT, but maybe also point out that in default mode the
SPI controller must be capable of keeping ADC SDI (controller SDO) line high
during ADC conversions.

> * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
> definition of 3-wire mode is NOT at all related to the standard
> spi-3wire property!) In this mode, SDI is tied to VIO, and the CNV line
> can be connected to the CS line of the SPI controller (typical) or to a
> GPIO, in which case the CS line of the controller is unused. The SDO
> line of the SPI controller is not connected.
> * multi: The datasheet calls this "4-wire mode" and is used when multiple
> chips are connected in parallel. In this mode, the ADC SDI line is tied
> to the CS line on the SPI controller and the CNV line is connected to
> a GPIO. The SDO line of the SPI controller is not connected.
> * chain: The datasheet calls this "chain mode". This mode is used to save
> on wiring when multiple ADCs are used. In this mode, the SDI line of
> one chip is tied to the SDO of the next chip in the chain and the SDI of
> the last chip in the chain is tied to GND. Only the first chip in the
> chain is connected to the SPI bus. The CNV line of all chips are tied
> together. The CS line of the SPI controller can be used as the CNV line
> only if it is active high.
>
> If one name is specified, it is assumed the chip is hard-wired in this
> configuration.
>
> If two names are specified, it is assumed that a pinmux can switch between
> the two wiring configurations. The first is the default mode for reading
> and writing registers on the chip and the second is the mode for reading
> the conversion data from the chip.
> oneOf:
> - items:
> - enum:
> - default
> - single
> - multi
> - chain
> - items:
> - const: default
> - enum:
> - single
> - multi
> - chain
>
> pinctrl-0:
> maxItems: 1
>
> pinctrl-1:
> maxItems: 1
>
>
> > + description: |
> > + This property indicates the SPI wiring configuration.
> > +
> > + When this property is omitted, it is assumed that the device is using what
> > + the datasheet calls "4-wire mode". This is the conventional SPI mode used
> > + when there are multiple devices on the same bus. In this mode, the CNV
> > + line is used to initiate the conversion and the SDI line is connected to
> > + CS on the SPI controller.
> > +
> > + When this property is present, it indicates that the device is using one
> > + of the following alternative wiring configurations:
> > +
> > + * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
> > + definition of 3-wire mode is NOT at all related to the standard
> > + spi-3wire property!) This mode is often used when the ADC is the only
> > + device on the bus. In this mode, SDI is tied to VIO, and the CNV line
> > + can be connected to the CS line of the SPI controller or to a GPIO, in
> > + which case the CS line of the controller is unused.
> > + * chain: The datasheet calls this "chain mode". This mode is used to save
> > + on wiring when multiple ADCs are used. In this mode, the SDI line of
> > + one chip is tied to the SDO of the next chip in the chain and the SDI of
> > + the last chip in the chain is tied to GND. Only the first chip in the
> > + chain is connected to the SPI bus. The CNV line of all chips are tied
> > + together. The CS line of the SPI controller can be used as the CNV line
> > + only if it is active high.
> > +
> > + '#daisy-chained-devices': true
> > +
> > + vdd-supply:
> > + description: A 1.8V supply that powers the chip (VDD).
> > +
> > + vio-supply:
> > + description:
> > + A 1.8V to 5.5V supply for the digital inputs and outputs (VIO).
> > +
> > + ref-supply:
> > + description:
> > + A 2.5 to 5V supply for the external reference voltage (REF).
> > +
> > + cnv-gpios:
> > + description:
> > + The Convert Input (CNV). This input has multiple functions. It initiates
> > + the conversions and selects the SPI mode of the device (chain or CS). In
> > + 'single' mode, this property is omitted if the CNV pin is connected to the
> > + CS line of the SPI controller. If 'single' mode is selected and this GPIO
> > + is provided, it must be active low.
>
> Since the conversion is triggered on the low to high transition of
> CNV, I think it only makes sense to have it active high and not active
> low.

The idea was to use the GPIO as a replacement for the controller CS when
in "3-wire"/single mode so we could have simpler handling of SPI transfers.
But if changing transfer to avoid latency then this might not simplify anything
anymore. Will probably drop this last line.

>
> > + maxItems: 1
> > +
> > + adi,high-z-input:
> > + type: boolean
> > + description:
> > + High-Z mode allows the amplifier and RC filter in front of the ADC to be
> > + chosen based on the signal bandwidth of interest, rather than the settling
> > + requirements of the switched capacitor SAR ADC inputs.
> > +
> > + adi,gain-milli:
> > + description: |
> > + The hardware gain applied to the ADC input (in milli units).
> > + The gain provided by the ADC input scaler is defined by the hardware
> > + connections between chip pins OUT+, R1K-, R1K1-, R1K+, R1K1+, and OUT-.
> > + If not present, default to 1000 (no actual gain applied).
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [454, 909, 1000, 1900]
> > + default: 1000
>
> Same suggestion as in V1 - we should make it clear that this property
> only applies to ADAQ chips (in the description and also a -if: for the
> bindings validator). Also, looking at the datasheet, it looks like
> there are a lot more pins on the ADAQ chips, so I think there are more
> properties missing here.
>
> Some trivial ones:
>
> vs-pos-supply (VS+ pin, 0 to 11V supply) and vs-neg-supply (VS- pin,
> -11 to 0V supply)
>
> pd-amp-gpios (active low) and pd-ref-gpios (active low) for optional
> runtime power management.

Ok, will have closer look to these and other pins described in the datasheet and
include them here too.

>
> Also the datasheet says the ADAQ chips supports "Single-ended to
> differential conversion". So it seems like we might need some extra
> properties to describe that case (a flag for indicating single-ended
> wiring and an optional voltage supply to describe what is connected to
> the negative input if it isn't tied to GND)

Yes, the differential ADCs also support "Single-ended to differential conversion".
Will provide support those too.

2024-04-09 15:46:39

[permalink] [raw]

Subject: Re: [PATCH v2 0/2] Add support for AD4000 series

On 04/08, David Lechner wrote:
> On Mon, Apr 8, 2024 at 9:31 AM Marcelo Schmitt
> <[email protected]> wrote:
> >
> > This is more like an RFC patch set since configuration read/write is currently
> > buggy.
> >
> > Change log v1 -> v2:
> > - Took device tree provided by David.
> > - Dropped ABI additions in favor of device tree properties.
> > - Set differential IIO channel subtype for differential ADCs.
> > - Set scan_type shift bits to mask out correct real bits from buffer.
> > - Added __aligned(8) to buffer timestamp.
> > - Used union to reduce buffer memory usage for 16-bit devices.
> > - Used SPI transfer functions rather than SPI message.
> > - Used c99 style structure initialization.
> > - Used iio_device_claim_direct_scoped().
> > - Removed unneeded pointer casts.
> > - Added other power supplies (VDD and VIO).
> >
> > Link to v1: https://lore.kernel.org/linux-iio/[email protected]/
> >
> > Additional topics:
> >
> > - Why there is no different handling for the different SPI wiring modes?
> > It looks like there is no need for different handling of "4-wire" and "3-wire"
> > modes.
> > If in "4-wire" (dt default mode), SDI is connected to SPI controller CS and
> > CNV is active high. We can activate the CNV GPIO then let the SPI controller
> > bring CS (connected to SDI) down when starting the transfer.
> > If in "3-wire" (dt single mode), if we have a CNV (active low) GPIO we activate
> > it and then proceed with with the transfer. If controller CS is connected to
> > CNV it works the same way.
> > I'm thinking it's better if we can support these devices in similar way
> > other SPI ADCs are supported. Does that make sense?
>
> In the AD7944 driver, I handled the "3-wire" mode separately because
> the sample conversion is triggered on the rising edge of the CNV line.
> In "4-wire" mode, since we have a GPIO connected to CNV, we can just
> toggle the GPIO from low to high, wait for the conversion time
> (t_CONV) and then read the sample (SPI xfer) then toggle the CNV line
> low again. In 3-wire mode, the CS line is connected to the CNV pin, so
> in order to get an up-to-date sample, we need to to toggle the CS line
> from low to high to trigger a conversion (spi xfer with no data, only
> delay), then wait for the conversion time, then read the sample (2nd
> spi xfer). So in "4-wire" mode, the CS line is getting toggled once
> per sample, but in "3-wire" mode, it is getting toggled twice per
> sample. I didn't add support for "3-wire" mode where CNV is connected
> to GPIO because we can't get max sample rate that way and it is
> unusual to not have CS connected to something. But if we do that here,
> the timing has to be different from 4-wire mode in order to not get
> stale data.

Yes, that's also the case for ad4000 series. The rising edge of CNV triggers
the conversion which causes a latency/delay of one read if CNV is connected
to a chip select.
I thought it was okay to just accept the latency since it's intrinsic to the
ADC start conversion on CNV rising edge and latency would be less noticeable
when doing continuous sampling.
Although, this indeed causes the timestamps to be disarranged, which is making me
rethink the implementation.
Yet, if we choose to have extra pulse of CNV for the sake of getting correct
timesamps, that might also impact performance for high sample rates (unless
split sample handling into different cases for single-shot and buffered readings).
I'll give it try and do it like ad7944.

>
> > To me, the "3-wire" mode with controller CS to ADC CNV is what most resembles
> > conventional SPI. The only important distinction is that the
> > controller must be able to keep ADC SDI line high during conversions.
> > Although, while the spi-engine implementation provided to me can keep SDI up
> > during conversions, I'm not sure its a thing all SPI controllers can do.
> > I tried a raspberry pi 4 some time ago and it was leaving the SDI line low if
> > no tx buffer was provided. Even with a tx full of 1s the controller would
> > bring SDI down between each 8 bits of transfer.
>
> This is a good point. It sounds like additional bindings are needed to
> describe the various wiring cases of the SDI line.
>
> It sounds like possibilities are:
>
> 1. SDI is hard-wired high -> can't write to registers, CNV is
> connected to SPI controller CS, chip is in "3-wire" mode. Currently
> adi,spi-mode="single"
> 2. SDI is connected to SDO of another chip, SDI of last chip is
> hard-wired low -> can't write to registers, CNV is connected to SPI
> controller CS, chips are in daisy chain mode. Currently
> adi,spi-mode="chain"
> 3. SDI is connected to SPI controller CS -> can't write registers,
> chip can operate in 4-wire mode with CNV connected to GPIO, Currently
> adi,spi-mode omitted.
> 4. SDI is connected to SPI controller SDO -> can write registers, and
> support all writing modes (3-wire, 4-wire, daisy chain) as long as SPI
> controller SDO line can be kept high or low at the appropriate time.
> Currently not handled.
> 5. There could be a pin mux that switches between the one of the first
> three and the 4th option (needed to avoid the issue with SPI
> controller not being able to place the SDI pin in the correct state
> during conversion trigger as described above).
>
> On AD7944, the proposed adi,spi-mode property was sufficient to
> describe what was wired to the SDI pin because we only had the first 3
> options (the AD7944 doesn't have SPI registers to write to).
>
> Also see related comments in my reply to the DT bindings patch.
>
> (From the complete bindings point of view, we should probably also
> consider the possibility of variations of 1. and 2. where CS of the
> SPI controller is not wired and CNV is connected to a GPIO - this can
> be determined by the combination of the adi,spi-mode property and the
> presence or absence of the cnv-gpios property.)

This sounds reasonable to me. I also think the comments on DT patch are good.
Will comment there too.

>
> > Anyway, single-shot and buffered reads work with the spi-engine controller
> > with ADC in "3-wire"/single mode with controller CS line connected to ADC CNV
> > pin which is how I've been testing it.
>
> Technically, yes data can be captured in "3-wire" mode with a single
> CS toggle, but then the data is stale and doesn't correspond to the
> soft timestamp because it is reading the data from the previous
> conversion triggered by the last SPI xfer, whenever that was. Since it
> is trivial to avoid this by adding the extra CS/CNV toggle I describe
> above, I don't see any reason not to.

Okay, will make it do the extra pulse to CNV for "3-wire" mode.

>
> But the way the driver is written now, it is actually only supporting
> the unnamed wiring option 4 from above, so now I understand the
> confusion about 3-wire vs. 4-wire mode in that context.
>
> >
> > - Why did not make vref regulator optional?
> > Other SAR ADCs I've seen needed a voltage reference otherwise they simply
> > could not provide any reasonable readings. Isn't it preferable to fail rather
> > than having a device that can't provide reliable data?
>
> In the device tree bindings, making vref-supply required makes sense
> since there is no internal reference. In the driver, as discussed in
> V1, it will fail if vref-supply in regulator_get_voltage() if
> vref-supply is missing and we use devm_regulator_get() instead of
> devm_regulator_get_optional(). So leaving it as-is is fine. We have a
> plan to clean this up later anyway.
>

Not sure I understand the idea here. Should the driver use
devm_regulator_get_optional() instead of devm_regulator_get() because
the optional call would fail immediately if no vref-supply while the regular
call would only fail at regulator_get_voltage()? Why? This looks very counter
intuitive to me.

> >
> > - Why did not split into AD and ADAQ patches?
> > The main difference between AD and ADAQ is the amplifier in front of the ADC.
> > If only supporting AD, we could probably avoid the scale table since it would
> > only have two possible values per ADC. But then the handling of span compression
> > scale would need refactoring to be in the scale table when adding ADAQ.
> > I'm not excited to implement something knowing it will need rework in the
> > following patch. Will do if required.
>
> If it isn't that much work, it seems worth it to me. If the driver
> work is too much, maybe just split the DT patch?
>
> >
> > - Span compression and offset.
> > For non-differential ADCs, enabling the span compression requires an input offset.
> > Link: https://www.analog.com/media/en/technical-documentation/data-sheets/AD4000-4004-4008.pdf
> > page 18
> > and
> > Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> > page 19
> > I updated the _offset attribute for those ADCs according to span compression
> > being enabled or not. Is it okay to have an attribute update cause an update to
> > another one?
> > Maybe also make the span compression a dt property and have it fixed after probe?
>
> This doesn't sound like something that belongs in DT since it doesn't
> depend on the physical properties of what is wired to the input.
>
> But the fact that offset should not be read until after scale is set
> sounds like a quirk that would be worth documenting in some
> chip-specific docs.
>
> >
> > - Configuration register
> > Despite it doing single-shot and buffered captures, read and writes to the
> > configuration register are currently buggy. It is as if the register was
> > "floating". I tried setting up buffers like ad7768-1, adxl355_core, bma220_spi,
> > bma400_core, and mcp3911.
>
> If the ADC CNV pin is connected to a GPIO and the ADC SDI pin is
> connected to SDO of the SPI controller, then nothing is connected to
> CS of the SPI controller, so that might be the problem.

ADC CNV is connected to controller CS and ADC SDI to controller SDO.
Think it's something to do with buffer alignment.
Will try the changes suggested in reply to driver patch.

>
> >
> >
> > Thanks,
> > Marcelo
> >
> > Marcelo Schmitt (2):
> > dt-bindings: iio: adc: Add AD4000
> > iio: adc: Add support for AD4000
> >
> > .../bindings/iio/adc/adi,ad4000.yaml | 201 ++++++
> > MAINTAINERS | 8 +
> > drivers/iio/adc/Kconfig | 12 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ad4000.c | 649 ++++++++++++++++++
> > 5 files changed, 871 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > create mode 100644 drivers/iio/adc/ad4000.c
> >
> > --
> > 2.43.0
> >
> >

2024-04-09 16:09:24

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] iio: adc: Add support for AD4000

On 04/08, David Lechner wrote:
> On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
> <[email protected]> wrote:
> >
> > Add support for AD4000 family of low noise, low power, high speed,
> > successive aproximation register (SAR) ADCs.
> >
> > Signed-off-by: Marcelo Schmitt <[email protected]>
> > ---
> > MAINTAINERS | 1 +
> > drivers/iio/adc/Kconfig | 12 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ad4000.c | 649 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 663 insertions(+)
> > create mode 100644 drivers/iio/adc/ad4000.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5dfe118a5dd3..86aa96115f5a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1165,6 +1165,7 @@ L: [email protected]
> > S: Supported
> > W: https://ez.analog.com/linux-software-drivers
> > F: Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > +F: drivers/iio/adc/ad4000.c
> >
> > ANALOG DEVICES INC AD4130 DRIVER
> > M: Cosmin Tanislav <[email protected]>
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 8db68b80b391..9c9d13d4b74f 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -21,6 +21,18 @@ config AD_SIGMA_DELTA
> > select IIO_BUFFER
> > select IIO_TRIGGERED_BUFFER
> >
> > +config AD4000
> > + tristate "Analog Devices AD4000 ADC Driver"
> > + depends on SPI
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > + help
> > + Say yes here to build support for Analog Devices AD4000 high speed
> > + SPI analog to digital converters (ADC).
> > +
> > + To compile this driver as a module, choose M here: the module will be
> > + called ad4000.
> > +
> > config AD4130
> > tristate "Analog Device AD4130 ADC Driver"
> > depends on SPI
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index edb32ce2af02..aa52068d864b 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -6,6 +6,7 @@
> > # When adding new entries keep the list in alphabetical order
> > obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
> > obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> > +obj-$(CONFIG_AD4000) += ad4000.o
> > obj-$(CONFIG_AD4130) += ad4130.o
> > obj-$(CONFIG_AD7091R) += ad7091r-base.o
> > obj-$(CONFIG_AD7091R5) += ad7091r5.o
> > diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
> > new file mode 100644
> > index 000000000000..7997d9d98743
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4000.c
> > @@ -0,0 +1,649 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * AD4000 SPI ADC driver
> > + *
> > + * Copyright 2024 Analog Devices Inc.
> > + */
> > +#include <asm/unaligned.h>
> > +#include <linux/bits.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/kernel.h>
> > +#include <linux/math.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/units.h>
> > +#include <linux/util_macros.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +
> > +#define AD400X_READ_COMMAND 0x54
> > +#define AD400X_WRITE_COMMAND 0x14
> > +
> > +/* AD4000 Configuration Register programmable bits */
> > +#define AD4000_STATUS BIT(4) /* Status bits output */
> > +#define AD4000_SPAN_COMP BIT(3) /* Input span compression */
> > +#define AD4000_HIGHZ BIT(2) /* High impedance mode */
> > +#define AD4000_TURBO BIT(1) /* Turbo mode */
>
> Usually bits of the same register share a similar prefix, e.g.
> AD4000_CFG_TURBO, AD4000_CFG_HIGHZ, etc.

This only has one register, but if this makes things look better will do it.

>
> > +
> > +#define AD4000_TQUIET2_NS 60
> > +
> > +#define AD4000_18BIT_MSK GENMASK(31, 14)
> > +#define AD4000_20BIT_MSK GENMASK(31, 12)
> > +
> > +#define AD4000_DIFF_CHANNEL(_sign, _real_bits) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .differential = 1, \
> > + .channel = 0, \
> > + .channel2 = 1, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_SCALE), \
> > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
> > + .scan_type = { \
> > + .sign = _sign, \
> > + .realbits = _real_bits, \
> > + .storagebits = _real_bits > 16 ? 32 : 16, \
> > + .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
> > + .endianness = IIO_BE, \
> > + }, \
> > + } \
> > +
> > +#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = 0, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_SCALE) | \
> > + BIT(IIO_CHAN_INFO_OFFSET), \
> > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
> > + .scan_type = { \
> > + .sign = _sign, \
> > + .realbits = _real_bits, \
> > + .storagebits = _real_bits > 16 ? 32 : 16, \
> > + .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
> > + .endianness = IIO_BE, \
> > + }, \
> > + } \
>
> It looks like all differential chips are signed and all
> pseduo-differential chips are unsigned, so I don't think we need the
> _sign parameter in these macros.

That's correct, the _sign param can be removed after the split of channel macros.
Will do it for v3.

>
> I also still have doubts about using IIO_BE and 8-bit xfers when it
> comes to adding support later to achieve max sample rate with a SPI
> offload. For example to get 2MSPS with an 18-bit chip, it will require
> an approx 33% faster SPI clock than the actual slowest clock possible
> because it will have to read 6 extra bits per sample. I didn't check
> the specs, but this may not even be physically possible without
> exceeding the datasheet max SPI clock rate. Also errors could be
> reduced if we could actually use the slowest allowable SPI clock rate.
> Furthermore, the offload hardware would have to be capable of adding
> an extra byte per sample for 18 and 20-bit chips when piping the data
> to DMA in order to get the 32-bit alignment in the buffer required by
> IIO scan_type and the natural alignment requirements of IIO buffers in
> general.

Maybe I should already implement support for reading with SPI offload
rather than doing it after this set is merged?
So we can test what happens at faster sample rates before we commit to a solution.

>
> > +
> > +enum ad4000_ids {
> > + ID_AD4000,
> > + ID_AD4001,
> > + ID_AD4002,
> > + ID_AD4003,
> > + ID_AD4004,
> > + ID_AD4005,
> > + ID_AD4006,
> > + ID_AD4007,
> > + ID_AD4008,
> > + ID_AD4010,
> > + ID_AD4011,
> > + ID_AD4020,
> > + ID_AD4021,
> > + ID_AD4022,
> > + ID_ADAQ4001,
> > + ID_ADAQ4003,
> > +};
> > +
> > +struct ad4000_chip_info {
> > + const char *dev_name;
> > + struct iio_chan_spec chan_spec;
> > +};
> > +
> > +static const struct ad4000_chip_info ad4000_chips[] = {
> > + [ID_AD4000] = {
> > + .dev_name = "ad4000",
> > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> > + },
> > + [ID_AD4001] = {
> > + .dev_name = "ad4001",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> > + },
> > + [ID_AD4002] = {
> > + .dev_name = "ad4002",
> > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> > + },
> > + [ID_AD4003] = {
> > + .dev_name = "ad4003",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> > + },
> > + [ID_AD4004] = {
> > + .dev_name = "ad4004",
> > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> > + },
> > + [ID_AD4005] = {
> > + .dev_name = "ad4005",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> > + },
> > + [ID_AD4006] = {
> > + .dev_name = "ad4006",
> > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> > + },
> > + [ID_AD4007] = {
> > + .dev_name = "ad4007",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> > + },
> > + [ID_AD4008] = {
> > + .dev_name = "ad4008",
> > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> > + },
> > + [ID_AD4010] = {
> > + .dev_name = "ad4010",
> > + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> > + },
> > + [ID_AD4011] = {
> > + .dev_name = "ad4011",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> > + },
> > + [ID_AD4020] = {
> > + .dev_name = "ad4020",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> > + },
> > + [ID_AD4021] = {
> > + .dev_name = "ad4021",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> > + },
> > + [ID_AD4022] = {
> > + .dev_name = "ad4022",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> > + },
> > + [ID_ADAQ4001] = {
> > + .dev_name = "adaq4001",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> > + },
> > + [ID_ADAQ4003] = {
> > + .dev_name = "adaq4003",
> > + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> > + },
> > +};
> > +
> > +enum ad4000_gains {
> > + AD4000_0454_GAIN = 0,
> > + AD4000_0909_GAIN = 1,
> > + AD4000_1_GAIN = 2,
>
> AD4000_1000_GAIN would be more consistent with the others.

Ack

>
> > + AD4000_1900_GAIN = 3,
> > + AD4000_GAIN_LEN
> > +};
> > +
> > +/*
> > + * Gains stored and computed as fractions to avoid introducing rounding errors.
> > + */
> > +static const int ad4000_gains_frac[AD4000_GAIN_LEN][2] = {
> > + [AD4000_0454_GAIN] = { 227, 500 },
> > + [AD4000_0909_GAIN] = { 909, 1000 },
> > + [AD4000_1_GAIN] = { 1, 1 },
> > + [AD4000_1900_GAIN] = { 19, 10 },
> > +};
>
> Why not just store the numerator in milli units and always use 1000
> for the denominator? It seems like it would simplify the code and make
> it easier to read and understand. Also, these values are coming from
> the adi,gain-milli property already, so we could avoid the enum and
> the lookup table entirely and simplify things even more.

Makes sense. Will do it.

>
> > +
> > +struct ad4000_state {
> > + struct spi_device *spi;
> > + struct gpio_desc *cnv_gpio;
> > + int vref;
> > + bool status_bits;
> > + bool span_comp;
> > + bool turbo_mode;
> > + bool high_z_mode;
> > +
> > + enum ad4000_gains pin_gain;
> > + int scale_tbl[AD4000_GAIN_LEN][2][2];
> > +
> > + /*
> > + * DMA (thus cache coherency maintenance) requires the
> > + * transfer buffers to live in their own cache lines.
> > + */
> > + struct {
> > + union {
> > + u16 sample_buf16;
> > + u32 sample_buf32;
>
> Technically, these are holding big-endian data, so __be16 and __be32
> would be more correct.

Ack

>
> > + } data;
> > + s64 timestamp __aligned(8);
> > + } scan;
> > + __be16 tx_buf __aligned(IIO_DMA_MINALIGN);
> > + __be16 rx_buf;
> > +};
>
> scan.data is used as SPI rx_buf so __aligned(IIO_DMA_MINALIGN); needs
> to be moved to the scan field.

I have already tried it. Maybe I did something wrong besides buffer alignment
at that time. Will give it another try.

>
> > +
> > +static void ad4000_fill_scale_tbl(struct ad4000_state *st, int scale_bits,
> > + const struct ad4000_chip_info *chip)
> > +{
> > + int diff = chip->chan_spec.differential;
> > + int val, val2, tmp0, tmp1, i;
> > + u64 tmp2;
> > +
> > + val2 = scale_bits;
> > + for (i = 0; i < AD4000_GAIN_LEN; i++) {
>
> Only one gain is selected by the devicetree, so why do we need to do
> this for all 4 gains?
>

Good point. Will think better how to simplify this.

> > + val = st->vref / 1000;
> > + /* Multiply by MILLI here to avoid losing precision */
> > + val = mult_frac(val, ad4000_gains_frac[i][1] * MILLI,
> > + ad4000_gains_frac[i][0]);
> > + /* Would multiply by NANO here but we already multiplied by MILLI */
> > + tmp2 = shift_right((u64)val * MICRO, val2);
> > + tmp0 = (int)div_s64_rem(tmp2, NANO, &tmp1);
> > + /* Store scale for when span compression is disabled */
> > + st->scale_tbl[i][0][0] = tmp0; /* Integer part */
> > + st->scale_tbl[i][0][1] = abs(tmp1); /* Fractional part */
> > + /* Store scale for when span compression is enabled */
> > + st->scale_tbl[i][1][0] = tmp0;
> > + if (diff)
> > + st->scale_tbl[i][1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
> > + else
> > + st->scale_tbl[i][1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
> > + }
> > +}
> > +
> > +static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
> > +{
> > + put_unaligned_be16(AD400X_WRITE_COMMAND << BITS_PER_BYTE | val,
> > + &st->tx_buf);
> > + return spi_write(st->spi, &st->tx_buf, 2);
> > +}
> > +
> > +static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
> > +{
> > + struct spi_transfer t[] = {
> > + {
> > + .tx_buf = &st->tx_buf,
> > + .rx_buf = &st->rx_buf,
> > + .len = 2,
> > + },
> > + };
> > + int ret;
> > +
> > + put_unaligned_be16(AD400X_READ_COMMAND << BITS_PER_BYTE, &st->tx_buf);
> > + ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val = get_unaligned_be16(&st->rx_buf);
> > +
> > + return ret;
> > +}
> > +
>
> It would be very helpful to have comments here explaining the exact
> expected wiring configuration and signal timing here since there are
> so many possibilities for this chip.
>

Ok, will be spliting this into different handling for the wiring modes so
will add comments to make clear what supports each configuration.

> > +static int ad4000_read_sample(struct ad4000_state *st,
> > + const struct iio_chan_spec *chan)
> > +{
> > + struct spi_transfer t[] = {
>
> Don't really need [] here since there is only one xfer.
>
Ack

> > + {
> > + .rx_buf = &st->scan.data,
> > + .len = BITS_TO_BYTES(chan->scan_type.storagebits),
> > + .delay = {
> > + .value = AD4000_TQUIET2_NS,
> > + .unit = SPI_DELAY_UNIT_NSECS,
> > + },
> > + },
> > + };
> > + int ret;
> > +
> > + ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int ad4000_single_conversion(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan, int *val)
> > +{
> > + struct ad4000_state *st = iio_priv(indio_dev);
> > + u32 sample;
> > + int ret;
> > +
> > + if (st->cnv_gpio)
> > + gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_HIGH);
>
> It would make more sense and be less redundant to move the gpio code
> into ad4000_read_sample().
>
> Also, gpiod_set_value_cansleep() checks for NULL, so the if () is redundant.
>
Good point. I think the execution flow migth change a bit here but will try
to avoid things like that.

> > +
> > + ret = ad4000_read_sample(st, chan);
> > + if (ret)
> > + return ret;
> > +
> > + if (st->cnv_gpio)
> > + gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_LOW);
> > +
> > + if (chan->scan_type.storagebits > 16)
> > + sample = get_unaligned_be32(&st->scan.data);
> > + else
> > + sample = get_unaligned_be16(&st->scan.data);
>
> data is aligned, so be32/16_to_cpu() should be fine. Also, Jonathan
> will suggest to use &st->scan.data.sample_b32/16 here too. :-)

Ack
>
> > +
> > + switch (chan->scan_type.realbits) {
> > + case 16:
> > + break;
> > + case 18:
> > + sample = FIELD_GET(AD4000_18BIT_MSK, sample);
> > + break;
> > + case 20:
> > + sample = FIELD_GET(AD4000_20BIT_MSK, sample);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (chan->scan_type.sign == 's')
> > + *val = sign_extend32(sample, chan->scan_type.realbits - 1);
> > +
> > + return IIO_VAL_INT;
> > +}
> > +
> > +static int ad4000_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int *val,
> > + int *val2, long info)
> > +{
> > + struct ad4000_state *st = iio_priv(indio_dev);
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_RAW:
> > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> > + return ad4000_single_conversion(indio_dev, chan, val);
> > + unreachable();
> > + case IIO_CHAN_INFO_SCALE:
> > + *val = st->scale_tbl[st->pin_gain][st->span_comp][0];
> > + *val2 = st->scale_tbl[st->pin_gain][st->span_comp][1];
> > + return IIO_VAL_INT_PLUS_NANO;
> > + case IIO_CHAN_INFO_OFFSET:
> > + *val = 0;
> > + if (st->span_comp)
> > + *val = mult_frac(st->vref / 1000, 1, 10);
> > +
> > + return IIO_VAL_INT;
> > + default:
> > + break;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int ad4000_read_avail(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + const int **vals, int *type, int *length,
> > + long info)
> > +{
> > + struct ad4000_state *st = iio_priv(indio_dev);
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_SCALE:
> > + *vals = (int *)st->scale_tbl[st->pin_gain];
> > + *length = 2 * 2;
> > + *type = IIO_VAL_INT_PLUS_NANO;
> > + return IIO_AVAIL_LIST;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, long mask)
> > +{
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + return IIO_VAL_INT_PLUS_NANO;
> > + default:
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + }
> > + return -EINVAL;
>
> not reachable because of default, so can be left out
>
Ack

> > +}
> > +
> > +static int ad4000_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int val, int val2,
> > + long mask)
> > +{
> > + struct ad4000_state *st = iio_priv(indio_dev);
> > + unsigned int reg_val;
> > + bool span_comp_en;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > + ret = ad4000_read_reg(st, &reg_val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + span_comp_en = (val2 == st->scale_tbl[st->pin_gain][1][1]);
> > + reg_val &= ~AD4000_SPAN_COMP;
> > + reg_val |= FIELD_PREP(AD4000_SPAN_COMP, span_comp_en);
> > +
> > + ret = ad4000_write_reg(st, reg_val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + st->span_comp = span_comp_en;
> > + return 0;
> > + }
> > + unreachable();
>
> Can bring out the return 0 to avoid unreachable.

Ack
>
> > + default:
> > + break;
>
> Can return -EINVAL to avoid break;
>
Ack

> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static irqreturn_t ad4000_trigger_handler(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct ad4000_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + if (st->cnv_gpio)
> > + gpiod_set_value(st->cnv_gpio, GPIOD_OUT_HIGH);
> > +
> > + ret = ad4000_read_sample(st, &indio_dev->channels[0]);
> > + if (ret < 0)
> > + goto err_out;
> > +
> > + if (st->cnv_gpio)
> > + gpiod_set_value(st->cnv_gpio, GPIOD_OUT_LOW);
> > +
> > + iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
> > + iio_get_time_ns(indio_dev));
> > +
> > +err_out:
> > + iio_trigger_notify_done(indio_dev->trig);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static const struct iio_info ad4000_info = {
> > + .read_raw = &ad4000_read_raw,
> > + .read_avail = &ad4000_read_avail,
> > + .write_raw = &ad4000_write_raw,
> > + .write_raw_get_fmt = &ad4000_write_raw_get_fmt,
> > +};
> > +
> > +static void ad4000_config(struct ad4000_state *st)
> > +{
> > + unsigned int reg_val;
> > + int ret;
> > +
> > + reg_val = FIELD_PREP(AD4000_TURBO, 1);
>
> Since the driver in it's current state can get anywhere near the max
> sample rate of ~1MSPS, I don't think it makes sense to enable turbo at
> this point.
>

This is just enabling turbo at start up. If not enabling turbo during probe,
we would want(need?) to provide some interface for that, which might not be
much desired.

> > +
> > + if (device_property_present(&st->spi->dev, "adi,high-z-input"))
> > + reg_val |= FIELD_PREP(AD4000_HIGHZ, 1);
> > +
> > + /*
> > + * The ADC SDI pin might be connected to controller CS line in which
> > + * case the write might fail. This, however, does not prevent the device
> > + * from functioning even though in a configuration other than the
> > + * requested one.
> > + */
> > + ret = ad4000_write_reg(st, reg_val);
> > + if (ret < 0)
> > + dev_dbg(&st->spi->dev, "Failed to config device\n");
>
> If writing fails because there is no CS line wired up, we won't get an
> error returned here. The SPI controller has no way of knowing this
> happened, so it can only assume the write was successful and return 0.
> So this should return ret.
>
Ok, ack.

> Ideally, the devicetree should tell us if CS is wired up or not.
>
> > +}
> > +
> > +static void ad4000_regulator_disable(void *reg)
> > +{
> > + regulator_disable(reg);
> > +}
> > +
> > +static int ad4000_probe(struct spi_device *spi)
> > +{
> > + const struct ad4000_chip_info *chip;
> > + struct regulator *vref_reg;
> > + struct iio_dev *indio_dev;
> > + struct ad4000_state *st;
> > + int ret;
>
> We need a check somewhere in here to make sure that adi,spi-mode is in
> a supported configuration. E.g. chain mode is not currently
> implemented.

ok, will add that.

>
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + chip = spi_get_device_match_data(spi);
> > + if (!chip)
> > + return -EINVAL;
> > +
> > + st = iio_priv(indio_dev);
> > + st->spi = spi;
> > +
> > + ret = devm_regulator_get_enable(&spi->dev, "vdd");
> > + if (ret)
> > + return dev_err_probe(&spi->dev, ret, "Failed to enable VDD supply\n");
> > +
> > + ret = devm_regulator_get_enable(&spi->dev, "vio");
> > + if (ret)
> > + return dev_err_probe(&spi->dev, ret, "Failed to enable VIO supply\n");
> > +
> > + vref_reg = devm_regulator_get(&spi->dev, "ref");
> > + if (IS_ERR(vref_reg))
> > + return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
> > + "Failed to get vref regulator\n");
> > +
> > + ret = regulator_enable(vref_reg);
> > + if (ret < 0)
> > + return dev_err_probe(&spi->dev, ret,
> > + "Failed to enable voltage regulator\n");
> > +
> > + ret = devm_add_action_or_reset(&spi->dev, ad4000_regulator_disable, vref_reg);
> > + if (ret)
> > + return dev_err_probe(&spi->dev, ret,
> > + "Failed to add regulator disable action\n");
> > +
> > + st->vref = regulator_get_voltage(vref_reg);
> > + if (st->vref < 0)
> > + return dev_err_probe(&spi->dev, st->vref, "Failed to get vref\n");
> > +
> > + st->cnv_gpio = devm_gpiod_get_optional(&spi->dev, "cnv", GPIOD_OUT_HIGH);
> > + if (IS_ERR(st->cnv_gpio)) {
> > + if (PTR_ERR(st->cnv_gpio) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
>
> EPROBE_DEFER check is not needed with dev_err_probe();, it already does that.

Ack
>
>
> > +
> > + return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio),
> > + "Failed to get CNV GPIO");
> > + }
> > +
> > + ad4000_config(st);
> > +
> > + indio_dev->name = chip->dev_name;
> > + indio_dev->info = &ad4000_info;
> > + indio_dev->channels = &chip->chan_spec;
> > + indio_dev->num_channels = 1;
> > +
> > + st->pin_gain = AD4000_1_GAIN;
> > + if (device_property_present(&spi->dev, "adi,gain-milli")) {
> > + u32 val;
>
> Should it be an error if adi,gain-milli is set on non-adaq chips?

Maybe. We should not change the scale if it's a chip that don't have the
amplifier in front of the ADC. I think the best handling would be to just
ignore adi,gain-milli if it's not an ADAQ device. Maybe better add a DT
constraint,
- if:
properties:
compatible:
contains:
enum:
- adi,adaq4001
- adi,adaq4003
then:
properties:
adi,gain-milli: false
?

>
> > +
> > + ret = device_property_read_u32(&spi->dev, "adi,gain-milli", &val);
> > + if (ret)
> > + return ret;
> > +
> > + switch (val) {
> > + case 454:
> > + st->pin_gain = AD4000_0454_GAIN;
> > + break;
> > + case 909:
> > + st->pin_gain = AD4000_0909_GAIN;
> > + break;
> > + case 1000:
> > + st->pin_gain = AD4000_1_GAIN;
> > + break;
> > + case 1900:
> > + st->pin_gain = AD4000_1900_GAIN;
> > + break;
> > + default:
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "Invalid firmware provided gain\n");
>
> Could help debugging if val is included in the error message.

Ack
>
>
> > + }
> > + }
> > +
> > + /*
> > + * ADCs that output twos complement code have one less bit to express
> > + * voltage magnitude.
> > + */
> > + if (chip->chan_spec.scan_type.sign == 's')
> > + ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits - 1,
> > + chip);
> > + else
> > + ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits,
> > + chip);
> > +
> > + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> > + &iio_pollfunc_store_time,
> > + &ad4000_trigger_handler, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + return devm_iio_device_register(&spi->dev, indio_dev);
> > +}
> > +
> > +static const struct spi_device_id ad4000_id[] = {
> > + { "ad4000", (kernel_ulong_t)&ad4000_chips[ID_AD4000] },
> > + { "ad4001", (kernel_ulong_t)&ad4000_chips[ID_AD4001] },
> > + { "ad4002", (kernel_ulong_t)&ad4000_chips[ID_AD4002] },
> > + { "ad4003", (kernel_ulong_t)&ad4000_chips[ID_AD4003] },
> > + { "ad4004", (kernel_ulong_t)&ad4000_chips[ID_AD4004] },
> > + { "ad4005", (kernel_ulong_t)&ad4000_chips[ID_AD4005] },
> > + { "ad4006", (kernel_ulong_t)&ad4000_chips[ID_AD4006] },
> > + { "ad4007", (kernel_ulong_t)&ad4000_chips[ID_AD4007] },
> > + { "ad4008", (kernel_ulong_t)&ad4000_chips[ID_AD4008] },
> > + { "ad4010", (kernel_ulong_t)&ad4000_chips[ID_AD4010] },
> > + { "ad4011", (kernel_ulong_t)&ad4000_chips[ID_AD4011] },
> > + { "ad4020", (kernel_ulong_t)&ad4000_chips[ID_AD4020] },
> > + { "ad4021", (kernel_ulong_t)&ad4000_chips[ID_AD4021] },
> > + { "ad4022", (kernel_ulong_t)&ad4000_chips[ID_AD4022] },
> > + { "adaq4001", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4001] },
> > + { "adaq4003", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4003] },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(spi, ad4000_id);
> > +
> > +static const struct of_device_id ad4000_of_match[] = {
> > + { .compatible = "adi,ad4000", .data = &ad4000_chips[ID_AD4000] },
> > + { .compatible = "adi,ad4001", .data = &ad4000_chips[ID_AD4001] },
> > + { .compatible = "adi,ad4002", .data = &ad4000_chips[ID_AD4002] },
> > + { .compatible = "adi,ad4003", .data = &ad4000_chips[ID_AD4003] },
> > + { .compatible = "adi,ad4004", .data = &ad4000_chips[ID_AD4004] },
> > + { .compatible = "adi,ad4005", .data = &ad4000_chips[ID_AD4005] },
> > + { .compatible = "adi,ad4006", .data = &ad4000_chips[ID_AD4006] },
> > + { .compatible = "adi,ad4007", .data = &ad4000_chips[ID_AD4007] },
> > + { .compatible = "adi,ad4008", .data = &ad4000_chips[ID_AD4008] },
> > + { .compatible = "adi,ad4010", .data = &ad4000_chips[ID_AD4010] },
> > + { .compatible = "adi,ad4011", .data = &ad4000_chips[ID_AD4011] },
> > + { .compatible = "adi,ad4020", .data = &ad4000_chips[ID_AD4020] },
> > + { .compatible = "adi,ad4021", .data = &ad4000_chips[ID_AD4021] },
> > + { .compatible = "adi,ad4022", .data = &ad4000_chips[ID_AD4022] },
> > + { .compatible = "adi,adaq4001", .data = &ad4000_chips[ID_ADAQ4001] },
> > + { .compatible = "adi,adaq4003", .data = &ad4000_chips[ID_ADAQ4003] },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ad4000_of_match);
> > +
> > +static struct spi_driver ad4000_driver = {
> > + .driver = {
> > + .name = "ad4000",
> > + .of_match_table = ad4000_of_match,
> > + },
> > + .probe = ad4000_probe,
> > + .id_table = ad4000_id,
> > +};
> > +module_spi_driver(ad4000_driver);
> > +
> > +MODULE_AUTHOR("Mircea Caprioru <[email protected]>");
> > +MODULE_AUTHOR("Marcelo Schmitt <[email protected]>");
> > +MODULE_DESCRIPTION("Analog Devices AD4000 ADC driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.43.0
> >
> >

2024-04-09 16:31:43

[permalink] [raw]

Subject: Re: [PATCH v2 0/2] Add support for AD4000 series

On Tue, Apr 9, 2024 at 9:59 AM Marcelo Schmitt
<[email protected]> wrote:
>
> On 04/08, David Lechner wrote:
> > On Mon, Apr 8, 2024 at 9:31 AM Marcelo Schmitt
> > <[email protected]> wrote:
> > >

..

> > >
> > > - Why did not make vref regulator optional?
> > > Other SAR ADCs I've seen needed a voltage reference otherwise they simply
> > > could not provide any reasonable readings. Isn't it preferable to fail rather
> > > than having a device that can't provide reliable data?
> >
> > In the device tree bindings, making vref-supply required makes sense
> > since there is no internal reference. In the driver, as discussed in
> > V1, it will fail if vref-supply in regulator_get_voltage() if
> > vref-supply is missing and we use devm_regulator_get() instead of
> > devm_regulator_get_optional(). So leaving it as-is is fine. We have a
> > plan to clean this up later anyway.
> >
>
> Not sure I understand the idea here. Should the driver use
> devm_regulator_get_optional() instead of devm_regulator_get() because
> the optional call would fail immediately if no vref-supply while the regular
> call would only fail at regulator_get_voltage()? Why? This looks very counter
> intuitive to me.

Right. I'm saying just leave it the way it is for now.

(I have a plan to simplify it later, but still working on that.)

2024-04-09 16:44:56

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] iio: adc: Add support for AD4000

On Tue, Apr 9, 2024 at 11:09 AM Marcelo Schmitt
<[email protected]> wrote:
>
> On 04/08, David Lechner wrote:
> > On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
> > <[email protected]> wrote:
> > >

..

> >
> > I also still have doubts about using IIO_BE and 8-bit xfers when it
> > comes to adding support later to achieve max sample rate with a SPI
> > offload. For example to get 2MSPS with an 18-bit chip, it will require
> > an approx 33% faster SPI clock than the actual slowest clock possible
> > because it will have to read 6 extra bits per sample. I didn't check
> > the specs, but this may not even be physically possible without
> > exceeding the datasheet max SPI clock rate. Also errors could be
> > reduced if we could actually use the slowest allowable SPI clock rate.
> > Furthermore, the offload hardware would have to be capable of adding
> > an extra byte per sample for 18 and 20-bit chips when piping the data
> > to DMA in order to get the 32-bit alignment in the buffer required by
> > IIO scan_type and the natural alignment requirements of IIO buffers in
> > general.
>
> Maybe I should already implement support for reading with SPI offload
> rather than doing it after this set is merged?
> So we can test what happens at faster sample rates before we commit to a solution.
>

Yes, that sounds like a wise thing to do.

>
> >
> > > + } data;
> > > + s64 timestamp __aligned(8);
> > > + } scan;
> > > + __be16 tx_buf __aligned(IIO_DMA_MINALIGN);
> > > + __be16 rx_buf;
> > > +};
> >
> > scan.data is used as SPI rx_buf so __aligned(IIO_DMA_MINALIGN); needs
> > to be moved to the scan field.
>
> I have already tried it. Maybe I did something wrong besides buffer alignment
> at that time. Will give it another try.

This is the alignment for DMA cache coherency. So it should not have
any affect on the actual data read, only performance.

> > > +static void ad4000_config(struct ad4000_state *st)
> > > +{
> > > + unsigned int reg_val;
> > > + int ret;
> > > +
> > > + reg_val = FIELD_PREP(AD4000_TURBO, 1);
> >
> > Since the driver in it's current state can get anywhere near the max
> > sample rate of ~1MSPS, I don't think it makes sense to enable turbo at
> > this point.
> >
>
> This is just enabling turbo at start up. If not enabling turbo during probe,
> we would want(need?) to provide some interface for that, which might not be
> much desired.
>

TURBO is only needed to achieve the max sample rate of 500k/1M/2MSPS
on the various chips by skipping powering down some circuitry between
samples. We can't get anywhere close to that in Linux without some
sort of SPI offloading. So, for now, we might as well leave it
disabled and save some power.

> > > +
> > > + st->pin_gain = AD4000_1_GAIN;
> > > + if (device_property_present(&spi->dev, "adi,gain-milli")) {
> > > + u32 val;
> >
> > Should it be an error if adi,gain-milli is set on non-adaq chips?
>
> Maybe. We should not change the scale if it's a chip that don't have the
> amplifier in front of the ADC. I think the best handling would be to just
> ignore adi,gain-milli if it's not an ADAQ device. Maybe better add a DT
> constraint,
> - if:
> properties:
> compatible:
> contains:
> enum:
> - adi,adaq4001
> - adi,adaq4003
> then:
> properties:
> adi,gain-milli: false
> ?

I think this is missing a not:, but otherwise yes this should be in
the DT bindings.

Even with that though, I would still be helpful to readers of the
driver to at least have a comment here pointing out that this property
and related gain scaling only applies to ADAQ chips.

2024-04-09 18:11:47

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] iio: adc: Add support for AD4000

On 04/09, David Lechner wrote:
> On Tue, Apr 9, 2024 at 11:09 AM Marcelo Schmitt
> <[email protected]> wrote:
> >
> > On 04/08, David Lechner wrote:
> > > On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
> > > <[email protected]> wrote:
> > > >
>
..
>
>
> > > > +static void ad4000_config(struct ad4000_state *st)
> > > > +{
> > > > + unsigned int reg_val;
> > > > + int ret;
> > > > +
> > > > + reg_val = FIELD_PREP(AD4000_TURBO, 1);
> > >
> > > Since the driver in it's current state can get anywhere near the max
> > > sample rate of ~1MSPS, I don't think it makes sense to enable turbo at
> > > this point.
> > >
> >
> > This is just enabling turbo at start up. If not enabling turbo during probe,
> > we would want(need?) to provide some interface for that, which might not be
> > much desired.
> >
>
> TURBO is only needed to achieve the max sample rate of 500k/1M/2MSPS
> on the various chips by skipping powering down some circuitry between
> samples. We can't get anywhere close to that in Linux without some
> sort of SPI offloading. So, for now, we might as well leave it
> disabled and save some power.
>

Humm, ad4000 datasheets don't really mention power usage differences for turbo
mode like ad7944 datasheet do. Though, they should be similar.
Yeah, will leave turbo disabled until providing offload support.

>
> > > > +
> > > > + st->pin_gain = AD4000_1_GAIN;
> > > > + if (device_property_present(&spi->dev, "adi,gain-milli")) {
> > > > + u32 val;
> > >
> > > Should it be an error if adi,gain-milli is set on non-adaq chips?
> >
> > Maybe. We should not change the scale if it's a chip that don't have the
> > amplifier in front of the ADC. I think the best handling would be to just
> > ignore adi,gain-milli if it's not an ADAQ device. Maybe better add a DT
> > constraint,
> > - if:
> > properties:
> > compatible:
> > contains:
> > enum:
> > - adi,adaq4001
> > - adi,adaq4003
> > then:
> > properties:
> > adi,gain-milli: false
> > ?
>
> I think this is missing a not:, but otherwise yes this should be in
> the DT bindings.

Oops, yeap, was missing the not:.

>
> Even with that though, I would still be helpful to readers of the
> driver to at least have a comment here pointing out that this property
> and related gain scaling only applies to ADAQ chips.

ok, will add a comment.

2024-04-13 16:07:48

[permalink] [raw]

Subject: Re: [PATCH v2 0/2] Add support for AD4000 series

> >
> > - Span compression and offset.
> > For non-differential ADCs, enabling the span compression requires an input offset.
> > Link: https://www.analog.com/media/en/technical-documentation/data-sheets/AD4000-4004-4008.pdf
> > page 18
> > and
> > Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> > page 19
> > I updated the _offset attribute for those ADCs according to span compression
> > being enabled or not. Is it okay to have an attribute update cause an update to
> > another one?

Yes. Happens all the time and it's not possible in general to define an ABI where
this doesn't happen on some combinations. scale in particular can be affected in lots
of fun non linear ways by other attributes.

How you handle it is a bit up to the driver writer and interpretation of what is
most natural for a given combination.
Sometimes:
a) Stash previously set value of the other attributes and try to find a new value
as near as possible give constraint of whatever else was just written
b) Just use whatever the register value now corresponds to given the other attribute
changed

Either way userspace needs to take another look at other attributes to see if they
changed after changing anything that might have an influence. It can then update
them however it likes.

> > Maybe also make the span compression a dt property and have it fixed after probe?
>
> This doesn't sound like something that belongs in DT since it doesn't
> depend on the physical properties of what is wired to the input.

>
> But the fact that offset should not be read until after scale is set
> sounds like a quirk that would be worth documenting in some
> chip-specific docs.

Agreed - it is useful to hint at this but in general software has to check
everything as there are lots of these sorts of interactions :(

Jonathan

2024-04-13 16:14:42

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000

On Tue, 9 Apr 2024 12:30:09 -0300
Marcelo Schmitt <[email protected]> wrote:

> On 04/08, David Lechner wrote:
> > On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
> > <[email protected]> wrote:
> > >
> > > Add device tree documentation for AD4000 family of ADC devices.
> > >
> > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> > >
> >
> > Suggested-by: David Lechner <[email protected]>
> >
> > (if you still use mostly my suggestions in the end)
>
> Yes, it's been of great help. Will include the tag in future ad4000 DT patches.
>
> >
> > > Signed-off-by: Marcelo Schmitt <[email protected]>
> > > ---
> > > .../bindings/iio/adc/adi,ad4000.yaml | 201 ++++++++++++++++++
> > > MAINTAINERS | 7 +
> > > 2 files changed, 208 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > > new file mode 100644
> > > index 000000000000..ca06afb5149e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > > @@ -0,0 +1,201 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Analog Devices AD4000 and similar Analog to Digital Converters
> > > +
> > > +maintainers:
> > > + - Marcelo Schmitt <[email protected]>
> > > +
> > > +description: |
> > > + Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
> > > + Specifications can be found at:
> > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> > > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> > > + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> > > + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> > > +
> > > +$ref: /schemas/spi/spi-peripheral-props.yaml#
> > > +
> > > +properties:
> > > + compatible:
> > > + enum:
> > > + - adi,ad4000
> > > + - adi,ad4001
> > > + - adi,ad4002
> > > + - adi,ad4003
> > > + - adi,ad4004
> > > + - adi,ad4005
> > > + - adi,ad4006
> > > + - adi,ad4007
> > > + - adi,ad4008
> > > + - adi,ad4010
> > > + - adi,ad4011
> > > + - adi,ad4020
> > > + - adi,ad4021
> > > + - adi,ad4022
> > > + - adi,adaq4001
> > > + - adi,adaq4003
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > + spi-max-frequency:
> > > + maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
> > > +
> > > + spi-cpha: true
> > > +
> > > + adi,spi-mode:
> > > + $ref: /schemas/types.yaml#/definitions/string
> > > + enum: [ single, chain ]
> >
> > It sounds like there are more possible wiring configurations for these
> > chips that I thought when suggesting reusing this binding from AD7944
> > so we probably need more options here. (see my reply to the cover
> > letter for the complete context of these remarks)
> >
> > We identified A) an additional wiring configuration where SDI of the
> > ADC chip is wired to SDO of the SPI controller and B) a potential need
> > to pin mux between wiring modes to work around SPI controller
> > limitations perhaps we could omit the adi,spi-mode property and just
> > use the standard pinctrl properties.
> >
> > pinctrl-names:

I'm lost on how pinctrl makes sense here.
Yes you are changing the modes of the pins, but not in a conventional sense
of some register that is being updated to say now use them like this.
The mode is dependent on the timing sequence of how the pins are used.
Otherwise looking at it a different way it's an external wiring thing we
aren't controlling it at all. Is pinctrl suitable for that?
I always thought of it as a way to change configurations of SoC pins.

A pointer to some precendence in another driver for using it like this
would go some way towards convincing me.

Jonathan

> > description: |
> > Names for possible ways the SDI line of the controller is wired.
> >
> > * default: The SDI line of the ADC is connected to the SDO line of the
> > SPI controller. CNV line of the ADC is connected to CS of the SPI
> > controller.
> Not sure if should be DT, but maybe also point out that in default mode the
> SPI controller must be capable of keeping ADC SDI (controller SDO) line high
> during ADC conversions.
>
> > * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
> > definition of 3-wire mode is NOT at all related to the standard
> > spi-3wire property!) In this mode, SDI is tied to VIO, and the CNV line
> > can be connected to the CS line of the SPI controller (typical) or to a
> > GPIO, in which case the CS line of the controller is unused. The SDO
> > line of the SPI controller is not connected.
> > * multi: The datasheet calls this "4-wire mode" and is used when multiple
> > chips are connected in parallel. In this mode, the ADC SDI line is tied
> > to the CS line on the SPI controller and the CNV line is connected to
> > a GPIO. The SDO line of the SPI controller is not connected.
> > * chain: The datasheet calls this "chain mode". This mode is used to save
> > on wiring when multiple ADCs are used. In this mode, the SDI line of
> > one chip is tied to the SDO of the next chip in the chain and the SDI of
> > the last chip in the chain is tied to GND. Only the first chip in the
> > chain is connected to the SPI bus. The CNV line of all chips are tied
> > together. The CS line of the SPI controller can be used as the CNV line
> > only if it is active high.
> >
> > If one name is specified, it is assumed the chip is hard-wired in this
> > configuration.
> >
> > If two names are specified, it is assumed that a pinmux can switch between
> > the two wiring configurations. The first is the default mode for reading
> > and writing registers on the chip and the second is the mode for reading
> > the conversion data from the chip.
> > oneOf:
> > - items:
> > - enum:
> > - default
> > - single
> > - multi
> > - chain
> > - items:
> > - const: default
> > - enum:
> > - single
> > - multi
> > - chain
> >
> > pinctrl-0:
> > maxItems: 1
> >
> > pinctrl-1:
> > maxItems: 1

2024-04-13 16:20:26

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] iio: adc: Add support for AD4000

>
> > +
> > + ret = ad4000_read_sample(st, chan);
> > + if (ret)
> > + return ret;
> > +
> > + if (st->cnv_gpio)
> > + gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_LOW);
> > +
> > + if (chan->scan_type.storagebits > 16)
> > + sample = get_unaligned_be32(&st->scan.data);
> > + else
> > + sample = get_unaligned_be16(&st->scan.data);
>
> data is aligned, so be32/16_to_cpu() should be fine. Also, Jonathan
> will suggest to use &st->scan.data.sample_b32/16 here too. :-)

Sparse may well complain as well and no one likes build warnings ;)

2024-04-13 16:34:44

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] iio: adc: Add support for AD4000

On Tue, 9 Apr 2024 11:44:26 -0500
David Lechner <[email protected]> wrote:

> On Tue, Apr 9, 2024 at 11:09 AM Marcelo Schmitt
> <[email protected]> wrote:
> >
> > On 04/08, David Lechner wrote:
> > > On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
> > > <[email protected]> wrote:
> > > >
>
> ...
>
> > >
> > > I also still have doubts about using IIO_BE and 8-bit xfers when it
> > > comes to adding support later to achieve max sample rate with a SPI
> > > offload. For example to get 2MSPS with an 18-bit chip, it will require
> > > an approx 33% faster SPI clock than the actual slowest clock possible
> > > because it will have to read 6 extra bits per sample. I didn't check
> > > the specs, but this may not even be physically possible without
> > > exceeding the datasheet max SPI clock rate. Also errors could be
> > > reduced if we could actually use the slowest allowable SPI clock rate.
> > > Furthermore, the offload hardware would have to be capable of adding
> > > an extra byte per sample for 18 and 20-bit chips when piping the data
> > > to DMA in order to get the 32-bit alignment in the buffer required by
> > > IIO scan_type and the natural alignment requirements of IIO buffers in
> > > general.
> >
> > Maybe I should already implement support for reading with SPI offload
> > rather than doing it after this set is merged?
> > So we can test what happens at faster sample rates before we commit to a solution.
> >
>
> Yes, that sounds like a wise thing to do.
>
> >
> > >
> > > > + } data;
> > > > + s64 timestamp __aligned(8);
> > > > + } scan;
> > > > + __be16 tx_buf __aligned(IIO_DMA_MINALIGN);
> > > > + __be16 rx_buf;
> > > > +};
> > >
> > > scan.data is used as SPI rx_buf so __aligned(IIO_DMA_MINALIGN); needs
> > > to be moved to the scan field.
> >
> > I have already tried it. Maybe I did something wrong besides buffer alignment
> > at that time. Will give it another try.
>
> This is the alignment for DMA cache coherency. So it should not have
> any affect on the actual data read, only performance.

Nope. It's a correctness issue not a performance one (though you may get
advantages there as well) You can get potential corruption of
other fields that end up in the same cacheline - so the aim is to make
sure that nothing that we might use concurrently is in that cacheline.

There was a good description of what is going on here in a talk Wolfram
gave a few years back when he was exploring how to avoid bounce buffers
in I2C https://www.youtube.com/watch?v=JDwaMClvV-s that includes links to descriptions
of the fun that can happen. The short description is that a DMA controller is
allowed to grab the whole of a cacheline (typically 64 bytes, can be bigger)
in coherently from the host (basically takes a copy). It can then merrily
do it's operations before finally copying it back to the actual memory.
The problem lies in that there may be other data in that cacheline that
is accessed at whilst the DMA controller was working on it's own prviate
copy. Those changes will be wiped out.

Now you probably didn't see it because:
a) Many controllers don't do this - either they don't cache stale data, or
are sufficiently coherent with CPU caches etc that any changes in this
'near by' data are correctly handled.

b) It's really hard to hit the races anyway. I've only seen it once when
debugging real systems, but people run into this occasionally on IIO
drivers and it is really nasty to debug.

c) On arm64 at least in many cases the DMA core will bounce buffer anyway
after some changes made a couple of years back. Unfortunately that isn't
true on all architectures yet (I think anyway) so we still need to be
careful around this.

Note some architectures (x86 I think) never allowed this cacheline corruption
path in the first place so the DMA_MINALIGN value is 8 bytes (IIRC).

Coherency is hard (but fun if you have time, particularly the places where
it is allowed to break and how they are avoided :)

Jonathan

2024-04-13 16:40:03

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] iio: adc: Add support for AD4000

On Mon, 8 Apr 2024 11:31:42 -0300
Marcelo Schmitt <[email protected]> wrote:

> Add support for AD4000 family of low noise, low power, high speed,
> successive aproximation register (SAR) ADCs.
>
> Signed-off-by: Marcelo Schmitt <[email protected]>
A few trivial comments inline to add to David's much more thorough review.

> +#include <asm/unaligned.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +#include <linux/units.h>
> +#include <linux/util_macros.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

You don't have custom attributes, so doubt you need that include.

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

> +
> +static int ad4000_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long info)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> + return ad4000_single_conversion(indio_dev, chan, val);
> + unreachable();
> + case IIO_CHAN_INFO_SCALE:
> + *val = st->scale_tbl[st->pin_gain][st->span_comp][0];
> + *val2 = st->scale_tbl[st->pin_gain][st->span_comp][1];
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_OFFSET:
> + *val = 0;
> + if (st->span_comp)
> + *val = mult_frac(st->vref / 1000, 1, 10);
> +
> + return IIO_VAL_INT;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
As below - I'd push into the default: and drop down here.

> +}
> +
> +static int ad4000_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long info)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SCALE:
> + *vals = (int *)st->scale_tbl[st->pin_gain];
> + *length = 2 * 2;
> + *type = IIO_VAL_INT_PLUS_NANO;
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> + return -EINVAL;

dead code.

> +}
> +
> +static int ad4000_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val, int val2,
> + long mask)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> + unsigned int reg_val;
> + bool span_comp_en;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + ret = ad4000_read_reg(st, &reg_val);
> + if (ret < 0)
> + return ret;
> +
> + span_comp_en = (val2 == st->scale_tbl[st->pin_gain][1][1]);
> + reg_val &= ~AD4000_SPAN_COMP;
> + reg_val |= FIELD_PREP(AD4000_SPAN_COMP, span_comp_en);
> +
> + ret = ad4000_write_reg(st, reg_val);
> + if (ret < 0)
> + return ret;
> +
> + st->span_comp = span_comp_en;
> + return 0;
> + }
> + unreachable();
> + default:
> + break;
> + }
> +
> + return -EINVAL;
Trivial but could push up into the default and make it clear there are no other ways out
of the switch statement.
> +}

2024-04-13 17:34:09

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000

On 4/13/24 11:14 AM, Jonathan Cameron wrote:
> On Tue, 9 Apr 2024 12:30:09 -0300
> Marcelo Schmitt <[email protected]> wrote:
>
>> On 04/08, David Lechner wrote:
>>> On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
>>> <[email protected]> wrote:
>>>>

..

>>>> +
>>>> + adi,spi-mode:
>>>> + $ref: /schemas/types.yaml#/definitions/string
>>>> + enum: [ single, chain ]
>>>
>>> It sounds like there are more possible wiring configurations for these
>>> chips that I thought when suggesting reusing this binding from AD7944
>>> so we probably need more options here. (see my reply to the cover
>>> letter for the complete context of these remarks)
>>>
>>> We identified A) an additional wiring configuration where SDI of the
>>> ADC chip is wired to SDO of the SPI controller and B) a potential need
>>> to pin mux between wiring modes to work around SPI controller
>>> limitations perhaps we could omit the adi,spi-mode property and just
>>> use the standard pinctrl properties.
>>>
>>> pinctrl-names:
>
> I'm lost on how pinctrl makes sense here.
> Yes you are changing the modes of the pins, but not in a conventional sense
> of some register that is being updated to say now use them like this.
> The mode is dependent on the timing sequence of how the pins are used.
> Otherwise looking at it a different way it's an external wiring thing we
> aren't controlling it at all. Is pinctrl suitable for that?
> I always thought of it as a way to change configurations of SoC pins.

Yes, this is exactly what I think we need here.

To write to the register, the chip has to be wired like this ("default"):

+-------------+
+-----------------------------------| SDO |
| | |
| +--------------------| CS |
| v | |
| +--------------------+ | HOST |
| | CNV | | |
+--->| SDI AD7944 SDO |-------->| SDI |
| SCK | | |
+--------------------+ | |
^ | |
+--------------------| SCLK |
+-------------+

But to read sample data, the chip has to be wired in one of these
3 configurations:

3-wire mode ("single"):

+-------------+
+--------------------| CS |
v | |
VIO +--------------------+ | HOST |
| | CNV | | |
+--->| SDI AD7944 SDO |-------->| SDI |
| SCK | | |
+--------------------+ | |
^ | |
+--------------------| SCLK |
+-------------+

4-wire mode ("multi"):
+-------------+
+-----------------------------------| CS |
| | |
| +--------------------| GPIO |
| v | |
| +--------------------+ | HOST |
| | CNV | | |
+--->| SDI AD7944 SDO |-------->| SDI |
| SCK | | |
+--------------------+ | |
^ | |
+--------------------| SCLK |
+-------------+

Chain mode ("chain"):

+-------------+
+--------------------| CS |
v | |
+--------------------+ | HOST |
| CNV | | |
+--->| SDI AD7944 SDO |-------->| SDI |
| | SCK | | |
GND +--------------------+ | |
^ | |
+--------------------| SCLK |
+-------------+

If we want to be able to both write the register and read data,
some reconfiguration is needed. It might be possible to read data
using the register-write wiring configuration, but that only
works if SDO can be set to the correct state *before* the
CS line changes. This is not something that I think most SPI
controllers can do (e.g. Marcelo mentioned in the cover letter
that RPi always returns SDO to low after every xfer while
the AXI SPI Engine leaves SDO wherever it was last).

>
> A pointer to some precendence in another driver for using it like this
> would go some way towards convincing me.
>
> Jonathan
>

I didn't find much precedence for something like this, but I
found devicetree/bindings/net/mediatek-bluetooth.txt that uses
pinctrl to pull a UART Rx pin low for a bootstrap mode which
sounds very similar to what we need to do here (pull the SPI
controller SDO pin high or low for 3-wire or chain mode).

For example, if we wanted to use 3-wire mode for reading
data, we would set the pinctrl to "default" to write the
register to configure the chip during driver probe. Then
to read data, we would change the pinctrl to "single" before
doing the SPI xfer to ensure that the ADC SDI pin is pulled
high independent of what the SDO line of the SPI controller
is currently doing.

2024-04-16 21:55:35

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000

So, I have been trying to make this work, though I haven't been successful so
far, and I don't really think using pinctrl is a good solution for this either.

Comments inline.

On 04/14, Jonathan Cameron wrote:
> On Sat, 13 Apr 2024 12:33:54 -0500
> David Lechner <[email protected]> wrote:
>
> > On 4/13/24 11:14 AM, Jonathan Cameron wrote:
> > > On Tue, 9 Apr 2024 12:30:09 -0300
> > > Marcelo Schmitt <[email protected]> wrote:
> > >
> > >> On 04/08, David Lechner wrote:
> > >>> On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
> > >>> <[email protected]> wrote:
> > >>>>
> >
> > ...
> >

..

> > >>>
> > >>> We identified A) an additional wiring configuration where SDI of the
> > >>> ADC chip is wired to SDO of the SPI controller and B) a potential need
> > >>> to pin mux between wiring modes to work around SPI controller
> > >>> limitations perhaps we could omit the adi,spi-mode property and just
> > >>> use the standard pinctrl properties.
> > >>>
> > >>> pinctrl-names:
> > >
> > > I'm lost on how pinctrl makes sense here.
> > > Yes you are changing the modes of the pins, but not in a conventional sense
> > > of some register that is being updated to say now use them like this.
> > > The mode is dependent on the timing sequence of how the pins are used.
> > > Otherwise looking at it a different way it's an external wiring thing we
> > > aren't controlling it at all. Is pinctrl suitable for that?
> > > I always thought of it as a way to change configurations of SoC pins.
> >
> > Yes, this is exactly what I think we need here.
> >
> > To write to the register, the chip has to be wired like this ("default"):
> >
> > +-------------+
> > +-----------------------------------| SDO |
> > | | |
> > | +--------------------| CS |
> > | v | |
> > | +--------------------+ | HOST |
> > | | CNV | | |
> > +--->| SDI AD7944 SDO |-------->| SDI |
> > | SCK | | |
> > +--------------------+ | |
> > ^ | |
> > +--------------------| SCLK |
> > +-------------+
> >
> > But to read sample data, the chip has to be wired in one of these
> > 3 configurations:
> >
> >
> > 3-wire mode ("single"):
> >
> > +-------------+
> > +--------------------| CS |
> > v | |
> > VIO +--------------------+ | HOST |
> > | | CNV | | |
> > +--->| SDI AD7944 SDO |-------->| SDI |
> > | SCK | | |
> > +--------------------+ | |
> > ^ | |
> > +--------------------| SCLK |
> > +-------------+
> >

3-wire mode like setup can be achieved if the SPI controller is capable of
keeping the ADC SDI line high (at VIO level) during ADC sampling, but keeping
controller SDO high throughout the entire transfer is not a thing all SPI
controllers can do (RPi's can't).
If the ADC is hard wired connected as in the 3-wire diagram then the user
can't write/read the configuration register. Same applies to "4-wire" mode
where controller CS is connected to ADC SDI.
The whole point of having pinctrl configurations was to make it possible
to both read/write config register and do ADC sampling if the SPI controller
can't keep ADC SDI at VIO. I don't think pinctrl can solve this problem though.

..

> >
> > If we want to be able to both write the register and read data,
> > some reconfiguration is needed. It might be possible to read data
> > using the register-write wiring configuration, but that only
> > works if SDO can be set to the correct state *before* the
> > CS line changes. This is not something that I think most SPI
> > controllers can do (e.g. Marcelo mentioned in the cover letter
> > that RPi always returns SDO to low after every xfer while
> > the AXI SPI Engine leaves SDO wherever it was last).
> >
> > >
> > > A pointer to some precendence in another driver for using it like this
> > > would go some way towards convincing me.
> > >
> > > Jonathan
> > >
> >
> >
> > I didn't find much precedence for something like this, but I
> > found devicetree/bindings/net/mediatek-bluetooth.txt that uses
> > pinctrl to pull a UART Rx pin low for a bootstrap mode which
> > sounds very similar to what we need to do here (pull the SPI
> > controller SDO pin high or low for 3-wire or chain mode).
> >

The pinctrl configuration for this ADC would not be meant to change once after
boot as it looks to be the usual use case for pinctrl (including mediatek-bluetooth.txt).

Also, no suitable mux for the "3-wire" mode in
Documentation/devicetree/bindings/pinctrl/xlnx,pinctrl-zynq.yaml
to do it like Documentation/devicetree/bindings/net/mediatek-bluetooth.txt.
The zynq pinctrl driver (drivers/pinctrl/pinctrl-zynq.c) would have to be
updated to add the new mux function in
static const struct zynq_pinmux_function zynq_pmux_functions[] = {
DEFINE_ZYNQ_PINMUX_FUNCTION(ethernet0, 1),
..
DEFINE_ZYNQ_PINMUX_FUNCTION(axi_spi_single, 1),
DEFINE_ZYNQ_PINMUX_FUNCTION(axi_spi_multi, 2),
though this is not really a thing that's on zynq, but one that is related to
these ADCs so I'm not sure it should go there.

> > For example, if we wanted to use 3-wire mode for reading
> > data, we would set the pinctrl to "default" to write the
> > register to configure the chip during driver probe. Then
> > to read data, we would change the pinctrl to "single" before
> > doing the SPI xfer to ensure that the ADC SDI pin is pulled
> > high independent of what the SDO line of the SPI controller
> > is currently doing.

No, the pin configuration for this ADCs would be expected to change unrestricted
amount of times. The pin configuration would have to change every time a sample
read is made after a register access transfers and vice-versa. If we decide
to support span compression, every change to _scale would lead to pinctrl
configuration change.

At pin level, we would want to rise SPI controller SDO line to VIO but then
the new SDO pin config would conflict with SPI pin group config.

I included pinctrl properties in my test dts to make use of pinctrl framework.
Yet, that doesn't really alternate SPI line configuration we are using because
the SPI function that is in the PS (FPGA's Processing System) is not what we are
interfacing when using spi-engine. Copy of my test dts at end of email.

Currently, the SPI controller we are using to test with these exotic ADCs
is the spi-engine which is implemented in the FPGA as an IP block which
owns control of the bus lines (SPI, SDO, CS, ...). To alternate the
configuration of SPI lines (pull SDO (ADC SDI) up to VIO, connect controller CS
to ADC SDI, etc.) I think it should be done in the HDL project. I don't think
it's a good idea to hijack spi-engine lines through pinctrl.

>
> Ah ok. This is implying that we are switching to a controllable
> mode to pull that pin high (or I guess one where it is always
> high). I'm not sure if that's more common than an SPI controller
> where you can set the 'don't' care state to high or low.
> I assume we can't get away with just setting the output buffer
> to all 1s because it won't hold that state between transfers?

I tried sending the tx buffer filled with 1s when testing this with a RPi4 but
it brought the controller SDO (ADC SDI) line low between each 8 bits of transfer
(attaching an image (yellow line is SCLK, green lines is controller SDO)).
Anyway, can we have any guaranties with respect to controller SDO line behaviour
when its not clocking out data?

>
> Feels like that could be rolled into the SPI subsystem with
> any necessary transitions handled there (maybe)

To me, this sounds more reasonable than pinctrl.
Yeah, if we can set a don't' care state for the SDO line then that should be
enough for these ADCs.
Otherwise, can we really do anything if the controller can't keep SDO high?

>
> Just to check, this isn't just a case of a missing pull up
> resistor to drag that line correctly when it isn't actively
> driven (combined with all 1s in the write buffer) etc?

When using spi-engine, the controller SDO is connected to ADC SDI, controller
CS to ADC CNV and, for reg access, it works as conventional SPI.
spi-engine leaves the SDO line the state it was after last transfer so it we
can make it idle high during sample read. No hardware pull-up needed.

Marcelo

>
> Jonathan
>
>

The device tree source file I was using for testing with the ADC with the
changes for using pinctrl. Didn't really work.

// SPDX-License-Identifier: GPL-2.0
/*
* Analog Devices ADAQ4003
* https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad400x
* https://wiki.analog.com/resources/eval/user-guides/ad400x
*
* hdl_project: <pulsar_adc_pmdz/zed>
* board_revision: <>
*
* Copyright (C) 2016-2023 Analog Devices Inc.
*/
/dts-v1/;

#include "zynq-zed.dtsi"
#include "zynq-zed-adv7511.dtsi"
#include <dt-bindings/pinctrl/pinctrl-zynq.h>

/ {
adc_vref: regulator-vref {
compatible = "regulator-fixed";
regulator-name = "EVAL 5V Vref";
regulator-min-microvolt = <5000000>;
regulator-max-microvolt = <5000000>;
regulator-always-on;
};

adc_vdd: regulator-vdd {
compatible = "regulator-fixed";
regulator-name = "Eval VDD supply";
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
regulator-always-on;
};

adc_vio: regulator-vio {
compatible = "regulator-fixed";
regulator-name = "Eval VIO supply";
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
regulator-always-on;
};
};

&pinctrl0 {
/* Restore conventional SPI pin configuration */
pinctrl_spi_default: default_config {
mux {
/* Are these the ones used by spi-engine? */
groups = "spi0_0_grp";
function = "spi0";
};
conf {
groups = "spi0_0_grp";
power-source = <IO_STANDARD_LVCMOS33>;
};
conf-spi-sdo {
pins = "MIO17"; /* SPI0 SDO? */
bias-disable;
};
};

/* Pull-up SPI SDO (ADC SDI) to VIO */
pinctrl_spi_single: single_config {
conf-spi-sdo {
pins = "MIO17"; /* Conflicts with SPI0 pin group */
bias-pull-up;
};
};
};

&fpga_axi {
rx_dma: rx-dmac@44a30000 {
compatible = "adi,axi-dmac-1.00.a";
reg = <0x44a30000 0x1000>;
#dma-cells = <1>;
interrupts = <0 57 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&clkc 17>;

adi,channels {
#size-cells = <0>;
#address-cells = <1>;

dma-channel@0 {
reg = <0>;
adi,source-bus-width = <32>;
adi,source-bus-type = <1>;
adi,destination-bus-width = <64>;
adi,destination-bus-type = <0>;
};
};
};

axi_pwm_gen: axi-pwm-gen@ {
compatible = "adi,axi-pwmgen";
reg = <0x44b00000 0x1000>;
label = "cnv";
#pwm-cells = <2>;
clocks = <&spi_clk>;
};

spi_clk: axi-clkgen@44a70000 {
compatible = "adi,axi-clkgen-2.00.a";
reg = <0x44a70000 0x10000>;
#clock-cells = <0>;
clocks = <&clkc 15>, <&clkc 15>;
clock-names = "s_axi_aclk", "clkin1";
clock-output-names = "spi_clk";
};

axi_spi_engine_0: spi@44a00000 {
compatible = "adi,axi-spi-engine-1.00.a";
reg = <0x44a00000 0x1000>;
interrupt-parent = <&intc>;
interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&clkc 15 &spi_clk>;
clock-names = "s_axi_aclk", "spi_clk";
num-cs = <1>;

#address-cells = <0x1>;
#size-cells = <0x0>;

adaq4003: adc@0 {
compatible = "adi,adaq4003";
reg = <0>;
spi-max-frequency = <102000000>;
spi-cpha;
pinctrl-names = "default", "single";
pinctrl-0 = <&pinctrl_spi_default>;
pinctrl-1 = <&pinctrl_spi_single>;
vdd-supply = <&adc_vdd>;
vio-supply = <&adc_vio>;
ref-supply = <&adc_vref>;
adi,high-z-input;
adi,gain-milli = <454>;
};
};
};

Attachments:

(No filename) (12.34 kB)
dso_01_01_00_34_00.jpg (106.18 kB)
Download all attachments

2024-04-20 14:17:50

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000

On Tue, 16 Apr 2024 18:46:11 -0300
Marcelo Schmitt <[email protected]> wrote:

> So, I have been trying to make this work, though I haven't been successful so
> far, and I don't really think using pinctrl is a good solution for this either.
>
> Comments inline.
>
> On 04/14, Jonathan Cameron wrote:
> > On Sat, 13 Apr 2024 12:33:54 -0500
> > David Lechner <[email protected]> wrote:
> >
> > > On 4/13/24 11:14 AM, Jonathan Cameron wrote:
> > > > On Tue, 9 Apr 2024 12:30:09 -0300
> > > > Marcelo Schmitt <[email protected]> wrote:
> > > >
> > > >> On 04/08, David Lechner wrote:
> > > >>> On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
> > > >>> <[email protected]> wrote:
> > > >>>>
> > >
> > > ...
> > >
>
> ...
>
> > > >>>
> > > >>> We identified A) an additional wiring configuration where SDI of the
> > > >>> ADC chip is wired to SDO of the SPI controller and B) a potential need
> > > >>> to pin mux between wiring modes to work around SPI controller
> > > >>> limitations perhaps we could omit the adi,spi-mode property and just
> > > >>> use the standard pinctrl properties.
> > > >>>
> > > >>> pinctrl-names:
> > > >
> > > > I'm lost on how pinctrl makes sense here.
> > > > Yes you are changing the modes of the pins, but not in a conventional sense
> > > > of some register that is being updated to say now use them like this.
> > > > The mode is dependent on the timing sequence of how the pins are used.
> > > > Otherwise looking at it a different way it's an external wiring thing we
> > > > aren't controlling it at all. Is pinctrl suitable for that?
> > > > I always thought of it as a way to change configurations of SoC pins.
> > >
> > > Yes, this is exactly what I think we need here.
> > >
> > > To write to the register, the chip has to be wired like this ("default"):
> > >
> > > +-------------+
> > > +-----------------------------------| SDO |
> > > | | |
> > > | +--------------------| CS |
> > > | v | |
> > > | +--------------------+ | HOST |
> > > | | CNV | | |
> > > +--->| SDI AD7944 SDO |-------->| SDI |
> > > | SCK | | |
> > > +--------------------+ | |
> > > ^ | |
> > > +--------------------| SCLK |
> > > +-------------+
> > >
> > > But to read sample data, the chip has to be wired in one of these
> > > 3 configurations:
> > >
> > >
> > > 3-wire mode ("single"):
> > >
> > > +-------------+
> > > +--------------------| CS |
> > > v | |
> > > VIO +--------------------+ | HOST |
> > > | | CNV | | |
> > > +--->| SDI AD7944 SDO |-------->| SDI |
> > > | SCK | | |
> > > +--------------------+ | |
> > > ^ | |
> > > +--------------------| SCLK |
> > > +-------------+
> > >
>
> 3-wire mode like setup can be achieved if the SPI controller is capable of
> keeping the ADC SDI line high (at VIO level) during ADC sampling, but keeping
> controller SDO high throughout the entire transfer is not a thing all SPI
> controllers can do (RPi's can't).
> If the ADC is hard wired connected as in the 3-wire diagram then the user
> can't write/read the configuration register. Same applies to "4-wire" mode
> where controller CS is connected to ADC SDI.
> The whole point of having pinctrl configurations was to make it possible
> to both read/write config register and do ADC sampling if the SPI controller
> can't keep ADC SDI at VIO. I don't think pinctrl can solve this problem though.
>
> ...
>
> > >
> > > If we want to be able to both write the register and read data,
> > > some reconfiguration is needed. It might be possible to read data
> > > using the register-write wiring configuration, but that only
> > > works if SDO can be set to the correct state *before* the
> > > CS line changes. This is not something that I think most SPI
> > > controllers can do (e.g. Marcelo mentioned in the cover letter
> > > that RPi always returns SDO to low after every xfer while
> > > the AXI SPI Engine leaves SDO wherever it was last).
> > >
> > > >
> > > > A pointer to some precendence in another driver for using it like this
> > > > would go some way towards convincing me.
> > > >
> > > > Jonathan
> > > >
> > >
> > >
> > > I didn't find much precedence for something like this, but I
> > > found devicetree/bindings/net/mediatek-bluetooth.txt that uses
> > > pinctrl to pull a UART Rx pin low for a bootstrap mode which
> > > sounds very similar to what we need to do here (pull the SPI
> > > controller SDO pin high or low for 3-wire or chain mode).
> > >
>
> The pinctrl configuration for this ADC would not be meant to change once after
> boot as it looks to be the usual use case for pinctrl (including mediatek-bluetooth.txt).
>
> Also, no suitable mux for the "3-wire" mode in
> Documentation/devicetree/bindings/pinctrl/xlnx,pinctrl-zynq.yaml
> to do it like Documentation/devicetree/bindings/net/mediatek-bluetooth.txt.
> The zynq pinctrl driver (drivers/pinctrl/pinctrl-zynq.c) would have to be
> updated to add the new mux function in
> static const struct zynq_pinmux_function zynq_pmux_functions[] = {
> DEFINE_ZYNQ_PINMUX_FUNCTION(ethernet0, 1),
> ...
> DEFINE_ZYNQ_PINMUX_FUNCTION(axi_spi_single, 1),
> DEFINE_ZYNQ_PINMUX_FUNCTION(axi_spi_multi, 2),
> though this is not really a thing that's on zynq, but one that is related to
> these ADCs so I'm not sure it should go there.

I'd argue we are after a specific SPI controller setup for this.
A controller driver would need modifying to make it work.

>
> > > For example, if we wanted to use 3-wire mode for reading
> > > data, we would set the pinctrl to "default" to write the
> > > register to configure the chip during driver probe. Then
> > > to read data, we would change the pinctrl to "single" before
> > > doing the SPI xfer to ensure that the ADC SDI pin is pulled
> > > high independent of what the SDO line of the SPI controller
> > > is currently doing.
>
> No, the pin configuration for this ADCs would be expected to change unrestricted
> amount of times. The pin configuration would have to change every time a sample
> read is made after a register access transfers and vice-versa. If we decide
> to support span compression, every change to _scale would lead to pinctrl
> configuration change.
>
> At pin level, we would want to rise SPI controller SDO line to VIO but then
> the new SDO pin config would conflict with SPI pin group config.
>
> I included pinctrl properties in my test dts to make use of pinctrl framework.
> Yet, that doesn't really alternate SPI line configuration we are using because
> the SPI function that is in the PS (FPGA's Processing System) is not what we are
> interfacing when using spi-engine. Copy of my test dts at end of email.
>
> Currently, the SPI controller we are using to test with these exotic ADCs
> is the spi-engine which is implemented in the FPGA as an IP block which
> owns control of the bus lines (SPI, SDO, CS, ...). To alternate the
> configuration of SPI lines (pull SDO (ADC SDI) up to VIO, connect controller CS
> to ADC SDI, etc.) I think it should be done in the HDL project. I don't think
> it's a good idea to hijack spi-engine lines through pinctrl.

Such functionality would need to be pushed to the spi controller driver
which could know if there was any need to do anything like this, or if there
was simply a register to set.

>
> >
> > Ah ok. This is implying that we are switching to a controllable
> > mode to pull that pin high (or I guess one where it is always
> > high). I'm not sure if that's more common than an SPI controller
> > where you can set the 'don't' care state to high or low.
> > I assume we can't get away with just setting the output buffer
> > to all 1s because it won't hold that state between transfers?
>
> I tried sending the tx buffer filled with 1s when testing this with a RPi4 but
> it brought the controller SDO (ADC SDI) line low between each 8 bits of transfer
> (attaching an image (yellow line is SCLK, green lines is controller SDO)).

Pity - thought that was overly optimistic.

> Anyway, can we have any guaranties with respect to controller SDO line behaviour
> when its not clocking out data?

>
> >
> > Feels like that could be rolled into the SPI subsystem with
> > any necessary transitions handled there (maybe)
>
> To me, this sounds more reasonable than pinctrl.
> Yeah, if we can set a don't' care state for the SDO line then that should be
> enough for these ADCs.
> Otherwise, can we really do anything if the controller can't keep SDO high?

There is one similar (ish) entry already.
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/spi/spi.h#L29
#define SPI_3WIRE_HIZ _BITUL(15) /* high impedance turnaround */
in that it is controlling state in what I think would normally be a don't care state.

I think we could have an
SPI_SDO_DONT_CARE_HIGH (naming to be improved upon ;)
that a driver could advertise support for and the spi device could request.

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-tpo-tpg110.c#L429

Implement that in the spi-gpio driver as a PoC probably and in your SPI copntoller
driver.

Ultimately if the controller really isn't capable (including dances through pin
mode changes if necessary) then the ADC won't work in this wiring with that host
controller.

I'd propose something along these lines and see whether Mark + any other active
SPI folk think it is reasonable or not?

>
> >
> > Just to check, this isn't just a case of a missing pull up
> > resistor to drag that line correctly when it isn't actively
> > driven (combined with all 1s in the write buffer) etc?
>
> When using spi-engine, the controller SDO is connected to ADC SDI, controller
> CS to ADC CNV and, for reg access, it works as conventional SPI.
> spi-engine leaves the SDO line the state it was after last transfer so it we
> can make it idle high during sample read. No hardware pull-up needed.

Fair enough. No multi master support I guess (that is a bit obscure for
SPI). A little ugly that it's dependent on the last access - so you would need
to do a dummy access if the normal last bit was wrong level?

Jonathan

>
> Marcelo
>
> >
> > Jonathan
> >
> >
>
> The device tree source file I was using for testing with the ADC with the
> changes for using pinctrl. Didn't really work.
>
> // SPDX-License-Identifier: GPL-2.0
> /*
> * Analog Devices ADAQ4003
> * https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad400x
> * https://wiki.analog.com/resources/eval/user-guides/ad400x
> *
> * hdl_project: <pulsar_adc_pmdz/zed>
> * board_revision: <>
> *
> * Copyright (C) 2016-2023 Analog Devices Inc.
> */
> /dts-v1/;
>
> #include "zynq-zed.dtsi"
> #include "zynq-zed-adv7511.dtsi"
> #include <dt-bindings/pinctrl/pinctrl-zynq.h>
>
> / {
> adc_vref: regulator-vref {
> compatible = "regulator-fixed";
> regulator-name = "EVAL 5V Vref";
> regulator-min-microvolt = <5000000>;
> regulator-max-microvolt = <5000000>;
> regulator-always-on;
> };
>
> adc_vdd: regulator-vdd {
> compatible = "regulator-fixed";
> regulator-name = "Eval VDD supply";
> regulator-min-microvolt = <1800000>;
> regulator-max-microvolt = <1800000>;
> regulator-always-on;
> };
>
> adc_vio: regulator-vio {
> compatible = "regulator-fixed";
> regulator-name = "Eval VIO supply";
> regulator-min-microvolt = <3300000>;
> regulator-max-microvolt = <3300000>;
> regulator-always-on;
> };
> };
>
> &pinctrl0 {
> /* Restore conventional SPI pin configuration */
> pinctrl_spi_default: default_config {
> mux {
> /* Are these the ones used by spi-engine? */
> groups = "spi0_0_grp";
> function = "spi0";
> };
> conf {
> groups = "spi0_0_grp";
> power-source = <IO_STANDARD_LVCMOS33>;
> };
> conf-spi-sdo {
> pins = "MIO17"; /* SPI0 SDO? */
> bias-disable;
> };
> };
>
> /* Pull-up SPI SDO (ADC SDI) to VIO */
> pinctrl_spi_single: single_config {
> conf-spi-sdo {
> pins = "MIO17"; /* Conflicts with SPI0 pin group */
> bias-pull-up;
> };
> };
> };
>
> &fpga_axi {
> rx_dma: rx-dmac@44a30000 {
> compatible = "adi,axi-dmac-1.00.a";
> reg = <0x44a30000 0x1000>;
> #dma-cells = <1>;
> interrupts = <0 57 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&clkc 17>;
>
> adi,channels {
> #size-cells = <0>;
> #address-cells = <1>;
>
> dma-channel@0 {
> reg = <0>;
> adi,source-bus-width = <32>;
> adi,source-bus-type = <1>;
> adi,destination-bus-width = <64>;
> adi,destination-bus-type = <0>;
> };
> };
> };
>
> axi_pwm_gen: axi-pwm-gen@ {
> compatible = "adi,axi-pwmgen";
> reg = <0x44b00000 0x1000>;
> label = "cnv";
> #pwm-cells = <2>;
> clocks = <&spi_clk>;
> };
>
> spi_clk: axi-clkgen@44a70000 {
> compatible = "adi,axi-clkgen-2.00.a";
> reg = <0x44a70000 0x10000>;
> #clock-cells = <0>;
> clocks = <&clkc 15>, <&clkc 15>;
> clock-names = "s_axi_aclk", "clkin1";
> clock-output-names = "spi_clk";
> };
>
> axi_spi_engine_0: spi@44a00000 {
> compatible = "adi,axi-spi-engine-1.00.a";
> reg = <0x44a00000 0x1000>;
> interrupt-parent = <&intc>;
> interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&clkc 15 &spi_clk>;
> clock-names = "s_axi_aclk", "spi_clk";
> num-cs = <1>;
>
> #address-cells = <0x1>;
> #size-cells = <0x0>;
>
> adaq4003: adc@0 {
> compatible = "adi,adaq4003";
> reg = <0>;
> spi-max-frequency = <102000000>;
> spi-cpha;
> pinctrl-names = "default", "single";
> pinctrl-0 = <&pinctrl_spi_default>;
> pinctrl-1 = <&pinctrl_spi_single>;
> vdd-supply = <&adc_vdd>;
> vio-supply = <&adc_vio>;
> ref-supply = <&adc_vref>;
> adi,high-z-input;
> adi,gain-milli = <454>;
> };
> };
> };

2024-04-21 22:38:06